Re: [PATCH v2 13/23] OvmfPkg/Virtio: take RingBaseShift in VirtioSetQueueAddress()

[Laszlo Ersek <lersek@redhat.com>]

On 08/16/17 23:43, Laszlo Ersek wrote:
> (1) The "VirtioSetQueueAddress" identifier does not exist in the edk2
> tree as a function or protocol member name, so I suggest at least
> replacing "VirtioSetQueueAddress" with "SetQueueAddress".
> 
> On 08/14/17 13:36, Brijesh Singh wrote:
>> VirtioRingMap() maps the ring buffer system physical to a bus address.
>> When an IOMMU is used for translating the address then bus address can
>> start at a different offset from the system physical address.
> 
> (2) I suggest the following wording in order to keep our references
> within the virtio device protocol:
> 
>     For the case when an IOMMU is used for translating system physical
>     addresses to DMA bus master addresses, the transport-independent
>     virtio device drivers will be required to map their VRING areas to
>     bus addresses with VIRTIO_DEVICE_PROTOCOL.MapSharedBuffer() calls.
> 
>> - MMIO and legacy virtio device does not use IOMMU to translate the
>>   addresses hence RingBaseShift will always be set to zero.
> 
> (3) s/device does not use/transports do not support/
> 
>>
>> - modern virtio device use IOMMU to translate the address, in next patch
> 
> (4) s/device use/transport supports/
> 
>>   we will update the Virtio10Dxe to use RingBaseShift offset.
>>
>> 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/Protocol/VirtioDevice.h                         | 5 ++++-
>>  OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h          | 3 ++-
>>  OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h                    | 3 ++-
>>  OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c | 5 ++++-
>>  OvmfPkg/Virtio10Dxe/Virtio10.c                                  | 5 ++++-
>>  OvmfPkg/VirtioBlkDxe/VirtioBlk.c                                | 2 +-
>>  OvmfPkg/VirtioGpuDxe/Commands.c                                 | 3 ++-
>>  OvmfPkg/VirtioNetDxe/SnpInitialize.c                            | 2 +-
>>  OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c                 | 5 ++++-
>>  OvmfPkg/VirtioRngDxe/VirtioRng.c                                | 2 +-
>>  OvmfPkg/VirtioScsiDxe/VirtioScsi.c                              | 2 +-
>>  11 files changed, 26 insertions(+), 11 deletions(-)
>>
>> diff --git a/OvmfPkg/Include/Protocol/VirtioDevice.h b/OvmfPkg/Include/Protocol/VirtioDevice.h
>> index 14f980d7bf0a..25fd73b847a5 100644
>> --- a/OvmfPkg/Include/Protocol/VirtioDevice.h
>> +++ b/OvmfPkg/Include/Protocol/VirtioDevice.h
>> @@ -156,6 +156,8 @@ EFI_STATUS
>>    @param[in] Ring             The initialized VRING object to take the
>>                                addresses from.
>>
>> +  @param[in] RingBaseShift    The offset for the Ring Base address.
>> +
> 
> OK, so this comment block is critical.
> 
> (5) Please update the documentation of the "Ring" parameter like this:
> 
>   @param[in] Ring             The initialized VRING object to take the
>                               addresses from. The caller is responsible for
>                               ensuring that on input, all Ring->NumPages pages,
>                               starting at Ring->Base, have been successfully
>                               mapped with a single call to
>                               This->MapSharedBuffer().

(5a) oops, small (but important) omission: please append "for
CommonBuffer bus master operation".

Thanks
Laszlo