Re: [PATCH v3 00/21] OvmfPkg/Virtio: introduce IOMMU-like member functions

[Laszlo Ersek <lersek@redhat.com>]

Hi Brijesh,

so here's what I'd like to do with v3:

On 08/23/17 14:22, Brijesh Singh wrote:
> Brijesh Singh (23):
>   OvmfPkg: introduce IOMMU-like member functions to
>     VIRTIO_DEVICE_PROTOCOL
>   OvmfPkg/Virtio10Dxe: implement IOMMU-like member functions
>   OvmfPkg/VirtioPciDeviceDxe: implement IOMMU-like member functions
>   OvmfPkg/VirtioMmioDeviceLib: implement IOMMU-like member functions
>   OvmfPkg/VirtioLib: add VirtioMapAllBytesInSharedBuffer() helper
>     function
>   OvmfPkg/VirtioLib: take VirtIo instance in
>     VirtioRingInit/VirtioRingUninit
>   OvmfPkg/Virtio: take RingBaseShift in SetQueueAddress()
>   OvmfPkg/Virtio10Dxe: add the RingBaseShift offset
>   OvmfPkg/VirtioLib: add function to map VRING
>   OvmfPkg/VirtioLib: alloc VRING buffer with AllocateSharedPages()
>   OvmfPkg/VirtioLib: change the parameter of VirtioAppendDesc() to
>     UINT64

(1) I'll take the first 11 patches (which work on the transports and
VirtioLib), fix up the trivial stuff I've found in the v3 review, and
add my R-b tags.


>   OvmfPkg/VirtioRngDxe: map host address to device address

(2) I'll do the same for the VirtioRngDxe driver.


(3) I'll test this initial sequence in various scenarios. I think that
the protocol / transport / VirtioLib changes are good at this point, and
the simplest driver (VirtioRngDxe) demonstrates how to put those new
features to use. It also enables regression testing.

Importantly, I also plan to regression-test the remaining (not yet
converted) drivers at this point. Those drivers are affected only by the
"alloc VRING buffer with AllocateSharedPages" patch, as follows:

- On legacy virtio-pci and virtio-mmio transports, the change is a
no-op, because the AllocateSharedPages() and FreeSharedPages() VirtIo
members are backed by MemoryAllocationLib's AllocatePages() /
FreePages(). So the behavior of VirtioRingInit() / VirtioRingUninit()
remains identical.

- On the virtio-1.0 transport, the direct AllocatePages() call in
VirtioRingInit() will be replaced with VirtIo.AllocateSharedPages ->
PciIo.AllocateBuffer -> PciRootBridgeIo.AllocateBuffer.

- The last function will either branch to gBS->AllocatePages -- if
there's no IoMmu protocol, i.e. no SEV -- which is identical to current
behavior, or branch to IoMmu.AllocateBuffer.

- in IoMmuAllocateBuffer(), we'll allocate StashBuffer on the side
(which is no problem), and the actual allocation (for HostAddress) will
be done with gBS->AllocatePages().

The end result is that at this point, the unconverted drivers won't yet
work on SEV, but they will continue working if SEV is absent. The only
difference is (dependent on transport) longer call chains to memory
allocation and freeing, and larger memory footprint. But, no regressions
in functionality.

If I'm satisfied with the above test results, I'll add my
Regression-tested-by tags as well, and push the first 12 patches.

This will provide a foundation for further driver work (incl. my
VirtioGpuDxe work).


>   OvmfPkg/VirtioBlkDxe: map host address to device address
>   OvmfPkg/VirtioScsiDxe: map host address to device address

(4) I've looked at these patches briefly. They are possibly fine now,
but they've grown way too big. I struggled with the verification of the
VirtioRngDxe driver patch, and each of these two is more than twice as big.

So please, split *each* of these two patches in two parts (a logical
build-up):
- the first part should map and unmap the vring (all relevant contexts),
- the second part should map the requests.


(5) (I think this is my most important point), with the foundation in
place, I suggest we switch to one series per driver. For each driver, I
have to look at the virtio spec, and "page-in" how the requests are
structured, what they do etc. It's not a mechanical process. All that
virtio stuff is easier to re-digest if we proceed device by device.

Should we need later tweaks for the foundation, then at this point I
prefer small incremental patches for that.


>   OvmfPkg/VirtioNetDxe: alloc Tx and Rx rings using AllocateSharedPage()
>   OvmfPkg/VirtioNetDxe: alloc RxBuf using AllocateSharedPages()
>   OvmfPkg/VirtioNetDxe: dynamically alloc transmit header
>   OvmfPkg/VirtioNetDxe: map transmit buffer host address to device
>     address

(6) This is obviously the most complex driver. I've only snuck a peek. I
have one comment at this point: *if* we have to do random lookups, then
lists aren't optimal.

Please consider using the following library class:

  MdePkg/Include/Library/OrderedCollectionLib.h

It is already resolved in the OVMF DSC files to the following instance:

  MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/

Examples for use:
- various modules in OvmfPkg,
- AppPkg/Applications/OrderedCollectionTest


>   OvmfPkg/Virtio10: define VIRITO_F_IOMMU_PLATFORM feature bit
>   OvmfPkg/VirtioRngDxe: negotiate VIRITO_F_IOMMU_PLATFORM

(7) I would have liked to include these two patches in my "initial
push", but minimally the second patch needs fixes from you.

After I'm done with point (3), please repost these patches *only*.


>   OvmfPkg/VirtioBlkDxe: negotiate VIRITO_F_IOMMU_PLATFORM
>   OvmfPkg/VirtioScsiDxe: negotiate VIRITO_F_IOMMU_PLATFORM
>   OvmfPkg/VirtioNetDxe: negotiate VIRITO_F_IOMMU_PLATFORM

(8) After these patches are fixed up, I suggest that you please post
each one of them at the end of the matching driver series.


> TODO:
>  * add VirtioGpuDxe (i will take Laszlo's offer that he can help with
>    this driver)

OK, will do, thank you!

In this work I'll also seek to follow the series layout proposed above.

>  * I did minimal test on aarch64 - I was running into some Linux
>    bootup issues with Fedora aarch64 iso. The issue does not appear to
>    be releated to virtio changes. If anyone can help doing additional
>    test with their aarch images that will be great ! thanks

I'll test on aarch64 too.

Thanks!
Laszlo