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