Re: [Qemu-devel] [PATCH 2/2] hw/pci-host/x86: extend the 64-bit PCI hole relative to the fw-assigned base

[Michael S. Tsirkin]

On Tue, Sep 25, 2018 at 09:07:45PM +0300, Marcel Apfelbaum wrote:
> Hi Laszlo,
> 
> On 9/25/18 1:13 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.
> > 
> 
> [...]
> 
> > Also, because SeaBIOS never assigns 64-bit GPAs to
> > 64-bit BARs, the patch makes no difference to SeaBIOS guests.
> 
> And this is why we didn't catch that earlier... we need to include
> OVMF in our basic tests.

For that we need to distribute the binary with QEMU, we don't
want basic tests to have another external dependency.

> >   (Which is in
> > turn why ACPI test data for the "bios-tables-test" need not be refreshed.)
> > 
> > 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.)
> > 
> > Cc: "Michael S. Tsirkin" <address@hidden>
> > Cc: Alex Williamson <address@hidden>
> > Cc: Marcel Apfelbaum <address@hidden>
> > Link: http://mid.mail-archive.com/address@hidden
> > Fixes: 9fa99d2519cbf71f871e46871df12cb446dc1c3e
> > Signed-off-by: Laszlo Ersek <address@hidden>
> > ---
> >   hw/pci-host/piix.c | 2 +-
> >   hw/pci-host/q35.c  | 2 +-
> >   2 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;
> 
> 
> Reviewed-by: Marcel Apfelbaum<address@hidden>
> 
> 
> Thanks,
> Marcel
>