From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 45DB41A1E49 for ; Wed, 31 Aug 2016 07:06:10 -0700 (PDT) Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C80FB80F75; Wed, 31 Aug 2016 14:06:09 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-108.phx2.redhat.com [10.3.116.108]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u7VE68Pr012918; Wed, 31 Aug 2016 10:06:08 -0400 To: Ard Biesheuvel , edk2-devel@ml01.01.org References: <1471847752-26574-1-git-send-email-ard.biesheuvel@linaro.org> <1471847752-26574-5-git-send-email-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: Date: Wed, 31 Aug 2016 16:06:07 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <1471847752-26574-5-git-send-email-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Wed, 31 Aug 2016 14:06:09 +0000 (UTC) Subject: Re: [PATCH 4/5] ArmVirtPkg/FdtPciHostBridgeLib: add MMIO64 support 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: Wed, 31 Aug 2016 14:06:10 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 08/22/16 08:35, Ard Biesheuvel wrote: > If the pci-host-ecam-generic DT node describes a 64-bit MMIO region, > account for it in the PCI_ROOT_BRIDGE description that we return to > the generic PciHostBridgeDxe implementation, which will be able to > allocate BARs from it without any further changes. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel > --- > ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c | 59 +++++++++++++------- > 1 file changed, 38 insertions(+), 21 deletions(-) (1) So this patch will have to be rebased to the IoTranslation change discussed under patch 2/5, > diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c > index 0aff149e8029..74fda4d23fa3 100644 > --- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c > +++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c > @@ -87,9 +87,10 @@ ProcessPciHost ( > OUT UINT64 *IoBase, > OUT UINT64 *IoSize, > OUT UINT64 *IoTranslation, > - OUT UINT64 *MmioBase, > - OUT UINT64 *MmioSize, > - OUT UINT64 *MmioTranslation, > + OUT UINT64 *Mmio32Base, > + OUT UINT64 *Mmio32Size, > + OUT UINT64 *Mmio64Base, > + OUT UINT64 *Mmio64Size, > OUT UINT32 *BusMin, > OUT UINT32 *BusMax > ) > @@ -101,6 +102,7 @@ ProcessPciHost ( > UINT32 Len; > UINT32 RecordIdx; > EFI_STATUS Status; > + UINT64 MmioTranslation; > > // > // The following output arguments are initialized only in > @@ -109,8 +111,8 @@ ProcessPciHost ( > // > *IoBase = 0; > *IoTranslation = 0; > - *MmioBase = 0; > - *MmioTranslation = 0; > + *Mmio32Base = 0; > + *Mmio64Base = MAX_UINT64; > *BusMin = 0; > *BusMax = 0; > > @@ -120,7 +122,8 @@ ProcessPciHost ( > // above, they are initialized early. > // > *IoSize = 0; > - *MmioSize = 0; > + *Mmio32Size = 0; > + *Mmio64Size = 0; (2) and here the leading comment should be updated -- at the moment it refers to IoSize and MmioSize. > > Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL, > (VOID **)&FdtClient); > @@ -207,26 +210,39 @@ ProcessPciHost ( > break; > > case DTB_PCI_HOST_RANGE_MMIO32: > - *MmioBase = SwapBytes64 (Record->ChildBase); > - *MmioSize = SwapBytes64 (Record->Size); > - *MmioTranslation = SwapBytes64 (Record->CpuBase) - *MmioBase; > + *Mmio32Base = SwapBytes64 (Record->ChildBase); > + *Mmio32Size = SwapBytes64 (Record->Size); > + MmioTranslation = SwapBytes64 (Record->CpuBase) - *Mmio32Base; > > - if (*MmioBase > MAX_UINT32 || *MmioSize > MAX_UINT32 || > - *MmioBase + *MmioSize > SIZE_4GB) { > + if (*Mmio32Base > MAX_UINT32 || *Mmio32Size > MAX_UINT32 || > + *Mmio32Base + *Mmio32Size > SIZE_4GB) { > DEBUG ((EFI_D_ERROR, "%a: MMIO32 space invalid\n", __FUNCTION__)); > return EFI_PROTOCOL_ERROR; > } > > - if (*MmioTranslation != 0) { > + if (MmioTranslation != 0) { > DEBUG ((EFI_D_ERROR, "%a: unsupported nonzero MMIO32 translation " > - "0x%Lx\n", __FUNCTION__, *MmioTranslation)); > + "0x%Lx\n", __FUNCTION__, MmioTranslation)); > + return EFI_UNSUPPORTED; > + } > + > + break; > + > + case DTB_PCI_HOST_RANGE_MMIO64: > + *Mmio64Base = SwapBytes64 (Record->ChildBase); > + *Mmio64Size = SwapBytes64 (Record->Size); > + MmioTranslation = SwapBytes64 (Record->CpuBase) - *Mmio64Base; > + > + if (MmioTranslation != 0) { > + DEBUG ((EFI_D_ERROR, "%a: unsupported nonzero MMIO64 translation " > + "0x%Lx\n", __FUNCTION__, MmioTranslation)); > return EFI_UNSUPPORTED; > } > > break; > } > } > - if (*IoSize == 0 || *MmioSize == 0) { > + if (*IoSize == 0 || *Mmio32Size == 0) { > DEBUG ((EFI_D_ERROR, "%a: %a space empty\n", __FUNCTION__, > (*IoSize == 0) ? "IO" : "MMIO32")); > return EFI_PROTOCOL_ERROR; > @@ -239,9 +255,9 @@ ProcessPciHost ( > ASSERT (PcdGet64 (PcdPciExpressBaseAddress) == ConfigBase); > > DEBUG ((EFI_D_INFO, "%a: Config[0x%Lx+0x%Lx) Bus[0x%x..0x%x] " > - "Io[0x%Lx+0x%Lx)@0x%Lx Mem[0x%Lx+0x%Lx)@0x%Lx\n", __FUNCTION__, ConfigBase, > - ConfigSize, *BusMin, *BusMax, *IoBase, *IoSize, *IoTranslation, *MmioBase, > - *MmioSize, *MmioTranslation)); > + "Io[0x%Lx+0x%Lx)@0x%Lx Mem32[0x%Lx+0x%Lx) Mem64[0x%Lx+0x%Lx)\n", > + __FUNCTION__, ConfigBase, ConfigSize, *BusMin, *BusMax, *IoBase, *IoSize, > + *IoTranslation, *Mmio32Base, *Mmio32Size, *Mmio64Base, *Mmio64Size)); > return EFI_SUCCESS; > } > > @@ -263,7 +279,8 @@ PciHostBridgeGetRootBridges ( > { > PCI_ROOT_BRIDGE *RootBridge; > UINT64 IoBase, IoSize, IoTranslation; > - UINT64 Mmio32Base, Mmio32Size, Mmio32Translation; > + UINT64 Mmio32Base, Mmio32Size; > + UINT64 Mmio64Base, Mmio64Size; > UINT32 BusMin, BusMax; > EFI_STATUS Status; > > @@ -275,7 +292,7 @@ PciHostBridgeGetRootBridges ( > } > > Status = ProcessPciHost (&IoBase, &IoSize, &IoTranslation, &Mmio32Base, > - &Mmio32Size, &Mmio32Translation, &BusMin, &BusMax); > + &Mmio32Size, &Mmio64Base, &Mmio64Size, &BusMin, &BusMax); > if (EFI_ERROR (Status)) { > DEBUG ((EFI_D_INFO, > "%a: failed to discover PCI host bridge: %s not present\n", > @@ -304,8 +321,8 @@ PciHostBridgeGetRootBridges ( > RootBridge->Io.Limit = IoBase + IoSize - 1; > RootBridge->Mem.Base = Mmio32Base; > RootBridge->Mem.Limit = Mmio32Base + Mmio32Size - 1; > - RootBridge->MemAbove4G.Base = MAX_UINT64; > - RootBridge->MemAbove4G.Limit = 0; > + RootBridge->MemAbove4G.Base = Mmio64Base; > + RootBridge->MemAbove4G.Limit = Mmio64Base + Mmio64Size - 1; > > // > // No separate ranges for prefetchable and non-prefetchable BARs > With those updates, the patch looks good to me: Reviewed-by: Laszlo Ersek Thanks Laszlo