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

[PATCH for 8.0] 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>
---
 hw/net/igb.c      |  6 ++++--
 hw/net/igb_core.c | 17 ++++++++++-------
 hw/net/igb_core.h |  3 +--
 3 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/hw/net/igb.c b/hw/net/igb.c
index 0792626322..50239a7cb1 100644
--- a/hw/net/igb.c
+++ b/hw/net/igb.c
@@ -499,8 +499,10 @@ static const VMStateDescription igb_vmstate_tx = {
     .version_id = 1,
     .minimum_version_id = 1,
     .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 41d1abae03..dbe24739d0 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;
         }
 
@@ -551,8 +552,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 */
@@ -576,8 +579,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);
@@ -4027,8 +4031,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, NULL);
-        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] igb: Save the entire Tx context descriptor

[Philippe Mathieu-Daudé]

On 16/3/23 13:28, 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>
> ---
>   hw/net/igb.c      |  6 ++++--
>   hw/net/igb_core.c | 17 ++++++++++-------
>   hw/net/igb_core.h |  3 +--
>   3 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/net/igb.c b/hw/net/igb.c
> index 0792626322..50239a7cb1 100644
> --- a/hw/net/igb.c
> +++ b/hw/net/igb.c
> @@ -499,8 +499,10 @@ static const VMStateDescription igb_vmstate_tx = {
>       .version_id = 1,

Don't we need to increment the vmstate version? See
https://qemu-project.gitlab.io/qemu/devel/migration.html#versions

>       .minimum_version_id = 1,
>       .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 41d1abae03..dbe24739d0 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;
>           }
>   
> @@ -551,8 +552,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 */
> @@ -576,8 +579,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);
> @@ -4027,8 +4031,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, NULL);
> -        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 */

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

[Akihiko Odaki]

On 2023/03/16 21:36, Philippe Mathieu-Daudé wrote:
> On 16/3/23 13:28, 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>
>> ---
>>   hw/net/igb.c      |  6 ++++--
>>   hw/net/igb_core.c | 17 ++++++++++-------
>>   hw/net/igb_core.h |  3 +--
>>   3 files changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/net/igb.c b/hw/net/igb.c
>> index 0792626322..50239a7cb1 100644
>> --- a/hw/net/igb.c
>> +++ b/hw/net/igb.c
>> @@ -499,8 +499,10 @@ static const VMStateDescription igb_vmstate_tx = {
>>       .version_id = 1,
> 
> Don't we need to increment the vmstate version? See
> https://qemu-project.gitlab.io/qemu/devel/migration.html#versions

This device is added only a week ago so it shouldn't need version bump. 
That is also why I tagged this change "for 8.0".

> 
>>       .minimum_version_id = 1,
>>       .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 41d1abae03..dbe24739d0 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;
>>           }
>> @@ -551,8 +552,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 */
>> @@ -576,8 +579,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);
>> @@ -4027,8 +4031,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, NULL);
>> -        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 */
> 

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

[Philippe Mathieu-Daudé]

On 16/3/23 13:40, Akihiko Odaki wrote:
> On 2023/03/16 21:36, Philippe Mathieu-Daudé wrote:
>> On 16/3/23 13:28, 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>
>>> ---
>>>   hw/net/igb.c      |  6 ++++--
>>>   hw/net/igb_core.c | 17 ++++++++++-------
>>>   hw/net/igb_core.h |  3 +--
>>>   3 files changed, 15 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/hw/net/igb.c b/hw/net/igb.c
>>> index 0792626322..50239a7cb1 100644
>>> --- a/hw/net/igb.c
>>> +++ b/hw/net/igb.c
>>> @@ -499,8 +499,10 @@ static const VMStateDescription igb_vmstate_tx = {
>>>       .version_id = 1,
>>
>> Don't we need to increment the vmstate version? See
>> https://qemu-project.gitlab.io/qemu/devel/migration.html#versions
> 
> This device is added only a week ago so it shouldn't need version bump. 
> That is also why I tagged this change "for 8.0".

Well it is cheaper than dealing with partially backported commits...
Also could be a better example for future developers IMHO. My 2 cents.

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

[Juan Quintela]

Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> On 16/3/23 13:40, Akihiko Odaki wrote:
>> On 2023/03/16 21:36, Philippe Mathieu-Daudé wrote:
>>> On 16/3/23 13:28, 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>
>>>> ---
>>>>   hw/net/igb.c      |  6 ++++--
>>>>   hw/net/igb_core.c | 17 ++++++++++-------
>>>>   hw/net/igb_core.h |  3 +--
>>>>   3 files changed, 15 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/hw/net/igb.c b/hw/net/igb.c
>>>> index 0792626322..50239a7cb1 100644
>>>> --- a/hw/net/igb.c
>>>> +++ b/hw/net/igb.c
>>>> @@ -499,8 +499,10 @@ static const VMStateDescription igb_vmstate_tx = {
>>>>       .version_id = 1,
>>>
>>> Don't we need to increment the vmstate version? See
>>> https://qemu-project.gitlab.io/qemu/devel/migration.html#versions
>> This device is added only a week ago so it shouldn't need version
>> bump. That is also why I tagged this change "for 8.0".
>
> Well it is cheaper than dealing with partially backported commits...
> Also could be a better example for future developers IMHO. My 2 cents.

You can't have everything O:-)

I would just bump the version and not do the "dance" where you can
migrate from v1 and v2.  I.e. don't add tests at all.

This way bisect will fail with the correct message.

Later, Juan.