On 08/14/17 13:36, Brijesh Singh wrote:
> The function can be used for mapping the system physical address to virtio
> device address using VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer (). The
> function helps centralizing error handling, and it allows caller to pass
(1) some grammar bikeshedding:
- s/helps centralizing/helps with centralizing/
- s/allows caller/allows the caller/
> in constant or other evaluated expression for NumberOfBytes.
- s/expression/expressions/
>
> 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 | 44 +++++++++++
> OvmfPkg/Library/VirtioLib/VirtioLib.c | 78 ++++++++++++++++++++
> 2 files changed, 122 insertions(+)
>
> diff --git a/OvmfPkg/Include/Library/VirtioLib.h b/OvmfPkg/Include/Library/VirtioLib.h
> index 5badfb32917f..d8f24a5d68b1 100644
> --- a/OvmfPkg/Include/Library/VirtioLib.h
> +++ b/OvmfPkg/Include/Library/VirtioLib.h
> @@ -3,6 +3,7 @@
> Declarations of utility functions used by virtio device drivers.
>
> Copyright (C) 2012-2016, Red Hat, Inc.
> + Copyright (C) 2017, AMD Inc, All rights reserved.<BR>
>
> This program and the accompanying materials are licensed and made available
> under the terms and conditions of the BSD License which accompanies this
> @@ -235,4 +236,47 @@ Virtio10WriteFeatures (
> IN OUT UINT8 *DeviceStatus
> );
>
> +/**
> + Provides the virtio device address required to access system memory from a
> + DMA bus master.
> +
This part is slightly out of synch with the protocol definition itself
(i.e., with VIRTIO_MAP_SHARED): the UEFI spec reference is missing.
However, that's fine: this function diverges slightly from the spec --
that's exactly where the extra convenience comes from. So this is OK.
> + @param[in] This The protocol instance pointer.
> +
(2) This should say instead:
@param[in] VirtIo The virtio device for which the mapping is
requested.
> + @param[in] Operation Indicates if the bus master is going to
> + read or write to system memory.
> +
> + @param[in] HostAddress The system memory address to map to shared
> + buffer address.
> +
> + @param[in,out] NumberOfBytes On input the number of bytes to map.
> + On output the number of bytes that were
> + mapped.
(3) This is wrong. In my v1 review
<http://mid.mail-archive.com/9e743ec8-f2fe-65f2-9594-4731d2481a88@redhat.com>,
point (3), I wrote,
Basically you can copy the MapSharedBuffer() documentation, just
point out the extra logic for NumberOfBytes.
That description is missing -- NumberOfBytes is an IN-only parameter
here, and it is interpreted as a hard requirement.
> +
> + @param[out] DeviceAddress The resulting shared map address for the
> + bus master to access the hosts HostAddress.
> +
> + @param[out] Mapping A resulting taken to pass to
> + VIRTIO_UNMAP_SHARED.
(4) s/taken/token/, as discussed before
> +
> +
> + @retval EFI_SUCCESS The range was mapped for the returned
> + NumberOfBytes.
(5) This too should be adapted to our particular NumberOfBytes
interpretation.
> + @retval EFI_UNSUPPORTED The HostAddress cannot be mapped as a
> + common buffer.
> + @retval EFI_INVALID_PARAMETER One or more parameters are invalid.
> + @retval EFI_OUT_OF_RESOURCES The request could not be completed due to
> + a lack of resources.
(6) Please add, "This includes the case when NumberOfBytes bytes cannot
be mapped in one consecutive range."
> + @retval EFI_DEVICE_ERROR The system hardware could not map the
> + requested address.
> +**/
> +EFI_STATUS
> +EFIAPI
> +VirtioMapAllBytesInSharedBuffer (
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN VIRTIO_MAP_OPERATION Operation,
> + IN VOID *HostAddress,
> + IN UINTN NumberOfBytes,
> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
> + OUT VOID **Mapping
> + );
> #endif // _VIRTIO_LIB_H_
> diff --git a/OvmfPkg/Library/VirtioLib/VirtioLib.c b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> index 845f206369a3..ac0ae97f3692 100644
> --- a/OvmfPkg/Library/VirtioLib/VirtioLib.c
> +++ b/OvmfPkg/Library/VirtioLib/VirtioLib.c
> @@ -4,6 +4,7 @@
>
> Copyright (C) 2012-2016, Red Hat, Inc.
> Portion of Copyright (C) 2013, ARM Ltd.
> + Copyright (C) 2017, AMD Inc, All rights reserved.<BR>
>
> This program and the accompanying materials are licensed and made available
> under the terms and conditions of the BSD License which accompanies this
> @@ -414,3 +415,80 @@ Virtio10WriteFeatures (
>
> return Status;
> }
> +
> +/**
> + Provides the virtio device address required to access system memory from a
> + DMA bus master.
> +
> + @param[in] This The protocol instance pointer.
> +
> + @param[in] Operation Indicates if the bus master is going to
> + read or write to system memory.
> +
> + @param[in] HostAddress The system memory address to map to shared
> + buffer address.
> +
> + @param[in,out] NumberOfBytes On input the number of bytes to map.
> + On output the number of bytes that were
> + mapped.
> +
> + @param[out] DeviceAddress The resulting shared map address for the
> + bus master to access the hosts HostAddress.
> +
> + @param[out] Mapping A resulting taken to pass to
> + VIRTIO_UNMAP_SHARED.
> +
> +
> + @retval EFI_SUCCESS The range was mapped for the returned
> + NumberOfBytes.
> + @retval EFI_UNSUPPORTED The HostAddress cannot be mapped as a
> + common buffer.
> + @retval EFI_INVALID_PARAMETER One or more parameters are invalid.
> + @retval EFI_OUT_OF_RESOURCES The request could not be completed due to
> + a lack of resources.
> + @retval EFI_DEVICE_ERROR The system hardware could not map the
> + requested address.
> +**/
(7) Please sync this with points (2) through (6) above.
> +EFI_STATUS
> +EFIAPI
> +VirtioMapAllBytesInSharedBuffer (
> + IN VIRTIO_DEVICE_PROTOCOL *VirtIo,
> + IN VIRTIO_MAP_OPERATION Operation,
> + IN VOID *HostAddress,
> + IN UINTN NumberOfBytes,
> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress,
> + OUT VOID **Mapping
> + )
> +{
> + EFI_STATUS Status;
> + VOID *MapInfo;
> + UINTN Size;
> + EFI_PHYSICAL_ADDRESS PhysicalAddress;
> +
> + Size = NumberOfBytes;
> + Status = VirtIo->MapSharedBuffer (
> + VirtIo,
> + Operation,
> + HostAddress,
> + &Size,
> + &PhysicalAddress,
> + &MapInfo
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + if (Size < NumberOfBytes) {
> + goto Failed;
> + }
> +
> + *Mapping = MapInfo;
> + *DeviceAddress = PhysicalAddress;
> +
> + return EFI_SUCCESS;
> +
> +Failed:
> + VirtIo->UnmapSharedBuffer (VirtIo, MapInfo);
> + *Mapping = NULL;
> + return EFI_OUT_OF_RESOURCES;
> +}
>
(8) The logic differs slightly from the previous version: in this
version you set Mapping to NULL on error.
I liked the previous version more; IMO we should not touch output
parameters on error (that's why you introduced MapInfo earlier, to begin
with).
So I suggest dropping the NULL-ing on error.
Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel