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

[Marcel Apfelbaum <marcel@redhat.com>]

On 05/19/2016 12:04 PM, Igor Mammedov wrote:
> On Wed, 18 May 2016 17:22:43 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> On 05/18/2016 04:53 PM, Igor Mammedov wrote:
>>> On Sun, 15 May 2016 22:23:30 +0300
>>> Marcel Apfelbaum <marcel@redhat.com> 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, I'll give a try (separate patch, of course)
Marcel

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