On 08/10/17 02:25, Laszlo Ersek wrote: > On 08/07/17 13:58, Brijesh Singh wrote: >> The patch uses newly introduced VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer() >> to map system memory to device address and programs the vring descriptors >> with device addresses. >> >> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> >> Cc: Jordan Justen <jordan.l.jus...@intel.com> >> Cc: Tom Lendacky <thomas.lenda...@amd.com> >> Cc: Laszlo Ersek <ler...@redhat.com> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com> >> --- >> OvmfPkg/VirtioRngDxe/VirtioRng.h | 1 + >> OvmfPkg/VirtioRngDxe/VirtioRng.c | 47 +++++++++++++++++--- >> 2 files changed, 43 insertions(+), 5 deletions(-) > > I'm skipping all the other device driver conversion patches for now (v1 > 8-13), because I think that this is the simplest driver, and we already > have enough changes queued from this review. > > I suggest that for v2, you please update and test all of the drivers (so > that we can be sure that my requests are feasible), but move this driver > patch in front of the others. > > I'll say a number of generalities: > > * Please never roll-back earlier actions directly within an > error-catching "if" -- instead, insert a new error handling label at > the appropriate place, and jump to it with "goto". Sooner or later > we'll grow yet more actions, and then we'll have to convert those "if" > bodies to goto's anyway. > > * NULL for RingMap is a valid value () -- see also point (8) in my v1 > 03/14 feedback -- and device driver logic should not depend on it. > Instead, use additional (precise) labels -- such as "UnmapQueue" and > "UnmapBuffer" -- and matching goto statements. > > If a VirtIo->MapSharedBuffer() implementation gives you RingMap=NULL > (on success), then VirtIo->UnmapSharedBuffer() will also take > RingMap=NULL -- you simply don't have to be aware of the value. > > * In most cases, the fact that you have a live exit-boot-services > callback implies that your device is "live" as well. So no need for > additional checks before unmapping the queue. (There are exceptions of > course, such as virtio-net -- IIRC the SimpleNetworkProtocol has an > internal state flag that affects this question.) > > For example, in the virtio-rng case, the exit-boot-services > notification function is installed in VirtioRngDriverBindingStart() > *after* the call to VirtioRngInit(). In addition, it is uninstalled -- > its associated event is closed -- in VirtioRngDriverBindingStop(), > *before* the call to VirtioRngUninit(). So whenever the notification > function can run, the device is guaranteed to be live.
Something else: please don't forget about the VIRTIO_F_IOMMU_PLATFORM flag -- pls see point (5.3) in <841bec5f-6f6e-8b1f-25ba-0fd37a915b72@redhat.com">http://mid.mail-archive.com/841bec5f-6f6e-8b1f-25ba-0fd37a915b72@redhat.com>. After this conversion is complete, we can claim that our virtio device drivers are "IOMMU aware", whenever we negotiate VIRTIO_F_VERSION_1. (It's a different question what IOMMU drivers we have.) Thanks, Laszlo >> diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.h >> b/OvmfPkg/VirtioRngDxe/VirtioRng.h >> index 998f9fae48c2..b372d84c1ebc 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; // VirtioRngInit 1 >> } VIRTIO_RNG_DEV; >> >> #define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \ >> diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c >> b/OvmfPkg/VirtioRngDxe/VirtioRng.c >> index e20602ac7225..5ff54867616a 100644 >> --- a/OvmfPkg/VirtioRngDxe/VirtioRng.c >> +++ b/OvmfPkg/VirtioRngDxe/VirtioRng.c >> @@ -140,11 +140,15 @@ VirtioRngGetRNG ( >> UINT32 Len; >> UINT32 BufferSize; >> EFI_STATUS Status; >> + UINTN NumPages; >> + EFI_PHYSICAL_ADDRESS DeviceAddress; >> + VOID *Mapping; >> >> if (This == NULL || RNGValueLength == 0 || RNGValue == NULL) { >> return EFI_INVALID_PARAMETER; >> } >> >> + Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This); >> // >> // We only support the raw algorithm, so reject requests for anything else >> // >> @@ -153,12 +157,18 @@ VirtioRngGetRNG ( >> return EFI_UNSUPPORTED; >> } >> >> - Buffer = (volatile UINT8 *)AllocatePool (RNGValueLength); >> - if (Buffer == NULL) { >> + NumPages = EFI_SIZE_TO_PAGES (RNGValueLength); >> + Status = VirtioAllocateSharedPages (Dev->VirtIo, NumPages, (VOID >> *)&Buffer); >> + if (EFI_ERROR (Status)) { >> return EFI_DEVICE_ERROR; >> } >> >> - Dev = VIRTIO_ENTROPY_SOURCE_FROM_RNG (This); >> + Status = VirtioMapSharedBufferWrite (Dev->VirtIo, (VOID *)Buffer, >> + RNGValueLength, &DeviceAddress, &Mapping); >> + if (EFI_ERROR (Status)) { >> + VirtioFreeSharedPages (Dev->VirtIo, NumPages, (VOID *)Buffer); >> + return EFI_DEVICE_ERROR; >> + } >> >> // >> // The Virtio RNG device may return less data than we asked it to, and can >> @@ -170,7 +180,7 @@ VirtioRngGetRNG ( >> >> VirtioPrepare (&Dev->Ring, &Indices); >> VirtioAppendDesc (&Dev->Ring, >> - (UINTN)Buffer + Index, >> + (UINTN)DeviceAddress + Index, >> BufferSize, >> VRING_DESC_F_WRITE, >> &Indices); >> @@ -178,19 +188,22 @@ VirtioRngGetRNG ( >> if (VirtioFlush (Dev->VirtIo, 0, &Dev->Ring, &Indices, &Len) != >> EFI_SUCCESS) { >> Status = EFI_DEVICE_ERROR; >> + VirtioUnmapSharedBuffer (Dev->VirtIo, Mapping); >> goto FreeBuffer; >> } >> ASSERT (Len > 0); >> ASSERT (Len <= BufferSize); >> } >> >> + VirtioUnmapSharedBuffer (Dev->VirtIo, Mapping); >> + >> for (Index = 0; Index < RNGValueLength; Index++) { >> RNGValue[Index] = Buffer[Index]; >> } >> Status = EFI_SUCCESS; >> >> FreeBuffer: >> - FreePool ((VOID *)Buffer); >> + VirtioFreeSharedPages (Dev->VirtIo, NumPages, (VOID *)Buffer); >> return Status; >> } >> >> @@ -281,6 +294,11 @@ VirtioRngInit ( >> goto Failed; >> } >> >> + Status = VirtioRingMap (Dev->VirtIo, &Dev->Ring, &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. >> @@ -332,6 +350,11 @@ VirtioRngInit ( >> return EFI_SUCCESS; >> >> ReleaseQueue: >> + if (Dev->RingMap != NULL) { >> + VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap); >> + Dev->RingMap = NULL; >> + } >> + >> VirtioRingUninit (Dev->VirtIo, &Dev->Ring); >> >> Failed: >> @@ -359,6 +382,11 @@ VirtioRngUninit ( >> // the old comms area. >> // >> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); >> + >> + if (Dev->RingMap != NULL) { >> + VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap); >> + } >> + >> VirtioRingUninit (Dev->VirtIo, &Dev->Ring); >> } >> >> @@ -385,6 +413,15 @@ VirtioRngExitBoot ( >> // >> Dev = Context; >> Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0); >> + >> + // >> + // If Ring mapping exist then umap it so that hypervisor will not able to >> + // get readable data after device reset. >> + // >> + if (Dev->RingMap != NULL) { >> + VirtioRingUnmap (Dev->VirtIo, &Dev->Ring, Dev->RingMap); >> + Dev->RingMap = NULL; >> + } >> } >> >> >> > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
