From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [63.128.21.74]) by mx.groups.io with SMTP id smtpd.web10.406.1585255894929245617 for ; Thu, 26 Mar 2020 13:51:35 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=L2n7SAMJ; spf=pass (domain: redhat.com, ip: 63.128.21.74, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585255894; 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=R6oK0kP7gDZwl9939LkiDwxbC6mp9j9/xX/BCdBCFkk=; b=L2n7SAMJIKrFZxVbZojWwAxNK7XCJgSEePjvF2lzyzf7MMqh22/29xxpoYtKD4RAMdPJAU XfXIKqhtzSOOdcVY8WsybVnY+di22SkKTXV63l0c1L0X2ZprkYHHxqqsty0AO1UCzD8rng 43fSOB+b2SzFPx1su5PPpa9d6oHcVYI= 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-104-JxBQJPqoOtSPRu2eVxXPWw-1; Thu, 26 Mar 2020 16:51:26 -0400 X-MC-Unique: JxBQJPqoOtSPRu2eVxXPWw-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 3695B801E53; Thu, 26 Mar 2020 20:51:25 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-128.ams2.redhat.com [10.36.113.128]) by smtp.corp.redhat.com (Postfix) with ESMTP id 607B87E311; Thu, 26 Mar 2020 20:51:23 +0000 (UTC) Subject: Re: [PATCH v2 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings To: Liran Alon , devel@edk2.groups.io Cc: nikita.leshchenko@oracle.com, aaron.young@oracle.com, jordan.l.justen@intel.com, ard.biesheuvel@linaro.org References: <20200325161005.16743-1-liran.alon@oracle.com> <20200325161005.16743-14-liran.alon@oracle.com> From: "Laszlo Ersek" Message-ID: <0739202a-9b8a-759d-5809-2f2df69e9352@redhat.com> Date: Thu, 26 Mar 2020 21:51:22 +0100 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: <20200325161005.16743-14-liran.alon@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 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 On 03/25/20 17:10, Liran Alon wrote: > These rings are shared memory buffers between host and device in which > a cyclic buffer is managed to send request descriptors from host to > device and receive completion descriptors from device to host. > > Note that because device may be constrained by IOMMU or guest may be run > under AMD SEV, we make sure to map these rings to device by using > PciIo->Map(). > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567 > Signed-off-by: Liran Alon > --- > OvmfPkg/PvScsiDxe/PvScsi.c | 269 +++++++++++++++++++++++++++++++++++++ > OvmfPkg/PvScsiDxe/PvScsi.h | 17 +++ > 2 files changed, 286 insertions(+) > > diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c > index 831a78cc18c7..59863f83c60c 100644 > --- a/OvmfPkg/PvScsiDxe/PvScsi.c > +++ b/OvmfPkg/PvScsiDxe/PvScsi.c > @@ -16,6 +16,7 @@ > #include > #include > #include > +#include > #include > > #include "PvScsi.h" > @@ -413,6 +414,264 @@ PvScsiRestorePciAttributes ( > ); > } > > +STATIC > +EFI_STATUS > +PvScsiAllocatePages ( > + IN PVSCSI_DEV *Dev, > + IN UINTN Pages, > + IN OUT VOID **HostAddress > + ) > +{ > + return Dev->PciIo->AllocateBuffer ( > + Dev->PciIo, > + AllocateAnyPages, > + EfiBootServicesData, > + Pages, > + HostAddress, > + EFI_PCI_ATTRIBUTE_MEMORY_CACHED > + ); > +} > + > +STATIC > +VOID > +PvScsiFreePages ( > + IN PVSCSI_DEV *Dev, > + IN UINTN Pages, > + IN VOID *HostAddress > + ) > +{ > + Dev->PciIo->FreeBuffer ( > + Dev->PciIo, > + Pages, > + HostAddress > + ); > +} > + > +STATIC > +EFI_STATUS > +PvScsiMapBuffer ( > + IN PVSCSI_DEV *Dev, > + IN EFI_PCI_IO_PROTOCOL_OPERATION PciIoOperation, > + IN VOID *HostAddress, > + IN UINTN NumberOfBytes, > + OUT PVSCSI_DMA_DESC *DmaDesc > + ) > +{ > + EFI_STATUS Status; > + UINTN BytesMapped; > + > + BytesMapped = NumberOfBytes; > + Status = Dev->PciIo->Map ( > + Dev->PciIo, > + PciIoOperation, > + HostAddress, > + &BytesMapped, > + &DmaDesc->DeviceAddress, > + &DmaDesc->Mapping > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + if (BytesMapped != NumberOfBytes) { > + Status = EFI_OUT_OF_RESOURCES; > + goto Unmap; > + } > + > + return EFI_SUCCESS; > + > +Unmap: > + Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping); > + > + return Status; > +} > + > +STATIC > +VOID > +PvScsiUnmapBuffer ( > + IN PVSCSI_DEV *Dev, > + IN OUT PVSCSI_DMA_DESC *DmaDesc) > +{ > + Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping); > +} > + > +STATIC > +EFI_STATUS > +PvScsiAllocateSharedPages ( > + IN PVSCSI_DEV *Dev, > + IN UINTN Pages, > + IN EFI_PCI_IO_PROTOCOL_OPERATION PciIoOperation, > + OUT VOID **HostAddress, > + OUT PVSCSI_DMA_DESC *DmaDesc > + ) > +{ > + EFI_STATUS Status; > + > + Status = PvScsiAllocatePages (Dev, Pages, HostAddress); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Status = PvScsiMapBuffer ( > + Dev, > + PciIoOperation, > + *HostAddress, > + EFI_PAGES_TO_SIZE (Pages), > + DmaDesc > + ); > + if (EFI_ERROR (Status)) { > + goto FreePages; > + } > + > + return EFI_SUCCESS; > + > +FreePages: > + PvScsiFreePages (Dev, Pages, *HostAddress); > + > + return Status; > +} > + > +STATIC > +VOID > +PvScsiFreeSharedPages ( > + IN PVSCSI_DEV *Dev, > + IN UINTN Pages, > + IN OUT VOID *HostAddress, > + IN OUT PVSCSI_DMA_DESC *DmaDesc > + ) > +{ > + PvScsiUnmapBuffer (Dev, DmaDesc); > + PvScsiFreePages (Dev, Pages, HostAddress); > +} (1) From all of the above functions, please keep only two: PvScsiAllocateSharedPages() and PvScsiFreeSharedPages(). The rest of the helper functions should be flattened into them. This is not a "frivolous" request -- it will make more sense once you read my next request. (2) Please eliminate the "PciIoOperation" parameter from the PvScsiAllocateSharedPages() prototype, and open-code "EfiPciIoOperationBusMasterCommonBuffer" in its place, in the (flattened) implementation of the function. Rationale: the buffer should be allocated separately with the PciIo->AllocateBuffer() API *if and only if* a common buffer operation is being set up. In other cases (one-shot bus master read, one-shot bus master write), *only* the Map() operation should be used, and then Map() and Unmap() will handle the bounce-buffering internally. The functions above are misleading because they suggest they handle all three bus master operations -- but for the one-shot BM read and write operations, the PvScsiAllocateSharedPages() / PvScsiFreeSharedPages() implementations are not correct. Now, given that the call sites only set up common buffer operations, the ultimately exercised logic is OK. And therefore the solution is not to generalize the implementation uselessly (to BM read and BM write operations too), but to tighten the interfaces such that they precisely reflect the actual intended use case. Hence, please open-code the common buffer operation internally to PvScsiAllocateSharedPages(), and also stop exposing the separate allocation / freeing / mapping / unmapping interfaces. The call sites need nothing else than (a) "give me a completely mapped range for common buffer operation" and (b) "release that". Here's the (untested, not even compiled) implementation I propose: > STATIC > EFI_STATUS > PvScsiAllocateSharedPages ( > IN PVSCSI_DEV *Dev, > IN UINTN Pages, > OUT VOID **HostAddress, > OUT PVSCSI_DMA_DESC *DmaDesc > ) > { > EFI_STATUS Status; > UINTN NumberOfBytes; > > Status = Dev->PciIo->AllocateBuffer ( > Dev->PciIo, > AllocateAnyPages, > EfiBootServicesData, > Pages, > HostAddress, > EFI_PCI_ATTRIBUTE_MEMORY_CACHED > ); > if (EFI_ERROR (Status)) { > return Status; > } > > NumberOfBytes = EFI_PAGES_TO_SIZE (Pages); > Status = Dev->PciIo->Map ( > Dev->PciIo, > EfiPciIoOperationBusMasterCommonBuffer, > *HostAddress, > &NumberOfBytes, > &DmaDesc->DeviceAddress, > &DmaDesc->Mapping > ); > if (EFI_ERROR (Status)) { > goto FreeBuffer; > } > > if (NumberOfBytes != EFI_PAGES_TO_SIZE (Pages)) { > Status = EFI_OUT_OF_RESOURCES; > goto Unmap; > } > > return EFI_SUCCESS; > > Unmap: > Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping); > > FreeBuffer: > Dev->PciIo->FreeBuffer (Dev->PciIo, Pages, *HostAddress); > > return Status; > } > > STATIC > VOID > PvScsiFreeSharedPages ( > IN PVSCSI_DEV *Dev, > IN UINTN Pages, > IN VOID *HostAddress, > IN PVSCSI_DMA_DESC *DmaDesc > ) > { > Dev->PciIo->Unmap (Dev->PciIo, DmaDesc->Mapping); > Dev->PciIo->FreeBuffer (Dev->PciIo, Pages, HostAddress); > } Back to your patch: On 03/25/20 17:10, Liran Alon wrote: > + > +STATIC > +EFI_STATUS > +PvScsiInitRings ( > + IN OUT PVSCSI_DEV *Dev > + ) > +{ > + EFI_STATUS Status; > + PVSCSI_CMD_DESC_SETUP_RINGS Cmd; > + > + Status = PvScsiAllocateSharedPages ( > + Dev, > + 1, > + EfiPciIoOperationBusMasterCommonBuffer, > + (VOID **)&Dev->RingDesc.RingState, > + &Dev->RingDesc.RingStateDmaDesc > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + ZeroMem (Dev->RingDesc.RingState, EFI_PAGE_SIZE); > + > + Status = PvScsiAllocateSharedPages ( > + Dev, > + 1, > + EfiPciIoOperationBusMasterCommonBuffer, > + (VOID **)&Dev->RingDesc.RingReqs, > + &Dev->RingDesc.RingReqsDmaDesc > + ); > + if (EFI_ERROR (Status)) { > + goto FreeRingState; > + } > + ZeroMem (Dev->RingDesc.RingReqs, EFI_PAGE_SIZE); > + > + Status = PvScsiAllocateSharedPages ( > + Dev, > + 1, > + EfiPciIoOperationBusMasterCommonBuffer, > + (VOID **)&Dev->RingDesc.RingCmps, > + &Dev->RingDesc.RingCmpsDmaDesc > + ); > + if (EFI_ERROR (Status)) { > + goto FreeRingReqs; > + } > + ZeroMem (Dev->RingDesc.RingCmps, EFI_PAGE_SIZE); > + > + ZeroMem (&Cmd, sizeof (Cmd)); > + Cmd.ReqRingNumPages = 1; > + Cmd.CmpRingNumPages = 1; > + Cmd.RingsStatePPN = RShiftU64 ( > + (UINT64)Dev->RingDesc.RingStateDmaDesc.DeviceAddress, > + EFI_PAGE_SHIFT > + ); > + Cmd.ReqRingPPNs[0] = RShiftU64 ( > + (UINT64)Dev->RingDesc.RingReqsDmaDesc.DeviceAddress, > + EFI_PAGE_SHIFT > + ); > + Cmd.CmpRingPPNs[0] = RShiftU64 ( > + (UINT64)Dev->RingDesc.RingCmpsDmaDesc.DeviceAddress, > + EFI_PAGE_SHIFT > + ); (3) Please drop the explicit UINT64 casts -- for brevity --, EFI_PHYSICAL_ADDRESS is just a different name (a typedef) for UINT64, by spec. (See in the EFI_BOOT_SERVICES.AllocatePages() chapter in the UEFI spec.) (4) Please #include in the C file, and also list BaseLib under [LibraryClasses] in the INF file. RShiftU64() comes from there. > + > + Status = PvScsiWriteCmdDesc ( > + Dev, > + PvScsiCmdSetupRings, > + (UINT32 *)&Cmd, (5) So I guess this is where the "union trick" will come, in v3. > + sizeof (Cmd) / sizeof (UINT32) (6) Can you please try inserting, before the PvScsiWriteCmdDesc() call: STATIC_ASSERT (sizeof Cmd % sizeof (UINT32) == 0, "invalid Cmd size"); > + ); > + if (EFI_ERROR (Status)) { > + goto FreeRingCmps; > + } > + > + return EFI_SUCCESS; > + > +FreeRingCmps: > + PvScsiFreeSharedPages ( > + Dev, > + 1, > + Dev->RingDesc.RingCmps, > + &Dev->RingDesc.RingCmpsDmaDesc > + ); > + > +FreeRingReqs: > + PvScsiFreeSharedPages ( > + Dev, > + 1, > + Dev->RingDesc.RingReqs, > + &Dev->RingDesc.RingReqsDmaDesc > + ); > + > +FreeRingState: > + PvScsiFreeSharedPages ( > + Dev, > + 1, > + Dev->RingDesc.RingState, > + &Dev->RingDesc.RingStateDmaDesc > + ); > + > + return Status; > +} > + > +STATIC > +VOID > +PvScsiFreeRings ( > + IN OUT PVSCSI_DEV *Dev > + ) > +{ > + PvScsiFreeSharedPages ( > + Dev, > + 1, > + Dev->RingDesc.RingCmps, > + &Dev->RingDesc.RingCmpsDmaDesc > + ); > + > + PvScsiFreeSharedPages ( > + Dev, > + 1, > + Dev->RingDesc.RingReqs, > + &Dev->RingDesc.RingReqsDmaDesc > + ); > + > + PvScsiFreeSharedPages ( > + Dev, > + 1, > + Dev->RingDesc.RingState, > + &Dev->RingDesc.RingStateDmaDesc > + ); > +} > + (7) This function is almost correct. Note that your last *such* action in PvScsiInitRings() that impacts any resource life cycle is the *exposure* of the rings to the device model (the hypervisor). Therefore, after PvScsiInitRings() succeeds, then on any subsequently occurring error path, you first need to make the device model forget about the rings, before you release the rings. A hypervisor-side reference to guest memory is just another resource you "allocate", so you need to drop it too, in reverse order of construction. In brief, please add the following to the top of the PvScsiFreeRings() function: PvScsiWriteCmdDesc (Dev, PvScsiCmdAdapterReset, NULL, 0); (Consider, for example, what would happen *otherwise*, if the DmaBuf allocation failed -- in the next patch -- in the PvScsiInit() function. There, you jump to "FreeRings" correctly, and release the rings with PvScsiFreeRings() -- but the hypervisor still remembers them!) > STATIC > EFI_STATUS > PvScsiInit ( > @@ -443,6 +702,14 @@ PvScsiInit ( > goto RestorePciAttributes; > } > > + // > + // Init PVSCSI rings > + // > + Status = PvScsiInitRings (Dev); > + if (EFI_ERROR (Status)) { > + goto RestorePciAttributes; > + } > + > // > // Populate the exported interface's attributes > // > @@ -486,6 +753,8 @@ PvScsiUninit ( > IN OUT PVSCSI_DEV *Dev > ) > { > + PvScsiFreeRings (Dev); > + > PvScsiRestorePciAttributes (Dev); > } > > diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h > index 5f611dbbc98c..6d23b6e1eccf 100644 > --- a/OvmfPkg/PvScsiDxe/PvScsi.h > +++ b/OvmfPkg/PvScsiDxe/PvScsi.h > @@ -15,12 +15,29 @@ > #include > #include > > +typedef struct { > + EFI_PHYSICAL_ADDRESS DeviceAddress; > + VOID *Mapping; > +} PVSCSI_DMA_DESC; > + > +typedef struct { > + PVSCSI_RINGS_STATE *RingState; > + PVSCSI_DMA_DESC RingStateDmaDesc; > + > + PVSCSI_RING_REQ_DESC *RingReqs; > + PVSCSI_DMA_DESC RingReqsDmaDesc; > + > + PVSCSI_RING_CMP_DESC *RingCmps; > + PVSCSI_DMA_DESC RingCmpsDmaDesc; > +} PVSCSI_RING_DESC; > + > #define PVSCSI_SIG SIGNATURE_32 ('P', 'S', 'C', 'S') > > typedef struct { > UINT32 Signature; > EFI_PCI_IO_PROTOCOL *PciIo; > UINT64 OriginalPciAttributes; > + PVSCSI_RING_DESC RingDesc; > UINT8 MaxTarget; > UINT8 MaxLun; > EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru; > These look fine. (8) A general request regarding your git setup: please run the "BaseTools/Scripts/SetupGit.py" script in your edk2 clone. The reason I'm asking for that right now is that the script will point the "diff.orderFile" setting to "BaseTools/Conf/diff.order". And that will make git-format-patch, on your end, place the header file changes ahead of the C file changes, in the generated emails. It helps reviewers greatly if they can first see the declarative changes (such as new types, new structure members, and so on), before they face the code using the new constructs. The script in question configures a bunch of other cool stuff too that boosts reviewer productivity. Thanks, Laszlo