[PATCH 0/2] add guest hint for memory layout, take two

[Gerd Hoffmann]

Gerd Hoffmann (2):
  update pci_pad_mem64 handling
  add fwcfg override for pci_pad_mem64

src/util.h        |  1 +
 src/fw/paravirt.c | 13 +++++++++++++
 src/fw/pciinit.c  | 11 ++++++++---
 3 files changed, 22 insertions(+), 3 deletions(-)

[PATCH 1/2] update pci_pad_mem64 handling

[Gerd Hoffmann]

Add a new possible state: '-1' means 'use default'.  In that case
seabios continue to use the current heuristic:  In case memory
above 4G is present enable 64-bit guest friendly configuration.

This allows forcing the one or the other behavior by setting the
pci_pad_mem64 variable beforehand (which is done by another patch).

Signed-off-by: Gerd Hoffmann 
---
 src/fw/pciinit.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
index b3e359d7fe62..df63acc19e24 100644
--- a/src/fw/pciinit.c
+++ b/src/fw/pciinit.c
@@ -55,7 +55,7 @@ u64 pcimem64_end   = BUILD_PCIMEM64_END;
 // Resource allocation limits
 static u64 pci_io_low_end = 0xa000;
 static u64 pci_mem64_top  = 0;
-static u32 pci_pad_mem64  = 0;
+static u32 pci_pad_mem64  = -1;

struct pci_region_entry {
     struct pci_device *dev;
@@ -1202,8 +1202,13 @@ pci_setup(void)
         }
     }

-    if (CPUPhysBits >= 36 && CPULongMode && RamSizeOver4G)
-        pci_pad_mem64 = 1;
+    if (pci_pad_mem64 == -1)
+        // when not set (via fw_cfg) enable in case memory over 4G is present
+        pci_pad_mem64 = RamSizeOver4G ? 1 : 0;
+
+    if (!CPULongMode)
+        // force off for 32-bit CPUs
+        pci_pad_mem64 = 0;

dprintf(1, "=== PCI bus & bridge init ===\n");
     if (pci_probe_host() != 0) {

[PATCH 2/2] add fwcfg override for pci_pad_mem64

[Gerd Hoffmann]

Allow setting pci_pad_mem64 via fw_cfg.
Usage: qemu -fw_cfg name=opt/org.seabios/pci64,string={yes,no}

Signed-off-by: Gerd Hoffmann 
---
 src/util.h        |  1 +
 src/fw/paravirt.c | 13 +++++++++++++
 src/fw/pciinit.c  |  2 +-
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/src/util.h b/src/util.h
index aff8e888955e..03dcafb83794 100644
--- a/src/util.h
+++ b/src/util.h
@@ -135,6 +135,7 @@ void multiboot_init(void);
 extern u64 pcimem_start, pcimem_end;
 extern u64 pcimem64_start, pcimem64_end;
 extern const u8 pci_irqs[4];
+extern u32 pci_pad_mem64;
 void pci_setup(void);
 void pci_resume(void);

diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
index e5d4eca0cb5a..6428a70361c4 100644
--- a/src/fw/paravirt.c
+++ b/src/fw/paravirt.c
@@ -718,6 +718,19 @@ void qemu_cfg_init(void)
         dprintf(1, "Moving pm_base to 0x%x\n", acpi_pm_base);
     }

+    // 64-bit pci mmio window
+    char *hint = romfile_loadfile("opt/org.seabios/pci64", NULL);
+    if (hint) {
+        if (0 == strcmp(hint, "0")  ||
+            0 == strcmp(hint, "no") ||
+            0 == strcmp(hint, "off"))
+            pci_pad_mem64 = 0;
+        if (0 == strcmp(hint, "1")   ||
+            0 == strcmp(hint, "yes") ||
+            0 == strcmp(hint, "on"))
+            pci_pad_mem64 = 1;
+    }
+
     // serial console
     u16 nogfx = 0;
     qemu_cfg_read_entry(&nogfx, QEMU_CFG_NOGRAPHIC, sizeof(nogfx));
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
index df63acc19e24..5b4cf3bd5598 100644
--- a/src/fw/pciinit.c
+++ b/src/fw/pciinit.c
@@ -55,7 +55,7 @@ u64 pcimem64_end   = BUILD_PCIMEM64_END;
 // Resource allocation limits
 static u64 pci_io_low_end = 0xa000;
 static u64 pci_mem64_top  = 0;
-static u32 pci_pad_mem64  = -1;
+u32 pci_pad_mem64  = -1;

struct pci_region_entry {
     struct pci_device *dev;

[PATCH 2/2] add fwcfg override for pci_pad_mem64

[Kevin O'Connor]

On Thu, Nov 21, 2024 at 05:07:50PM +0100, Gerd Hoffmann wrote:
> Allow setting pci_pad_mem64 via fw_cfg.
> Usage: qemu -fw_cfg name=opt/org.seabios/pci64,string={yes,no}
> Signed-off-by: Gerd Hoffmann
>
> src/util.h        |  1 +
>  src/fw/paravirt.c | 13 +++++++++++++
>  src/fw/pciinit.c  |  2 +-
>  3 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/src/util.h b/src/util.h
> index aff8e888955e..03dcafb83794 100644
> --- a/src/util.h
> +++ b/src/util.h
> @@ -135,6 +135,7 @@ void multiboot_init(void);
>  extern u64 pcimem_start, pcimem_end;
>  extern u64 pcimem64_start, pcimem64_end;
>  extern const u8 pci_irqs[4];
> +extern u32 pci_pad_mem64;
>  void pci_setup(void);
>  void pci_resume(void);
>
> diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
> index e5d4eca0cb5a..6428a70361c4 100644
> --- a/src/fw/paravirt.c
> +++ b/src/fw/paravirt.c
> @@ -718,6 +718,19 @@ void qemu_cfg_init(void)
>          dprintf(1, "Moving pm_base to 0x%x\n", acpi_pm_base);
>      }
> // 64-bit pci mmio window
> char *hint = romfile_loadfile("opt/org.seabios/pci64", NULL);
> if (hint) {
>    if (0 == strcmp(hint, "0")  ||
>        0 == strcmp(hint, "no") ||
>        0 == strcmp(hint, "off"))
>        pci_pad_mem64 = 0;
>    if (0 == strcmp(hint, "1")   ||
>        0 == strcmp(hint, "yes") ||
>        0 == strcmp(hint, "on"))
>        pci_pad_mem64 = 1;
> }

Thanks.  In general it seems fine to me.

For what it is worth, the "filename" and format seems a little odd to
me.  We've not used the "opt/org.seabios/" scheme prior.  We can adopt
that scheme going forward I guess.  The "pci64" seems very generic to
me - if I understand the capability, we're padding space between 64bit
memory regions for busses, not enabling 64bit pci.  Alas, I'm not sure
I have a good suggestion for an alternative.

If we want to support human readable values (eg, "on") perhaps it
would be good to add a romfile_readbool() helper to romfile.c (or
similar).

Also, any reason to put this in paravirt.c instead of directly in
pciinit.c?

Thanks again,
-Kevin

[PATCH 2/2] add fwcfg override for pci_pad_mem64

[Gerd Hoffmann]

> For what it is worth, the "filename" and format seems a little odd to
> me.  We've not used the "opt/org.seabios/" scheme prior.  We can adopt
> that scheme going forward I guess.

In qemu places fw_cfg files for all firmwares in 'etc/'.

fw_cfg files which do not come from qemu but are just passed through
because someone adds them to the qemu command line should go into
'opt/<reverse-dns>/<name>'.  So edk2 uses 'opt/org.tianocore/...' for
most options (there are some in 'opt/ovmf/...' for historical reasons).
For seabios-specific options 'opt/org.seabios/...' would be the correct
choice.
> The "pci64" seems very generic to
> me - if I understand the capability, we're padding space between 64bit
> memory regions for busses, not enabling 64bit pci.

It does both.
> If we want to support human readable values (eg, "on") perhaps it
> would be good to add a romfile_readbool() helper to romfile.c (or
> similar).

Makes sense indeed.
> Also, any reason to put this in paravirt.c instead of directly in
> pciinit.c?

Looked reasonable to me because all fw_cfg access is there, but placing
it directly in pciinit.c should work too.

take care,
  Gerd