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

[Brijesh Singh <brijesh.singh@amd.com>]

Hi Laszlo,


On 8/23/17 7:26 PM, Laszlo Ersek wrote:
> 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.

Thanks

>
>>   OvmfPkg/VirtioRngDxe: map host address to device address
> (2) I'll do the same for the VirtioRngDxe driver.
>

Thanks

> (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).

I agree with you. Sounds like a good plan.
>
>>   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.
Great thanks, I agree the series is getting bigger. After v1 feedbacks,
I was almost tempted to say that lets work to enable the base first then
will do one driver at a time. I was repeating the same mistake in each
patch and that was causing trouble to you and also I had to rework the
all drivers.

> 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

Okay, I will look into it - thanks for the tip. I wanted to actually use
the Simple array (because we know the maximum number of buffer we can
queue) but was  not sure about your preferences hence I went to with
list. If you are okay then I can use array's too.
>
>>   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*.

Sure will do. thanks
>
>>   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.
>

Will do.

>> 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.

thank you.

-Brijesh