Re: [PATCH net-next v3 04/15] gve: add struct gve_device_info to hold device properties

[Harshitha Ramamurthy]

On Thu, Aug 6, 2026 at 12:19 AM Przemek Kitszel
<przemyslaw.kitszel@xxxxxxxxx> wrote:
>
> On 8/3/26 20:46, Harshitha Ramamurthy wrote:
> > In the current AdminQ mode, device properties are written into
> > struct gve_device_descriptor that is allocated in shared memory
> > between the driver and device. In the upcoming MailboxQ mode,
> > these properties will be returned in the response of a mailbox
> > message. Hence, add struct gve_device_info as the structure that
> > holds all the properties that are negotiated with the device in
> > either mode.
> >
> > Change the AdminQ mode method gve_adminq_describe_device()
> > and its children to fill up device information into this newly
> > introduced struct gve_device_info. Move a few helper functions
> > and code that set device properties in the priv structure into
> > gve_init_priv(). So now gve_init_priv() calls/does the following:
> >
> > - gve_set_mtu()
> > - gve_set_mac()
> > - gve_set_queue_properties()
> > - gve_set_buf_sizes()
> > - set flow steering and RSS properties
> > - set other priv properties
> >
> > When MailboxQ support is added, device information will be filled
> > into the same structure and the same gve_init_priv() path would be
> > used to set device properties to ensure common code reusage.
> >
> > These changes are refactors only, no functional change.
> >
> > Reviewed-by: Willem de Bruijn <willemb@xxxxxxxxxx>
> > Reviewed-by: Jordan Rhee <jordanrhee@xxxxxxxxxx>
> > Signed-off-by: Harshitha Ramamurthy <hramamurthy@xxxxxxxxxx>
> > ---
> > Changes in v3:
> > - Read default_min_ring_size from device info instead of priv
> >
> >   drivers/net/ethernet/google/gve/gve.h        |  29 +++++
> >   drivers/net/ethernet/google/gve/gve_adminq.c | 115 +++++++++++--------
> >   drivers/net/ethernet/google/gve/gve_adminq.h |   6 -
> >   drivers/net/ethernet/google/gve/gve_main.c   | 101 +++++++++++-----
> >   4 files changed, 170 insertions(+), 81 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> > index c280ff35ee77..021adb9108df 100644
> > --- a/drivers/net/ethernet/google/gve/gve.h
> > +++ b/drivers/net/ethernet/google/gve/gve.h
> > @@ -797,6 +797,34 @@ struct gve_ptp {
> >       struct gve_priv *priv;
> >   };
> >
> > +struct gve_device_info {
> > +     enum gve_queue_format queue_format;
> > +     u16 default_tx_queues;
> > +     u16 default_rx_queues;
> > +     u16 max_tx_queues;
> > +     u16 max_rx_queues;
> > +     u16 default_tx_ring_size;
> > +     u16 default_rx_ring_size;
> > +     u16 max_tx_ring_size;
> > +     u16 max_rx_ring_size;
> > +     u16 min_tx_ring_size;
> > +     u16 min_rx_ring_size;
> > +     u16 max_mtu;
> > +     u8 mac[ETH_ALEN];
> > +     u16 max_rx_buffer_size;
> > +     u16 header_buf_size;
> > +     u32 max_flow_rules;
> > +     u16 rss_key_size;
> > +     u16 rss_lut_size;
> > +     u16 tx_pages_per_qpl;
> > +     u16 num_event_counters;
> > +     u64 max_registered_pages;
> > +     bool default_min_ring_size;
> > +     bool nic_timestamp_supported;
> > +     bool modify_ring_size_enabled;
> > +     bool cache_rss_config;
> > +};
>
> you could save some bytes of the struct size by reordering fields

I did run pahole on this struct previously:

$pahole -C gve_device_info gve.ko
struct gve_device_info {
enum gve_queue_format      queue_format;         /*     0     4 */
u16                        default_tx_queues;    /*     4     2 */
u16                        default_rx_queues;    /*     6     2 */
u16                        max_tx_queues;        /*     8     2 */
u16                        max_rx_queues;        /*    10     2 */
u16                        default_tx_ring_size; /*    12     2 */
u16                        default_rx_ring_size; /*    14     2 */
u16                        max_tx_ring_size;     /*    16     2 */
u16                        max_rx_ring_size;     /*    18     2 */
u16                        min_tx_ring_size;     /*    20     2 */
u16                        min_rx_ring_size;     /*    22     2 */
u16                        max_mtu;              /*    24     2 */
u8                         mac[6];               /*    26     6 */
u16                        max_rx_buffer_size;   /*    32     2 */
u16                        header_buf_size;      /*    34     2 */
u32                        max_flow_rules;       /*    36     4 */
u16                        rss_key_size;         /*    40     2 */
u16                        rss_lut_size;         /*    42     2 */
u16                        tx_pages_per_qpl;     /*    44     2 */
u16                        num_event_counters;   /*    46     2 */
u64                        max_registered_pages; /*    48     8 */
bool                       default_min_ring_size; /*    56     1 */
bool                       nic_timestamp_supported; /*    57     1 */
bool                       modify_ring_size_enabled; /*    58     1 */
bool                       cache_rss_config;     /*    59     1 */

/* size: 64, cachelines: 1, members: 25 */
/* padding: 4 */
};

>
> > +
> >   struct gve_priv {
> >       struct net_device *dev;
> >       struct gve_tx_ring *tx; /* array of tx_cfg.num_queues */
> > @@ -929,6 +957,7 @@ struct gve_priv {
> >       struct gve_nic_ts_report *nic_ts_report;
> >       dma_addr_t nic_ts_report_bus;
> >       u64 last_sync_nic_counter; /* Clock counter from last NIC TS report */
> > +     struct gve_device_info device_info;
> >   };
>
>
> [..]
>
> > -int gve_set_mtu(struct gve_priv *priv,
> > -             struct gve_device_descriptor *descriptor)
> > +static int gve_set_mtu(struct gve_priv *priv)
> >   {
> > +     struct gve_device_info *device_info = &priv->device_info;
> >       u16 mtu;
> >
> > -     mtu = be16_to_cpu(descriptor->mtu);
> > +     mtu = device_info->max_mtu;
> >       if (mtu < ETH_MIN_MTU) {
> >               dev_err(&priv->pdev->dev, "MTU %d below minimum MTU\n", mtu);
> >               return -EINVAL;
> >       }
> >       priv->dev->max_mtu = mtu;
> > +     priv->dev->mtu = priv->dev->max_mtu;
>
> nit: I would just use `= mtu`
>
> >
> >       return 0;
> >   }
> >
> > -void gve_set_mac(struct gve_priv *priv,
> > -              struct gve_device_descriptor *descriptor)
> > +static void gve_set_mac(struct gve_priv *priv)
> >   {
> > -     eth_hw_addr_set(priv->dev, descriptor->mac);
> > -     dev_info(&priv->pdev->dev, "MAC addr: %pM\n", descriptor->mac);
> > +     struct gve_device_info *device_info = &priv->device_info;
> > +
> > +     eth_hw_addr_set(priv->dev, device_info->mac);
> > +     dev_info(&priv->pdev->dev, "MAC addr: %pM\n", device_info->mac);
> > +}
>
> whole function is rewritten (in git sense), and it was just added in
> previous commit
>
> one way to avoid that is to extract "mac" variable in the prev patch,
> and here only update the assignement and function signature

Makes sense, will be easier to review the diff. Will make this change.

>
> > +
> > +static void gve_set_buf_sizes(struct gve_priv *priv)
> > +{
> > +     struct gve_device_info *device_info = &priv->device_info;
> > +
> > +     if (device_info->max_rx_buffer_size > priv->max_rx_buffer_size)
> > +             priv->max_rx_buffer_size = device_info->max_rx_buffer_size;
> > +
> > +     if (gve_is_dqo(priv) &&
> > +         priv->max_rx_buffer_size > GVE_DEFAULT_RX_BUFFER_SIZE)
> > +             priv->rx_cfg.packet_buffer_size = priv->max_rx_buffer_size;
> > +
> > +     if (device_info->header_buf_size)
> > +             priv->header_buf_size = device_info->header_buf_size;
> >   }
> >
> >   static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
> >   {
> > +     struct gve_device_info *device_info = &priv->device_info;
> >       int err;
> >
> >       /* Set up the adminq */
> > @@ -2514,7 +2533,7 @@ static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
> >       if (skip_describe_device)
> >               goto setup_device;
> >
> > -     priv->queue_format = GVE_QUEUE_FORMAT_UNSPECIFIED;
> > +     device_info->queue_format = GVE_QUEUE_FORMAT_UNSPECIFIED;
> >       /* Get the initial information we need from the device */
> >       err = gve_adminq_describe_device(priv);
> >       if (err) {
> > @@ -2523,6 +2542,8 @@ static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
> >               goto err;
> >       }
> >
> > +     priv->queue_format = priv->device_info.queue_format;
> > +
> >       err = gve_set_num_ntfy_blks(priv);
> >       if (err) {
> >               dev_err(&priv->pdev->dev,
> > @@ -2546,12 +2567,34 @@ static int gve_init_priv(struct gve_priv *priv, bool skip_describe_device)
> >               netif_set_tso_max_size(priv->dev, GVE_DQO_TX_MAX);
> >       }
> >
> > -     priv->dev->mtu = priv->dev->max_mtu;
> > +     if (gve_set_mtu(priv)) {
> > +             err = -EINVAL;
> > +             goto err;
> > +     }
> > +
> > +     priv->num_event_counters = device_info->num_event_counters;
> > +
> > +     gve_set_mac(priv);
> > +
> > +     gve_set_queue_properties(priv);
> > +     priv->modify_ring_size_enabled = device_info->modify_ring_size_enabled;
> > +
> > +     gve_set_buf_sizes(priv);
> > +
> > +     priv->max_flow_rules = device_info->max_flow_rules;
> > +     if (priv->max_flow_rules)
> > +             priv->dev->hw_features |= NETIF_F_NTUPLE;
> > +
> > +     priv->rss_key_size = device_info->rss_key_size;
> > +     priv->rss_lut_size = device_info->rss_lut_size;
> > +     priv->cache_rss_config = device_info->cache_rss_config;
>
> I don't know, especially with your MailboxQ coming next, if following
> makes sense, but I wonder why you have to copy whole device info into
> priv (or IOW, hold two copies of most params)?

For the upcoming mailboxQ control plane path, device properties will
be negotiated with the device before the priv structure is available.
Hence this struct is introduced to hold device properties. Also the
idea is that gve_device_info contains properties negotiated
with/returned by the device while priv tracks active driver
configuration properties.

>
> Alternative will be to use variables from device info everywhere,
> you could even achieve that without any code changes via struct_group
>
> > +
> >       priv->numa_node = dev_to_node(&priv->pdev->dev);
> >       priv->tx_cfg.num_xdp_queues = 0;
> >       priv->rx_copybreak = GVE_DEFAULT_RX_COPYBREAK;
> >       priv->ts_config.tx_type = HWTSTAMP_TX_OFF;
> >       priv->ts_config.rx_filter = HWTSTAMP_FILTER_NONE;
> > +     priv->nic_timestamp_supported = device_info->nic_timestamp_supported;
> >
> >   setup_device:
> >       priv->xsk_pools = bitmap_zalloc(priv->rx_cfg.max_queues, GFP_KERNEL);
>