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 <liran.a...@oracle.com> > --- > 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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56425): https://edk2.groups.io/g/devel/message/56425 Mute This Topic: https://groups.io/mt/72544125/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
