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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
