[SeaBIOS] [PATCH 2/6] paravirt/qemu: virtio-mmio device discovery

Gerd Hoffmann posted 6 patches 9 weeks ago

[SeaBIOS] [PATCH 2/6] paravirt/qemu: virtio-mmio device discovery

Posted by Gerd Hoffmann 9 weeks ago
Use bootorder fw_cfg file to find bootable virtio-mmio devices.
Also add a new virtio-mmio.c source file, providing a function
to register virtio-mmio devices.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 Makefile             |  2 +-
 src/hw/virtio-mmio.h |  6 ++++++
 src/fw/paravirt.c    | 24 ++++++++++++++++++++++++
 src/hw/virtio-mmio.c | 30 ++++++++++++++++++++++++++++++
 4 files changed, 61 insertions(+), 1 deletion(-)
 create mode 100644 src/hw/virtio-mmio.h
 create mode 100644 src/hw/virtio-mmio.c

diff --git a/Makefile b/Makefile
index 5f7d5370198a..985ef591a13b 100644
--- a/Makefile
+++ b/Makefile
@@ -43,7 +43,7 @@ SRC32FLAT=$(SRCBOTH) post.c e820map.c malloc.c romfile.c x86.c optionroms.c \
     fw/coreboot.c fw/lzmadecode.c fw/multiboot.c fw/csm.c fw/biostables.c \
     fw/paravirt.c fw/shadow.c fw/pciinit.c fw/smm.c fw/smp.c fw/mtrr.c fw/xen.c \
     fw/acpi.c fw/mptable.c fw/pirtable.c fw/smbios.c fw/romfile_loader.c \
-    hw/virtio-ring.c hw/virtio-pci.c hw/virtio-blk.c hw/virtio-scsi.c \
+    hw/virtio-ring.c hw/virtio-pci.c hw/virtio-mmio.c hw/virtio-blk.c hw/virtio-scsi.c \
     hw/tpm_drivers.c hw/nvme.c
 SRC32SEG=string.c output.c pcibios.c apm.c stacks.c hw/pci.c hw/serialio.c
 DIRS=src src/hw src/fw vgasrc
diff --git a/src/hw/virtio-mmio.h b/src/hw/virtio-mmio.h
new file mode 100644
index 000000000000..751984241f49
--- /dev/null
+++ b/src/hw/virtio-mmio.h
@@ -0,0 +1,6 @@
+#ifndef _VIRTIO_MMIO_H
+#define _VIRTIO_MMIO_H
+
+void virtio_mmio_register(u64 mmio);
+
+#endif /* _VIRTIO_MMIO_H */
diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
index 119280c574fd..aaaf8b8dad31 100644
--- a/src/fw/paravirt.c
+++ b/src/fw/paravirt.c
@@ -15,6 +15,7 @@
 #include "hw/pcidevice.h" // pci_probe_devices
 #include "hw/pci_regs.h" // PCI_DEVICE_ID
 #include "hw/serialio.h" // PORT_SERIAL1
+#include "hw/virtio-mmio.h" // virtio_mmio_register
 #include "hw/rtc.h" // CMOS_*
 #include "malloc.h" // malloc_tmp
 #include "output.h" // dprintf
@@ -192,6 +193,27 @@ static void msr_feature_control_setup(void)
         wrmsr_smp(MSR_IA32_FEATURE_CONTROL, feature_control_bits);
 }
 
+static void qemu_probe_virtio_mmio(void)
+{
+    char *data, *next;
+    int size;
+    u64 mmio;
+
+    data = romfile_loadfile("bootorder", &size);
+    while (data) {
+        next = strchr(data, '\n');
+        if (next) {
+            *next = '\0';
+            next++;
+        }
+        if (memcmp("/virtio-mmio@", data, 13) == 0) {
+            mmio = strtol(data+13, 16);
+            virtio_mmio_register(mmio);
+        }
+        data = next;
+    }
+}
+
 void
 qemu_platform_setup(void)
 {
@@ -217,6 +239,8 @@ qemu_platform_setup(void)
     msr_feature_control_setup();
     smp_setup();
 
+    qemu_probe_virtio_mmio();
+
     // Create bios tables
     if (MaxCountCPUs <= 255) {
         pirtable_setup();
diff --git a/src/hw/virtio-mmio.c b/src/hw/virtio-mmio.c
new file mode 100644
index 000000000000..0f320921df30
--- /dev/null
+++ b/src/hw/virtio-mmio.c
@@ -0,0 +1,30 @@
+#include "config.h" // CONFIG_DEBUG_LEVEL
+#include "malloc.h" // free
+#include "output.h" // dprintf
+#include "virtio-pci.h"
+#include "virtio-ring.h"
+
+/* qemu microvm supports 8 virtio-mmio devices */
+static u64 devs[8];
+
+void virtio_mmio_register(u64 mmio)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(devs); i++) {
+        if (devs[i] == mmio) {
+            /*
+             * This can happen in case we have multiple scsi devices
+             * attached to a single virtio-scsi controller
+             */
+            dprintf(3, "virtio-mmio: duplicate device at 0x%llx, ignoring\n", mmio);
+            return;
+        }
+        if (devs[i] == 0) {
+            dprintf(1, "virtio-mmio: device at 0x%llx\n", mmio);
+            devs[i] = mmio;
+            return;
+        }
+    }
+    dprintf(1, "virtio-mmio: device list full\n");
+}
-- 
2.18.2
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH 2/6] paravirt/qemu: virtio-mmio device discovery

Posted by Kevin O'Connor 9 weeks ago
On Thu, Mar 19, 2020 at 08:39:33AM +0100, Gerd Hoffmann wrote:
> Use bootorder fw_cfg file to find bootable virtio-mmio devices.
> Also add a new virtio-mmio.c source file, providing a function
> to register virtio-mmio devices.

Can you expand on this?  I'm not sure I understand what this patch
does.  It seems the bootorder file is used for device detection, but
that seems odd.

-Kevin


> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  Makefile             |  2 +-
>  src/hw/virtio-mmio.h |  6 ++++++
>  src/fw/paravirt.c    | 24 ++++++++++++++++++++++++
>  src/hw/virtio-mmio.c | 30 ++++++++++++++++++++++++++++++
>  4 files changed, 61 insertions(+), 1 deletion(-)
>  create mode 100644 src/hw/virtio-mmio.h
>  create mode 100644 src/hw/virtio-mmio.c
> 
> diff --git a/Makefile b/Makefile
> index 5f7d5370198a..985ef591a13b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -43,7 +43,7 @@ SRC32FLAT=$(SRCBOTH) post.c e820map.c malloc.c romfile.c x86.c optionroms.c \
>      fw/coreboot.c fw/lzmadecode.c fw/multiboot.c fw/csm.c fw/biostables.c \
>      fw/paravirt.c fw/shadow.c fw/pciinit.c fw/smm.c fw/smp.c fw/mtrr.c fw/xen.c \
>      fw/acpi.c fw/mptable.c fw/pirtable.c fw/smbios.c fw/romfile_loader.c \
> -    hw/virtio-ring.c hw/virtio-pci.c hw/virtio-blk.c hw/virtio-scsi.c \
> +    hw/virtio-ring.c hw/virtio-pci.c hw/virtio-mmio.c hw/virtio-blk.c hw/virtio-scsi.c \
>      hw/tpm_drivers.c hw/nvme.c
>  SRC32SEG=string.c output.c pcibios.c apm.c stacks.c hw/pci.c hw/serialio.c
>  DIRS=src src/hw src/fw vgasrc
> diff --git a/src/hw/virtio-mmio.h b/src/hw/virtio-mmio.h
> new file mode 100644
> index 000000000000..751984241f49
> --- /dev/null
> +++ b/src/hw/virtio-mmio.h
> @@ -0,0 +1,6 @@
> +#ifndef _VIRTIO_MMIO_H
> +#define _VIRTIO_MMIO_H
> +
> +void virtio_mmio_register(u64 mmio);
> +
> +#endif /* _VIRTIO_MMIO_H */
> diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
> index 119280c574fd..aaaf8b8dad31 100644
> --- a/src/fw/paravirt.c
> +++ b/src/fw/paravirt.c
> @@ -15,6 +15,7 @@
>  #include "hw/pcidevice.h" // pci_probe_devices
>  #include "hw/pci_regs.h" // PCI_DEVICE_ID
>  #include "hw/serialio.h" // PORT_SERIAL1
> +#include "hw/virtio-mmio.h" // virtio_mmio_register
>  #include "hw/rtc.h" // CMOS_*
>  #include "malloc.h" // malloc_tmp
>  #include "output.h" // dprintf
> @@ -192,6 +193,27 @@ static void msr_feature_control_setup(void)
>          wrmsr_smp(MSR_IA32_FEATURE_CONTROL, feature_control_bits);
>  }
>  
> +static void qemu_probe_virtio_mmio(void)
> +{
> +    char *data, *next;
> +    int size;
> +    u64 mmio;
> +
> +    data = romfile_loadfile("bootorder", &size);
> +    while (data) {
> +        next = strchr(data, '\n');
> +        if (next) {
> +            *next = '\0';
> +            next++;
> +        }
> +        if (memcmp("/virtio-mmio@", data, 13) == 0) {
> +            mmio = strtol(data+13, 16);
> +            virtio_mmio_register(mmio);
> +        }
> +        data = next;
> +    }
> +}
> +
>  void
>  qemu_platform_setup(void)
>  {
> @@ -217,6 +239,8 @@ qemu_platform_setup(void)
>      msr_feature_control_setup();
>      smp_setup();
>  
> +    qemu_probe_virtio_mmio();
> +
>      // Create bios tables
>      if (MaxCountCPUs <= 255) {
>          pirtable_setup();
> diff --git a/src/hw/virtio-mmio.c b/src/hw/virtio-mmio.c
> new file mode 100644
> index 000000000000..0f320921df30
> --- /dev/null
> +++ b/src/hw/virtio-mmio.c
> @@ -0,0 +1,30 @@
> +#include "config.h" // CONFIG_DEBUG_LEVEL
> +#include "malloc.h" // free
> +#include "output.h" // dprintf
> +#include "virtio-pci.h"
> +#include "virtio-ring.h"
> +
> +/* qemu microvm supports 8 virtio-mmio devices */
> +static u64 devs[8];
> +
> +void virtio_mmio_register(u64 mmio)
> +{
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(devs); i++) {
> +        if (devs[i] == mmio) {
> +            /*
> +             * This can happen in case we have multiple scsi devices
> +             * attached to a single virtio-scsi controller
> +             */
> +            dprintf(3, "virtio-mmio: duplicate device at 0x%llx, ignoring\n", mmio);
> +            return;
> +        }
> +        if (devs[i] == 0) {
> +            dprintf(1, "virtio-mmio: device at 0x%llx\n", mmio);
> +            devs[i] = mmio;
> +            return;
> +        }
> +    }
> +    dprintf(1, "virtio-mmio: device list full\n");
> +}
> -- 
> 2.18.2
> _______________________________________________
> SeaBIOS mailing list -- seabios@seabios.org
> To unsubscribe send an email to seabios-leave@seabios.org
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH 2/6] paravirt/qemu: virtio-mmio device discovery

Posted by Gerd Hoffmann 9 weeks ago
On Thu, Mar 19, 2020 at 07:48:19PM -0400, Kevin O'Connor wrote:
> On Thu, Mar 19, 2020 at 08:39:33AM +0100, Gerd Hoffmann wrote:
> > Use bootorder fw_cfg file to find bootable virtio-mmio devices.
> > Also add a new virtio-mmio.c source file, providing a function
> > to register virtio-mmio devices.
> 
> Can you expand on this?  I'm not sure I understand what this patch
> does.  It seems the bootorder file is used for device detection, but
> that seems odd.

Yes, it does exactly that.

virtio-mmio isn't a discoverable bus, so we need some way to find the
devices.  Right now qemu simply appends device information to the kernel
command line.  Seabios can look there too.  That works only for direct
kernel boot though, so I'm trying to find something better.  The
alternative is doing it like aarch64: declare devices in device tree, or
in ACPI tables.  device tree doesn't work very well in x86.  ACPI works
fine, so I think this is the way to go.

FYI: qemu patch series is here:
   https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg06144.html

seabios hasn't an ACPI parser though.  (ab)using the bootorder file to
find the virtio-mmio devices is the only other option we have, and it
looked alot simpler than adding an ACPI parser ...

cheers,
  Gerd
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH 2/6] paravirt/qemu: virtio-mmio device discovery

Posted by Kevin O'Connor 9 weeks ago
On Fri, Mar 20, 2020 at 07:13:02AM +0100, Gerd Hoffmann wrote:
> On Thu, Mar 19, 2020 at 07:48:19PM -0400, Kevin O'Connor wrote:
> > On Thu, Mar 19, 2020 at 08:39:33AM +0100, Gerd Hoffmann wrote:
> > > Use bootorder fw_cfg file to find bootable virtio-mmio devices.
> > > Also add a new virtio-mmio.c source file, providing a function
> > > to register virtio-mmio devices.
> > 
> > Can you expand on this?  I'm not sure I understand what this patch
> > does.  It seems the bootorder file is used for device detection, but
> > that seems odd.
> 
> Yes, it does exactly that.
> 
> virtio-mmio isn't a discoverable bus, so we need some way to find the
> devices.  Right now qemu simply appends device information to the kernel
> command line.  Seabios can look there too.  That works only for direct
> kernel boot though, so I'm trying to find something better.  The
> alternative is doing it like aarch64: declare devices in device tree, or
> in ACPI tables.  device tree doesn't work very well in x86.

I'd be leery of overloading the bootorder file in this way.  The QEMU
to firmware interface is already complex and additional quirks like
this make it harder to document and understand that interface.

Can we just introduce a new fw_cfg file (eg, /etc/virtio-mmio-devices)
with the required information?

>ACPI works
> fine, so I think this is the way to go.
> 
> FYI: qemu patch series is here:
>    https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg06144.html
> 
> seabios hasn't an ACPI parser though.  (ab)using the bootorder file to
> find the virtio-mmio devices is the only other option we have, and it
> looked alot simpler than adding an ACPI parser ...

If you mean a DSDT parser then I suspect a full implementation in
SeaBIOS would be too burdensome.  However, it might be possible to
introduce a minimal DSDT parser (eg, one that only supports extracting
constants).

FWIW, it's unclear to me if adding a parser would be a net gain when
compared to adding adhoc fw_cfg files.  In particular, it seems some
users don't want to enable acpi.

Cheers,
-Kevin
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH 2/6] paravirt/qemu: virtio-mmio device discovery

Posted by Gerd Hoffmann 9 weeks ago
  Hi,

> If you mean a DSDT parser then I suspect a full implementation in
> SeaBIOS would be too burdensome.  However, it might be possible to
> introduce a minimal DSDT parser (eg, one that only supports extracting
> constants).

https://git.kraxel.org/cgit/seabios/commit/?h=dsdt&id=d7cdf49ec624f42fa6b42b7f590dd47552d4c92c

~500 lines of code.

qemu microvm:

[ ... ]
dsdt at 0x7ffe0040 (len 335)
    dsdt: RTC, hid PNP0b00, io 0x70 -> 0x71, 1 io+, irq 8
    dsdt: COM1, hid PNP0501, io 0x3f8 -> 0x3ff, irq 4
    dsdt: FWCF, hid QEMU0002, io 0x510 -> 0x51b, sta 0xb
    dsdt: VR07, hid LNRO0005, mem 0xc0000e00 -> 0xc0000fff, irq 23
    dsdt: VR06, hid LNRO0005, mem 0xc0000c00 -> 0xc0000dff, irq 22
[ ... ]

_HID == LNRO0005 are the virtio-mmio devices, so this can work.

qemu pc:
[ ... ]
dsdt at 0x07fe0040 (len 5131)
    dsdt: PCI0, hid PNP0a03
    dsdt: HPET, hid PNP0103, mem 0xfed00000 -> 0xfed003ff, sta method
    dsdt: PX13
    dsdt: ISA
    dsdt: RTC, hid PNP0b00, io 0x70 -> 0x71, 1 io+, irq 8
    dsdt: KBD, hid PNP0303, io 0x60 -> 0x60, 1 io+, irq 1, sta method
    dsdt: MOU, hid PNP0f13, irq 12, sta method
    dsdt: FLPA
    dsdt: FLPA
    dsdt: LPT, hid PNP0400, io 0x378 -> 0x37f, irq 7, sta method
    dsdt: COM1, hid PNP0501, io 0x3f8 -> 0x3ff, irq 4, sta method
    dsdt: COM2, hid PNP0501, io 0x2f8 -> 0x2ff, irq 3, sta method
    dsdt: LNKA, hid PNP0c0f, sta method
    dsdt: LNKB, hid PNP0c0f, sta method
    dsdt: LNKC, hid PNP0c0f, sta method
    dsdt: LNKD, hid PNP0c0f, sta method
    dsdt: LNKS, hid PNP0c0f, sta method
    dsdt: \_SB\PCI0\PRES, hid PNP0a06, io 0xaf00 -> 0xaf0b
    dsdt: \_SB\CPUS, hid ACPI0010
    dsdt: GPE0, hid PNP0A06, io 0xafe0 -> 0xafe3, sta 0xb
    dsdt: PHPR, hid PNP0A06, io 0xae00 -> 0xae13, sta 0xb
    dsdt: FWCF, hid QEMU0002, io 0x510 -> 0x51b, sta 0xb
    dsdt: S00
[ ... ]

If we have this _anyway_ we might use it to figure whenever specific
(isa) hardware is present, keyboard for example.  We need to fix qemu
to not use _STA methods for no good reason, like this one ...

        Device (KBD)
        {
            Name (_HID, EisaId ("PNP0303") 
            Method (_STA, 0, NotSerialized)
            {
                Return (0x0F)
            }

... so it can be checked without a full-blown acpi interpreter.

> FWIW, it's unclear to me if adding a parser would be a net gain when
> compared to adding adhoc fw_cfg files.  In particular, it seems some
> users don't want to enable acpi.

Well, at least for the microvm disk-boot use case there is not really a
way around ACPI, because otherwise the kernel doesn't find the
virtio-mmio devices.

With direct kernel boot (qemu -kernel) one can add the virtio-mmio
devices to the linux command line instead of depending on acpi, but in
that case seabios doesn't need to find virtio-mmio devices anyway so it
would be happy without acpi too.

cheers,
  Gerd
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH 2/6] paravirt/qemu: virtio-mmio device discovery

Posted by Kevin O'Connor 8 weeks ago
On Wed, Mar 25, 2020 at 10:35:50AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > If you mean a DSDT parser then I suspect a full implementation in
> > SeaBIOS would be too burdensome.  However, it might be possible to
> > introduce a minimal DSDT parser (eg, one that only supports extracting
> > constants).
> 
> https://git.kraxel.org/cgit/seabios/commit/?h=dsdt&id=d7cdf49ec624f42fa6b42b7f590dd47552d4c92c
> 
> ~500 lines of code.

Nice.

Could we just parse the static device tree into a local data structure
though?  Something like:

struct hlist_head DSDTTree;

struct dsdt_entry {
    struct hlist_node node;
    char *name;  // eg, "_SB.PCI0.ISA.COM1._HID"
    int type; // eg: Device, Name, Integer, String, ResourceTemplate, other
    void *value; // points to integer, string, resource template, or NULL
};

Once the tree is parsed, it should be simple to walk the in-memory
linked-list to find desired values later.

> If we have this _anyway_ we might use it to figure whenever specific
> (isa) hardware is present, keyboard for example.  We need to fix qemu
> to not use _STA methods for no good reason, like this one ...
> 
>         Device (KBD)
>         {
>             Name (_HID, EisaId ("PNP0303") 
>             Method (_STA, 0, NotSerialized)
>             {
>                 Return (0x0F)
>             }
> 
> ... so it can be checked without a full-blown acpi interpreter.

FWIW, it should not be difficult to detect the simple case above and
treat it as simple integer locally.  As before, it does raise the
question of how far one would want to go down that path though.

Cheers,
-Kevin
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH 2/6] paravirt/qemu: virtio-mmio device discovery

Posted by Gerd Hoffmann 8 weeks ago
  Hi,

> Could we just parse the static device tree into a local data structure
> though?

Sure.

> Something like:
> 
> struct hlist_head DSDTTree;
> 
> struct dsdt_entry {
>     struct hlist_node node;
>     char *name;  // eg, "_SB.PCI0.ISA.COM1._HID"
>     int type; // eg: Device, Name, Integer, String, ResourceTemplate, other
>     void *value; // points to integer, string, resource template, or NULL
> };
> 
> Once the tree is parsed, it should be simple to walk the in-memory
> linked-list to find desired values later.

I was thinking more about storing a list of devices with parsed data,
i.e. basically put into a linked list what the current code collects
in the parse_dev struct.

Then we can easily lookup the virtio-mmio devices later.  Maybe also
check for isa devices (don't bother waiting for ps2 probe timeout if
acpi says there isn't a keyboard ...).  I don't see any other use
cases.

> FWIW, it should not be difficult to detect the simple case above and
> treat it as simple integer locally.  As before, it does raise the
> question of how far one would want to go down that path though.

I don't want handle methods.

cheers,
  Gerd
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH 2/6] paravirt/qemu: virtio-mmio device discovery

Posted by Kevin O'Connor 8 weeks ago
On Fri, Mar 27, 2020 at 09:12:06AM +0100, Gerd Hoffmann wrote:
> > Something like:
> > 
> > struct hlist_head DSDTTree;
> > 
> > struct dsdt_entry {
> >     struct hlist_node node;
> >     char *name;  // eg, "_SB.PCI0.ISA.COM1._HID"
> >     int type; // eg: Device, Name, Integer, String, ResourceTemplate, other
> >     void *value; // points to integer, string, resource template, or NULL
> > };
> > 
> > Once the tree is parsed, it should be simple to walk the in-memory
> > linked-list to find desired values later.
> 
> I was thinking more about storing a list of devices with parsed data,
> i.e. basically put into a linked list what the current code collects
> in the parse_dev struct.

FWIW, I suspect it will be painful to grow 'struct parse_dev' if there
are future users.  (For example, if there is more than one memory
range needed, if it is necessary to check for the existence of some
name other than _STA, etc. .)  In contrast, I suspect a few helper
functions that can walk the tree would be sufficient to extract the
info currently in 'struct parse_dev' as needed.

> Then we can easily lookup the virtio-mmio devices later.  Maybe also
> check for isa devices (don't bother waiting for ps2 probe timeout if
> acpi says there isn't a keyboard ...).  I don't see any other use
> cases.

It would be helpful to extract the location of builtin sdcards from
the dsdt (currently, the "etc/sdcard" cbfs file is used instead).

In the past, it may have been useful to extract information on ATA DMA
capabilities from the dsdt.  (Though, now, it seems unlikely that is
worthwhile doing.)

-Kevin
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH 2/6] paravirt/qemu: virtio-mmio device discovery

Posted by Gerd Hoffmann 8 weeks ago
On Fri, Mar 27, 2020 at 09:15:19AM -0400, Kevin O'Connor wrote:
> On Fri, Mar 27, 2020 at 09:12:06AM +0100, Gerd Hoffmann wrote:
> > > Something like:
> > > 
> > > struct hlist_head DSDTTree;
> > > 
> > > struct dsdt_entry {
> > >     struct hlist_node node;
> > >     char *name;  // eg, "_SB.PCI0.ISA.COM1._HID"
> > >     int type; // eg: Device, Name, Integer, String, ResourceTemplate, other
> > >     void *value; // points to integer, string, resource template, or NULL
> > > };
> > > 
> > > Once the tree is parsed, it should be simple to walk the in-memory
> > > linked-list to find desired values later.
> > 
> > I was thinking more about storing a list of devices with parsed data,
> > i.e. basically put into a linked list what the current code collects
> > in the parse_dev struct.
> 
> FWIW, I suspect it will be painful to grow 'struct parse_dev' if there
> are future users.  (For example, if there is more than one memory
> range needed, if it is necessary to check for the existence of some
> name other than _STA, etc. .)  In contrast, I suspect a few helper
> functions that can walk the tree would be sufficient to extract the
> info currently in 'struct parse_dev' as needed.

Went with a hybrid approach now.  parse_dev is still there, but for the
most part part it stores pointers into the dsdt table and parsing
happens later as needed.

sneak preview @ https://git.kraxel.org/cgit/seabios/log/?h=dsdt

> > Then we can easily lookup the virtio-mmio devices later.  Maybe also
> > check for isa devices (don't bother waiting for ps2 probe timeout if
> > acpi says there isn't a keyboard ...).  I don't see any other use
> > cases.
> 
> It would be helpful to extract the location of builtin sdcards from
> the dsdt (currently, the "etc/sdcard" cbfs file is used instead).

That'll be used with coreboot, right?  How do sdcard dsdt entries look
like?

cheers,
  Gerd
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH 2/6] paravirt/qemu: virtio-mmio device discovery

Posted by Kevin O'Connor 8 weeks ago
On Tue, Mar 31, 2020 at 04:53:02PM +0200, Gerd Hoffmann wrote:
> On Fri, Mar 27, 2020 at 09:15:19AM -0400, Kevin O'Connor wrote:
> > On Fri, Mar 27, 2020 at 09:12:06AM +0100, Gerd Hoffmann wrote:
> > > > Something like:
> > > > 
> > > > struct hlist_head DSDTTree;
> > > > 
> > > > struct dsdt_entry {
> > > >     struct hlist_node node;
> > > >     char *name;  // eg, "_SB.PCI0.ISA.COM1._HID"
> > > >     int type; // eg: Device, Name, Integer, String, ResourceTemplate, other
> > > >     void *value; // points to integer, string, resource template, or NULL
> > > > };
> > > > 
> > > > Once the tree is parsed, it should be simple to walk the in-memory
> > > > linked-list to find desired values later.
> > > 
> > > I was thinking more about storing a list of devices with parsed data,
> > > i.e. basically put into a linked list what the current code collects
> > > in the parse_dev struct.
> > 
> > FWIW, I suspect it will be painful to grow 'struct parse_dev' if there
> > are future users.  (For example, if there is more than one memory
> > range needed, if it is necessary to check for the existence of some
> > name other than _STA, etc. .)  In contrast, I suspect a few helper
> > functions that can walk the tree would be sufficient to extract the
> > info currently in 'struct parse_dev' as needed.
> 
> Went with a hybrid approach now.  parse_dev is still there, but for the
> most part part it stores pointers into the dsdt table and parsing
> happens later as needed.
> 
> sneak preview @ https://git.kraxel.org/cgit/seabios/log/?h=dsdt

Thanks.  FWIW, I think the code would be simpler if it first parsed
the tree into an internal structure, and then searched for _STA, _HID,
etc.  Attempting to do both at the same time seems to complicate the
code.

> > > Then we can easily lookup the virtio-mmio devices later.  Maybe also
> > > check for isa devices (don't bother waiting for ps2 probe timeout if
> > > acpi says there isn't a keyboard ...).  I don't see any other use
> > > cases.
> > 
> > It would be helpful to extract the location of builtin sdcards from
> > the dsdt (currently, the "etc/sdcard" cbfs file is used instead).
> 
> That'll be used with coreboot, right?  How do sdcard dsdt entries look
> like?

Yes, with coreboot.  I don't recall the details right now - perhaps
Matt or John recall?

-Kevin
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH 2/6] paravirt/qemu: virtio-mmio device discovery

Posted by Gerd Hoffmann 7 weeks ago
  Hi,

> > Went with a hybrid approach now.  parse_dev is still there, but for the
> > most part part it stores pointers into the dsdt table and parsing
> > happens later as needed.
> > 
> > sneak preview @ https://git.kraxel.org/cgit/seabios/log/?h=dsdt
> 
> Thanks.  FWIW, I think the code would be simpler if it first parsed
> the tree into an internal structure, and then searched for _STA, _HID,
> etc.

I don't think so.  When just storing everything the data structures
needed become more complicated, we'll need a tree then and can't work
with a simple linked list for the devices found.

But, yes, we don't need to actually decode anything while parsing the
tree (except for the namestrings).  We can simply store pointers into
the aml and decode things later when extracting information.

I'll send the full patch series in a moment.

cheers,
  Gerd
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH 2/6] paravirt/qemu: virtio-mmio device discovery

Posted by Matt DeVillier 8 weeks ago
On Tue, Mar 31, 2020 at 12:29 PM Kevin O'Connor <kevin@koconnor.net> wrote:
>
> On Tue, Mar 31, 2020 at 04:53:02PM +0200, Gerd Hoffmann wrote:
> > On Fri, Mar 27, 2020 at 09:15:19AM -0400, Kevin O'Connor wrote:
> > > On Fri, Mar 27, 2020 at 09:12:06AM +0100, Gerd Hoffmann wrote:
> > > > > Something like:
> > > > >
> > > > > struct hlist_head DSDTTree;
> > > > >
> > > > > struct dsdt_entry {
> > > > >     struct hlist_node node;
> > > > >     char *name;  // eg, "_SB.PCI0.ISA.COM1._HID"
> > > > >     int type; // eg: Device, Name, Integer, String, ResourceTemplate, other
> > > > >     void *value; // points to integer, string, resource template, or NULL
> > > > > };
> > > > >
> > > > > Once the tree is parsed, it should be simple to walk the in-memory
> > > > > linked-list to find desired values later.
> > > >
> > > > I was thinking more about storing a list of devices with parsed data,
> > > > i.e. basically put into a linked list what the current code collects
> > > > in the parse_dev struct.
> > >
> > > FWIW, I suspect it will be painful to grow 'struct parse_dev' if there
> > > are future users.  (For example, if there is more than one memory
> > > range needed, if it is necessary to check for the existence of some
> > > name other than _STA, etc. .)  In contrast, I suspect a few helper
> > > functions that can walk the tree would be sufficient to extract the
> > > info currently in 'struct parse_dev' as needed.
> >
> > Went with a hybrid approach now.  parse_dev is still there, but for the
> > most part part it stores pointers into the dsdt table and parsing
> > happens later as needed.
> >
> > sneak preview @ https://git.kraxel.org/cgit/seabios/log/?h=dsdt
>
> Thanks.  FWIW, I think the code would be simpler if it first parsed
> the tree into an internal structure, and then searched for _STA, _HID,
> etc.  Attempting to do both at the same time seems to complicate the
> code.
>
> > > > Then we can easily lookup the virtio-mmio devices later.  Maybe also
> > > > check for isa devices (don't bother waiting for ps2 probe timeout if
> > > > acpi says there isn't a keyboard ...).  I don't see any other use
> > > > cases.
> > >
> > > It would be helpful to extract the location of builtin sdcards from
> > > the dsdt (currently, the "etc/sdcard" cbfs file is used instead).
> >
> > That'll be used with coreboot, right?  How do sdcard dsdt entries look
> > like?
>
> Yes, with coreboot.  I don't recall the details right now - perhaps
> Matt or John recall?

currently, the etc/sdcard entries point to the PCI BAR0 for the
eMMC/SD controllers on Baytrail/Braswell devices which coreboot puts
into ACPI mode. Those values are somewhat build-specific (ie, they
differ between the stock Google firmware and upstream coreboot), and
are extracted from the cbmem log of a booted system. They aren't
directly contained in the DSDT anywhere either -- you'll just find
references to BAR0 inside the EMMC/SDCD objects, which is a GNVS
variable.

>
> -Kevin
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org