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