On Fri, Nov 10, 2023 at 12:04:24PM +0100, Gerd Hoffmann wrote:
> Hi,
>
> > This only changes the placement of the PCI bars. The pci setup code is
> > the only consumer of that variable, guess it makes sense to move the
> > quirk to the pci code (as suggested by Kevin) to clarify this.
>
> i.e. like this:
>
> From d538dc7d4316e557ae302464252444d09de0681d Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Tue, 7 Nov 2023 13:49:31 +0100
> Subject: [PATCH] limit physical address space size
>
> For better compatibility with old linux kernels,
> see source code comment.
>
> Related (same problem in ovmf):
> https://github.com/tianocore/edk2/commit/c1e853769046
>
> Reported-by: Claudio Fontana <cfontana@suse.de>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> src/fw/pciinit.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
> index c7084f5e397e..7aeea61bfd05 100644
> --- a/src/fw/pciinit.c
> +++ b/src/fw/pciinit.c
> @@ -52,6 +52,7 @@ u64 pcimem64_start = BUILD_PCIMEM64_START;
> u64 pcimem64_end = BUILD_PCIMEM64_END;
> u64 pci_io_low_end = 0xa000;
> u32 pci_use_64bit = 0;
> +u32 pci_phys_bits = 0;
FWIW, all these flags are getting a bit confusing. Maybe do something
like the patch below.
>
> struct pci_region_entry {
> struct pci_device *dev;
> @@ -967,7 +968,7 @@ static int pci_bios_check_devices(struct pci_bus *busses)
> if (hotplug_support == HOTPLUG_PCIE)
> resource_optional = pcie_cap && (type == PCI_REGION_TYPE_IO);
> if (hotplug_support && pci_use_64bit && is64 && (type == PCI_REGION_TYPE_PREFMEM))
> - align = (u64)1 << (CPUPhysBits - 11);
> + align = (u64)1 << (pci_phys_bits - 11);
> if (align > sum && hotplug_support && !resource_optional)
> sum = align; /* reserve min size for hot-plug */
> if (size > sum) {
> @@ -1131,12 +1132,12 @@ static void pci_bios_map_devices(struct pci_bus *busses)
> r64_mem.base = le64_to_cpu(romfile_loadint("etc/reserved-memory-end", 0));
> if (r64_mem.base < 0x100000000LL + RamSizeOver4G)
> r64_mem.base = 0x100000000LL + RamSizeOver4G;
> - if (CPUPhysBits) {
> - u64 top = 1LL << CPUPhysBits;
> + if (pci_phys_bits) {
FYI, this is a change in behavior - previously this condition would
have been taken even if CPULongMode or RamSizeOver4G is false. I'm
not sure if this change is intentional. (My example patch below
follows your lead here.)
> + u64 top = 1LL << pci_phys_bits;
> u64 size = (ALIGN(sum_mem, (1LL<<30)) +
> ALIGN(sum_pref, (1LL<<30)));
> if (pci_use_64bit)
> - size = ALIGN(size, (1LL<<(CPUPhysBits-3)));
> + size = ALIGN(size, (1LL<<(pci_phys_bits-3)));
> if (r64_mem.base < top - size) {
> r64_mem.base = top - size;
> }
> @@ -1181,8 +1182,16 @@ pci_setup(void)
>
> dprintf(3, "pci setup\n");
>
> - if (CPUPhysBits >= 36 && CPULongMode && RamSizeOver4G)
> + if (CPUPhysBits >= 36 && CPULongMode && RamSizeOver4G) {
> pci_use_64bit = 1;
> + pci_phys_bits = CPUPhysBits;
> + if (pci_phys_bits > 46) {
> + // Old linux kernels have trouble dealing with more than 46
> + // phys-bits, so avoid that for now. Seems to be a bug in the
> + // virtio-pci driver. Reported: centos-7, ubuntu-18.04
> + pci_phys_bits = 46;
> + }
> + }
>
> dprintf(1, "=== PCI bus & bridge init ===\n");
> if (pci_probe_host() != 0) {
> --
> 2.41.0
>
Possible alternate variable naming:
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
index c7084f5..cd64d6e 100644
--- a/src/fw/pciinit.c
+++ b/src/fw/pciinit.c
@@ -46,12 +46,16 @@ static const char *region_type_name[] = {
[ PCI_REGION_TYPE_PREFMEM ] = "prefmem",
};
+// Memory ranges exported to legacy ACPI type table generation
u64 pcimem_start = BUILD_PCIMEM_START;
u64 pcimem_end = BUILD_PCIMEM_END;
u64 pcimem64_start = BUILD_PCIMEM64_START;
u64 pcimem64_end = BUILD_PCIMEM64_END;
-u64 pci_io_low_end = 0xa000;
-u32 pci_use_64bit = 0;
+
+// Memory resource allocation tracking
+static u64 pci_io_low_end = 0xa000;
+static u32 pci_use_64bit = 0;
+static u64 pci_mem64_top = 0;
struct pci_region_entry {
struct pci_device *dev;
@@ -966,8 +970,9 @@ static int pci_bios_check_devices(struct pci_bus *busses)
int resource_optional = 0;
if (hotplug_support == HOTPLUG_PCIE)
resource_optional = pcie_cap && (type == PCI_REGION_TYPE_IO);
- if (hotplug_support && pci_use_64bit && is64 && (type == PCI_REGION_TYPE_PREFMEM))
- align = (u64)1 << (CPUPhysBits - 11);
+ if (hotplug_support && pci_use_64bit && is64
+ && (type == PCI_REGION_TYPE_PREFMEM))
+ align = pci_mem64_top >> 11;
if (align > sum && hotplug_support && !resource_optional)
sum = align; /* reserve min size for hot-plug */
if (size > sum) {
@@ -1131,14 +1136,13 @@ static void pci_bios_map_devices(struct pci_bus *busses)
r64_mem.base = le64_to_cpu(romfile_loadint("etc/reserved-memory-end", 0));
if (r64_mem.base < 0x100000000LL + RamSizeOver4G)
r64_mem.base = 0x100000000LL + RamSizeOver4G;
- if (CPUPhysBits) {
- u64 top = 1LL << CPUPhysBits;
+ if (pci_mem64_top) {
u64 size = (ALIGN(sum_mem, (1LL<<30)) +
ALIGN(sum_pref, (1LL<<30)));
if (pci_use_64bit)
- size = ALIGN(size, (1LL<<(CPUPhysBits-3)));
- if (r64_mem.base < top - size) {
- r64_mem.base = top - size;
+ size = ALIGN(size, pci_mem64_top >> 3);
+ if (r64_mem.base < pci_mem64_top - size) {
+ r64_mem.base = pci_mem64_top - size;
}
if (e820_is_used(r64_mem.base, size))
r64_mem.base -= size;
@@ -1181,8 +1185,16 @@ pci_setup(void)
dprintf(3, "pci setup\n");
- if (CPUPhysBits >= 36 && CPULongMode && RamSizeOver4G)
+ if (CPUPhysBits >= 36 && CPULongMode && RamSizeOver4G) {
pci_use_64bit = 1;
+ pci_mem64_top = 1LL << CPUPhysBits;
+ if (CPUPhysBits > 46) {
+ // Old linux kernels have trouble dealing with more than 46
+ // phys-bits, so avoid that for now. Seems to be a bug in the
+ // virtio-pci driver. Reported: centos-7, ubuntu-18.04
+ pci_mem64_top = 1LL << 46;
+ }
+ }
dprintf(1, "=== PCI bus & bridge init ===\n");
if (pci_probe_host() != 0) {
Cheers,
-Kevin
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.orgOn Fri, Nov 10, 2023 at 11:44:48AM -0500, Kevin O'Connor wrote:
> On Fri, Nov 10, 2023 at 12:04:24PM +0100, Gerd Hoffmann wrote:
> > - if (CPUPhysBits) {
> > - u64 top = 1LL << CPUPhysBits;
> > + if (pci_phys_bits) {
>
> FYI, this is a change in behavior - previously this condition would
> have been taken even if CPULongMode or RamSizeOver4G is false. I'm
> not sure if this change is intentional. (My example patch below
> follows your lead here.)
>
On closer inspection, I think this change in behavior was not
intended. How about variable names like the below instead.
-Kevin
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c
index c7084f5..6b13cd5 100644
--- a/src/fw/pciinit.c
+++ b/src/fw/pciinit.c
@@ -46,12 +46,16 @@ static const char *region_type_name[] = {
[ PCI_REGION_TYPE_PREFMEM ] = "prefmem",
};
+// Memory ranges exported to legacy ACPI type table generation
u64 pcimem_start = BUILD_PCIMEM_START;
u64 pcimem_end = BUILD_PCIMEM_END;
u64 pcimem64_start = BUILD_PCIMEM64_START;
u64 pcimem64_end = BUILD_PCIMEM64_END;
-u64 pci_io_low_end = 0xa000;
-u32 pci_use_64bit = 0;
+
+// Resource allocation limits
+static u64 pci_io_low_end = 0xa000;
+static u64 pci_mem64_top = 0;
+static u32 pci_pad_mem64 = 0;
struct pci_region_entry {
struct pci_device *dev;
@@ -966,8 +970,9 @@ static int pci_bios_check_devices(struct pci_bus *busses)
int resource_optional = 0;
if (hotplug_support == HOTPLUG_PCIE)
resource_optional = pcie_cap && (type == PCI_REGION_TYPE_IO);
- if (hotplug_support && pci_use_64bit && is64 && (type == PCI_REGION_TYPE_PREFMEM))
- align = (u64)1 << (CPUPhysBits - 11);
+ if (hotplug_support && pci_pad_mem64 && is64
+ && (type == PCI_REGION_TYPE_PREFMEM))
+ align = pci_mem64_top >> 11;
if (align > sum && hotplug_support && !resource_optional)
sum = align; /* reserve min size for hot-plug */
if (size > sum) {
@@ -1111,7 +1116,7 @@ static void pci_bios_map_devices(struct pci_bus *busses)
panic("PCI: out of I/O address space\n");
dprintf(1, "PCI: 32: %016llx - %016llx\n", pcimem_start, pcimem_end);
- if (pci_use_64bit || pci_bios_init_root_regions_mem(busses)) {
+ if (pci_pad_mem64 || pci_bios_init_root_regions_mem(busses)) {
struct pci_region r64_mem, r64_pref;
r64_mem.list.first = NULL;
r64_pref.list.first = NULL;
@@ -1131,14 +1136,13 @@ static void pci_bios_map_devices(struct pci_bus *busses)
r64_mem.base = le64_to_cpu(romfile_loadint("etc/reserved-memory-end", 0));
if (r64_mem.base < 0x100000000LL + RamSizeOver4G)
r64_mem.base = 0x100000000LL + RamSizeOver4G;
- if (CPUPhysBits) {
- u64 top = 1LL << CPUPhysBits;
+ if (pci_mem64_top) {
u64 size = (ALIGN(sum_mem, (1LL<<30)) +
ALIGN(sum_pref, (1LL<<30)));
- if (pci_use_64bit)
- size = ALIGN(size, (1LL<<(CPUPhysBits-3)));
- if (r64_mem.base < top - size) {
- r64_mem.base = top - size;
+ if (pci_pad_mem64)
+ size = ALIGN(size, pci_mem64_top >> 3);
+ if (r64_mem.base < pci_mem64_top - size) {
+ r64_mem.base = pci_mem64_top - size;
}
if (e820_is_used(r64_mem.base, size))
r64_mem.base -= size;
@@ -1181,8 +1185,18 @@ pci_setup(void)
dprintf(3, "pci setup\n");
+ if (CPUPhysBits) {
+ pci_mem64_top = 1LL << CPUPhysBits;
+ if (CPUPhysBits > 46) {
+ // Old linux kernels have trouble dealing with more than 46
+ // phys-bits, so avoid that for now. Seems to be a bug in the
+ // virtio-pci driver. Reported: centos-7, ubuntu-18.04
+ pci_mem64_top = 1LL << 46;
+ }
+ }
+
if (CPUPhysBits >= 36 && CPULongMode && RamSizeOver4G)
- pci_use_64bit = 1;
+ pci_pad_mem64 = 1;
dprintf(1, "=== PCI bus & bridge init ===\n");
if (pci_probe_host() != 0) {
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org© 2016 - 2024 Red Hat, Inc.