Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v6 4/4] x86/xen: Add "nopv" support for HVM guest
To:     Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        linux-kernel@vger.kernel.org
Cc:     xen-devel@lists.xenproject.org, jgross@suse.com,
        sstabellini@kernel.org, tglx@linutronix.de, mingo@redhat.com,
        bp@alien8.de
References: <1562490908-17882-1-git-send-email-zhenzhong.duan@oracle.com>
 <1562490908-17882-5-git-send-email-zhenzhong.duan@oracle.com>
 <86b0dbb9-74a7-6883-e6d7-210bd35a6944@oracle.com>
From:   Zhenzhong Duan <zhenzhong.duan@oracle.com>
Organization: Oracle Corporation
Message-ID: <6cbd7b78-3d8d-64ae-fd2e-82244dbe6a1e@oracle.com>
Date:   Tue, 9 Jul 2019 12:20:40 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.2
MIME-Version: 1.0
In-Reply-To: <86b0dbb9-74a7-6883-e6d7-210bd35a6944@oracle.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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