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

[Igor Mammedov]

On Wed, 18 May 2016 17:22:43 +0300
Marcel Apfelbaum <address@hidden> wrote:

> On 05/18/2016 04:53 PM, 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
> >
> > 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.
> >  
> I'll add the test, thanks, and also I'll look for the PXB warning.
> 
> >
> > 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()
> >  
> 
> Their PRT are not exactly the same, please see build_prt in 
> hw/i386/acpi-build.c .
build_prt() can generate shared AML if is_pci0_prt
would be checked in AML, something like this:

@@@ -667,13 -684,13 +667,13 @@@ static Aml *build_prt(bool is_pci0_prt
  
          /* route[2] = "LNK[D|A|B|C]", selection based on pin % 3  */
          aml_append(while_ctx, initialize_route(route, "LNKD", lnk_idx, 0));
--        if (is_pci0_prt) {
++        {
              Aml *if_device_1, *if_pin_4, *else_pin_4;
  
              /* device 1 is the power-management device, needs SCI */
              if_device_1 = aml_if(aml_equal(lnk_idx, aml_int(1)));
              {
--                if_pin_4 = aml_if(aml_equal(pin, aml_int(4)));
++                if_pin_4 = aml_if(aml_and(aml_equal(pin, aml_int(4)), 
is_pci0_prt, NULL));
                  {
                      aml_append(if_pin_4,
                          aml_store(build_prt_entry("LNKS"), route));
@@@ -687,8 -704,8 +687,6 @@@
                  aml_append(if_device_1, else_pin_4);
              }
              aml_append(while_ctx, if_device_1);
--        } else {
--            aml_append(while_ctx, initialize_route(route, "LNKA", lnk_idx, 
1));
          }
          aml_append(while_ctx, initialize_route(route, "LNKB", lnk_idx, 2));
          aml_append(while_ctx, initialize_route(route, "LNKC", lnk_idx, 3));


as is_pci0_prt key, it's probably possible to use _UID == 0 condition
or make a wrapper method per bus that explicitly will tell
common_prt() if it's root bus or not.

> 
> Thanks,
> Marcel
> 
> >> 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(-)
> >>  
> >  
> 
>