Re: [PATCH v3 11/23] OvmfPkg/VirtioLib: change the parameter of VirtioAppendDesc() to UINT64

[Laszlo Ersek <lersek@redhat.com>]

On 08/23/17 14:22, Brijesh Singh wrote:
> The patch change the "BufferPhysAddr" parameter of VirtioAppendDesc()
> from type UINTN to UINT64.
> 
> 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.

(1) s/this feature/bus master device addresses/

> Should we implement "real" IOMMU support at some point, UINTN could
> break in 32-bit builds of OVMF.
> 
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> 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   | 35 +++++++++---------
>  OvmfPkg/Library/VirtioLib/VirtioLib.c | 38 ++++++++++----------
>  2 files changed, 37 insertions(+), 36 deletions(-)
> 
> diff --git a/OvmfPkg/Include/Library/VirtioLib.h b/OvmfPkg/Include/Library/VirtioLib.h
> index c3e56ea23b89..a966311ac941 100644
> --- a/OvmfPkg/Include/Library/VirtioLib.h
> +++ b/OvmfPkg/Include/Library/VirtioLib.h
> @@ -151,33 +151,34 @@ VirtioPrepare (
>    The caller is responsible for initializing *Indices with VirtioPrepare()
>    first.
>  
> -  @param[in,out] Ring        The virtio ring to append the buffer to, as a
> -                             descriptor.
> +  @param[in,out] Ring               The virtio ring to append the buffer to,
> +                                    as a descriptor.
>  
> -  @param[in] BufferPhysAddr  (Guest pseudo-physical) start address of the
> -                             transmit / receive buffer.
> +  @param[in] BufferDeviceAddress    (Bus master device)) start address of the

(2) Unbalanced parentheses

> +                                    transmit / receive buffer.
>  
> -  @param[in] BufferSize      Number of bytes to transmit or receive.
> +  @param[in] BufferSize             Number of bytes to transmit or receive.
>  
> -  @param[in] Flags           A bitmask of VRING_DESC_F_* flags. The caller
> -                             computes this mask dependent on further buffers to
> -                             append and transfer direction.
> -                             VRING_DESC_F_INDIRECT is unsupported. The
> -                             VRING_DESC.Next field is always set, but the host
> -                             only interprets it dependent on VRING_DESC_F_NEXT.
> +  @param[in] Flags                  A bitmask of VRING_DESC_F_* flags. The
> +                                    caller computes this mask dependent on
> +                                    further buffers to append and transfer
> +                                    direction. VRING_DESC_F_INDIRECT is
> +                                    unsupported. The VRING_DESC.Next field is
> +                                    always set, but the host only interprets
> +                                    it dependent on VRING_DESC_F_NEXT.
>  
> -  @param[in,out] Indices     Indices->HeadDescIdx is not accessed.
> -                             On input, Indices->NextDescIdx identifies the next
> -                             descriptor to carry the buffer. On output,
> -                             Indices->NextDescIdx is incremented by one, modulo
> -                             2^16.
> +  @param[in,out] Indices            Indices->HeadDescIdx is not accessed.
> +                                    On input, Indices->NextDescIdx identifies
> +                                    the next descriptor to carry the buffer.
> +                                    On output, Indices->NextDescIdx is
> +                                    incremented by one, modulo 2^16.
>  
>  **/
>  VOID
>  EFIAPI
>  VirtioAppendDesc (
>    IN OUT VRING        *Ring,
> -  IN     UINTN        BufferPhysAddr,
> +  IN     UINT64       BufferDeviceAddress,
>    IN     UINT32       BufferSize,
>    IN     UINT16       Flags,
>    IN OUT DESC_INDICES *Indices
> diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> index e5366e385f5d..fcd484fffada 100644
> --- a/OvmfPkg/Library/VirtioLib/VirtioLib.c
> +++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> @@ -189,7 +189,6 @@ VirtioPrepare (
>    Indices->NextDescIdx = Indices->HeadDescIdx;
>  }
>  
> -
>  /**
>  
>    Append a contiguous buffer for transmission / reception via the virtio ring.
> @@ -205,33 +204,34 @@ VirtioPrepare (
>    The caller is responsible for initializing *Indices with VirtioPrepare()
>    first.
>  
> -  @param[in,out] Ring        The virtio ring to append the buffer to, as a
> -                             descriptor.
> +  @param[in,out] Ring               The virtio ring to append the buffer to,
> +                                    as a descriptor.
>  
> -  @param[in] BufferPhysAddr  (Guest pseudo-physical) start address of the
> -                             transmit / receive buffer.
> +  @param[in] BufferDeviceAddress    (Bus master device)) start address of the

(3) Same as (2), unbalanced parens.


With these fixed up (which I plan to do before pushing the patches):

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

Thanks
Laszlo

> +                                    transmit / receive buffer.
>  
> -  @param[in] BufferSize      Number of bytes to transmit or receive.
> +  @param[in] BufferSize             Number of bytes to transmit or receive.
>  
> -  @param[in] Flags           A bitmask of VRING_DESC_F_* flags. The caller
> -                             computes this mask dependent on further buffers to
> -                             append and transfer direction.
> -                             VRING_DESC_F_INDIRECT is unsupported. The
> -                             VRING_DESC.Next field is always set, but the host
> -                             only interprets it dependent on VRING_DESC_F_NEXT.
> +  @param[in] Flags                  A bitmask of VRING_DESC_F_* flags. The
> +                                    caller computes this mask dependent on
> +                                    further buffers to append and transfer
> +                                    direction. VRING_DESC_F_INDIRECT is
> +                                    unsupported. The VRING_DESC.Next field is
> +                                    always set, but the host only interprets
> +                                    it dependent on VRING_DESC_F_NEXT.
>  
> -  @param[in,out] Indices     Indices->HeadDescIdx is not accessed.
> -                             On input, Indices->NextDescIdx identifies the next
> -                             descriptor to carry the buffer. On output,
> -                             Indices->NextDescIdx is incremented by one, modulo
> -                             2^16.
> +  @param[in,out] Indices            Indices->HeadDescIdx is not accessed.
> +                                    On input, Indices->NextDescIdx identifies
> +                                    the next descriptor to carry the buffer.
> +                                    On output, Indices->NextDescIdx is
> +                                    incremented by one, modulo 2^16.
>  
>  **/
>  VOID
>  EFIAPI
>  VirtioAppendDesc (
>    IN OUT VRING        *Ring,
> -  IN     UINTN        BufferPhysAddr,
> +  IN     UINT64       BufferDeviceAddress,
>    IN     UINT32       BufferSize,
>    IN     UINT16       Flags,
>    IN OUT DESC_INDICES *Indices
> @@ -240,7 +240,7 @@ VirtioAppendDesc (
>    volatile VRING_DESC *Desc;
>  
>    Desc        = &Ring->Desc[Indices->NextDescIdx++ % Ring->QueueSize];
> -  Desc->Addr  = BufferPhysAddr;
> +  Desc->Addr  = BufferDeviceAddress;
>    Desc->Len   = BufferSize;
>    Desc->Flags = Flags;
>    Desc->Next  = Indices->NextDescIdx % Ring->QueueSize;
>