[Xen 4.12 bug] HVM/PVH boot confusion

[Xen 4.12 bug] HVM/PVH boot confusion

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 1806 bytes --]

Hello,

Given the following vm.cfg file:

name="vm"
type="hvm"

vcpus=4
memory=1024

firmware_override="/root/xen-syms"

kernel="/boot/vmlinuz-4.4-xen"
ramdisk="/boot/initrd-4.4.0+10.img"

cmdline="console=xen,pv dom0=pv --- earlyprintk=xen"

Xen crashes with the following trace:

(d15) (XEN) Xen BUG at pvh-boot.c:82
(d15) (XEN) ----[ Xen-4.12.0-rc  x86_64  debug=y   Not tainted ]----
(d15) (XEN) CPU:    0
(d15) (XEN) RIP:    e008:[<ffff82d0804331f2>] pvh_init+0x27d/0x2fe
<snip>
(d15) (XEN) Xen call trace:
(d15) (XEN)    [<ffff82d0804331f2>] pvh_init+0x27d/0x2fe
(d15) (XEN)    [<ffff82d080429000>] __start_xen+0x14c/0x28f6
(d15) (XEN)    [<ffff82d0802000f3>] __high_start+0x53/0x55
(d15) (XEN)
(d15) (XEN)
(d15) (XEN) ****************************************
(d15) (XEN) Panic on CPU 0:
(d15) (XEN) Xen BUG at pvh-boot.c:82
(d15) (XEN) ****************************************

The problem is that Xen is started at its PVH entrypoint (contrary to
the instructions in the vm config file), and Xen unconditionally expects
RSDP to be passed.

There are at least two bugs here.

1) RSDP was a late addition to the PVH boot protocol.  Xen's PVH
entrypoint must not mandate its existence, because there are releases of
the domain builder which don't provide it.

2) The HVM/PVH boot confusion.  This think this is a still-outstanding
bug around the broken assumption that the hvmloader binary speaks the
PVH protocol without advertising itself appropriately (I really regret
not objecting to those patches before they went in).  At the least, that
needs fixing by putting a proper ELF note in hvmloader, and the domain
builder needs to be updated to build all PVH-boot-ABI images consistently.

I don't have time to look into this at the moment, if anyone fancies
trying to fix these issues.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 2295 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen 4.12 bug] HVM/PVH boot confusion

[Roger Pau Monné]

Adding Anthony who likely knows more about all this since it's closely
related to QEMU.

On Tue, Jan 29, 2019 at 07:49:51PM +0000, Andrew Cooper wrote:
> Hello,
> 
> Given the following vm.cfg file:
> 
> name="vm"
> type="hvm"
> 
> vcpus=4
> memory=1024
> 
> firmware_override="/root/xen-syms"
> 
> kernel="/boot/vmlinuz-4.4-xen"
> ramdisk="/boot/initrd-4.4.0+10.img"

I know very little about direct kernel booting with HVM, but I assume
using firmware_override gets rid of hvmloader and the BIOS/UEFI
payload plus the option rom to direct boot into a Linux kernel?

So basically the module list is `mod[0] = kernel` and `mod[1] =
ramdisk`?

> 
> cmdline="console=xen,pv dom0=pv --- earlyprintk=xen"
> 
> Xen crashes with the following trace:
> 
> (d15) (XEN) Xen BUG at pvh-boot.c:82
> (d15) (XEN) ----[ Xen-4.12.0-rc  x86_64  debug=y   Not tainted ]----
> (d15) (XEN) CPU:    0
> (d15) (XEN) RIP:    e008:[<ffff82d0804331f2>] pvh_init+0x27d/0x2fe
> <snip>
> (d15) (XEN) Xen call trace:
> (d15) (XEN)    [<ffff82d0804331f2>] pvh_init+0x27d/0x2fe
> (d15) (XEN)    [<ffff82d080429000>] __start_xen+0x14c/0x28f6
> (d15) (XEN)    [<ffff82d0802000f3>] __high_start+0x53/0x55
> (d15) (XEN)
> (d15) (XEN)
> (d15) (XEN) ****************************************
> (d15) (XEN) Panic on CPU 0:
> (d15) (XEN) Xen BUG at pvh-boot.c:82
> (d15) (XEN) ****************************************
> 
> The problem is that Xen is started at its PVH entrypoint (contrary to
> the instructions in the vm config file), and Xen unconditionally expects
> RSDP to be passed.
> 
> There are at least two bugs here.
> 
> 1) RSDP was a late addition to the PVH boot protocol.  Xen's PVH
> entrypoint must not mandate its existence, because there are releases of
> the domain builder which don't provide it.

Right, I think it should be fine to attempt to boot without a rsdp
hint, Xen can always resort to scanning the low 1MB in order to
attempt to find the rsdp.

The problem here would be that the toolstack is not going to create
any ACPI tables because it's a HVM guest and thus the toolstack
expects the firmware to create such tables (hvmloader). Since in the
above example you skip the firmware, you won't get any ACPI tables at
all.

> 2) The HVM/PVH boot confusion.  This think this is a still-outstanding
> bug around the broken assumption that the hvmloader binary speaks the
> PVH protocol without advertising itself appropriately (I really regret
> not objecting to those patches before they went in).  At the least, that
> needs fixing by putting a proper ELF note in hvmloader, and the domain
> builder needs to be updated to build all PVH-boot-ABI images consistently.

Hm, booting hvmloader using the hvm_start_info has always been kind of
a hack, that relied on the toolstack being in full control of the
firmware and the user not playing with it.

I think part of the problem here also comes from the fact that the
loader list (xc_dom_loader *first_loader) is shared between
all the guest types, thus allowing a guest in a hvm container to be
booted using the pvh entry point.

I think a proper fix to this mess involves the HVM and the PVH domain
creation paths being exactly the same, ie: ACPI tables always created
by the toolstack for example. The only difference between a PVH and a
HVM guests from the toolstack PoV should be the emulated devices that
it gets.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen 4.12 bug] HVM/PVH boot confusion

[Wei Liu]

On Tue, Jan 29, 2019 at 07:49:51PM +0000, Andrew Cooper wrote:
> Hello,
> 
> Given the following vm.cfg file:
> 
> name="vm"
> type="hvm"
> 
> vcpus=4
> memory=1024
> 
> firmware_override="/root/xen-syms"
> 
> kernel="/boot/vmlinuz-4.4-xen"
> ramdisk="/boot/initrd-4.4.0+10.img"
> 
> cmdline="console=xen,pv dom0=pv --- earlyprintk=xen"
> 

What is your use case here? If you want to use the shim, why don't you
put pvshim=1 directly?

> Xen crashes with the following trace:
> 
> (d15) (XEN) Xen BUG at pvh-boot.c:82
> (d15) (XEN) ----[ Xen-4.12.0-rc  x86_64  debug=y   Not tainted ]----
> (d15) (XEN) CPU:    0
> (d15) (XEN) RIP:    e008:[<ffff82d0804331f2>] pvh_init+0x27d/0x2fe
> <snip>
> (d15) (XEN) Xen call trace:
> (d15) (XEN)    [<ffff82d0804331f2>] pvh_init+0x27d/0x2fe
> (d15) (XEN)    [<ffff82d080429000>] __start_xen+0x14c/0x28f6
> (d15) (XEN)    [<ffff82d0802000f3>] __high_start+0x53/0x55
> (d15) (XEN)
> (d15) (XEN)
> (d15) (XEN) ****************************************
> (d15) (XEN) Panic on CPU 0:
> (d15) (XEN) Xen BUG at pvh-boot.c:82
> (d15) (XEN) ****************************************
> 
> The problem is that Xen is started at its PVH entrypoint (contrary to
> the instructions in the vm config file), and Xen unconditionally expects
> RSDP to be passed.
> 
> There are at least two bugs here.
> 
> 1) RSDP was a late addition to the PVH boot protocol.  Xen's PVH
> entrypoint must not mandate its existence, because there are releases of
> the domain builder which don't provide it.

The inner Xen, in this case, can fall back to scanning low 1MB memory
for RSDP.

> 
> 2) The HVM/PVH boot confusion.  This think this is a still-outstanding
> bug around the broken assumption that the hvmloader binary speaks the
> PVH protocol without advertising itself appropriately (I really regret
> not objecting to those patches before they went in).  At the least, that
> needs fixing by putting a proper ELF note in hvmloader, and the domain
> builder needs to be updated to build all PVH-boot-ABI images consistently.

Do you expect users to drop an arbitrary hvmloader into an arbitrary
version of Xen and they continue to work?

Wei.

> 
> I don't have time to look into this at the moment, if anyone fancies
> trying to fix these issues.
> 
> ~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen 4.12 bug] HVM/PVH boot confusion

[Anthony PERARD]

On Wed, Jan 30, 2019 at 12:25:20PM +0100, Roger Pau Monné wrote:
> Adding Anthony who likely knows more about all this since it's closely
> related to QEMU.
> 
> On Tue, Jan 29, 2019 at 07:49:51PM +0000, Andrew Cooper wrote:
> > Hello,
> > 
> > Given the following vm.cfg file:
> > 
> > name="vm"
> > type="hvm"
> > 
> > vcpus=4
> > memory=1024
> > 
> > firmware_override="/root/xen-syms"
> > 
> > kernel="/boot/vmlinuz-4.4-xen"
> > ramdisk="/boot/initrd-4.4.0+10.img"
> 
> I know very little about direct kernel booting with HVM, but I assume
> using firmware_override gets rid of hvmloader and the BIOS/UEFI
> payload plus the option rom to direct boot into a Linux kernel?

In HVM direct boot case, SeaBIOS takes care of loading the kernel and
booting it. QEMU communicate with SeaBIOS via the firmware config
interface (fw_cfg).

Setting firmware_override only replace hvmloader by that other elf.


> So basically the module list is `mod[0] = kernel` and `mod[1] =
> ramdisk`?

No, the module list isn't magically change, I don't think.
The toolstack is still going to but seabios as one of the module
(probably only, I forgot), but there isn't a reason to put the
kernel/ramdisk in there.


In the config above, xen can be a kernel at most (I think), but
certainly not a firmware.

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen 4.12 bug] HVM/PVH boot confusion

[Andrew Cooper]

On 30/01/2019 11:25, Roger Pau Monné wrote:
> Adding Anthony who likely knows more about all this since it's closely
> related to QEMU.
>
> On Tue, Jan 29, 2019 at 07:49:51PM +0000, Andrew Cooper wrote:
>> Hello,
>>
>> Given the following vm.cfg file:
>>
>> name="vm"
>> type="hvm"
>>
>> vcpus=4
>> memory=1024
>>
>> firmware_override="/root/xen-syms"
>>
>> kernel="/boot/vmlinuz-4.4-xen"
>> ramdisk="/boot/initrd-4.4.0+10.img"
> I know very little about direct kernel booting with HVM, but I assume
> using firmware_override gets rid of hvmloader and the BIOS/UEFI
> payload plus the option rom to direct boot into a Linux kernel?

Correct.

> So basically the module list is `mod[0] = kernel` and `mod[1] =
> ramdisk`?

I'd expect so.  That was setup I was aiming for.

>> cmdline="console=xen,pv dom0=pv --- earlyprintk=xen"
>>
>> Xen crashes with the following trace:
>>
>> (d15) (XEN) Xen BUG at pvh-boot.c:82
>> (d15) (XEN) ----[ Xen-4.12.0-rc  x86_64  debug=y   Not tainted ]----
>> (d15) (XEN) CPU:    0
>> (d15) (XEN) RIP:    e008:[<ffff82d0804331f2>] pvh_init+0x27d/0x2fe
>> <snip>
>> (d15) (XEN) Xen call trace:
>> (d15) (XEN)    [<ffff82d0804331f2>] pvh_init+0x27d/0x2fe
>> (d15) (XEN)    [<ffff82d080429000>] __start_xen+0x14c/0x28f6
>> (d15) (XEN)    [<ffff82d0802000f3>] __high_start+0x53/0x55
>> (d15) (XEN)
>> (d15) (XEN)
>> (d15) (XEN) ****************************************
>> (d15) (XEN) Panic on CPU 0:
>> (d15) (XEN) Xen BUG at pvh-boot.c:82
>> (d15) (XEN) ****************************************
>>
>> The problem is that Xen is started at its PVH entrypoint (contrary to
>> the instructions in the vm config file), and Xen unconditionally expects
>> RSDP to be passed.
>>
>> There are at least two bugs here.
>>
>> 1) RSDP was a late addition to the PVH boot protocol.  Xen's PVH
>> entrypoint must not mandate its existence, because there are releases of
>> the domain builder which don't provide it.
> Right, I think it should be fine to attempt to boot without a rsdp
> hint, Xen can always resort to scanning the low 1MB in order to
> attempt to find the rsdp.

Agreed.

> The problem here would be that the toolstack is not going to create
> any ACPI tables because it's a HVM guest and thus the toolstack
> expects the firmware to create such tables (hvmloader). Since in the
> above example you skip the firmware, you won't get any ACPI tables at
> all.

This, while true, isn't actually a problem for the case I'm testing.

But yes - I don't expect to be able to substitute Xen for hvmloader in
the general case.  What I didn't expect was for Xen to explode in the
way it did.

>
>> 2) The HVM/PVH boot confusion.  This think this is a still-outstanding
>> bug around the broken assumption that the hvmloader binary speaks the
>> PVH protocol without advertising itself appropriately (I really regret
>> not objecting to those patches before they went in).  At the least, that
>> needs fixing by putting a proper ELF note in hvmloader, and the domain
>> builder needs to be updated to build all PVH-boot-ABI images consistently.
> Hm, booting hvmloader using the hvm_start_info has always been kind of
> a hack, that relied on the toolstack being in full control of the
> firmware and the user not playing with it.

Its doubly a hack because our provided hvmloader binary doesn't
advertise itself as a PVH-capable binary.

> I think part of the problem here also comes from the fact that the
> loader list (xc_dom_loader *first_loader) is shared between
> all the guest types, thus allowing a guest in a hvm container to be
> booted using the pvh entry point.

I think it would help to have a clear separation between the
PVH-boot-ABI (which is used by HVM guests as well), and the concept of a
PVH guest simply being an HVM with less emulation.

The domain builder does need to have some knowledge, even if only for
building the memory map.

>
> I think a proper fix to this mess involves the HVM and the PVH domain
> creation paths being exactly the same, ie: ACPI tables always created
> by the toolstack for example. The only difference between a PVH and a
> HVM guests from the toolstack PoV should be the emulated devices that
> it gets.

This is definitely a good move.  We've got too many different ways of
constructing guests, and there is a lot of unnecessary duplication.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen 4.12 bug] HVM/PVH boot confusion

[Andrew Cooper]

On 30/01/2019 11:59, Wei Liu wrote:
> On Tue, Jan 29, 2019 at 07:49:51PM +0000, Andrew Cooper wrote:
>> Hello,
>>
>> Given the following vm.cfg file:
>>
>> name="vm"
>> type="hvm"
>>
>> vcpus=4
>> memory=1024
>>
>> firmware_override="/root/xen-syms"
>>
>> kernel="/boot/vmlinuz-4.4-xen"
>> ramdisk="/boot/initrd-4.4.0+10.img"
>>
>> cmdline="console=xen,pv dom0=pv --- earlyprintk=xen"
>>
> What is your use case here? If you want to use the shim, why don't you
> put pvshim=1 directly?

I'm testing a corner case.

>
>> Xen crashes with the following trace:
>>
>> (d15) (XEN) Xen BUG at pvh-boot.c:82
>> (d15) (XEN) ----[ Xen-4.12.0-rc  x86_64  debug=y   Not tainted ]----
>> (d15) (XEN) CPU:    0
>> (d15) (XEN) RIP:    e008:[<ffff82d0804331f2>] pvh_init+0x27d/0x2fe
>> <snip>
>> (d15) (XEN) Xen call trace:
>> (d15) (XEN)    [<ffff82d0804331f2>] pvh_init+0x27d/0x2fe
>> (d15) (XEN)    [<ffff82d080429000>] __start_xen+0x14c/0x28f6
>> (d15) (XEN)    [<ffff82d0802000f3>] __high_start+0x53/0x55
>> (d15) (XEN)
>> (d15) (XEN)
>> (d15) (XEN) ****************************************
>> (d15) (XEN) Panic on CPU 0:
>> (d15) (XEN) Xen BUG at pvh-boot.c:82
>> (d15) (XEN) ****************************************
>>
>> The problem is that Xen is started at its PVH entrypoint (contrary to
>> the instructions in the vm config file), and Xen unconditionally expects
>> RSDP to be passed.
>>
>> There are at least two bugs here.
>>
>> 1) RSDP was a late addition to the PVH boot protocol.  Xen's PVH
>> entrypoint must not mandate its existence, because there are releases of
>> the domain builder which don't provide it.
> The inner Xen, in this case, can fall back to scanning low 1MB memory
> for RSDP.

Agreed.

>
>> 2) The HVM/PVH boot confusion.  This think this is a still-outstanding
>> bug around the broken assumption that the hvmloader binary speaks the
>> PVH protocol without advertising itself appropriately (I really regret
>> not objecting to those patches before they went in).  At the least, that
>> needs fixing by putting a proper ELF note in hvmloader, and the domain
>> builder needs to be updated to build all PVH-boot-ABI images consistently.
> Do you expect users to drop an arbitrary hvmloader into an arbitrary
> version of Xen and they continue to work?

This is hard to answer.  HVMLoader specifically, perhaps not.  After
all, it does link unstable libraries from tools/

That said, how we boot guests is an ABI, and this should be kept in mind
when making changes.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen 4.12 bug] HVM/PVH boot confusion

[Wei Liu]

On Wed, Jan 30, 2019 at 12:30:44PM +0000, Andrew Cooper wrote:
> >> There are at least two bugs here.
> >>
> >> 1) RSDP was a late addition to the PVH boot protocol.  Xen's PVH
> >> entrypoint must not mandate its existence, because there are releases of
> >> the domain builder which don't provide it.
> > The inner Xen, in this case, can fall back to scanning low 1MB memory
> > for RSDP.
> 
> Agreed.
> 
> >
> >> 2) The HVM/PVH boot confusion.  This think this is a still-outstanding
> >> bug around the broken assumption that the hvmloader binary speaks the
> >> PVH protocol without advertising itself appropriately (I really regret
> >> not objecting to those patches before they went in).  At the least, that
> >> needs fixing by putting a proper ELF note in hvmloader, and the domain
> >> builder needs to be updated to build all PVH-boot-ABI images consistently.
> > Do you expect users to drop an arbitrary hvmloader into an arbitrary
> > version of Xen and they continue to work?
> 
> This is hard to answer.  HVMLoader specifically, perhaps not.  After
> all, it does link unstable libraries from tools/

That's my thought as well.

To me it would be nice to make hvmloader a proper PVH binary, but this
task's priority is rather low, because I consider it internal to Xen.

> 
> That said, how we boot guests is an ABI, and this should be kept in mind
> when making changes.

Sure. I agree.

I think we've been mindful in that regard. Domain builder code isn't
perfect, but it doesn't remove or add to the ABI willy-nilly. Long term
I agree with Roger -- things can be streamlined.

Your use case is rather special. At this moment I think if we fix issue
bug #1 you should be good to go?

Wei.

> 
> ~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen 4.12 bug] HVM/PVH boot confusion

[Wei Liu]

On Wed, Jan 30, 2019 at 12:46:45PM +0000, Wei Liu wrote:
> On Wed, Jan 30, 2019 at 12:30:44PM +0000, Andrew Cooper wrote:
> > >> There are at least two bugs here.
> > >>
> > >> 1) RSDP was a late addition to the PVH boot protocol.  Xen's PVH
> > >> entrypoint must not mandate its existence, because there are releases of
> > >> the domain builder which don't provide it.
> > > The inner Xen, in this case, can fall back to scanning low 1MB memory
> > > for RSDP.
> > 
> > Agreed.

---8<---
From 46e3686355da177955ca63ce3e8cf6a80cdb897d Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Wed, 30 Jan 2019 13:31:32 +0000
Subject: [PATCH] x86/pvh: don't mandate presence of RSDP pointer

RSDP hint is not mandatory according to PVH spec. Only set the hint if
pvh_info contains a valid pointer. The guest (xen) will fall back to
scanning for RSDP in lower 1MB of memory.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/guest/pvh-boot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/guest/pvh-boot.c b/xen/arch/x86/guest/pvh-boot.c
index 544775eeb4..c6e21fa83b 100644
--- a/xen/arch/x86/guest/pvh-boot.c
+++ b/xen/arch/x86/guest/pvh-boot.c
@@ -79,8 +79,8 @@ static void __init convert_pvh_info(multiboot_info_t **mbi,
         pvh_mbi_mods[i].string    = entry[i].cmdline_paddr;
     }
 
-    BUG_ON(!pvh_info->rsdp_paddr);
-    rsdp_hint = pvh_info->rsdp_paddr;
+    if ( pvh_info->rsdp_paddr )
+        rsdp_hint = pvh_info->rsdp_paddr;
 
     *mbi = &pvh_mbi;
     *mod = pvh_mbi_mods;
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen 4.12 bug] HVM/PVH boot confusion

[Juergen Gross]

On 30/01/2019 14:38, Wei Liu wrote:
> On Wed, Jan 30, 2019 at 12:46:45PM +0000, Wei Liu wrote:
>> On Wed, Jan 30, 2019 at 12:30:44PM +0000, Andrew Cooper wrote:
>>>>> There are at least two bugs here.
>>>>>
>>>>> 1) RSDP was a late addition to the PVH boot protocol.  Xen's PVH
>>>>> entrypoint must not mandate its existence, because there are releases of
>>>>> the domain builder which don't provide it.
>>>> The inner Xen, in this case, can fall back to scanning low 1MB memory
>>>> for RSDP.
>>>
>>> Agreed.
> 
> ---8<---
> From 46e3686355da177955ca63ce3e8cf6a80cdb897d Mon Sep 17 00:00:00 2001
> From: Wei Liu <wei.liu2@citrix.com>
> Date: Wed, 30 Jan 2019 13:31:32 +0000
> Subject: [PATCH] x86/pvh: don't mandate presence of RSDP pointer
> 
> RSDP hint is not mandatory according to PVH spec. Only set the hint if
> pvh_info contains a valid pointer. The guest (xen) will fall back to
> scanning for RSDP in lower 1MB of memory.
> 
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/arch/x86/guest/pvh-boot.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/guest/pvh-boot.c b/xen/arch/x86/guest/pvh-boot.c
> index 544775eeb4..c6e21fa83b 100644
> --- a/xen/arch/x86/guest/pvh-boot.c
> +++ b/xen/arch/x86/guest/pvh-boot.c
> @@ -79,8 +79,8 @@ static void __init convert_pvh_info(multiboot_info_t **mbi,
>          pvh_mbi_mods[i].string    = entry[i].cmdline_paddr;
>      }
>  
> -    BUG_ON(!pvh_info->rsdp_paddr);
> -    rsdp_hint = pvh_info->rsdp_paddr;
> +    if ( pvh_info->rsdp_paddr )

Why do you need the if here? rsdp_hint is NULL initially.

So just removing the BUG_ON() seems to be required.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen 4.12 bug] HVM/PVH boot confusion

[Wei Liu]

On Wed, Jan 30, 2019 at 02:49:52PM +0100, Juergen Gross wrote:
> On 30/01/2019 14:38, Wei Liu wrote:
> > On Wed, Jan 30, 2019 at 12:46:45PM +0000, Wei Liu wrote:
> >> On Wed, Jan 30, 2019 at 12:30:44PM +0000, Andrew Cooper wrote:
> >>>>> There are at least two bugs here.
> >>>>>
> >>>>> 1) RSDP was a late addition to the PVH boot protocol.  Xen's PVH
> >>>>> entrypoint must not mandate its existence, because there are releases of
> >>>>> the domain builder which don't provide it.
> >>>> The inner Xen, in this case, can fall back to scanning low 1MB memory
> >>>> for RSDP.
> >>>
> >>> Agreed.
> > 
> > ---8<---
> > From 46e3686355da177955ca63ce3e8cf6a80cdb897d Mon Sep 17 00:00:00 2001
> > From: Wei Liu <wei.liu2@citrix.com>
> > Date: Wed, 30 Jan 2019 13:31:32 +0000
> > Subject: [PATCH] x86/pvh: don't mandate presence of RSDP pointer
> > 
> > RSDP hint is not mandatory according to PVH spec. Only set the hint if
> > pvh_info contains a valid pointer. The guest (xen) will fall back to
> > scanning for RSDP in lower 1MB of memory.
> > 
> > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  xen/arch/x86/guest/pvh-boot.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/x86/guest/pvh-boot.c b/xen/arch/x86/guest/pvh-boot.c
> > index 544775eeb4..c6e21fa83b 100644
> > --- a/xen/arch/x86/guest/pvh-boot.c
> > +++ b/xen/arch/x86/guest/pvh-boot.c
> > @@ -79,8 +79,8 @@ static void __init convert_pvh_info(multiboot_info_t **mbi,
> >          pvh_mbi_mods[i].string    = entry[i].cmdline_paddr;
> >      }
> >  
> > -    BUG_ON(!pvh_info->rsdp_paddr);
> > -    rsdp_hint = pvh_info->rsdp_paddr;
> > +    if ( pvh_info->rsdp_paddr )
> 
> Why do you need the if here? rsdp_hint is NULL initially.
> 
> So just removing the BUG_ON() seems to be required.

Good point.

Wei.

> 
> 
> Juergen
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel