(on the road atm, will reply in full later)
> On 2 sep. 2016, at 14:09, Laszlo Ersek <[email protected]> wrote:
>
>> 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!
>
Does it work when you limit DMA to < 4 GB?
> (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
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
