Re: [PATCH v4 5/5] igb: packet-split descriptors support

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

On 2023/05/04 22:10, Tomasz Dzieciol wrote:
> Packet-split descriptors are used by Linux VF driver for MTU values from 2048
> 
> Signed-off-by: Tomasz Dzieciol <t.dzieciol@partner.samsung.com>
> ---
>   hw/net/igb_core.c   | 368 ++++++++++++++++++++++++++++++++++++++------
>   hw/net/igb_regs.h   |   8 +
>   hw/net/trace-events |   2 +-
>   3 files changed, 332 insertions(+), 46 deletions(-)
> 
> diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
> index 8c0291665f..9c1a2fa136 100644
> --- a/hw/net/igb_core.c
> +++ b/hw/net/igb_core.c
> @@ -276,6 +276,20 @@ typedef struct E1000ERingInfo {
>       int idx;
>   } E1000ERingInfo;
>   
> +static uint32_t
> +igb_rx_queue_desctyp_get(IGBCore *core, const E1000ERingInfo *r)
> +{
> +    return core->mac[E1000_SRRCTL(r->idx) >> 2] & E1000_SRRCTL_DESCTYPE_MASK;
> +}
> +
> +static bool
> +igb_rx_use_ps_descriptor(IGBCore *core, const E1000ERingInfo *r)
> +{
> +    uint32_t desctyp = igb_rx_queue_desctyp_get(core, r);
> +    return desctyp == E1000_SRRCTL_DESCTYPE_HDR_SPLIT ||
> +           desctyp == E1000_SRRCTL_DESCTYPE_HDR_SPLIT_ALWAYS;
> +}
> +
>   static inline bool
>   igb_rss_enabled(IGBCore *core)
>   {
> @@ -1233,21 +1247,70 @@ igb_read_lgcy_rx_descr(IGBCore *core, struct e1000_rx_desc *desc,
>   }
>   
>   static inline void
> -igb_read_adv_rx_descr(IGBCore *core, union e1000_adv_rx_desc *desc,
> -                      hwaddr *buff_addr)
> +igb_read_adv_rx_single_buf_descr(IGBCore *core, union e1000_adv_rx_desc *desc,
> +                                 hwaddr *buff_addr)
>   {
>       *buff_addr = le64_to_cpu(desc->read.pkt_addr);
>   }
>   
>   static inline void
> -igb_read_rx_descr(IGBCore *core, union e1000_rx_desc_union *desc,
> -                  hwaddr *buff_addr)
> +igb_read_adv_rx_split_buf_descr(IGBCore *core, union e1000_adv_rx_desc *desc,
> +                                hwaddr *buff_addr)
>   {
> +    buff_addr[0] = le64_to_cpu(desc->read.hdr_addr);
> +    buff_addr[1] = le64_to_cpu(desc->read.pkt_addr);
> +}
> +
> +typedef struct IGBBaState {
> +    uint16_t written[IGB_MAX_PS_BUFFERS];
> +    uint8_t cur_idx;
> +} IGBBaState;

This struct derives from e1000e so you should rename the corresponding 
struct of e1000e.

> +
> +typedef struct IGBPacketRxDMAState {
> +    size_t size;
> +    size_t total_size;
> +    size_t ps_hdr_len;
> +    size_t desc_size;
> +    size_t desc_offset;
> +    uint32_t rx_desc_packet_buf_size;
> +    uint32_t rx_desc_header_buf_size;
> +    struct iovec *iov;
> +    size_t iov_ofs;
> +    bool do_ps;
> +    bool is_first;
> +    IGBBaState bastate;
> +    hwaddr ba[IGB_MAX_PS_BUFFERS];

I meant this should not be an array but instead should be defined as a 
struct as it is in the "read" member of union e1000_adv_rx_desc. It's 
defined in a way different from e1000e's extended packet split 
descriptor (union e1000_rx_desc_packet_split) and that is based on the 
differences of notations in e1000e's and igb's datasheets so I want you 
to respect that.

Also, this definition is moved from the place where it is defined at 
first in an earlier patch, but I suggest to place it here in the earlier 
patch to keep this patch concise unless there is something to prevent that.