Re: [Qemu-devel] [edk2] Could not add PCI device with big memory to aarch64 VMs

[liang yan <liangy@hpe.com>]

Hello, Laszlo,

On 11/30/2015 03:05 PM, Laszlo Ersek wrote:
> On 11/30/15 19:45, liang yan wrote:
>>
>> On 11/04/2015 05:53 PM, Laszlo Ersek wrote:
>>> On 11/04/15 23:22, liang yan wrote:
>>>> Hello, Laszlo,
>>>>
>>>>
>>>> (2)It also has a problem that once I use a memory bigger than 256M for
>>>> ivshmem, it could not get through UEFI,
>>>> the error message is
>>>>
>>>> PciBus: Discovered PCI @ [00|01|00]
>>>>      BAR[0]: Type =  Mem32; Alignment = 0xFFF;    Length = 0x100;
>>>> Offset =
>>>> 0x10
>>>>      BAR[1]: Type =  Mem32; Alignment = 0xFFF;    Length = 0x1000; Offset
>>>> = 0x14
>>>>      BAR[2]: Type = PMem64; Alignment = 0x3FFFFFFF;    Length =
>>>> 0x40000000;    Offset = 0x18
>>>>
>>>> PciBus: HostBridge->SubmitResources() - Success
>>>> ASSERT
>>>> /home/liang/studio/edk2/ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.c(449):
>>>> ((BOOLEAN)(0==1))
>>>>
>>>>
>>>> I am wandering if there are memory limitation for pcie devices under
>>>> Qemu environment?
>>>>
>>>>
>>>> Just thank you in advance and any information would be appreciated.
>>> (CC'ing Ard.)
>>>
>>> "Apparently", the firmware-side counterpart of QEMU commit 5125f9cd2532
>>> has never been contributed to edk2.
>>>
>>> Therefore the the ProcessPciHost() function in
>>> "ArmVirtPkg/VirtFdtDxe/VirtFdtDxe.c" ignores the
>>> DTB_PCI_HOST_RANGE_MMIO64 type range from the DTB. (Thus only
>>> DTB_PCI_HOST_RANGE_MMIO32 is recognized as PCI MMIO aperture.)
>>>
>>> However, even if said driver was extended to parse the new 64-bit
>>> aperture into PCDs (which wouldn't be hard), the
>>> ArmVirtPkg/PciHostBridgeDxe driver would still have to be taught to look
>>> at that aperture (from the PCDs) and to serve MMIO BAR allocation
>>> requests from it. That could be hard.
>>>
>>> Please check edk2 commits e48f1f15b0e2^..e5ceb6c9d390, approximately,
>>> for the background on the current code. See also chapter 13 "Protocols -
>>> PCI Bus Support" in the UEFI spec.
>>>
>>> Patches welcome. :)
>>>
>>> (A separate note on ACPI vs. DT: the firmware forwards *both* from QEMU
>>> to the runtime guest OS. However, the firmware parses only the DT for
>>> its own purposes.)
>> Hello, Laszlo,
>>
>> Thanks for your advices above, it's very helpful.
>>
>> When debugging, I also found some problems for 32 bit PCI devices.
>> Hope could get some clues from you.
>>
>> I checked on 512M, 1G, and 2G devices.(4G return invalid parameter
>> error, so I think it may be taken as a 64bit devices, is this right?).
> I guess so.
>
>>
>> First,
>>
>> All devices start from base address 3EFEFFFF.
> According to the below:
>
>> ProcessPciHost: Config[0x3F000000+0x1000000) Bus[0x0..0xF]
>> Io[0x0+0x10000)@0x3EFF0000 Mem[0x10000000+0x2EFF0000)@0x0
> the address you mention (0x3EFEFFFF) is the *highest* inclusive
> guest-phys address that an MMIO BAR can take. Not sure if that's what
> you meant.

Yes, you are right, current allocation is 
EfiGcdAllocateMaxAddressSearchTopDown, so base address here is the 
highest inclusive address.

> The size of the MMIO aperture for the entire PCI host is 0x2EFF0000
> bytes: a little less than 752 MB. So devices that need 1G and 2G MMIO
> BARs have no chance.
>
>> PcdPciMmio32Base is  10000000=====================
>> PcdPciMmio32Size is  2EFF0000=====================
>>
>>
>> Second,
>>
>> It could not get new base address when searching memory space in GCD map.
>>
>> For 512M devices,
>>
>> *BaseAddress = (*BaseAddress + 1 - Length) & (~AlignmentMask);
> This seems to be from CoreAllocateSpace()
> [MdeModulePkg/Core/Dxe/Gcd/Gcd.c]. AlignmentMask is computed from the
> Alignment input parameter.
>
> Which in turn seems to come from the BitsOfAlignment parameter computed
> in NotifyPhase(), "ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.c".
>
>> BaseAddress is 3EFEFFFF==========================
> So this is the highest address (inclusive) where the 512MB BAR could end.
>
>> new BaseAddress is 1EEF0000==========================
> This is the highest address (inclusive) where the 512MB BAR could start.
>
> This should be rounded down to an 512MB alignment (I believe), and then
> checked if that is still in the MMIO aperture.
>
> 512MB is 0x2000_0000.
>
> Rounding 0x1EEF_0000 down to an integral multiple of 0x2000_0000 results
> in zero:
>
>> ~AlignmentMask is E0000000==========================
>> Final BaseAddress is 0000
> And that address does not fall into the MMIO aperture.
>
> In other words, although your size requirement of 512MB could be
> theoretically satisfied from the aperture (which extends from exactly
> 256 MB to a little lower than 1008 MB), if you *also* require the base
> address to be aligned at 512MB, then that cannot be satisfied.
>
Thanks for the detail explanation above, I will write email with detail 
information too. Really appreciate.

>> Status = CoreSearchGcdMapEntry (*BaseAddress, Length, &StartLink,
>> &EndLink, Map);
>>
>>
>>
>> For bigger devices:
>>
>> all stops when searching memory space because below code, Length will
>> bigger than MaxAddress(3EFEFFFF)
>>
>> if ((Entry->BaseAddress + Length) > MaxAddress) {
>>           continue;
>> }
>>
>>
>> I also checked on ArmVirtQemu.dsc which all set to 0.
>>
>>    gArmPlatformTokenSpaceGuid.PcdPciBusMin|0x0
>>    gArmPlatformTokenSpaceGuid.PcdPciBusMax|0x0
>>    gArmPlatformTokenSpaceGuid.PcdPciIoBase|0x0
>>    gArmPlatformTokenSpaceGuid.PcdPciIoSize|0x0
>>    gArmPlatformTokenSpaceGuid.PcdPciIoTranslation|0x0
>>    gArmPlatformTokenSpaceGuid.PcdPciMmio32Base|0x0
>>    gArmPlatformTokenSpaceGuid.PcdPciMmio32Size|0x0
>>    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0x0
> These are just default values for the PCDs, to be set later by
> VirtFdtDxe, keyed off of QEMU's DTB.
>
> (In a platform DSC, you cannot mark a PCD as dynamic without providing a
> default value too. Hence the actual default values here aren't relevant.)
>
>>
>> Do you think I should change from PcdPciMmio32Base and PcdPciMmio32Size,
>> or do some change for GCD entry list, so it could allocate resources for
>> PCI devices(CoreSearchGcdMapEntry)?
> No, I think the code is working as expected.
>
> For devices with 1GB and larger MMIO BAR requirements, the host bridge
> simply doesn't have enough room in its MMIO aperture.
>
> For a device with an 512MB MMIO BAR requirement, there would be enough
> room (considering nothing but the size); however the alignment
> requirement of the BAR makes the request impossible to satisfy. The only
> address aligned at 512MB in the host bridge aperture is the absolute
> address 512MB itself, and from that point upwards, you don't have 512MB
> space, only approx. 1008 - 512 = 496 MB.
>
> This is all rooted in the specific, numerical boundaries of the
> VIRT_PCIE_MMIO entry, in QEMU's "hw/arm/virt.c".
>
> Now, if you changed *those* values (that is, the VIRT_PCIE_MMIO
> boundaries in QEMU), then that would trickle down to the firmware as
> well, through the generated DTB, and then larger devices might work.
> But, of course this is not the right way to go about it; that's why QEMU
> introduced VIRT_PCIE_MMIO_HIGH.
>
> ... You didn't mention 256MB devices -- I assume that is because those
> work already. Namely, two such BARs should be possible in the current
> aperture; I think at 256MB and at 512MB.

Yes, 256M works correctly, 512M could not get the right base address 
when doing alignment,  just like you said above.

> If you need more room (with large alignments), then there's no way
> around supporting QEMU's 64 bit aperture, VIRT_PCIE_MMIO_HIGH (see my
> earlier email).
I checked the function create_pcie form pathtoqemu/hw/arm/virt.c, it has 
a flag value use_highmem(which has default "true" value).

It set base_mmio_high and size_mmio_high to device tree by function below,

         qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "ranges",
                                      1, FDT_PCI_RANGE_IOPORT, 2, 0,
                                      2, base_pio, 2, size_pio,
                                      1, FDT_PCI_RANGE_MMIO, 2, base_mmio,
                                      2, base_mmio, 2, size_mmio,
                                      1, FDT_PCI_RANGE_MMIO_64BIT,
                                      2, base_mmio_high,
                                      2, base_mmio_high, 2, size_mmio_high);

So basically, I need to add two UINT64 variables like mmio_high_base and 
mmio_high_size to PCD under function ProcessPciHost(VirtFdtDxe.c),
and try to use this high base address and size as new aperture.

Is this correct?

> Unfortunately I can't readily help with that in the
> "ArmVirtPkg/PciHostBridgeDxe" driver; there's no such (open-source)
> example in the edk2 tree. Of course, I could experiment with it myself
> -- only not right now.
If possible, I do want to finish this part or help you finish it. I just 
work on UEFI recently, and thank you so much for your patient and detail 
explanation. I really appreciate it.
> I guess copying and adapting the TypeMem32 logic to TypeMem64 (currently
> short-circuited with EFI_ABORTED) could work.

Is the 32 or 64 bit determined by BAR(2-3bit) or by the PCI device memory size? Is there an option from QEMU?

Does TypeMem32 still keep  "VIRT_PCIE_MMIO" aperture and TypeMem64 use 
"VIRT_PCIE_MMIO_HIGH" aperture? or It's more like device property 
controlled from QEMU device simulation?


Best,
Liang
> Thanks
> Laszlo
>
>> Looking forward to your reply.
>>
>>
>> Thanks,
>> Liang
>>
>>> Thanks
>>> Laszlo
>>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>