Re: [PATCH v1 14/14] OvmfPkg/VirtioRngDxe: Use DeviceAddresses in vring descriptors

[Laszlo Ersek <lersek@redhat.com>]

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.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/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;
> +  }
>  }
>  
>  
>