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

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

[aaron.young]

   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?
   2. Was aarch64 given much testing with this new patchset?
   3. Is reverting this patchset a safe workaround for now until a fix 
is found? Or is there another simple workaround perhaps?

   4. Any hunches what the issue could be?


   Thanks for any info/help/suggestions,

   -Aaron Young

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

[Laszlo Ersek]

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

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

[aaron.young]

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

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

[Ard Biesheuvel]

(+ Leif)

On 3 October 2018 at 18:56,  <aaron.young@oracle.com> wrote:
>
> 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
>
>

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

[aaron.young]

  I am suspecting that this patch to GRUB is the cause of a Buffer being 
re-transmitted before reaping the Buffer via SNP->GetStatus():

https://git.centos.org/blob/rpms!grub2.git/1065bd29e776aef83f927747882140dcb6fd5cde/SOURCES!0183-efinet-retransmit-if-our-device-is-busy.patch

  So, to reproduce the issue, the GRUB used via PXE boot needs to 
include this patch.

  Thanks,

  -Aaron


On 10/03/18 10:35, Ard Biesheuvel wrote:
> (+ Leif)
>
> On 3 October 2018 at 18:56,  <aaron.young@oracle.com> wrote:
>> 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
>>

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

[Leif Lindholm]

+Peter

On Wed, Oct 03, 2018 at 04:59:54PM -0700, aaron.young@oracle.com wrote:
>  I am suspecting that this patch to GRUB is the cause of a Buffer being
> re-transmitted before reaping the Buffer via SNP->GetStatus():
> 
> https://git.centos.org/blob/rpms!grub2.git/1065bd29e776aef83f927747882140dcb6fd5cde/SOURCES!0183-efinet-retransmit-if-our-device-is-busy.patch
> 
>  So, to reproduce the issue, the GRUB used via PXE boot needs to include
> this patch.

So the issue cannot be reproduced with upstream GRUB?

Does Fedora/Red Hat include the same patch?

/
    Leif

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

[Laszlo Ersek]

On 10/04/18 11:24, Leif Lindholm wrote:
> +Peter
> 
> On Wed, Oct 03, 2018 at 04:59:54PM -0700, aaron.young@oracle.com wrote:
>>  I am suspecting that this patch to GRUB is the cause of a Buffer being
>> re-transmitted before reaping the Buffer via SNP->GetStatus():
>>
>> https://git.centos.org/blob/rpms!grub2.git/1065bd29e776aef83f927747882140dcb6fd5cde/SOURCES!0183-efinet-retransmit-if-our-device-is-busy.patch
>>
>>  So, to reproduce the issue, the GRUB used via PXE boot needs to include
>> this patch.
> 
> So the issue cannot be reproduced with upstream GRUB?
> 
> Does Fedora/Red Hat include the same patch?

Here's what I can see.

(1) In upstream grub <https://git.savannah.gnu.org/git/grub.git>, at
commit 8ada906031d9 ("msdos: Fix overflow in converting partition start
and length into 512B blocks", 2018-09-27), on the master branch, the
patch is not present.

(2) In "rhboot" grub2 <https://github.com/rhboot/grub2>, where the
master branch seems to track upstream grub, the patch is present on at
least the "fedora-28" and "rhel-7.5" branches. Commit hashes,
respectively: c2b126f52143, 1b9767c136082.

(3) In the commit message, Josef wrote, "When I fixed the txbuf handling
I ripped out the retransmission code". I think he referred to his
earlier commit 4fe8e6d4a127 ("efinet: handle get_status() on buggy
firmware properly", 2015-08-09). That commit is upstream.

In my opinion, commit 4fe8e6d4a127, the chronologically first, and
upstream, tweak, was right (assuming the comment it added was true,
about grub).

On the other hand, the downstream-only (chronologically 2nd) commit was
wrong. Not only did it break the spec, it broke even grub's own internal
invariant, described in the comment that was added in upstream commit
4fe8e6d4a127. The comment states, "we only transmit one packet at a
time". With the downstream-only tweak applied, that's no longer true.
Namely, SNP.Transmit() is called while we know another transmission is
pending on the egress queue. That's the definition of sending more than
one packet at a time.

I'm curious why the patch in question is not upstream. Was it submitted
and rejected? Submitted and ignored? Not submitted at all?

I'm not a fan of the hard-liner "spec above everything" approach. In
this case though, after the downstream-only tweak, grub is inconsistent
not only with the spec, but with itself too.

IMO, downstream(s) should revert the downstream-only patch.

Thanks,
Laszlo

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

[aaron.young]

On 10/04/18 06:06, Laszlo Ersek wrote:
> On 10/04/18 11:24, Leif Lindholm wrote:
>> +Peter
>>
>> On Wed, Oct 03, 2018 at 04:59:54PM -0700, aaron.young@oracle.com wrote:
>>>   I am suspecting that this patch to GRUB is the cause of a Buffer being
>>> re-transmitted before reaping the Buffer via SNP->GetStatus():
>>>
>>> https://git.centos.org/blob/rpms!grub2.git/1065bd29e776aef83f927747882140dcb6fd5cde/SOURCES!0183-efinet-retransmit-if-our-device-is-busy.patch
>>>
>>>   So, to reproduce the issue, the GRUB used via PXE boot needs to include
>>> this patch.
>> So the issue cannot be reproduced with upstream GRUB?
>>
>> Does Fedora/Red Hat include the same patch?
> Here's what I can see.
>
> (1) In upstream grub <https://git.savannah.gnu.org/git/grub.git>, at
> commit 8ada906031d9 ("msdos: Fix overflow in converting partition start
> and length into 512B blocks", 2018-09-27), on the master branch, the
> patch is not present.
>
> (2) In "rhboot" grub2 <https://github.com/rhboot/grub2>, where the
> master branch seems to track upstream grub, the patch is present on at
> least the "fedora-28" and "rhel-7.5" branches. Commit hashes,
> respectively: c2b126f52143, 1b9767c136082.
>
> (3) In the commit message, Josef wrote, "When I fixed the txbuf handling
> I ripped out the retransmission code". I think he referred to his
> earlier commit 4fe8e6d4a127 ("efinet: handle get_status() on buggy
> firmware properly", 2015-08-09). That commit is upstream.
>
> In my opinion, commit 4fe8e6d4a127, the chronologically first, and
> upstream, tweak, was right (assuming the comment it added was true,
> about grub).
>
> On the other hand, the downstream-only (chronologically 2nd) commit was
> wrong. Not only did it break the spec, it broke even grub's own internal
> invariant, described in the comment that was added in upstream commit
> 4fe8e6d4a127. The comment states, "we only transmit one packet at a
> time". With the downstream-only tweak applied, that's no longer true.
> Namely, SNP.Transmit() is called while we know another transmission is
> pending on the egress queue. That's the definition of sending more than
> one packet at a time.
>
> I'm curious why the patch in question is not upstream. Was it submitted
> and rejected? Submitted and ignored? Not submitted at all?
>
> I'm not a fan of the hard-liner "spec above everything" approach. In
> this case though, after the downstream-only tweak, grub is inconsistent
> not only with the spec, but with itself too.
>
> IMO, downstream(s) should revert the downstream-only patch.
>
> Thanks,
> Laszlo

I have confirmed that reverting this GRUB patch indeed fixes the issue 
(i.e. with the VirtioNetDxe/SnpSharedHelpers.c(184) ASSERT).

   Thanks for the help/info in resolving this issue.

   As a follow up question - it seems that the VirtioNetDxe driver is 
fragile in that it can get into broken state if (for whatever reason) a 
tx buffer is not successfully transmitted and thus never shows back up 
on the Used Ring. i.e. If a SNP client has repeatedly called 
SNP-GetStatus() and, after a certain amount of time, fails gets back the 
buffer, what should it do? If it attempts to re-transmit the buffer, 
it'll hit the ASSERT. Perhaps it should shutdown/re-init the interface 
in this case (to free up the buffer mapping in Dev->TxBufCollection)? 
Or, are we confident this condition can _never_ happen?

   Thanks again,

   -Aaron

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

[Ard Biesheuvel]

On 15 October 2018 at 22:52,  <aaron.young@oracle.com> wrote:
>
>
> On 10/04/18 06:06, Laszlo Ersek wrote:
>>
>> On 10/04/18 11:24, Leif Lindholm wrote:
>>>
>>> +Peter
>>>
>>> On Wed, Oct 03, 2018 at 04:59:54PM -0700, aaron.young@oracle.com wrote:
>>>>
>>>>   I am suspecting that this patch to GRUB is the cause of a Buffer being
>>>> re-transmitted before reaping the Buffer via SNP->GetStatus():
>>>>
>>>>
>>>> https://git.centos.org/blob/rpms!grub2.git/1065bd29e776aef83f927747882140dcb6fd5cde/SOURCES!0183-efinet-retransmit-if-our-device-is-busy.patch
>>>>
>>>>   So, to reproduce the issue, the GRUB used via PXE boot needs to
>>>> include
>>>> this patch.
>>>
>>> So the issue cannot be reproduced with upstream GRUB?
>>>
>>> Does Fedora/Red Hat include the same patch?
>>
>> Here's what I can see.
>>
>> (1) In upstream grub <https://git.savannah.gnu.org/git/grub.git>, at
>> commit 8ada906031d9 ("msdos: Fix overflow in converting partition start
>> and length into 512B blocks", 2018-09-27), on the master branch, the
>> patch is not present.
>>
>> (2) In "rhboot" grub2 <https://github.com/rhboot/grub2>, where the
>> master branch seems to track upstream grub, the patch is present on at
>> least the "fedora-28" and "rhel-7.5" branches. Commit hashes,
>> respectively: c2b126f52143, 1b9767c136082.
>>
>> (3) In the commit message, Josef wrote, "When I fixed the txbuf handling
>> I ripped out the retransmission code". I think he referred to his
>> earlier commit 4fe8e6d4a127 ("efinet: handle get_status() on buggy
>> firmware properly", 2015-08-09). That commit is upstream.
>>
>> In my opinion, commit 4fe8e6d4a127, the chronologically first, and
>> upstream, tweak, was right (assuming the comment it added was true,
>> about grub).
>>
>> On the other hand, the downstream-only (chronologically 2nd) commit was
>> wrong. Not only did it break the spec, it broke even grub's own internal
>> invariant, described in the comment that was added in upstream commit
>> 4fe8e6d4a127. The comment states, "we only transmit one packet at a
>> time". With the downstream-only tweak applied, that's no longer true.
>> Namely, SNP.Transmit() is called while we know another transmission is
>> pending on the egress queue. That's the definition of sending more than
>> one packet at a time.
>>
>> I'm curious why the patch in question is not upstream. Was it submitted
>> and rejected? Submitted and ignored? Not submitted at all?
>>
>> I'm not a fan of the hard-liner "spec above everything" approach. In
>> this case though, after the downstream-only tweak, grub is inconsistent
>> not only with the spec, but with itself too.
>>
>> IMO, downstream(s) should revert the downstream-only patch.
>>
>> Thanks,
>> Laszlo
>
>
> I have confirmed that reverting this GRUB patch indeed fixes the issue (i.e.
> with the VirtioNetDxe/SnpSharedHelpers.c(184) ASSERT).
>
>   Thanks for the help/info in resolving this issue.
>
>   As a follow up question - it seems that the VirtioNetDxe driver is fragile
> in that it can get into broken state if (for whatever reason) a tx buffer is
> not successfully transmitted and thus never shows back up on the Used Ring.
> i.e. If a SNP client has repeatedly called SNP-GetStatus() and, after a
> certain amount of time, fails gets back the buffer, what should it do? If it
> attempts to re-transmit the buffer, it'll hit the ASSERT. Perhaps it should
> shutdown/re-init the interface in this case (to free up the buffer mapping
> in Dev->TxBufCollection)? Or, are we confident this condition can _never_
> happen?
>

It is an implementation detail of GRUB that it only ever uses a single
TX buffer: the protocol supports an arbitrary number of buffers, and
repeated calls to GetStatus() will return each one once the network
stack is done with them.

So the answer is simply to allocate another buffer, and use that.

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

[Laszlo Ersek]

Hi Aaron,

On 10/16/18 03:55, Ard Biesheuvel wrote:
> On 15 October 2018 at 22:52,  <aaron.young@oracle.com> wrote:
>> On 10/04/18 06:06, Laszlo Ersek wrote:
>>> On 10/04/18 11:24, Leif Lindholm wrote:
>>>>
>>>> +Peter
>>>>
>>>> On Wed, Oct 03, 2018 at 04:59:54PM -0700, aaron.young@oracle.com wrote:
>>>>>
>>>>>   I am suspecting that this patch to GRUB is the cause of a Buffer being
>>>>> re-transmitted before reaping the Buffer via SNP->GetStatus():
>>>>>
>>>>>
>>>>> https://git.centos.org/blob/rpms!grub2.git/1065bd29e776aef83f927747882140dcb6fd5cde/SOURCES!0183-efinet-retransmit-if-our-device-is-busy.patch
>>>>>
>>>>>   So, to reproduce the issue, the GRUB used via PXE boot needs to
>>>>> include
>>>>> this patch.
>>>>
>>>> So the issue cannot be reproduced with upstream GRUB?
>>>>
>>>> Does Fedora/Red Hat include the same patch?
>>>
>>> Here's what I can see.
>>>
>>> (1) In upstream grub <https://git.savannah.gnu.org/git/grub.git>, at
>>> commit 8ada906031d9 ("msdos: Fix overflow in converting partition start
>>> and length into 512B blocks", 2018-09-27), on the master branch, the
>>> patch is not present.
>>>
>>> (2) In "rhboot" grub2 <https://github.com/rhboot/grub2>, where the
>>> master branch seems to track upstream grub, the patch is present on at
>>> least the "fedora-28" and "rhel-7.5" branches. Commit hashes,
>>> respectively: c2b126f52143, 1b9767c136082.
>>>
>>> (3) In the commit message, Josef wrote, "When I fixed the txbuf handling
>>> I ripped out the retransmission code". I think he referred to his
>>> earlier commit 4fe8e6d4a127 ("efinet: handle get_status() on buggy
>>> firmware properly", 2015-08-09). That commit is upstream.
>>>
>>> In my opinion, commit 4fe8e6d4a127, the chronologically first, and
>>> upstream, tweak, was right (assuming the comment it added was true,
>>> about grub).
>>>
>>> On the other hand, the downstream-only (chronologically 2nd) commit was
>>> wrong. Not only did it break the spec, it broke even grub's own internal
>>> invariant, described in the comment that was added in upstream commit
>>> 4fe8e6d4a127. The comment states, "we only transmit one packet at a
>>> time". With the downstream-only tweak applied, that's no longer true.
>>> Namely, SNP.Transmit() is called while we know another transmission is
>>> pending on the egress queue. That's the definition of sending more than
>>> one packet at a time.
>>>
>>> I'm curious why the patch in question is not upstream. Was it submitted
>>> and rejected? Submitted and ignored? Not submitted at all?
>>>
>>> I'm not a fan of the hard-liner "spec above everything" approach. In
>>> this case though, after the downstream-only tweak, grub is inconsistent
>>> not only with the spec, but with itself too.
>>>
>>> IMO, downstream(s) should revert the downstream-only patch.
>>>
>>> Thanks,
>>> Laszlo
>>
>>
>> I have confirmed that reverting this GRUB patch indeed fixes the issue (i.e.
>> with the VirtioNetDxe/SnpSharedHelpers.c(184) ASSERT).
>>
>>   Thanks for the help/info in resolving this issue.
>>
>>   As a follow up question - it seems that the VirtioNetDxe driver is fragile
>> in that it can get into broken state if (for whatever reason) a tx buffer is
>> not successfully transmitted and thus never shows back up on the Used Ring.
>> i.e. If a SNP client has repeatedly called SNP-GetStatus() and, after a
>> certain amount of time, fails gets back the buffer, what should it do? If it
>> attempts to re-transmit the buffer, it'll hit the ASSERT. Perhaps it should
>> shutdown/re-init the interface in this case (to free up the buffer mapping
>> in Dev->TxBufCollection)? Or, are we confident this condition can _never_
>> happen?
>>
> 
> It is an implementation detail of GRUB that it only ever uses a single
> TX buffer: the protocol supports an arbitrary number of buffers, and
> repeated calls to GetStatus() will return each one once the network
> stack is done with them.
> 
> So the answer is simply to allocate another buffer, and use that.

I agree with Ard's answer.

I'd like to add:

* If the virtio network device (= the hypervisor) *never* puts the head
of a specific descriptor chain back on the Used Ring (in the TX queue),
then either that is a bug in the virtio network device implementation,
or the packet is genuinely waiting for transmission.

In the former case (= problematic virtio device impl), I think whatever
workaround we came up with would only beget more problems down the road.
(In the present case too, the grub story originally started with a
grub-side workaround for the broken layer underneath, namely an SNP
driver returning bogus buffer addresses.)

In the latter case (= packet still pending transmission), the client
should simply wait. If it decides not to wait any longer, that's fine,
but it's no excuse for reusing the exact same buffer (see Ard's comment).

* Alternatively, I agree that "power cycling" the SNP instance is a
valid thing to do, using EFI_SIMPLE_NETWORK.Shutdown(), and then
EFI_SIMPLE_NETWORK.Initialize(). I'm stating this based on both the
spec, and on the VirtioNetDxe implementation.

In particular, VirtioNetShutdown() starts with a virtio reset, which
will de-configure the virtio device (make it forget all guest memory
references, for example). Then VirtioNetShutdown() calls
VirtioNetShutdownTx() too, which tears down Dev->TxBufCollection, as you
say.

Thanks,
Laszlo