[PATCH] efi_loader: Set EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT again in efi_net_receive()

[Masami Hiramatsu]

Hi Heinrich,

2021年9月15日(水) 10:31 Masami Hiramatsu <masami.hiramatsu at linaro.org>:
>
> Hi Heinrich,
>
> This is obscure in the specification (I can not see any detailed
> description about EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT).

OK, I checked the UEFI specification v2.9 again.
The main purpose of EFI_SIMPLE_NETWORK.GetStatus() seems to update the
EFI_SIMPLE_NETWORK_MODE::MediaPresent, in other words, it checks link
status.
And InterruptStatus of EFI_SIMPLE_NETWORK.GetStatus() means "the
currently active interrupts", not "packet to be received".
Thus, I think it should be tested for checking the link status instead
of checking receive packets in DHCP.
You also can see the EFI_SIMPLE_NETWORK_PROTOCOL::WaitForPacket
ensures that you can receive the packet (The spec says "Event used
with EFI_BOOT_SERVICES.WaitForEvent() to wait for a packet to be
received." )
So, it is enough to use the WaitForPacket in the packet receiving
loop, no need to use GetStatus(). Then, correct test should be
something like this.

----
net->get_status();
if (!net->mode.MediaPresent) {
   error(no link up!)
   return;
}

submit_dhcp_discover()
for (;;) {
   wait_for_event(net)
   while (net->receive() != EFI_NOT_READY) {
      // check dhcp reply
   }
}
----

For your information, as far as I can see, the interrupt bit is
cleared or not depends on the platform driver implementation in EDK2
too.
In many cases (e.g. virtio-net), they do not clear the original bits,
in another case, no interrupt bit supported (IntrruptStatus always be
0).
But I couldn't find the code which really cleared the interrupt bit...
So I think there is a difference between UEFI specification and
reference implementation (EDK2).

Thank you,


>
> Thank you,
>
> 2021年9月15日(水) 0:45 Heinrich Schuchardt <xypron.glpk at gmx.de>:
>
> >
> > On 9/14/21 4:06 AM, Masami Hiramatsu wrote:
> > > From: Kazuhiko Sakamoto <sakamoto.kazuhiko at socionext.com>
> > >
> > > Since 'this->int_status' is cleared by efi_net_get_status(), if user
> >
> > Is it correct to clear int_status unconditionally in efi_net_get_status()?
> >
> > Shouldn't two consecutive calls to EFI_SIMPLE_NETWORK.GetStatus() return
> > the same result?
> >
> > Best regards
> >
> > Heinrich
> >
> > > does wait_for_event(wait_for_packet) and efi_net_get_status() loop
> > > and there are several received packets on the buffer, the second
> > > efi_net_get_status() does not return EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT
> > > even if the 'wait_for_packet' is ready.
> > >
> > > This happens on "efiboot selftest" with noisy network.
> > > (The network device can receive both of other packets and dhcp discover
> > >   packet in a short time.)
> > >
> > > Set EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT again when we found any
> > > packet still on the receive buffer.
> > >
> > > Signed-off-by: Kazuhiko Sakamoto <sakamoto.kazuhiko at socionext.com>
> > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu at linaro.org>
> > > ---
> > >   lib/efi_loader/efi_net.c |   13 +++++++++++--
> > >   1 file changed, 11 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/efi_loader/efi_net.c b/lib/efi_loader/efi_net.c
> > > index 69276b275d..9ca1e0c354 100644
> > > --- a/lib/efi_loader/efi_net.c
> > > +++ b/lib/efi_loader/efi_net.c
> > > @@ -640,10 +640,19 @@ static efi_status_t EFIAPI efi_net_receive
> > >       *buffer_size = receive_lengths[rx_packet_idx];
> > >       rx_packet_idx = (rx_packet_idx + 1) % ETH_PACKETS_BATCH_RECV;
> > >       rx_packet_num--;
> > > -     if (rx_packet_num)
> > > +     if (rx_packet_num) {
> > > +             /*
> > > +              * Since 'this->int_status' is cleared by efi_net_get_status(),
> > > +              * if user does wait_for_event(wait_for_packet) and
> > > +              * efi_net_get_status() again, EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT
> > > +              * is not set even if 'wait_for_packet' is ready.
> > > +              * Set EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT again here.
> > > +              */
> > > +             this->int_status |= EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
> > >               wait_for_packet->is_signaled = true;
> > > -     else
> > > +     } else {
> > >               this->int_status &= ~EFI_SIMPLE_NETWORK_RECEIVE_INTERRUPT;
> > > +     }
> > >   out:
> > >       return EFI_EXIT(ret);
> > >   }
> > >
> >
>
>
> --
> Masami Hiramatsu



--
Masami Hiramatsu