On Fri, Sep 9, 2022 at 7:20 PM dinghui <ding...@sangfor.com.cn> wrote: > > On 2022/9/9 10:40, Jason Wang wrote: > > On Sat, Aug 27, 2022 at 12:06 AM Ding Hui <ding...@sangfor.com.cn> wrote: > >> > >> Like commit 034d00d48581 ("e1000: set RX descriptor status in > >> a separate operation"), there is also same issue in e1000e, which > >> would cause lost packets or stop sending packets to VM with DPDK. > >> > >> Do similar fix in e1000e. > >> > >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/402 > >> Signed-off-by: Ding Hui <ding...@sangfor.com.cn> > >> --- > >> hw/net/e1000e_core.c | 54 +++++++++++++++++++++++++++++++++++++++++++- > >> 1 file changed, 53 insertions(+), 1 deletion(-) > >> > >> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c > >> index 208e3e0d79..b8038e4014 100644 > >> --- a/hw/net/e1000e_core.c > >> +++ b/hw/net/e1000e_core.c > >> @@ -1364,6 +1364,58 @@ struct NetRxPkt *pkt, const E1000E_RSSInfo > >> *rss_info, > >> } > >> } > >> > >> +static inline void > >> +e1000e_pci_dma_write_rx_desc(E1000ECore *core, dma_addr_t addr, > >> + uint8_t *desc, dma_addr_t len) > >> +{ > >> + PCIDevice *dev = core->owner; > >> + > >> + if (e1000e_rx_use_legacy_descriptor(core)) { > >> + struct e1000_rx_desc *d = (struct e1000_rx_desc *) desc; > >> + size_t offset = offsetof(struct e1000_rx_desc, status); > >> + typeof(d->status) status = d->status; > >> + > >> + d->status &= ~E1000_RXD_STAT_DD; > >> + pci_dma_write(dev, addr, desc, len); > >> + > >> + if (status & E1000_RXD_STAT_DD) { > >> + d->status = status; > >> + pci_dma_write(dev, addr + offset, &status, sizeof(status)); > >> + } > >> + } else { > >> + if (core->mac[RCTL] & E1000_RCTL_DTYP_PS) { > >> + union e1000_rx_desc_packet_split *d = > >> + (union e1000_rx_desc_packet_split *) desc; > >> + size_t offset = offsetof(union e1000_rx_desc_packet_split, > >> + wb.middle.status_error); > >> + typeof(d->wb.middle.status_error) status = > >> + d->wb.middle.status_error; > > > > Any reason to use typeof here? Its type is known to be uint32_t? > > > > My intention was using exact type same with struct member status_error, > which is indeed uint32_t now. If the type of status_error in struct be > changed in some case, we do not need to change everywhere. > > Maybe I worry too much, the struct is related to h/w spec, so it is > unlikely be changed in the future. > > Should I send v2 to use uint32_t directly? I'm also OK with it.
Yes, please. Thanks > > >> + > >> + d->wb.middle.status_error &= ~E1000_RXD_STAT_DD; > >> + pci_dma_write(dev, addr, desc, len); > >> + > >> + if (status & E1000_RXD_STAT_DD) { > >> + d->wb.middle.status_error = status; > >> + pci_dma_write(dev, addr + offset, &status, > >> sizeof(status)); > >> + } > >> + } else { > >> + union e1000_rx_desc_extended *d = > >> + (union e1000_rx_desc_extended *) desc; > >> + size_t offset = offsetof(union e1000_rx_desc_extended, > >> + wb.upper.status_error); > >> + typeof(d->wb.upper.status_error) status = > >> d->wb.upper.status_error; > > > > So did here. > > > > Thanks > > > >> + > >> + d->wb.upper.status_error &= ~E1000_RXD_STAT_DD; > >> + pci_dma_write(dev, addr, desc, len); > >> + > >> + if (status & E1000_RXD_STAT_DD) { > >> + d->wb.upper.status_error = status; > >> + pci_dma_write(dev, addr + offset, &status, > >> sizeof(status)); > >> + } > >> + } > >> + } > >> +} > >> + > >> typedef struct e1000e_ba_state_st { > >> uint16_t written[MAX_PS_BUFFERS]; > >> uint8_t cur_idx; > >> @@ -1600,7 +1652,7 @@ e1000e_write_packet_to_guest(E1000ECore *core, > >> struct NetRxPkt *pkt, > >> > >> e1000e_write_rx_descr(core, desc, is_last ? core->rx_pkt : NULL, > >> rss_info, do_ps ? ps_hdr_len : 0, > >> &bastate.written); > >> - pci_dma_write(d, base, &desc, core->rx_desc_len); > >> + e1000e_pci_dma_write_rx_desc(core, base, desc, core->rx_desc_len); > >> > >> e1000e_ring_advance(core, rxi, > >> core->rx_desc_len / E1000_MIN_RX_DESC_LEN); > >> -- > >> 2.17.1 > >> > > > > > > > -- > Thanks, > - Ding Hui >