Re: [PATCH] e1000e: set RX desc status with DD flag in a separate operation

[Jason Wang]

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
>