* [PATCH 0/3] OvmfPkg/VirtioBlkDxe: map host address to device address
@ 2017-08-25 21:43 Brijesh Singh
2017-08-25 21:43 ` [PATCH 1/3] OvmfPkg/VirtioBlkDxe: map VRING using VirtioRingMap() Brijesh Singh
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Brijesh Singh @ 2017-08-25 21:43 UTC (permalink / raw)
To: edk2-devel
Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky,
Laszlo Ersek
The patch updates the VirtioBlkDxe to use IOMMU-like member functions to map
the system physical address to device address for buffers (including vring,
device specific request and response pointed by vring descriptor, and any
furter memory reference by those request and response).
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Repo: https://github.com/codomania/edk2
Branch: virtio-blk-1
Brijesh Singh (3):
OvmfPkg/VirtioBlkDxe: map VRING using VirtioRingMap()
Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response buffers
OvmfPkg/VirtioBlkDxe: negotiate VIRTIO_F_IOMMU_PLATFORM
OvmfPkg/VirtioBlkDxe/VirtioBlk.h | 1 +
OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 207 +++++++++++++++++---
2 files changed, 184 insertions(+), 24 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/3] OvmfPkg/VirtioBlkDxe: map VRING using VirtioRingMap() 2017-08-25 21:43 [PATCH 0/3] OvmfPkg/VirtioBlkDxe: map host address to device address Brijesh Singh @ 2017-08-25 21:43 ` Brijesh Singh 2017-08-27 18:58 ` Laszlo Ersek 2017-08-25 21:43 ` [PATCH 2/3] Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response buffers Brijesh Singh 2017-08-25 21:43 ` [PATCH 3/3] OvmfPkg/VirtioBlkDxe: negotiate VIRTIO_F_IOMMU_PLATFORM Brijesh Singh 2 siblings, 1 reply; 9+ messages in thread From: Brijesh Singh @ 2017-08-25 21:43 UTC (permalink / raw) To: edk2-devel Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky, Laszlo Ersek When device is behind the IOMMU then driver need to pass the device address when programing the bus master. The patch uses VirtioRingMap() to map the VRING system physical address to device address. Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Tom Lendacky <thomas.lendacky@amd.com> Cc: Laszlo Ersek <lersek@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- OvmfPkg/VirtioBlkDxe/VirtioBlk.h | 1 + OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 45 ++++++++++++++++---- 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.h b/OvmfPkg/VirtioBlkDxe/VirtioBlk.h index 6c402ca88ea4..9ec0b956b818 100644 --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.h +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.h @@ -41,6 +41,7 @@ typedef struct { VRING Ring; // VirtioRingInit 2 EFI_BLOCK_IO_PROTOCOL BlockIo; // VirtioBlkInit 1 EFI_BLOCK_IO_MEDIA BlockIoMedia; // VirtioBlkInit 1 + VOID *RingMap; // VirtioRingMap 2 } VBLK_DEV; #define VIRTIO_BLK_FROM_BLOCK_IO(BlockIoPointer) \ diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c index bff15fe3add1..663ba281ab73 100644 --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c @@ -580,7 +580,8 @@ VirtioBlkDriverBindingSupported ( virtio-blk attributes the host provides. @return Error codes from VirtioRingInit() or - VIRTIO_CFG_READ() / VIRTIO_CFG_WRITE(). + VIRTIO_CFG_READ() / VIRTIO_CFG_WRITE or + VirtioRingMap(). **/ @@ -601,6 +602,7 @@ VirtioBlkInit ( UINT8 AlignmentOffset; UINT32 OptIoSize; UINT16 QueueSize; + UINT64 RingBaseShift; PhysicalBlockExp = 0; AlignmentOffset = 0; @@ -729,25 +731,42 @@ VirtioBlkInit ( } // + // If anything fails from here on, we must release the ring resources + // + Status = VirtioRingMap ( + Dev->VirtIo, + &Dev->Ring, + &RingBaseShift, + &Dev->RingMap + ); + 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. + // size. If anything fails from here on, we must unmap 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; } // // step 4c -- Report GPFN (guest-physical frame number) of queue. // - Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring, 0); + Status = Dev->VirtIo->SetQueueAddress ( + Dev->VirtIo, + &Dev->Ring, + RingBaseShift + ); if (EFI_ERROR (Status)) { - goto ReleaseQueue; + goto UnmapQueue; } @@ -758,7 +777,7 @@ VirtioBlkInit ( Features &= ~(UINT64)VIRTIO_F_VERSION_1; Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features); if (EFI_ERROR (Status)) { - goto ReleaseQueue; + goto UnmapQueue; } } @@ -768,7 +787,7 @@ VirtioBlkInit ( NextDevStat |= VSTAT_DRIVER_OK; Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat); if (EFI_ERROR (Status)) { - goto ReleaseQueue; + goto UnmapQueue; } // @@ -811,6 +830,9 @@ VirtioBlkInit ( } return EFI_SUCCESS; +UnmapQueue: + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap); + ReleaseQueue: VirtioRingUninit (Dev->VirtIo, &Dev->Ring); @@ -849,6 +871,7 @@ VirtioBlkUninit ( // Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap); VirtioRingUninit (Dev->VirtIo, &Dev->Ring); SetMem (&Dev->BlockIo, sizeof Dev->BlockIo, 0x00); @@ -885,6 +908,12 @@ VirtioBlkExitBoot ( // Dev = Context; Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); + + // + // Unmap the ring buffer so that hypervisor will not be able to get + // readable data after device is reset. + // + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap); } /** -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] OvmfPkg/VirtioBlkDxe: map VRING using VirtioRingMap() 2017-08-25 21:43 ` [PATCH 1/3] OvmfPkg/VirtioBlkDxe: map VRING using VirtioRingMap() Brijesh Singh @ 2017-08-27 18:58 ` Laszlo Ersek 0 siblings, 0 replies; 9+ messages in thread From: Laszlo Ersek @ 2017-08-27 18:58 UTC (permalink / raw) To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel On 08/25/17 23:43, Brijesh Singh wrote: > When device is behind the IOMMU then driver need to pass the device > address when programing the bus master. The patch uses VirtioRingMap() to > map the VRING system physical address to device address. > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Tom Lendacky <thomas.lendacky@amd.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > OvmfPkg/VirtioBlkDxe/VirtioBlk.h | 1 + > OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 45 ++++++++++++++++---- > 2 files changed, 38 insertions(+), 8 deletions(-) > > diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.h b/OvmfPkg/VirtioBlkDxe/VirtioBlk.h > index 6c402ca88ea4..9ec0b956b818 100644 > --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.h > +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.h > @@ -41,6 +41,7 @@ typedef struct { > VRING Ring; // VirtioRingInit 2 > EFI_BLOCK_IO_PROTOCOL BlockIo; // VirtioBlkInit 1 > EFI_BLOCK_IO_MEDIA BlockIoMedia; // VirtioBlkInit 1 > + VOID *RingMap; // VirtioRingMap 2 > } VBLK_DEV; > > #define VIRTIO_BLK_FROM_BLOCK_IO(BlockIoPointer) \ > diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c > index bff15fe3add1..663ba281ab73 100644 > --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c > +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c > @@ -580,7 +580,8 @@ VirtioBlkDriverBindingSupported ( > virtio-blk attributes the host provides. > > @return Error codes from VirtioRingInit() or > - VIRTIO_CFG_READ() / VIRTIO_CFG_WRITE(). > + VIRTIO_CFG_READ() / VIRTIO_CFG_WRITE or > + VirtioRingMap(). > > **/ > > @@ -601,6 +602,7 @@ VirtioBlkInit ( > UINT8 AlignmentOffset; > UINT32 OptIoSize; > UINT16 QueueSize; > + UINT64 RingBaseShift; > > PhysicalBlockExp = 0; > AlignmentOffset = 0; > @@ -729,25 +731,42 @@ VirtioBlkInit ( > } > > // > + // If anything fails from here on, we must release the ring resources > + // > + Status = VirtioRingMap ( > + Dev->VirtIo, > + &Dev->Ring, > + &RingBaseShift, > + &Dev->RingMap > + ); > + 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. > + // size. If anything fails from here on, we must unmap 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; > } > > // > // step 4c -- Report GPFN (guest-physical frame number) of queue. > // > - Status = Dev->VirtIo->SetQueueAddress (Dev->VirtIo, &Dev->Ring, 0); > + Status = Dev->VirtIo->SetQueueAddress ( > + Dev->VirtIo, > + &Dev->Ring, > + RingBaseShift > + ); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > > > @@ -758,7 +777,7 @@ VirtioBlkInit ( > Features &= ~(UINT64)VIRTIO_F_VERSION_1; > Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > } > > @@ -768,7 +787,7 @@ VirtioBlkInit ( > NextDevStat |= VSTAT_DRIVER_OK; > Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > > // > @@ -811,6 +830,9 @@ VirtioBlkInit ( > } > return EFI_SUCCESS; > > +UnmapQueue: > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap); > + > ReleaseQueue: > VirtioRingUninit (Dev->VirtIo, &Dev->Ring); > > @@ -849,6 +871,7 @@ VirtioBlkUninit ( > // > Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); > > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap); > VirtioRingUninit (Dev->VirtIo, &Dev->Ring); > > SetMem (&Dev->BlockIo, sizeof Dev->BlockIo, 0x00); > @@ -885,6 +908,12 @@ VirtioBlkExitBoot ( > // > Dev = Context; > Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); > + > + // > + // Unmap the ring buffer so that hypervisor will not be able to get > + // readable data after device is reset. > + // > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap); > } > > /** > Reviewed-by: Laszlo Ersek <lersek@redhat.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response buffers 2017-08-25 21:43 [PATCH 0/3] OvmfPkg/VirtioBlkDxe: map host address to device address Brijesh Singh 2017-08-25 21:43 ` [PATCH 1/3] OvmfPkg/VirtioBlkDxe: map VRING using VirtioRingMap() Brijesh Singh @ 2017-08-25 21:43 ` Brijesh Singh 2017-08-27 20:40 ` Laszlo Ersek 2017-08-25 21:43 ` [PATCH 3/3] OvmfPkg/VirtioBlkDxe: negotiate VIRTIO_F_IOMMU_PLATFORM Brijesh Singh 2 siblings, 1 reply; 9+ messages in thread From: Brijesh Singh @ 2017-08-25 21:43 UTC (permalink / raw) To: edk2-devel Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky, Laszlo Ersek When device is behind the IOMMU, driver is require to pass the device address of virtio request, response and any memory referenced by those request/response to the bus master. The patch uses IOMMU-like member functions from VIRTIO_DEVICE_PROTOCOL to map request and response buffers system physical address to the device address. - If the buffer need to be accessed by both the processor and a bus master then map with BusMasterCommonBuffer. - If the buffer need to be accessed for a write operation by a bus master then map with BusMasterWrite. - If the buffer need to be accessed for a read operation by a bus master then map with BusMasterRead. Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Tom Lendacky <thomas.lendacky@amd.com> Cc: Laszlo Ersek <lersek@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 157 ++++++++++++++++++-- 1 file changed, 143 insertions(+), 14 deletions(-) diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c index 663ba281ab73..ab69cb08a625 100644 --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c @@ -232,7 +232,8 @@ VerifyReadWriteRequest ( @retval EFI_DEVICE_ERROR Failed to notify host side via VirtIo write, or unable to parse host response, or host response - is not VIRTIO_BLK_S_OK. + is not VIRTIO_BLK_S_OK or failed to map Buffer + for a bus master operation. **/ @@ -249,8 +250,16 @@ SynchronousRequest ( { UINT32 BlockSize; volatile VIRTIO_BLK_REQ Request; - volatile UINT8 HostStatus; + volatile UINT8 *HostStatus; + VOID *HostStatusBuffer; DESC_INDICES Indices; + VOID *RequestMapping; + VOID *StatusMapping; + VOID *BufferMapping; + EFI_PHYSICAL_ADDRESS BufferDeviceAddress; + EFI_PHYSICAL_ADDRESS HostStatusDeviceAddress; + EFI_PHYSICAL_ADDRESS RequestDeviceAddress; + EFI_STATUS Status, Ret; BlockSize = Dev->BlockIoMedia.BlockSize; @@ -278,9 +287,89 @@ SynchronousRequest ( VirtioPrepare (&Dev->Ring, &Indices); // + // Host status is bi-directional (we preset with a value and expect the + // device to update it). Allocate a host status buffer which can be mapped + // to access equally by both processor and the device. + // + Status = Dev->VirtIo->AllocateSharedPages ( + Dev->VirtIo, + EFI_SIZE_TO_PAGES (sizeof *HostStatus), + &HostStatusBuffer + ); + if (EFI_ERROR (Status)) { + return EFI_DEVICE_ERROR; + } + + // + // Map virtio-blk request header (must be done after request header is + // populated) + // + Status = VirtioMapAllBytesInSharedBuffer ( + Dev->VirtIo, + VirtioOperationBusMasterRead, + (VOID *) &Request, + sizeof Request, + &RequestDeviceAddress, + &RequestMapping + ); + if (EFI_ERROR (Status)) { + Status = EFI_DEVICE_ERROR; + goto FreeHostStatusBuffer; + } + + // + // Map data buffer + // + if (BufferSize > 0) { + if (RequestIsWrite) { + Status = VirtioMapAllBytesInSharedBuffer ( + Dev->VirtIo, + VirtioOperationBusMasterRead, + (VOID *) Buffer, + BufferSize, + &BufferDeviceAddress, + &BufferMapping + ); + } else { + Status = VirtioMapAllBytesInSharedBuffer ( + Dev->VirtIo, + VirtioOperationBusMasterWrite, + (VOID *) Buffer, + BufferSize, + &BufferDeviceAddress, + &BufferMapping + ); + } + + if (EFI_ERROR (Status)) { + Status = EFI_DEVICE_ERROR; + goto UnmapRequestBuffer; + } + } + + // + // Map the Status Buffer with VirtioOperationBusMasterCommonBuffer so that + // both processor and device can access it. + // + Status = VirtioMapAllBytesInSharedBuffer ( + Dev->VirtIo, + VirtioOperationBusMasterCommonBuffer, + HostStatusBuffer, + sizeof *HostStatus, + &HostStatusDeviceAddress, + &StatusMapping + ); + if (EFI_ERROR (Status)) { + Status = EFI_DEVICE_ERROR; + goto UnmapDataBuffer; + } + + HostStatus = HostStatusBuffer; + + // // preset a host status for ourselves that we do not accept as success // - HostStatus = VIRTIO_BLK_S_IOERR; + *HostStatus = VIRTIO_BLK_S_IOERR; // // ensured by VirtioBlkInit() -- this predicate, in combination with the @@ -291,8 +380,13 @@ SynchronousRequest ( // // virtio-blk header in first desc // - VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request, - VRING_DESC_F_NEXT, &Indices); + VirtioAppendDesc ( + &Dev->Ring, + RequestDeviceAddress, + sizeof Request, + VRING_DESC_F_NEXT, + &Indices + ); // // data buffer for read/write in second desc @@ -311,27 +405,62 @@ SynchronousRequest ( // // VRING_DESC_F_WRITE is interpreted from the host's point of view. // - VirtioAppendDesc (&Dev->Ring, (UINTN) Buffer, (UINT32) BufferSize, + VirtioAppendDesc ( + &Dev->Ring, + BufferDeviceAddress, + (UINT32) BufferSize, VRING_DESC_F_NEXT | (RequestIsWrite ? 0 : VRING_DESC_F_WRITE), - &Indices); + &Indices + ); } // // host status in last (second or third) desc // - VirtioAppendDesc (&Dev->Ring, (UINTN) &HostStatus, sizeof HostStatus, - VRING_DESC_F_WRITE, &Indices); + VirtioAppendDesc ( + &Dev->Ring, + HostStatusDeviceAddress, + sizeof *HostStatus, + VRING_DESC_F_WRITE, + &Indices + ); // // virtio-blk's only virtqueue is #0, called "requestq" (see Appendix D). // - if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, - NULL) == EFI_SUCCESS && - HostStatus == VIRTIO_BLK_S_OK) { - return EFI_SUCCESS; + Status = VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, NULL); + + // + // Unmap the HostStatus buffer before accessing it + // + Ret = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, StatusMapping); + if (EFI_ERROR (Ret)) { + Status = EFI_DEVICE_ERROR; + } + + if (!EFI_ERROR (Status) && + *HostStatus == VIRTIO_BLK_S_OK) { + Status = EFI_SUCCESS; + } else { + Status = EFI_DEVICE_ERROR; } - return EFI_DEVICE_ERROR; +UnmapDataBuffer: + if (BufferSize > 0) { + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping); + } + +UnmapRequestBuffer: + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping); + +FreeHostStatusBuffer: + Dev->VirtIo->FreeSharedPages ( + Dev->VirtIo, + EFI_SIZE_TO_PAGES (sizeof *HostStatus), + HostStatusBuffer + ); + + return Status; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response buffers 2017-08-25 21:43 ` [PATCH 2/3] Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response buffers Brijesh Singh @ 2017-08-27 20:40 ` Laszlo Ersek 2017-08-27 22:05 ` Laszlo Ersek 0 siblings, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2017-08-27 20:40 UTC (permalink / raw) To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel This patch is functionally correct. I'd like to request three stylistic improvements: On 08/25/17 23:43, Brijesh Singh wrote: > When device is behind the IOMMU, driver is require to pass the device > address of virtio request, response and any memory referenced by those > request/response to the bus master. > > The patch uses IOMMU-like member functions from VIRTIO_DEVICE_PROTOCOL to > map request and response buffers system physical address to the device > address. > > - If the buffer need to be accessed by both the processor and a bus > master then map with BusMasterCommonBuffer. > > - If the buffer need to be accessed for a write operation by a bus master > then map with BusMasterWrite. > > - If the buffer need to be accessed for a read operation by a bus master > then map with BusMasterRead. > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Tom Lendacky <thomas.lendacky@amd.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 157 ++++++++++++++++++-- > 1 file changed, 143 insertions(+), 14 deletions(-) > > diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c > index 663ba281ab73..ab69cb08a625 100644 > --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c > +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c > @@ -232,7 +232,8 @@ VerifyReadWriteRequest ( > > @retval EFI_DEVICE_ERROR Failed to notify host side via VirtIo write, or > unable to parse host response, or host response > - is not VIRTIO_BLK_S_OK. > + is not VIRTIO_BLK_S_OK or failed to map Buffer > + for a bus master operation. > > **/ > > @@ -249,8 +250,16 @@ SynchronousRequest ( > { > UINT32 BlockSize; > volatile VIRTIO_BLK_REQ Request; > - volatile UINT8 HostStatus; > + volatile UINT8 *HostStatus; > + VOID *HostStatusBuffer; > DESC_INDICES Indices; > + VOID *RequestMapping; > + VOID *StatusMapping; > + VOID *BufferMapping; > + EFI_PHYSICAL_ADDRESS BufferDeviceAddress; > + EFI_PHYSICAL_ADDRESS HostStatusDeviceAddress; > + EFI_PHYSICAL_ADDRESS RequestDeviceAddress; > + EFI_STATUS Status, Ret; (1) Please rename the "Ret" variable to "UnmapStatus", and define it separately. > > BlockSize = Dev->BlockIoMedia.BlockSize; > > @@ -278,9 +287,89 @@ SynchronousRequest ( > VirtioPrepare (&Dev->Ring, &Indices); > > // > + // Host status is bi-directional (we preset with a value and expect the > + // device to update it). Allocate a host status buffer which can be mapped > + // to access equally by both processor and the device. > + // > + Status = Dev->VirtIo->AllocateSharedPages ( > + Dev->VirtIo, > + EFI_SIZE_TO_PAGES (sizeof *HostStatus), > + &HostStatusBuffer > + ); > + if (EFI_ERROR (Status)) { > + return EFI_DEVICE_ERROR; > + } > + > + // > + // Map virtio-blk request header (must be done after request header is > + // populated) > + // > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterRead, > + (VOID *) &Request, > + sizeof Request, > + &RequestDeviceAddress, > + &RequestMapping > + ); > + if (EFI_ERROR (Status)) { > + Status = EFI_DEVICE_ERROR; > + goto FreeHostStatusBuffer; > + } > + > + // > + // Map data buffer > + // > + if (BufferSize > 0) { > + if (RequestIsWrite) { > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterRead, > + (VOID *) Buffer, > + BufferSize, > + &BufferDeviceAddress, > + &BufferMapping > + ); > + } else { > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterWrite, > + (VOID *) Buffer, > + BufferSize, > + &BufferDeviceAddress, > + &BufferMapping > + ); > + } (2) Please squash these two branches into one, and determine the Operation argument like this: (RequestIsWrite ? VirtioOperationBusMasterRead : VirtioOperationBusMasterWrite), (The conditional operator (? :) should be used sparingly, but when it improves readability, it should be used.) > + > + if (EFI_ERROR (Status)) { > + Status = EFI_DEVICE_ERROR; > + goto UnmapRequestBuffer; > + } > + } > + > + // > + // Map the Status Buffer with VirtioOperationBusMasterCommonBuffer so that > + // both processor and device can access it. > + // > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterCommonBuffer, > + HostStatusBuffer, > + sizeof *HostStatus, > + &HostStatusDeviceAddress, > + &StatusMapping > + ); > + if (EFI_ERROR (Status)) { > + Status = EFI_DEVICE_ERROR; > + goto UnmapDataBuffer; > + } > + > + HostStatus = HostStatusBuffer; > + > + // > // preset a host status for ourselves that we do not accept as success > // > - HostStatus = VIRTIO_BLK_S_IOERR; > + *HostStatus = VIRTIO_BLK_S_IOERR; > > // > // ensured by VirtioBlkInit() -- this predicate, in combination with the > @@ -291,8 +380,13 @@ SynchronousRequest ( > // > // virtio-blk header in first desc > // > - VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request, > - VRING_DESC_F_NEXT, &Indices); > + VirtioAppendDesc ( > + &Dev->Ring, > + RequestDeviceAddress, > + sizeof Request, > + VRING_DESC_F_NEXT, > + &Indices > + ); > > // > // data buffer for read/write in second desc > @@ -311,27 +405,62 @@ SynchronousRequest ( > // > // VRING_DESC_F_WRITE is interpreted from the host's point of view. > // > - VirtioAppendDesc (&Dev->Ring, (UINTN) Buffer, (UINT32) BufferSize, > + VirtioAppendDesc ( > + &Dev->Ring, > + BufferDeviceAddress, > + (UINT32) BufferSize, > VRING_DESC_F_NEXT | (RequestIsWrite ? 0 : VRING_DESC_F_WRITE), > - &Indices); > + &Indices > + ); > } > > // > // host status in last (second or third) desc > // > - VirtioAppendDesc (&Dev->Ring, (UINTN) &HostStatus, sizeof HostStatus, > - VRING_DESC_F_WRITE, &Indices); > + VirtioAppendDesc ( > + &Dev->Ring, > + HostStatusDeviceAddress, > + sizeof *HostStatus, > + VRING_DESC_F_WRITE, > + &Indices > + ); > > // > // virtio-blk's only virtqueue is #0, called "requestq" (see Appendix D). > // > - if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, > - NULL) == EFI_SUCCESS && > - HostStatus == VIRTIO_BLK_S_OK) { > - return EFI_SUCCESS; > + Status = VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, NULL); > + > + // > + // Unmap the HostStatus buffer before accessing it > + // > + Ret = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, StatusMapping); (3) Okay, so here you will assign UnmapStatus. And then, the rest: > + if (EFI_ERROR (Ret)) { > + Status = EFI_DEVICE_ERROR; > + } > + > + if (!EFI_ERROR (Status) && > + *HostStatus == VIRTIO_BLK_S_OK) { > + Status = EFI_SUCCESS; > + } else { > + Status = EFI_DEVICE_ERROR; > } should be written like this: if (EFI_ERROR (Status) || EFI_ERROR (UnmapStatus) || *HostStatus != VIRTIO_BLK_S_OK) { Status = EFI_DEVICE_ERROR; } Namely, - this block will ensure that we only look at *HostStatus when we're allowed to (i.e., after both VirtioFlush() and Unmap succeeded), - If VirtioFlush() fails and sets Status to some error code, then we coerce Status to EFI_DEVICE_ERROR, - If the entire condition evaluates to FALSE, then Status is already EFI_SUCCESS, so no need to touch it. Regarding the VirtioScsiDxe driver... in this patch, we get away with the above shortcut (without making a mess of the code), but in the VirtioScsiDxe driver, we won't. In that driver, the bus master device can produce *two* output buffers, so you will have to unmap at least one of those under an error-handling label -- when you mapped the first successfully, and failed to map the second. (In this patch you managed to unmap StatusMapping in shared code, but that only worked because you had only one output buffer, and you could put its mapping last.) And once you unmap an output buffer on both the success path and the failure path, things get more complex. I think that's OK, we should just be consistent about it. So, for VirtioScsiDxe, I suggest the pattern I laid out in <http://mid.mail-archive.com/f7736294-0368-cddc-3fcd-de76cae5650e@redhat.com>. Thank you, Laszlo > > - return EFI_DEVICE_ERROR; > +UnmapDataBuffer: > + if (BufferSize > 0) { > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping); > + } > + > +UnmapRequestBuffer: > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping); > + > +FreeHostStatusBuffer: > + Dev->VirtIo->FreeSharedPages ( > + Dev->VirtIo, > + EFI_SIZE_TO_PAGES (sizeof *HostStatus), > + HostStatusBuffer > + ); > + > + return Status; > } > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response buffers 2017-08-27 20:40 ` Laszlo Ersek @ 2017-08-27 22:05 ` Laszlo Ersek 2017-08-27 23:18 ` Brijesh Singh 0 siblings, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2017-08-27 22:05 UTC (permalink / raw) To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Ard Biesheuvel, Tom Lendacky On 08/27/17 22:40, Laszlo Ersek wrote: > This patch is functionally correct. I'd like to request three stylistic > improvements: > > On 08/25/17 23:43, Brijesh Singh wrote: >> When device is behind the IOMMU, driver is require to pass the device >> address of virtio request, response and any memory referenced by those >> request/response to the bus master. >> >> The patch uses IOMMU-like member functions from VIRTIO_DEVICE_PROTOCOL to >> map request and response buffers system physical address to the device >> address. >> >> - If the buffer need to be accessed by both the processor and a bus >> master then map with BusMasterCommonBuffer. >> >> - If the buffer need to be accessed for a write operation by a bus master >> then map with BusMasterWrite. >> >> - If the buffer need to be accessed for a read operation by a bus master >> then map with BusMasterRead. >> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Cc: Jordan Justen <jordan.l.justen@intel.com> >> Cc: Tom Lendacky <thomas.lendacky@amd.com> >> Cc: Laszlo Ersek <lersek@redhat.com> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 157 ++++++++++++++++++-- >> 1 file changed, 143 insertions(+), 14 deletions(-) >> >> diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c >> index 663ba281ab73..ab69cb08a625 100644 >> --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c >> +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c >> @@ -232,7 +232,8 @@ VerifyReadWriteRequest ( >> >> @retval EFI_DEVICE_ERROR Failed to notify host side via VirtIo write, or >> unable to parse host response, or host response >> - is not VIRTIO_BLK_S_OK. >> + is not VIRTIO_BLK_S_OK or failed to map Buffer >> + for a bus master operation. >> >> **/ >> >> @@ -249,8 +250,16 @@ SynchronousRequest ( >> { >> UINT32 BlockSize; >> volatile VIRTIO_BLK_REQ Request; >> - volatile UINT8 HostStatus; >> + volatile UINT8 *HostStatus; >> + VOID *HostStatusBuffer; >> DESC_INDICES Indices; >> + VOID *RequestMapping; >> + VOID *StatusMapping; >> + VOID *BufferMapping; >> + EFI_PHYSICAL_ADDRESS BufferDeviceAddress; >> + EFI_PHYSICAL_ADDRESS HostStatusDeviceAddress; >> + EFI_PHYSICAL_ADDRESS RequestDeviceAddress; >> + EFI_STATUS Status, Ret; > > (1) Please rename the "Ret" variable to "UnmapStatus", and define it > separately. > >> >> BlockSize = Dev->BlockIoMedia.BlockSize; >> >> @@ -278,9 +287,89 @@ SynchronousRequest ( >> VirtioPrepare (&Dev->Ring, &Indices); >> >> // >> + // Host status is bi-directional (we preset with a value and expect the >> + // device to update it). Allocate a host status buffer which can be mapped >> + // to access equally by both processor and the device. >> + // >> + Status = Dev->VirtIo->AllocateSharedPages ( >> + Dev->VirtIo, >> + EFI_SIZE_TO_PAGES (sizeof *HostStatus), >> + &HostStatusBuffer >> + ); >> + if (EFI_ERROR (Status)) { >> + return EFI_DEVICE_ERROR; >> + } >> + >> + // >> + // Map virtio-blk request header (must be done after request header is >> + // populated) >> + // >> + Status = VirtioMapAllBytesInSharedBuffer ( >> + Dev->VirtIo, >> + VirtioOperationBusMasterRead, >> + (VOID *) &Request, >> + sizeof Request, >> + &RequestDeviceAddress, >> + &RequestMapping >> + ); >> + if (EFI_ERROR (Status)) { >> + Status = EFI_DEVICE_ERROR; >> + goto FreeHostStatusBuffer; >> + } >> + >> + // >> + // Map data buffer >> + // >> + if (BufferSize > 0) { >> + if (RequestIsWrite) { >> + Status = VirtioMapAllBytesInSharedBuffer ( >> + Dev->VirtIo, >> + VirtioOperationBusMasterRead, >> + (VOID *) Buffer, >> + BufferSize, >> + &BufferDeviceAddress, >> + &BufferMapping >> + ); >> + } else { >> + Status = VirtioMapAllBytesInSharedBuffer ( >> + Dev->VirtIo, >> + VirtioOperationBusMasterWrite, >> + (VOID *) Buffer, >> + BufferSize, >> + &BufferDeviceAddress, >> + &BufferMapping >> + ); >> + } > > (2) Please squash these two branches into one, and determine the > Operation argument like this: > > (RequestIsWrite ? > VirtioOperationBusMasterRead : > VirtioOperationBusMasterWrite), > > (The conditional operator (? :) should be used sparingly, but when it > improves readability, it should be used.) > >> + >> + if (EFI_ERROR (Status)) { >> + Status = EFI_DEVICE_ERROR; >> + goto UnmapRequestBuffer; >> + } >> + } >> + >> + // >> + // Map the Status Buffer with VirtioOperationBusMasterCommonBuffer so that >> + // both processor and device can access it. >> + // >> + Status = VirtioMapAllBytesInSharedBuffer ( >> + Dev->VirtIo, >> + VirtioOperationBusMasterCommonBuffer, >> + HostStatusBuffer, >> + sizeof *HostStatus, >> + &HostStatusDeviceAddress, >> + &StatusMapping >> + ); >> + if (EFI_ERROR (Status)) { >> + Status = EFI_DEVICE_ERROR; >> + goto UnmapDataBuffer; >> + } >> + >> + HostStatus = HostStatusBuffer; >> + >> + // >> // preset a host status for ourselves that we do not accept as success >> // >> - HostStatus = VIRTIO_BLK_S_IOERR; >> + *HostStatus = VIRTIO_BLK_S_IOERR; (4) I think we should be careful with BusMasterCommonBuffer operations similarly to BusMasterWrite operations -- populate first, map second. This is to avoid exposing any stale data, even for a short window, to the device. Speaking in SEV terms, IoMmuMap() will decrypt NumberOfBytes in-place -- which is by-design, but then it should decrypt what we just put there, not whatever used to be there (until we overwrite it). IOW, please move the mapping just under the *HostStatus assignment. ... I've now checked all the VirtioOperationBusMasterCommonBuffer mappings in the tree, and the rest is fine -- in fact there is only one (outside of this patch) at the moment: in VirtioRingMap(). And that is fine, because VirtioRingInit() zeroes out the entire ring, after allocating it. Probably a good idea to attend to this in the other drivers (wherever they use BusMasterCommonBuffer). Thanks! Laszlo >> >> // >> // ensured by VirtioBlkInit() -- this predicate, in combination with the >> @@ -291,8 +380,13 @@ SynchronousRequest ( >> // >> // virtio-blk header in first desc >> // >> - VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request, >> - VRING_DESC_F_NEXT, &Indices); >> + VirtioAppendDesc ( >> + &Dev->Ring, >> + RequestDeviceAddress, >> + sizeof Request, >> + VRING_DESC_F_NEXT, >> + &Indices >> + ); >> >> // >> // data buffer for read/write in second desc >> @@ -311,27 +405,62 @@ SynchronousRequest ( >> // >> // VRING_DESC_F_WRITE is interpreted from the host's point of view. >> // >> - VirtioAppendDesc (&Dev->Ring, (UINTN) Buffer, (UINT32) BufferSize, >> + VirtioAppendDesc ( >> + &Dev->Ring, >> + BufferDeviceAddress, >> + (UINT32) BufferSize, >> VRING_DESC_F_NEXT | (RequestIsWrite ? 0 : VRING_DESC_F_WRITE), >> - &Indices); >> + &Indices >> + ); >> } >> >> // >> // host status in last (second or third) desc >> // >> - VirtioAppendDesc (&Dev->Ring, (UINTN) &HostStatus, sizeof HostStatus, >> - VRING_DESC_F_WRITE, &Indices); >> + VirtioAppendDesc ( >> + &Dev->Ring, >> + HostStatusDeviceAddress, >> + sizeof *HostStatus, >> + VRING_DESC_F_WRITE, >> + &Indices >> + ); >> >> // >> // virtio-blk's only virtqueue is #0, called "requestq" (see Appendix D). >> // >> - if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, >> - NULL) == EFI_SUCCESS && >> - HostStatus == VIRTIO_BLK_S_OK) { >> - return EFI_SUCCESS; >> + Status = VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, NULL); >> + >> + // >> + // Unmap the HostStatus buffer before accessing it >> + // >> + Ret = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, StatusMapping); > > (3) Okay, so here you will assign UnmapStatus. And then, the rest: > >> + if (EFI_ERROR (Ret)) { >> + Status = EFI_DEVICE_ERROR; >> + } >> + >> + if (!EFI_ERROR (Status) && >> + *HostStatus == VIRTIO_BLK_S_OK) { >> + Status = EFI_SUCCESS; >> + } else { >> + Status = EFI_DEVICE_ERROR; >> } > > should be written like this: > > if (EFI_ERROR (Status) || EFI_ERROR (UnmapStatus) || > *HostStatus != VIRTIO_BLK_S_OK) { > Status = EFI_DEVICE_ERROR; > } > > Namely, > > - this block will ensure that we only look at *HostStatus when we're > allowed to (i.e., after both VirtioFlush() and Unmap succeeded), > > - If VirtioFlush() fails and sets Status to some error code, then we > coerce Status to EFI_DEVICE_ERROR, > > - If the entire condition evaluates to FALSE, then Status is already > EFI_SUCCESS, so no need to touch it. > > > Regarding the VirtioScsiDxe driver... in this patch, we get away with > the above shortcut (without making a mess of the code), but in the > VirtioScsiDxe driver, we won't. In that driver, the bus master device > can produce *two* output buffers, so you will have to unmap at least one > of those under an error-handling label -- when you mapped the first > successfully, and failed to map the second. > > (In this patch you managed to unmap StatusMapping in shared code, but > that only worked because you had only one output buffer, and you could > put its mapping last.) > > And once you unmap an output buffer on both the success path and the > failure path, things get more complex. I think that's OK, we should just > be consistent about it. So, for VirtioScsiDxe, I suggest the pattern I > laid out in > <http://mid.mail-archive.com/f7736294-0368-cddc-3fcd-de76cae5650e@redhat.com>. > > Thank you, > Laszlo > >> >> - return EFI_DEVICE_ERROR; >> +UnmapDataBuffer: >> + if (BufferSize > 0) { >> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping); >> + } >> + >> +UnmapRequestBuffer: >> + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping); >> + >> +FreeHostStatusBuffer: >> + Dev->VirtIo->FreeSharedPages ( >> + Dev->VirtIo, >> + EFI_SIZE_TO_PAGES (sizeof *HostStatus), >> + HostStatusBuffer >> + ); >> + >> + return Status; >> } >> >> >> > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response buffers 2017-08-27 22:05 ` Laszlo Ersek @ 2017-08-27 23:18 ` Brijesh Singh 0 siblings, 0 replies; 9+ messages in thread From: Brijesh Singh @ 2017-08-27 23:18 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel Cc: brijesh.singh, Jordan Justen, Ard Biesheuvel, Tom Lendacky On 8/27/17 5:05 PM, Laszlo Ersek wrote: [...] > (4) I think we should be careful with BusMasterCommonBuffer operations > similarly to BusMasterWrite operations -- populate first, map second. > > This is to avoid exposing any stale data, even for a short window, to > the device. > > Speaking in SEV terms, IoMmuMap() will decrypt NumberOfBytes in-place -- > which is by-design, but then it should decrypt what we just put there, > not whatever used to be there (until we overwrite it). > > IOW, please move the mapping just under the *HostStatus assignment. > > ... I've now checked all the VirtioOperationBusMasterCommonBuffer > mappings in the tree, and the rest is fine -- in fact there is only one > (outside of this patch) at the moment: in VirtioRingMap(). And that is > fine, because VirtioRingInit() zeroes out the entire ring, after > allocating it. > > Probably a good idea to attend to this in the other drivers (wherever > they use BusMasterCommonBuffer). Will do. [...] >> >>> + if (EFI_ERROR (Ret)) { >>> + Status = EFI_DEVICE_ERROR; >>> + } >>> + >>> + if (!EFI_ERROR (Status) && >>> + *HostStatus == VIRTIO_BLK_S_OK) { >>> + Status = EFI_SUCCESS; >>> + } else { >>> + Status = EFI_DEVICE_ERROR; >>> } >> should be written like this: >> >> if (EFI_ERROR (Status) || EFI_ERROR (UnmapStatus) || >> *HostStatus != VIRTIO_BLK_S_OK) { >> Status = EFI_DEVICE_ERROR; >> } >> >> Namely, >> >> - this block will ensure that we only look at *HostStatus when we're >> allowed to (i.e., after both VirtioFlush() and Unmap succeeded), Actually since the HostStatus is mapped with BusMasterCommonBuffer hence we can safely move the unmapping of HostStatus buffer later in the code. In summary, I can still retain the original if statement and do the unmapping outside the check. In v2 I will try to move the code around. thanks >> >> - If VirtioFlush() fails and sets Status to some error code, then we >> coerce Status to EFI_DEVICE_ERROR, >> >> - If the entire condition evaluates to FALSE, then Status is already >> EFI_SUCCESS, so no need to touch it. >> >> >> Regarding the VirtioScsiDxe driver... in this patch, we get away with >> the above shortcut (without making a mess of the code), but in the >> VirtioScsiDxe driver, we won't. In that driver, the bus master device >> can produce *two* output buffers, so you will have to unmap at least one >> of those under an error-handling label -- when you mapped the first >> successfully, and failed to map the second. >> >> (In this patch you managed to unmap StatusMapping in shared code, but >> that only worked because you had only one output buffer, and you could >> put its mapping last.) >> >> And once you unmap an output buffer on both the success path and the >> failure path, things get more complex. I think that's OK, we should just >> be consistent about it. So, for VirtioScsiDxe, I suggest the pattern I >> laid out in >> <http://mid.mail-archive.com/f7736294-0368-cddc-3fcd-de76cae5650e@redhat.com>. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] OvmfPkg/VirtioBlkDxe: negotiate VIRTIO_F_IOMMU_PLATFORM 2017-08-25 21:43 [PATCH 0/3] OvmfPkg/VirtioBlkDxe: map host address to device address Brijesh Singh 2017-08-25 21:43 ` [PATCH 1/3] OvmfPkg/VirtioBlkDxe: map VRING using VirtioRingMap() Brijesh Singh 2017-08-25 21:43 ` [PATCH 2/3] Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response buffers Brijesh Singh @ 2017-08-25 21:43 ` Brijesh Singh 2017-08-27 19:01 ` Laszlo Ersek 2 siblings, 1 reply; 9+ messages in thread From: Brijesh Singh @ 2017-08-25 21:43 UTC (permalink / raw) To: edk2-devel Cc: Brijesh Singh, Ard Biesheuvel, Jordan Justen, Tom Lendacky, Laszlo Ersek VirtioBlkDxe driver has been updated to use IOMMU-like member functions from VIRTIO_DEVICE_PROTOCOL to translate the system physical address to device address. We do not need to do anything special when VIRTIO_F_IOMMU_PLATFORM bit is present hence treat it in parallel with VIRTIO_F_VERSION_1. Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Tom Lendacky <thomas.lendacky@amd.com> Cc: Laszlo Ersek <lersek@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c index ab69cb08a625..3c4d9dfd4d54 100644 --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c @@ -825,7 +825,8 @@ VirtioBlkInit ( } Features &= VIRTIO_BLK_F_BLK_SIZE | VIRTIO_BLK_F_TOPOLOGY | VIRTIO_BLK_F_RO | - VIRTIO_BLK_F_FLUSH | VIRTIO_F_VERSION_1; + VIRTIO_BLK_F_FLUSH | VIRTIO_F_VERSION_1 | + VIRTIO_F_IOMMU_PLATFORM; // // In virtio-1.0, feature negotiation is expected to complete before queue @@ -903,7 +904,7 @@ VirtioBlkInit ( // step 5 -- Report understood features. // if (Dev->VirtIo->Revision < VIRTIO_SPEC_REVISION (1, 0, 0)) { - Features &= ~(UINT64)VIRTIO_F_VERSION_1; + Features &= ~(UINT64)(VIRTIO_F_VERSION_1 | VIRTIO_F_IOMMU_PLATFORM); Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features); if (EFI_ERROR (Status)) { goto UnmapQueue; -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] OvmfPkg/VirtioBlkDxe: negotiate VIRTIO_F_IOMMU_PLATFORM 2017-08-25 21:43 ` [PATCH 3/3] OvmfPkg/VirtioBlkDxe: negotiate VIRTIO_F_IOMMU_PLATFORM Brijesh Singh @ 2017-08-27 19:01 ` Laszlo Ersek 0 siblings, 0 replies; 9+ messages in thread From: Laszlo Ersek @ 2017-08-27 19:01 UTC (permalink / raw) To: Brijesh Singh, edk2-devel; +Cc: Jordan Justen, Tom Lendacky, Ard Biesheuvel On 08/25/17 23:43, Brijesh Singh wrote: > VirtioBlkDxe driver has been updated to use IOMMU-like member functions > from VIRTIO_DEVICE_PROTOCOL to translate the system physical address to > device address. We do not need to do anything special when > VIRTIO_F_IOMMU_PLATFORM bit is present hence treat it in parallel with > VIRTIO_F_VERSION_1. > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Tom Lendacky <thomas.lendacky@amd.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c > index ab69cb08a625..3c4d9dfd4d54 100644 > --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c > +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c > @@ -825,7 +825,8 @@ VirtioBlkInit ( > } > > Features &= VIRTIO_BLK_F_BLK_SIZE | VIRTIO_BLK_F_TOPOLOGY | VIRTIO_BLK_F_RO | > - VIRTIO_BLK_F_FLUSH | VIRTIO_F_VERSION_1; > + VIRTIO_BLK_F_FLUSH | VIRTIO_F_VERSION_1 | > + VIRTIO_F_IOMMU_PLATFORM; > > // > // In virtio-1.0, feature negotiation is expected to complete before queue > @@ -903,7 +904,7 @@ VirtioBlkInit ( > // step 5 -- Report understood features. > // > if (Dev->VirtIo->Revision < VIRTIO_SPEC_REVISION (1, 0, 0)) { > - Features &= ~(UINT64)VIRTIO_F_VERSION_1; > + Features &= ~(UINT64)(VIRTIO_F_VERSION_1 | VIRTIO_F_IOMMU_PLATFORM); > Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features); > if (EFI_ERROR (Status)) { > goto UnmapQueue; > Reviewed-by: Laszlo Ersek <lersek@redhat.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-08-27 23:15 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-08-25 21:43 [PATCH 0/3] OvmfPkg/VirtioBlkDxe: map host address to device address Brijesh Singh 2017-08-25 21:43 ` [PATCH 1/3] OvmfPkg/VirtioBlkDxe: map VRING using VirtioRingMap() Brijesh Singh 2017-08-27 18:58 ` Laszlo Ersek 2017-08-25 21:43 ` [PATCH 2/3] Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response buffers Brijesh Singh 2017-08-27 20:40 ` Laszlo Ersek 2017-08-27 22:05 ` Laszlo Ersek 2017-08-27 23:18 ` Brijesh Singh 2017-08-25 21:43 ` [PATCH 3/3] OvmfPkg/VirtioBlkDxe: negotiate VIRTIO_F_IOMMU_PLATFORM Brijesh Singh 2017-08-27 19:01 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox