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.web11.117.1585261045722918567 for ; Thu, 26 Mar 2020 15:17:25 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=RXvb5kZN; 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=1585261044; 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=CWCLgDc+UxRRvrYx0DFF5tDxKiLHFl1OtMJC0517bqU=; b=RXvb5kZN3xc+fScobqHZBPd6yAHQ4bIOyvTMVW97zB6uv7+5IRJweAphiIQDhgKBwDEE5Z +iPvh3CNN6/JXFhnVRiY+l1PeBvbAfeA+NNxWvDgExhjoltPrvk5oHluddMz9atN5XhH4m +VRLHlvroIottr7PDWjJE2eTwsOOX84= 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-156-lIECFZ6tNJaocJ5UUYLF8g-1; Thu, 26 Mar 2020 18:17:19 -0400 X-MC-Unique: lIECFZ6tNJaocJ5UUYLF8g-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id C1FDE8017CC; Thu, 26 Mar 2020 22:17:17 +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 1DE005C1D6; Thu, 26 Mar 2020 22:17:15 +0000 (UTC) Subject: Re: [PATCH v2 14/17] OvmfPkg/PvScsiDxe: Introduce DMA communication buffer 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-15-liran.alon@oracle.com> From: "Laszlo Ersek" Message-ID: <45e5950c-ec82-c24b-4c38-2912402747b2@redhat.com> Date: Thu, 26 Mar 2020 23:17:15 +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-15-liran.alon@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 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: > 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 (1) s/And/and/ > 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 > Signed-off-by: Liran Alon > --- > OvmfPkg/PvScsiDxe/PvScsi.c | 27 +++++++++++++++++++++++++++ > OvmfPkg/PvScsiDxe/PvScsi.h | 10 ++++++++++ > 2 files changed, 37 insertions(+) > > diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c > index 59863f83c60c..928984099520 100644 > --- a/OvmfPkg/PvScsiDxe/PvScsi.c > +++ b/OvmfPkg/PvScsiDxe/PvScsi.c > @@ -710,6 +710,20 @@ PvScsiInit ( > goto RestorePciAttributes; > } > > + // > + // Allocate DMA communication buffer > + // > + Status = PvScsiAllocateSharedPages ( > + Dev, > + EFI_SIZE_TO_PAGES (sizeof (*Dev->DmaBuf)), > + EfiPciIoOperationBusMasterCommonBuffer, > + (VOID **)&Dev->DmaBuf, > + &Dev->DmaBufDmaDesc > + ); > + if (EFI_ERROR (Status)) { > + goto FreeRings; > + } > + > // > // Populate the exported interface's attributes > // > @@ -741,6 +755,9 @@ PvScsiInit ( > > return EFI_SUCCESS; > > +FreeRings: > + PvScsiFreeRings (Dev); > + > RestorePciAttributes: > PvScsiRestorePciAttributes (Dev); > > @@ -753,6 +770,16 @@ PvScsiUninit ( > IN OUT PVSCSI_DEV *Dev > ) > { > + // > + // Free DMA communication buffer > + // > + PvScsiFreeSharedPages ( (2) From peeking ahead at the next patch, the following seems possible: - we send a data transfer request to the device model, passing some pointers into the DMA communication buffer to the hypervisor - PvScsiWaitForRequestCompletion() fails (for whatever reason), and so we can't be sure whether the device is completely done with the buffer that we exposed to it. Therefore, please *prepend* a Reset operation to this PvScsiFreeSharedPages() call as well, at the top of PvScsiUninit(). (Note: we could be tempted to somehow "centralize" all of these Reset operations into a single spot. Bad idea. We are revoking the device's access rights to different resources, so the revocation operations will show up in different spots. It's a mere circumstance that the revocations all happen to be Reset operations.) I might be paranoid of course -- I just feel that maybe-superfluous reset operations on error paths are much better than silently corrupted guest memory and/or disk contents. > + Dev, > + EFI_SIZE_TO_PAGES (sizeof (*Dev->DmaBuf)), > + (VOID **)&Dev->DmaBuf, (3) Copy-paste typo: you should only pass "Dev->DmaBuf". The compiler doesn't catch this for you because PvScsiFreeSharedPages() takes a "VOID *HostAddress" here, and (VOID **) -- like all other pointer-to-object types -- converts to (VOID *) silently. > + &Dev->DmaBufDmaDesc > + ); > + > PvScsiFreeRings (Dev); > > PvScsiRestorePciAttributes (Dev); > 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]; (4) Is the maximum possible size of the sense data specified somewhere? If so, it would be nice to document it with a comment at least. > + UINT8 Data[0x2000]; (5) Same here. From peeking at the next patch, we seem to be choosing this size arbitrarily. If it works for you in all relevant boot scenarios, I'm OK with it, but we should be clear that this value is arbitrarily chosen. No need for a #define, but a comment would be nice. (6) Should we declare this structure as packed? > +} 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_ > Thanks, Laszlo