From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-x231.google.com (mail-wm0-x231.google.com [IPv6:2a00:1450:400c:c09::231]) (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 AD5DB1A1E06 for ; Fri, 2 Sep 2016 07:58:44 -0700 (PDT) Received: by mail-wm0-x231.google.com with SMTP id v143so35818941wmv.0 for ; Fri, 02 Sep 2016 07:58:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=references:mime-version:in-reply-to:content-transfer-encoding :message-id:cc:from:subject:date:to; bh=vFbu8+9q6ne2Y1O14hML1VqBVf7aNASATSnYjkvbTrg=; b=RMPKo8RARrgmOxbVbXF9w9d4ASBGtxFZPob+WSGCxzMm96HPXR9qC0lOdt9WN2OGFL VgTXy6Z3gcpz5jRm5Y2LjJZd1kP+AG4ECKQD2rZ//cICb0dhsH1Z1CdYalOgfFnT9VqZ JMJkxi1bhWj3dZ2I1jmMTC4+psZuiy0gSmRHY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:references:mime-version:in-reply-to :content-transfer-encoding:message-id:cc:from:subject:date:to; bh=vFbu8+9q6ne2Y1O14hML1VqBVf7aNASATSnYjkvbTrg=; b=A8KJbmlJZhK0zxifHsqSh5BlBUBpZ1m8TYhugkJufGCZhIvHeuMWR6QFh9T9qRsZJ8 HESouqCHPBmAaVRCyS0IfkCximL5mBXxeIzGzfl+c4qJg+0nVbzSn6E05Z/qfSLDMipC d6OKg0tpCB9z3tyLnZd4ZpIM+ruZcLKegqGoOybQHflobmEAD2ehzgjz6eCFKQEAHsqL e5OpXc9NyzKXpZtIsa6z/p5kp+2yDHA5GopORQ2lcdkHHK9rP1rMCvqNCltaL6SNgyH1 uC4016LqGO507OuLLS84c0i7Wm0EmJb9BiYyNYv0nA3Ej1uhYEpfh+vRedNikKrdwH7T BFcA== X-Gm-Message-State: AE9vXwPOSnrPjxT5a/nw/XmvcG0FrvMy5aNefGlmhWcshriIERSAde8iNXUxm5f3DhTN7zZD X-Received: by 10.28.56.195 with SMTP id f186mr3546852wma.59.1472828323193; Fri, 02 Sep 2016 07:58:43 -0700 (PDT) Received: from [10.19.140.63] (dynrak234g-63-4-67-105.inwitelecom.net. [105.67.4.63]) by smtp.gmail.com with ESMTPSA id a194sm3956488wmd.24.2016.09.02.07.58.42 (version=TLS1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Fri, 02 Sep 2016 07:58:42 -0700 (PDT) References: <1472666379-25426-1-git-send-email-ard.biesheuvel@linaro.org> Mime-Version: 1.0 (1.0) In-Reply-To: Message-Id: <207F4239-0958-4A0E-9DAA-36ABB56E7BB7@linaro.org> Cc: edk2-devel@ml01.01.org, leif.lindholm@linaro.org X-Mailer: iPhone Mail (13G35) From: Ard Biesheuvel Date: Fri, 2 Sep 2016 15:58:38 +0100 To: Laszlo Ersek 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 14:58:45 -0000 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable (on the road atm, will reply in full later) > On 2 sep. 2016, at 14:09, Laszlo Ersek wrote: >=20 >> On 08/31/16 19:59, Ard Biesheuvel wrote: >> Now that Laszlo's virtio-gpu-pci series has removed the last remaining ob= stacle, >> we can get rid of the special PciHostBridgeDxe implementation in ArmVirtP= kg, >> and move to the generic one. On AArch64, this will allow us to perform DM= A above >> 4GB without bounce buffering, and use 64-bit MMIO BARs allocated above 4 G= B. >>=20 >> Changes since v1: >> - new patch #2 to move the IoTranslation discovery to FdtPciPcdProducerLi= b, >> which is a cleaner approach since other drivers (i.e., ArmPciCpuIo2Dxe) >> depend on it as well >> - add support for ARM, i.e., disable the 64-bit range in that case, since= we >> cannot access 64-bit MMIO BARs if they are allocated there >> - use statically allocated PCI_ROOT_BRIDGE[] array of size 1 >> - enable support for ISA and VGA I/O range decoding >> - various other minor fixes based on Laszlo's review comments >> - added ref links and Laszlo's acks where appropriate, i.e., where given a= nd >> where the version of the patch in this series does not deviate substanti= ally >> from the suggested version on which the preliminary ack was based >>=20 >> Patch #1 removes the linux,pci-probe-only override which does more harm t= han >> good now that we switched to virtio-gpu-pci, which does not expose a raw >> framebuffer. >>=20 >> Patch #2 extends FdtPciPcdProducerLib so that it also sets PcdPciIoTransl= ation, >> which is required for the FdtPciHostBridgeLib implementation this series >> introduces, but also for ArmPciCpuIo2Dxe, which produces EFI_CPU_IO2_PROT= OCOL >> on which the generic PciHostBridgeDxe depends as well. >>=20 >> Patch #3 implements PciHostBridgeLib for platforms exposing a PCI host br= idge >> using a pci-host-ecam-generic DT node. The initial version is based on th= e >> ArmVirtPkg implementation of PciHostBridgeDxe, so it does not support 64-= bit >> MMIO BARs allocated above 4 GB, but it does support DMA above 4 GB withou= t >> bounce buffering. >>=20 >> Patch #4 switches to the generic PciHostBridgeDxe, with no change in >> functionality other than support for DMA above 4 GB without bounce buffer= ing. >>=20 >> Patch #5 adds support for allocating 64-bit MMIO BARs above 4 GB. >>=20 >> Patch #6 removes the now obsolete PciHostBridgeDxe from ArmVirtPkg. >>=20 >>=20 >> Ard Biesheuvel (6): >> ArmVirtPkg/PciHostBridgeDxe: don't set linux,pci-probe-only DT >> property >> ArmVirtPkg/FdtPciPcdProducerLib: add handling of PcdPciIoTranslation >> ArmVirtPkg: implement FdtPciHostBridgeLib >> ArmVirtPkg/ArmVirtQemu: switch to generic PciHostBridgeDxe >> ArmVirtPkg/FdtPciHostBridgeLib: add MMIO64 support >> ArmVirtPkg: remove now unused PciHostBridgeDxe >>=20 >> ArmVirtPkg/ArmVirtQemu.dsc | 10 += - >> ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc | 3 += - >> ArmVirtPkg/ArmVirtQemuKernel.dsc | 10 += - >> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 434 += +++ >> ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf | 57 += >> ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c | 108 += - >> ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf | 2 += >> ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.c | 1496 -= ------------- >> ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.h | 499 -= ---- >> ArmVirtPkg/PciHostBridgeDxe/PciHostBridgeDxe.inf | 64 -= >> ArmVirtPkg/PciHostBridgeDxe/PciRootBridgeIo.c | 2144 -= ------------------- >> 11 files changed, 611 insertions(+), 4216 deletions(-) >> create mode 100644 ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridg= eLib.c >> create mode 100644 ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridg= eLib.inf >> delete mode 100644 ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.c >> delete mode 100644 ArmVirtPkg/PciHostBridgeDxe/PciHostBridge.h >> delete mode 100644 ArmVirtPkg/PciHostBridgeDxe/PciHostBridgeDxe.inf >> delete mode 100644 ArmVirtPkg/PciHostBridgeDxe/PciRootBridgeIo.c >=20 > Test results: >=20 > (1) aarch64 TCG on an x86_64 host, using standard VGA (-device VGA) and US= B 3 keyboard (-device nec-usb-xhci -device usb-kbd). I tested the display br= owser, the UEFI shell, and the GRUB menu. Didn't boot an actual OS. (Ain't n= obody got time for that using TCG :)) >=20 > I also checked the ArmVirtQemu debug log, it looks very nice; there was ev= en a 64-bit Mem64 BAR that got allocated high. >=20 > (2) aarch64 KVM, using virtio-gpu-pci and USB 2 keyboard and tablet. I act= ually 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. >=20 > (2a) However, the USB 2 keyboard is broken while in the firmware (in spite= of it working well in the guest OS). >=20 > -device ich9-usb-ehci1,multifunction=3Don,id=3Dehci,addr=3D05.0 \ > -device ich9-usb-uhci1,multifunction=3Don,masterbus=3Dehci.0,firstport=3D= 0,addr=3D05.1 \ > -device ich9-usb-uhci2,multifunction=3Don,masterbus=3Dehci.0,firstport=3D= 2,addr=3D05.2 \ > -device ich9-usb-uhci3,multifunction=3Don,masterbus=3Dehci.0,firstport=3D= 4,addr=3D05.3 \ > -device usb-kbd,bus=3Dehci.0 \ > -device usb-tablet,bus=3Dehci.0 \ >=20 > My QEMU has your commit 5d636e21c44e ("hw/arm/virt: mark the PCIe host con= troller as DMA coherent in the DT"), but I guess the EHCI driver in edk2 doe= sn't comply with the "guest drivers should use cacheable accesses as well wh= en running under KVM" part. :( >=20 > The following snippet repeats in the log: >=20 > 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 >=20 > 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! >=20 Does it work when you limit DMA to < 4 GB? > (2b) Your series does not break virtio-{scsi,blk}-pci though, I verified t= hat. >=20 > (3) Another issue I noticed in testing is that the following line from pat= ch #3 ("ArmVirtPkg: implement FdtPciHostBridgeLib"): >=20 > + EISA_PNP_ID(0x0A08), // PCI Express >=20 > which I initially thought would be okay actually breaks "OvmfPkg/Library/Q= emuBootOrderLib". The TranslatePciOfwNodes() function translates OFW devpath= s into textual UEFI devpath fragments that start with PciRoot(), not PcieRoo= t(), and after your patch, these prefixes will no longer match the UEFI devp= aths of the actual PCI devices. >=20 > So, for this I'll have to go back on my initial approval, and ask you to k= eep this as 0x0A03. Yes, I know it won't match QEMU's ACPI payload perfectly= , but the same is the case with the Q35 x86_64 machine type -- search "hw/i3= 86/acpi-build.c" for the string "PNP0A08" -- and in practice it causes no pr= oblems at all. >=20 > (4) This is for the longer term, but now that we have 64-bit MMIO aperture= , we should include OvmfPkg/IncompatiblePciDeviceSupportDxe in ArmVirtQemu (= please see commit 855743f71774). The circumstances described in the commit m= essage now apply to ArmVirtQemu as well, if at least virtio-net-pci is used -= - that device has an oprom, and by default that is reason enough for PciBusD= xe to degrade the 64-bit MMIO BAR to 32-bit. Including "OvmfPkg/Incompatible= PciDeviceSupportDxe" will prevent this. >=20 >=20 > I think (3) can be fixed up easily without a repost, and (4) can be addres= sed with a separate patch, later. >=20 > However, I'm completely dumbfounded about (2a). Please do not tell me that= we'll have to implement a virtio-input driver... :( I think there must be a= mysterious caching difference between "MdeModulePkg/Bus/Pci/PciHostBridgeDx= e" and "ArmVirtPkg/PciHostBridgeDxe", even after having removed the "kludge"= from the latter! >=20 > Maybe it has to do with the IO aperture. The USB1 companion controllers on= the EHCI controller have one IO BAR each: >=20 > PciBus: Resource Map for Root Bridge PcieRoot(0x0) > Type =3D Io16; Base =3D 0x0; Length =3D 0x1000; Alignment =3D= 0xFFF > [...] > Base =3D 0x80; Length =3D 0x20; Alignment =3D 0x1F; Owner =3D PCI= [00|05|03:20] > Base =3D 0xA0; Length =3D 0x20; Alignment =3D 0x1F; Owner =3D PCI= [00|05|02:20] > Base =3D 0xC0; Length =3D 0x20; Alignment =3D 0x1F; Owner =3D PCI= [00|05|01:20] > [...] >=20 > and with your patches applied, accesses to these are now delegated to ArmP= ciCpuIo2Dxe. Maybe that makes a difference? I'm just guessing. (Again, the v= irtio-xxxx-pci devices, even forcing them to use IO BARs, with ",disable-mod= ern=3Don", continue to work fine.) >=20 > Since you authored QEMU commit 5d636e21c44e, I reckon (hope!) you have som= e background on the USB thing... What's your take? >=20 > Thanks > Laszlo