| Message ID | 20220826160543.2120-1-dinghui@sangfor.com.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | e1000e: set RX desc status with DD flag in a separate operation | expand |
On Sat, Aug 27, 2022 at 12:06 AM Ding Hui <dinghui@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 <dinghui@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? > + > + 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 >
On 2022/9/9 10:40, Jason Wang wrote: > On Sat, Aug 27, 2022 at 12:06 AM Ding Hui <dinghui@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 <dinghui@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. >> + >> + 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 >> > >
On Fri, Sep 9, 2022 at 7:20 PM dinghui <dinghui@sangfor.com.cn> wrote: > > On 2022/9/9 10:40, Jason Wang wrote: > > On Sat, Aug 27, 2022 at 12:06 AM Ding Hui <dinghui@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 <dinghui@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 >
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; + + 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; + + 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);
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 <dinghui@sangfor.com.cn> --- hw/net/e1000e_core.c | 54 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-)