[PATCH net-next 2/3] gve: Allow ethtool to configure rx_buf_len

[PATCH net-next 2/3] gve: Allow ethtool to configure rx_buf_len

[Joshua Washington]

From: Ankit Garg <nktgrg@google.com>

Add support for getting and setting the RX buffer length via the
ethtool ring parameters (`ethtool -g`/`-G`). The driver restricts the
allowed buffer length to 2048 (SZ_2K) or 4096 (SZ_4K).

As XDP is only supported when the `rx_buf_len` is 2048, the driver now
enforces this in two places:
1.  In `gve_xdp_set`, rejecting XDP programs if the current buffer
    length is not 2048.
2.  In `gve_set_rx_buf_len_config`, rejecting buffer length changes if
    XDP is loaded and the new length is not 2048.

Signed-off-by: Ankit Garg <nktgrg@google.com>
Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com>
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Reviewed-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: Joshua Washington <joshwash@google.com>
---
 drivers/net/ethernet/google/gve/gve.h         |  9 ++++
 drivers/net/ethernet/google/gve/gve_ethtool.c | 13 +++++-
 drivers/net/ethernet/google/gve/gve_main.c    | 45 +++++++++++++++++++
 3 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index c237d00c5ab3..a33b44c1eb86 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -1167,6 +1167,12 @@ static inline bool gve_is_gqi(struct gve_priv *priv)
 		priv->queue_format == GVE_GQI_QPL_FORMAT;
 }
 
+static inline bool gve_is_dqo(struct gve_priv *priv)
+{
+	return priv->queue_format == GVE_DQO_RDA_FORMAT ||
+	       priv->queue_format == GVE_DQO_QPL_FORMAT;
+}
+
 static inline u32 gve_num_tx_queues(struct gve_priv *priv)
 {
 	return priv->tx_cfg.num_queues + priv->tx_cfg.num_xdp_queues;
@@ -1248,6 +1254,9 @@ void gve_rx_free_rings_gqi(struct gve_priv *priv,
 void gve_rx_start_ring_gqi(struct gve_priv *priv, int idx);
 void gve_rx_stop_ring_gqi(struct gve_priv *priv, int idx);
 bool gve_header_split_supported(const struct gve_priv *priv);
+int gve_set_rx_buf_len_config(struct gve_priv *priv, u32 rx_buf_len,
+			      struct netlink_ext_ack *extack,
+			      struct gve_rx_alloc_rings_cfg *rx_alloc_cfg);
 int gve_set_hsplit_config(struct gve_priv *priv, u8 tcp_data_split,
 			  struct gve_rx_alloc_rings_cfg *rx_alloc_cfg);
 /* rx buffer handling */
diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
index db6fc855a511..52500ae8348e 100644
--- a/drivers/net/ethernet/google/gve/gve_ethtool.c
+++ b/drivers/net/ethernet/google/gve/gve_ethtool.c
@@ -529,6 +529,8 @@ static void gve_get_ringparam(struct net_device *netdev,
 	cmd->rx_pending = priv->rx_desc_cnt;
 	cmd->tx_pending = priv->tx_desc_cnt;
 
+	kernel_cmd->rx_buf_len = priv->rx_cfg.packet_buffer_size;
+
 	if (!gve_header_split_supported(priv))
 		kernel_cmd->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_UNKNOWN;
 	else if (priv->header_split_enabled)
@@ -589,6 +591,12 @@ static int gve_set_ringparam(struct net_device *netdev,
 	int err;
 
 	gve_get_curr_alloc_cfgs(priv, &tx_alloc_cfg, &rx_alloc_cfg);
+
+	err = gve_set_rx_buf_len_config(priv, kernel_cmd->rx_buf_len, extack,
+					&rx_alloc_cfg);
+	if (err)
+		return err;
+
 	err = gve_set_hsplit_config(priv, kernel_cmd->tcp_data_split,
 				    &rx_alloc_cfg);
 	if (err)
@@ -605,6 +613,8 @@ static int gve_set_ringparam(struct net_device *netdev,
 			return err;
 	} else {
 		/* Set ring params for the next up */
+		priv->rx_cfg.packet_buffer_size =
+			rx_alloc_cfg.packet_buffer_size;
 		priv->header_split_enabled = rx_alloc_cfg.enable_header_split;
 		priv->tx_desc_cnt = tx_alloc_cfg.ring_size;
 		priv->rx_desc_cnt = rx_alloc_cfg.ring_size;
@@ -944,7 +954,8 @@ static int gve_get_ts_info(struct net_device *netdev,
 
 const struct ethtool_ops gve_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS,
-	.supported_ring_params = ETHTOOL_RING_USE_TCP_DATA_SPLIT,
+	.supported_ring_params = ETHTOOL_RING_USE_TCP_DATA_SPLIT |
+				 ETHTOOL_RING_USE_RX_BUF_LEN,
 	.get_drvinfo = gve_get_drvinfo,
 	.get_strings = gve_get_strings,
 	.get_sset_count = gve_get_sset_count,
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 8d825218965a..8009819c73f2 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -1722,6 +1722,13 @@ static int verify_xdp_configuration(struct net_device *dev)
 		return -EOPNOTSUPP;
 	}
 
+	if (priv->rx_cfg.packet_buffer_size != SZ_2K) {
+		netdev_warn(dev,
+			    "XDP is not supported for Rx buf len %d. Set Rx buf len to %d before using XDP.\n",
+			    priv->rx_cfg.packet_buffer_size, SZ_2K);
+		return -EOPNOTSUPP;
+	}
+
 	max_xdp_mtu = priv->rx_cfg.packet_buffer_size - sizeof(struct ethhdr);
 	if (priv->queue_format == GVE_GQI_QPL_FORMAT)
 		max_xdp_mtu -= GVE_RX_PAD;
@@ -2050,6 +2057,44 @@ bool gve_header_split_supported(const struct gve_priv *priv)
 		priv->queue_format == GVE_DQO_RDA_FORMAT && !priv->xdp_prog;
 }
 
+int gve_set_rx_buf_len_config(struct gve_priv *priv, u32 rx_buf_len,
+			      struct netlink_ext_ack *extack,
+			      struct gve_rx_alloc_rings_cfg *rx_alloc_cfg)
+{
+	u32 old_rx_buf_len = rx_alloc_cfg->packet_buffer_size;
+
+	if (rx_buf_len == old_rx_buf_len)
+		return 0;
+
+	if (!gve_is_dqo(priv)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Modifying Rx buf len is only supported with DQO format");
+		return -EOPNOTSUPP;
+	}
+
+	if (priv->xdp_prog && rx_buf_len != SZ_2K) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Rx buf len can only be 2048 when XDP is on");
+		return -EINVAL;
+	}
+
+	if (rx_buf_len > priv->max_rx_buffer_size) {
+		NL_SET_ERR_MSG_FMT_MOD(extack,
+				       "Rx buf len exceeds the max supported value of %u",
+				       priv->max_rx_buffer_size);
+		return -EINVAL;
+	}
+
+	if (rx_buf_len != SZ_2K && rx_buf_len != SZ_4K) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Rx buf len can only be 2048 or 4096");
+		return -EINVAL;
+	}
+	rx_alloc_cfg->packet_buffer_size = rx_buf_len;
+
+	return 0;
+}
+
 int gve_set_hsplit_config(struct gve_priv *priv, u8 tcp_data_split,
 			  struct gve_rx_alloc_rings_cfg *rx_alloc_cfg)
 {
-- 
2.51.1.814.gb8fa24458f-goog

Re: [PATCH net-next 2/3] gve: Allow ethtool to configure rx_buf_len

[Jakub Kicinski]

On Wed, 22 Oct 2025 11:22:24 -0700 Joshua Washington wrote:
> +	if (priv->rx_cfg.packet_buffer_size != SZ_2K) {
> +		netdev_warn(dev,
> +			    "XDP is not supported for Rx buf len %d. Set Rx buf len to %d before using XDP.\n",
> +			    priv->rx_cfg.packet_buffer_size, SZ_2K);
> +		return -EOPNOTSUPP;
> +	}

Please plumb extack thru to here. It's inside struct netdev_bpf

>  	max_xdp_mtu = priv->rx_cfg.packet_buffer_size - sizeof(struct ethhdr);
>  	if (priv->queue_format == GVE_GQI_QPL_FORMAT)
>  		max_xdp_mtu -= GVE_RX_PAD;
> @@ -2050,6 +2057,44 @@ bool gve_header_split_supported(const struct gve_priv *priv)
>  		priv->queue_format == GVE_DQO_RDA_FORMAT && !priv->xdp_prog;
>  }
>  
> +int gve_set_rx_buf_len_config(struct gve_priv *priv, u32 rx_buf_len,
> +			      struct netlink_ext_ack *extack,
> +			      struct gve_rx_alloc_rings_cfg *rx_alloc_cfg)
> +{
> +	u32 old_rx_buf_len = rx_alloc_cfg->packet_buffer_size;
> +
> +	if (rx_buf_len == old_rx_buf_len)
> +		return 0;
> +
> +	if (!gve_is_dqo(priv)) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Modifying Rx buf len is only supported with DQO format");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (priv->xdp_prog && rx_buf_len != SZ_2K) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Rx buf len can only be 2048 when XDP is on");
> +		return -EINVAL;
> +	}
> +
> +	if (rx_buf_len > priv->max_rx_buffer_size) {

This check looks kinda pointless given the check right below against
the exact sizes?

> +		NL_SET_ERR_MSG_FMT_MOD(extack,
> +				       "Rx buf len exceeds the max supported value of %u",
> +				       priv->max_rx_buffer_size);
> +		return -EINVAL;
> +	}
> +
> +	if (rx_buf_len != SZ_2K && rx_buf_len != SZ_4K) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Rx buf len can only be 2048 or 4096");
> +		return -EINVAL;
> +	}
> +	rx_alloc_cfg->packet_buffer_size = rx_buf_len;
> +
> +	return 0;
> +}

Re: [PATCH net-next 2/3] gve: Allow ethtool to configure rx_buf_len

[Ankit Garg]

On Thu, Oct 23, 2025 at 5:14 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 22 Oct 2025 11:22:24 -0700 Joshua Washington wrote:
> > +     if (priv->rx_cfg.packet_buffer_size != SZ_2K) {
> > +             netdev_warn(dev,
> > +                         "XDP is not supported for Rx buf len %d. Set Rx buf len to %d before using XDP.\n",
> > +                         priv->rx_cfg.packet_buffer_size, SZ_2K);
> > +             return -EOPNOTSUPP;
> > +     }
>
> Please plumb extack thru to here. It's inside struct netdev_bpf
>

Using extack just for this log will make it inconsistent with other
logs in this method. Would it be okay if I send a fast follow patch to
use exstack in this method and others?

> >       max_xdp_mtu = priv->rx_cfg.packet_buffer_size - sizeof(struct ethhdr);
> >       if (priv->queue_format == GVE_GQI_QPL_FORMAT)
> >               max_xdp_mtu -= GVE_RX_PAD;
> > @@ -2050,6 +2057,44 @@ bool gve_header_split_supported(const struct gve_priv *priv)
> >               priv->queue_format == GVE_DQO_RDA_FORMAT && !priv->xdp_prog;
> >  }
> >
> > +int gve_set_rx_buf_len_config(struct gve_priv *priv, u32 rx_buf_len,
> > +                           struct netlink_ext_ack *extack,
> > +                           struct gve_rx_alloc_rings_cfg *rx_alloc_cfg)
> > +{
> > +     u32 old_rx_buf_len = rx_alloc_cfg->packet_buffer_size;
> > +
> > +     if (rx_buf_len == old_rx_buf_len)
> > +             return 0;
> > +
> > +     if (!gve_is_dqo(priv)) {
> > +             NL_SET_ERR_MSG_MOD(extack,
> > +                                "Modifying Rx buf len is only supported with DQO format");
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> > +     if (priv->xdp_prog && rx_buf_len != SZ_2K) {
> > +             NL_SET_ERR_MSG_MOD(extack,
> > +                                "Rx buf len can only be 2048 when XDP is on");
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (rx_buf_len > priv->max_rx_buffer_size) {
>
> This check looks kinda pointless given the check right below against
> the exact sizes?
>

My intent was to code defensively against device accidently advertising
anything in [2k+1,4k) as max buffer size. I will remove this check.

> > +             NL_SET_ERR_MSG_FMT_MOD(extack,
> > +                                    "Rx buf len exceeds the max supported value of %u",
> > +                                    priv->max_rx_buffer_size);
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (rx_buf_len != SZ_2K && rx_buf_len != SZ_4K) {
> > +             NL_SET_ERR_MSG_MOD(extack,
> > +                                "Rx buf len can only be 2048 or 4096");
> > +             return -EINVAL;
> > +     }
> > +     rx_alloc_cfg->packet_buffer_size = rx_buf_len;
> > +
> > +     return 0;
> > +}

Re: [PATCH net-next 2/3] gve: Allow ethtool to configure rx_buf_len

[Jakub Kicinski]

On Fri, 24 Oct 2025 11:17:04 -0700 Ankit Garg wrote:
> > Please plumb extack thru to here. It's inside struct netdev_bpf
> 
> Using extack just for this log will make it inconsistent with other
> logs in this method. Would it be okay if I send a fast follow patch to
> use exstack in this method and others?

Could you make it part of this series, tho?

Re: [PATCH net-next 2/3] gve: Allow ethtool to configure rx_buf_len

[Ankit Garg]

On Fri, Oct 24, 2025 at 1:10 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 24 Oct 2025 11:17:04 -0700 Ankit Garg wrote:
> > > Please plumb extack thru to here. It's inside struct netdev_bpf
> >
> > Using extack just for this log will make it inconsistent with other
> > logs in this method. Would it be okay if I send a fast follow patch to
> > use exstack in this method and others?
>
> Could you make it part of this series, tho?

Absolutely. Will include in v2.

Re: [PATCH net-next 2/3] gve: Allow ethtool to configure rx_buf_len

[Ankit Garg]

On Fri, Oct 24, 2025 at 11:17 AM Ankit Garg <nktgrg@google.com> wrote:
>
> On Thu, Oct 23, 2025 at 5:14 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Wed, 22 Oct 2025 11:22:24 -0700 Joshua Washington wrote:
> > > +     if (priv->rx_cfg.packet_buffer_size != SZ_2K) {
> > > +             netdev_warn(dev,
> > > +                         "XDP is not supported for Rx buf len %d. Set Rx buf len to %d before using XDP.\n",
> > > +                         priv->rx_cfg.packet_buffer_size, SZ_2K);
> > > +             return -EOPNOTSUPP;
> > > +     }
> >
> > Please plumb extack thru to here. It's inside struct netdev_bpf
> >
>
> Using extack just for this log will make it inconsistent with other
> logs in this method. Would it be okay if I send a fast follow patch to
> use exstack in this method and others?
>
> > >       max_xdp_mtu = priv->rx_cfg.packet_buffer_size - sizeof(struct ethhdr);
> > >       if (priv->queue_format == GVE_GQI_QPL_FORMAT)
> > >               max_xdp_mtu -= GVE_RX_PAD;
> > > @@ -2050,6 +2057,44 @@ bool gve_header_split_supported(const struct gve_priv *priv)
> > >               priv->queue_format == GVE_DQO_RDA_FORMAT && !priv->xdp_prog;
> > >  }
> > >
> > > +int gve_set_rx_buf_len_config(struct gve_priv *priv, u32 rx_buf_len,
> > > +                           struct netlink_ext_ack *extack,
> > > +                           struct gve_rx_alloc_rings_cfg *rx_alloc_cfg)
> > > +{
> > > +     u32 old_rx_buf_len = rx_alloc_cfg->packet_buffer_size;
> > > +
> > > +     if (rx_buf_len == old_rx_buf_len)
> > > +             return 0;
> > > +
> > > +     if (!gve_is_dqo(priv)) {
> > > +             NL_SET_ERR_MSG_MOD(extack,
> > > +                                "Modifying Rx buf len is only supported with DQO format");
> > > +             return -EOPNOTSUPP;
> > > +     }
> > > +
> > > +     if (priv->xdp_prog && rx_buf_len != SZ_2K) {
> > > +             NL_SET_ERR_MSG_MOD(extack,
> > > +                                "Rx buf len can only be 2048 when XDP is on");
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     if (rx_buf_len > priv->max_rx_buffer_size) {
> >
> > This check looks kinda pointless given the check right below against
> > the exact sizes?
> >
>
> My intent was to code defensively against device accidently advertising
> anything in [2k+1,4k) as max buffer size. I will remove this check.
>

After taking another look, an additional check is still needed to
handle scenario when device doesn't advertise support for 4k buffers.
I reworked this check (and added a comment) in v2 which hopefully
conveys the intent better.


> > > +             NL_SET_ERR_MSG_FMT_MOD(extack,
> > > +                                    "Rx buf len exceeds the max supported value of %u",
> > > +                                    priv->max_rx_buffer_size);
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     if (rx_buf_len != SZ_2K && rx_buf_len != SZ_4K) {
> > > +             NL_SET_ERR_MSG_MOD(extack,
> > > +                                "Rx buf len can only be 2048 or 4096");
> > > +             return -EINVAL;
> > > +     }
> > > +     rx_alloc_cfg->packet_buffer_size = rx_buf_len;
> > > +
> > > +     return 0;
> > > +}