[SeaBIOS PATCH] xen: require Xen info structure at 0x1000 to detect Xen

David Woodhouse posted 1 patch 2 weeks, 5 days ago
src/fw/xen.c | 45 ++++++++++++++++++++++++++++++++-------------
1 file changed, 32 insertions(+), 13 deletions(-)
[SeaBIOS PATCH] xen: require Xen info structure at 0x1000 to detect Xen
Posted by David Woodhouse 2 weeks, 5 days ago
From: David Woodhouse <dwmw@amazon.co.uk>

When running under Xen, hvmloader places a table at 0x1000 with the e820
information and BIOS tables. If this isn't present, SeaBIOS will 
currently panic.

We now have support for running Xen guests natively in QEMU/KVM, which
boots SeaBIOS directly instead of via hvmloader, and does not provide
the same structure.

As it happens, this doesn't matter on first boot. because although we
set PlatformRunningOn to PF_QEMU|PF_XEN, reading it back again still
gives zero. Presumably because in true Xen, this is all already RAM. But
in QEMU with a faithfully-emulated PAM config in the host bridge, it's
still in ROM mode at this point so we don't see what we've just written.

On reboot, however, the region *is* set to RAM mode and we do see the
updated value of PlatformRunningOn, do manage to remember that we've
detected Xen in CPUID, and hit the panic.

It's not trivial to detect QEMU vs. real Xen at the time xen_preinit()
runs, because it's so early. We can't even make a XENVER_extraversion
hypercall to look for hints, because we haven't set up the hypercall
page (and don't have an allocator to give us a page in which to do so).

So just make Xen detection contingent on the info structure being
present. If it wasn't, we were going to panic anyway. That leaves us
taking the standard QEMU init path for Xen guests in native QEMU,
which is just fine.

Untested on actual Xen but ObviouslyCorrect™.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 src/fw/xen.c | 45 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/src/fw/xen.c b/src/fw/xen.c
index a215b9e..00e4b0c 100644
--- a/src/fw/xen.c
+++ b/src/fw/xen.c
@@ -40,16 +40,25 @@ struct xen_seabios_info {
     u32 e820_nr;
 } PACKED;
 
-static void validate_info(struct xen_seabios_info *t)
+static struct xen_seabios_info *validate_info(void)
 {
-    if ( memcmp(t->signature, "XenHVMSeaBIOS", 14) )
-        panic("Bad Xen info signature\n");
+    struct xen_seabios_info *t = (void *)INFO_PHYSICAL_ADDRESS;
 
-    if ( t->length < sizeof(struct xen_seabios_info) )
-        panic("Bad Xen info length\n");
+    if ( memcmp(t->signature, "XenHVMSeaBIOS", 14) ) {
+        dprintf(1, "Bad Xen info signature\n");
+        return NULL;
+    }
+
+    if ( t->length < sizeof(struct xen_seabios_info) ) {
+        dprintf(1, "Bad Xen info length\n");
+        return NULL;
+    }
 
-    if (checksum(t, t->length) != 0)
-        panic("Bad Xen info checksum\n");
+    if (checksum(t, t->length) != 0) {
+        dprintf(1, "Bad Xen info checksum\n");
+        return NULL;
+    }
+    return t;
 }
 
 void xen_preinit(void)
@@ -86,7 +95,10 @@ void xen_preinit(void)
         dprintf(1, "No Xen hypervisor found.\n");
         return;
     }
-    PlatformRunningOn = PF_QEMU|PF_XEN;
+    if (validate_info())
+        PlatformRunningOn = PF_QEMU|PF_XEN;
+    else
+        dprintf(1, "Not enabling Xen support due to lack of Xen info\n");
 }
 
 static int hypercall_xen_version( int cmd, void *arg)
@@ -122,10 +134,14 @@ void xen_hypercall_setup(void)
 
 void xen_biostable_setup(void)
 {
-    struct xen_seabios_info *info = (void *)INFO_PHYSICAL_ADDRESS;
-    void **tables = (void*)info->tables;
+    struct xen_seabios_info *info = validate_info();
+    void **tables;
     int i;
 
+    if (!info)
+        panic("Xen info corrupted\n");
+
+    tables = (void*)info->tables;
     dprintf(1, "xen: copy BIOS tables...\n");
     for (i=0; i<info->tables_nr; i++)
         copy_table(tables[i]);
@@ -136,12 +152,15 @@ void xen_biostable_setup(void)
 void xen_ramsize_preinit(void)
 {
     int i;
-    struct xen_seabios_info *info = (void *)INFO_PHYSICAL_ADDRESS;
-    struct e820entry *e820 = (struct e820entry *)info->e820;
-    validate_info(info);
+    struct xen_seabios_info *info = validate_info();
+    struct e820entry *e820;
+
+    if (!info)
+        panic("Xen info corrupted\n");
 
     dprintf(1, "xen: copy e820...\n");
 
+    e820 = (struct e820entry *)info->e820;
     for (i = 0; i < info->e820_nr; i++) {
         struct e820entry *e = &e820[i];
         e820_add(e->start, e->size, e->type);
-- 
2.34.1


Re: [SeaBIOS] [SeaBIOS PATCH] xen: require Xen info structure at 0x1000 to detect Xen
Posted by Kevin O'Connor 1 week ago
On Fri, Jan 20, 2023 at 11:33:19AM +0000, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> When running under Xen, hvmloader places a table at 0x1000 with the e820
> information and BIOS tables. If this isn't present, SeaBIOS will 
> currently panic.
> 
> We now have support for running Xen guests natively in QEMU/KVM, which
> boots SeaBIOS directly instead of via hvmloader, and does not provide
> the same structure.
> 
> As it happens, this doesn't matter on first boot. because although we
> set PlatformRunningOn to PF_QEMU|PF_XEN, reading it back again still
> gives zero. Presumably because in true Xen, this is all already RAM. But
> in QEMU with a faithfully-emulated PAM config in the host bridge, it's
> still in ROM mode at this point so we don't see what we've just written.
> 
> On reboot, however, the region *is* set to RAM mode and we do see the
> updated value of PlatformRunningOn, do manage to remember that we've
> detected Xen in CPUID, and hit the panic.
> 
> It's not trivial to detect QEMU vs. real Xen at the time xen_preinit()
> runs, because it's so early. We can't even make a XENVER_extraversion
> hypercall to look for hints, because we haven't set up the hypercall
> page (and don't have an allocator to give us a page in which to do so).
> 
> So just make Xen detection contingent on the info structure being
> present. If it wasn't, we were going to panic anyway. That leaves us
> taking the standard QEMU init path for Xen guests in native QEMU,
> which is just fine.

Thanks.  I committed this change.

-Kevin
Re: [SeaBIOS] [SeaBIOS PATCH] xen: require Xen info structure at 0x1000 to detect Xen
Posted by David Woodhouse 6 days, 21 hours ago
On Wed, 2023-02-01 at 21:13 -0500, Kevin O'Connor wrote:
> On Fri, Jan 20, 2023 at 11:33:19AM +0000, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > When running under Xen, hvmloader places a table at 0x1000 with the e820
> > information and BIOS tables. If this isn't present, SeaBIOS will 
> > currently panic.
> > 
> > We now have support for running Xen guests natively in QEMU/KVM, which
> > boots SeaBIOS directly instead of via hvmloader, and does not provide
> > the same structure.
> > 
> > As it happens, this doesn't matter on first boot. because although we
> > set PlatformRunningOn to PF_QEMU|PF_XEN, reading it back again still
> > gives zero. Presumably because in true Xen, this is all already RAM. But
> > in QEMU with a faithfully-emulated PAM config in the host bridge, it's
> > still in ROM mode at this point so we don't see what we've just written.
> > 
> > On reboot, however, the region *is* set to RAM mode and we do see the
> > updated value of PlatformRunningOn, do manage to remember that we've
> > detected Xen in CPUID, and hit the panic.
> > 
> > It's not trivial to detect QEMU vs. real Xen at the time xen_preinit()
> > runs, because it's so early. We can't even make a XENVER_extraversion
> > hypercall to look for hints, because we haven't set up the hypercall
> > page (and don't have an allocator to give us a page in which to do so).
> > 
> > So just make Xen detection contingent on the info structure being
> > present. If it wasn't, we were going to panic anyway. That leaves us
> > taking the standard QEMU init path for Xen guests in native QEMU,
> > which is just fine.
> 
> Thanks.  I committed this change.
> 
> -Kevin

Thanks, Kevin.

I'd like to get the rest of the Xen platform support in to qemu 8.0 if
possible. Which is currently scheduled for March.

Is there likely to be a SeaBIOS release before then which Gerd would
pull into qemu anyway, or should I submit a submodule update to a
snapshot of today's tree? That would just pull in this commit, and the
one other fix that's in the SeaBIOS tree since 1.16.1?


 $ git shortlog rel-1.16.1..
David Woodhouse (1):
      xen: require Xen info structure at 0x1000 to detect Xen

Qi Zhou (1):
      usb: fix wrong init of keyboard/mouse's if first interface is not boot protocol



Re: [SeaBIOS] Re: [SeaBIOS PATCH] xen: require Xen info structure at 0x1000 to detect Xen
Posted by Gerd Hoffmann 6 days, 20 hours ago
> Thanks, Kevin.
> 
> I'd like to get the rest of the Xen platform support in to qemu 8.0 if
> possible. Which is currently scheduled for March.
> 
> Is there likely to be a SeaBIOS release before then which Gerd would
> pull into qemu anyway, or should I submit a submodule update to a
> snapshot of today's tree? That would just pull in this commit, and the
> one other fix that's in the SeaBIOS tree since 1.16.1?

Tagging 1.16.2 in time for the qemu 8.0 should not be a problem given
that we have only bugfixes in master.  Roughly around soft freeze is
probably a good time for that.

take care,
  Gerd
Re: [SeaBIOS] [SeaBIOS PATCH] xen: require Xen info structure at 0x1000 to detect Xen
Posted by Kevin O'Connor 1 week, 6 days ago
On Fri, Jan 20, 2023 at 11:33:19AM +0000, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> When running under Xen, hvmloader places a table at 0x1000 with the e820
> information and BIOS tables. If this isn't present, SeaBIOS will 
> currently panic.
> 
> We now have support for running Xen guests natively in QEMU/KVM, which
> boots SeaBIOS directly instead of via hvmloader, and does not provide
> the same structure.
> 
> As it happens, this doesn't matter on first boot. because although we
> set PlatformRunningOn to PF_QEMU|PF_XEN, reading it back again still
> gives zero. Presumably because in true Xen, this is all already RAM. But
> in QEMU with a faithfully-emulated PAM config in the host bridge, it's
> still in ROM mode at this point so we don't see what we've just written.
> 
> On reboot, however, the region *is* set to RAM mode and we do see the
> updated value of PlatformRunningOn, do manage to remember that we've
> detected Xen in CPUID, and hit the panic.
> 
> It's not trivial to detect QEMU vs. real Xen at the time xen_preinit()
> runs, because it's so early. We can't even make a XENVER_extraversion
> hypercall to look for hints, because we haven't set up the hypercall
> page (and don't have an allocator to give us a page in which to do so).
> 
> So just make Xen detection contingent on the info structure being
> present. If it wasn't, we were going to panic anyway. That leaves us
> taking the standard QEMU init path for Xen guests in native QEMU,
> which is just fine.
> 
> Untested on actual Xen but ObviouslyCorrect™.

Thanks.  I don't have a way to test this, but it looks fine to me.
I'll give a few more days to see if others have comments, and
otherwise look to commit.

Cheers,
-Kevin

Re: [SeaBIOS PATCH] xen: require Xen info structure at 0x1000 to detect Xen
Posted by Paul Durrant 1 week, 6 days ago
On 20/01/2023 11:33, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> When running under Xen, hvmloader places a table at 0x1000 with the e820
> information and BIOS tables. If this isn't present, SeaBIOS will
> currently panic.
> 
> We now have support for running Xen guests natively in QEMU/KVM, which
> boots SeaBIOS directly instead of via hvmloader, and does not provide
> the same structure.
> 
> As it happens, this doesn't matter on first boot. because although we
> set PlatformRunningOn to PF_QEMU|PF_XEN, reading it back again still
> gives zero. Presumably because in true Xen, this is all already RAM. But
> in QEMU with a faithfully-emulated PAM config in the host bridge, it's
> still in ROM mode at this point so we don't see what we've just written.
> 
> On reboot, however, the region *is* set to RAM mode and we do see the
> updated value of PlatformRunningOn, do manage to remember that we've
> detected Xen in CPUID, and hit the panic.
> 
> It's not trivial to detect QEMU vs. real Xen at the time xen_preinit()
> runs, because it's so early. We can't even make a XENVER_extraversion
> hypercall to look for hints, because we haven't set up the hypercall
> page (and don't have an allocator to give us a page in which to do so).
> 
> So just make Xen detection contingent on the info structure being
> present. If it wasn't, we were going to panic anyway. That leaves us
> taking the standard QEMU init path for Xen guests in native QEMU,
> which is just fine.
> 
> Untested on actual Xen but ObviouslyCorrect™.

Works for me...

Tested-by: Paul Durrant <paul@xen.org>

> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   src/fw/xen.c | 45 ++++++++++++++++++++++++++++++++-------------
>   1 file changed, 32 insertions(+), 13 deletions(-)
> 

Also...

Reviewed-by: Paul Durrant <paul@xen.org>