| Message ID | 1528138710-31848-1-git-send-email-shannon.nelson@oracle.com |
|---|---|
| State | Superseded |
| Delegated to: | Jeff Kirsher |
| Headers | show |
| Series | [net-queue] ixgbe: check that ipsec is available on the chip | expand |
On Mon, Jun 4, 2018 at 11:58 AM, Shannon Nelson <shannon.nelson@oracle.com> wrote: > Check the writability of the IPsec configuration registers > before setting up the offload. > > Fixes: 49a94d74d948 ("ixgbe: add ipsec engine start and stop routines") > Reported-by: Andre Tomt <andre@tomt.net> > Cc: Alexander Duyck <alexander.h.duyck@intel.com> > Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com> > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c > index 344a1f2..003b53f 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c > @@ -210,7 +210,10 @@ static void ixgbe_ipsec_stop_engine(struct ixgbe_adapter *adapter) > struct ixgbe_hw *hw = &adapter->hw; > u32 reg; > > - ixgbe_ipsec_stop_data(adapter); > + /* stop data if it has been running */ > + reg = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); > + if (!(reg & IXGBE_SECTXCTRL_SECTX_DIS)) > + ixgbe_ipsec_stop_data(adapter); > > /* disable Rx and Tx SA lookup */ > IXGBE_WRITE_REG(hw, IXGBE_IPSTXIDX, 0); Instead of doing this here why not look at doing it in ixgbe_ipsec_stop_data itself. That way you cover both places where this gets called. > @@ -966,12 +969,27 @@ void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring, > **/ > void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter) > { > + struct ixgbe_hw *hw = &adapter->hw; > struct ixgbe_ipsec *ipsec; > size_t size; > + u32 reg1, reg2; > > if (adapter->hw.mac.type == ixgbe_mac_82598EB) > return; > > + /* verify that the ipsec offload is available by checking > + * the writability of the engine DISable bit - can we clear > + * the bit? If not, don't set up ipsec. If yes, the put > + * it back and continue the setup. > + */ > + reg1 = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); > + reg2 = reg1 & ~IXGBE_SECTXCTRL_SECTX_DIS; > + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, reg2); > + reg2 = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); > + if (reg2 & IXGBE_SECTXCTRL_SECTX_DIS) > + return; > + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, reg1); > + Take a look in SECTXSTAT and SECRXSTAT. I think there is are bits (SECRX_OFF_DIS & SECTX_OFF_DIS) there that tell you if the offload is disabled via the strapping pin. > ipsec = kzalloc(sizeof(*ipsec), GFP_KERNEL); > if (!ipsec) > goto err1; > -- > 2.7.4 > > _______________________________________________ > Intel-wired-lan mailing list > Intel-wired-lan@osuosl.org > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
On 6/4/2018 2:44 PM, Alexander Duyck wrote: > On Mon, Jun 4, 2018 at 11:58 AM, Shannon Nelson > <shannon.nelson@oracle.com> wrote: >> Check the writability of the IPsec configuration registers >> before setting up the offload. >> >> Fixes: 49a94d74d948 ("ixgbe: add ipsec engine start and stop routines") >> Reported-by: Andre Tomt <andre@tomt.net> >> Cc: Alexander Duyck <alexander.h.duyck@intel.com> >> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com> >> --- >> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 20 +++++++++++++++++++- >> 1 file changed, 19 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >> index 344a1f2..003b53f 100644 >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >> @@ -210,7 +210,10 @@ static void ixgbe_ipsec_stop_engine(struct ixgbe_adapter *adapter) >> struct ixgbe_hw *hw = &adapter->hw; >> u32 reg; >> >> - ixgbe_ipsec_stop_data(adapter); >> + /* stop data if it has been running */ >> + reg = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); >> + if (!(reg & IXGBE_SECTXCTRL_SECTX_DIS)) >> + ixgbe_ipsec_stop_data(adapter); >> >> /* disable Rx and Tx SA lookup */ >> IXGBE_WRITE_REG(hw, IXGBE_IPSTXIDX, 0); > > Instead of doing this here why not look at doing it in > ixgbe_ipsec_stop_data itself. That way you cover both places where > this gets called. Sure. > >> @@ -966,12 +969,27 @@ void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring, >> **/ >> void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter) >> { >> + struct ixgbe_hw *hw = &adapter->hw; >> struct ixgbe_ipsec *ipsec; >> size_t size; >> + u32 reg1, reg2; >> >> if (adapter->hw.mac.type == ixgbe_mac_82598EB) >> return; >> >> + /* verify that the ipsec offload is available by checking >> + * the writability of the engine DISable bit - can we clear >> + * the bit? If not, don't set up ipsec. If yes, the put >> + * it back and continue the setup. >> + */ >> + reg1 = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); >> + reg2 = reg1 & ~IXGBE_SECTXCTRL_SECTX_DIS; >> + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, reg2); >> + reg2 = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); >> + if (reg2 & IXGBE_SECTXCTRL_SECTX_DIS) >> + return; >> + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, reg1); >> + > > Take a look in SECTXSTAT and SECRXSTAT. I think there is are bits > (SECRX_OFF_DIS & SECTX_OFF_DIS) there that tell you if the offload is > disabled via the strapping pin. Yeah, that would be good. It looks like there's a problem with SECnXSTAT bit definitions in ixgbe_types.h: for both RX and TX the ECC_nXERR is wrong and there is no definition for SECnX_OFF_DIS. In light of a separate discussion regarding shared code patches, perhaps I could ask Jeff or you to fix that file so we can use those bits? I'm out of time today, will look in again late tonight. sln > >> ipsec = kzalloc(sizeof(*ipsec), GFP_KERNEL); >> if (!ipsec) >> goto err1; >> -- >> 2.7.4 >> >> _______________________________________________ >> Intel-wired-lan mailing list >> Intel-wired-lan@osuosl.org >> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
On Mon, Jun 4, 2018 at 4:00 PM, Shannon Nelson <shannon.nelson@oracle.com> wrote: > On 6/4/2018 2:44 PM, Alexander Duyck wrote: >> >> On Mon, Jun 4, 2018 at 11:58 AM, Shannon Nelson >> <shannon.nelson@oracle.com> wrote: >>> >>> Check the writability of the IPsec configuration registers >>> before setting up the offload. >>> >>> Fixes: 49a94d74d948 ("ixgbe: add ipsec engine start and stop routines") >>> Reported-by: Andre Tomt <andre@tomt.net> >>> Cc: Alexander Duyck <alexander.h.duyck@intel.com> >>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com> >>> --- >>> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 20 >>> +++++++++++++++++++- >>> 1 file changed, 19 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >>> index 344a1f2..003b53f 100644 >>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >>> @@ -210,7 +210,10 @@ static void ixgbe_ipsec_stop_engine(struct >>> ixgbe_adapter *adapter) >>> struct ixgbe_hw *hw = &adapter->hw; >>> u32 reg; >>> >>> - ixgbe_ipsec_stop_data(adapter); >>> + /* stop data if it has been running */ >>> + reg = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); >>> + if (!(reg & IXGBE_SECTXCTRL_SECTX_DIS)) >>> + ixgbe_ipsec_stop_data(adapter); >>> >>> /* disable Rx and Tx SA lookup */ >>> IXGBE_WRITE_REG(hw, IXGBE_IPSTXIDX, 0); >> >> >> Instead of doing this here why not look at doing it in >> ixgbe_ipsec_stop_data itself. That way you cover both places where >> this gets called. > > > Sure. > > >> >>> @@ -966,12 +969,27 @@ void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring, >>> **/ >>> void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter) >>> { >>> + struct ixgbe_hw *hw = &adapter->hw; >>> struct ixgbe_ipsec *ipsec; >>> size_t size; >>> + u32 reg1, reg2; >>> >>> if (adapter->hw.mac.type == ixgbe_mac_82598EB) >>> return; >>> >>> + /* verify that the ipsec offload is available by checking >>> + * the writability of the engine DISable bit - can we clear >>> + * the bit? If not, don't set up ipsec. If yes, the put >>> + * it back and continue the setup. >>> + */ >>> + reg1 = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); >>> + reg2 = reg1 & ~IXGBE_SECTXCTRL_SECTX_DIS; >>> + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, reg2); >>> + reg2 = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); >>> + if (reg2 & IXGBE_SECTXCTRL_SECTX_DIS) >>> + return; >>> + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, reg1); >>> + >> >> >> Take a look in SECTXSTAT and SECRXSTAT. I think there is are bits >> (SECRX_OFF_DIS & SECTX_OFF_DIS) there that tell you if the offload is >> disabled via the strapping pin. > > > Yeah, that would be good. > > It looks like there's a problem with SECnXSTAT bit definitions in > ixgbe_types.h: for both RX and TX the ECC_nXERR is wrong and there is no > definition for SECnX_OFF_DIS. In light of a separate discussion regarding > shared code patches, perhaps I could ask Jeff or you to fix that file so we > can use those bits? > > I'm out of time today, will look in again late tonight. > > sln I got started but didn't have time to finish it. I will have 2 more patches I will submit tomorrow morning to address this and another issue I found. Thanks. - Alex
Can we add ipsec state to "ethtool -k"? So we run "ethtool -k" to check ipsec offload enabled or not. Zhu Yanjun On Tue, Jun 5, 2018 at 2:58 AM, Shannon Nelson <shannon.nelson@oracle.com> wrote: > Check the writability of the IPsec configuration registers > before setting up the offload. > > Fixes: 49a94d74d948 ("ixgbe: add ipsec engine start and stop routines") > Reported-by: Andre Tomt <andre@tomt.net> > Cc: Alexander Duyck <alexander.h.duyck@intel.com> > Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com> > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c > index 344a1f2..003b53f 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c > @@ -210,7 +210,10 @@ static void ixgbe_ipsec_stop_engine(struct ixgbe_adapter *adapter) > struct ixgbe_hw *hw = &adapter->hw; > u32 reg; > > - ixgbe_ipsec_stop_data(adapter); > + /* stop data if it has been running */ > + reg = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); > + if (!(reg & IXGBE_SECTXCTRL_SECTX_DIS)) > + ixgbe_ipsec_stop_data(adapter); > > /* disable Rx and Tx SA lookup */ > IXGBE_WRITE_REG(hw, IXGBE_IPSTXIDX, 0); > @@ -966,12 +969,27 @@ void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring, > **/ > void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter) > { > + struct ixgbe_hw *hw = &adapter->hw; > struct ixgbe_ipsec *ipsec; > size_t size; > + u32 reg1, reg2; > > if (adapter->hw.mac.type == ixgbe_mac_82598EB) > return; > > + /* verify that the ipsec offload is available by checking > + * the writability of the engine DISable bit - can we clear > + * the bit? If not, don't set up ipsec. If yes, the put > + * it back and continue the setup. > + */ > + reg1 = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); > + reg2 = reg1 & ~IXGBE_SECTXCTRL_SECTX_DIS; > + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, reg2); > + reg2 = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); > + if (reg2 & IXGBE_SECTXCTRL_SECTX_DIS) > + return; > + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, reg1); > + > ipsec = kzalloc(sizeof(*ipsec), GFP_KERNEL); > if (!ipsec) > goto err1; > -- > 2.7.4 > > _______________________________________________ > Intel-wired-lan mailing list > Intel-wired-lan@osuosl.org > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
If ipsec offload is not enabled, can we run "ethtool -K" to enable it in ixgbe(82599)? And also, we can run "ethtool -k" to check ixgbe support ipsec offload or not. Then the user can know the ixgbe chip support ipsec offload or not before he comes accross a bug. Zhu Yanjun On Tue, Jun 5, 2018 at 11:25 AM, zhuyj <zyjzyj2000@gmail.com> wrote: > Can we add ipsec state to "ethtool -k"? So we run "ethtool -k" to > check ipsec offload enabled or not. > > Zhu Yanjun > > On Tue, Jun 5, 2018 at 2:58 AM, Shannon Nelson > <shannon.nelson@oracle.com> wrote: >> Check the writability of the IPsec configuration registers >> before setting up the offload. >> >> Fixes: 49a94d74d948 ("ixgbe: add ipsec engine start and stop routines") >> Reported-by: Andre Tomt <andre@tomt.net> >> Cc: Alexander Duyck <alexander.h.duyck@intel.com> >> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com> >> --- >> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 20 +++++++++++++++++++- >> 1 file changed, 19 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >> index 344a1f2..003b53f 100644 >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >> @@ -210,7 +210,10 @@ static void ixgbe_ipsec_stop_engine(struct ixgbe_adapter *adapter) >> struct ixgbe_hw *hw = &adapter->hw; >> u32 reg; >> >> - ixgbe_ipsec_stop_data(adapter); >> + /* stop data if it has been running */ >> + reg = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); >> + if (!(reg & IXGBE_SECTXCTRL_SECTX_DIS)) >> + ixgbe_ipsec_stop_data(adapter); >> >> /* disable Rx and Tx SA lookup */ >> IXGBE_WRITE_REG(hw, IXGBE_IPSTXIDX, 0); >> @@ -966,12 +969,27 @@ void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring, >> **/ >> void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter) >> { >> + struct ixgbe_hw *hw = &adapter->hw; >> struct ixgbe_ipsec *ipsec; >> size_t size; >> + u32 reg1, reg2; >> >> if (adapter->hw.mac.type == ixgbe_mac_82598EB) >> return; >> >> + /* verify that the ipsec offload is available by checking >> + * the writability of the engine DISable bit - can we clear >> + * the bit? If not, don't set up ipsec. If yes, the put >> + * it back and continue the setup. >> + */ >> + reg1 = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); >> + reg2 = reg1 & ~IXGBE_SECTXCTRL_SECTX_DIS; >> + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, reg2); >> + reg2 = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); >> + if (reg2 & IXGBE_SECTXCTRL_SECTX_DIS) >> + return; >> + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, reg1); >> + >> ipsec = kzalloc(sizeof(*ipsec), GFP_KERNEL); >> if (!ipsec) >> goto err1; >> -- >> 2.7.4 >> >> _______________________________________________ >> Intel-wired-lan mailing list >> Intel-wired-lan@osuosl.org >> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
On 6/4/2018 8:25 PM, zhuyj wrote: > Can we add ipsec state to "ethtool -k"? So we run "ethtool -k" to > check ipsec offload enabled or not. It is already there - see # ethtool -k ens1f0 | grep esp tx-esp-segmentation: on [fixed] esp-hw-offload: on [fixed] esp-tx-csum-hw-offload: on [fixed] sln > > Zhu Yanjun > > On Tue, Jun 5, 2018 at 2:58 AM, Shannon Nelson > <shannon.nelson@oracle.com> wrote: >> Check the writability of the IPsec configuration registers >> before setting up the offload. >> >> Fixes: 49a94d74d948 ("ixgbe: add ipsec engine start and stop routines") >> Reported-by: Andre Tomt <andre@tomt.net> >> Cc: Alexander Duyck <alexander.h.duyck@intel.com> >> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com> >> --- >> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 20 +++++++++++++++++++- >> 1 file changed, 19 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >> index 344a1f2..003b53f 100644 >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >> @@ -210,7 +210,10 @@ static void ixgbe_ipsec_stop_engine(struct ixgbe_adapter *adapter) >> struct ixgbe_hw *hw = &adapter->hw; >> u32 reg; >> >> - ixgbe_ipsec_stop_data(adapter); >> + /* stop data if it has been running */ >> + reg = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); >> + if (!(reg & IXGBE_SECTXCTRL_SECTX_DIS)) >> + ixgbe_ipsec_stop_data(adapter); >> >> /* disable Rx and Tx SA lookup */ >> IXGBE_WRITE_REG(hw, IXGBE_IPSTXIDX, 0); >> @@ -966,12 +969,27 @@ void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring, >> **/ >> void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter) >> { >> + struct ixgbe_hw *hw = &adapter->hw; >> struct ixgbe_ipsec *ipsec; >> size_t size; >> + u32 reg1, reg2; >> >> if (adapter->hw.mac.type == ixgbe_mac_82598EB) >> return; >> >> + /* verify that the ipsec offload is available by checking >> + * the writability of the engine DISable bit - can we clear >> + * the bit? If not, don't set up ipsec. If yes, the put >> + * it back and continue the setup. >> + */ >> + reg1 = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); >> + reg2 = reg1 & ~IXGBE_SECTXCTRL_SECTX_DIS; >> + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, reg2); >> + reg2 = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); >> + if (reg2 & IXGBE_SECTXCTRL_SECTX_DIS) >> + return; >> + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, reg1); >> + >> ipsec = kzalloc(sizeof(*ipsec), GFP_KERNEL); >> if (!ipsec) >> goto err1; >> -- >> 2.7.4 >> >> _______________________________________________ >> Intel-wired-lan mailing list >> Intel-wired-lan@osuosl.org >> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
In the mainline kernel source code, it seems that the above does not exist. Do you use the latest ixgbe source code from e1000 maillist? And can we use "ethtool -K" to enable/disable ipsec offload? On Tue, Jun 5, 2018 at 3:40 PM, Shannon Nelson <shannon.nelson@oracle.com> wrote: > On 6/4/2018 8:25 PM, zhuyj wrote: >> >> Can we add ipsec state to "ethtool -k"? So we run "ethtool -k" to >> check ipsec offload enabled or not. > > > It is already there - see > # ethtool -k ens1f0 | grep esp > tx-esp-segmentation: on [fixed] > esp-hw-offload: on [fixed] > esp-tx-csum-hw-offload: on [fixed] > > sln > > >> >> Zhu Yanjun >> >> On Tue, Jun 5, 2018 at 2:58 AM, Shannon Nelson >> <shannon.nelson@oracle.com> wrote: >>> >>> Check the writability of the IPsec configuration registers >>> before setting up the offload. >>> >>> Fixes: 49a94d74d948 ("ixgbe: add ipsec engine start and stop routines") >>> Reported-by: Andre Tomt <andre@tomt.net> >>> Cc: Alexander Duyck <alexander.h.duyck@intel.com> >>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com> >>> --- >>> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 20 >>> +++++++++++++++++++- >>> 1 file changed, 19 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >>> index 344a1f2..003b53f 100644 >>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >>> @@ -210,7 +210,10 @@ static void ixgbe_ipsec_stop_engine(struct >>> ixgbe_adapter *adapter) >>> struct ixgbe_hw *hw = &adapter->hw; >>> u32 reg; >>> >>> - ixgbe_ipsec_stop_data(adapter); >>> + /* stop data if it has been running */ >>> + reg = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); >>> + if (!(reg & IXGBE_SECTXCTRL_SECTX_DIS)) >>> + ixgbe_ipsec_stop_data(adapter); >>> >>> /* disable Rx and Tx SA lookup */ >>> IXGBE_WRITE_REG(hw, IXGBE_IPSTXIDX, 0); >>> @@ -966,12 +969,27 @@ void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring, >>> **/ >>> void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter) >>> { >>> + struct ixgbe_hw *hw = &adapter->hw; >>> struct ixgbe_ipsec *ipsec; >>> size_t size; >>> + u32 reg1, reg2; >>> >>> if (adapter->hw.mac.type == ixgbe_mac_82598EB) >>> return; >>> >>> + /* verify that the ipsec offload is available by checking >>> + * the writability of the engine DISable bit - can we clear >>> + * the bit? If not, don't set up ipsec. If yes, the put >>> + * it back and continue the setup. >>> + */ >>> + reg1 = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); >>> + reg2 = reg1 & ~IXGBE_SECTXCTRL_SECTX_DIS; >>> + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, reg2); >>> + reg2 = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); >>> + if (reg2 & IXGBE_SECTXCTRL_SECTX_DIS) >>> + return; >>> + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, reg1); >>> + >>> ipsec = kzalloc(sizeof(*ipsec), GFP_KERNEL); >>> if (!ipsec) >>> goto err1; >>> -- >>> 2.7.4 >>> >>> _______________________________________________ >>> Intel-wired-lan mailing list >>> Intel-wired-lan@osuosl.org >>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
On 6/5/2018 5:10 AM, zhuyj wrote: > In the mainline kernel source code, it seems that the above does not > exist. Do you use the latest ixgbe source code from e1000 maillist? > And can we use "ethtool -K" to enable/disable ipsec offload? I believe it is in Linux v4.15 and later. Be sure to enable CONFIG_INET_ESP_OFFLOAD and CONFIG_INET6_ESP_OFFLOAD to enable all the IPsec offload features. Since the output shows "[fixed]", the user is not able to change it on the fly. sln > > On Tue, Jun 5, 2018 at 3:40 PM, Shannon Nelson > <shannon.nelson@oracle.com> wrote: >> On 6/4/2018 8:25 PM, zhuyj wrote: >>> >>> Can we add ipsec state to "ethtool -k"? So we run "ethtool -k" to >>> check ipsec offload enabled or not. >> >> >> It is already there - see >> # ethtool -k ens1f0 | grep esp >> tx-esp-segmentation: on [fixed] >> esp-hw-offload: on [fixed] >> esp-tx-csum-hw-offload: on [fixed] >> >> sln >> >> >>> >>> Zhu Yanjun >>> >>> On Tue, Jun 5, 2018 at 2:58 AM, Shannon Nelson >>> <shannon.nelson@oracle.com> wrote: >>>> >>>> Check the writability of the IPsec configuration registers >>>> before setting up the offload. >>>> >>>> Fixes: 49a94d74d948 ("ixgbe: add ipsec engine start and stop routines") >>>> Reported-by: Andre Tomt <andre@tomt.net> >>>> Cc: Alexander Duyck <alexander.h.duyck@intel.com> >>>> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com> >>>> --- >>>> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 20 >>>> +++++++++++++++++++- >>>> 1 file changed, 19 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >>>> index 344a1f2..003b53f 100644 >>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >>>> @@ -210,7 +210,10 @@ static void ixgbe_ipsec_stop_engine(struct >>>> ixgbe_adapter *adapter) >>>> struct ixgbe_hw *hw = &adapter->hw; >>>> u32 reg; >>>> >>>> - ixgbe_ipsec_stop_data(adapter); >>>> + /* stop data if it has been running */ >>>> + reg = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); >>>> + if (!(reg & IXGBE_SECTXCTRL_SECTX_DIS)) >>>> + ixgbe_ipsec_stop_data(adapter); >>>> >>>> /* disable Rx and Tx SA lookup */ >>>> IXGBE_WRITE_REG(hw, IXGBE_IPSTXIDX, 0); >>>> @@ -966,12 +969,27 @@ void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring, >>>> **/ >>>> void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter) >>>> { >>>> + struct ixgbe_hw *hw = &adapter->hw; >>>> struct ixgbe_ipsec *ipsec; >>>> size_t size; >>>> + u32 reg1, reg2; >>>> >>>> if (adapter->hw.mac.type == ixgbe_mac_82598EB) >>>> return; >>>> >>>> + /* verify that the ipsec offload is available by checking >>>> + * the writability of the engine DISable bit - can we clear >>>> + * the bit? If not, don't set up ipsec. If yes, the put >>>> + * it back and continue the setup. >>>> + */ >>>> + reg1 = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); >>>> + reg2 = reg1 & ~IXGBE_SECTXCTRL_SECTX_DIS; >>>> + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, reg2); >>>> + reg2 = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); >>>> + if (reg2 & IXGBE_SECTXCTRL_SECTX_DIS) >>>> + return; >>>> + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, reg1); >>>> + >>>> ipsec = kzalloc(sizeof(*ipsec), GFP_KERNEL); >>>> if (!ipsec) >>>> goto err1; >>>> -- >>>> 2.7.4 >>>> >>>> _______________________________________________ >>>> Intel-wired-lan mailing list >>>> Intel-wired-lan@osuosl.org >>>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
On Tue, Jun 5, 2018 at 9:22 AM, Shannon Nelson <shannon.nelson@oracle.com> wrote: > On 6/5/2018 5:10 AM, zhuyj wrote: >> >> In the mainline kernel source code, it seems that the above does not >> exist. Do you use the latest ixgbe source code from e1000 maillist? >> And can we use "ethtool -K" to enable/disable ipsec offload? > > > I believe it is in Linux v4.15 and later. Be sure to enable > CONFIG_INET_ESP_OFFLOAD and CONFIG_INET6_ESP_OFFLOAD to enable all the IPsec > offload features. > > Since the output shows "[fixed]", the user is not able to change it on the > fly. > > sln I'm pretty sure the "[fixed]" was due to a bug. Specifically we were setting the bits in hw_enc_features instead of hw_features. I fixed that in the patch set I submitted yesterday. - Alex
On 6/5/2018 9:55 AM, Alexander Duyck wrote: > On Tue, Jun 5, 2018 at 9:22 AM, Shannon Nelson > <shannon.nelson@oracle.com> wrote: >> On 6/5/2018 5:10 AM, zhuyj wrote: >>> >>> In the mainline kernel source code, it seems that the above does not >>> exist. Do you use the latest ixgbe source code from e1000 maillist? >>> And can we use "ethtool -K" to enable/disable ipsec offload? >> >> >> I believe it is in Linux v4.15 and later. Be sure to enable >> CONFIG_INET_ESP_OFFLOAD and CONFIG_INET6_ESP_OFFLOAD to enable all the IPsec >> offload features. >> >> Since the output shows "[fixed]", the user is not able to change it on the >> fly. >> >> sln > > I'm pretty sure the "[fixed]" was due to a bug. Specifically we were > setting the bits in hw_enc_features instead of hw_features. I fixed > that in the patch set I submitted yesterday. No, that wasn't a bug, that was intended: turning the offload on and off willy-nilly will only serve to confuse the XFRM/IPsec offloads. If you don't want to use an offload, don't create and IPsec SA with the 'offload' tag. sln > > - Alex >
On Tue, Jun 5, 2018 at 10:20 AM, Shannon Nelson <shannon.nelson@oracle.com> wrote: > On 6/5/2018 9:55 AM, Alexander Duyck wrote: >> >> On Tue, Jun 5, 2018 at 9:22 AM, Shannon Nelson >> <shannon.nelson@oracle.com> wrote: >>> >>> On 6/5/2018 5:10 AM, zhuyj wrote: >>>> >>>> >>>> In the mainline kernel source code, it seems that the above does not >>>> exist. Do you use the latest ixgbe source code from e1000 maillist? >>>> And can we use "ethtool -K" to enable/disable ipsec offload? >>> >>> >>> >>> I believe it is in Linux v4.15 and later. Be sure to enable >>> CONFIG_INET_ESP_OFFLOAD and CONFIG_INET6_ESP_OFFLOAD to enable all the >>> IPsec >>> offload features. >>> >>> Since the output shows "[fixed]", the user is not able to change it on >>> the >>> fly. >>> >>> sln >> >> >> I'm pretty sure the "[fixed]" was due to a bug. Specifically we were >> setting the bits in hw_enc_features instead of hw_features. I fixed >> that in the patch set I submitted yesterday. > > > No, that wasn't a bug, that was intended: turning the offload on and off > willy-nilly will only serve to confuse the XFRM/IPsec offloads. If you > don't want to use an offload, don't create and IPsec SA with the 'offload' > tag. > > sln Well the change I made that moved the feature bits allows us to turn it on and off, so we have the bits set in features, hw_features, vlan_features, and hw_enc_features. However the hardware supports it either way as all we turn on or off is the advertising of the feature. - Alex
On 6/5/2018 11:41 AM, Alexander Duyck wrote: > On Tue, Jun 5, 2018 at 10:20 AM, Shannon Nelson > <shannon.nelson@oracle.com> wrote: >> On 6/5/2018 9:55 AM, Alexander Duyck wrote: >>> >>> On Tue, Jun 5, 2018 at 9:22 AM, Shannon Nelson >>> <shannon.nelson@oracle.com> wrote: >>>> >>>> On 6/5/2018 5:10 AM, zhuyj wrote: >>>>> >>>>> >>>>> In the mainline kernel source code, it seems that the above does not >>>>> exist. Do you use the latest ixgbe source code from e1000 maillist? >>>>> And can we use "ethtool -K" to enable/disable ipsec offload? >>>> >>>> >>>> >>>> I believe it is in Linux v4.15 and later. Be sure to enable >>>> CONFIG_INET_ESP_OFFLOAD and CONFIG_INET6_ESP_OFFLOAD to enable all the >>>> IPsec >>>> offload features. >>>> >>>> Since the output shows "[fixed]", the user is not able to change it on >>>> the >>>> fly. >>>> >>>> sln >>> >>> >>> I'm pretty sure the "[fixed]" was due to a bug. Specifically we were >>> setting the bits in hw_enc_features instead of hw_features. I fixed >>> that in the patch set I submitted yesterday. >> >> >> No, that wasn't a bug, that was intended: turning the offload on and off >> willy-nilly will only serve to confuse the XFRM/IPsec offloads. If you >> don't want to use an offload, don't create and IPsec SA with the 'offload' >> tag. >> >> sln > > Well the change I made that moved the feature bits allows us to turn > it on and off, so we have the bits set in features, hw_features, > vlan_features, and hw_enc_features. However the hardware supports it > either way as all we turn on or off is the advertising of the feature. The features are expected to be enabled in a certain pattern. For example, if the user disables NETIF_F_HW_ESP without disabling NETIF_F_HW_ESP_TX_CSUM, __ip_append_data() may make a poor decision in setting the checksum mode. I think the other uses of NETIF_F_HW_ESP_TX_CSUM are safe at the moment, but I don't think we should allow the user to break this expectation. This is part of why we have a check for this in xfrm_api_check(), to be sure the driver has set it up correctly in the first place. sln > > - Alex >
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c index 344a1f2..003b53f 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c @@ -210,7 +210,10 @@ static void ixgbe_ipsec_stop_engine(struct ixgbe_adapter *adapter) struct ixgbe_hw *hw = &adapter->hw; u32 reg; - ixgbe_ipsec_stop_data(adapter); + /* stop data if it has been running */ + reg = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); + if (!(reg & IXGBE_SECTXCTRL_SECTX_DIS)) + ixgbe_ipsec_stop_data(adapter); /* disable Rx and Tx SA lookup */ IXGBE_WRITE_REG(hw, IXGBE_IPSTXIDX, 0); @@ -966,12 +969,27 @@ void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring, **/ void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter) { + struct ixgbe_hw *hw = &adapter->hw; struct ixgbe_ipsec *ipsec; size_t size; + u32 reg1, reg2; if (adapter->hw.mac.type == ixgbe_mac_82598EB) return; + /* verify that the ipsec offload is available by checking + * the writability of the engine DISable bit - can we clear + * the bit? If not, don't set up ipsec. If yes, the put + * it back and continue the setup. + */ + reg1 = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); + reg2 = reg1 & ~IXGBE_SECTXCTRL_SECTX_DIS; + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, reg2); + reg2 = IXGBE_READ_REG(hw, IXGBE_SECTXCTRL); + if (reg2 & IXGBE_SECTXCTRL_SECTX_DIS) + return; + IXGBE_WRITE_REG(hw, IXGBE_SECTXCTRL, reg1); + ipsec = kzalloc(sizeof(*ipsec), GFP_KERNEL); if (!ipsec) goto err1;
Check the writability of the IPsec configuration registers before setting up the offload. Fixes: 49a94d74d948 ("ixgbe: add ipsec engine start and stop routines") Reported-by: Andre Tomt <andre@tomt.net> Cc: Alexander Duyck <alexander.h.duyck@intel.com> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com> --- drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)