From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [216.205.24.74]) by mx.groups.io with SMTP id smtpd.web12.10318.1585066431278472681 for ; Tue, 24 Mar 2020 09:13:51 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=grJ0/2Ne; spf=pass (domain: redhat.com, ip: 216.205.24.74, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585066430; 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=6KXG4itAP7kJ9Rs+9w5UlyglXTfY32tpfY42st8N36M=; b=grJ0/2NeH/uohKykUSTsmhXWmXNOq955Uu81piddytwfzBmsZcHAYaToerYYGCkZEE5gnX y23sB2xXRPmuWDsRhTVnAw03zfzUaExU7dsTnGlQzPN2adfP8PrqFukhnzez+Bc52y0ue5 AUbVp+veB0x5SZE5WomZs4dmUjArNjU= 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-239-iZhIRZniMieZuLXqWpeSVw-1; Tue, 24 Mar 2020 12:13:46 -0400 X-MC-Unique: iZhIRZniMieZuLXqWpeSVw-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 973DB18C35A0; Tue, 24 Mar 2020 16:13:44 +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 6B1976EF86; Tue, 24 Mar 2020 16:13:19 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer 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-15-liran.alon@oracle.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 24 Mar 2020 17:13:13 +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-15-liran.alon@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 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 On 03/16/20 16:01, Liran Alon wrote: > In case device is constrained by IOMMU or guest is running under AMD SEV, > input/output buffers provided to device (DataBuffer and SenseData) needs > to be explicitly mapped to device by PciIo->Map(). > > To avoid the overhead of mapping/unmapping the DataBuffer and SenseData > to the device for every SCSI requst (And to simplify code), introduce a > single DMA communication buffer that will be mapped to device on > initialization. When a SCSI request needs to be sent to device, the > DataBuffer and SenseData will be copied from/to the DMA communication > buffer as required. This will be done by the following commits. > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2567 > Reviewed-by: Nikita Leshenko > Signed-off-by: Liran Alon > --- > OvmfPkg/PvScsiDxe/PvScsi.c | 24 ++++++++++++++++++++++++ > OvmfPkg/PvScsiDxe/PvScsi.h | 10 ++++++++++ > 2 files changed, 34 insertions(+) > > diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c > index c3f5d38f3d30..e48929bf044c 100644 > --- a/OvmfPkg/PvScsiDxe/PvScsi.c > +++ b/OvmfPkg/PvScsiDxe/PvScsi.c > @@ -638,6 +638,20 @@ PvScsiInit ( > return Status; > } > > + // > + // Allocate DMA communication buffer > + // > + Status = PvScsiAllocateSharedPages ( > + Dev, > + EFI_SIZE_TO_PAGES (sizeof (*Dev->DmaBuf)), > + EfiPciIoOperationBusMasterCommonBuffer, > + (VOID **)&Dev->DmaBuf, > + &Dev->DmaBufDmaDesc > + ); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + (1) Superficial review: in my set of "resource management comments", just pointing out that here we both fail to restore the original PCI attributes, and leak the rings. More careful review of this patch slated for v2. Thanks Laszlo > // > // Populate the exported interface's attributes > // > @@ -676,6 +690,16 @@ PvScsiUninit ( > IN OUT PVSCSI_DEV *Dev > ) > { > + // > + // Free DMA communication buffer > + // > + PvScsiFreeSharedPages ( > + Dev, > + EFI_SIZE_TO_PAGES (sizeof (*Dev->DmaBuf)), > + (VOID **)&Dev->DmaBuf, > + &Dev->DmaBufDmaDesc > + ); > + > // > // Free PVSCSI rings > // > diff --git a/OvmfPkg/PvScsiDxe/PvScsi.h b/OvmfPkg/PvScsiDxe/PvScsi.h > index 6d23b6e1eccf..7f91d70fec79 100644 > --- a/OvmfPkg/PvScsiDxe/PvScsi.h > +++ b/OvmfPkg/PvScsiDxe/PvScsi.h > @@ -31,6 +31,11 @@ typedef struct { > PVSCSI_DMA_DESC RingCmpsDmaDesc; > } PVSCSI_RING_DESC; > > +typedef struct { > + UINT8 SenseData[MAX_UINT8]; > + UINT8 Data[0x2000]; > +} PVSCSI_DMA_BUFFER; > + > #define PVSCSI_SIG SIGNATURE_32 ('P', 'S', 'C', 'S') > > typedef struct { > @@ -38,6 +43,8 @@ typedef struct { > EFI_PCI_IO_PROTOCOL *PciIo; > UINT64 OriginalPciAttributes; > PVSCSI_RING_DESC RingDesc; > + PVSCSI_DMA_BUFFER *DmaBuf; > + PVSCSI_DMA_DESC DmaBufDmaDesc; > UINT8 MaxTarget; > UINT8 MaxLun; > EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru; > @@ -47,4 +54,7 @@ typedef struct { > #define PVSCSI_FROM_PASS_THRU(PassThruPointer) \ > CR (PassThruPointer, PVSCSI_DEV, PassThru, PVSCSI_SIG) > > +#define PVSCSI_DMA_BUF_DEV_ADDR(Dev, MemberName) \ > + (Dev->DmaBufDmaDesc.DeviceAddress + OFFSET_OF(PVSCSI_DMA_BUFFER, MemberName)) > + > #endif // __PVSCSI_DXE_H_ >