After all, I've taken a look here as well. I'm not going to point out all the earlier remarks (please do address them here anyway, because they certainly apply), I'll just say what I feel is specific to this patch:
On 08/14/17 13:36, Brijesh Singh wrote: > The VirtioScsiPassThru(), programs the vring descriptor using the host > addresses pointed-by virtio-scsi request, response and memory that is > referenced inside the request and response header. > > The patch uses newly introduced VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer() > function to map system memory to device address and programs the vring > descriptors with device addresses. > > Cc: Ard Biesheuvel <[email protected]> > Cc: Jordan Justen <[email protected]> > Cc: Tom Lendacky <[email protected]> > Cc: Laszlo Ersek <[email protected]> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh <[email protected]> > --- > OvmfPkg/VirtioScsiDxe/VirtioScsi.h | 1 + > OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 148 +++++++++++++++++--- > 2 files changed, 133 insertions(+), 16 deletions(-) > > diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.h > b/OvmfPkg/VirtioScsiDxe/VirtioScsi.h > index 6d00567e8cb8..bb1c5c70ef74 100644 > --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.h > +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.h > @@ -60,6 +60,7 @@ typedef struct { > VRING Ring; // VirtioRingInit 2 > EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru; // VirtioScsiInit 1 > EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode; // VirtioScsiInit 1 > + VOID *RingBufMapping;// VirtioScsiInit 1 > } VSCSI_DEV; > > #define VIRTIO_SCSI_FROM_PASS_THRU(PassThruPointer) \ > diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c > b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c > index a983b3df7b9c..65e9bda0827a 100644 > --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c > +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c > @@ -409,8 +409,13 @@ VirtioScsiPassThru ( > UINT16 TargetValue; > EFI_STATUS Status; > volatile VIRTIO_SCSI_REQ Request; > - volatile VIRTIO_SCSI_RESP Response; > + VIRTIO_SCSI_RESP *Response; (1) Please don't drop the volatile qualification. If you want to turn it into a pointer, that might be OK, but the target of the pointer should remain volatile. > DESC_INDICES Indices; > + VOID *RequestMapping; > + VOID *ResponseMapping; > + VOID *InDataMapping; > + VOID *OutDataMapping; > + EFI_PHYSICAL_ADDRESS DeviceAddress; > > ZeroMem ((VOID*) &Request, sizeof (Request)); > ZeroMem ((VOID*) &Response, sizeof (Response)); (2) Hmm, this ZeroMem() now nulls the pointer. Probably not what you intended. > @@ -418,9 +423,41 @@ VirtioScsiPassThru ( > Dev = VIRTIO_SCSI_FROM_PASS_THRU (This); > CopyMem (&TargetValue, Target, sizeof TargetValue); > > + Response = NULL; > + ResponseMapping = NULL; > + RequestMapping = NULL; > + InDataMapping = NULL; > + OutDataMapping = NULL; > + > + // > + // Response header is bi-direction (we preset with host status and expect > the > + // device to update it). Allocate a response buffer which can be mapped to > + // access equally by both processor and device. > + // Good point! (3) In that case however, please do the same for "HostStatus" in the VirtioBlkDxe driver as well. We also pre-set that value for ourselves. > + Status = Dev->VirtIo->AllocateSharedPages ( > + Dev->VirtIo, > + EFI_SIZE_TO_PAGES (sizeof *Response), > + (VOID *) &Response > + ); (4) In general I abhor this kind of pointer type punning (it is undefined behavior in standard C), but it is so pervasive in edk2, when looking up protocol interfaces, that I guess we can live with it. However, the type that we cast &Response to should be (VOID **), not (VOID *). The compiler didn't complain to you because (VOID *) silently converts to and from other object pointer types -- assuming same const / volatile qualification. The cleanest solution for this would be, IMO: volatile VIRTIO_SCSI_RESP *Response; VOID *ResponseBuffer; Status = Dev->VirtIo->AllocateSharedPages ( Dev->VirtIo, EFI_SIZE_TO_PAGES (sizeof *Response), &ResponseBuffer ); ... Response = ResponseBuffer; ... I think this is all I wanted to say about this patch for now, on top of my earlier comments for virtio-rng and virtio-blk, which also apply here. Thanks, Laszlo > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterCommonBuffer, > + (VOID *) Response, > + sizeof (*Response), > + &DeviceAddress, > + &ResponseMapping > + ); > + if (EFI_ERROR (Status)) { > + goto Free_Response_Buffer; > + } > + > Status = PopulateRequest (Dev, TargetValue, Lun, Packet, &Request); > if (EFI_ERROR (Status)) { > - return Status; > + goto Unmap_Response_Buffer; > } > > VirtioPrepare (&Dev->Ring, &Indices); > @@ -428,7 +465,7 @@ VirtioScsiPassThru ( > // > // preset a host status for ourselves that we do not accept as success > // > - Response.Response = VIRTIO_SCSI_S_FAILURE; > + Response->Response = VIRTIO_SCSI_S_FAILURE; > > // > // ensured by VirtioScsiInit() -- this predicate, in combination with the > @@ -437,23 +474,51 @@ VirtioScsiPassThru ( > ASSERT (Dev->Ring.QueueSize >= 4); > > // > + // Map the scsi-blk request header HostAddress to DeviceAddress > + // > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterRead, > + (VOID *) &Request, > + sizeof Request, > + &DeviceAddress, > + &RequestMapping); > + if (EFI_ERROR (Status)) { > + goto Unmap_Response_Buffer; > + } > + > + // > // enqueue Request > // > - VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request, > + VirtioAppendDesc (&Dev->Ring, (UINTN) DeviceAddress, sizeof Request, > VRING_DESC_F_NEXT, &Indices); > > // > // enqueue "dataout" if any > // > if (Packet->OutTransferLength > 0) { > - VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->OutDataBuffer, > + // > + // Map the buffer address to a device address > + // > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterRead, > + Packet->OutDataBuffer, > + Packet->OutTransferLength, > + &DeviceAddress, > + &OutDataMapping > + ); > + if (EFI_ERROR (Status)) { > + goto Unmap_Request_Buffer; > + } > + VirtioAppendDesc (&Dev->Ring, (UINTN) DeviceAddress, > Packet->OutTransferLength, VRING_DESC_F_NEXT, &Indices); > } > > // > // enqueue Response, to be written by the host > // > - VirtioAppendDesc (&Dev->Ring, (UINTN) &Response, sizeof Response, > + VirtioAppendDesc (&Dev->Ring, (UINTN) Response, sizeof *Response, > VRING_DESC_F_WRITE | (Packet->InTransferLength > 0 ? > VRING_DESC_F_NEXT : 0), > &Indices); > @@ -462,7 +527,22 @@ VirtioScsiPassThru ( > // enqueue "datain" if any, to be written by the host > // > if (Packet->InTransferLength > 0) { > - VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->InDataBuffer, > + // > + // Map the buffer address to a device address > + // > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterWrite, > + Packet->InDataBuffer, > + Packet->InTransferLength, > + &DeviceAddress, > + &InDataMapping > + ); > + if (EFI_ERROR (Status)) { > + goto Unmap_Out_Buffer; > + } > + > + VirtioAppendDesc (&Dev->Ring, (UINTN) DeviceAddress, > Packet->InTransferLength, VRING_DESC_F_WRITE, &Indices); > } > > @@ -480,7 +560,29 @@ VirtioScsiPassThru ( > return EFI_DEVICE_ERROR; > } > > - return ParseResponse (Packet, &Response); > + Status = ParseResponse (Packet, Response); > + > + if (Packet->InTransferLength > 0) { > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, InDataMapping); > + } > + > +Unmap_Out_Buffer: > + if (Packet->OutTransferLength > 0) { > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, OutDataMapping); > + } > + > +Unmap_Request_Buffer: > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping); > +Unmap_Response_Buffer: > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, ResponseMapping); > +Free_Response_Buffer: > + Dev->VirtIo->FreeSharedPages ( > + Dev->VirtIo, > + EFI_SIZE_TO_PAGES (sizeof *Response), > + (VOID *) Response > + ); > + > + return Status; > } > > > @@ -707,7 +809,7 @@ VirtioScsiInit ( > { > UINT8 NextDevStat; > EFI_STATUS Status; > - > + UINT64 RingBaseShift; > UINT64 Features; > UINT16 MaxChannel; // for validation only > UINT32 NumQueues; // for validation only > @@ -838,18 +940,24 @@ VirtioScsiInit ( > goto Failed; > } > > + Status = VirtioRingMap (Dev->VirtIo, &Dev->Ring, &RingBaseShift, > + &Dev->RingBufMapping); > + if (EFI_ERROR (Status)) { > + goto ReleaseQueue; > + } > + > // > // Additional steps for MMIO: align the queue appropriately, and set the > // size. If anything fails from here on, we must release the ring > resources. > // > Status = Dev->VirtIo->SetQueueNum (Dev->VirtIo, QueueSize); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > > Status = Dev->VirtIo->SetQueueAlign (Dev->VirtIo, EFI_PAGE_SIZE); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > > // > @@ -857,7 +965,7 @@ VirtioScsiInit ( > // > Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring, 0); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > > // > @@ -867,7 +975,7 @@ VirtioScsiInit ( > Features &= ~(UINT64)VIRTIO_F_VERSION_1; > Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > } > > @@ -877,11 +985,11 @@ VirtioScsiInit ( > // > Status = VIRTIO_CFG_WRITE (Dev, CdbSize, VIRTIO_SCSI_CDB_SIZE); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > Status = VIRTIO_CFG_WRITE (Dev, SenseSize, VIRTIO_SCSI_SENSE_SIZE); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > > // > @@ -890,7 +998,7 @@ VirtioScsiInit ( > NextDevStat |= VSTAT_DRIVER_OK; > Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > > // > @@ -926,6 +1034,8 @@ VirtioScsiInit ( > > return EFI_SUCCESS; > > +UnmapQueue: > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingBufMapping); > ReleaseQueue: > VirtioRingUninit (Dev->VirtIo, &Dev->Ring); > > @@ -995,6 +1105,12 @@ VirtioScsiExitBoot ( > // > Dev = Context; > Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); > + > + // > + // If Ring buffer mapping exist then unmap it so that hypervisor can > + // not get readable data after device reset. > + // > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingBufMapping); > } > > > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
