From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::241; helo=mail-io0-x241.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x241.google.com (mail-io0-x241.google.com [IPv6:2607:f8b0:4001:c06::241]) (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 9E328207E5412 for ; Thu, 24 May 2018 01:08:57 -0700 (PDT) Received: by mail-io0-x241.google.com with SMTP id c9-v6so1219328iob.12 for ; Thu, 24 May 2018 01:08:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=HJeKdTsrzQKAHEjD2tQ5X1Kg489pCRcWuJBcvyOCdhM=; b=gUT4tuTKAWGtsIr7qvoxGgNGnHWNKb+PeUOMrlmIVSSQkDPmOfugj6eygncU6h6+YB +eVlrQfmq44MCjoJJuJAI7u0CgSrYpMWv+i2hvHFoHjvozqQ+fzSHVbsM9lJJZzZlmqJ ApIC38UTT5kVPwfNVxIR6APeUktl10aLPOT5A= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=HJeKdTsrzQKAHEjD2tQ5X1Kg489pCRcWuJBcvyOCdhM=; b=Vfu5NxCYj/+lKiT8xB/Nf3Cu4Xk37pc+y7RMc4t8vzZliA2n5UQFwM3IR/HUve2Yf2 7TYcE2uIsuF1sr4+VVvgdL9Umfp8cR69Cho/kSbRFOSMTNZNgF/b78zE+hzqlxU8sw0V coOqoEZRsFs7GssDnV4JjxFZu37c8Ya8rnwRc2FGqrdlRFZ1eBSrMNuPp4vt0AyVJT1T apmL1SPgxRM6+KUOfe4YpK41B119ji6VQ2fp0XYNCQjxTXZ0rqYyJD0K9sHw/bAzt3Wq 67fz8Bji5sAjiCUGEV99U+Q9XNSjlcHJoxjjoNqOlBktX5yFbGcAb1439/fWAtqyudP3 wSnA== X-Gm-Message-State: ALKqPwedglUZIbsntK57gQv42MiFUhY8Nrv0KvdU88pYK9d6Rjhl3sG8 1Va4bmEAH+fpzKPFrEs0IlFctOuwh2e6RVz8fqpqfA== X-Google-Smtp-Source: AB8JxZqCmFlq619JWu44/K9TL3P5HazYB5YOOHelElAwJ4U6BQjuSnXN+moBFWgfTRfPY6kkTOMMx3pwlG3MrppPKaU= X-Received: by 2002:a6b:545:: with SMTP id 66-v6mr5652773iof.173.1527149336755; Thu, 24 May 2018 01:08:56 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bb86:0:0:0:0:0 with HTTP; Thu, 24 May 2018 01:08:56 -0700 (PDT) In-Reply-To: <20180523202121.8125-3-lersek@redhat.com> References: <20180523202121.8125-1-lersek@redhat.com> <20180523202121.8125-3-lersek@redhat.com> From: Ard Biesheuvel Date: Thu, 24 May 2018 10:08:56 +0200 Message-ID: To: Laszlo Ersek Cc: edk2-devel-01 , Jordan Justen Subject: Re: [PATCH v2 2/7] OvmfPkg: introduce PciCapPciSegmentLib X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 May 2018 08:08:58 -0000 Content-Type: text/plain; charset="UTF-8" On 23 May 2018 at 22:21, Laszlo Ersek wrote: > Add a library class, and a BASE lib instance, that are layered on top of > PciCapLib, and allow clients to plug a PciSegmentLib backend into > PciCapLib, for config space access. > > (Side note: > For the record: I am fine with keeping this side note in the commit log. > The "MaxDomain" parameter is provided because, in practice, platforms > exist where a PCI Express device may show up on a root bridge such that > the root bridge doesn't support access to extended config space. Earlier > the same issue was handled for MdeModulePkg/PciHostBridgeDxe in commit > 014b472053ae3. However, that solution does not apply to the PciSegmentLib > class, because: > > (1) The config space accessor functions of the PciSegmentLib class, such > as PciSegmentReadBuffer(), have no way of informing the caller whether > access to extended config space actually succeeds. > > (For example, in the UefiPciSegmentLibPciRootBridgeIo instace, which > could in theory benefit from commit 014b472053ae3, the > EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.Pci.Read() status code is explicitly > ignored, because there's no way for the lib instance to propagate it > to the PciSegmentLib caller. If the > EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.Pci.Read() call fails, then > DxePciSegmentLibPciRootBridgeIoReadWorker() returns Data with > indeterminate value.) > > (2) There is no *general* way for any firmware platform to provide, or > use, a PciSegmentLib instance in which access to extended config space > always succeeds. > > In brief, on a platform where config space may be limited to 256 bytes, > access to extended config space through PciSegmentLib may invoke undefined > behavior; therefore PciCapPciSegmentLib must give platforms a way to > prevent such access.) > > Cc: Ard Biesheuvel > Cc: Jordan Justen > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek > --- > > Notes: > v2: > - move library from MdePkg to OvmfPkg, for initial introduction > > OvmfPkg/OvmfPkg.dec | 5 + > OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf | 35 +++ > OvmfPkg/Include/Library/PciCapPciSegmentLib.h | 82 +++++++ > OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h | 47 ++++ > OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c | 226 ++++++++++++++++++++ > 5 files changed, 395 insertions(+) > > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec > index 74818a2e2a19..fbec1cfe4a8e 100644 > --- a/OvmfPkg/OvmfPkg.dec > +++ b/OvmfPkg/OvmfPkg.dec > @@ -35,6 +35,11 @@ [LibraryClasses] > # config space. > PciCapLib|Include/Library/PciCapLib.h > > + ## @libraryclass Layered on top of PciCapLib, allows clients to plug a > + # PciSegmentLib backend into PciCapLib, for config space > + # access. > + PciCapPciSegmentLib|Include/Library/PciCapPciSegmentLib.h > + > ## @libraryclass Access QEMU's firmware configuration interface > # > QemuFwCfgLib|Include/Library/QemuFwCfgLib.h > diff --git a/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf > new file mode 100644 > index 000000000000..e3cf5de49b15 > --- /dev/null > +++ b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.inf > @@ -0,0 +1,35 @@ > +## @file > +# Plug a PciSegmentLib backend into PciCapLib, for config space access. > +# > +# Copyright (C) 2018, Red Hat, Inc. > +# > +# This program and the accompanying materials are licensed and made available > +# under the terms and conditions of the BSD License which accompanies this > +# distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT > +# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +## > + > +[Defines] > + INF_VERSION = 1.27 > + BASE_NAME = BasePciCapPciSegmentLib > + FILE_GUID = ED011855-AA31-43B9-ACC0-BF45A05C5985 > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = PciCapPciSegmentLib > + > +[Sources] > + BasePciCapPciSegmentLib.h > + BasePciCapPciSegmentLib.c > + > +[Packages] > + MdePkg/MdePkg.dec > + OvmfPkg/OvmfPkg.dec > + > +[LibraryClasses] > + BaseLib > + DebugLib > + MemoryAllocationLib > + PciSegmentLib > diff --git a/OvmfPkg/Include/Library/PciCapPciSegmentLib.h b/OvmfPkg/Include/Library/PciCapPciSegmentLib.h > new file mode 100644 > index 000000000000..6b6930288d16 > --- /dev/null > +++ b/OvmfPkg/Include/Library/PciCapPciSegmentLib.h > @@ -0,0 +1,82 @@ > +/** @file > + Library class layered on top of PciCapLib that allows clients to plug a > + PciSegmentLib backend into PciCapLib, for config space access. > + > + Copyright (C) 2018, Red Hat, Inc. > + > + This program and the accompanying materials are licensed and made available > + under the terms and conditions of the BSD License which accompanies this > + distribution. The full text of the license may be found at > + http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT > + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +**/ > + > +#ifndef __PCI_CAP_PCI_SEGMENT_LIB_H__ > +#define __PCI_CAP_PCI_SEGMENT_LIB_H__ > + > +#include > + > + > +/** > + Create a PCI_CAP_DEV object from the PCI Segment:Bus:Device.Function > + quadruplet. The config space accessors are based upon PciSegmentLib. > + > + @param[in] MaxDomain If MaxDomain is PciCapExtended, then > + PciDevice->ReadConfig() and PciDevice->WriteConfig() > + will delegate extended config space accesses too to > + PciSegmentReadBuffer() and PciSegmentWriteBuffer(), > + respectively. Otherwise, PciDevice->ReadConfig() and > + PciDevice->WriteConfig() will reject accesses to > + extended config space with RETURN_UNSUPPORTED, without > + calling PciSegmentReadBuffer() or > + PciSegmentWriteBuffer(). By setting MaxDomain to > + PciCapNormal, the platform can prevent undefined > + PciSegmentLib behavior when the PCI root bridge under > + the PCI device at Segment:Bus:Device.Function doesn't > + support extended config space. > + > + @param[in] Segment 16-bit wide segment number. > + > + @param[in] Bus 8-bit wide bus number. > + > + @param[in] Device 5-bit wide device number. > + > + @param[in] Function 3-bit wide function number. > + > + @param[out] PciDevice The PCI_CAP_DEV object constructed as described above. > + PciDevice can be passed to the PciCapLib APIs. > + > + @retval RETURN_SUCCESS PciDevice has been constructed and output. > + > + @retval RETURN_INVALID_PARAMETER Device or Function does not fit in the > + permitted number of bits. > + > + @retval RETURN_OUT_OF_RESOURCES Memory allocation failed. > +**/ > +RETURN_STATUS > +EFIAPI > +PciCapPciSegmentDeviceInit ( > + IN PCI_CAP_DOMAIN MaxDomain, > + IN UINT16 Segment, > + IN UINT8 Bus, > + IN UINT8 Device, > + IN UINT8 Function, > + OUT PCI_CAP_DEV **PciDevice > + ); > + > + > +/** > + Free the resources used by PciDevice. > + > + @param[in] PciDevice The PCI_CAP_DEV object to free, originally produced by > + PciCapPciSegmentDeviceInit(). > +**/ > +VOID > +EFIAPI > +PciCapPciSegmentDeviceUninit ( > + IN PCI_CAP_DEV *PciDevice > + ); > + > +#endif // __PCI_CAP_PCI_SEGMENT_LIB_H__ > diff --git a/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h > new file mode 100644 > index 000000000000..3ce15fe0fb57 > --- /dev/null > +++ b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.h > @@ -0,0 +1,47 @@ > +/** @file > + Plug a PciSegmentLib backend into PciCapLib, for config space access -- > + internal macro and type definitions. > + > + Copyright (C) 2018, Red Hat, Inc. > + > + This program and the accompanying materials are licensed and made available > + under the terms and conditions of the BSD License which accompanies this > + distribution. The full text of the license may be found at > + http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT > + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +**/ > + > +#ifndef __BASE_PCI_CAP_PCI_SEGMENT_LIB_H__ > +#define __BASE_PCI_CAP_PCI_SEGMENT_LIB_H__ > + > +#include > + > +#include > + > +#define SEGMENT_DEV_SIG SIGNATURE_64 ('P', 'C', 'P', 'S', 'G', 'M', 'N', 'T') > + > +typedef struct { > + // > + // Signature identifying the derived class. > + // > + UINT64 Signature; > + // > + // Members added by the derived class, specific to the use of PciSegmentLib. > + // > + PCI_CAP_DOMAIN MaxDomain; > + UINT16 SegmentNr; > + UINT8 BusNr; > + UINT8 DeviceNr; > + UINT8 FunctionNr; > + // > + // Base class. > + // > + PCI_CAP_DEV BaseDevice; > +} SEGMENT_DEV; > + > +#define SEGMENT_DEV_FROM_PCI_CAP_DEV(PciDevice) \ > + CR (PciDevice, SEGMENT_DEV, BaseDevice, SEGMENT_DEV_SIG) > + > +#endif // __BASE_PCI_CAP_PCI_SEGMENT_LIB_H__ > diff --git a/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c > new file mode 100644 > index 000000000000..57eb6b625b56 > --- /dev/null > +++ b/OvmfPkg/Library/BasePciCapPciSegmentLib/BasePciCapPciSegmentLib.c > @@ -0,0 +1,226 @@ > +/** @file > + Plug a PciSegmentLib backend into PciCapLib, for config space access. > + > + Copyright (C) 2018, Red Hat, Inc. > + > + This program and the accompanying materials are licensed and made available > + under the terms and conditions of the BSD License which accompanies this > + distribution. The full text of the license may be found at > + http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT > + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +**/ > + > +#include > + > +#include > +#include > +#include > + > +#include "BasePciCapPciSegmentLib.h" > + > + > +/** > + Read the config space of a given PCI device (both normal and extended). > + > + SegmentDevReadConfig() performs as few config space accesses as possible > + (without attempting 64-bit wide accesses). > + > + @param[in] PciDevice Implementation-specific unique representation > + of the PCI device in the PCI hierarchy. > + > + @param[in] SourceOffset Source offset in the config space of the PCI > + device to start reading from. > + > + @param[out] DestinationBuffer Buffer to store the read data to. > + > + @param[in] Size The number of bytes to transfer. > + > + @retval RETURN_SUCCESS Size bytes have been transferred from config > + space to DestinationBuffer. > + > + @retval RETURN_UNSUPPORTED Accessing Size bytes from SourceOffset exceeds > + the config space limit of the PCI device. > + Although PCI_CAP_DEV_READ_CONFIG allows reading > + fewer than Size bytes in this case, > + SegmentDevReadConfig() will read none. > +**/ > +STATIC > +RETURN_STATUS > +EFIAPI > +SegmentDevReadConfig ( > + IN PCI_CAP_DEV *PciDevice, > + IN UINT16 SourceOffset, > + OUT VOID *DestinationBuffer, > + IN UINT16 Size > + ) > +{ > + SEGMENT_DEV *SegmentDev; > + UINT16 ConfigSpaceSize; > + UINT64 SourceAddress; > + > + SegmentDev = SEGMENT_DEV_FROM_PCI_CAP_DEV (PciDevice); > + ConfigSpaceSize = (SegmentDev->MaxDomain == PciCapNormal ? > + PCI_MAX_CONFIG_OFFSET : PCI_EXP_MAX_CONFIG_OFFSET); > + // > + // Note that all UINT16 variables below are promoted to INT32, and the > + // addition and the comparison is carried out in INT32. > + // > + if (SourceOffset + Size > ConfigSpaceSize) { > + return RETURN_UNSUPPORTED; > + } > + SourceAddress = PCI_SEGMENT_LIB_ADDRESS (SegmentDev->SegmentNr, > + SegmentDev->BusNr, SegmentDev->DeviceNr, > + SegmentDev->FunctionNr, SourceOffset); > + PciSegmentReadBuffer (SourceAddress, Size, DestinationBuffer); > + return RETURN_SUCCESS; > +} > + > + > +/** > + Write the config space of a given PCI device (both normal and extended). > + > + SegmentDevWriteConfig() performs as few config space accesses as possible > + (without attempting 64-bit wide accesses). > + > + @param[in] PciDevice Implementation-specific unique representation > + of the PCI device in the PCI hierarchy. > + > + @param[in] DestinationOffset Destination offset in the config space of the > + PCI device to start writing at. > + > + @param[in] SourceBuffer Buffer to read the data to be stored from. > + > + @param[in] Size The number of bytes to transfer. > + > + @retval RETURN_SUCCESS Size bytes have been transferred from > + SourceBuffer to config space. > + > + @retval RETURN_UNSUPPORTED Accessing Size bytes at DestinationOffset exceeds > + the config space limit of the PCI device. > + Although PCI_CAP_DEV_WRITE_CONFIG allows writing > + fewer than Size bytes in this case, > + SegmentDevWriteConfig() will write none. > +**/ > +STATIC > +RETURN_STATUS > +EFIAPI > +SegmentDevWriteConfig ( > + IN PCI_CAP_DEV *PciDevice, > + IN UINT16 DestinationOffset, > + IN VOID *SourceBuffer, > + IN UINT16 Size > + ) > +{ > + SEGMENT_DEV *SegmentDev; > + UINT16 ConfigSpaceSize; > + UINT64 DestinationAddress; > + > + SegmentDev = SEGMENT_DEV_FROM_PCI_CAP_DEV (PciDevice); > + ConfigSpaceSize = (SegmentDev->MaxDomain == PciCapNormal ? > + PCI_MAX_CONFIG_OFFSET : PCI_EXP_MAX_CONFIG_OFFSET); > + // > + // Note that all UINT16 variables below are promoted to INT32, and the > + // addition and the comparison is carried out in INT32. > + // > + if (DestinationOffset + Size > ConfigSpaceSize) { > + return RETURN_UNSUPPORTED; > + } > + DestinationAddress = PCI_SEGMENT_LIB_ADDRESS (SegmentDev->SegmentNr, > + SegmentDev->BusNr, SegmentDev->DeviceNr, > + SegmentDev->FunctionNr, DestinationOffset); > + PciSegmentWriteBuffer (DestinationAddress, Size, SourceBuffer); > + return RETURN_SUCCESS; > +} > + > + > +/** > + Create a PCI_CAP_DEV object from the PCI Segment:Bus:Device.Function > + quadruplet. The config space accessors are based upon PciSegmentLib. > + > + @param[in] MaxDomain If MaxDomain is PciCapExtended, then > + PciDevice->ReadConfig() and PciDevice->WriteConfig() > + will delegate extended config space accesses too to > + PciSegmentReadBuffer() and PciSegmentWriteBuffer(), > + respectively. Otherwise, PciDevice->ReadConfig() and > + PciDevice->WriteConfig() will reject accesses to > + extended config space with RETURN_UNSUPPORTED, without > + calling PciSegmentReadBuffer() or > + PciSegmentWriteBuffer(). By setting MaxDomain to > + PciCapNormal, the platform can prevent undefined > + PciSegmentLib behavior when the PCI root bridge under > + the PCI device at Segment:Bus:Device.Function doesn't > + support extended config space. > + > + @param[in] Segment 16-bit wide segment number. > + > + @param[in] Bus 8-bit wide bus number. > + > + @param[in] Device 5-bit wide device number. > + > + @param[in] Function 3-bit wide function number. > + > + @param[out] PciDevice The PCI_CAP_DEV object constructed as described above. > + PciDevice can be passed to the PciCapLib APIs. > + > + @retval RETURN_SUCCESS PciDevice has been constructed and output. > + > + @retval RETURN_INVALID_PARAMETER Device or Function does not fit in the > + permitted number of bits. > + > + @retval RETURN_OUT_OF_RESOURCES Memory allocation failed. > +**/ > +RETURN_STATUS > +EFIAPI > +PciCapPciSegmentDeviceInit ( > + IN PCI_CAP_DOMAIN MaxDomain, > + IN UINT16 Segment, > + IN UINT8 Bus, > + IN UINT8 Device, > + IN UINT8 Function, > + OUT PCI_CAP_DEV **PciDevice > + ) > +{ > + SEGMENT_DEV *SegmentDev; > + > + if (Device > PCI_MAX_DEVICE || Function > PCI_MAX_FUNC) { > + return RETURN_INVALID_PARAMETER; > + } > + > + SegmentDev = AllocatePool (sizeof *SegmentDev); > + if (SegmentDev == NULL) { > + return RETURN_OUT_OF_RESOURCES; > + } > + > + SegmentDev->Signature = SEGMENT_DEV_SIG; > + SegmentDev->MaxDomain = MaxDomain; > + SegmentDev->SegmentNr = Segment; > + SegmentDev->BusNr = Bus; > + SegmentDev->DeviceNr = Device; > + SegmentDev->FunctionNr = Function; > + SegmentDev->BaseDevice.ReadConfig = SegmentDevReadConfig; > + SegmentDev->BaseDevice.WriteConfig = SegmentDevWriteConfig; > + > + *PciDevice = &SegmentDev->BaseDevice; > + return RETURN_SUCCESS; > +} > + > + > +/** > + Free the resources used by PciDevice. > + > + @param[in] PciDevice The PCI_CAP_DEV object to free, originally produced by > + PciCapPciSegmentDeviceInit(). > +**/ > +VOID > +EFIAPI > +PciCapPciSegmentDeviceUninit ( > + IN PCI_CAP_DEV *PciDevice > + ) > +{ > + SEGMENT_DEV *SegmentDev; > + > + SegmentDev = SEGMENT_DEV_FROM_PCI_CAP_DEV (PciDevice); > + FreePool (SegmentDev); > +} > -- > 2.14.1.3.gb7cf6e02401b > >