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.10262.1585066303599272752 for ; Tue, 24 Mar 2020 09:11:43 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=VNNfOigf; 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=1585066302; 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=AqxoAsHdLhQ4dbgEuFVffS3MwacPF9VSRE63qpcnBX0=; b=VNNfOigfpar5i5jAyFYhqW9yFuqWEDoa1vQCsg+ur0lSlQ9rt9RQitIgq9gDe5v1rNOiMa 8QaURf5xeoIqglc0ks5qUhbuDraNWjuvWmOlGfn3x9JDU5w4D3SGQtrTZuNh/0PP14I4qA EmH51kfLIP0+PFjifIidTH8gdRdSyYA= 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-98-nPwhlutIMRmuaaVivdi2PQ-1; Tue, 24 Mar 2020 12:11:38 -0400 X-MC-Unique: nPwhlutIMRmuaaVivdi2PQ-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 70743DB64; Tue, 24 Mar 2020 16:11:37 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-139.ams2.redhat.com [10.36.115.139]) by smtp.corp.redhat.com (Postfix) with ESMTP id AD0C91001DD8; Tue, 24 Mar 2020 16:11:10 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings To: devel@edk2.groups.io, liran.alon@oracle.com Cc: nikita.leshchenko@oracle.com, aaron.young@oracle.com, jordan.l.justen@intel.com, ard.biesheuvel@linaro.org References: <20200316150113.104630-1-liran.alon@oracle.com> <20200316150113.104630-14-liran.alon@oracle.com> From: "Laszlo Ersek" Message-ID: <4c6a30a6-0840-4a02-fa6f-70ed1c60f7fc@redhat.com> Date: Tue, 24 Mar 2020 17:11:08 +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: <20200316150113.104630-14-liran.alon@oracle.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit really trivial style comments, for now (I intend to look at this in more detail in v2): On 03/16/20 16:01, 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 > Reviewed-by: Nikita Leshenko > Signed-off-by: Liran Alon > --- > OvmfPkg/PvScsiDxe/PvScsi.c | 235 +++++++++++++++++++++++++++++++++++++ > OvmfPkg/PvScsiDxe/PvScsi.h | 17 +++ > 2 files changed, 252 insertions(+) > > diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c > index fb2407d2adb2..c3f5d38f3d30 100644 > --- a/OvmfPkg/PvScsiDxe/PvScsi.c > +++ b/OvmfPkg/PvScsiDxe/PvScsi.c > @@ -16,6 +16,7 @@ > #include > #include > #include > +#include > > #include "PvScsi.h" > > @@ -396,6 +397,209 @@ PvScsiSetPCIAttributes ( > return EFI_SUCCESS; > } > > +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); > + DmaDesc->Mapping = NULL; > + > + 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; > + > + *HostAddress = NULL; > + DmaDesc->Mapping = NULL; > + > + Status = PvScsiAllocatePages (Dev, Pages, HostAddress); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Status = PvScsiMapBuffer ( > + Dev, > + PciIoOperation, > + *HostAddress, > + Pages * EFI_PAGE_SIZE, (1) Please use EFI_PAGES_TO_SIZE(); it's more idiomatic. (The argument should have type UINTN -- "Pages" is already UINTN, so that's good.) > + DmaDesc > + ); > + if (EFI_ERROR (Status)) { > + goto FreePages; > + } > + > + return EFI_SUCCESS; > + > +FreePages: > + PvScsiFreePages (Dev, Pages, *HostAddress); > + *HostAddress = NULL; > + > + return Status; > +} > + > +STATIC > +VOID > +PvScsiFreeSharedPages ( > + IN PVSCSI_DEV *Dev, > + IN UINTN Pages, > + IN OUT VOID **HostAddress, > + IN OUT PVSCSI_DMA_DESC *DmaDesc > + ) > +{ > + if (*HostAddress) { > + if (DmaDesc->Mapping) { > + PvScsiUnmapBuffer (Dev, DmaDesc); > + DmaDesc->Mapping = NULL; > + } > + > + PvScsiFreePages (Dev, Pages, *HostAddress); > + *HostAddress = NULL; > + } > +} > + > +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)) { > + return Status; > + } > + ZeroMem (Dev->RingDesc.RingReqs, EFI_PAGE_SIZE); > + > + Status = PvScsiAllocateSharedPages ( > + Dev, > + 1, > + EfiPciIoOperationBusMasterCommonBuffer, > + (VOID **)&Dev->RingDesc.RingCmps, > + &Dev->RingDesc.RingCmpsDmaDesc > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + ZeroMem (Dev->RingDesc.RingCmps, EFI_PAGE_SIZE); > + > + ZeroMem (&Cmd, sizeof Cmd); > + Cmd.ReqRingNumPages = 1; > + Cmd.CmpRingNumPages = 1; > + Cmd.RingsStatePPN = > + ((UINT64) Dev->RingDesc.RingStateDmaDesc.DeviceAddress) >> > + EFI_PAGE_SHIFT; > + Cmd.ReqRingPPNs[0] = > + ((UINT64) Dev->RingDesc.RingReqsDmaDesc.DeviceAddress) >> > + EFI_PAGE_SHIFT; > + Cmd.CmpRingPPNs[0] = > + ((UINT64) Dev->RingDesc.RingCmpsDmaDesc.DeviceAddress) >> > + EFI_PAGE_SHIFT; (2) Edk2 allows the << and >> operators when the LHS operand is not wider than UINTN. For shifting UINT64, please call the LShiftU64() and RShiftU64() functions from BaseLib. > + > + return PvScsiWriteCmdDesc(Dev, PVSCSI_CMD_SETUP_RINGS, &Cmd, sizeof Cmd); > +} > + > STATIC > EFI_STATUS > PvScsiInit ( > @@ -425,6 +629,15 @@ PvScsiInit ( > if (EFI_ERROR (Status)) { > return Status; > } > + > + // > + // Init PVSCSI rings > + // > + Status = PvScsiInitRings (Dev); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + (3) Again, we should jump to "RestorePciAttributes" here. (More careful review postponed to v2.) Thanks Laszlo > // > // Populate the exported interface's attributes > // > @@ -463,6 +676,28 @@ PvScsiUninit ( > IN OUT PVSCSI_DEV *Dev > ) > { > + // > + // Free PVSCSI rings > + // > + PvScsiFreeSharedPages ( > + Dev, > + 1, > + (VOID **)&Dev->RingDesc.RingCmps, > + &Dev->RingDesc.RingCmpsDmaDesc > + ); > + PvScsiFreeSharedPages ( > + Dev, > + 1, > + (VOID **)&Dev->RingDesc.RingReqs, > + &Dev->RingDesc.RingReqsDmaDesc > + ); > + PvScsiFreeSharedPages ( > + Dev, > + 1, > + (VOID **)&Dev->RingDesc.RingState, > + &Dev->RingDesc.RingStateDmaDesc > + ); > + > // > // Restore PCI Attributes > // > 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; >