From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x230.google.com (mail-it0-x230.google.com [IPv6:2607:f8b0:4001:c0b::230]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 350181A1E06 for ; Fri, 2 Sep 2016 09:26:36 -0700 (PDT) Received: by mail-it0-x230.google.com with SMTP id i184so46583756itf.1 for ; Fri, 02 Sep 2016 09:26:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=yow1hu8gmpGsygVw2+9HvxyUiAEMIPZNnOb1qp4OttI=; b=UMbWNEzu68KjE1Xw3Rf43dITjURGvICXqSeSlyXUK627t647FAMGeEifwURNX1szyD v9thi15xf6RtMb6jUxCHN/7D2ALrsvcIEZDY6g4kQ3mP5cVxwa9Rg1jOHsZuIzCE+Mal wH5mnV8DhMAfw+insvenz05GK3BPdY+1NzLmU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=yow1hu8gmpGsygVw2+9HvxyUiAEMIPZNnOb1qp4OttI=; b=L38tqhreFd0J1qInN7wxXQc+V3Ni24njhG8kQv8ZvFN8Z5wQRYSHdPHu0AwMVTY+pi xNOOxy5lbVCfyDB2CurxOQR+6boalDL7aj5mqw3Ka+NjpFxdRxLknGR3fyfADv+8EPoH 9QdFkgcgEO5Zi3gZL6jVRDubFbia8QRzl+aVY/nDKK7wKCSDKzkWZAB55F0T0xM8lgN2 bVF+61Nd3ztmnm0VAPcf/hMyJoYQ23zici3E5tdG+Gk+C/bZOBAZKsp7bsdiSrtoA2CV 3A+2NCvngOWwNjUiKsRC7enb0FU4PllQ7lGEiTCuR4Z8g/r2ypc0H+WkfpoHu5Ue09ZI R8Lw== X-Gm-Message-State: AE9vXwPs8q+2Yk2Ny/iTMtFMbkf9KFqky+NxyVffIfqP5sN93O3nerggk659M8uUoBinBApyXc+5RgIdjxI1XFK/ X-Received: by 10.107.3.204 with SMTP id e73mr147328ioi.130.1472833595068; Fri, 02 Sep 2016 09:26:35 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.204.195 with HTTP; Fri, 2 Sep 2016 09:26:34 -0700 (PDT) In-Reply-To: References: <1472666379-25426-1-git-send-email-ard.biesheuvel@linaro.org> <207F4239-0958-4A0E-9DAA-36ABB56E7BB7@linaro.org> From: Ard Biesheuvel Date: Fri, 2 Sep 2016 17:26:34 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Leif Lindholm Subject: Re: [PATCH v2 0/6] ArmVirtQemu: move to generic PciHostBridgeDxe X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Sep 2016 16:26:36 -0000 Content-Type: text/plain; charset=UTF-8 On 2 September 2016 at 17:13, Laszlo Ersek 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 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 > ), > 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.