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.
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
