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

[Igor Mammedov]

On Wed, 18 May 2016 17:44:57 +0300
"Michael S. Tsirkin" <address@hidden> wrote:

> 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.
it should work in this case as it would fall back to non 'variant'
expected table.
Fail only happens if there isn't ANY expected table with
requested name.

So I use it to check the changes, I've made myself.

> 
> > >   
> > > > 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(-)
> > > > >     
> > >