* [PATCH] e1000e: set RX desc status with DD flag in a separate operation
@ 2022-08-26 16:05 Ding Hui
2022-09-09 2:40 ` Jason Wang
0 siblings, 1 reply; 4+ messages in thread
From: Ding Hui @ 2022-08-26 16:05 UTC (permalink / raw)
To: dmitry.fleytman, jasowang; +Cc: qemu-devel, georgmueller, Ding Hui
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;
+
+ 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);
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] e1000e: set RX desc status with DD flag in a separate operation
2022-08-26 16:05 [PATCH] e1000e: set RX desc status with DD flag in a separate operation Ding Hui
@ 2022-09-09 2:40 ` Jason Wang
2022-09-09 11:20 ` dinghui
0 siblings, 1 reply; 4+ messages in thread
From: Jason Wang @ 2022-09-09 2:40 UTC (permalink / raw)
To: Ding Hui; +Cc: Dmitry Fleytman, qemu-devel, georgmueller
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] e1000e: set RX desc status with DD flag in a separate operation
2022-09-09 2:40 ` Jason Wang
@ 2022-09-09 11:20 ` dinghui
2022-09-14 4:41 ` Jason Wang
0 siblings, 1 reply; 4+ messages in thread
From: dinghui @ 2022-09-09 11:20 UTC (permalink / raw)
To: Jason Wang; +Cc: Dmitry Fleytman, qemu-devel, georgmueller
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
>>
>
>
--
Thanks,
- Ding Hui
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] e1000e: set RX desc status with DD flag in a separate operation
2022-09-09 11:20 ` dinghui
@ 2022-09-14 4:41 ` Jason Wang
0 siblings, 0 replies; 4+ messages in thread
From: Jason Wang @ 2022-09-14 4:41 UTC (permalink / raw)
To: dinghui; +Cc: Dmitry Fleytman, qemu-devel, georgmueller
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-09-14 4:42 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-26 16:05 [PATCH] e1000e: set RX desc status with DD flag in a separate operation Ding Hui
2022-09-09 2:40 ` Jason Wang
2022-09-09 11:20 ` dinghui
2022-09-14 4:41 ` Jason Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).