[PATCH v2 1/2] xen/pvh: set xen_domain_type to HVM in xen_pvh_init

[PATCH v2 1/2] xen/pvh: set xen_domain_type to HVM in xen_pvh_init

[Roger Pau Monne]

Or else xen_domain() returns false despite xen_pvh being set.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: xen-devel@lists.xenproject.org
---
 arch/x86/xen/enlighten_pvh.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/xen/enlighten_pvh.c b/arch/x86/xen/enlighten_pvh.c
index 35b7599d2d0b..bbffa409e0e8 100644
--- a/arch/x86/xen/enlighten_pvh.c
+++ b/arch/x86/xen/enlighten_pvh.c
@@ -27,6 +27,7 @@ void __init xen_pvh_init(void)
 	u64 pfn;
 
 	xen_pvh = 1;
+	xen_domain_type = XEN_HVM_DOMAIN;
 	xen_start_flags = pvh_start_info.flags;
 
 	msr = cpuid_ebx(xen_cpuid_base() + 2);
-- 
2.17.2 (Apple Git-113)

[PATCH v2 2/2] xen/pvh: correctly setup the PV EFI interface for dom0

[Roger Pau Monne]

This involves initializing the boot params EFI related fields and the
efi global variable.

Without this fix a PVH dom0 doesn't detect when booted from EFI, and
thus doesn't support accessing any of the EFI related data.

Reported-by: PGNet Dev <pgnet.dev@gmail.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Andy Shevchenko <andy@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: xen-devel@lists.xenproject.org
Cc: platform-driver-x86@vger.kernel.org
---
Changes since v1:
 - Call xen_efi_init from xen_pvh_init, this avoids having to move the
   prototype of xen_efi_init to a different header.
---
 arch/x86/platform/pvh/enlighten.c |  8 ++++----
 arch/x86/xen/efi.c                | 12 ++++++------
 arch/x86/xen/enlighten_pv.c       |  2 +-
 arch/x86/xen/enlighten_pvh.c      |  6 +++++-
 arch/x86/xen/xen-ops.h            |  4 ++--
 5 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/arch/x86/platform/pvh/enlighten.c b/arch/x86/platform/pvh/enlighten.c
index 62f5c7045944..1861a2ba0f2b 100644
--- a/arch/x86/platform/pvh/enlighten.c
+++ b/arch/x86/platform/pvh/enlighten.c
@@ -44,8 +44,6 @@ void __init __weak mem_map_via_hcall(struct boot_params *ptr __maybe_unused)
 
 static void __init init_pvh_bootparams(bool xen_guest)
 {
-	memset(&pvh_bootparams, 0, sizeof(pvh_bootparams));
-
 	if ((pvh_start_info.version > 0) && (pvh_start_info.memmap_entries)) {
 		struct hvm_memmap_table_entry *ep;
 		int i;
@@ -103,7 +101,7 @@ static void __init init_pvh_bootparams(bool xen_guest)
  * If we are trying to boot a Xen PVH guest, it is expected that the kernel
  * will have been configured to provide the required override for this routine.
  */
-void __init __weak xen_pvh_init(void)
+void __init __weak xen_pvh_init(struct boot_params *boot_params)
 {
 	xen_raw_printk("Error: Missing xen PVH initialization\n");
 	BUG();
@@ -112,7 +110,7 @@ void __init __weak xen_pvh_init(void)
 static void hypervisor_specific_init(bool xen_guest)
 {
 	if (xen_guest)
-		xen_pvh_init();
+		xen_pvh_init(&pvh_bootparams);
 }
 
 /*
@@ -131,6 +129,8 @@ void __init xen_prepare_pvh(void)
 		BUG();
 	}
 
+	memset(&pvh_bootparams, 0, sizeof(pvh_bootparams));
+
 	hypervisor_specific_init(xen_guest);
 
 	init_pvh_bootparams(xen_guest);
diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c
index 1fbb629a9d78..0d3365cb64de 100644
--- a/arch/x86/xen/efi.c
+++ b/arch/x86/xen/efi.c
@@ -158,7 +158,7 @@ static enum efi_secureboot_mode xen_efi_get_secureboot(void)
 	return efi_secureboot_mode_unknown;
 }
 
-void __init xen_efi_init(void)
+void __init xen_efi_init(struct boot_params *boot_params)
 {
 	efi_system_table_t *efi_systab_xen;
 
@@ -167,12 +167,12 @@ void __init xen_efi_init(void)
 	if (efi_systab_xen == NULL)
 		return;
 
-	strncpy((char *)&boot_params.efi_info.efi_loader_signature, "Xen",
-			sizeof(boot_params.efi_info.efi_loader_signature));
-	boot_params.efi_info.efi_systab = (__u32)__pa(efi_systab_xen);
-	boot_params.efi_info.efi_systab_hi = (__u32)(__pa(efi_systab_xen) >> 32);
+	strncpy((char *)&boot_params->efi_info.efi_loader_signature, "Xen",
+			sizeof(boot_params->efi_info.efi_loader_signature));
+	boot_params->efi_info.efi_systab = (__u32)__pa(efi_systab_xen);
+	boot_params->efi_info.efi_systab_hi = (__u32)(__pa(efi_systab_xen) >> 32);
 
-	boot_params.secure_boot = xen_efi_get_secureboot();
+	boot_params->secure_boot = xen_efi_get_secureboot();
 
 	set_bit(EFI_BOOT, &efi.flags);
 	set_bit(EFI_PARAVIRT, &efi.flags);
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index c54a493e139a..4722ba2966ac 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -1403,7 +1403,7 @@ asmlinkage __visible void __init xen_start_kernel(void)
 	/* We need this for printk timestamps */
 	xen_setup_runstate_info(0);
 
-	xen_efi_init();
+	xen_efi_init(&boot_params);
 
 	/* Start the world */
 #ifdef CONFIG_X86_32
diff --git a/arch/x86/xen/enlighten_pvh.c b/arch/x86/xen/enlighten_pvh.c
index bbffa409e0e8..80a79db72fcf 100644
--- a/arch/x86/xen/enlighten_pvh.c
+++ b/arch/x86/xen/enlighten_pvh.c
@@ -13,6 +13,8 @@
 
 #include <xen/interface/memory.h>
 
+#include "xen-ops.h"
+
 /*
  * PVH variables.
  *
@@ -21,7 +23,7 @@
  */
 bool xen_pvh __attribute__((section(".data"))) = 0;
 
-void __init xen_pvh_init(void)
+void __init xen_pvh_init(struct boot_params *boot_params)
 {
 	u32 msr;
 	u64 pfn;
@@ -33,6 +35,8 @@ void __init xen_pvh_init(void)
 	msr = cpuid_ebx(xen_cpuid_base() + 2);
 	pfn = __pa(hypercall_page);
 	wrmsr_safe(msr, (u32)pfn, (u32)(pfn >> 32));
+
+	xen_efi_init(boot_params);
 }
 
 void __init mem_map_via_hcall(struct boot_params *boot_params_p)
diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
index 0e60bd918695..2f111f47ba98 100644
--- a/arch/x86/xen/xen-ops.h
+++ b/arch/x86/xen/xen-ops.h
@@ -122,9 +122,9 @@ static inline void __init xen_init_vga(const struct dom0_vga_console_info *info,
 void __init xen_init_apic(void);
 
 #ifdef CONFIG_XEN_EFI
-extern void xen_efi_init(void);
+extern void xen_efi_init(struct boot_params *boot_params);
 #else
-static inline void __init xen_efi_init(void)
+static inline void __init xen_efi_init(struct boot_params *boot_params)
 {
 }
 #endif
-- 
2.17.2 (Apple Git-113)

Re: [PATCH v2 1/2] xen/pvh: set xen_domain_type to HVM in xen_pvh_init

[Boris Ostrovsky]

On 4/23/19 9:04 AM, Roger Pau Monne wrote:
> Or else xen_domain() returns false despite xen_pvh being set.

Is this new xen_domain() invocation somewhere in EFI initialization that
you add in the second patch?

Asking because I am trying to figure out whether this needs to go to
stable tree.

-boris

>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: xen-devel@lists.xenproject.org
> ---
>  arch/x86/xen/enlighten_pvh.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/xen/enlighten_pvh.c b/arch/x86/xen/enlighten_pvh.c
> index 35b7599d2d0b..bbffa409e0e8 100644
> --- a/arch/x86/xen/enlighten_pvh.c
> +++ b/arch/x86/xen/enlighten_pvh.c
> @@ -27,6 +27,7 @@ void __init xen_pvh_init(void)
>  	u64 pfn;
>  
>  	xen_pvh = 1;
> +	xen_domain_type = XEN_HVM_DOMAIN;
>  	xen_start_flags = pvh_start_info.flags;
>  
>  	msr = cpuid_ebx(xen_cpuid_base() + 2);

Re: [Xen-devel] [PATCH v2 1/2] xen/pvh: set xen_domain_type to HVM in xen_pvh_init

[Roger Pau Monné]

On Tue, Apr 23, 2019 at 09:37:51AM -0400, Boris Ostrovsky wrote:
> On 4/23/19 9:04 AM, Roger Pau Monne wrote:
> > Or else xen_domain() returns false despite xen_pvh being set.
> 
> Is this new xen_domain() invocation somewhere in EFI initialization that
> you add in the second patch?

Yes, there's a xen_initial_domain() call in xen_efi_probe() which
would return false without this fix.

> Asking because I am trying to figure out whether this needs to go to
> stable tree.

I'm not aware of this being an issue without the EFI code that I add
in patch 2, xen_domain_type would get set slightly later in the boot
process.

I guess both patches should be backported to 4.19 since that's the
only LTS release that has PVH dom0 support IIRC, and it would be good
to get this fixed.

Thanks, Roger.

Re: [PATCH v2 2/2] xen/pvh: correctly setup the PV EFI interface for dom0

[Boris Ostrovsky]

On 4/23/19 9:04 AM, Roger Pau Monne wrote:
> This involves initializing the boot params EFI related fields and the
> efi global variable.
>
> Without this fix a PVH dom0 doesn't detect when booted from EFI, and
> thus doesn't support accessing any of the EFI related data.
>
> Reported-by: PGNet Dev <pgnet.dev@gmail.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Hmm.. This seems to be breaking save/restore for me (for domU), and I
can't see any obvious reasons.

Can you try it?

-boris

Re: [PATCH v2 2/2] xen/pvh: correctly setup the PV EFI interface for dom0

[Roger Pau Monné]

On Wed, Apr 24, 2019 at 11:36:41AM -0400, Boris Ostrovsky wrote:
> On 4/23/19 9:04 AM, Roger Pau Monne wrote:
> > This involves initializing the boot params EFI related fields and the
> > efi global variable.
> >
> > Without this fix a PVH dom0 doesn't detect when booted from EFI, and
> > thus doesn't support accessing any of the EFI related data.
> >
> > Reported-by: PGNet Dev <pgnet.dev@gmail.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Hmm.. This seems to be breaking save/restore for me (for domU), and I
> can't see any obvious reasons.
> 
> Can you try it?

Sure, thanks for the extra testing! I have to admit I haven't tested
save/restore with this patch applied, it didn't come to mind it might
affect that functionality.

I assume it's save/restore of a PVH domU that's broken (HVM and PV are
fine)?

Thanks, Roger.

Re: [PATCH v2 2/2] xen/pvh: correctly setup the PV EFI interface for dom0

[Boris Ostrovsky]

On 4/24/19 11:45 AM, Roger Pau Monné wrote:
> On Wed, Apr 24, 2019 at 11:36:41AM -0400, Boris Ostrovsky wrote:
>> On 4/23/19 9:04 AM, Roger Pau Monne wrote:
>>> This involves initializing the boot params EFI related fields and the
>>> efi global variable.
>>>
>>> Without this fix a PVH dom0 doesn't detect when booted from EFI, and
>>> thus doesn't support accessing any of the EFI related data.
>>>
>>> Reported-by: PGNet Dev <pgnet.dev@gmail.com>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Hmm.. This seems to be breaking save/restore for me (for domU), and I
>> can't see any obvious reasons.
>>
>> Can you try it?
> Sure, thanks for the extra testing! I have to admit I haven't tested
> save/restore with this patch applied, it didn't come to mind it might
> affect that functionality.
>
> I assume it's save/restore of a PVH domU that's broken (HVM and PV are
> fine)?
>


PV is fine, I haven't tried HVM.

-boris

Re: [PATCH v2 2/2] xen/pvh: correctly setup the PV EFI interface for dom0

[Roger Pau Monné]

On Wed, Apr 24, 2019 at 11:45:43AM -0400, Boris Ostrovsky wrote:
> On 4/24/19 11:45 AM, Roger Pau Monné wrote:
> > On Wed, Apr 24, 2019 at 11:36:41AM -0400, Boris Ostrovsky wrote:
> >> On 4/23/19 9:04 AM, Roger Pau Monne wrote:
> >>> This involves initializing the boot params EFI related fields and the
> >>> efi global variable.
> >>>
> >>> Without this fix a PVH dom0 doesn't detect when booted from EFI, and
> >>> thus doesn't support accessing any of the EFI related data.
> >>>
> >>> Reported-by: PGNet Dev <pgnet.dev@gmail.com>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> Hmm.. This seems to be breaking save/restore for me (for domU), and I
> >> can't see any obvious reasons.
> >>
> >> Can you try it?
> > Sure, thanks for the extra testing! I have to admit I haven't tested
> > save/restore with this patch applied, it didn't come to mind it might
> > affect that functionality.
> >
> > I assume it's save/restore of a PVH domU that's broken (HVM and PV are
> > fine)?
> >
> 
> 
> PV is fine, I haven't tried HVM.

I've tested live migration and save/restore of this series on top of
5.1.0-rc5 and seems to work fine for me. On top of which Linux version
where you testing, and can you paste the guest config file?

Thanks, Roger.

Re: [PATCH v2 2/2] xen/pvh: correctly setup the PV EFI interface for dom0

[Boris Ostrovsky]

On 4/25/19 6:02 AM, Roger Pau Monné wrote:
> On Wed, Apr 24, 2019 at 11:45:43AM -0400, Boris Ostrovsky wrote:
>> On 4/24/19 11:45 AM, Roger Pau Monné wrote:
>>> On Wed, Apr 24, 2019 at 11:36:41AM -0400, Boris Ostrovsky wrote:
>>>> On 4/23/19 9:04 AM, Roger Pau Monne wrote:
>>>>> This involves initializing the boot params EFI related fields and the
>>>>> efi global variable.
>>>>>
>>>>> Without this fix a PVH dom0 doesn't detect when booted from EFI, and
>>>>> thus doesn't support accessing any of the EFI related data.
>>>>>
>>>>> Reported-by: PGNet Dev <pgnet.dev@gmail.com>
>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>> Hmm.. This seems to be breaking save/restore for me (for domU), and I
>>>> can't see any obvious reasons.
>>>>
>>>> Can you try it?
>>> Sure, thanks for the extra testing! I have to admit I haven't tested
>>> save/restore with this patch applied, it didn't come to mind it might
>>> affect that functionality.
>>>
>>> I assume it's save/restore of a PVH domU that's broken (HVM and PV are
>>> fine)?
>>>
>>
>> PV is fine, I haven't tried HVM.
> I've tested live migration and save/restore of this series on top of
> 5.1.0-rc5 and seems to work fine for me. On top of which Linux version
> where you testing, and can you paste the guest config file?
>

It's not your change.

I was booting with threadirqs boot option and hvc console apparently
doesn't like that. It's not 100% reproducible, but I was able to fail
save/restore without your patches as well.

I may look at this sometime next week, but in the meantime for both patches

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

and I will add 4.19+ for stable.

-boris