| Message ID | 20200722085747.6514-2-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] net: forbid the reentrant RX | expand |
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 >
FWIW, this is not "making TX reentrant", it is about forbidding reentrancy instead :) /mjt
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 >>
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 >
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
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
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 >
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
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(+)