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

[Akihiko Odaki <akihiko.odaki@daynix.com>]

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
>>
>