| Message ID | 20191121185049.16666-8-liuwe@microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Port Xen to Hyper-V | expand |
> -----Original Message----- > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Wei > Liu > Sent: 21 November 2019 19:51 > To: Xen Development List <xen-devel@lists.xenproject.org> > Cc: Wei Liu <liuwe@microsoft.com>; Wei Liu <wl@xen.org>; Andrew Cooper > <andrew.cooper3@citrix.com>; Michael Kelley <mikelley@microsoft.com>; Jan > Beulich <jbeulich@suse.com>; Roger Pau Monné <roger.pau@citrix.com> > Subject: [Xen-devel] [PATCH v4 7/8] x86: be more verbose when running on a > hypervisor > > Also replace xen_guest with running_on_hypervisor boolean. > > Signed-off-by: Wei Liu <liuwe@microsoft.com> > --- > Changes in v4: > 1. Access ->name directly. > 2. Drop Roger's review tag. > --- > xen/arch/x86/setup.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index 19606d909b..123436b35a 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -689,6 +689,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) > int i, j, e820_warn = 0, bytes = 0; > bool acpi_boot_table_init_done = false, relocated = false; > int ret; > + bool running_on_hypervisor; Why not stash hops here? Then you can... > struct ns16550_defaults ns16550 = { > .data_bits = 8, > .parity = 'n', > @@ -763,7 +764,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) > * allocing any xenheap structures wanted in lower memory. */ > kexec_early_calculations(); > > - hypervisor_probe(); > + running_on_hypervisor = !!hypervisor_probe(); > > parse_video_info(); > > @@ -788,6 +789,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) > printk("Command line: %s\n", cmdline); > > printk("Xen image load base address: %#lx\n", xen_phys_start); > + if ( running_on_hypervisor ) > + printk("Running on %s\n", hypervisor_probe()->name); ...avoid calling hypervisor_probe() again here. Paul > > #ifdef CONFIG_VIDEO > printk("Video information:\n"); > @@ -1569,7 +1572,7 @@ void __init noreturn __start_xen(unsigned long > mbi_p) > max_cpus = nr_cpu_ids; > } > > - if ( xen_guest ) > + if ( running_on_hypervisor ) > hypervisor_setup(); > > /* Low mappings were only needed for some BIOS table parsing. */ > -- > 2.20.1 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel
On 21.11.2019 19:50, Wei Liu wrote: > @@ -763,7 +764,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) > * allocing any xenheap structures wanted in lower memory. */ > kexec_early_calculations(); > > - hypervisor_probe(); > + running_on_hypervisor = !!hypervisor_probe(); No need for !! I think? > @@ -788,6 +789,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) > printk("Command line: %s\n", cmdline); > > printk("Xen image load base address: %#lx\n", xen_phys_start); > + if ( running_on_hypervisor ) > + printk("Running on %s\n", hypervisor_probe()->name); Invoking hypervisor_probe() twice seems odd to me. I realize the function copes, but why do everything a 2nd time when not running on any hypervisor? Furthermore if this use is the only reason why struct hypervisor_ops can't be local to xen/arch/x86/guest/hypervisor.c, then I think once again that the name should be return from hypervisor_probe(). The initial if() in there then could also go away. Jan
On 29.11.2019 15:31, Jan Beulich wrote: > On 21.11.2019 19:50, Wei Liu wrote: >> @@ -763,7 +764,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) >> * allocing any xenheap structures wanted in lower memory. */ >> kexec_early_calculations(); >> >> - hypervisor_probe(); >> + running_on_hypervisor = !!hypervisor_probe(); > > No need for !! I think? > >> @@ -788,6 +789,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) >> printk("Command line: %s\n", cmdline); >> >> printk("Xen image load base address: %#lx\n", xen_phys_start); >> + if ( running_on_hypervisor ) >> + printk("Running on %s\n", hypervisor_probe()->name); > > Invoking hypervisor_probe() twice seems odd to me. I realize > the function copes, but why do everything a 2nd time when > not running on any hypervisor? Furthermore if this use is > the only reason why struct hypervisor_ops can't be local to > xen/arch/x86/guest/hypervisor.c, then I think once again > that the name should be return from hypervisor_probe(). "Local to xen/arch/x86/guest/hypervisor.c" was rubbish of course; local to xen/arch/x86/guest/ would be more correct. Jan
On 21/11/2019 18:50, Wei Liu wrote: > Also replace xen_guest with running_on_hypervisor boolean. I agree with dropping xen_guest, but... > > Signed-off-by: Wei Liu <liuwe@microsoft.com> > --- > Changes in v4: > 1. Access ->name directly. > 2. Drop Roger's review tag. > --- > xen/arch/x86/setup.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > index 19606d909b..123436b35a 100644 > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -689,6 +689,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) > int i, j, e820_warn = 0, bytes = 0; > bool acpi_boot_table_init_done = false, relocated = false; > int ret; > + bool running_on_hypervisor; ... this is semantically ambiguous with cpu_has_hypervisor. Where they differ is whether Xen has managed to recognise the hypervisor it is running under, or not. Given that the hypervisor_*() hooks are nops by default, I'd suggest just making blind calls. ~Andrew
On Fri, Nov 29, 2019 at 06:15:52PM +0000, Andrew Cooper wrote: > On 21/11/2019 18:50, Wei Liu wrote: > > Also replace xen_guest with running_on_hypervisor boolean. > > I agree with dropping xen_guest, but... > > > > > Signed-off-by: Wei Liu <liuwe@microsoft.com> > > --- > > Changes in v4: > > 1. Access ->name directly. > > 2. Drop Roger's review tag. > > --- > > xen/arch/x86/setup.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c > > index 19606d909b..123436b35a 100644 > > --- a/xen/arch/x86/setup.c > > +++ b/xen/arch/x86/setup.c > > @@ -689,6 +689,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) > > int i, j, e820_warn = 0, bytes = 0; > > bool acpi_boot_table_init_done = false, relocated = false; > > int ret; > > + bool running_on_hypervisor; > > ... this is semantically ambiguous with cpu_has_hypervisor. > > Where they differ is whether Xen has managed to recognise the hypervisor > it is running under, or not. > > Given that the hypervisor_*() hooks are nops by default, I'd suggest > just making blind calls. Well Jan asked to drop the hypervisor_name hook. I can't make blind calls here. He's unhappy with calling hypervisor_probe twice either. I can, however, do the following: 1. Change hypervisor_probe to return NULL or a string 2. Cache and use that return value inside this function This should make both of you happy. Wei. > > ~Andrew
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 19606d909b..123436b35a 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -689,6 +689,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) int i, j, e820_warn = 0, bytes = 0; bool acpi_boot_table_init_done = false, relocated = false; int ret; + bool running_on_hypervisor; struct ns16550_defaults ns16550 = { .data_bits = 8, .parity = 'n', @@ -763,7 +764,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) * allocing any xenheap structures wanted in lower memory. */ kexec_early_calculations(); - hypervisor_probe(); + running_on_hypervisor = !!hypervisor_probe(); parse_video_info(); @@ -788,6 +789,8 @@ void __init noreturn __start_xen(unsigned long mbi_p) printk("Command line: %s\n", cmdline); printk("Xen image load base address: %#lx\n", xen_phys_start); + if ( running_on_hypervisor ) + printk("Running on %s\n", hypervisor_probe()->name); #ifdef CONFIG_VIDEO printk("Video information:\n"); @@ -1569,7 +1572,7 @@ void __init noreturn __start_xen(unsigned long mbi_p) max_cpus = nr_cpu_ids; } - if ( xen_guest ) + if ( running_on_hypervisor ) hypervisor_setup(); /* Low mappings were only needed for some BIOS table parsing. */
Also replace xen_guest with running_on_hypervisor boolean. Signed-off-by: Wei Liu <liuwe@microsoft.com> --- Changes in v4: 1. Access ->name directly. 2. Drop Roger's review tag. --- xen/arch/x86/setup.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)