[PATCH for 8.0 v2] igb: Save the entire Tx context descriptor

[PATCH for 8.0 v2] igb: Save the entire Tx context descriptor

[Akihiko Odaki]

The current implementation of igb uses only part of a advanced Tx
context descriptor because it misses some features and sniffs the trait
of the packet instead of respecting the packet type specified in the
descriptor. However, we will certainly need the entire Tx context
descriptor when we update igb to respect these ignored fields. Save the
entire Tx context descriptor to prepare for such a change.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
V1 -> V2: Bump igb-tx version

 hw/net/igb.c      | 10 ++++++----
 hw/net/igb_core.c | 17 ++++++++++-------
 hw/net/igb_core.h |  3 +--
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/hw/net/igb.c b/hw/net/igb.c
index c6d753df87..f9ec82fc28 100644
--- a/hw/net/igb.c
+++ b/hw/net/igb.c
@@ -504,11 +504,13 @@ static int igb_post_load(void *opaque, int version_id)
 
 static const VMStateDescription igb_vmstate_tx = {
     .name = "igb-tx",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT16(vlan, struct igb_tx),
-        VMSTATE_UINT16(mss, struct igb_tx),
+        VMSTATE_UINT32(ctx.vlan_macip_lens, struct igb_tx),
+        VMSTATE_UINT32(ctx.seqnum_seed, struct igb_tx),
+        VMSTATE_UINT32(ctx.type_tucmd_mlhl, struct igb_tx),
+        VMSTATE_UINT32(ctx.mss_l4len_idx, struct igb_tx),
         VMSTATE_BOOL(tse, struct igb_tx),
         VMSTATE_BOOL(ixsm, struct igb_tx),
         VMSTATE_BOOL(txsm, struct igb_tx),
diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
index a7c7bfdc75..304f5d849f 100644
--- a/hw/net/igb_core.c
+++ b/hw/net/igb_core.c
@@ -390,7 +390,8 @@ static bool
 igb_setup_tx_offloads(IGBCore *core, struct igb_tx *tx)
 {
     if (tx->tse) {
-        if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, tx->mss)) {
+        uint32_t mss = tx->ctx.mss_l4len_idx >> 16;
+        if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, mss)) {
             return false;
         }
 
@@ -550,8 +551,10 @@ igb_process_tx_desc(IGBCore *core,
                    E1000_ADVTXD_DTYP_CTXT) {
             /* advanced transmit context descriptor */
             tx_ctx_desc = (struct e1000_adv_tx_context_desc *)tx_desc;
-            tx->vlan = le32_to_cpu(tx_ctx_desc->vlan_macip_lens) >> 16;
-            tx->mss = le32_to_cpu(tx_ctx_desc->mss_l4len_idx) >> 16;
+            tx->ctx.vlan_macip_lens = le32_to_cpu(tx_ctx_desc->vlan_macip_lens);
+            tx->ctx.seqnum_seed = le32_to_cpu(tx_ctx_desc->seqnum_seed);
+            tx->ctx.type_tucmd_mlhl = le32_to_cpu(tx_ctx_desc->type_tucmd_mlhl);
+            tx->ctx.mss_l4len_idx = le32_to_cpu(tx_ctx_desc->mss_l4len_idx);
             return;
         } else {
             /* unknown descriptor type */
@@ -575,8 +578,9 @@ igb_process_tx_desc(IGBCore *core,
     if (cmd_type_len & E1000_TXD_CMD_EOP) {
         if (!tx->skip_cp && net_tx_pkt_parse(tx->tx_pkt)) {
             if (cmd_type_len & E1000_TXD_CMD_VLE) {
-                net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, tx->vlan,
-                    core->mac[VET] & 0xffff);
+                uint16_t vlan = tx->ctx.vlan_macip_lens >> 16;
+                uint16_t vet = core->mac[VET] & 0xffff;
+                net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, vlan, vet);
             }
             if (igb_tx_pkt_send(core, tx, queue_index)) {
                 igb_on_tx_done_update_stats(core, tx->tx_pkt);
@@ -4024,8 +4028,7 @@ static void igb_reset(IGBCore *core, bool sw)
     for (i = 0; i < ARRAY_SIZE(core->tx); i++) {
         tx = &core->tx[i];
         net_tx_pkt_reset(tx->tx_pkt);
-        tx->vlan = 0;
-        tx->mss = 0;
+        memset(&tx->ctx, 0, sizeof(tx->ctx));
         tx->tse = false;
         tx->ixsm = false;
         tx->txsm = false;
diff --git a/hw/net/igb_core.h b/hw/net/igb_core.h
index 814c1e264b..3483edc655 100644
--- a/hw/net/igb_core.h
+++ b/hw/net/igb_core.h
@@ -72,8 +72,7 @@ struct IGBCore {
     QEMUTimer *autoneg_timer;
 
     struct igb_tx {
-        uint16_t vlan;  /* VLAN Tag */
-        uint16_t mss;   /* Maximum Segment Size */
+        struct e1000_adv_tx_context_desc ctx;
         bool tse;       /* TCP/UDP Segmentation Enable */
         bool ixsm;      /* Insert IP Checksum */
         bool txsm;      /* Insert TCP/UDP Checksum */
-- 
2.39.2

Re: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor

[Philippe Mathieu-Daudé]

On 16/3/23 16:57, Akihiko Odaki wrote:
> The current implementation of igb uses only part of a advanced Tx
> context descriptor because it misses some features and sniffs the trait
> of the packet instead of respecting the packet type specified in the
> descriptor. However, we will certainly need the entire Tx context
> descriptor when we update igb to respect these ignored fields. Save the
> entire Tx context descriptor to prepare for such a change.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> V1 -> V2: Bump igb-tx version
> 
>   hw/net/igb.c      | 10 ++++++----
>   hw/net/igb_core.c | 17 ++++++++++-------
>   hw/net/igb_core.h |  3 +--
>   3 files changed, 17 insertions(+), 13 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor

[Juan Quintela]

Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> The current implementation of igb uses only part of a advanced Tx
> context descriptor because it misses some features and sniffs the trait
> of the packet instead of respecting the packet type specified in the
> descriptor. However, we will certainly need the entire Tx context
> descriptor when we update igb to respect these ignored fields. Save the
> entire Tx context descriptor to prepare for such a change.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

RE: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor

[Sriram Yagnaraman]

> -----Original Message-----
> From: qemu-devel-bounces+sriram.yagnaraman=est.tech@nongnu.org
> <qemu-devel-bounces+sriram.yagnaraman=est.tech@nongnu.org> On Behalf
> Of Akihiko Odaki
> Sent: Thursday, 16 March 2023 16:57
> Cc: qemu-devel@nongnu.org; Jason Wang <jasowang@redhat.com>; Dmitry
> Fleytman <dmitry.fleytman@gmail.com>; quintela@redhat.com; Philippe
> Mathieu-Daudé <philmd@linaro.org>; Akihiko Odaki
> <akihiko.odaki@daynix.com>
> Subject: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor
> 
> The current implementation of igb uses only part of a advanced Tx context
> descriptor because it misses some features and sniffs the trait of the packet
> instead of respecting the packet type specified in the descriptor. However, we
> will certainly need the entire Tx context descriptor when we update igb to
> respect these ignored fields. Save the entire Tx context descriptor to prepare
> for such a change.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> V1 -> V2: Bump igb-tx version
> 
>  hw/net/igb.c      | 10 ++++++----
>  hw/net/igb_core.c | 17 ++++++++++-------  hw/net/igb_core.h |  3 +--
>  3 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/net/igb.c b/hw/net/igb.c index c6d753df87..f9ec82fc28 100644
> --- a/hw/net/igb.c
> +++ b/hw/net/igb.c
> @@ -504,11 +504,13 @@ static int igb_post_load(void *opaque, int
> version_id)
> 
>  static const VMStateDescription igb_vmstate_tx = {
>      .name = "igb-tx",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
> -        VMSTATE_UINT16(vlan, struct igb_tx),
> -        VMSTATE_UINT16(mss, struct igb_tx),
> +        VMSTATE_UINT32(ctx.vlan_macip_lens, struct igb_tx),
> +        VMSTATE_UINT32(ctx.seqnum_seed, struct igb_tx),
> +        VMSTATE_UINT32(ctx.type_tucmd_mlhl, struct igb_tx),
> +        VMSTATE_UINT32(ctx.mss_l4len_idx, struct igb_tx),
>          VMSTATE_BOOL(tse, struct igb_tx),
>          VMSTATE_BOOL(ixsm, struct igb_tx),
>          VMSTATE_BOOL(txsm, struct igb_tx), diff --git a/hw/net/igb_core.c
> b/hw/net/igb_core.c index a7c7bfdc75..304f5d849f 100644
> --- a/hw/net/igb_core.c
> +++ b/hw/net/igb_core.c
> @@ -390,7 +390,8 @@ static bool
>  igb_setup_tx_offloads(IGBCore *core, struct igb_tx *tx)  {
>      if (tx->tse) {
> -        if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, tx->mss)) {
> +        uint32_t mss = tx->ctx.mss_l4len_idx >> 16;
> +        if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, mss)) {
>              return false;
>          }
> 
> @@ -550,8 +551,10 @@ igb_process_tx_desc(IGBCore *core,
>                     E1000_ADVTXD_DTYP_CTXT) {
>              /* advanced transmit context descriptor */
>              tx_ctx_desc = (struct e1000_adv_tx_context_desc *)tx_desc;
> -            tx->vlan = le32_to_cpu(tx_ctx_desc->vlan_macip_lens) >> 16;
> -            tx->mss = le32_to_cpu(tx_ctx_desc->mss_l4len_idx) >> 16;
> +            tx->ctx.vlan_macip_lens = le32_to_cpu(tx_ctx_desc->vlan_macip_lens);
> +            tx->ctx.seqnum_seed = le32_to_cpu(tx_ctx_desc->seqnum_seed);
> +            tx->ctx.type_tucmd_mlhl = le32_to_cpu(tx_ctx_desc-
> >type_tucmd_mlhl);
> +            tx->ctx.mss_l4len_idx =
> + le32_to_cpu(tx_ctx_desc->mss_l4len_idx);

Wouldn't it be better to parse the context into all the required fields like vlan, mss, etc., already when handling the context descriptor, instead of parsing it for every data descriptor later?
Also, in my yet to be merged patch [1] which handles VLAN insertion for VMDq I use the vlan field in multiple places, so it would be better to have the vlan value readily available. 
[1]: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg00393.html

>              return;
>          } else {
>              /* unknown descriptor type */ @@ -575,8 +578,9 @@
> igb_process_tx_desc(IGBCore *core,
>      if (cmd_type_len & E1000_TXD_CMD_EOP) {
>          if (!tx->skip_cp && net_tx_pkt_parse(tx->tx_pkt)) {
>              if (cmd_type_len & E1000_TXD_CMD_VLE) {
> -                net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, tx->vlan,
> -                    core->mac[VET] & 0xffff);
> +                uint16_t vlan = tx->ctx.vlan_macip_lens >> 16;
> +                uint16_t vet = core->mac[VET] & 0xffff;
> +                net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, vlan, vet);
>              }
>              if (igb_tx_pkt_send(core, tx, queue_index)) {
>                  igb_on_tx_done_update_stats(core, tx->tx_pkt); @@ -4024,8
> +4028,7 @@ static void igb_reset(IGBCore *core, bool sw)
>      for (i = 0; i < ARRAY_SIZE(core->tx); i++) {
>          tx = &core->tx[i];
>          net_tx_pkt_reset(tx->tx_pkt);
> -        tx->vlan = 0;
> -        tx->mss = 0;
> +        memset(&tx->ctx, 0, sizeof(tx->ctx));
>          tx->tse = false;
>          tx->ixsm = false;
>          tx->txsm = false;
> diff --git a/hw/net/igb_core.h b/hw/net/igb_core.h index
> 814c1e264b..3483edc655 100644
> --- a/hw/net/igb_core.h
> +++ b/hw/net/igb_core.h
> @@ -72,8 +72,7 @@ struct IGBCore {
>      QEMUTimer *autoneg_timer;
> 
>      struct igb_tx {
> -        uint16_t vlan;  /* VLAN Tag */
> -        uint16_t mss;   /* Maximum Segment Size */
> +        struct e1000_adv_tx_context_desc ctx;
>          bool tse;       /* TCP/UDP Segmentation Enable */
>          bool ixsm;      /* Insert IP Checksum */
>          bool txsm;      /* Insert TCP/UDP Checksum */
> --
> 2.39.2
> 

Re: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor

[Akihiko Odaki]

On 2023/03/17 5:27, Sriram Yagnaraman wrote:
> 
>> -----Original Message-----
>> From: qemu-devel-bounces+sriram.yagnaraman=est.tech@nongnu.org
>> <qemu-devel-bounces+sriram.yagnaraman=est.tech@nongnu.org> On Behalf
>> Of Akihiko Odaki
>> Sent: Thursday, 16 March 2023 16:57
>> Cc: qemu-devel@nongnu.org; Jason Wang <jasowang@redhat.com>; Dmitry
>> Fleytman <dmitry.fleytman@gmail.com>; quintela@redhat.com; Philippe
>> Mathieu-Daudé <philmd@linaro.org>; Akihiko Odaki
>> <akihiko.odaki@daynix.com>
>> Subject: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor
>>
>> The current implementation of igb uses only part of a advanced Tx context
>> descriptor because it misses some features and sniffs the trait of the packet
>> instead of respecting the packet type specified in the descriptor. However, we
>> will certainly need the entire Tx context descriptor when we update igb to
>> respect these ignored fields. Save the entire Tx context descriptor to prepare
>> for such a change.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>> V1 -> V2: Bump igb-tx version
>>
>>   hw/net/igb.c      | 10 ++++++----
>>   hw/net/igb_core.c | 17 ++++++++++-------  hw/net/igb_core.h |  3 +--
>>   3 files changed, 17 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/net/igb.c b/hw/net/igb.c index c6d753df87..f9ec82fc28 100644
>> --- a/hw/net/igb.c
>> +++ b/hw/net/igb.c
>> @@ -504,11 +504,13 @@ static int igb_post_load(void *opaque, int
>> version_id)
>>
>>   static const VMStateDescription igb_vmstate_tx = {
>>       .name = "igb-tx",
>> -    .version_id = 1,
>> -    .minimum_version_id = 1,
>> +    .version_id = 2,
>> +    .minimum_version_id = 2,
>>       .fields = (VMStateField[]) {
>> -        VMSTATE_UINT16(vlan, struct igb_tx),
>> -        VMSTATE_UINT16(mss, struct igb_tx),
>> +        VMSTATE_UINT32(ctx.vlan_macip_lens, struct igb_tx),
>> +        VMSTATE_UINT32(ctx.seqnum_seed, struct igb_tx),
>> +        VMSTATE_UINT32(ctx.type_tucmd_mlhl, struct igb_tx),
>> +        VMSTATE_UINT32(ctx.mss_l4len_idx, struct igb_tx),
>>           VMSTATE_BOOL(tse, struct igb_tx),
>>           VMSTATE_BOOL(ixsm, struct igb_tx),
>>           VMSTATE_BOOL(txsm, struct igb_tx), diff --git a/hw/net/igb_core.c
>> b/hw/net/igb_core.c index a7c7bfdc75..304f5d849f 100644
>> --- a/hw/net/igb_core.c
>> +++ b/hw/net/igb_core.c
>> @@ -390,7 +390,8 @@ static bool
>>   igb_setup_tx_offloads(IGBCore *core, struct igb_tx *tx)  {
>>       if (tx->tse) {
>> -        if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, tx->mss)) {
>> +        uint32_t mss = tx->ctx.mss_l4len_idx >> 16;
>> +        if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, mss)) {
>>               return false;
>>           }
>>
>> @@ -550,8 +551,10 @@ igb_process_tx_desc(IGBCore *core,
>>                      E1000_ADVTXD_DTYP_CTXT) {
>>               /* advanced transmit context descriptor */
>>               tx_ctx_desc = (struct e1000_adv_tx_context_desc *)tx_desc;
>> -            tx->vlan = le32_to_cpu(tx_ctx_desc->vlan_macip_lens) >> 16;
>> -            tx->mss = le32_to_cpu(tx_ctx_desc->mss_l4len_idx) >> 16;
>> +            tx->ctx.vlan_macip_lens = le32_to_cpu(tx_ctx_desc->vlan_macip_lens);
>> +            tx->ctx.seqnum_seed = le32_to_cpu(tx_ctx_desc->seqnum_seed);
>> +            tx->ctx.type_tucmd_mlhl = le32_to_cpu(tx_ctx_desc-
>>> type_tucmd_mlhl);
>> +            tx->ctx.mss_l4len_idx =
>> + le32_to_cpu(tx_ctx_desc->mss_l4len_idx);
> 
> Wouldn't it be better to parse the context into all the required fields like vlan, mss, etc., already when handling the context descriptor, instead of parsing it for every data descriptor later?
> Also, in my yet to be merged patch [1] which handles VLAN insertion for VMDq I use the vlan field in multiple places, so it would be better to have the vlan value readily available.
> [1]: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg00393.html

If there is a better representation of the entire context descriptor we 
may use it as an internal use, but I think it is good enough for the 
purpose, too.

For patch [1], I think it is better to gather the logic to derive the 
VID into one place instead of cluttering several places with the 
relevant code. Concretely, igb_tx_insert_vlan() can decide whether to 
read the VID from the context descriptor or from VMIR, or not to read 
(in case VLAN insertion is disabled).

Regards,
Akihiko Odaki

> 
>>               return;
>>           } else {
>>               /* unknown descriptor type */ @@ -575,8 +578,9 @@
>> igb_process_tx_desc(IGBCore *core,
>>       if (cmd_type_len & E1000_TXD_CMD_EOP) {
>>           if (!tx->skip_cp && net_tx_pkt_parse(tx->tx_pkt)) {
>>               if (cmd_type_len & E1000_TXD_CMD_VLE) {
>> -                net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, tx->vlan,
>> -                    core->mac[VET] & 0xffff);
>> +                uint16_t vlan = tx->ctx.vlan_macip_lens >> 16;
>> +                uint16_t vet = core->mac[VET] & 0xffff;
>> +                net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, vlan, vet);
>>               }
>>               if (igb_tx_pkt_send(core, tx, queue_index)) {
>>                   igb_on_tx_done_update_stats(core, tx->tx_pkt); @@ -4024,8
>> +4028,7 @@ static void igb_reset(IGBCore *core, bool sw)
>>       for (i = 0; i < ARRAY_SIZE(core->tx); i++) {
>>           tx = &core->tx[i];
>>           net_tx_pkt_reset(tx->tx_pkt);
>> -        tx->vlan = 0;
>> -        tx->mss = 0;
>> +        memset(&tx->ctx, 0, sizeof(tx->ctx));
>>           tx->tse = false;
>>           tx->ixsm = false;
>>           tx->txsm = false;
>> diff --git a/hw/net/igb_core.h b/hw/net/igb_core.h index
>> 814c1e264b..3483edc655 100644
>> --- a/hw/net/igb_core.h
>> +++ b/hw/net/igb_core.h
>> @@ -72,8 +72,7 @@ struct IGBCore {
>>       QEMUTimer *autoneg_timer;
>>
>>       struct igb_tx {
>> -        uint16_t vlan;  /* VLAN Tag */
>> -        uint16_t mss;   /* Maximum Segment Size */
>> +        struct e1000_adv_tx_context_desc ctx;
>>           bool tse;       /* TCP/UDP Segmentation Enable */
>>           bool ixsm;      /* Insert IP Checksum */
>>           bool txsm;      /* Insert TCP/UDP Checksum */
>> --
>> 2.39.2
>>
> 

RE: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor

[Sriram Yagnaraman]

> -----Original Message-----
> From: Akihiko Odaki <akihiko.odaki@daynix.com>
> Sent: Friday, 17 March 2023 06:46
> To: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> Cc: qemu-devel@nongnu.org; Jason Wang <jasowang@redhat.com>; Dmitry
> Fleytman <dmitry.fleytman@gmail.com>; quintela@redhat.com; Philippe
> Mathieu-Daudé <philmd@linaro.org>
> Subject: Re: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor
> 
> On 2023/03/17 5:27, Sriram Yagnaraman wrote:
> >
> >> -----Original Message-----
> >> From: qemu-devel-bounces+sriram.yagnaraman=est.tech@nongnu.org
> >> <qemu-devel-bounces+sriram.yagnaraman=est.tech@nongnu.org> On
> Behalf
> >> Of Akihiko Odaki
> >> Sent: Thursday, 16 March 2023 16:57
> >> Cc: qemu-devel@nongnu.org; Jason Wang <jasowang@redhat.com>;
> Dmitry
> >> Fleytman <dmitry.fleytman@gmail.com>; quintela@redhat.com; Philippe
> >> Mathieu-Daudé <philmd@linaro.org>; Akihiko Odaki
> >> <akihiko.odaki@daynix.com>
> >> Subject: [PATCH for 8.0 v2] igb: Save the entire Tx context
> >> descriptor
> >>
> >> The current implementation of igb uses only part of a advanced Tx
> >> context descriptor because it misses some features and sniffs the
> >> trait of the packet instead of respecting the packet type specified
> >> in the descriptor. However, we will certainly need the entire Tx
> >> context descriptor when we update igb to respect these ignored
> >> fields. Save the entire Tx context descriptor to prepare for such a change.
> >>
> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >> ---
> >> V1 -> V2: Bump igb-tx version
> >>
> >>   hw/net/igb.c      | 10 ++++++----
> >>   hw/net/igb_core.c | 17 ++++++++++-------  hw/net/igb_core.h |  3 +--
> >>   3 files changed, 17 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/hw/net/igb.c b/hw/net/igb.c index c6d753df87..f9ec82fc28
> >> 100644
> >> --- a/hw/net/igb.c
> >> +++ b/hw/net/igb.c
> >> @@ -504,11 +504,13 @@ static int igb_post_load(void *opaque, int
> >> version_id)
> >>
> >>   static const VMStateDescription igb_vmstate_tx = {
> >>       .name = "igb-tx",
> >> -    .version_id = 1,
> >> -    .minimum_version_id = 1,
> >> +    .version_id = 2,
> >> +    .minimum_version_id = 2,
> >>       .fields = (VMStateField[]) {
> >> -        VMSTATE_UINT16(vlan, struct igb_tx),
> >> -        VMSTATE_UINT16(mss, struct igb_tx),
> >> +        VMSTATE_UINT32(ctx.vlan_macip_lens, struct igb_tx),
> >> +        VMSTATE_UINT32(ctx.seqnum_seed, struct igb_tx),
> >> +        VMSTATE_UINT32(ctx.type_tucmd_mlhl, struct igb_tx),
> >> +        VMSTATE_UINT32(ctx.mss_l4len_idx, struct igb_tx),
> >>           VMSTATE_BOOL(tse, struct igb_tx),
> >>           VMSTATE_BOOL(ixsm, struct igb_tx),
> >>           VMSTATE_BOOL(txsm, struct igb_tx), diff --git
> >> a/hw/net/igb_core.c b/hw/net/igb_core.c index a7c7bfdc75..304f5d849f
> >> 100644
> >> --- a/hw/net/igb_core.c
> >> +++ b/hw/net/igb_core.c
> >> @@ -390,7 +390,8 @@ static bool
> >>   igb_setup_tx_offloads(IGBCore *core, struct igb_tx *tx)  {
> >>       if (tx->tse) {
> >> -        if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, tx->mss)) {
> >> +        uint32_t mss = tx->ctx.mss_l4len_idx >> 16;
> >> +        if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, mss))
> >> + {
> >>               return false;
> >>           }
> >>
> >> @@ -550,8 +551,10 @@ igb_process_tx_desc(IGBCore *core,
> >>                      E1000_ADVTXD_DTYP_CTXT) {
> >>               /* advanced transmit context descriptor */
> >>               tx_ctx_desc = (struct e1000_adv_tx_context_desc *)tx_desc;
> >> -            tx->vlan = le32_to_cpu(tx_ctx_desc->vlan_macip_lens) >> 16;
> >> -            tx->mss = le32_to_cpu(tx_ctx_desc->mss_l4len_idx) >> 16;
> >> +            tx->ctx.vlan_macip_lens = le32_to_cpu(tx_ctx_desc-
> >vlan_macip_lens);
> >> +            tx->ctx.seqnum_seed = le32_to_cpu(tx_ctx_desc->seqnum_seed);
> >> +            tx->ctx.type_tucmd_mlhl = le32_to_cpu(tx_ctx_desc-
> >>> type_tucmd_mlhl);
> >> +            tx->ctx.mss_l4len_idx =
> >> + le32_to_cpu(tx_ctx_desc->mss_l4len_idx);
> >
> > Wouldn't it be better to parse the context into all the required fields like vlan,
> mss, etc., already when handling the context descriptor, instead of parsing it for
> every data descriptor later?
> > Also, in my yet to be merged patch [1] which handles VLAN insertion for
> VMDq I use the vlan field in multiple places, so it would be better to have the
> vlan value readily available.
> > [1]:
> > https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg00393.html
> 
> If there is a better representation of the entire context descriptor we may use it
> as an internal use, but I think it is good enough for the purpose, too.

If I understand correctly this change will help to realize "Table 7-31: Valid context vs Required Offload"? 
If so, the internal representation can already be prepared to hold the following: 
- vlan
- l4len
- iplen
- maclen
- mss
- l4type
- ip4

If we have an internal representation, the fields can hold values from the context, or from VMDq registers, or from the data descriptor depending on what the driver has configured.
Anyhow, I don't want to derail this patch, you already have ACKs, please go ahead if you don't want to do this now.

> 
> For patch [1], I think it is better to gather the logic to derive the VID into one
> place instead of cluttering several places with the relevant code. Concretely,
> igb_tx_insert_vlan() can decide whether to read the VID from the context
> descriptor or from VMIR, or not to read (in case VLAN insertion is disabled).
> 
> Regards,
> Akihiko Odaki
> 
> >
> >>               return;
> >>           } else {
> >>               /* unknown descriptor type */ @@ -575,8 +578,9 @@
> >> igb_process_tx_desc(IGBCore *core,
> >>       if (cmd_type_len & E1000_TXD_CMD_EOP) {
> >>           if (!tx->skip_cp && net_tx_pkt_parse(tx->tx_pkt)) {
> >>               if (cmd_type_len & E1000_TXD_CMD_VLE) {
> >> -                net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, tx->vlan,
> >> -                    core->mac[VET] & 0xffff);
> >> +                uint16_t vlan = tx->ctx.vlan_macip_lens >> 16;
> >> +                uint16_t vet = core->mac[VET] & 0xffff;
> >> +                net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, vlan,
> >> + vet);
> >>               }
> >>               if (igb_tx_pkt_send(core, tx, queue_index)) {
> >>                   igb_on_tx_done_update_stats(core, tx->tx_pkt); @@
> >> -4024,8
> >> +4028,7 @@ static void igb_reset(IGBCore *core, bool sw)
> >>       for (i = 0; i < ARRAY_SIZE(core->tx); i++) {
> >>           tx = &core->tx[i];
> >>           net_tx_pkt_reset(tx->tx_pkt);
> >> -        tx->vlan = 0;
> >> -        tx->mss = 0;
> >> +        memset(&tx->ctx, 0, sizeof(tx->ctx));
> >>           tx->tse = false;
> >>           tx->ixsm = false;
> >>           tx->txsm = false;
> >> diff --git a/hw/net/igb_core.h b/hw/net/igb_core.h index
> >> 814c1e264b..3483edc655 100644
> >> --- a/hw/net/igb_core.h
> >> +++ b/hw/net/igb_core.h
> >> @@ -72,8 +72,7 @@ struct IGBCore {
> >>       QEMUTimer *autoneg_timer;
> >>
> >>       struct igb_tx {
> >> -        uint16_t vlan;  /* VLAN Tag */
> >> -        uint16_t mss;   /* Maximum Segment Size */
> >> +        struct e1000_adv_tx_context_desc ctx;
> >>           bool tse;       /* TCP/UDP Segmentation Enable */
> >>           bool ixsm;      /* Insert IP Checksum */
> >>           bool txsm;      /* Insert TCP/UDP Checksum */
> >> --
> >> 2.39.2
> >>
> >

Re: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor

[Akihiko Odaki]

On 2023/03/17 18:19, Sriram Yagnaraman wrote:
> 
> 
>> -----Original Message-----
>> From: Akihiko Odaki <akihiko.odaki@daynix.com>
>> Sent: Friday, 17 March 2023 06:46
>> To: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
>> Cc: qemu-devel@nongnu.org; Jason Wang <jasowang@redhat.com>; Dmitry
>> Fleytman <dmitry.fleytman@gmail.com>; quintela@redhat.com; Philippe
>> Mathieu-Daudé <philmd@linaro.org>
>> Subject: Re: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor
>>
>> On 2023/03/17 5:27, Sriram Yagnaraman wrote:
>>>
>>>> -----Original Message-----
>>>> From: qemu-devel-bounces+sriram.yagnaraman=est.tech@nongnu.org
>>>> <qemu-devel-bounces+sriram.yagnaraman=est.tech@nongnu.org> On
>> Behalf
>>>> Of Akihiko Odaki
>>>> Sent: Thursday, 16 March 2023 16:57
>>>> Cc: qemu-devel@nongnu.org; Jason Wang <jasowang@redhat.com>;
>> Dmitry
>>>> Fleytman <dmitry.fleytman@gmail.com>; quintela@redhat.com; Philippe
>>>> Mathieu-Daudé <philmd@linaro.org>; Akihiko Odaki
>>>> <akihiko.odaki@daynix.com>
>>>> Subject: [PATCH for 8.0 v2] igb: Save the entire Tx context
>>>> descriptor
>>>>
>>>> The current implementation of igb uses only part of a advanced Tx
>>>> context descriptor because it misses some features and sniffs the
>>>> trait of the packet instead of respecting the packet type specified
>>>> in the descriptor. However, we will certainly need the entire Tx
>>>> context descriptor when we update igb to respect these ignored
>>>> fields. Save the entire Tx context descriptor to prepare for such a change.
>>>>
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> ---
>>>> V1 -> V2: Bump igb-tx version
>>>>
>>>>    hw/net/igb.c      | 10 ++++++----
>>>>    hw/net/igb_core.c | 17 ++++++++++-------  hw/net/igb_core.h |  3 +--
>>>>    3 files changed, 17 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/hw/net/igb.c b/hw/net/igb.c index c6d753df87..f9ec82fc28
>>>> 100644
>>>> --- a/hw/net/igb.c
>>>> +++ b/hw/net/igb.c
>>>> @@ -504,11 +504,13 @@ static int igb_post_load(void *opaque, int
>>>> version_id)
>>>>
>>>>    static const VMStateDescription igb_vmstate_tx = {
>>>>        .name = "igb-tx",
>>>> -    .version_id = 1,
>>>> -    .minimum_version_id = 1,
>>>> +    .version_id = 2,
>>>> +    .minimum_version_id = 2,
>>>>        .fields = (VMStateField[]) {
>>>> -        VMSTATE_UINT16(vlan, struct igb_tx),
>>>> -        VMSTATE_UINT16(mss, struct igb_tx),
>>>> +        VMSTATE_UINT32(ctx.vlan_macip_lens, struct igb_tx),
>>>> +        VMSTATE_UINT32(ctx.seqnum_seed, struct igb_tx),
>>>> +        VMSTATE_UINT32(ctx.type_tucmd_mlhl, struct igb_tx),
>>>> +        VMSTATE_UINT32(ctx.mss_l4len_idx, struct igb_tx),
>>>>            VMSTATE_BOOL(tse, struct igb_tx),
>>>>            VMSTATE_BOOL(ixsm, struct igb_tx),
>>>>            VMSTATE_BOOL(txsm, struct igb_tx), diff --git
>>>> a/hw/net/igb_core.c b/hw/net/igb_core.c index a7c7bfdc75..304f5d849f
>>>> 100644
>>>> --- a/hw/net/igb_core.c
>>>> +++ b/hw/net/igb_core.c
>>>> @@ -390,7 +390,8 @@ static bool
>>>>    igb_setup_tx_offloads(IGBCore *core, struct igb_tx *tx)  {
>>>>        if (tx->tse) {
>>>> -        if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, tx->mss)) {
>>>> +        uint32_t mss = tx->ctx.mss_l4len_idx >> 16;
>>>> +        if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, mss))
>>>> + {
>>>>                return false;
>>>>            }
>>>>
>>>> @@ -550,8 +551,10 @@ igb_process_tx_desc(IGBCore *core,
>>>>                       E1000_ADVTXD_DTYP_CTXT) {
>>>>                /* advanced transmit context descriptor */
>>>>                tx_ctx_desc = (struct e1000_adv_tx_context_desc *)tx_desc;
>>>> -            tx->vlan = le32_to_cpu(tx_ctx_desc->vlan_macip_lens) >> 16;
>>>> -            tx->mss = le32_to_cpu(tx_ctx_desc->mss_l4len_idx) >> 16;
>>>> +            tx->ctx.vlan_macip_lens = le32_to_cpu(tx_ctx_desc-
>>> vlan_macip_lens);
>>>> +            tx->ctx.seqnum_seed = le32_to_cpu(tx_ctx_desc->seqnum_seed);
>>>> +            tx->ctx.type_tucmd_mlhl = le32_to_cpu(tx_ctx_desc-
>>>>> type_tucmd_mlhl);
>>>> +            tx->ctx.mss_l4len_idx =
>>>> + le32_to_cpu(tx_ctx_desc->mss_l4len_idx);
>>>
>>> Wouldn't it be better to parse the context into all the required fields like vlan,
>> mss, etc., already when handling the context descriptor, instead of parsing it for
>> every data descriptor later?
>>> Also, in my yet to be merged patch [1] which handles VLAN insertion for
>> VMDq I use the vlan field in multiple places, so it would be better to have the
>> vlan value readily available.
>>> [1]:
>>> https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg00393.html
>>
>> If there is a better representation of the entire context descriptor we may use it
>> as an internal use, but I think it is good enough for the purpose, too.
> 
> If I understand correctly this change will help to realize "Table 7-31: Valid context vs Required Offload"?
> If so, the internal representation can already be prepared to hold the following:
> - vlan
> - l4len
> - iplen
> - maclen
> - mss
> - l4type
> - ip4

This patch is not intended to cover a specific feature, but it is 
intended to allow us to implement any (currently) uncovered features in 
the future. In this regard, all of the fields except the reserved fields 
and DTYP are necessary. Anyway, in my opinion, the original format of 
the context descriptor is well-suited to save the fields you listed.

> 
> If we have an internal representation, the fields can hold values from the context, or from VMDq registers, or from the data descriptor depending on what the driver has configured.

Certainly if you save the context descriptor as is, you do not expect 
the fields of the saved descriptor to repesent values from other places 
like VMDq registers. However I rather see this as a more favorable 
feature. The driver can change the configuration anytime, and the 
effective value can change accordingly. For example, you may see the 
configuration at a time, and you overwrite the value saved from the 
context with the value provided from the configuration, the value of the 
field may be valid at the time, but if the configuration gets changed 
after that and the value from the context becomes valid again, you will 
no longer know what the effective value is. Such a situation is rare in 
this case, but still better to cover if possible.

And it seems your patch for VLAN actually has such a bug. I thought it 
is OK to overwrite the VID got from the context descriptor with another 
in igb_tx_insert_vlan() as it is EOP and the context becomes invalid 
anyway, but I have just realized nothing says the old context gets 
invalidated after EOP. The VID from the context descriptor should not be 
overwritten.

In general, a persistent state derived from other persistent states are 
prone to such bugs. The less error-prone way is to have a function which 
derives the value effective now.

> Anyhow, I don't want to derail this patch, you already have ACKs, please go ahead if you don't want to do this now.

I'll keep this moving forward, but thank you for sharing your thoughts.

For your VLAN patch, please correct the bug I described and submit a new 
version (probably after this patch is landed as your patch is affected 
with the change of vlan field.)

Regards,
Akihiko Odaki

> 
>>
>> For patch [1], I think it is better to gather the logic to derive the VID into one
>> place instead of cluttering several places with the relevant code. Concretely,
>> igb_tx_insert_vlan() can decide whether to read the VID from the context
>> descriptor or from VMIR, or not to read (in case VLAN insertion is disabled).
>>
>> Regards,
>> Akihiko Odaki
>>
>>>
>>>>                return;
>>>>            } else {
>>>>                /* unknown descriptor type */ @@ -575,8 +578,9 @@
>>>> igb_process_tx_desc(IGBCore *core,
>>>>        if (cmd_type_len & E1000_TXD_CMD_EOP) {
>>>>            if (!tx->skip_cp && net_tx_pkt_parse(tx->tx_pkt)) {
>>>>                if (cmd_type_len & E1000_TXD_CMD_VLE) {
>>>> -                net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, tx->vlan,
>>>> -                    core->mac[VET] & 0xffff);
>>>> +                uint16_t vlan = tx->ctx.vlan_macip_lens >> 16;
>>>> +                uint16_t vet = core->mac[VET] & 0xffff;
>>>> +                net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, vlan,
>>>> + vet);
>>>>                }
>>>>                if (igb_tx_pkt_send(core, tx, queue_index)) {
>>>>                    igb_on_tx_done_update_stats(core, tx->tx_pkt); @@
>>>> -4024,8
>>>> +4028,7 @@ static void igb_reset(IGBCore *core, bool sw)
>>>>        for (i = 0; i < ARRAY_SIZE(core->tx); i++) {
>>>>            tx = &core->tx[i];
>>>>            net_tx_pkt_reset(tx->tx_pkt);
>>>> -        tx->vlan = 0;
>>>> -        tx->mss = 0;
>>>> +        memset(&tx->ctx, 0, sizeof(tx->ctx));
>>>>            tx->tse = false;
>>>>            tx->ixsm = false;
>>>>            tx->txsm = false;
>>>> diff --git a/hw/net/igb_core.h b/hw/net/igb_core.h index
>>>> 814c1e264b..3483edc655 100644
>>>> --- a/hw/net/igb_core.h
>>>> +++ b/hw/net/igb_core.h
>>>> @@ -72,8 +72,7 @@ struct IGBCore {
>>>>        QEMUTimer *autoneg_timer;
>>>>
>>>>        struct igb_tx {
>>>> -        uint16_t vlan;  /* VLAN Tag */
>>>> -        uint16_t mss;   /* Maximum Segment Size */
>>>> +        struct e1000_adv_tx_context_desc ctx;
>>>>            bool tse;       /* TCP/UDP Segmentation Enable */
>>>>            bool ixsm;      /* Insert IP Checksum */
>>>>            bool txsm;      /* Insert TCP/UDP Checksum */
>>>> --
>>>> 2.39.2
>>>>
>>>

RE: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor

[Sriram Yagnaraman]

> -----Original Message-----
> From: Akihiko Odaki <akihiko.odaki@daynix.com>
> Sent: Friday, 17 March 2023 12:13
> To: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> Cc: qemu-devel@nongnu.org; Jason Wang <jasowang@redhat.com>; Dmitry
> Fleytman <dmitry.fleytman@gmail.com>; quintela@redhat.com; Philippe
> Mathieu-Daudé <philmd@linaro.org>
> Subject: Re: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor
> 
> On 2023/03/17 18:19, Sriram Yagnaraman wrote:
> >
> >
> >> -----Original Message-----
> >> From: Akihiko Odaki <akihiko.odaki@daynix.com>
> >> Sent: Friday, 17 March 2023 06:46
> >> To: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> >> Cc: qemu-devel@nongnu.org; Jason Wang <jasowang@redhat.com>;
> Dmitry
> >> Fleytman <dmitry.fleytman@gmail.com>; quintela@redhat.com; Philippe
> >> Mathieu-Daudé <philmd@linaro.org>
> >> Subject: Re: [PATCH for 8.0 v2] igb: Save the entire Tx context
> >> descriptor
> >>
> >> On 2023/03/17 5:27, Sriram Yagnaraman wrote:
> >>>
> >>>> -----Original Message-----
> >>>> From: qemu-devel-bounces+sriram.yagnaraman=est.tech@nongnu.org
> >>>> <qemu-devel-bounces+sriram.yagnaraman=est.tech@nongnu.org> On
> >> Behalf
> >>>> Of Akihiko Odaki
> >>>> Sent: Thursday, 16 March 2023 16:57
> >>>> Cc: qemu-devel@nongnu.org; Jason Wang <jasowang@redhat.com>;
> >> Dmitry
> >>>> Fleytman <dmitry.fleytman@gmail.com>; quintela@redhat.com; Philippe
> >>>> Mathieu-Daudé <philmd@linaro.org>; Akihiko Odaki
> >>>> <akihiko.odaki@daynix.com>
> >>>> Subject: [PATCH for 8.0 v2] igb: Save the entire Tx context
> >>>> descriptor
> >>>>
> >>>> The current implementation of igb uses only part of a advanced Tx
> >>>> context descriptor because it misses some features and sniffs the
> >>>> trait of the packet instead of respecting the packet type specified
> >>>> in the descriptor. However, we will certainly need the entire Tx
> >>>> context descriptor when we update igb to respect these ignored
> >>>> fields. Save the entire Tx context descriptor to prepare for such a change.
> >>>>
> >>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >>>> ---
> >>>> V1 -> V2: Bump igb-tx version
> >>>>
> >>>>    hw/net/igb.c      | 10 ++++++----
> >>>>    hw/net/igb_core.c | 17 ++++++++++-------  hw/net/igb_core.h |  3 +--
> >>>>    3 files changed, 17 insertions(+), 13 deletions(-)
> >>>>
> >>>> diff --git a/hw/net/igb.c b/hw/net/igb.c index
> >>>> c6d753df87..f9ec82fc28
> >>>> 100644
> >>>> --- a/hw/net/igb.c
> >>>> +++ b/hw/net/igb.c
> >>>> @@ -504,11 +504,13 @@ static int igb_post_load(void *opaque, int
> >>>> version_id)
> >>>>
> >>>>    static const VMStateDescription igb_vmstate_tx = {
> >>>>        .name = "igb-tx",
> >>>> -    .version_id = 1,
> >>>> -    .minimum_version_id = 1,
> >>>> +    .version_id = 2,
> >>>> +    .minimum_version_id = 2,
> >>>>        .fields = (VMStateField[]) {
> >>>> -        VMSTATE_UINT16(vlan, struct igb_tx),
> >>>> -        VMSTATE_UINT16(mss, struct igb_tx),
> >>>> +        VMSTATE_UINT32(ctx.vlan_macip_lens, struct igb_tx),
> >>>> +        VMSTATE_UINT32(ctx.seqnum_seed, struct igb_tx),
> >>>> +        VMSTATE_UINT32(ctx.type_tucmd_mlhl, struct igb_tx),
> >>>> +        VMSTATE_UINT32(ctx.mss_l4len_idx, struct igb_tx),
> >>>>            VMSTATE_BOOL(tse, struct igb_tx),
> >>>>            VMSTATE_BOOL(ixsm, struct igb_tx),
> >>>>            VMSTATE_BOOL(txsm, struct igb_tx), diff --git
> >>>> a/hw/net/igb_core.c b/hw/net/igb_core.c index
> >>>> a7c7bfdc75..304f5d849f
> >>>> 100644
> >>>> --- a/hw/net/igb_core.c
> >>>> +++ b/hw/net/igb_core.c
> >>>> @@ -390,7 +390,8 @@ static bool
> >>>>    igb_setup_tx_offloads(IGBCore *core, struct igb_tx *tx)  {
> >>>>        if (tx->tse) {
> >>>> -        if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, tx->mss)) {
> >>>> +        uint32_t mss = tx->ctx.mss_l4len_idx >> 16;
> >>>> +        if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true,
> >>>> + mss)) {
> >>>>                return false;
> >>>>            }
> >>>>
> >>>> @@ -550,8 +551,10 @@ igb_process_tx_desc(IGBCore *core,
> >>>>                       E1000_ADVTXD_DTYP_CTXT) {
> >>>>                /* advanced transmit context descriptor */
> >>>>                tx_ctx_desc = (struct e1000_adv_tx_context_desc *)tx_desc;
> >>>> -            tx->vlan = le32_to_cpu(tx_ctx_desc->vlan_macip_lens) >> 16;
> >>>> -            tx->mss = le32_to_cpu(tx_ctx_desc->mss_l4len_idx) >> 16;
> >>>> +            tx->ctx.vlan_macip_lens = le32_to_cpu(tx_ctx_desc-
> >>> vlan_macip_lens);
> >>>> +            tx->ctx.seqnum_seed = le32_to_cpu(tx_ctx_desc->seqnum_seed);
> >>>> +            tx->ctx.type_tucmd_mlhl = le32_to_cpu(tx_ctx_desc-
> >>>>> type_tucmd_mlhl);
> >>>> +            tx->ctx.mss_l4len_idx =
> >>>> + le32_to_cpu(tx_ctx_desc->mss_l4len_idx);
> >>>
> >>> Wouldn't it be better to parse the context into all the required
> >>> fields like vlan,
> >> mss, etc., already when handling the context descriptor, instead of
> >> parsing it for every data descriptor later?
> >>> Also, in my yet to be merged patch [1] which handles VLAN insertion
> >>> for
> >> VMDq I use the vlan field in multiple places, so it would be better
> >> to have the vlan value readily available.
> >>> [1]:
> >>> https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg00393.html
> >>
> >> If there is a better representation of the entire context descriptor
> >> we may use it as an internal use, but I think it is good enough for the
> purpose, too.
> >
> > If I understand correctly this change will help to realize "Table 7-31: Valid
> context vs Required Offload"?
> > If so, the internal representation can already be prepared to hold the
> following:
> > - vlan
> > - l4len
> > - iplen
> > - maclen
> > - mss
> > - l4type
> > - ip4
> 
> This patch is not intended to cover a specific feature, but it is intended to allow
> us to implement any (currently) uncovered features in the future. In this regard,
> all of the fields except the reserved fields and DTYP are necessary. Anyway, in
> my opinion, the original format of the context descriptor is well-suited to save
> the fields you listed.

Ok.

> 
> >
> > If we have an internal representation, the fields can hold values from the
> context, or from VMDq registers, or from the data descriptor depending on
> what the driver has configured.
> 
> Certainly if you save the context descriptor as is, you do not expect the fields of
> the saved descriptor to repesent values from other places like VMDq registers.
> However I rather see this as a more favorable feature. The driver can change
> the configuration anytime, and the effective value can change accordingly. For
> example, you may see the configuration at a time, and you overwrite the value
> saved from the context with the value provided from the configuration, the
> value of the field may be valid at the time, but if the configuration gets changed
> after that and the value from the context becomes valid again, you will no
> longer know what the effective value is. Such a situation is rare in this case, but
> still better to cover if possible.
> 
> And it seems your patch for VLAN actually has such a bug. I thought it is OK to
> overwrite the VID got from the context descriptor with another in
> igb_tx_insert_vlan() as it is EOP and the context becomes invalid anyway, but I
> have just realized nothing says the old context gets invalidated after EOP. The
> VID from the context descriptor should not be overwritten.
> 
> In general, a persistent state derived from other persistent states are prone to
> such bugs. The less error-prone way is to have a function which derives the
> value effective now.
> 
> > Anyhow, I don't want to derail this patch, you already have ACKs, please go
> ahead if you don't want to do this now.
> 
> I'll keep this moving forward, but thank you for sharing your thoughts.

Ok, please keep in mind that the driver is allowed configure multiple context descriptors with an index, so storing into just one variable will create problems.
See DPDK E1000 PMD for e.g.: https://github.com/DPDK/dpdk/blob/main/drivers/net/e1000/igb_rxtx.c#L120

> 
> For your VLAN patch, please correct the bug I described and submit a new
> version (probably after this patch is landed as your patch is affected with the
> change of vlan field.)

Yes, I realize there is a bug there, I will fix once you get this in.

> 
> Regards,
> Akihiko Odaki
> 
> >
> >>
> >> For patch [1], I think it is better to gather the logic to derive the
> >> VID into one place instead of cluttering several places with the
> >> relevant code. Concretely,
> >> igb_tx_insert_vlan() can decide whether to read the VID from the
> >> context descriptor or from VMIR, or not to read (in case VLAN insertion is
> disabled).
> >>
> >> Regards,
> >> Akihiko Odaki
> >>
> >>>
> >>>>                return;
> >>>>            } else {
> >>>>                /* unknown descriptor type */ @@ -575,8 +578,9 @@
> >>>> igb_process_tx_desc(IGBCore *core,
> >>>>        if (cmd_type_len & E1000_TXD_CMD_EOP) {
> >>>>            if (!tx->skip_cp && net_tx_pkt_parse(tx->tx_pkt)) {
> >>>>                if (cmd_type_len & E1000_TXD_CMD_VLE) {
> >>>> -                net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, tx->vlan,
> >>>> -                    core->mac[VET] & 0xffff);
> >>>> +                uint16_t vlan = tx->ctx.vlan_macip_lens >> 16;
> >>>> +                uint16_t vet = core->mac[VET] & 0xffff;
> >>>> +                net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, vlan,
> >>>> + vet);
> >>>>                }
> >>>>                if (igb_tx_pkt_send(core, tx, queue_index)) {
> >>>>                    igb_on_tx_done_update_stats(core, tx->tx_pkt);
> >>>> @@
> >>>> -4024,8
> >>>> +4028,7 @@ static void igb_reset(IGBCore *core, bool sw)
> >>>>        for (i = 0; i < ARRAY_SIZE(core->tx); i++) {
> >>>>            tx = &core->tx[i];
> >>>>            net_tx_pkt_reset(tx->tx_pkt);
> >>>> -        tx->vlan = 0;
> >>>> -        tx->mss = 0;
> >>>> +        memset(&tx->ctx, 0, sizeof(tx->ctx));
> >>>>            tx->tse = false;
> >>>>            tx->ixsm = false;
> >>>>            tx->txsm = false;
> >>>> diff --git a/hw/net/igb_core.h b/hw/net/igb_core.h index
> >>>> 814c1e264b..3483edc655 100644
> >>>> --- a/hw/net/igb_core.h
> >>>> +++ b/hw/net/igb_core.h
> >>>> @@ -72,8 +72,7 @@ struct IGBCore {
> >>>>        QEMUTimer *autoneg_timer;
> >>>>
> >>>>        struct igb_tx {
> >>>> -        uint16_t vlan;  /* VLAN Tag */
> >>>> -        uint16_t mss;   /* Maximum Segment Size */
> >>>> +        struct e1000_adv_tx_context_desc ctx;
> >>>>            bool tse;       /* TCP/UDP Segmentation Enable */
> >>>>            bool ixsm;      /* Insert IP Checksum */
> >>>>            bool txsm;      /* Insert TCP/UDP Checksum */
> >>>> --
> >>>> 2.39.2
> >>>>
> >>>