From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web10.7218.1585835552525029027 for ; Thu, 02 Apr 2020 06:52:32 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=hkvXTlfo; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585835551; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=b7mrLBAJl0/ECjBjX4FobRWmZTaTHtyh5qxQ6Jqj5tc=; b=hkvXTlfoXDPeZr0QLFrTnmJmt0TDFuCPZgX2ZS2esFoVEocu1JueenTz9QoY9VjXhpIsak fv2QKGe2lFB/ZiYi/AVwhawVtNFPe3tHRctmKU1RXTvcIs/f16vUHsXGNjoCUenPdfB5UB zcZR/qMa71hkBT0GC1FOPZbOLTlWSH8= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-281-hqATwob4NVyyKClAcnes8w-1; Thu, 02 Apr 2020 09:52:26 -0400 X-MC-Unique: hqATwob4NVyyKClAcnes8w-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id DF658107B78A; Thu, 2 Apr 2020 13:52:24 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-186.ams2.redhat.com [10.36.115.186]) by smtp.corp.redhat.com (Postfix) with ESMTP id 199F696B9B; Thu, 2 Apr 2020 13:52:22 +0000 (UTC) Subject: Re: [PATCH] ArmVirtPkg/FdtPciHostBridgeLib: add extra roots for Arm. To: Yubo Miao , ard.biesheuvel@linaro.org, leif@nuviainc.com Cc: devel@edk2.groups.io, xiexiangyou@huawei.com References: <20200402120803.1482-1-miaoyubo@huawei.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 2 Apr 2020 15:52:21 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200402120803.1482-1-miaoyubo@huawei.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Quick review comments only, for now: On 04/02/20 14:08, Yubo Miao wrote: > From: miaoyubo > > Add support for extra roots for arm, in this patch, we > import the scan for extra root buses therefore the uefi > could recognize multiply roots for arm. > > The logic keeps the same with the logic in > "OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c" > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: miaoyubo > --- > .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 264 +++++++++++++++--- > .../FdtPciHostBridgeLib.inf | 3 + > 2 files changed, 230 insertions(+), 37 deletions(-) (1) The "Contributed-under:" line is no longer necessary (even defined) in the commit message. (2) This code is way too large for my taste to duplicate between ArmVirtPkg and OvmfPkg. I would strongly prefer if we could factor the logic in OvmfPkg out to a separate library, and use that from both consumer sites. (3) Can you please provide a pointer to the QEMU-side work? In particular, this logic depdens on the "etc/extra-pci-roots" fw_cfg file. Where is that file being exposed by qemu-system-aarch64 / "virt"? In general, the firmware code is merged after the QEMU work is merged. Has the design been accepted for QEMU at least? (So that it make sense for us to look at the interfacing ArmVirtPkg code.) (4) Have you tested booting from PCI devices behind the "extra" root bridges? In particular, have you tested the boot order manipulation via the "bootindex" device properties? (OvmfPkg/Library/QemuBootOrderLib is already being used by the ArmVirtQemu platform, so I'd expect no changes related to boot order filtering / reordering -- but it should be tested.) (5) I think this feature deserves a TianoCore Feature Request. Can you please file one at ? Then the bugzilla link should be referenced in the commit message. (Preferably the bugzilla entry should summarize the present QEMU status too, or provide further links to QEMU discussions etc.) Thanks, Laszlo > > diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c > index 496b192d22..706efeb416 100644 > --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c > +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c > @@ -14,6 +14,10 @@ > #include > #include > #include > +#include > +#include > +#include > +#include > > #include > #include > @@ -306,7 +310,70 @@ ProcessPciHost ( > return Status; > } > > -STATIC PCI_ROOT_BRIDGE mRootBridge; > +EFI_STATUS > +InitRootBridge ( > + IN UINT64 Supports, > + IN UINT64 Attributes, > + IN UINT64 AllocAttributes, > + IN UINT8 RootBusNumber, > + IN UINT8 MaxSubBusNumber, > + IN PCI_ROOT_BRIDGE_APERTURE *Io, > + IN PCI_ROOT_BRIDGE_APERTURE *Mem, > + IN PCI_ROOT_BRIDGE_APERTURE *MemAbove4G, > + IN PCI_ROOT_BRIDGE_APERTURE *PMem, > + IN PCI_ROOT_BRIDGE_APERTURE *PMemAbove4G, > + OUT PCI_ROOT_BRIDGE *RootBus > + ) > +{ > + EFI_PCI_ROOT_BRIDGE_DEVICE_PATH *DevicePath; > + > + // > + // Be safe if other fields are added to PCI_ROOT_BRIDGE later. > + // > + ZeroMem (RootBus, sizeof *RootBus); > + > + RootBus->Segment = 0; > + RootBus->ResourceAssigned = FALSE; > + RootBus->Supports = Supports; > + RootBus->Attributes = Attributes; > + > + RootBus->DmaAbove4G = TRUE; > + > + RootBus->AllocationAttributes = AllocAttributes; > + RootBus->Bus.Base = RootBusNumber; > + RootBus->Bus.Limit = MaxSubBusNumber; > + CopyMem (&RootBus->Io, Io, sizeof (*Io)); > + CopyMem (&RootBus->Mem, Mem, sizeof (*Mem)); > + CopyMem (&RootBus->MemAbove4G, MemAbove4G, sizeof (*MemAbove4G)); > + CopyMem (&RootBus->PMem, PMem, sizeof (*PMem)); > + CopyMem (&RootBus->PMemAbove4G, PMemAbove4G, sizeof (*PMemAbove4G)); > + > + RootBus->NoExtendedConfigSpace = FALSE; > + > + DevicePath = AllocateCopyPool (sizeof mEfiPciRootBridgeDevicePath, > + &mEfiPciRootBridgeDevicePath); > + if (DevicePath == NULL) { > + DEBUG ((EFI_D_ERROR, "%a: %r\n", __FUNCTION__, EFI_OUT_OF_RESOURCES)); > + return EFI_OUT_OF_RESOURCES; > + } > + DevicePath->AcpiDevicePath.UID = RootBusNumber; > + RootBus->DevicePath = (EFI_DEVICE_PATH_PROTOCOL *)DevicePath; > + > + DEBUG ((EFI_D_INFO, > + "%a: populated root bus %d, with room for %d subordinate bus(es)\n", > + __FUNCTION__, RootBusNumber, MaxSubBusNumber - RootBusNumber)); > + return EFI_SUCCESS; > +} > + > +STATIC > +VOID > +UninitRootBridge ( > + IN PCI_ROOT_BRIDGE *RootBus > + ) > +{ > + FreePool (RootBus->DevicePath); > +} > + > > /** > Return all the root bridge instances in an array. > @@ -323,11 +390,25 @@ PciHostBridgeGetRootBridges ( > UINTN *Count > ) > { > - UINT64 IoBase, IoSize; > - UINT64 Mmio32Base, Mmio32Size; > - UINT64 Mmio64Base, Mmio64Size; > - UINT32 BusMin, BusMax; > - EFI_STATUS Status; > + UINT64 IoBase, IoSize; > + UINT64 Mmio32Base, Mmio32Size; > + UINT64 Mmio64Base, Mmio64Size; > + UINT32 BusMin, BusMax; > + FIRMWARE_CONFIG_ITEM FwCfgItem; > + UINTN FwCfgSize; > + UINT64 ExtraRootBridges; > + PCI_ROOT_BRIDGE *Bridges; > + UINTN Initialized; > + UINTN LastRootBridgeNumber; > + UINTN RootBridgeNumber; > + UINT64 Attributes; > + UINT64 AllocationAttributes; > + EFI_STATUS Status; > + PCI_ROOT_BRIDGE_APERTURE Io; > + PCI_ROOT_BRIDGE_APERTURE PMem; > + PCI_ROOT_BRIDGE_APERTURE PMemAbove4G; > + PCI_ROOT_BRIDGE_APERTURE Mem; > + PCI_ROOT_BRIDGE_APERTURE MemAbove4G; > > if (PcdGet64 (PcdPciExpressBaseAddress) == 0) { > DEBUG ((EFI_D_INFO, "%a: PCI host bridge not present\n", __FUNCTION__)); > @@ -345,33 +426,27 @@ PciHostBridgeGetRootBridges ( > return NULL; > } > > - *Count = 1; > + ZeroMem (&Io, sizeof (Io)); > + ZeroMem (&Mem, sizeof (Mem)); > + ZeroMem (&MemAbove4G, sizeof (MemAbove4G)); > > - mRootBridge.Segment = 0; > - mRootBridge.Supports = EFI_PCI_ATTRIBUTE_ISA_IO_16 | > - EFI_PCI_ATTRIBUTE_ISA_MOTHERBOARD_IO | > - EFI_PCI_ATTRIBUTE_VGA_IO_16 | > - EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO_16; > - mRootBridge.Attributes = mRootBridge.Supports; > + Attributes = EFI_PCI_ATTRIBUTE_ISA_IO_16 | > + EFI_PCI_ATTRIBUTE_ISA_MOTHERBOARD_IO | > + EFI_PCI_ATTRIBUTE_VGA_IO_16 | > + EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO_16; > > - mRootBridge.DmaAbove4G = TRUE; > - mRootBridge.NoExtendedConfigSpace = FALSE; > - mRootBridge.ResourceAssigned = FALSE; > + AllocationAttributes = EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM; > > - mRootBridge.AllocationAttributes = EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM; > - > - mRootBridge.Bus.Base = BusMin; > - mRootBridge.Bus.Limit = BusMax; > - mRootBridge.Io.Base = IoBase; > - mRootBridge.Io.Limit = IoBase + IoSize - 1; > - mRootBridge.Mem.Base = Mmio32Base; > - mRootBridge.Mem.Limit = Mmio32Base + Mmio32Size - 1; > + Io.Base = IoBase; > + Io.Limit = IoBase + IoSize - 1; > + Mem.Base = Mmio32Base; > + Mem.Limit = Mmio32Base + Mmio32Size - 1; > > if (sizeof (UINTN) == sizeof (UINT64)) { > - mRootBridge.MemAbove4G.Base = Mmio64Base; > - mRootBridge.MemAbove4G.Limit = Mmio64Base + Mmio64Size - 1; > + MemAbove4G.Base = Mmio64Base; > + MemAbove4G.Limit = Mmio64Base + Mmio64Size - 1; > if (Mmio64Size > 0) { > - mRootBridge.AllocationAttributes |= EFI_PCI_HOST_BRIDGE_MEM64_DECODE; > + AllocationAttributes |= EFI_PCI_HOST_BRIDGE_MEM64_DECODE; > } > } else { > // > @@ -380,21 +455,127 @@ PciHostBridgeGetRootBridges ( > // BARs unless they are allocated below 4 GB. So ignore the range above > // 4 GB in this case. > // > - mRootBridge.MemAbove4G.Base = MAX_UINT64; > - mRootBridge.MemAbove4G.Limit = 0; > + MemAbove4G.Base = MAX_UINT64; > + MemAbove4G.Limit = 0; > } > > // > // No separate ranges for prefetchable and non-prefetchable BARs > // > - mRootBridge.PMem.Base = MAX_UINT64; > - mRootBridge.PMem.Limit = 0; > - mRootBridge.PMemAbove4G.Base = MAX_UINT64; > - mRootBridge.PMemAbove4G.Limit = 0; > + PMem.Base = MAX_UINT64; > + PMem.Limit = 0; > + PMemAbove4G.Base = MAX_UINT64; > + PMemAbove4G.Limit = 0; > + > + // > + // QEMU provides the number of extra root buses, shortening the exhaustive > + // search below. If there is no hint, the feature is missing. > + // > + Status = QemuFwCfgFindFile ("etc/extra-pci-roots", &FwCfgItem, &FwCfgSize); > + if (EFI_ERROR (Status) || FwCfgSize != sizeof ExtraRootBridges) { > + ExtraRootBridges = 0; > + } else { > + QemuFwCfgSelectItem (FwCfgItem); > + QemuFwCfgReadBytes (FwCfgSize, &ExtraRootBridges); > > - mRootBridge.DevicePath = (EFI_DEVICE_PATH_PROTOCOL *)&mEfiPciRootBridgeDevicePath; > + if (ExtraRootBridges > BusMax) { > + DEBUG ((EFI_D_ERROR, "%a: invalid count of extra root buses (%Lu) " > + "reported by QEMU\n", __FUNCTION__, ExtraRootBridges)); > + return NULL; > + } > + DEBUG ((EFI_D_INFO, "%a: %Lu extra root buses reported by QEMU\n", > + __FUNCTION__, ExtraRootBridges)); > + } > > - return &mRootBridge; > + // > + // Allocate the "main" root bridge, and any extra root bridges. > + // > + Bridges = AllocatePool ((1 + (UINTN)ExtraRootBridges) * sizeof *Bridges); > + if (Bridges == NULL) { > + DEBUG ((EFI_D_ERROR, "%a: %r\n", __FUNCTION__, EFI_OUT_OF_RESOURCES)); > + return NULL; > + } > + Initialized = 0; > + > + // > + // The "main" root bus is always there. > + // > + LastRootBridgeNumber = 0; > + // > + // Scan all other root buses. If function 0 of any device on a bus returns a > + // VendorId register value different from all-bits-one, then that bus is > + // alive. > + // > + for (RootBridgeNumber = 1; > + RootBridgeNumber <= BusMax && Initialized < ExtraRootBridges; > + ++RootBridgeNumber) { > + UINTN Device; > + > + for (Device = 0; Device <= PCI_MAX_DEVICE; ++Device) { > + if (PciRead16 (PCI_LIB_ADDRESS (RootBridgeNumber, Device, 0, > + PCI_VENDOR_ID_OFFSET)) != MAX_UINT16) { > + break; > + } > + } > + if (Device <= PCI_MAX_DEVICE) { > + // > + // Found the next root bus. We can now install the *previous* one, > + // because now we know how big a bus number range *that* one has, for any > + // subordinate buses that might exist behind PCI bridges hanging off it. > + // > + Status = InitRootBridge ( > + Attributes, > + Attributes, > + AllocationAttributes, > + (UINT8) LastRootBridgeNumber, > + (UINT8) (RootBridgeNumber - 1), > + &Io, > + &Mem, > + &MemAbove4G, > + &PMem, > + &PMemAbove4G, > + &Bridges[Initialized] > + ); > + if (EFI_ERROR (Status)) { > + goto FreeBridges; > + } > + ++Initialized; > + LastRootBridgeNumber = RootBridgeNumber; > + } > + } > + // > + // Install the last root bus (which might be the only, ie. main, root bus, if > + // we've found no extra root buses). > + // > + Status = InitRootBridge ( > + Attributes, > + Attributes, > + AllocationAttributes, > + (UINT8) LastRootBridgeNumber, > + BusMax, > + &Io, > + &Mem, > + &MemAbove4G, > + &PMem, > + &PMemAbove4G, > + &Bridges[Initialized] > + ); > + if (EFI_ERROR (Status)) { > + goto FreeBridges; > + } > + ++Initialized; > + > + *Count = Initialized; > + return Bridges; > + > +FreeBridges: > + while (Initialized > 0) { > + --Initialized; > + UninitRootBridge (&Bridges[Initialized]); > + } > + > + FreePool (Bridges); > + return NULL; > } > > /** > @@ -411,9 +592,18 @@ PciHostBridgeFreeRootBridges ( > UINTN Count > ) > { > - ASSERT (Count == 1); > -} > + if (Bridges == NULL && Count == 0) { > + return; > + } > + ASSERT (Bridges != NULL && Count > 0); > + > + do { > + --Count; > + UninitRootBridge (&Bridges[Count]); > + } while (Count > 0); > > + FreePool (Bridges); > +} > /** > Inform the platform that the resource conflict happens. > > diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf > index 277ccfd245..3ac58855af 100644 > --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf > +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf > @@ -31,11 +31,14 @@ > ArmVirtPkg/ArmVirtPkg.dec > MdeModulePkg/MdeModulePkg.dec > MdePkg/MdePkg.dec > + OvmfPkg/OvmfPkg.dec > > [LibraryClasses] > DebugLib > DevicePathLib > DxeServicesTableLib > + BaseMemoryLib > + QemuFwCfgLib > MemoryAllocationLib > PciPcdProducerLib > >