[v2] net/ixgbe: check jumbo frame enable parameter
Checks
Commit Message
There is necessary to do some check of max packet size boundary
for code safe in order to avoid error in register setting
when enable jumbo.
Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
---
v2:
-add more log infomation and fix build issue
---
drivers/net/ixgbe/ixgbe_ethdev.c | 4 +---
drivers/net/ixgbe/ixgbe_ethdev.h | 2 ++
drivers/net/ixgbe/ixgbe_rxtx.c | 31 +++++++++++++++++++++++++++++++
3 files changed, 34 insertions(+), 3 deletions(-)
Comments
Add the background for this patch.
https://mails.dpdk.org/archives/dev/2018-November/117771.html
https://mails.dpdk.org/archives/dev/2018-November/117772.html
> -----Original Message-----
> From: Zhao1, Wei
> Sent: Thursday, November 1, 2018 4:29 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>;
> yamashita.hideyuki@po.ntt-tx.co.jp; stephen@networkplumber.org
> Subject: [PATCH v2] net/ixgbe: check jumbo frame enable parameter
>
> There is necessary to do some check of max packet size boundary for code
> safe in order to avoid error in register setting when enable jumbo.
>
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
>
> ---
>
> v2:
> -add more log infomation and fix build issue
> ---
> drivers/net/ixgbe/ixgbe_ethdev.c | 4 +---
> drivers/net/ixgbe/ixgbe_ethdev.h | 2 ++
> drivers/net/ixgbe/ixgbe_rxtx.c | 31
> +++++++++++++++++++++++++++++++
> 3 files changed, 34 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 5a2c351..d65a911 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -173,8 +173,6 @@ static int
> ixgbe_dev_queue_stats_mapping_set(struct rte_eth_dev *eth_dev,
> uint8_t is_rx);
> static int ixgbe_fw_version_get(struct rte_eth_dev *dev, char *fw_version,
> size_t fw_size);
> -static void ixgbe_dev_info_get(struct rte_eth_dev *dev,
> - struct rte_eth_dev_info *dev_info);
> static const uint32_t *ixgbe_dev_supported_ptypes_get(struct
> rte_eth_dev *dev); static void ixgbevf_dev_info_get(struct rte_eth_dev
> *dev,
> struct rte_eth_dev_info *dev_info); @@ -
> 3692,7 +3690,7 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused
> struct rte_eth_dev *dev,
> return 0;
> }
>
> -static void
> +void
> ixgbe_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info
> *dev_info) {
> struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev); diff --
> git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
> index d0b9396..a474be4 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.h
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.h
> @@ -727,6 +727,8 @@ int ixgbe_action_rss_same(const struct
> rte_flow_action_rss *comp,
> const struct rte_flow_action_rss *with); int
> ixgbe_config_rss_filter(struct rte_eth_dev *dev,
> struct ixgbe_rte_flow_rss_conf *conf, bool add);
> +void ixgbe_dev_info_get(struct rte_eth_dev *dev,
> + struct rte_eth_dev_info *dev_info);
>
> static inline int
> ixgbe_ethertype_filter_lookup(struct ixgbe_filter_info *filter_info, diff --git
> a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c index
> 2f0262a..94051ca 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -4814,6 +4814,34 @@ void __attribute__((cold))
> return 0;
> }
>
> +static int __attribute__((cold))
> +ixgbe_dev_jumboenable_check(struct rte_eth_dev *dev,
> + uint16_t max_rx_pkt_len)
> +{
> + struct rte_eth_dev_info dev_info;
> +
> + ixgbe_dev_info_get(dev, &dev_info);
> +
> + /* check that max packet size is within the allowed range */
> + if (max_rx_pkt_len < ETHER_MIN_MTU) {
> + PMD_INIT_LOG(ERR, "max packet size is too small.");
> + return -EINVAL;
> + }
> +
> + if (max_rx_pkt_len > dev_info.max_rx_pktlen) {
> + PMD_INIT_LOG(ERR, "max packet size is too big.");
> + return -EINVAL;
> + }
> +
> + /* check jumbo mode if needed */
> + if (max_rx_pkt_len < ETHER_MAX_LEN) {
> + PMD_INIT_LOG(ERR, "No need to enable jumbo.");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> /*
> * Initializes Receive Unit.
> */
> @@ -4865,6 +4893,9 @@ int __attribute__((cold))
> * Configure jumbo frame support, if any.
> */
> if (rx_conf->offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) {
> + if (ixgbe_dev_jumboenable_check(dev, rx_conf-
> >max_rx_pkt_len))
> + return -EINVAL;
> +
> hlreg0 |= IXGBE_HLREG0_JUMBOEN;
> maxfrs = IXGBE_READ_REG(hw, IXGBE_MAXFRS);
> maxfrs &= 0x0000FFFF;
> --
> 1.8.3.1
Hi Wei:
> -----Original Message-----
> From: Zhao1, Wei
> Sent: Thursday, November 1, 2018 1:40 AM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>;
> yamashita.hideyuki@po.ntt-tx.co.jp; stephen@networkplumber.org
> Subject: RE: [PATCH v2] net/ixgbe: check jumbo frame enable parameter
>
> Add the background for this patch.
> https://mails.dpdk.org/archives/dev/2018-November/117771.html
> https://mails.dpdk.org/archives/dev/2018-November/117772.html
>
>
>
> > -----Original Message-----
> > From: Zhao1, Wei
> > Sent: Thursday, November 1, 2018 4:29 PM
> > To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>;
> > yamashita.hideyuki@po.ntt-tx.co.jp; stephen@networkplumber.org
> > Subject: [PATCH v2] net/ixgbe: check jumbo frame enable parameter
> >
> > There is necessary to do some check of max packet size boundary for
> > code safe in order to avoid error in register setting when enable jumbo.
> >
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> >
> > ---
> >
> > v2:
> > -add more log infomation and fix build issue
> > ---
> > drivers/net/ixgbe/ixgbe_ethdev.c | 4 +---
> > drivers/net/ixgbe/ixgbe_ethdev.h | 2 ++
> > drivers/net/ixgbe/ixgbe_rxtx.c | 31
> > +++++++++++++++++++++++++++++++
> > 3 files changed, 34 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index 5a2c351..d65a911 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -173,8 +173,6 @@ static int
> > ixgbe_dev_queue_stats_mapping_set(struct rte_eth_dev *eth_dev,
> > uint8_t is_rx);
> > static int ixgbe_fw_version_get(struct rte_eth_dev *dev, char
> *fw_version,
> > size_t fw_size);
> > -static void ixgbe_dev_info_get(struct rte_eth_dev *dev,
> > - struct rte_eth_dev_info *dev_info);
> > static const uint32_t *ixgbe_dev_supported_ptypes_get(struct
> > rte_eth_dev *dev); static void ixgbevf_dev_info_get(struct
> > rte_eth_dev *dev,
> > struct rte_eth_dev_info *dev_info); @@ -
> > 3692,7 +3690,7 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused
> > struct rte_eth_dev *dev,
> > return 0;
> > }
> >
> > -static void
> > +void
> > ixgbe_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info
> > *dev_info) {
> > struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev); diff --
> > git a/drivers/net/ixgbe/ixgbe_ethdev.h
> > b/drivers/net/ixgbe/ixgbe_ethdev.h
> > index d0b9396..a474be4 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.h
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.h
> > @@ -727,6 +727,8 @@ int ixgbe_action_rss_same(const struct
> > rte_flow_action_rss *comp,
> > const struct rte_flow_action_rss *with); int
> > ixgbe_config_rss_filter(struct rte_eth_dev *dev,
> > struct ixgbe_rte_flow_rss_conf *conf, bool add);
> > +void ixgbe_dev_info_get(struct rte_eth_dev *dev,
> > + struct rte_eth_dev_info *dev_info);
> >
> > static inline int
> > ixgbe_ethertype_filter_lookup(struct ixgbe_filter_info *filter_info,
> > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c
> > b/drivers/net/ixgbe/ixgbe_rxtx.c index 2f0262a..94051ca 100644
> > --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> > +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> > @@ -4814,6 +4814,34 @@ void __attribute__((cold))
> > return 0;
> > }
> >
> > +static int __attribute__((cold))
> > +ixgbe_dev_jumboenable_check(struct rte_eth_dev *dev,
> > + uint16_t max_rx_pkt_len)
it's better to return Boolean for this function
> > +{
> > + struct rte_eth_dev_info dev_info;
> > +
> > + ixgbe_dev_info_get(dev, &dev_info);
> > +
> > + /* check that max packet size is within the allowed range */
> > + if (max_rx_pkt_len < ETHER_MIN_MTU) {
> > + PMD_INIT_LOG(ERR, "max packet size is too small.");
> > + return -EINVAL;
> > + }
Why this is only needed for DEV_RX_OFFLOAD_JUMBO_FRAME?
And we will compare to ETHER_MAX_LEN which is a bigger value, this looks redundant.
> > +
> > + if (max_rx_pkt_len > dev_info.max_rx_pktlen) {
> > + PMD_INIT_LOG(ERR, "max packet size is too big.");
> > + return -EINVAL;
> > + }
dev_info.max_rx_pktlen = 15872, it should be replaced by a macro, then we don't need to call ixgbe_dev_info_get here.
and why this is only needed for DEV_RX_OFFLOAD_JUMBO_FRAME?
> > +
> > + /* check jumbo mode if needed */
> > + if (max_rx_pkt_len < ETHER_MAX_LEN) {
> > + PMD_INIT_LOG(ERR, "No need to enable jumbo.");
> > + return -EINVAL;
And I agree with Stephen's comment,
Looks all of these is common for all drivers, it should be moved to ether layer.
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /*
> > * Initializes Receive Unit.
> > */
> > @@ -4865,6 +4893,9 @@ int __attribute__((cold))
> > * Configure jumbo frame support, if any.
> > */
> > if (rx_conf->offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) {
> > + if (ixgbe_dev_jumboenable_check(dev, rx_conf-
> > >max_rx_pkt_len))
> > + return -EINVAL;
> > +
> > hlreg0 |= IXGBE_HLREG0_JUMBOEN;
> > maxfrs = IXGBE_READ_REG(hw, IXGBE_MAXFRS);
> > maxfrs &= 0x0000FFFF;
> > --
> > 1.8.3.1
> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Tuesday, November 6, 2018 12:34 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; yamashita.hideyuki@po.ntt-tx.co.jp;
> stephen@networkplumber.org
> Subject: RE: [PATCH v2] net/ixgbe: check jumbo frame enable parameter
>
> Hi Wei:
>
> > -----Original Message-----
> > From: Zhao1, Wei
> > Sent: Thursday, November 1, 2018 1:40 AM
> > To: dev@dpdk.org
> > Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>;
> > yamashita.hideyuki@po.ntt-tx.co.jp; stephen@networkplumber.org
> > Subject: RE: [PATCH v2] net/ixgbe: check jumbo frame enable parameter
> >
> > Add the background for this patch.
> > https://mails.dpdk.org/archives/dev/2018-November/117771.html
> > https://mails.dpdk.org/archives/dev/2018-November/117772.html
> >
> >
> >
> > > -----Original Message-----
> > > From: Zhao1, Wei
> > > Sent: Thursday, November 1, 2018 4:29 PM
> > > To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> > > Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>;
> > > yamashita.hideyuki@po.ntt-tx.co.jp; stephen@networkplumber.org
> > > Subject: [PATCH v2] net/ixgbe: check jumbo frame enable parameter
> > >
> > > There is necessary to do some check of max packet size boundary for
> > > code safe in order to avoid error in register setting when enable jumbo.
> > >
> > > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > >
> > > ---
> > >
> > > v2:
> > > -add more log infomation and fix build issue
> > > ---
> > > drivers/net/ixgbe/ixgbe_ethdev.c | 4 +---
> > > drivers/net/ixgbe/ixgbe_ethdev.h | 2 ++
> > > drivers/net/ixgbe/ixgbe_rxtx.c | 31
> > > +++++++++++++++++++++++++++++++
> > > 3 files changed, 34 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > index 5a2c351..d65a911 100644
> > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > @@ -173,8 +173,6 @@ static int
> > > ixgbe_dev_queue_stats_mapping_set(struct rte_eth_dev *eth_dev,
> > > uint8_t is_rx);
> > > static int ixgbe_fw_version_get(struct rte_eth_dev *dev, char
> > *fw_version,
> > > size_t fw_size);
> > > -static void ixgbe_dev_info_get(struct rte_eth_dev *dev,
> > > - struct rte_eth_dev_info *dev_info);
> > > static const uint32_t *ixgbe_dev_supported_ptypes_get(struct
> > > rte_eth_dev *dev); static void ixgbevf_dev_info_get(struct
> > > rte_eth_dev *dev,
> > > struct rte_eth_dev_info *dev_info); @@ -
> > > 3692,7 +3690,7 @@ static int
> > > ixgbevf_dev_xstats_get_names(__rte_unused
> > > struct rte_eth_dev *dev,
> > > return 0;
> > > }
> > >
> > > -static void
> > > +void
> > > ixgbe_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info
> > > *dev_info) {
> > > struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev); diff --
> > > git a/drivers/net/ixgbe/ixgbe_ethdev.h
> > > b/drivers/net/ixgbe/ixgbe_ethdev.h
> > > index d0b9396..a474be4 100644
> > > --- a/drivers/net/ixgbe/ixgbe_ethdev.h
> > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.h
> > > @@ -727,6 +727,8 @@ int ixgbe_action_rss_same(const struct
> > > rte_flow_action_rss *comp,
> > > const struct rte_flow_action_rss *with); int
> > > ixgbe_config_rss_filter(struct rte_eth_dev *dev,
> > > struct ixgbe_rte_flow_rss_conf *conf, bool add);
> > > +void ixgbe_dev_info_get(struct rte_eth_dev *dev,
> > > + struct rte_eth_dev_info *dev_info);
> > >
> > > static inline int
> > > ixgbe_ethertype_filter_lookup(struct ixgbe_filter_info
> > > *filter_info, diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c
> > > b/drivers/net/ixgbe/ixgbe_rxtx.c index 2f0262a..94051ca 100644
> > > --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> > > +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> > > @@ -4814,6 +4814,34 @@ void __attribute__((cold))
> > > return 0;
> > > }
> > >
> > > +static int __attribute__((cold))
> > > +ixgbe_dev_jumboenable_check(struct rte_eth_dev *dev,
> > > + uint16_t max_rx_pkt_len)
>
> it's better to return Boolean for this function
Ok
>
> > > +{
> > > + struct rte_eth_dev_info dev_info;
> > > +
> > > + ixgbe_dev_info_get(dev, &dev_info);
> > > +
> > > + /* check that max packet size is within the allowed range */
> > > + if (max_rx_pkt_len < ETHER_MIN_MTU) {
> > > + PMD_INIT_LOG(ERR, "max packet size is too small.");
> > > + return -EINVAL;
> > > + }
>
> Why this is only needed for DEV_RX_OFFLOAD_JUMBO_FRAME?
> And we will compare to ETHER_MAX_LEN which is a bigger value, this looks
> redundant.
Ok, I will check that
>
> > > +
> > > + if (max_rx_pkt_len > dev_info.max_rx_pktlen) {
> > > + PMD_INIT_LOG(ERR, "max packet size is too big.");
> > > + return -EINVAL;
> > > + }
>
> dev_info.max_rx_pktlen = 15872, it should be replaced by a macro, then we
> don't need to call ixgbe_dev_info_get here.
> and why this is only needed for DEV_RX_OFFLOAD_JUMBO_FRAME?
> > > +
> > > + /* check jumbo mode if needed */
> > > + if (max_rx_pkt_len < ETHER_MAX_LEN) {
> > > + PMD_INIT_LOG(ERR, "No need to enable jumbo.");
> > > + return -EINVAL;
>
> And I agree with Stephen's comment,
> Looks all of these is common for all drivers, it should be moved to ether layer.
Ok, I will move to rte layer
>
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > /*
> > > * Initializes Receive Unit.
> > > */
> > > @@ -4865,6 +4893,9 @@ int __attribute__((cold))
> > > * Configure jumbo frame support, if any.
> > > */
> > > if (rx_conf->offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) {
> > > + if (ixgbe_dev_jumboenable_check(dev, rx_conf-
> > > >max_rx_pkt_len))
> > > + return -EINVAL;
> > > +
> > > hlreg0 |= IXGBE_HLREG0_JUMBOEN;
> > > maxfrs = IXGBE_READ_REG(hw, IXGBE_MAXFRS);
> > > maxfrs &= 0x0000FFFF;
> > > --
> > > 1.8.3.1
@@ -173,8 +173,6 @@ static int ixgbe_dev_queue_stats_mapping_set(struct rte_eth_dev *eth_dev,
uint8_t is_rx);
static int ixgbe_fw_version_get(struct rte_eth_dev *dev, char *fw_version,
size_t fw_size);
-static void ixgbe_dev_info_get(struct rte_eth_dev *dev,
- struct rte_eth_dev_info *dev_info);
static const uint32_t *ixgbe_dev_supported_ptypes_get(struct rte_eth_dev *dev);
static void ixgbevf_dev_info_get(struct rte_eth_dev *dev,
struct rte_eth_dev_info *dev_info);
@@ -3692,7 +3690,7 @@ static int ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev,
return 0;
}
-static void
+void
ixgbe_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
{
struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
@@ -727,6 +727,8 @@ int ixgbe_action_rss_same(const struct rte_flow_action_rss *comp,
const struct rte_flow_action_rss *with);
int ixgbe_config_rss_filter(struct rte_eth_dev *dev,
struct ixgbe_rte_flow_rss_conf *conf, bool add);
+void ixgbe_dev_info_get(struct rte_eth_dev *dev,
+ struct rte_eth_dev_info *dev_info);
static inline int
ixgbe_ethertype_filter_lookup(struct ixgbe_filter_info *filter_info,
@@ -4814,6 +4814,34 @@ void __attribute__((cold))
return 0;
}
+static int __attribute__((cold))
+ixgbe_dev_jumboenable_check(struct rte_eth_dev *dev,
+ uint16_t max_rx_pkt_len)
+{
+ struct rte_eth_dev_info dev_info;
+
+ ixgbe_dev_info_get(dev, &dev_info);
+
+ /* check that max packet size is within the allowed range */
+ if (max_rx_pkt_len < ETHER_MIN_MTU) {
+ PMD_INIT_LOG(ERR, "max packet size is too small.");
+ return -EINVAL;
+ }
+
+ if (max_rx_pkt_len > dev_info.max_rx_pktlen) {
+ PMD_INIT_LOG(ERR, "max packet size is too big.");
+ return -EINVAL;
+ }
+
+ /* check jumbo mode if needed */
+ if (max_rx_pkt_len < ETHER_MAX_LEN) {
+ PMD_INIT_LOG(ERR, "No need to enable jumbo.");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
/*
* Initializes Receive Unit.
*/
@@ -4865,6 +4893,9 @@ int __attribute__((cold))
* Configure jumbo frame support, if any.
*/
if (rx_conf->offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) {
+ if (ixgbe_dev_jumboenable_check(dev, rx_conf->max_rx_pkt_len))
+ return -EINVAL;
+
hlreg0 |= IXGBE_HLREG0_JUMBOEN;
maxfrs = IXGBE_READ_REG(hw, IXGBE_MAXFRS);
maxfrs &= 0x0000FFFF;