On 08/23/17 14:22, Brijesh Singh wrote: > patch maps the host address to a device address for buffers (including > rings, device specifc request and response pointed by vring descriptor, > and any further memory reference by those request and response). > > 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/VirtioRngDxe/VirtioRng.h | 1 + > OvmfPkg/VirtioRngDxe/VirtioRng.c | 82 +++++++++++++++++--- > 2 files changed, 74 insertions(+), 9 deletions(-) > > diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.h > b/OvmfPkg/VirtioRngDxe/VirtioRng.h > index 998f9fae48c2..389c8ddc8d31 100644 > --- a/OvmfPkg/VirtioRngDxe/VirtioRng.h > +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.h > @@ -38,6 +38,7 @@ typedef struct { > EFI_EVENT ExitBoot; // DriverBindingStart 0 > VRING Ring; // VirtioRingInit 2 > EFI_RNG_PROTOCOL Rng; // VirtioRngInit 1 > + VOID *RingMap; // VirtioRingMap 2 > } VIRTIO_RNG_DEV; > > #define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \ > diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c > b/OvmfPkg/VirtioRngDxe/VirtioRng.c > index 0abca488e6cd..59f32d343179 100644 > --- a/OvmfPkg/VirtioRngDxe/VirtioRng.c > +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c > @@ -140,6 +140,8 @@ VirtioRngGetRNG ( > UINT32 Len; > UINT32 BufferSize; > EFI_STATUS Status; > + EFI_PHYSICAL_ADDRESS DeviceAddress; > + VOID *Mapping; > > if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) { > return EFI_INVALID_PARAMETER; > @@ -159,6 +161,20 @@ VirtioRngGetRNG ( > } > > Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This); > + // > + // Map Buffer's system phyiscal address to device address > + // > + Status = VirtioMapAllBytesInSharedBuffer ( > + Dev->VirtIo, > + VirtioOperationBusMasterWrite, > + (VOID *)Buffer, > + RNGValueLength, > + &DeviceAddress, > + &Mapping > + ); > + if (EFI_ERROR (Status)) { > + goto FreeBuffer; > + }
(1) Sorry, I missed this in my previous review: According to the interface contract of this protocol member function, we should set Status to EFI_DEVICE_ERROR here, before jumping to the FreeBuffer label. (Both old code, and new code other than this, do it.) I can fix this up. With that: Reviewed-by: Laszlo Ersek <[email protected]> Thanks, Laszlo > > // > // The Virtio RNG device may return less data than we asked it to, and can > @@ -170,7 +186,7 @@ VirtioRngGetRNG ( > > VirtioPrepare (&Dev->Ring, &Indices); > VirtioAppendDesc (&Dev->Ring, > - (UINTN)Buffer + Index, > + DeviceAddress + Index, > BufferSize, > VRING_DESC_F_WRITE, > &Indices); > @@ -178,17 +194,35 @@ VirtioRngGetRNG ( > if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, &Len) != > EFI_SUCCESS) { > Status = EFI_DEVICE_ERROR; > - goto FreeBuffer; > + goto UnmapBuffer; > } > ASSERT (Len > 0); > ASSERT (Len <= BufferSize); > } > > + // > + // Unmap the device buffer before accessing it. > + // > + Status = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping); > + if (EFI_ERROR (Status)) { > + Status = EFI_DEVICE_ERROR; > + goto FreeBuffer; > + } > + > for (Index = 0; Index < RNGValueLength; Index++) { > RNGValue[Index] = Buffer[Index]; > } > Status = EFI_SUCCESS; > > +UnmapBuffer: > + // > + // If we are reached here due to the error then unmap the buffer otherwise > + // the buffer is already unmapped after VirtioFlush(). > + // > + if (EFI_ERROR (Status)) { > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping); > + } > + > FreeBuffer: > FreePool ((VOID *)Buffer); > return Status; > @@ -205,6 +239,7 @@ VirtioRngInit ( > EFI_STATUS Status; > UINT16 QueueSize; > UINT64 Features; > + UINT64 RingBaseShift; > > // > // Execute virtio-0.9.5, 2.2.1 Device Initialization Sequence. > @@ -282,25 +317,42 @@ VirtioRngInit ( > } > > // > + // 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; > } > > // > @@ -310,7 +362,7 @@ VirtioRngInit ( > Features &= ~(UINT64)VIRTIO_F_VERSION_1; > Status = Dev->VirtIo->SetGuestFeatures (Dev->VirtIo, Features); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > } > > @@ -320,7 +372,7 @@ VirtioRngInit ( > NextDevStat |= VSTAT_DRIVER_OK; > Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat); > if (EFI_ERROR (Status)) { > - goto ReleaseQueue; > + goto UnmapQueue; > } > > // > @@ -331,6 +383,9 @@ VirtioRngInit ( > > return EFI_SUCCESS; > > +UnmapQueue: > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap); > + > ReleaseQueue: > VirtioRingUninit (Dev->VirtIo, &Dev->Ring); > > @@ -359,6 +414,9 @@ VirtioRngUninit ( > // the old comms area. > // > Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); > + > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap); > + > VirtioRingUninit (Dev->VirtIo, &Dev->Ring); > } > > @@ -385,6 +443,12 @@ VirtioRngExitBoot ( > // > 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 reset. > + // > + Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap); > } > > > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
