Re: [PATCH v2 16/23] OvmfPkg/VirtioRngDxe: map host address to device address

[Laszlo Ersek <lersek@redhat.com>]

On 08/14/17 13:36, 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 <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 | 65 +++++++++++++++++---
>  2 files changed, 58 insertions(+), 8 deletions(-)
> 
> 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;

(1) Please change the "depth" value from 1 to 2 on the right-hand side.
"RingMap" has the same initialization depth as "Ring".

Also, please change the function name from "VirtioRngInit" to
"VirtioRingMap".

>  
>  #define VIRTIO_ENTROPY_SOURCE_FROM_RNG(RngPointer) \
> diff --git a/OvmfPkg/VirtioRngDxe/VirtioRng.c b/OvmfPkg/VirtioRngDxe/VirtioRng.c
> index 0abca488e6cd..fc01f1996654 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 the Buffers system phyiscal address to device address
> +  //

(2) s/the Buffers/Buffer's/

> +  Status = VirtioMapAllBytesInSharedBuffer (
> +             Dev->VirtIo,
> +             VirtioOperationBusMasterWrite,
> +             (VOID *)Buffer,
> +             RNGValueLength,
> +             &DeviceAddress,
> +             &Mapping
> +             );
> +  if (EFI_ERROR (Status)) {
> +    goto FreeBuffer;
> +  }
>  
>    //
>    // 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,
> +      (UINTN)DeviceAddress + Index,

(3) Please insert a new VirtioLib patch in the series, before this
patch. The new patch should:

(3a) change the "BufferPhysAddr" parameter of VirtioAppendDesc() from
     type UINTN to UINT64,

(3b) rename the same parameter to "BufferDeviceAddress",

(3c) replace the following parameter documentation string:

     "(Guest pseudo-physical)"

     with the string

     "Bus master device"

Justification:

UINTN is appropriate as long as we pass system memory references. After
the introduction of this feature, that's no longer the case in general.
Should we implement "real" IOMMU support at some point, UINTN could
break in 32-bit builds of OVMF.

The definition of VirtioAppendDesc() itself needs no update (beyond the
parameter rename), because the VRING_DESC.Addr field already has type
UINT64.


(4) In this patch, please remove the UINTN cast, and just say

  DeviceAddress + Index

Same for the rest of the patches. (I haven't seen them yet, but I assume
the same pattern is repeated in them.)

>        BufferSize,
>        VRING_DESC_F_WRITE,
>        &Indices);
> @@ -178,17 +194,30 @@ 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 accesing it.
> +  //
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping);
> +

(5) In the following messages:

-
http://mid.mail-archive.com/f953ec03-b160-726b-b9ed-5e3122642815@redhat.com
- http://mid.mail-archive.com/35b958de-ed97-6275-be96-1cbb93a99d97@amd.com

we agreed that EFI_STATUS was the appropriate return type for
VIRTIO_UNMAP_SHARED. We also agreed that we'd check the return value
when necessary -- that is, when we are after a bus master write
operation and want to consume the data produced by the device.

So, please check the return status here, and jump to FreeBuffer if the
unmap fails.

(With VIRTIO_UNMAP_SHARED, we imitate EFI_PCI_IO_PROTOCOL.Unmap(). The
specification says, "Any resources used for the mapping are freed" --
this is unconditional. The part that can fail is "committing the data to
target system memory". So, if the UnmapSharedBuffer() call fails, we
must skip consuming the data, but still proceed with freeing the buffer.)

Ignoring the return value of UnmapSharedBuffer() is OK on error handling
paths.

>    for (Index = 0; Index < RNGValueLength; Index++) {
>      RNGValue[Index] = Buffer[Index];
>    }
>    Status = EFI_SUCCESS;
>  
> +  //
> +  // Buffer is already Unmaped(), goto free it.
> +  //
> +  goto FreeBuffer;

(6) We restrict "goto" statements to error handling:

https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/57_c_programming.html

> 5.7.3.8 Goto Statements should not be used (in general)
> [...]
> It is common to use goto for error handling and thus, exiting a
> routine in an error case is the only legal use of a goto. [...]

So please open-code the FreePool() call and the "return" statement here.

> +
> +UnmapBuffer:
> +    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Mapping);
> +

(7) The function call is incorrectly indented.

>  FreeBuffer:
>    FreePool ((VOID *)Buffer);
>    return Status;
> @@ -205,6 +234,7 @@ VirtioRngInit (
>    EFI_STATUS Status;
>    UINT16     QueueSize;
>    UINT64     Features;
> +  UINT64     RingBaseShift;
>  
>    //
>    // Execute virtio-0.9.5, 2.2.1 Device Initialization Sequence.
> @@ -281,26 +311,33 @@ VirtioRngInit (
>      goto Failed;
>    }
>  

(8) Please add a comment here:

  //
  // If anything fails from here on, we must release the ring resources.
  //

> +  Status = VirtioRingMap (Dev->VirtIo, &Dev->Ring, &RingBaseShift,
> +             &Dev->RingMap);

(9) Please break each argument to a separate line.

(As much as I personally prefer the style visible above, it is not (one
of) the edk2 coding style(s). :( Not yet, anyway.)

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

(10) Please replace the word "release" with "unmap", in the above comment.

>    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);

(11) Please break each argument to a separate line.

>    if (EFI_ERROR (Status)) {
> -    goto ReleaseQueue;
> +    goto UnmapQueue;
>    }
>  
>    //
> @@ -310,7 +347,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 +357,7 @@ VirtioRngInit (
>    NextDevStat |= VSTAT_DRIVER_OK;
>    Status = Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, NextDevStat);
>    if (EFI_ERROR (Status)) {
> -    goto ReleaseQueue;
> +    goto UnmapQueue;
>    }
>  
>    //
> @@ -331,6 +368,9 @@ VirtioRngInit (
>  
>    return EFI_SUCCESS;
>  
> +UnmapQueue:
> +    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
> +

(12) The function call is incorrectly indented.

>  ReleaseQueue:
>    VirtioRingUninit (Dev->VirtIo, &Dev->Ring);
>  
> @@ -359,6 +399,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 +428,12 @@ VirtioRngExitBoot (
>    //
>    Dev = Context;
>    Dev->VirtIo->SetDeviceStatus (Dev->VirtIo, 0);
> +
> +  //
> +  // Umap the ring buffer so that hypervisor will not able to get readable data

(13) s/Umap/Unmap/; also s/will not able/will not be able/

(Please make sure the line doesn't become overlong.)

> +  // after device reset.
> +  //
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, Dev->RingMap);
>  }
>  
>  
> 

The logic seems fine otherwise.

Thanks!
Laszlo