Re: [PATCH v2 0/6] ArmVirtQemu: move to generic PciHostBridgeDxe

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 2 September 2016 at 17:13, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/02/16 17:27, Laszlo Ersek wrote:
>> On 09/02/16 16:58, Ard Biesheuvel wrote:
>>> (on the road atm, will reply in full later)
>>>
>>>> On 2 sep. 2016, at 14:09, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>>>> (2) aarch64 KVM, using virtio-gpu-pci and USB 2 keyboard and
>>>> tablet. I actually booted a Fedora 24 guest with this, and in the
>>>> guest, everything works just fine (display, keyboard,
>>>> mouse/tablet). Most of the firmware log looks good too.
>>>>
>>>> (2a) However, the USB 2 keyboard is broken while in the firmware
>>>> (in spite of it working well in the guest OS).
>>>>
>>>>  -device ich9-usb-ehci1,multifunction=on,id=ehci,addr=05.0 \
>>>>  -device ich9-usb-uhci1,multifunction=on,masterbus=ehci.0,firstport=0,addr=05.1 \
>>>>  -device ich9-usb-uhci2,multifunction=on,masterbus=ehci.0,firstport=2,addr=05.2 \
>>>>  -device ich9-usb-uhci3,multifunction=on,masterbus=ehci.0,firstport=4,addr=05.3 \
>>>>  -device usb-kbd,bus=ehci.0 \
>>>>  -device usb-tablet,bus=ehci.0 \
>>>>
>>>> My QEMU has your commit 5d636e21c44e ("hw/arm/virt: mark the PCIe
>>>> host controller as DMA coherent in the DT"), but I guess the EHCI
>>>> driver in edk2 doesn't comply with the "guest drivers should use
>>>> cacheable accesses as well when running under KVM" part. :(
>>>>
>>>> The following snippet repeats in the log:
>>>>
>>>>  EhcClearLegacySupport: called to clear legacy support
>>>>  processing error - resetting ehci HC
>>>>  EhcInitHC: failed to enable period schedule
>>>>  EhcDriverBindingStart: failed to init host controller
>>>>  EhcCreateUsb2Hc: capability length 32
>>>>
>>>> Interestingly, if I back out your series, then USB2 works in the
>>>> firmware. I don't understand this, given that my build includes
>>>> commit 3ef3209d3028 ("ArmVirtPkg: remove
>>>> PcdKludgeMapPciMmioAsCached") from the master branch!
>>>>
>>>
>>> Does it work when you limit DMA to < 4 GB?
>>
>> You are one wicked genius, man; the following change
>>
>>> diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>>> index efccedcca14f..1f0f87cac8a9 100644
>>> --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>>> +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
>>> @@ -317,7 +317,7 @@ PciHostBridgeGetRootBridges (
>>>                                        EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO_16;
>>>    mRootBridge.Attributes            = mRootBridge.Supports;
>>>
>>> -  mRootBridge.DmaAbove4G            = TRUE;
>>> +  mRootBridge.DmaAbove4G            = FALSE;
>>>    mRootBridge.NoExtendedConfigSpace = FALSE;
>>>    mRootBridge.ResourceAssigned      = FALSE;
>>>
>>
>> does make it work! Excellent!
>>
>> Explain please. :) (Although, I'll look into PciHostBridgeDxe in a moment too. :))
>

Thanks. You seem to have a good handle on things already, though :-)

> Well okay, I reviewed the RootBridgeIoMap() and RootBridgeIoUnmap()
> functions in "MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c".
> They implement bounce buffering when DmaAbove4G is set to FALSE, and
> when the original RAM buffer, to be DMA'd from or to by the PCI device,
> would end outside of 32-bit space.
>
> For common buffer operations (when device and CPU collaborate on memory
> repeatedly, without intervening Map() and Unmap() calls), Map() and
> Unmap() cannot implement bounce buffering, so the initial buffer must be
> allocated low enough. This is what RootBridgeIoAllocateBuffer() does,
> and yes it considers DmaAbove4G as well.
>
> EhciDxe uses these functions quite a bit. And, my test VM has 4G of
> memory, with a base at 0x4000_0000 (1GB); the base is fixed of course,
> from "-M virt". So, I guess, some buffers that EhciDxe allocated itself,
> for DMA'ing from/to the device, and some buffers that it allocated with
> AllocateBuffer(), for common operations with the device, ended up in the
> 4GB..5GB range. Due to DmaAbove4G = TRUE, those host addresses got
> passed to the PCI device (the USB 2 host controller) verbatim, but that
> device can only access host RAM in the 32-bit address range?....
>
> Hm, let me check the QEMU code (hw/usb/hcd-ehci.c)...
>
> Alright, I've found it. According to the EHCI specification
> ("ehci-specification-for-usb.pdf", link found under
> <https://en.wikipedia.org/wiki/Extensible_Host_Controller_Interface#References>),
> revision 1.0, section "2.2.4 HCCPARAMS -- Capability Parameters", bit #0
> (value 1) in the HCCPARAMS capability register stands for:
>
>
>     64-bit Addressing Capability. This field documents the addressing
>     range capability of this implementation. The value of this field
>     determines whether software should use the data structures defined
>     in Section 3 (32-bit) or those defined in Appendix B (64-bit).
>     Values for this field have the following interpretation:
>
>     0b  data structures using 32-bit address memory pointers
>     1b  data structures using 64-bit address memory pointers
>
> Furthermore, the HCCPARAMS register lives at address "Base + (08h)".
>
> Now, looking at the QEMU code, we have usb_ehci_init()
> [hw/usb/hcd-ehci.c] performing the following assignment:
>
>   s->caps[0x08] = 0x80;        /* We can cache whole frame, no 64-bit */
>
> (And, the "cache whole frame" reference, for bit #7, is consistent with
> the documentation of that bit in the spec: "When bit [7] is a
> one, then host software assumes the host controller may cache an
> isochronous data structure for an entire frame.")
>
> So, bingo. Please flip DmaAbove4G to FALSE in patch #3, and please drop
> the "DMA above 4 GB" paragraph from the commit message of patch #4.
>

Actually, I suspect this is a bug in PciHostBridgeDxe. It ignores the
absence of the EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE attribute, which
should be set by the driver if it knows the device is capable of
64-bit DMA.

Could you please try the below?


diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index b2d76d67afa2..b53b9a834816 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -1308,7 +1308,8 @@ RootBridgeIoAllocateBuffer (
   RootBridge = ROOT_BRIDGE_FROM_THIS (This);

   AllocateType = AllocateAnyPages;
-  if (!RootBridge->DmaAbove4G) {
+  if (!RootBridge->DmaAbove4G ||
+      (Attributes & EFI_PCI_ATTRIBUTE_DUAL_ADDRESS_CYCLE) == 0) {
     //
     // Limit allocations to memory below 4GB
     //

Thanks,
Ard.