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::243; helo=mail-it0-x243.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x243.google.com (mail-it0-x243.google.com [IPv6:2607:f8b0:4001:c0b::243]) (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 947F1207E5412 for ; Thu, 24 May 2018 01:16:23 -0700 (PDT) Received: by mail-it0-x243.google.com with SMTP id 144-v6so1270182iti.5 for ; Thu, 24 May 2018 01:16:23 -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=VRoXdW6EXcDeryiykyLsPQydFMdP9PMWmI0dwuCodnw=; b=jxGksQlxqNnI0qxlFJ3LgBMZ+OU/MdQaNdhA7yaNDaNLXfEOfi/NK2V23H+B+tiwTh ZtLSh+IOvuLCQsgSGCngY8IBk+fppV2OW81ctR9aHe31UMWwIyi8Ooy3vRrrUV5KFrwv Z3wPfSnI1tp4JY8EhwI1kwB2C0PVEbVAVKEQU= 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=VRoXdW6EXcDeryiykyLsPQydFMdP9PMWmI0dwuCodnw=; b=fKT/QoE9cVbfd1hI+XX0lCzhG63zvsDq5XCmyGnL3bTl/2HCfF4nPXUEPtEIQ2t7SF mjEVcXuDbsge6I4101MiBDErZOReCqNnzlCseaAAmZlRQfd5N3xFvQd9dkhGAKMptn9B 5qnE3KOkLqoz1OrFjltYPK0EF7cYoRCVQU7JXh+2aimvdtdM3xDGDOhTdLouLzKk129y 5mgHF2DEnxbJiDPqtioPPYpDkV4dmAnoZe/n2A5tKbwShruFSgvIWMFL5LZXBybRQouq mzhUCJJYjv//XgYgLpd9Uin+KXBkxEC9XKScn1pFgPsx6by3+wIOCZsaQTbwn4i3QsQk M07g== X-Gm-Message-State: ALKqPwchOb9GoQVtq8jZavb3AssKUHJON2gGRMkkUn4dnb/WLsO4Yx0J NvnpsvI5PrJQGFwggkv9BBD3FmnbmgDdtkx3v41PwQ== X-Google-Smtp-Source: AB8JxZoK4620CHHQTAhEb/zjpbO1ZLtqCwSasE+DeZK3F0H6a18EpudLv9TCmQGTE+v2V5t+LooN9L/9g/+Zw9qRuEE= X-Received: by 2002:a24:af45:: with SMTP id l5-v6mr7704010iti.106.1527149782873; Thu, 24 May 2018 01:16:22 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bb86:0:0:0:0:0 with HTTP; Thu, 24 May 2018 01:16:22 -0700 (PDT) In-Reply-To: <20180523202121.8125-8-lersek@redhat.com> References: <20180523202121.8125-1-lersek@redhat.com> <20180523202121.8125-8-lersek@redhat.com> From: Ard Biesheuvel Date: Thu, 24 May 2018 10:16:22 +0200 Message-ID: To: Laszlo Ersek Cc: edk2-devel-01 , Jordan Justen Subject: Re: [PATCH v2 7/7] OvmfPkg/Virtio10Dxe: convert to PciCapLib 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:16:23 -0000 Content-Type: text/plain; charset="UTF-8" On 23 May 2018 at 22:21, Laszlo Ersek wrote: > Replace the manual capability list parsing in OvmfPkg/Virtio10Dxe with > PciCapLib and PciCapPciIoLib API calls. > > The VIRTIO_PCI_CAP_LINK structure type is now superfluous. (Well, it > always has been; we should have used EFI_PCI_CAPABILITY_HDR.) > > Also, EFI_PCI_CAPABILITY_VENDOR_HDR is now included at the front of > VIRTIO_PCI_CAP. No driver other than Virtio10Dxe relies on VIRTIO_PCI_CAP. > > Cc: Ard Biesheuvel > Cc: Jordan Justen > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Laszlo Ersek Reviewed-by: Ard Biesheuvel > --- > > Notes: > v2: > - no changes > > OvmfPkg/Virtio10Dxe/Virtio10.inf | 2 + > OvmfPkg/Include/IndustryStandard/Virtio10.h | 7 +- > OvmfPkg/Virtio10Dxe/Virtio10.c | 135 +++++++------------- > 3 files changed, 52 insertions(+), 92 deletions(-) > > diff --git a/OvmfPkg/Virtio10Dxe/Virtio10.inf b/OvmfPkg/Virtio10Dxe/Virtio10.inf > index c4ef15d94bfc..db0cb1189a29 100644 > --- a/OvmfPkg/Virtio10Dxe/Virtio10.inf > +++ b/OvmfPkg/Virtio10Dxe/Virtio10.inf > @@ -32,6 +32,8 @@ [LibraryClasses] > BaseMemoryLib > DebugLib > MemoryAllocationLib > + PciCapLib > + PciCapPciIoLib > UefiBootServicesTableLib > UefiDriverEntryPoint > UefiLib > diff --git a/OvmfPkg/Include/IndustryStandard/Virtio10.h b/OvmfPkg/Include/IndustryStandard/Virtio10.h > index c5efb5cfcb8a..7d51aa36b326 100644 > --- a/OvmfPkg/Include/IndustryStandard/Virtio10.h > +++ b/OvmfPkg/Include/IndustryStandard/Virtio10.h > @@ -16,6 +16,7 @@ > #ifndef _VIRTIO_1_0_H_ > #define _VIRTIO_1_0_H_ > > +#include > #include > > // > @@ -29,11 +30,7 @@ > // > #pragma pack (1) > typedef struct { > - UINT8 CapId; // Capability identifier (generic) > - UINT8 CapNext; // Link to next capability (generic) > -} VIRTIO_PCI_CAP_LINK; > - > -typedef struct { > + EFI_PCI_CAPABILITY_VENDOR_HDR VendorHdr; > UINT8 ConfigType; // Identifies the specific VirtIo 1.0 config structure > UINT8 Bar; // The BAR that contains the structure > UINT8 Padding[3]; > diff --git a/OvmfPkg/Virtio10Dxe/Virtio10.c b/OvmfPkg/Virtio10Dxe/Virtio10.c > index e9b50b6e437b..9ebb72c76bfd 100644 > --- a/OvmfPkg/Virtio10Dxe/Virtio10.c > +++ b/OvmfPkg/Virtio10Dxe/Virtio10.c > @@ -21,6 +21,8 @@ > #include > #include > #include > +#include > +#include > #include > #include > > @@ -184,48 +186,6 @@ GetBarType ( > } > > > -/** > - Read a slice from PCI config space at the given offset, then advance the > - offset. > - > - @param [in] PciIo The EFI_PCI_IO_PROTOCOL instance that represents the > - device. > - > - @param [in,out] Offset On input, the offset in PCI config space to start > - reading from. On output, the offset of the first byte > - that was not read. On error, Offset is not modified. > - > - @param [in] Size The number of bytes to read. > - > - @param [out] Buffer On output, the bytes read from PCI config space are > - stored in this object. > - > - @retval EFI_SUCCESS Size bytes have been transferred from PCI config space > - (from Offset) to Buffer, and Offset has been incremented > - by Size. > - > - @return Error codes from PciIo->Pci.Read(). > -**/ > -STATIC > -EFI_STATUS > -ReadConfigSpace ( > - IN EFI_PCI_IO_PROTOCOL *PciIo, > - IN OUT UINT32 *Offset, > - IN UINTN Size, > - OUT VOID *Buffer > - ) > -{ > - EFI_STATUS Status; > - > - Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint8, *Offset, Size, Buffer); > - if (EFI_ERROR (Status)) { > - return Status; > - } > - *Offset += (UINT32)Size; > - return EFI_SUCCESS; > -} > - > - > /* > Traverse the PCI capabilities list of a virtio-1.0 device, and capture the > locations of the interesting virtio-1.0 register blocks. > @@ -239,57 +199,51 @@ ReadConfigSpace ( > will have been updated from the PCI > capabilities found. > > - @param[in] CapabilityPtr The offset of the first capability in PCI > - config space, taken from the standard PCI > - device header. > - > @retval EFI_SUCCESS Traversal successful. > > - @return Error codes from the ReadConfigSpace() and GetBarType() > - helper functions. > + @return Error codes from PciCapPciIoLib, PciCapLib, and the > + GetBarType() helper function. > */ > STATIC > EFI_STATUS > ParseCapabilities ( > - IN OUT VIRTIO_1_0_DEV *Device, > - IN UINT8 CapabilityPtr > + IN OUT VIRTIO_1_0_DEV *Device > ) > { > - UINT32 Offset; > - VIRTIO_PCI_CAP_LINK CapLink; > + EFI_STATUS Status; > + PCI_CAP_DEV *PciDevice; > + PCI_CAP_LIST *CapList; > + UINT16 VendorInstance; > + PCI_CAP *VendorCap; > > - for (Offset = CapabilityPtr & 0xFC; > - Offset > 0; > - Offset = CapLink.CapNext & 0xFC > - ) { > - EFI_STATUS Status; > + Status = PciCapPciIoDeviceInit (Device->PciIo, &PciDevice); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + Status = PciCapListInit (PciDevice, &CapList); > + if (EFI_ERROR (Status)) { > + goto UninitPciDevice; > + } > + > + for (VendorInstance = 0; > + !EFI_ERROR (PciCapListFindCap (CapList, PciCapNormal, > + EFI_PCI_CAPABILITY_ID_VENDOR, VendorInstance, > + &VendorCap)); > + VendorInstance++) { > UINT8 CapLen; > VIRTIO_PCI_CAP VirtIoCap; > VIRTIO_1_0_CONFIG *ParsedConfig; > > - // > - // Read capability identifier and link to next capability. > - // > - Status = ReadConfigSpace (Device->PciIo, &Offset, sizeof CapLink, > - &CapLink); > - if (EFI_ERROR (Status)) { > - return Status; > - } > - if (CapLink.CapId != 0x09) { > - // > - // Not a vendor-specific capability, move to the next one. > - // > - continue; > - } > - > // > // Big enough to accommodate a VIRTIO_PCI_CAP structure? > // > - Status = ReadConfigSpace (Device->PciIo, &Offset, sizeof CapLen, &CapLen); > + Status = PciCapRead (PciDevice, VendorCap, > + OFFSET_OF (EFI_PCI_CAPABILITY_VENDOR_HDR, Length), &CapLen, > + sizeof CapLen); > if (EFI_ERROR (Status)) { > - return Status; > + goto UninitCapList; > } > - if (CapLen < sizeof CapLink + sizeof CapLen + sizeof VirtIoCap) { > + if (CapLen < sizeof VirtIoCap) { > // > // Too small, move to next. > // > @@ -299,11 +253,11 @@ ParseCapabilities ( > // > // Read interesting part of capability. > // > - Status = ReadConfigSpace (Device->PciIo, &Offset, sizeof VirtIoCap, > - &VirtIoCap); > + Status = PciCapRead (PciDevice, VendorCap, 0, &VirtIoCap, sizeof VirtIoCap); > if (EFI_ERROR (Status)) { > - return Status; > + goto UninitCapList; > } > + > switch (VirtIoCap.ConfigType) { > case VIRTIO_PCI_CAP_COMMON_CFG: > ParsedConfig = &Device->CommonConfig; > @@ -326,7 +280,7 @@ ParseCapabilities ( > // > Status = GetBarType (Device->PciIo, VirtIoCap.Bar, &ParsedConfig->BarType); > if (EFI_ERROR (Status)) { > - return Status; > + goto UninitCapList; > } > ParsedConfig->Bar = VirtIoCap.Bar; > ParsedConfig->Offset = VirtIoCap.Offset; > @@ -337,19 +291,18 @@ ParseCapabilities ( > // This capability has an additional field called NotifyOffsetMultiplier; > // parse it too. > // > - if (CapLen < sizeof CapLink + sizeof CapLen + sizeof VirtIoCap + > - sizeof Device->NotifyOffsetMultiplier) { > + if (CapLen < sizeof VirtIoCap + sizeof Device->NotifyOffsetMultiplier) { > // > // Too small, move to next. > // > continue; > } > > - Status = ReadConfigSpace (Device->PciIo, &Offset, > - sizeof Device->NotifyOffsetMultiplier, > - &Device->NotifyOffsetMultiplier); > + Status = PciCapRead (PciDevice, VendorCap, sizeof VirtIoCap, > + &Device->NotifyOffsetMultiplier, > + sizeof Device->NotifyOffsetMultiplier); > if (EFI_ERROR (Status)) { > - return Status; > + goto UninitCapList; > } > } > > @@ -359,7 +312,15 @@ ParseCapabilities ( > ParsedConfig->Exists = TRUE; > } > > - return EFI_SUCCESS; > + ASSERT_EFI_ERROR (Status); > + > +UninitCapList: > + PciCapListUninit (CapList); > + > +UninitPciDevice: > + PciCapPciIoDeviceUninit (PciDevice); > + > + return Status; > } > > > @@ -1015,7 +976,7 @@ Virtio10BindingStart ( > > Device->VirtIo.SubSystemDeviceId = Pci.Hdr.DeviceId - 0x1040; > > - Status = ParseCapabilities (Device, Pci.Device.CapabilityPtr); > + Status = ParseCapabilities (Device); > if (EFI_ERROR (Status)) { > goto ClosePciIo; > } > -- > 2.14.1.3.gb7cf6e02401b >