Re: [PATCH v2 15/23] OvmfPkg/VirtioLib: alloc vring buffer with AllocateSharedPages()

[Laszlo Ersek <lersek@redhat.com>]

On 08/14/17 13:36, Brijesh Singh wrote:
> 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 | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)

(1) Where did the commit message go? :) It was pretty good. Here's my
suggestion for reinstating it:

    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.

(This is a VirtioLib patch so we can refer to VirtioRingMap() -- after
the reordering that I requested under [PATCH v2 12/23], point (2), the
VirtioRingMap() function will be introduced just before this patch.)

> diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> index 7d07dcc09d3d..8010b5cf4735 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,16 @@ 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 = VirtIo->AllocateSharedPages (
> +                          VirtIo,
> +                          Ring->NumPages,
> +                          &Ring->Base
> +                          );
> +  if (EFI_ERROR (Status)) {
>      return EFI_OUT_OF_RESOURCES;
>    }
>    SetMem (Ring->Base, RingSize, 0x00);
> @@ -143,7 +151,7 @@ VirtioRingUninit (
>    IN OUT VRING                  *Ring
>    )
>  {
> -  FreePages (Ring->Base, Ring->NumPages);
> +  VirtIo->FreeSharedPages (VirtIo, Ring->NumPages, Ring->Base);
>    SetMem (Ring, sizeof *Ring, 0x00);
>  }
>
>

You seem to have missed my points (2), (3) and (4) from my previous
review
<http://mid.mail-archive.com/26a6be37-9983-7a34-afbf-d25e5501b138@redhat.com>:

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

(5) In addition, the indentation of the AllocateSharedPages() arguments
is incorrect.

To be continued.

Thanks,
Laszlo