| Message ID | 1562490908-17882-5-git-send-email-zhenzhong.duan@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | misc fixes to PV extensions code | expand |
On 7/7/19 5:15 AM, Zhenzhong Duan wrote: > > +static uint32_t __init xen_platform_hvm(void) > +{ > + if (xen_pv_domain()) > + return 0; > + > + if (xen_pvh_domain() && nopv) { > + /* Guest booting via the Xen-PVH boot entry goes here */ > + pr_info("\"nopv\" parameter is ignored in PVH guest\n"); > + nopv = false; > + } else if (nopv) { > + /* > + * Guest booting via normal boot entry (like via grub2) goes > + * here. > + */ > + x86_init.hyper.guest_late_init = xen_hvm_guest_late_init; > + return 0; After staring at this some more I don't think we can do this. Hypervisor-specific code should not muck with with x86_init.hyper, it's layering violation. That's what init_hypervisor_platform() is for. So we may have to create another hypervisor_x86 ops structure for Xen nopv (which I don't find very appealing TBH). Or update x86_hyper_xen_hvm and return xen_cpuid_base() instead of zero (but then you'd need to update all these structs from __initconst to _init or some such). Or something else. -boris > + } > + return xen_cpuid_base(); > +} > + > const __initconst struct hypervisor_x86 x86_hyper_xen_hvm = { > .name = "Xen HVM", > .detect = xen_platform_hvm, > @@ -268,4 +283,5 @@ static __init void xen_hvm_guest_late_init(void) > .init.init_mem_mapping = xen_hvm_init_mem_mapping, > .init.guest_late_init = xen_hvm_guest_late_init, > .runtime.pin_vcpu = xen_pin_vcpu, > + .ignore_nopv = true, > };
On 2019/7/8 21:46, Boris Ostrovsky wrote: > On 7/7/19 5:15 AM, Zhenzhong Duan wrote: >> >> +static uint32_t __init xen_platform_hvm(void) >> +{ >> + if (xen_pv_domain()) >> + return 0; >> + >> + if (xen_pvh_domain() && nopv) { >> + /* Guest booting via the Xen-PVH boot entry goes here */ >> + pr_info("\"nopv\" parameter is ignored in PVH guest\n"); >> + nopv = false; >> + } else if (nopv) { >> + /* >> + * Guest booting via normal boot entry (like via grub2) goes >> + * here. >> + */ >> + x86_init.hyper.guest_late_init = xen_hvm_guest_late_init; >> + return 0; > After staring at this some more I don't think we can do this. > Hypervisor-specific code should not muck with with x86_init.hyper, it's > layering violation. That's what init_hypervisor_platform() is for. Ok, I see. > > So we may have to create another hypervisor_x86 ops structure for Xen > nopv (which I don't find very appealing TBH). Or update > x86_hyper_xen_hvm and return xen_cpuid_base() instead of zero (but then > you'd need to update all these structs from __initconst to _init or some > such). Or something else. I choose the second. I made below changes, not clear if it's also a layering violation as use x86_init.hyper as source for memcpy. I choose memcpy instead of updating functions pointers one-by-one because if x86_hyper_init interface functions extends in the future we need no changes. Let me know if you prefer updating pointers directly. This isn't a formal patch for review, just want to get answer of above question. diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c index 1756cf7..8416640d 100644 --- a/arch/x86/xen/enlighten_hvm.c +++ b/arch/x86/xen/enlighten_hvm.c @@ -231,14 +231,6 @@ bool __init xen_hvm_need_lapic(void) return true; } -static uint32_t __init xen_platform_hvm(void) -{ - if (xen_pv_domain()) - return 0; - - return xen_cpuid_base(); -} - static __init void xen_hvm_guest_late_init(void) { #ifdef CONFIG_XEN_PVH @@ -250,6 +242,9 @@ static __init void xen_hvm_guest_late_init(void) /* PVH detected. */ xen_pvh = true; + if (nopv) + panic("\"nopv\" and \"xen_nopv\" parameters are unsupported in PVH guest."); + /* Make sure we don't fall back to (default) ACPI_IRQ_MODEL_PIC. */ if (!nr_ioapics && acpi_irq_model == ACPI_IRQ_MODEL_PIC) acpi_irq_model = ACPI_IRQ_MODEL_PLATFORM; @@ -259,7 +254,36 @@ static __init void xen_hvm_guest_late_init(void) #endif } -const __initconst struct hypervisor_x86 x86_hyper_xen_hvm = { +static uint32_t __init xen_platform_hvm(void) +{ + uint32_t xen_domain = xen_cpuid_base(); + struct x86_hyper_init *h = &x86_hyper_xen_hvm.init; + + if (xen_pv_domain()) + return 0; + + if (xen_pvh_domain() && nopv) { + /* Guest booting via the Xen-PVH boot entry goes here */ + pr_info("\"nopv\" parameter is ignored in PVH guest\n"); + nopv = false; + } else if (nopv && xen_domain) { + /* + * Guest booting via normal boot entry (like via grub2) goes + * here. + * + * Use interface functions for bare hardware if nopv, + * xen_hvm_guest_late_init is an exception as we need to + * detect PVH and panic there. + */ + memcpy(h, (void *)&x86_init.hyper, sizeof(x86_init.hyper)); + memcpy(&x86_hyper_xen_hvm.runtime, (void *)&x86_platform.hyper, + sizeof(x86_platform.hyper)); + h->guest_late_init = xen_hvm_guest_late_init; + } + return xen_domain; +} + +struct hypervisor_x86 x86_hyper_xen_hvm __initdata = { .name = "Xen HVM", .detect = xen_platform_hvm, .type = X86_HYPER_XEN_HVM, @@ -268,4 +292,5 @@ static __init void xen_hvm_guest_late_init(void) .init.init_mem_mapping = xen_hvm_init_mem_mapping, .init.guest_late_init = xen_hvm_guest_late_init, .runtime.pin_vcpu = xen_pin_vcpu, + .ignore_nopv = true, }; Thanks Zhenzhong
On 7/9/19 12:20 AM, Zhenzhong Duan wrote: > > -const __initconst struct hypervisor_x86 x86_hyper_xen_hvm = { > +static uint32_t __init xen_platform_hvm(void) > +{ > + uint32_t xen_domain = xen_cpuid_base(); > + struct x86_hyper_init *h = &x86_hyper_xen_hvm.init; > + > + if (xen_pv_domain()) > + return 0; > + > + if (xen_pvh_domain() && nopv) { > + /* Guest booting via the Xen-PVH boot entry goes here */ > + pr_info("\"nopv\" parameter is ignored in PVH guest\n"); > + nopv = false; > + } else if (nopv && xen_domain) { > + /* > + * Guest booting via normal boot entry (like via > grub2) goes > + * here. > + * > + * Use interface functions for bare hardware if nopv, > + * xen_hvm_guest_late_init is an exception as we need to > + * detect PVH and panic there. > + */ > + memcpy(h, (void *)&x86_init.hyper, > sizeof(x86_init.hyper)); And this worked? I'd think it would fail since h points to RO section. > + memcpy(&x86_hyper_xen_hvm.runtime, (void > *)&x86_platform.hyper, > + sizeof(x86_platform.hyper)); > + h->guest_late_init = xen_hvm_guest_late_init; To me this still doesn't look right --- you are making assumptions about x86_platform/x86_init.hyper and I don't think you can assume they have not been set to point to another hypervisor, for example. Would modifying all x86_hyper_xen_hvm's ops (except, I guess, xen_hvm_guest_late_init()) to immediately return if nopv is set work? Also question for Juergen --- is there anything perhaps in zeropage that indicates we are booting PVH guest from grub? -boris > + } > + return xen_domain; > +} > + > +struct hypervisor_x86 x86_hyper_xen_hvm __initdata = { > .name = "Xen HVM", > .detect = xen_platform_hvm, > .type = X86_HYPER_XEN_HVM, > @@ -268,4 +292,5 @@ static __init void xen_hvm_guest_late_init(void) > .init.init_mem_mapping = xen_hvm_init_mem_mapping, > .init.guest_late_init = xen_hvm_guest_late_init, > .runtime.pin_vcpu = xen_pin_vcpu, > + .ignore_nopv = true, > }; > > > Thanks > > Zhenzhong >
On 2019/7/9 22:54, Boris Ostrovsky wrote: > On 7/9/19 12:20 AM, Zhenzhong Duan wrote: >> -const __initconst struct hypervisor_x86 x86_hyper_xen_hvm = { >> +static uint32_t __init xen_platform_hvm(void) >> +{ >> + uint32_t xen_domain = xen_cpuid_base(); >> + struct x86_hyper_init *h = &x86_hyper_xen_hvm.init; >> + >> + if (xen_pv_domain()) >> + return 0; >> + >> + if (xen_pvh_domain() && nopv) { >> + /* Guest booting via the Xen-PVH boot entry goes here */ >> + pr_info("\"nopv\" parameter is ignored in PVH guest\n"); >> + nopv = false; >> + } else if (nopv && xen_domain) { >> + /* >> + * Guest booting via normal boot entry (like via >> grub2) goes >> + * here. >> + * >> + * Use interface functions for bare hardware if nopv, >> + * xen_hvm_guest_late_init is an exception as we need to >> + * detect PVH and panic there. >> + */ >> + memcpy(h, (void *)&x86_init.hyper, >> sizeof(x86_init.hyper)); > > And this worked? I'd think it would fail since h points to RO section. Yes, I have below changes in the patch. -const __initconst struct hypervisor_x86 x86_hyper_xen_hvm = { +struct hypervisor_x86 x86_hyper_xen_hvm __initdata = { > > >> + memcpy(&x86_hyper_xen_hvm.runtime, (void >> *)&x86_platform.hyper, >> + sizeof(x86_platform.hyper)); >> + h->guest_late_init = xen_hvm_guest_late_init; > > To me this still doesn't look right --- you are making assumptions about > x86_platform/x86_init.hyper and I don't think you can assume they have > not been set to point to another hypervisor, for example. You mean copy_array() calls in init_hypervisor_platform()? But that happens after detect_hypervisor_vendor() shoose out the prefered hypervisor. In detect stage, x86_platform/x86_init.hyper has default value for bare hardware, or I missed something? Just realized I can use memset to zero instead of memcpy which looks more rational. > > Would modifying all x86_hyper_xen_hvm's ops (except, I guess, > xen_hvm_guest_late_init()) to immediately return if nopv is set work? I think so, Let me try it. Zhenzhong
On 7/9/19 10:07 PM, Zhenzhong Duan wrote: > On 2019/7/9 22:54, Boris Ostrovsky wrote: >> On 7/9/19 12:20 AM, Zhenzhong Duan wrote: >>> -const __initconst struct hypervisor_x86 x86_hyper_xen_hvm = { >>> +static uint32_t __init xen_platform_hvm(void) >>> +{ >>> + uint32_t xen_domain = xen_cpuid_base(); >>> + struct x86_hyper_init *h = &x86_hyper_xen_hvm.init; >>> + >>> + if (xen_pv_domain()) >>> + return 0; >>> + >>> + if (xen_pvh_domain() && nopv) { >>> + /* Guest booting via the Xen-PVH boot entry goes >>> here */ >>> + pr_info("\"nopv\" parameter is ignored in PVH >>> guest\n"); >>> + nopv = false; >>> + } else if (nopv && xen_domain) { >>> + /* >>> + * Guest booting via normal boot entry (like via >>> grub2) goes >>> + * here. >>> + * >>> + * Use interface functions for bare hardware if nopv, >>> + * xen_hvm_guest_late_init is an exception as we >>> need to >>> + * detect PVH and panic there. >>> + */ >>> + memcpy(h, (void *)&x86_init.hyper, >>> sizeof(x86_init.hyper)); >> >> And this worked? I'd think it would fail since h points to RO section. > Yes, I have below changes in the patch. > > -const __initconst struct hypervisor_x86 x86_hyper_xen_hvm = { > +struct hypervisor_x86 x86_hyper_xen_hvm __initdata = { But hypervisors[] stays__initconst so that I thought could be a problem. But apparently it's not. > >> >> >>> + memcpy(&x86_hyper_xen_hvm.runtime, (void >>> *)&x86_platform.hyper, >>> + sizeof(x86_platform.hyper)); >>> + h->guest_late_init = xen_hvm_guest_late_init; >> >> To me this still doesn't look right --- you are making assumptions about >> x86_platform/x86_init.hyper and I don't think you can assume they have >> not been set to point to another hypervisor, for example. > > You mean copy_array() calls in init_hypervisor_platform()? But that > happens after > > detect_hypervisor_vendor() shoose out the prefered hypervisor. In > detect stage, > > x86_platform/x86_init.hyper has default value for bare hardware, or I > missed something? Right. My point though is that this ordering is opaque to hypervisor-specific code and can change. The same is true about making assumptions about x86_platform/x86_init.hyper values. -boris
diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c index 1756cf7..7e1c75f 100644 --- a/arch/x86/xen/enlighten_hvm.c +++ b/arch/x86/xen/enlighten_hvm.c @@ -231,14 +231,6 @@ bool __init xen_hvm_need_lapic(void) return true; } -static uint32_t __init xen_platform_hvm(void) -{ - if (xen_pv_domain()) - return 0; - - return xen_cpuid_base(); -} - static __init void xen_hvm_guest_late_init(void) { #ifdef CONFIG_XEN_PVH @@ -250,6 +242,9 @@ static __init void xen_hvm_guest_late_init(void) /* PVH detected. */ xen_pvh = true; + if (nopv) + panic("\"nopv\" and \"xen_nopv\" parameters are unsupported in PVH guest."); + /* Make sure we don't fall back to (default) ACPI_IRQ_MODEL_PIC. */ if (!nr_ioapics && acpi_irq_model == ACPI_IRQ_MODEL_PIC) acpi_irq_model = ACPI_IRQ_MODEL_PLATFORM; @@ -259,6 +254,26 @@ static __init void xen_hvm_guest_late_init(void) #endif } +static uint32_t __init xen_platform_hvm(void) +{ + if (xen_pv_domain()) + return 0; + + if (xen_pvh_domain() && nopv) { + /* Guest booting via the Xen-PVH boot entry goes here */ + pr_info("\"nopv\" parameter is ignored in PVH guest\n"); + nopv = false; + } else if (nopv) { + /* + * Guest booting via normal boot entry (like via grub2) goes + * here. + */ + x86_init.hyper.guest_late_init = xen_hvm_guest_late_init; + return 0; + } + return xen_cpuid_base(); +} + const __initconst struct hypervisor_x86 x86_hyper_xen_hvm = { .name = "Xen HVM", .detect = xen_platform_hvm, @@ -268,4 +283,5 @@ static __init void xen_hvm_guest_late_init(void) .init.init_mem_mapping = xen_hvm_init_mem_mapping, .init.guest_late_init = xen_hvm_guest_late_init, .runtime.pin_vcpu = xen_pin_vcpu, + .ignore_nopv = true, };
PVH guest needs PV extentions to work, so "nopv" parameter should be ignored for PVH but not for HVM guest. If PVH guest boots up via the Xen-PVH boot entry, xen_pvh is set early, we know it's PVH guest and ignore "nopv" parameter directly. If PVH guest boots up via the normal boot entry same as HVM guest, it's hard to distinguish PVH and HVM guest at that time. In this case, we have to panic early if PVH is detected and nopv is enabled to avoid a worse situation later. Move xen_platform_hvm() after xen_hvm_guest_late_init() to avoid compile error. Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> Cc: Juergen Gross <jgross@suse.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Ingo Molnar <mingo@redhat.com> Cc: Borislav Petkov <bp@alien8.de> --- arch/x86/xen/enlighten_hvm.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-)