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

[Laszlo Ersek <lersek@redhat.com>]

On 08/24/17 02:26, 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.
>
>
>>   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.

For patches v3 1-12:

Regression-tested-by: Laszlo Ersek <lersek@redhat.com>

I've pushed those patches now; commit range c69071bd7e21..0a568ccbcbd1.

The testing took incredibly long. For AARCH64 testing, I used my Mustang
(KVM). For IA32 and X64, I used my laptop (also KVM). This was all
without SEV (hence "regression testing".) Here's the matrix and some
remarks:

                            virtio-mmio  legacy PCI        modern PCI
                            -----------  ----------------- --------------------------
  device  test scenario     AARCH64      IA32     X64      IA32     X64      AARCH64
  ------  ----------------  -----------  -------- -------- -------- -------- --------
  rng     shell RECONNECT   PASS         PASS     PASS     PASS     PASS     PASS

  rng     RngTest.efi       PASS         PASS     PASS     PASS     PASS     PASS

  rng     ExitBootServices  PASS         PASS     PASS     PASS     PASS     PASS

  blk     shell RECONNECT   PASS         PASS     PASS     PASS     PASS     PASS

  blk     shell LS/TYPE     PASS         PASS     PASS     PASS     PASS     PASS

  blk     ExitBootServices  PASS         PASS     PASS     PASS     PASS     PASS

  scsi    shell RECONNECT   PASS         PASS     PASS     PASS     PASS     PASS

  scsi    OS boot           PASS         PASS     PASS     PASS     PASS     PASS

  scsi    ExitBootServices  PASS         PASS     PASS     PASS     PASS     PASS

  net     shell RECONNECT   PASS         PASS     PASS     PASS     PASS     PASS
          (parent, child)

  net     DHCP +            PASS[06]     SKIP[08] SKIP[10] SKIP[08] SKIP[10] PASS[03]
          DataSink.efi

  net     DHCP +            PASS[05]     SKIP[07] PASS[11] SKIP[07] PASS[09] PASS[04]
          DataSource.efi

  net     DHCP + PXE boot   PASS         PASS     PASS     PASS     PASS     PASS

  net     ExitBootServices  PASS         PASS     PASS     PASS     PASS     PASS

  gpu     shell RECONNECT   N/A[01]      N/A[01]  N/A[01]  N/A[02]  N/A[02]  PASS
          (parent, child)

  gpu     shell MODE        N/A[01]      N/A[01]  N/A[01]  N/A[02]  N/A[02]  PASS

  gpu     ExitBootServices  N/A[01]      N/A[01]  N/A[01]  N/A[02]  N/A[02]  PASS

Remarks:

[01] virtio-gpu is modern-only

[02] Libvirt maps the "virtio" video model to "virtio-vga" on IA32/X64,
     which is bound by QemuVideoDxe, and to "virtio-gpu-pci" on AARCH64,
     which is bound by VirtioGpuDxe.

[03] transferred 200 MB, average speed 1.3-1.6 MB/s

[04] transferred 20 MB, average speed 280 KB/s

[05] transferred 20 MB, average speed 96 KB/s

[06] transferred 80 MB, average speed 374 KB/s

[07] 295 KB were transferred, but when DataSource.efi wanted to print
     the first stats line, it crashed with a divide error:

     !!!! IA32 Exception Type - 00(#DE - Divide Error)  CPU Apic ID - 00000000 !!!!
     EIP  - 7CD2077A, CS  - 00000010, EFLAGS - 00010246
     EAX  - 00000000, ECX - 00000000, EDX - 00000000, EBX - 00000000
     ESP  - 7EEBE2EC, EBP - 7EEBE328, ESI - 000004D2, EDI - 00000000
     DS   - 00000008, ES  - 00000008, FS  - 00000008, GS  - 00000008, SS - 00000008
     CR0  - 00000033, CR2 - 00000000, CR3 - 00000000, CR4 - 00000640
     DR0  - 00000000, DR1 - 00000000, DR2 - 00000000, DR3 - 00000000
     DR6  - FFFF0FF0, DR7 - 00000400
     GDTR - 7EE07A90 00000047, IDTR - 7DDB7010 000007FF
     LDTR - 00000000, TR - 00000000
     FXSAVE_STATE - 7EEBE030
     !!!! Find image
     Build/AppPkg/NOOPT_GCC48/IA32/AppPkg/Applications/Sockets/DataSource/DataSource/DEBUG/DataSource.dll
     (ImageBase=000000007CCF3000, EntryPoint=000000007CCF3220) !!!!

     0002d770 <InternalMathDivRemU64x32>:
        2d770:       8b 4c 24 0c             mov    0xc(%esp),%ecx
        2d774:       8b 44 24 08             mov    0x8(%esp),%eax
        2d778:       31 d2                   xor    %edx,%edx
     -> 2d77a:       f7 f1                   div    %ecx
        2d77c:       50                      push   %eax
        2d77d:       8b 44 24 08             mov    0x8(%esp),%eax
        2d781:       f7 f1                   div    %ecx
        2d783:       8b 4c 24 14             mov    0x14(%esp),%ecx
        2d787:       e3 02                   jecxz  2d78b <InternalMathDivRemU64x32.0>
        2d789:       89 11                   mov    %edx,(%ecx)

     The same symptom reproduces without applying Brijesh's patches.

[08] after the connection is established, DataSink.efi crashes

     !!!! IA32 Exception Type - 00(#DE - Divide Error)  CPU Apic ID - 00000000 !!!!
     EIP  - 7CD211CA, CS  - 00000010, EFLAGS - 00010246
     EAX  - 00000000, ECX - 00000000, EDX - 00000000, EBX - 00000000
     ESP  - 7EEBE23C, EBP - 7EEBE278, ESI - 00000002, EDI - 00000000
     DS   - 00000008, ES  - 00000008, FS  - 00000008, GS  - 00000008, SS - 00000008
     CR0  - 00000033, CR2 - 00000000, CR3 - 00000000, CR4 - 00000640
     DR0  - 00000000, DR1 - 00000000, DR2 - 00000000, DR3 - 00000000
     DR6  - FFFF0FF0, DR7 - 00000400
     GDTR - 7EE07A90 00000047, IDTR - 7DDB7010 000007FF
     LDTR - 00000000, TR - 00000000
     FXSAVE_STATE - 7EEBDF80
     !!!! Find image
     Build/AppPkg/NOOPT_GCC48/IA32/AppPkg/Applications/Sockets/DataSink/DataSink/DEBUG/DataSink.dll
     (ImageBase=000000007CCFA000, EntryPoint=000000007CCFA220) !!!!

     000271c0 <InternalMathDivRemU64x32>:
        271c0:       8b 4c 24 0c             mov    0xc(%esp),%ecx
        271c4:       8b 44 24 08             mov    0x8(%esp),%eax
        271c8:       31 d2                   xor    %edx,%edx
     -> 271ca:       f7 f1                   div    %ecx
        271cc:       50                      push   %eax
        271cd:       8b 44 24 08             mov    0x8(%esp),%eax
        271d1:       f7 f1                   div    %ecx
        271d3:       8b 4c 24 14             mov    0x14(%esp),%ecx
        271d7:       e3 02                   jecxz  271db <InternalMathDivRemU64x32.0>
        271d9:       89 11                   mov    %edx,(%ecx)

     The same symptom reproduces without applying Brijesh's patches.

[09] transferred 30MB, average speed 282 KB/s

[10] DataSink.efi hangs. The connection is established (according to
     "netstat"), but no data is transferred. The OVMF log contains
     repeated messages

     TcpInput: sequence acceptance test failed for segment of TCB 7CF52998
     Tcp4Input: Discard a packet

     The same symptom reproduces without applying Brijesh's patches.

[11] transferred 10.2MB, average speed 282 KB/s


The DataSource.efi and DataSink.efi applications seem quite brittle to
me, so I think in the future I might switch to different tools for
testing virtio-net with TCP. HTTP boot looks like a good candidate
<https://github.com/tianocore/tianocore.github.io/wiki/HTTP-Boot>;
perhaps I'll try it out.

Brijesh, can you please proceed with step (7) below?

Thank you,
Laszlo

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