Message ID | 20180927212438.32024-5-lersek@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | hw/pci-host/x86: extend the 64-bit PCI hole relative to the fw-assigned base | expand |
On 9/28/18 12:24 AM, Laszlo Ersek wrote: > In commit 9fa99d2519cb ("hw/pci-host: Fix x86 Host Bridges 64bit PCI > hole", 2017-11-16), we meant to expose such a 64-bit PCI MMIO aperture in > the ACPI DSDT that would be at least as large as the new "pci-hole64-size" > property (2GB on i440fx, 32GB on q35). The goal was to offer "enough" > 64-bit MMIO aperture to the guest OS for hotplug purposes. > > In that commit, we added or modified five functions: > > - pc_pci_hole64_start(): shared between i440fx and q35. Provides a default > 64-bit base, which starts beyond the cold-plugged 64-bit RAM, and skips > the DIMM hotplug area too (if any). > > - i440fx_pcihost_get_pci_hole64_start(), q35_host_get_pci_hole64_start(): > board-specific 64-bit base property getters called abstractly by the > ACPI generator. Both of these fall back to pc_pci_hole64_start() if the > firmware didn't program any 64-bit hole (i.e. if the firmware didn't > assign a 64-bit GPA to any MMIO BAR on any device). Otherwise, they > honor the firmware's BAR assignments (i.e., they treat the lowest 64-bit > GPA programmed by the firmware as the base address for the aperture). > > - i440fx_pcihost_get_pci_hole64_end(), q35_host_get_pci_hole64_end(): > these intended to extend the aperture to our size recommendation, > calculated relative to the base of the aperture. > > Despite the original intent, i440fx_pcihost_get_pci_hole64_end() and > q35_host_get_pci_hole64_end() currently only extend the aperture relative > to the default base (pc_pci_hole64_start()), ignoring any programming done > by the firmware. This means that our size recommendation may not be met. > Fix it by honoring the firmware's address assignments. > > The strange extension sizes were spotted by Alex, in the log of a guest > kernel running on top of OVMF (which prefers to assign 64-bit GPAs to > 64-bit BARs). > > This change only affects DSDT generation, therefore no new compat property > is being introduced. > > Using an i440fx OVMF guest with 5GB RAM, an example _CRS change is: > >> @@ -881,9 +881,9 @@ >> QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, Cacheable, ReadWrite, >> 0x0000000000000000, // Granularity >> 0x0000000800000000, // Range Minimum >> - 0x000000080001C0FF, // Range Maximum >> + 0x000000087FFFFFFF, // Range Maximum >> 0x0000000000000000, // Translation Offset >> - 0x000000000001C100, // Length >> + 0x0000000080000000, // Length >> ,, , AddressRangeMemory, TypeStatic) >> }) >> Device (GPE0) > (On i440fx, the low RAM split is at 3GB, in this case. Therefore, with 5GB > guest RAM and no DIMM hotplug range, pc_pci_hole64_start() returns 4 + > (5-3) = 6 GB. Adding the 2GB extension to that yields 8GB, which is below > the firmware-programmed base of 32GB, before the patch. Therefore, before > the patch, the extension is ineffective. After the patch, we add the 2GB > extension to the firmware-programmed base, namely 32GB.) > > Using a q35 OVMF guest with 5GB RAM, an example _CRS change is: > >> @@ -3162,9 +3162,9 @@ >> QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, Cacheable, ReadWrite, >> 0x0000000000000000, // Granularity >> 0x0000000800000000, // Range Minimum >> - 0x00000009BFFFFFFF, // Range Maximum >> + 0x0000000FFFFFFFFF, // Range Maximum >> 0x0000000000000000, // Translation Offset >> - 0x00000001C0000000, // Length >> + 0x0000000800000000, // Length >> ,, , AddressRangeMemory, TypeStatic) >> }) >> Device (GPE0) > (On Q35, the low RAM split is at 2GB. Therefore, with 5GB guest RAM and no > DIMM hotplug range, pc_pci_hole64_start() returns 4 + (5-2) = 7 GB. Adding > the 32GB extension to that yields 39GB (0x0000_0009_BFFF_FFFF + 1), before > the patch. After the patch, we add the 32GB extension to the > firmware-programmed base, namely 32GB.) > > The ACPI test data for the bios-tables-test case that we added earlier in > this series are corrected too, as follows: > >> @@ -3339,9 +3339,9 @@ >> QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, Cacheable, ReadWrite, >> 0x0000000000000000, // Granularity >> 0x0000000200000000, // Range Minimum >> - 0x00000009BFFFFFFF, // Range Maximum >> + 0x00000009FFFFFFFF, // Range Maximum >> 0x0000000000000000, // Translation Offset >> - 0x00000007C0000000, // Length >> + 0x0000000800000000, // Length >> ,, , AddressRangeMemory, TypeStatic) >> }) >> Device (GPE0) > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Alex Williamson <alex.williamson@redhat.com> > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Igor Mammedov <imammedo@redhat.com> > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > Fixes: 9fa99d2519cbf71f871e46871df12cb446dc1c3e > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > > Notes: > v2: > - no code changes > - remove the incorrect claim from the commit message that SeaBIOS is not > affected [Alex] > - update test case data; include textual diff > - drop "Link:" tag pointing to now-stale original discussion > > hw/pci-host/piix.c | 2 +- > hw/pci-host/q35.c | 2 +- > tests/acpi-test-data/q35/DSDT.mmio64 | Bin 8947 -> 8947 bytes > 3 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c > index 0df91e002076..fb4b0669ac9f 100644 > --- a/hw/pci-host/piix.c > +++ b/hw/pci-host/piix.c > @@ -285,7 +285,7 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v, > { > PCIHostState *h = PCI_HOST_BRIDGE(obj); > I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj); > - uint64_t hole64_start = pc_pci_hole64_start(); > + uint64_t hole64_start = i440fx_pcihost_get_pci_hole64_start_value(obj); > Range w64; > uint64_t value, hole64_end; > > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > index 8acf942b5e65..1c5433161f14 100644 > --- a/hw/pci-host/q35.c > +++ b/hw/pci-host/q35.c > @@ -145,7 +145,7 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v, > { > PCIHostState *h = PCI_HOST_BRIDGE(obj); > Q35PCIHost *s = Q35_HOST_DEVICE(obj); > - uint64_t hole64_start = pc_pci_hole64_start(); > + uint64_t hole64_start = q35_host_get_pci_hole64_start_value(obj); > Range w64; > uint64_t value, hole64_end; > > diff --git a/tests/acpi-test-data/q35/DSDT.mmio64 b/tests/acpi-test-data/q35/DSDT.mmio64 > index 498c535f2a85a058b76a0a5ebc67c3d5236c3265..a058ff2ee31a22a55b5b198bc1531c7f20b243f6 100644 > GIT binary patch > delta 43 > mcmezD`q`DsCD<k8vl0UX<Gqbs#}$SCb26X;j?Ir11sDMxoC+%d > > delta 43 > ncmezD`q`DsCD<k8vl0UXWBf+0<BCH2IT=vE0rt(06$Kap4zmg? > Reviewed-by: Marcel Apfelbaum<marcel.apfelbaum@gmail.com> Thanks, Marcel
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c index 0df91e002076..fb4b0669ac9f 100644 --- a/hw/pci-host/piix.c +++ b/hw/pci-host/piix.c @@ -285,7 +285,7 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v, { PCIHostState *h = PCI_HOST_BRIDGE(obj); I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj); - uint64_t hole64_start = pc_pci_hole64_start(); + uint64_t hole64_start = i440fx_pcihost_get_pci_hole64_start_value(obj); Range w64; uint64_t value, hole64_end; diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c index 8acf942b5e65..1c5433161f14 100644 --- a/hw/pci-host/q35.c +++ b/hw/pci-host/q35.c @@ -145,7 +145,7 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v, { PCIHostState *h = PCI_HOST_BRIDGE(obj); Q35PCIHost *s = Q35_HOST_DEVICE(obj); - uint64_t hole64_start = pc_pci_hole64_start(); + uint64_t hole64_start = q35_host_get_pci_hole64_start_value(obj); Range w64; uint64_t value, hole64_end;