Hello, 2017-02-08 16:38 GMT+08:00 Dmitry Fleytman <[email protected]>:
> Hello, > > Thanks for the patch! > > The problem of infinite loop indeed exists in e1000e, however it is > different from the one fixed in e1000. > Please find my comments inline. > > If I read the code correctly, I think this issue is the same. Could you please explain more? Thanks. > ~Dmitry > > > On 7 Feb 2017, at 11:43 AM, Li Qiang <[email protected]> wrote: > > > > From: Li Qiang <[email protected]> > > > > This issue is the same as e1000 network card in this commit: > > e1000: eliminate infinite loops on out-of-bounds transfer start. > > > > Signed-off-by: Li Qiang <[email protected]> > > --- > > hw/net/e1000e_core.c | 17 ++++++++++++++++- > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c > > index 2b11499..53f2b1d 100644 > > --- a/hw/net/e1000e_core.c > > +++ b/hw/net/e1000e_core.c > > @@ -914,7 +914,8 @@ e1000e_start_xmit(E1000ECore *core, const > E1000E_TxRing *txr) > > struct e1000_tx_desc desc; > > bool ide = false; > > const E1000E_RingInfo *txi = txr->i; > > - uint32_t cause = E1000_ICS_TXQE; > > + uint32_t tdh_start = core->mac[txi->dh], cause = E1000_ICS_TXQE; > > + > > > > if (!(core->mac[TCTL] & E1000_TCTL_EN)) { > > trace_e1000e_tx_disabled(); > > @@ -933,6 +934,14 @@ e1000e_start_xmit(E1000ECore *core, const > E1000E_TxRing *txr) > > cause |= e1000e_txdesc_writeback(core, base, &desc, &ide, > txi->idx); > > > > e1000e_ring_advance(core, txi, 1); > > + > > + /* > > + * The following avoid infinite loop, just as the e1000 > > + */ > > + if (core->mac[txi->dh] == tdh_start || > > + tdh_start >= core->mac[txi->dlen] / E1000_RING_DESC_LEN) { > > + break; > > + } > > Part of this validity check, namely > tdh_start >= core->mac[txi->dlen] / E1000_RING_DESC_LEN) > already done by e1000e_ring_advance(), therefore not needed here. > > e1000e_ring_advance() check the added r->dh. Not the original one. > The second part - full wraparound protection is relevant, but it fixes > more symptoms than the cause. > The only possible cause for full wraparound is r->dt (tail) value bigger > or equal to r->dlen (length), > so I would suggest to check for this in e1000e_ring_empty() and cover both > TX and RX cases at once. > > Do you mean 'core->mac[r->dt] >= core->mac[r->dlen]' indicate an empty ring? > > } > > > > if (!ide || !e1000e_intrmgr_delay_tx_causes(core, &cause)) { > > @@ -1500,6 +1509,7 @@ e1000e_write_packet_to_guest(E1000ECore *core, > struct NetRxPkt *pkt, > > size_t desc_size; > > size_t desc_offset = 0; > > size_t iov_ofs = 0; > > + uint32_t rdh_start; > > > > struct iovec *iov = net_rx_pkt_get_iovec(pkt); > > size_t size = net_rx_pkt_get_total_len(pkt); > > @@ -1509,6 +1519,7 @@ e1000e_write_packet_to_guest(E1000ECore *core, > struct NetRxPkt *pkt, > > bool do_ps = e1000e_do_ps(core, pkt, &ps_hdr_len); > > > > rxi = rxr->i; > > + rdh_start = core->mac[rxi->dh]; > > > > do { > > hwaddr ba[MAX_PS_BUFFERS]; > > @@ -1605,6 +1616,10 @@ e1000e_write_packet_to_guest(E1000ECore *core, > struct NetRxPkt *pkt, > > e1000e_ring_advance(core, rxi, > > core->rx_desc_len / E1000_MIN_RX_DESC_LEN); > > > > + if (core->mac[rxi->dh] == rdh_start || > > + rdh_start >= core->mac[rxi->dlen] / E1000_RING_DESC_LEN) { > > + break; > > + } > > } while (desc_offset < total_size); > > > > e1000e_update_rx_stats(core, size, total_size); > > -- > > 1.8.3.1 > > > >
