Re: [PATCH v1 07/14] OvmfPkg/VirtioLib: Use AllocateShared() to allocate vring buffer

[Laszlo Ersek <lersek@redhat.com>]

On 08/07/17 13:58, Brijesh Singh wrote:
> The vring buffer is communication block between guest and hypervisor,
> allocate the vring buffer using AllocateSharedPages() so that it can be
> Map() with BusMasterCommonBufer 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.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> index 09a3f9b7f2e5..9ba64204326f 100644
> --- a/OvmfPkg/Library/VirtioLib/VirtioLib.c
> +++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> @@ -61,6 +61,7 @@ VirtioRingInit (
>    OUT VRING                  *Ring
>    )
>  {
> +  EFI_STATUS     Status;
>    UINTN          RingSize;
>    volatile UINT8 *RingPagesPtr;
>  
> @@ -79,9 +80,12 @@ 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) {
> +  Status = VirtioAllocateSharedPages (VirtIo, Ring->NumPages, &Ring->Base);
> +  if (EFI_ERROR (Status)) {
>      return EFI_OUT_OF_RESOURCES;
>    }
>    SetMem (Ring->Base, RingSize, 0x00);
> @@ -143,7 +147,7 @@ VirtioRingUninit (
>    IN OUT VRING                  *Ring
>    )
>  {
> -  FreePages (Ring->Base, Ring->NumPages);
> +  VirtioFreeSharedPages (VirtIo, Ring->NumPages, Ring->Base);
>    SetMem (Ring, sizeof *Ring, 0x00);
>  }
>  
> 

The logic looks good, I have some superficial comments:

(1) Please update the subject, the commit message, and the code to refer
to VirtIo->AllocateSharedPages() (or even VIRTIO_ALLOCATE_SHARED, as you
see fit -- just not the helper function, because we won't have that).

(2) Please update the documentation of "@retval EFI_OUT_OF_RESOURCES" --
replace it with

  @return  Status codes propagated from VirtIo->AllocateSharedPages().

(3) Accordingly, please forward the Status received, on the error branch.

(4) Please remove MemoryAllocationLib from the #includes and also from
[LibraryClasses].

Thanks,
Laszlo