Re: Regression with PXE boot on OvmfPkg/VirtioNetDxe driver on aarch64 system

[aaron.young@oracle.com]

Hello,

  I have some more information on this issue that I wanted to share.

  The SNP client appears to be GRUB - i.e. the ASSERT() is asserted just 
after PXE code has loaded GRUB, presented the GRUB menu and a boot 
option is selected.

  And, I have confirmed that it, as predicted, attempts to call 
SNP->Transmit() twice with the same Buffer - without successfully 
reaping the Buffer via SNP->GetStatus() in between calls - thus tripping 
the ASSERT() in SnpSharedHelpers.c:VirtioNetMapTxBuf().

  This was quite easy to see since GRUB appears to use only a single 
Buffer for which to transmit packets a few bytes at a time. i.e. it just 
calls SNP->Transmit() and then calls SNP->GetStatus() over and over 
again with the same buffer to transmit its packets.

  From my debug, I can see it work successfully most of the time (i.e. 
SNP->Transmit() is called successfully with a corresponding subsequent 
call to SNP->GetStatus() which returns the Buffer). However, 
occasionally, SNP->GetStatus() will return TxBuf=NULL (indicating that 
the Buffer has not yet been transmitted). In this case, GRUB appears to 
attempt to call SNP->GetStatus() *twice* before giving up (possibly due 
to a timer which I can see in the GRUB code) and then attempts to 
transfer the Buffer again - resulting in the ASSERT().

  So, this explains the regression since the old code would allow the 
same Buffer to to be re-transmitted.

  As a quick test, I coded up a quick workaround in VirtioNetMapTxBuf() 
to walk through Dev->TxBufCollection to see if the Buffer is present - 
i.e. has already been transmitted (and not yet reaped via 
SNP->GetStatus()) - and if so, just return EFI_SUCCESS from 
VirtioNetTransmit(). This allowed the PXE boot/installation to succeed. 
I'm not advocating this as a solution, but is an interesting data point.

  Any suggestion where we go from here?

  Is there any way to make the code more tolerant of these 
re-transmissions (like the code was before)?

  I'm not confident that declaring GRUB as not spec compliant is the answer.

  Thanks,

  -Aaron


On 08/31/18 06:17, Laszlo Ersek wrote:
> On 08/30/18 22:17, aaron.young@oracle.com wrote:
>>    We have encountered what we believe to be a regression in the
>> OvmfPkg/VirtioNetDxe driver.
>>
>>    This regression causes PXE boot to fail with the following ASSERT:
>>
>> ASSERT
>> [VirtioNetDxe]/builddir/build/BUILD/ovmf-92d07e48907f/OvmfPkg/VirtioNetDxe/SnpSharedHelpers.c(184):
>> ((BOOLEAN)(0==1))
>>
>>    The failure was encountered on a aarch64/Armv8 system (Ampere server)
>> using the latest mainline (edk2.git/master) code as of 8-28-2018.
>>
>>    On a hunch, I reverted the following patchset which resulted in PXE
>> boot being successful - which suggests this patchset is likely the culprit:
>>
>> https://lists.01.org/pipermail/edk2-devel/2017-September/014679.html
>>
>>    The command used to initiate the VM PXE boot/install is:
>>
>>    virt-install --name guest1  --vcpus 16 --ram 32000 --arch aarch64
>> --disk guest1.img,size=100 --network
>> type=direct,source=enP4p1s0,model=virtio --pxe
>>
>>    NOTE that I also tried simply removing the offending ASSERT() and PXE
>> boot still fails with multiple errors (exceptions of some sort that I
>> didn't log).
>>
>>
>>    What I'd like to know:
>>
>>    1. Is this a known failure?
> Please read the comment in the context of the failed assertion. If this
> assertion is triggered, then that's an error in the code that consumes
> the SNP instance provided by VirtioNetDxe. The client code invokes
> behavior that is undefined by the UEFI spec.
>
> You can find more details at:
>
> http://mid.mail-archive.com/2adc266a-949d-c546-9bed-d78fea19d969@redhat.com
>
>>    2. Was aarch64 given much testing with this new patchset?
> Yes. I described the tests that I had done before pushing the patch set
> here:
>
> http://mid.mail-archive.com/581a568d-b0ce-34a1-8e52-95adf14c03e0@redhat.com
>
> http://mid.mail-archive.com/1e4267dc-0133-e833-f805-8381ef82c52e@redhat.com
>
>
> And most recently (independently of the patch set you refer to), I
> successfully tested VirtioNetDxe on aarch64 as part of PXEv4, PXEv6,
> HTTPv4, HTTPv6 net-boots, for commit ae08ea246fe9
> ("ArmVirtPkg/ArmVirtQemu: enable the IPv6 stack", 2018-07-13):
>
> http://mid.mail-archive.com/20180712234112.3566-1-lersek@redhat.com
>
>>    3. Is reverting this patchset a safe workaround for now until a fix is
>> found? Or is there another simple workaround perhaps?
> Reverting the set should make the symptom go away, but that just papers
> over the undefined behavior in the protocol caller.
>
> The goal of the series was to enable VirtioNetDxe to function correctly
> on top of:
> - a virtio-1.0 PCI transport, provided by Virtio10Dxe, such that
> - explicit DMA mapping is performed using the EFI_PCI_IO_PROTOCOL
>    underlying the virtio transport, such that
> - the platform provides an IOMMU protocol implementation for
>    PciHostBridgeDxe, which underlies PciBusDxe.
>
> If this use case does not apply to your platform, then you can revert
> the patch set.
>
> In particular, in the ArmVirtQemu platform, we don't hook
> "OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf" into
> "PciHostBridgeDxe" via NULL class resolution, thus PciHostBridgeDxe
> there will not wait for another platform driver to decide dynamically
> whether the platform has an actual IOMMU protocol or not. And,
> accordingly, we never produce an IOMMU protocol instance in ArmVirtQemu.
>
> (Obviously the patch set was extensively regression-tested to confirm it
> was a no-op on platforms where the above use case wouldn't apply.)
>
>>    4. Any hunches what the issue could be?
> Code that uses VirtioNetDxe's SNP protocol interface invokes undefined
> behavior; please see my answer to your Q1.
>
>>    Thanks for any info/help/suggestions,
> I'd like to give you a hint on debugging this further as well:
>
> (1) in the VirtioNetTransmit() function, please add DEBUG messages both
> before and after the call to VirtioNetMapTxBuf(). Before, log "Buffer"
> (with %p). After, log "DeviceAddress" (with "%Lx").
>
> (2) In the VirtioNetGetStatus() function, please add DEBUG messages both
> before and after the call to VirtioNetUnmapTxBuf(). Before, log
> "DeviceAddress" (with "%Lx"). After, log "*TxBuf" (with "%p").
>
> In each of (1) and (2), the values printed before/after should be
> identical (= identity-mapping for DMA).
>
> Please write a python script that parses the log generated by (1) and
> (2). The python script should maintain an associative array where the
> key type is "address", and the value type is "integer" (starting value 0).
>
> For each "Buffer" address printed by (1), the python script should
> increase the counter for that address.
>
> For each "*TxBuf" address printed by (2), the python script should
> decrease the counter for that address.
>
> If any counter goes above 1, then the client code invokes undefined
> behavior. If any counter goes under 0, then we messed up something. If
> all counters only take values 0 and 1, but you still hit the assert,
> then again we messed up something.
>
> The idea is to check whether:
>
> - the SNP consumer code tries to Transmit() the same buffer for a second
> time *before* it gets it back from GetStatus() -- this is the error that
> I'm currently suspecting,
>
> - or we messed up something (return a buffer that hasn't been queued for
> transmission, or else we mistake a not-yet-queued buffer for an already
> queued buffer when it is Transmit()ted for the first time).
>
>
> ... BTW, if you build ArmVirtQemu for the RELEASE target, the ASSERT()
> will disappear from VirtioNetMapTxBuf(), and Transmit() will simply
> return EFI_DEVICE_ERROR. This is consistent with your last paragraph --
> i.e., "boot still fails with multiple errors" -- and the reason for
> returning this error is that the SNP consumer really tries to invoke
> undefined behavior. It's not always possible to detect UB, but whenever
> we can, without a huge performance hit, then in DEBUG builds it's
> appropriate to trip an assert, and in RELEASE builds, to return an error.
>
> Thanks
> Laszlo
>
>>    -Aaron Young
>>
> Thanks
> Laszlo