Re: [PATCH v6 4/4] x86/xen: Add "nopv" support for HVM guest

[Zhenzhong Duan]

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