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