Re: [Qemu-devel] [PATCH V2 0/4] pci: better support for 64-bit MMIO allocation

[Michael S. Tsirkin]

On Wed, May 18, 2016 at 04:38:26PM +0200, Igor Mammedov wrote:
> On Wed, 18 May 2016 17:09:20 +0300
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > On Wed, May 18, 2016 at 03:53:08PM +0200, Igor Mammedov wrote:
> > > On Sun, 15 May 2016 22:23:30 +0300
> > > Marcel Apfelbaum <address@hidden> wrote:
> > >   
> > > > Hi,
> > > > 
> > > > First two patches allocate (max_reserved_ram - 
> > > > max_addr_cpu_addressable) range for PCI hotplug
> > > > (for PC Machines) instead of the previous 64-bit PCI window that 
> > > > included only
> > > > the ranges allocated by the firmware.
> > > > 
> > > > The next two patches fix 64-bit CRS computations.  
> > > I'd would add test case + expected tables as the first 2 patches
> > > and then finish series with expected tables update with fixed 64bit range 
> > >  
> > 
> > I don't think we necessarily need this noise when running tests.
> > Adding tests last is OK I think, but I prefer updating expected acpi tables
> > myself as these can't be merged.
> the reason I've asked for tables updating is that it makes
> life of reviewers easier, i.e. reviewer can just run
> test and see a ASL diff before fix and after.
> 
> Patches with updates could be discarded during merge.
> /mark them with "DONT APPLY"/

I think a new test will currently fail if expected
is not there though. We might want to fix that
to allow the workflow you describe.

> > 
> > > as experiment I've hacked existing piix4 case:
> > > 
> > > @@ -744,7 +744,9 @@ static void test_acpi_piix4_tcg(void)
> > >       */
> > >      memset(&data, 0, sizeof(data));
> > >      data.machine = MACHINE_PC;
> > > -    test_acpi_one("-machine accel=tcg", &data);
> > > +    test_acpi_one("-machine accel=tcg"
> > > +                  " -device pxb,id=bridge1,bus=pci.0,bus_nr=4"
> > > +                  " -device ivshmem,bus=bridge1,size=4G,shm", &data);
> > >      free_test_data(&data);
> > >  }
> > > 
> > > And it shows not related to this series, but another pxb issue
> > > 
> > > +    External (_SB_.PCI0.S18_.PCNT, MethodObj)    // Warning: Unresolved 
> > > method, guessing 0 arguments
> > > ...
> > > @@ -1197,8 +1322,8 @@ DefinitionBlock 
> > > ("tests/acpi-test-data/pc/DSDT.aml", "DSDT", 1, "BOCHS ", "BXPCD
> > >  
> > >              Device (S18)
> > >              {
> > > -                Name (_SUN, 0x03)  // _SUN: Slot User Number
> > >                  Name (_ADR, 0x00030000)  // _ADR: Address
> > > +                Name (_SUN, 0x03)  // _SUN: Slot User Number
> > >                  Method (_EJ0, 1, NotSerialized)  // _EJx: Eject Device
> > >                  {
> > >                      PCEJ (BSEL, _SUN)
> > > @@ -1638,6 +1763,7 @@ DefinitionBlock 
> > > ("tests/acpi-test-data/pc/DSDT.aml", "DSDT", 1, "BOCHS ", "BXPCD
> > >                  BNUM = Zero
> > >                  DVNT (PCIU, One)
> > >                  DVNT (PCID, 0x03)
> > > +                ^S18.PCNT ()
> > >              }
> > >          }
> > >      }
> > > 
> > > so it's better to have test case in place so that changes to pxb
> > > parts wouldn't go unnoticed and would be observable.
> > > 
> > > 
> > > Also from above experiment I see that pxb duplicates and uses
> > > the same _PRT method as PCI0, probably should be changed to
> > > something like:
> > > 
> > >  Method(_PRT)
> > >     return ^PCI0._PRT()
> > >   
> > > > v1 -> v2:
> > > >  - resolved some styling issues (Laszlo)
> > > >  - rebase on latest master (Laszlo)
> > > > 
> > > > Thank you,
> > > > Marcel
> > > > 
> > > > Marcel Apfelbaum (4):
> > > >   hw/pc: extract reserved memory end computation to a standalone
> > > >     function
> > > >   pci: reserve 64 bit MMIO range for PCI hotplug
> > > >   acpi: refactor pxb crs computation
> > > >   hw/apci: handle 64-bit MMIO regions correctly
> > > > 
> > > >  hw/i386/acpi-build.c | 127 
> > > > ++++++++++++++++++++++++++++++++++++---------------
> > > >  hw/i386/pc.c         |  29 ++++++++----
> > > >  hw/pci/pci.c         |  16 ++++++-
> > > >  include/hw/i386/pc.h |   1 +
> > > >  4 files changed, 127 insertions(+), 46 deletions(-)
> > > >   
> >