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

[Laszlo Ersek <lersek@redhat.com>]

On 08/31/16 19:59, Ard Biesheuvel wrote:
> Now that Laszlo's virtio-gpu-pci series has removed the last remaining obstacle,
> we can get rid of the special PciHostBridgeDxe implementation in ArmVirtPkg,
> and move to the generic one. On AArch64, this will allow us to perform DMA above
> 4GB without bounce buffering, and use 64-bit MMIO BARs allocated above 4 GB.
> 
> Changes since v1:
> - new patch #2 to move the IoTranslation discovery to FdtPciPcdProducerLib,
>   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 and
>   where the version of the patch in this series does not deviate substantially
>   from the suggested version on which the preliminary ack was based
> 
> Patch #1 removes the linux,pci-probe-only override which does more harm than
> good now that we switched to virtio-gpu-pci, which does not expose a raw
> framebuffer.
> 
> Patch #2 extends FdtPciPcdProducerLib so that it also sets PcdPciIoTranslation,
> which is required for the FdtPciHostBridgeLib implementation this series
> introduces, but also for ArmPciCpuIo2Dxe, which produces EFI_CPU_IO2_PROTOCOL
> on which the generic PciHostBridgeDxe depends as well.
> 
> Patch #3 implements PciHostBridgeLib for platforms exposing a PCI host bridge
> using a pci-host-ecam-generic DT node. The initial version is based on the
> 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 without
> bounce buffering.
> 
> Patch #4 switches to the generic PciHostBridgeDxe, with no change in
> functionality other than support for DMA above 4 GB without bounce buffering.
> 
> Patch #5 adds support for allocating 64-bit MMIO BARs above 4 GB.
> 
> Patch #6 removes the now obsolete PciHostBridgeDxe from ArmVirtPkg.
> 
> 
> 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
> 
>  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/FdtPciHostBridgeLib.c
>  create mode 100644 ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.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
> 

Test results:

(1) aarch64 TCG on an x86_64 host, using standard VGA (-device VGA) and USB 3 keyboard (-device nec-usb-xhci -device usb-kbd). I tested the display browser, the UEFI shell, and the GRUB menu. Didn't boot an actual OS. (Ain't nobody got time for that using TCG :))

I also checked the ArmVirtQemu debug log, it looks very nice; there was even a 64-bit Mem64 BAR that got allocated high.

(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!

(2b) Your series does not break virtio-{scsi,blk}-pci though, I verified that.

(3) Another issue I noticed in testing is that the following line from patch #3 ("ArmVirtPkg: implement FdtPciHostBridgeLib"):

+    EISA_PNP_ID(0x0A08), // PCI Express

which I initially thought would be okay actually breaks "OvmfPkg/Library/QemuBootOrderLib". The TranslatePciOfwNodes() function translates OFW devpaths into textual UEFI devpath fragments that start with PciRoot(), not PcieRoot(), and after your patch, these prefixes will no longer match the UEFI devpaths of the actual PCI devices.

So, for this I'll have to go back on my initial approval, and ask you to keep 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/i386/acpi-build.c" for the string "PNP0A08" -- and in practice it causes no problems at all.

(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 message 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 PciBusDxe to degrade the 64-bit MMIO BAR to 32-bit. Including "OvmfPkg/IncompatiblePciDeviceSupportDxe" will prevent this.


I think (3) can be fixed up easily without a repost, and (4) can be addressed with a separate patch, later.

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/PciHostBridgeDxe" and "ArmVirtPkg/PciHostBridgeDxe", even after having removed the "kludge" from the latter!

Maybe it has to do with the IO aperture. The USB1 companion controllers on the EHCI controller have one IO BAR each:

PciBus: Resource Map for Root Bridge PcieRoot(0x0)
Type =   Io16; Base = 0x0;      Length = 0x1000;        Alignment = 0xFFF
   [...]
   Base = 0x80; Length = 0x20;  Alignment = 0x1F;       Owner = PCI [00|05|03:20]
   Base = 0xA0; Length = 0x20;  Alignment = 0x1F;       Owner = PCI [00|05|02:20]
   Base = 0xC0; Length = 0x20;  Alignment = 0x1F;       Owner = PCI [00|05|01:20]
   [...]

and with your patches applied, accesses to these are now delegated to ArmPciCpuIo2Dxe. Maybe that makes a difference? I'm just guessing. (Again, the virtio-xxxx-pci devices, even forcing them to use IO BARs, with ",disable-modern=on", continue to work fine.)

Since you authored QEMU commit 5d636e21c44e, I reckon (hope!) you have some background on the USB thing... What's your take?

Thanks
Laszlo