Re: [PATCH v2 12/23] OvmfPkg/VirtioLib: add functions to map/unmap VRING

[Laszlo Ersek <lersek@redhat.com>]

On 08/14/17 13:36, Brijesh Singh wrote:
> Add functions to map and unmap the ring buffer with BusMasterCommonBuffer
> so that ring can be accessed by both guest and hypervisor.
> 
> 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/Include/Library/VirtioLib.h   | 25 +++++++++++
>  OvmfPkg/Library/VirtioLib/VirtioLib.c | 44 ++++++++++++++++++++
>  2 files changed, 69 insertions(+)

(1) The subject and the commit message both should say "a function"
(singular) now -- we've only kept VirtioRingMap().

Unmap shoud not be referenced in either the subject or the commit message.

> 
> diff --git a/OvmfPkg/Include/Library/VirtioLib.h b/OvmfPkg/Include/Library/VirtioLib.h
> index ca0b217a04eb..1efb02a68cf1 100644
> --- a/OvmfPkg/Include/Library/VirtioLib.h
> +++ b/OvmfPkg/Include/Library/VirtioLib.h
> @@ -62,6 +62,31 @@ VirtioRingInit (
>  
>  /**
>  
> +  Map the ring buffer so that it can be accessed equally by both guest
> +  and hypervisor.
> +
> +  @param[in]      VirtIo          The virtio device instance.
> +
> +  @param[in]      Ring            The virtio ring to map.
> +
> +  @param[out]     RingBaseShift   

The fact that you either forgot, or had difficulties with, the
explanation of the RingBaseShift output parameter, is not random. It has
a deeper cause.

The cause is that placing this patch at this exact position in the
series constitutes a layering violation.

In my previous review
<http://mid.mail-archive.com/01220a3b-565b-8a36-406d-cfcc63265fb7@redhat.com>,
point (10), I tried to clarify this ("*Then* please include the present
patch", emphasis added now), but I must have been unclear.

So, the idea is that
- the VirtioLib helper functions (code, comments, and commit messages)
are allowed to reference VIRTIO_DEVICE_PROTOCOL specifics,
- but references in the opposite direction should be avoided if possible.

You are introducing this patch at position 12 in the series, and in the
next patch, for the virtio protocol, you refer to VirtioRingMap() in the
commit message. This should not be done -- the opposite dependency
should be constructed.

(2) So please, move this patch two positions toward the tail of the series:

  OvmfPkg/VirtioLib: add functions to map/unmap VRING -----------+
  OvmfPkg/Virtio: take RingBaseShift in VirtioSetQueueAddress()  |
  OvmfPkg/Virtio10Dxe: add the RingBaseShift offset              |
  <HERE> <-------------------------------------------------------+
  OvmfPkg/VirtioLib: alloc vring buffer with AllocateSharedPages()

(3) Then you can document the RingBaseShift parameter simply like this,
in this patch:

  @param[out] RingBaseShift  A resulting translation offset, to be
                             passed to VirtIo->SetQueueAddress().


> +
> +  @param[out]     Mapping         A resulting token to pass to
> +                                  VirtIo->UnmapSharedBuffer().
> +
> +  @return         Status from VirtIo->MapSharedBuffer()

(4) This is an improvement on the previous version of the patch, but the
.c and .h files differ ("Status code" vs "Status").

So please sync the full comment blocks (after addressing (3) as well).

> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioRingMap (
> +  IN  VIRTIO_DEVICE_PROTOCOL *VirtIo,
> +  IN  VRING                  *Ring,
> +  OUT UINT64                 *RingBaseShift,
> +  OUT VOID                   **Mapping
> +  );
> +
> +/**
> +
>    Tear down the internal resources of a configured virtio ring.
>  
>    The caller is responsible to stop the host from using this ring before
> diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> index 5b64d18a8d6f..7d07dcc09d3d 100644
> --- a/OvmfPkg/Library/VirtioLib/VirtioLib.c
> +++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> @@ -498,3 +498,47 @@ Failed:
>    *Mapping = NULL;
>    return EFI_OUT_OF_RESOURCES;
>  }
> +
> +/**
> +
> +  Map the ring buffer so that it can be accessed equally by both guest
> +  and hypervisor.
> +
> +  @param[in]      VirtIo          The virtio device instance.
> +
> +  @param[in]      Ring            The virtio ring to map.
> +
> +  @param[out]     RingBaseShift   RingBaseShift
> +
> +  @param[out]     Mapping         A resulting token to pass to
> +                                  VirtIo->UnmapSharedBuffer().
> +
> +  @return         Status code from VirtIo->MapSharedBuffer()
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioRingMap (
> +  IN  VIRTIO_DEVICE_PROTOCOL *VirtIo,
> +  IN  VRING                  *Ring,
> +  OUT UINT64                 *RingBaseShift,
> +  OUT VOID                   **Mapping
> +  )
> +{
> +  EFI_STATUS        Status;
> +  PHYSICAL_ADDRESS  DeviceAddress;

(5) Technically this is correct (both EFI_PHYSICAL_ADDRESS and
PHYSICAL_ADDRESS are typedefs for UINT64).

For consistency with the prototype of VirtioMapAllBytesInSharedBuffer(),
EFI_PHYSICAL_ADDRESS would be slightly better in my opinion.

> +
> +  Status = VirtioMapAllBytesInSharedBuffer (
> +             VirtIo,
> +             VirtioOperationBusMasterCommonBuffer,
> +             Ring->Base,
> +             EFI_PAGES_TO_SIZE (Ring->NumPages),
> +             &DeviceAddress,
> +             Mapping

This is fine -- we're passing "Mapping" directly to
VirtioMapAllBytesInSharedBuffer(), but we know that said function won't
modify Mapping if it fails.

(Just please address point (8) in my "PATCH v2 10/23" feedback -- i.e.,
VirtioMapAllBytesInSharedBuffer() should not null "Mapping" on error.)

> +             );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  *RingBaseShift = DeviceAddress - (UINT64)(UINTN)Ring->Base;
> +  return EFI_SUCCESS;
> +}
> 

Looks good!

Thank you!
Laszlo