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:c0b::244; helo=mail-it0-x244.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x244.google.com (mail-it0-x244.google.com [IPv6:2607:f8b0:4001:c0b::244]) (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 65643207E5412 for ; Thu, 24 May 2018 01:13:33 -0700 (PDT) Received: by mail-it0-x244.google.com with SMTP id 70-v6so1274664ity.2 for ; Thu, 24 May 2018 01:13:33 -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=VnPqHlF1Rh5vGOFFo76HnLiQF1GKhqW4+lYcwS6Qs9Q=; b=MvD7gr0DE2LW2UMHtbMh6QnkAfHlNIT7IvKgfdtSVf8eFYGKzC9yOSPaRnm5B4LVTl oglzaVYaLXFCyyyIkN+csYCfmBgtTF9REYDenPNNhwSu6SMX0vTNlJ2i+s18Z/bVhX3t Jb88p5OrL41ABb1g8JHkQetMGKMmFL8wHvsCQ= 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=VnPqHlF1Rh5vGOFFo76HnLiQF1GKhqW4+lYcwS6Qs9Q=; b=cZwHz9pM5GlVAHzWoiCMXIa2M9KgGkLtqqjjsrwQwky0O35364PT3BPKVVfLw8zwUr t0TqsCE3lZxh5dOwmWqwCbDja8yhavtND7Ia59q/jihqOoswxEEKiRvEf0HdV0tL96lh qufgBLrS72hgnFUFYCM7lXLg1AbMugABaUWvy1Nibz3zVyh7p5QPhvHIFBI11yEeIQQk mVyn3u7olgx6jDYdGCU81bmpDUtwjwJVRRQNcaZA8A0DHVxGZs4xpc1I1bU0jKcuNuE8 TOQkllDXEDl4Nlq10dMlpRkT5MRNCUXUsREoA7giIefM92W+OohJ1a2PahHZ0v956CTA CAww== X-Gm-Message-State: ALKqPwfORfz3F6KI6iZI2Zmu/9iTOKaoHoX4dRsiOlo9UpYcgR0y4EFO R6heYom/44RlHR/sRNUK1brObSPIXfPmzuZzbaKqow== X-Google-Smtp-Source: AB8JxZpoEAPgTHt4G/QS0N+QRpJ3MutNDg/zKc3/dRxLau2H8qRZBnfrZtuTuxxwi6lJ0GySj7zXKzBBxGh0p3N7SsM= X-Received: by 2002:a24:534e:: with SMTP id n75-v6mr8340846itb.138.1527149612403; Thu, 24 May 2018 01:13:32 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bb86:0:0:0:0:0 with HTTP; Thu, 24 May 2018 01:13:31 -0700 (PDT) In-Reply-To: <20180523202121.8125-4-lersek@redhat.com> References: <20180523202121.8125-1-lersek@redhat.com> <20180523202121.8125-4-lersek@redhat.com> From: Ard Biesheuvel Date: Thu, 24 May 2018 10:13:31 +0200 Message-ID: To: Laszlo Ersek Cc: edk2-devel-01 , Jordan Justen Subject: Re: [PATCH v2 3/7] OvmfPkg: introduce PciCapPciIoLib 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:13:33 -0000 Content-Type: text/plain; charset="UTF-8" On 23 May 2018 at 22:21, Laszlo Ersek wrote: > Add a library class, and a UEFI_DRIVER lib instance, that are layered on > top of PciCapLib, and allow clients to plug an EFI_PCI_IO_PROTOCOL backend > into PciCapLib, for config space access. > > (Side note: > Again, please retain the below. > Although the UEFI spec says that EFI_PCI_IO_PROTOCOL_CONFIG() returns > EFI_UNSUPPORTED if "[t]he address range specified by Offset, Width, and > Count is not valid for the PCI configuration header of the PCI > controller", this patch doesn't directly document the EFI_UNSUPPORTED > error code, for ProtoDevTransferConfig() and its callers > ProtoDevReadConfig() and ProtoDevWriteConfig(). Instead, the patch refers > to "unspecified error codes". The reason is that in edk2, the > PciIoConfigRead() and PciIoConfigWrite() functions [1] can also return > EFI_INVALID_PARAMETER for the above situation. > > Namely, PciIoConfigRead() and PciIoConfigWrite() first call > PciIoVerifyConfigAccess(), which indeed produces the standard > EFI_UNSUPPORTED error code, if the device's config space is exceeded. > However, if PciIoVerifyConfigAccess() passes, and we reach > RootBridgeIoPciRead() and RootBridgeIoPciWrite() [2], then > RootBridgeIoCheckParameter() can still fail, e.g. if the root bridge > doesn't support extended config space (see commit 014b472053ae3). > > For all kinds of Limit violations in IO, MMIO, and config space, > RootBridgeIoCheckParameter() returns EFI_INVALID_PARAMETER, not > EFI_UNSUPPORTED. That error code is then propagated up to, and out of, > PciIoConfigRead() and PciIoConfigWrite(). > > [1] MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c > [2] MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c > ) > > 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/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf | 36 +++ > OvmfPkg/Include/Library/PciCapPciIoLib.h | 58 +++++ > OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h | 44 ++++ > OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c | 243 ++++++++++++++++++++ > 5 files changed, 386 insertions(+) > > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec > index fbec1cfe4a8e..dc5597db4136 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 an > + # EFI_PCI_IO_PROTOCOL backend into PciCapLib, for config > + # space access. > + PciCapPciIoLib|Include/Library/PciCapPciIoLib.h > + > ## @libraryclass Layered on top of PciCapLib, allows clients to plug a > # PciSegmentLib backend into PciCapLib, for config space > # access. > diff --git a/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf > new file mode 100644 > index 000000000000..2e14acb0ab75 > --- /dev/null > +++ b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.inf > @@ -0,0 +1,36 @@ > +## @file > +# Plug an EFI_PCI_IO_PROTOCOL 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 = UefiPciCapPciIoLib > + FILE_GUID = 4102F4FE-DA10-4F0F-AC18-4982ED506154 > + MODULE_TYPE = UEFI_DRIVER > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = PciCapPciIoLib > + > +[Sources] > + UefiPciCapPciIoLib.h > + UefiPciCapPciIoLib.c > + > +[Packages] > + MdePkg/MdePkg.dec > + OvmfPkg/OvmfPkg.dec > + > +[LibraryClasses] > + DebugLib > + MemoryAllocationLib > + > +[Protocols] > + gEfiPciIoProtocolGuid ## CONSUMES > diff --git a/OvmfPkg/Include/Library/PciCapPciIoLib.h b/OvmfPkg/Include/Library/PciCapPciIoLib.h > new file mode 100644 > index 000000000000..553715fd5080 > --- /dev/null > +++ b/OvmfPkg/Include/Library/PciCapPciIoLib.h > @@ -0,0 +1,58 @@ > +/** @file > + Library class layered on top of PciCapLib that allows clients to plug an > + EFI_PCI_IO_PROTOCOL 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_IO_LIB_H__ > +#define __PCI_CAP_PCI_IO_LIB_H__ > + > +#include > + > +#include > + > + > +/** > + Create a PCI_CAP_DEV object from an EFI_PCI_IO_PROTOCOL instance. The config > + space accessors are based upon EFI_PCI_IO_PROTOCOL.Pci.Read() and > + EFI_PCI_IO_PROTOCOL.Pci.Write(). > + > + @param[in] PciIo EFI_PCI_IO_PROTOCOL representation of the PCI device. > + > + @param[out] PciDevice The PCI_CAP_DEV object constructed as described above. > + PciDevice can be passed to the PciCapLib APIs. > + > + @retval EFI_SUCCESS PciDevice has been constructed and output. > + > + @retval EFI_OUT_OF_RESOURCES Memory allocation failed. > +**/ > +EFI_STATUS > +EFIAPI > +PciCapPciIoDeviceInit ( > + IN EFI_PCI_IO_PROTOCOL *PciIo, > + OUT PCI_CAP_DEV **PciDevice > + ); > + > + > +/** > + Free the resources used by PciDevice. > + > + @param[in] PciDevice The PCI_CAP_DEV object to free, originally produced by > + PciCapPciIoDeviceInit(). > +**/ > +VOID > +EFIAPI > +PciCapPciIoDeviceUninit ( > + IN PCI_CAP_DEV *PciDevice > + ); > + > +#endif // __PCI_CAP_PCI_IO_LIB_H__ > diff --git a/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h > new file mode 100644 > index 000000000000..a94f65e5a4e3 > --- /dev/null > +++ b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.h > @@ -0,0 +1,44 @@ > +/** @file > + Plug an EFI_PCI_IO_PROTOCOL 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 __UEFI_PCI_CAP_PCI_IO_LIB_H__ > +#define __UEFI_PCI_CAP_PCI_IO_LIB_H__ > + > +#include > + > +#include > + > +#define PROTO_DEV_SIG SIGNATURE_64 ('P', 'C', 'P', 'I', 'O', 'P', 'R', 'T') > + > +typedef struct { > + // > + // Signature identifying the derived class. > + // > + UINT64 Signature; > + // > + // Members added by the derived class, specific to the use of > + // EFI_PCI_IO_PROTOCOL. > + // > + EFI_PCI_IO_PROTOCOL *PciIo; > + // > + // Base class. > + // > + PCI_CAP_DEV BaseDevice; > +} PROTO_DEV; > + > +#define PROTO_DEV_FROM_PCI_CAP_DEV(PciDevice) \ > + CR (PciDevice, PROTO_DEV, BaseDevice, PROTO_DEV_SIG) > + > +#endif // __UEFI_PCI_CAP_PCI_IO_LIB_H__ > diff --git a/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c > new file mode 100644 > index 000000000000..84369e4dc3a8 > --- /dev/null > +++ b/OvmfPkg/Library/UefiPciCapPciIoLib/UefiPciCapPciIoLib.c > @@ -0,0 +1,243 @@ > +/** @file > + Plug an EFI_PCI_IO_PROTOCOL 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 "UefiPciCapPciIoLib.h" > + > + > +/** > + Transfer bytes between the config space of a given PCI device and a memory > + buffer. > + > + ProtoDevTransferConfig() performs as few config space accesses as possible > + (without attempting 64-bit wide accesses). > + > + @param[in] PciIo The EFI_PCI_IO_PROTOCOL representation of the > + PCI device. > + > + @param[in] TransferFunction The EFI_PCI_IO_PROTOCOL_CONFIG function that > + implements the transfer. The direction of the > + transfer is inherent to TransferFunction. > + TransferFunction() is required to return an > + unspecified error if any sub-transfer within > + Size bytes from ConfigOffset exceeds the config > + space limit of the PCI device. > + > + @param[in] ConfigOffset The offset in the config space of the PCI device > + at which the transfer should commence. > + > + @param[in,out] Buffer The memory buffer where the transfer should > + occur. > + > + @param[in] Size The number of bytes to transfer. > + > + @retval EFI_SUCCESS Size bytes have been transferred between config space > + and Buffer. > + > + @return Error codes propagated from TransferFunction(). Fewer > + than Size bytes may have been transferred. > +**/ > +STATIC > +EFI_STATUS > +ProtoDevTransferConfig ( > + IN EFI_PCI_IO_PROTOCOL *PciIo, > + IN EFI_PCI_IO_PROTOCOL_CONFIG TransferFunction, > + IN UINT16 ConfigOffset, > + IN OUT UINT8 *Buffer, > + IN UINT16 Size > + ) > +{ > + while (Size > 0) { > + EFI_PCI_IO_PROTOCOL_WIDTH Width; > + UINT16 Count; > + EFI_STATUS Status; > + UINT16 Progress; > + > + // > + // Pick the largest access size that is allowed by the remaining transfer > + // Size and by the alignment of ConfigOffset. > + // > + // When the largest access size is available, transfer as many bytes as > + // possible in one iteration of the loop. Otherwise, transfer only one > + // unit, to improve the alignment. > + // > + if (Size >= BIT2 && (ConfigOffset & (BIT2 - 1)) == 0) { Ugh. Just use '4' or sizeof(UINT32). > + Width = EfiPciIoWidthUint32; > + Count = Size >> Width; > + } else if (Size >= BIT1 && (ConfigOffset & (BIT1 - 1)) == 0) { > + Width = EfiPciIoWidthUint16; > + Count = 1; > + } else { > + Width = EfiPciIoWidthUint8; > + Count = 1; > + } > + Status = TransferFunction (PciIo, Width, ConfigOffset, Count, Buffer); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + Progress = Count << Width; > + ConfigOffset += Progress; > + Buffer += Progress; > + Size -= Progress; > + } > + return EFI_SUCCESS; > +} > + > + > +/** > + Read the config space of a given PCI device (both normal and extended). > + > + ProtoDevReadConfig() performs as few config space accesses as possible > + (without attempting 64-bit wide accesses). > + > + ProtoDevReadConfig() returns an unspecified error if accessing Size bytes > + from SourceOffset exceeds the config space limit of the PCI device. Fewer > + than Size bytes may have been read in this case. > + > + @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. > + > + @return Error codes propagated from > + EFI_PCI_IO_PROTOCOL.Pci.Read(). Fewer than Size bytes > + may have been read. > +**/ > +STATIC > +RETURN_STATUS > +EFIAPI > +ProtoDevReadConfig ( > + IN PCI_CAP_DEV *PciDevice, > + IN UINT16 SourceOffset, > + OUT VOID *DestinationBuffer, > + IN UINT16 Size > + ) > +{ > + PROTO_DEV *ProtoDev; > + > + ProtoDev = PROTO_DEV_FROM_PCI_CAP_DEV (PciDevice); > + return ProtoDevTransferConfig (ProtoDev->PciIo, ProtoDev->PciIo->Pci.Read, > + SourceOffset, DestinationBuffer, Size); > +} > + > + > +/** > + Write the config space of a given PCI device (both normal and extended). > + > + ProtoDevWriteConfig() performs as few config space accesses as possible > + (without attempting 64-bit wide accesses). > + > + ProtoDevWriteConfig() returns an unspecified error if accessing Size bytes at > + DestinationOffset exceeds the config space limit of the PCI device. Fewer > + than Size bytes may have been written in this case. > + > + @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. > + > + @return Error codes propagated from > + EFI_PCI_IO_PROTOCOL.Pci.Write(). Fewer than Size > + bytes may have been written. > +**/ > +STATIC > +RETURN_STATUS > +EFIAPI > +ProtoDevWriteConfig ( > + IN PCI_CAP_DEV *PciDevice, > + IN UINT16 DestinationOffset, > + IN VOID *SourceBuffer, > + IN UINT16 Size > + ) > +{ > + PROTO_DEV *ProtoDev; > + > + ProtoDev = PROTO_DEV_FROM_PCI_CAP_DEV (PciDevice); > + return ProtoDevTransferConfig (ProtoDev->PciIo, ProtoDev->PciIo->Pci.Write, > + DestinationOffset, SourceBuffer, Size); > +} > + > + > +/** > + Create a PCI_CAP_DEV object from an EFI_PCI_IO_PROTOCOL instance. The config > + space accessors are based upon EFI_PCI_IO_PROTOCOL.Pci.Read() and > + EFI_PCI_IO_PROTOCOL.Pci.Write(). > + > + @param[in] PciIo EFI_PCI_IO_PROTOCOL representation of the PCI device. > + > + @param[out] PciDevice The PCI_CAP_DEV object constructed as described above. > + PciDevice can be passed to the PciCapLib APIs. > + > + @retval EFI_SUCCESS PciDevice has been constructed and output. > + > + @retval EFI_OUT_OF_RESOURCES Memory allocation failed. > +**/ > +EFI_STATUS > +EFIAPI > +PciCapPciIoDeviceInit ( > + IN EFI_PCI_IO_PROTOCOL *PciIo, > + OUT PCI_CAP_DEV **PciDevice > + ) > +{ > + PROTO_DEV *ProtoDev; > + > + ProtoDev = AllocatePool (sizeof *ProtoDev); > + if (ProtoDev == NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + ProtoDev->Signature = PROTO_DEV_SIG; > + ProtoDev->PciIo = PciIo; > + ProtoDev->BaseDevice.ReadConfig = ProtoDevReadConfig; > + ProtoDev->BaseDevice.WriteConfig = ProtoDevWriteConfig; > + > + *PciDevice = &ProtoDev->BaseDevice; > + return EFI_SUCCESS; > +} > + > + > +/** > + Free the resources used by PciDevice. > + > + @param[in] PciDevice The PCI_CAP_DEV object to free, originally produced by > + PciCapPciIoDeviceInit(). > +**/ > +VOID > +EFIAPI > +PciCapPciIoDeviceUninit ( > + IN PCI_CAP_DEV *PciDevice > + ) > +{ > + PROTO_DEV *ProtoDev; > + > + ProtoDev = PROTO_DEV_FROM_PCI_CAP_DEV (PciDevice); > + FreePool (ProtoDev); > +} > -- > 2.14.1.3.gb7cf6e02401b > >