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 <ler...@redhat.com> >> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> >> Cc: Jordan Justen <jordan.l.jus...@intel.com> >> Cc: Tom Lendacky <thomas.lenda...@amd.com> >> Cc: Laszlo Ersek <ler...@redhat.com> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Brijesh Singh <brijesh.si...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel