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

[Marcel Apfelbaum]

On 06/28/2016 05:27 PM, Igor Mammedov wrote:
> On Tue, 28 Jun 2016 12:59:25 +0300
> Marcel Apfelbaum <address@hidden> 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