Re: [PATCH v3 10/23] OvmfPkg/VirtioLib: alloc VRING buffer with AllocateSharedPages()

[Laszlo Ersek <lersek@redhat.com>]

On 08/23/17 14:22, Brijesh Singh wrote:
> The VRING buffer is a communication area between guest and hypervisor.
> Allocate it using VIRTIO_DEVICE_PROTOCOL.AllocateSharedPages() so that
> it can be mapped later with VirtioRingMap() for bi-directional access.
> 
> 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/Library/VirtioLib/VirtioLib.inf |  1 -
>  OvmfPkg/Include/Library/VirtioLib.h     |  5 ++---
>  OvmfPkg/Library/VirtioLib/VirtioLib.c   | 22 +++++++++++++-------
>  3 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.inf b/OvmfPkg/Library/VirtioLib/VirtioLib.inf
> index fb5897a88ecf..e33856de38c4 100644
> --- a/OvmfPkg/Library/VirtioLib/VirtioLib.inf
> +++ b/OvmfPkg/Library/VirtioLib/VirtioLib.inf
> @@ -32,5 +32,4 @@ [LibraryClasses]
>    BaseLib
>    BaseMemoryLib
>    DebugLib
> -  MemoryAllocationLib
>    UefiBootServicesTableLib
> diff --git a/OvmfPkg/Include/Library/VirtioLib.h b/OvmfPkg/Include/Library/VirtioLib.h
> index 3d9314b3acaf..c3e56ea23b89 100644
> --- a/OvmfPkg/Include/Library/VirtioLib.h
> +++ b/OvmfPkg/Include/Library/VirtioLib.h
> @@ -42,9 +42,8 @@
>  
>    @param[out] Ring              The virtio ring to set up.
>  
> -  @retval EFI_OUT_OF_RESOURCES  AllocatePages() failed to allocate contiguous
> -                                pages for the requested QueueSize. Fields of
> -                                Ring have indeterminate value.
> +  @retval                       Status codes propagated from
> +                                VirtIo->AllocateSharedPages().

(1) The documentation has been updated (thanks for that), but I also
requested "@return", not "@retval". Can be fixed up on push.

>  
>    @retval EFI_SUCCESS           Allocation and setup successful. Ring->Base
>                                  (and nothing else) is responsible for
> diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> index 535635ac0ba8..e5366e385f5d 100644
> --- a/OvmfPkg/Library/VirtioLib/VirtioLib.c
> +++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> @@ -19,7 +19,6 @@
>  #include <Library/BaseLib.h>
>  #include <Library/BaseMemoryLib.h>
>  #include <Library/DebugLib.h>
> -#include <Library/MemoryAllocationLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  
>  #include <Library/VirtioLib.h>
> @@ -44,9 +43,8 @@
>  
>    @param[out] Ring              The virtio ring to set up.
>  
> -  @retval EFI_OUT_OF_RESOURCES  AllocatePages() failed to allocate contiguous
> -                                pages for the requested QueueSize. Fields of
> -                                Ring have indeterminate value.
> +  @retval                       Status codes propagated from
> +                                VirtIo->AllocateSharedPages().

(2) Same as (1), should be @return.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

>  
>    @retval EFI_SUCCESS           Allocation and setup successful. Ring->Base
>                                  (and nothing else) is responsible for
> @@ -61,6 +59,7 @@ VirtioRingInit (
>    OUT VRING                  *Ring
>    )
>  {
> +  EFI_STATUS     Status;
>    UINTN          RingSize;
>    volatile UINT8 *RingPagesPtr;
>  
> @@ -79,10 +78,17 @@ VirtioRingInit (
>                  sizeof *Ring->Used.AvailEvent,
>                  EFI_PAGE_SIZE);
>  
> +  //
> +  // Allocate a shared ring buffer
> +  //
>    Ring->NumPages = EFI_SIZE_TO_PAGES (RingSize);
> -  Ring->Base = AllocatePages (Ring->NumPages);
> -  if (Ring->Base == NULL) {
> -    return EFI_OUT_OF_RESOURCES;
> +  Status = VirtIo->AllocateSharedPages (
> +                     VirtIo,
> +                     Ring->NumPages,
> +                     &Ring->Base
> +                     );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
>    }
>    SetMem (Ring->Base, RingSize, 0x00);
>    RingPagesPtr = Ring->Base;
> @@ -143,7 +149,7 @@ VirtioRingUninit (
>    IN OUT VRING                  *Ring
>    )
>  {
> -  FreePages (Ring->Base, Ring->NumPages);
> +  VirtIo->FreeSharedPages (VirtIo, Ring->NumPages, Ring->Base);
>    SetMem (Ring, sizeof *Ring, 0x00);
>  }
>  
>