Re: [Qemu-devel] [PATCH V3 0/3] pxb: fix 64-bit MMIO allocation

[Marcel Apfelbaum <marcel@redhat.com>]

On 06/28/2016 05:27 PM, Igor Mammedov wrote:
> On Tue, 28 Jun 2016 12:59:25 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> This series fixes 64-bit BARs allocations for devices behind PXBs/PXB-PCIEs.
>>
>> In build_crs() the calculation and merging of the ranges already happens
>> in 64-bit, but the entry boundaries are silently truncated to 32-bit in the
>> call to aml_dword_memory(). Fix it by handling the 64-bit MMIO ranges separately.
>
> I'd rearrange pathc order by putting test without DSDT.pxb blob,
> as the first patch so changes to AML would become observable
> for each of following patches and finish series with blob update
> (provided below issue also fixed within series, otherwise it will
> become hidden and we probably will forget about it)
>

Sure, I'll do that for next version.

> on piix4 machine PXB is marked as hotpluggable and as result
> it generates following bogus change:
>
> @@ -1156,8 +1280,8 @@ DefinitionBlock ("", "DSDT", 1, "BOCHS ", "BXPCDSDT", 0x00000001)
>
>               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)
> @@ -1597,6 +1721,7 @@ DefinitionBlock ("", "DSDT", 1, "BOCHS ", "BXPCDSDT", 0x00000001)
>                   BNUM = Zero
>                   DVNT (PCIU, One)
>                   DVNT (PCID, 0x03)
> +                ^S18.PCNT ()
>               }
>           }
>       }
>
> with ^S18.PCNT() leading nowhere,
> so question is:
>    Is PXB present at boot ACPI hot(un)pluggable itself?
>

No, is not, and it should definitively be marked as not hot-pluggable.

I remember you already commented about that, I was testing it
with q35 and didn't see the issue, sorry for missing it earlier.


> perhaps you need to play with bridge_in_acpi or dc->hotpluggable
> to make it not hotpluggable so won't create non existent call.
>

I'll be sure to add a patch for that, thanks!
Marcel

>> v2 -> v3:
>>   - split original series "pci: better support for 64-bit MMIO allocation" into 2 series:
>>      - this is the first part dealing with correct 64-bit MMIO ACPI computation
>>      - the second one will include 64-bit MMIO reservation for PCI hotplug
>>   - Add pxb/pxb-pcie tests (Igor) - See diffs below (*)
>>   - Re-based on latest master.
>>
>> v1 -> v2:
>>   - resolved some styling issues (Laszlo)
>>   - rebase on latest master (Laszlo)
>>
>> Thank you,
>> Marcel
>>
>>
>>
>> (*)
>>
>>    PC/pxb
>>    =======================================================================================================
>>    8c8
>>    <  * Disassembly of /tmp/aml-5UR3JY, Tue Jun 28 12:51:27 2016
>>    ---
>>    >  * Disassembly of tests/acpi-test-data/pc/DSDT.pxb, Tue Jun 28 12:51:27 2016
>>    12c12
>>    <  *     Length           0x000018A5 (6309)
>>      ---
>>      >  *     Length           0x000018B9 (6329)
>>      14c14
>>      <  *     Checksum         0xC3
>>      ---
>>      >  *     Checksum         0x03
>>      21c21
>>      < DefinitionBlock ("/tmp/aml-5UR3JY.aml", "DSDT", 1, "BOCHS ", "BXPCDSDT", 0x00000001)
>>      ---
>>      > DefinitionBlock ("tests/acpi-test-data/pc/DSDT.aml", "DSDT", 1, "BOCHS ", "BXPCDSDT", 0x00000001)
>>      1063,1068c1063,1068
>>      <                 DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
>>              <                     0x00000000,         // Granularity
>>              <                     0x00000000,         // Range Minimum
>>              <                     0xFFFFFFFF,         // Range Maximum
>>              <                     0x00000000,         // Translation Offset
>>              <                     0x00000000,         // Length
>>              ---
>>              >                 QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
>>                  >                     0x0000000000000000, // Granularity
>>                  >                     0x0000000100000000, // Range Minimum
>>                  >                     0x00000001FFFFFFFF, // Range Maximum
>>                  >                     0x0000000000000000, // Translation Offset
>>                  >                     0x0000000100000000, // Length
>>                  1129c1129
>>                  <                 0xFFFFFFFF,         // Range Maximum
>>                  ---
>>                  >                 0xFEBFFFFF,         // Range Maximum
>>                  1131c1131
>>                  <                 0x01600000,         // Length
>>                  ---
>>                  >                 0x00200000,         // Length
>>
>>    Q35/pxb-pcie
>>    ============================================================================================================
>>    8c8
>>    <  * Disassembly of /tmp/aml-U1VPJY, Tue Jun 28 12:51:31 2016
>>              ---
>>              >  * Disassembly of tests/acpi-test-data/q35/DSDT.pxb_pcie, Tue Jun 28 12:51:31 2016
>>              12c12
>>              <  *     Length           0x00002376 (9078)
>>      ---
>>      >  *     Length           0x0000238A (9098)
>>      14c14
>>      <  *     Checksum         0xA9
>>      ---
>>      >  *     Checksum         0xE9
>>      21c21
>>      < DefinitionBlock ("/tmp/aml-U1VPJY.aml", "DSDT", 1, "BOCHS ", "BXPCDSDT", 0x00000001)
>>      ---
>>      > DefinitionBlock ("tests/acpi-test-data/q35/DSDT.aml", "DSDT", 1, "BOCHS ", "BXPCDSDT", 0x00000001)
>>      3309,3314c3309,3314
>>      <                 DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
>>              <                     0x00000000,         // Granularity
>>              <                     0x00000000,         // Range Minimum
>>              <                     0xFFFFFFFF,         // Range Maximum
>>              <                     0x00000000,         // Translation Offset
>>              <                     0x00000000,         // Length
>>              ---
>>              >                 QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
>>                  >                     0x0000000000000000, // Granularity
>>                  >                     0x0000000100000000, // Range Minimum
>>                  >                     0x00000001FFFFFFFF, // Range Maximum
>>                  >                     0x0000000000000000, // Translation Offset
>>                  >                     0x0000000100000000, // Length
>>                  3375c3375
>>                  <                 0xFFFFFFFF,         // Range Maximum
>>                  ---
>>                  >                 0xFEBFFFFF,         // Range Maximum
>>                  3377c3377
>>                  <                 0x01600000,         // Length
>>                  ---
>>                  >                 0x00200000,         // Length
>>
>>
>>
>>
>> Marcel Apfelbaum (3):
>>    acpi: refactor pxb crs computation
>>    hw/apci: handle 64-bit MMIO regions correctly
>>    tests/acpi: add pxb/pxb-pcie tests
>>
>>   hw/i386/acpi-build.c                   | 127 +++++++++++++++++++++++----------
>>   tests/acpi-test-data/pc/DSDT.pxb       | Bin 0 -> 6329 bytes
>>   tests/acpi-test-data/q35/DSDT.pxb_pcie | Bin 0 -> 9098 bytes
>>   tests/bios-tables-test.c               |  37 ++++++++++
>>   4 files changed, 128 insertions(+), 36 deletions(-)
>>   create mode 100644 tests/acpi-test-data/pc/DSDT.pxb
>>   create mode 100644 tests/acpi-test-data/q35/DSDT.pxb_pcie
>>
>