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