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 <nikita.leshche...@oracle.com> > Signed-off-by: Liran Alon <liran.a...@oracle.com> > --- > 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 <Library/UefiBootServicesTableLib.h> > #include <Library/UefiLib.h> > #include <Protocol/PciIo.h> > +#include <Protocol/PciRootBridgeIo.h> > > #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 <Library/DebugLib.h> > #include <Protocol/ScsiPassThruExt.h> > > +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; > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56163): https://edk2.groups.io/g/devel/message/56163 Mute This Topic: https://groups.io/mt/72001281/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
