[PATCH 1/2] net: forbid the reentrant RX

[PATCH 1/2] net: forbid the reentrant RX

[Jason Wang]

The memory API allows DMA into NIC's MMIO area. This means the NIC's
RX routine must be reentrant. Instead of auditing all the NIC, we can
simply detect the reentrancy and return early. The queue->delivering
is set and cleared by qemu_net_queue_deliver() for other queue helpers
to know whether the delivering in on going (NIC's receive is being
called). We can check it and return early in qemu_net_queue_flush() to
forbid reentrant RX.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/queue.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/queue.c b/net/queue.c
index 0164727e39..19e32c80fd 100644
--- a/net/queue.c
+++ b/net/queue.c
@@ -250,6 +250,9 @@ void qemu_net_queue_purge(NetQueue *queue, NetClientState *from)
 
 bool qemu_net_queue_flush(NetQueue *queue)
 {
+    if (queue->delivering)
+        return false;
+
     while (!QTAILQ_EMPTY(&queue->packets)) {
         NetPacket *packet;
         int ret;
-- 
2.20.1

[PATCH 2/2] e1000e: make TX reentrant

[Jason Wang]

In loopback mode, e1000e RX can DMA into TX doorbell which requires
TX to be reentrant. This patch make e1000e's TX routine reentrant by
introducing a per device boolean for recording whether or not a TX
rountine is being called and return early.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/e1000e_core.c | 8 ++++++++
 hw/net/e1000e_core.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index bcd186cac5..8126a644a5 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -923,6 +923,12 @@ e1000e_start_xmit(E1000ECore *core, const E1000E_TxRing *txr)
         return;
     }
 
+    if (core->sending) {
+        return;
+    }
+
+    core->sending = true;
+
     while (!e1000e_ring_empty(core, txi)) {
         base = e1000e_ring_head_descr(core, txi);
 
@@ -940,6 +946,8 @@ e1000e_start_xmit(E1000ECore *core, const E1000E_TxRing *txr)
     if (!ide || !e1000e_intrmgr_delay_tx_causes(core, &cause)) {
         e1000e_set_interrupt_cause(core, cause);
     }
+
+    core->sending = false;
 }
 
 static bool
diff --git a/hw/net/e1000e_core.h b/hw/net/e1000e_core.h
index aee32f7e48..4679c1761f 100644
--- a/hw/net/e1000e_core.h
+++ b/hw/net/e1000e_core.h
@@ -114,6 +114,7 @@ struct E1000Core {
     void (*owner_start_recv)(PCIDevice *d);
 
     uint32_t msi_causes_pending;
+    bool sending;
 };
 
 void
-- 
2.20.1

Re: [PATCH 2/2] e1000e: make TX reentrant

[Li Qiang]

Jason Wang <jasowang@redhat.com> 于2020年7月22日周三 下午4:58写道：
>
> In loopback mode, e1000e RX can DMA into TX doorbell which requires
> TX to be reentrant. This patch make e1000e's TX routine reentrant by
> introducing a per device boolean for recording whether or not a TX
> rountine is being called and return early.
>

Could we introduce a per-queue 'sending' variable just like the RX.
So we can do this in net core layer.

Thanks,
Li Qiang

> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/net/e1000e_core.c | 8 ++++++++
>  hw/net/e1000e_core.h | 1 +
>  2 files changed, 9 insertions(+)
>
> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> index bcd186cac5..8126a644a5 100644
> --- a/hw/net/e1000e_core.c
> +++ b/hw/net/e1000e_core.c
> @@ -923,6 +923,12 @@ e1000e_start_xmit(E1000ECore *core, const E1000E_TxRing *txr)
>          return;
>      }
>
> +    if (core->sending) {
> +        return;
> +    }
> +
> +    core->sending = true;
> +
>      while (!e1000e_ring_empty(core, txi)) {
>          base = e1000e_ring_head_descr(core, txi);
>
> @@ -940,6 +946,8 @@ e1000e_start_xmit(E1000ECore *core, const E1000E_TxRing *txr)
>      if (!ide || !e1000e_intrmgr_delay_tx_causes(core, &cause)) {
>          e1000e_set_interrupt_cause(core, cause);
>      }
> +
> +    core->sending = false;
>  }
>
>  static bool
> diff --git a/hw/net/e1000e_core.h b/hw/net/e1000e_core.h
> index aee32f7e48..4679c1761f 100644
> --- a/hw/net/e1000e_core.h
> +++ b/hw/net/e1000e_core.h
> @@ -114,6 +114,7 @@ struct E1000Core {
>      void (*owner_start_recv)(PCIDevice *d);
>
>      uint32_t msi_causes_pending;
> +    bool sending;
>  };
>
>  void
> --
> 2.20.1
>

Re: [PATCH 2/2] e1000e: make TX reentrant

[Michael Tokarev]

FWIW, this is not "making TX reentrant", it is about forbidding
reentrancy instead :)

/mjt

Re: [PATCH 2/2] e1000e: make TX reentrant

[Jason Wang]

On 2020/7/22 下午7:24, Li Qiang wrote:
> Jason Wang <jasowang@redhat.com> 于2020年7月22日周三 下午4:58写道：
>> In loopback mode, e1000e RX can DMA into TX doorbell which requires
>> TX to be reentrant. This patch make e1000e's TX routine reentrant by
>> introducing a per device boolean for recording whether or not a TX
>> rountine is being called and return early.
>>
> Could we introduce a per-queue 'sending' variable just like the RX.
> So we can do this in net core layer.


It's kind of not easy since TX routine is called before the packet can 
reach network queue.

Thanks


>
> Thanks,
> Li Qiang
>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   hw/net/e1000e_core.c | 8 ++++++++
>>   hw/net/e1000e_core.h | 1 +
>>   2 files changed, 9 insertions(+)
>>
>> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
>> index bcd186cac5..8126a644a5 100644
>> --- a/hw/net/e1000e_core.c
>> +++ b/hw/net/e1000e_core.c
>> @@ -923,6 +923,12 @@ e1000e_start_xmit(E1000ECore *core, const E1000E_TxRing *txr)
>>           return;
>>       }
>>
>> +    if (core->sending) {
>> +        return;
>> +    }
>> +
>> +    core->sending = true;
>> +
>>       while (!e1000e_ring_empty(core, txi)) {
>>           base = e1000e_ring_head_descr(core, txi);
>>
>> @@ -940,6 +946,8 @@ e1000e_start_xmit(E1000ECore *core, const E1000E_TxRing *txr)
>>       if (!ide || !e1000e_intrmgr_delay_tx_causes(core, &cause)) {
>>           e1000e_set_interrupt_cause(core, cause);
>>       }
>> +
>> +    core->sending = false;
>>   }
>>
>>   static bool
>> diff --git a/hw/net/e1000e_core.h b/hw/net/e1000e_core.h
>> index aee32f7e48..4679c1761f 100644
>> --- a/hw/net/e1000e_core.h
>> +++ b/hw/net/e1000e_core.h
>> @@ -114,6 +114,7 @@ struct E1000Core {
>>       void (*owner_start_recv)(PCIDevice *d);
>>
>>       uint32_t msi_causes_pending;
>> +    bool sending;
>>   };
>>
>>   void
>> --
>> 2.20.1
>>

Re: [PATCH 2/2] e1000e: make TX reentrant

[Jason Wang]

On 2020/7/22 下午8:53, Michael Tokarev wrote:
> FWIW, this is not "making TX reentrant", it is about forbidding
> reentrancy instead :)
>
> /mjt


Indeed, I will rename the title.

Thanks


>

Re: [PATCH 2/2] e1000e: make TX reentrant

[Peter Maydell]

On Wed, 22 Jul 2020 at 10:00, Jason Wang <jasowang@redhat.com> wrote:
>
> In loopback mode, e1000e RX can DMA into TX doorbell which requires
> TX to be reentrant. This patch make e1000e's TX routine reentrant by
> introducing a per device boolean for recording whether or not a TX
> rountine is being called and return early.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---

This feels like a sticking-plaster fix that's not really in the
right place... It stops us from calling back into
e1000e_start_xmit(), but it doesn't prevent a DMA request
from touching other device registers that update state in
the E100ECore struct that the transmit code is not expecting
to change.

thanks
-- PMM

Re: [PATCH 2/2] e1000e: make TX reentrant

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 372 bytes --]

On Thu, Jul 23, 2020 at 10:25:35AM +0800, Jason Wang wrote:
> 
> On 2020/7/22 下午8:53, Michael Tokarev wrote:
> > FWIW, this is not "making TX reentrant", it is about forbidding
> > reentrancy instead :)
> > 
> > /mjt
> 
> 
> Indeed, I will rename the title.

Please also include a comment explaining the purpose of the early return
in the code.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] e1000e: make TX reentrant

[Jason Wang]

On 2020/7/23 下午6:36, Peter Maydell wrote:
> On Wed, 22 Jul 2020 at 10:00, Jason Wang <jasowang@redhat.com> wrote:
>> In loopback mode, e1000e RX can DMA into TX doorbell which requires
>> TX to be reentrant. This patch make e1000e's TX routine reentrant by
>> introducing a per device boolean for recording whether or not a TX
>> rountine is being called and return early.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
> This feels like a sticking-plaster fix that's not really in the
> right place... It stops us from calling back into
> e1000e_start_xmit(), but it doesn't prevent a DMA request
> from touching other device registers that update state in
> the E100ECore struct that the transmit code is not expecting
> to change.


Right, so we can track the mr owner and fail the memory access if 
there's another mr transaction in memory core: 
memory_region_dispatch_read() and memory_region_dispatch_write().

But what's more interesting is that some device uses bh for the doorbell 
which may require more thought...

Thanks


>
> thanks
> -- PMM
>

Re: [PATCH 1/2] net: forbid the reentrant RX

[Jason Wang]

On 2020/7/22 下午4:57, Jason Wang wrote:
> The memory API allows DMA into NIC's MMIO area. This means the NIC's
> RX routine must be reentrant. Instead of auditing all the NIC, we can
> simply detect the reentrancy and return early. The queue->delivering
> is set and cleared by qemu_net_queue_deliver() for other queue helpers
> to know whether the delivering in on going (NIC's receive is being
> called). We can check it and return early in qemu_net_queue_flush() to
> forbid reentrant RX.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>   net/queue.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/net/queue.c b/net/queue.c
> index 0164727e39..19e32c80fd 100644
> --- a/net/queue.c
> +++ b/net/queue.c
> @@ -250,6 +250,9 @@ void qemu_net_queue_purge(NetQueue *queue, NetClientState *from)
>   
>   bool qemu_net_queue_flush(NetQueue *queue)
>   {
> +    if (queue->delivering)
> +        return false;
> +
>       while (!QTAILQ_EMPTY(&queue->packets)) {
>           NetPacket *packet;
>           int ret;


Queued for rc2.

Thanks

Re: [PATCH 1/2] net: forbid the reentrant RX

[Alexander Bulekov]

On 200728 1200, Jason Wang wrote:
> 
> On 2020/7/22 下午4:57, Jason Wang wrote:
> > The memory API allows DMA into NIC's MMIO area. This means the NIC's
> > RX routine must be reentrant. Instead of auditing all the NIC, we can
> > simply detect the reentrancy and return early. The queue->delivering
> > is set and cleared by qemu_net_queue_deliver() for other queue helpers
> > to know whether the delivering in on going (NIC's receive is being
> > called). We can check it and return early in qemu_net_queue_flush() to
> > forbid reentrant RX.
> > 
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >   net/queue.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/net/queue.c b/net/queue.c
> > index 0164727e39..19e32c80fd 100644
> > --- a/net/queue.c
> > +++ b/net/queue.c
> > @@ -250,6 +250,9 @@ void qemu_net_queue_purge(NetQueue *queue, NetClientState *from)
> >   bool qemu_net_queue_flush(NetQueue *queue)
> >   {
> > +    if (queue->delivering)
> > +        return false;
> > +
> >       while (!QTAILQ_EMPTY(&queue->packets)) {
> >           NetPacket *packet;
> >           int ret;
> 
> 
> Queued for rc2.
> 
> Thanks
> 

Hi Jason,
I don't think this ever made it in. Are there any remaining problems?
Thanks
-Alex

Re: [PATCH 1/2] net: forbid the reentrant RX

[Jason Wang]

On 2020/9/2 下午11:56, Alexander Bulekov wrote:
> On 200728 1200, Jason Wang wrote:
>> On 2020/7/22 下午4:57, Jason Wang wrote:
>>> The memory API allows DMA into NIC's MMIO area. This means the NIC's
>>> RX routine must be reentrant. Instead of auditing all the NIC, we can
>>> simply detect the reentrancy and return early. The queue->delivering
>>> is set and cleared by qemu_net_queue_deliver() for other queue helpers
>>> to know whether the delivering in on going (NIC's receive is being
>>> called). We can check it and return early in qemu_net_queue_flush() to
>>> forbid reentrant RX.
>>>
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> ---
>>>    net/queue.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/net/queue.c b/net/queue.c
>>> index 0164727e39..19e32c80fd 100644
>>> --- a/net/queue.c
>>> +++ b/net/queue.c
>>> @@ -250,6 +250,9 @@ void qemu_net_queue_purge(NetQueue *queue, NetClientState *from)
>>>    bool qemu_net_queue_flush(NetQueue *queue)
>>>    {
>>> +    if (queue->delivering)
>>> +        return false;
>>> +
>>>        while (!QTAILQ_EMPTY(&queue->packets)) {
>>>            NetPacket *packet;
>>>            int ret;
>>
>> Queued for rc2.
>>
>> Thanks
>>
> Hi Jason,
> I don't think this ever made it in. Are there any remaining problems?
> Thanks
> -Alex


Hi Alex:

It should have been merged:

https://git.qemu.org/?p=qemu.git;a=commit;h=22dc8663d9fc7baa22100544c600b6285a63c7a3

Thanks


>

Re: [PATCH 1/2] net: forbid the reentrant RX

[Alexander Bulekov]

On 200903 1156, Jason Wang wrote:
> 
> On 2020/9/2 下午11:56, Alexander Bulekov wrote:
> > On 200728 1200, Jason Wang wrote:
> > > On 2020/7/22 下午4:57, Jason Wang wrote:
> > > > The memory API allows DMA into NIC's MMIO area. This means the NIC's
> > > > RX routine must be reentrant. Instead of auditing all the NIC, we can
> > > > simply detect the reentrancy and return early. The queue->delivering
> > > > is set and cleared by qemu_net_queue_deliver() for other queue helpers
> > > > to know whether the delivering in on going (NIC's receive is being
> > > > called). We can check it and return early in qemu_net_queue_flush() to
> > > > forbid reentrant RX.
> > > > 
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > ---
> > > >    net/queue.c | 3 +++
> > > >    1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/net/queue.c b/net/queue.c
> > > > index 0164727e39..19e32c80fd 100644
> > > > --- a/net/queue.c
> > > > +++ b/net/queue.c
> > > > @@ -250,6 +250,9 @@ void qemu_net_queue_purge(NetQueue *queue, NetClientState *from)
> > > >    bool qemu_net_queue_flush(NetQueue *queue)
> > > >    {
> > > > +    if (queue->delivering)
> > > > +        return false;
> > > > +
> > > >        while (!QTAILQ_EMPTY(&queue->packets)) {
> > > >            NetPacket *packet;
> > > >            int ret;
> > > 
> > > Queued for rc2.
> > > 
> > > Thanks
> > > 
> > Hi Jason,
> > I don't think this ever made it in. Are there any remaining problems?
> > Thanks
> > -Alex
> 
> 
> Hi Alex:
> 
> It should have been merged:
> 
> https://git.qemu.org/?p=qemu.git;a=commit;h=22dc8663d9fc7baa22100544c600b6285a63c7a3
> 
> Thanks
> 

Ah. I missed only 1/2 was queued. I guess the e1000 patch didn't make
the cut..
Thanks
-Alex

> 
> > 
>