| Message ID | 20230214082657.20468-2-ehakim@nvidia.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/2] mka: Allow configuration of MACsec hardware offload | expand |
++ sd@queasysnail.net add missing maintainer of relevant subsystem > -----Original Message----- > From: Emeel Hakim <ehakim@nvidia.com> > Sent: Tuesday, 14 February 2023 10:27 > To: hostap@lists.infradead.org > Cc: Emeel Hakim <ehakim@nvidia.com> > Subject: [PATCH 2/2] macsec_linux: Add support for MACsec hardware offload > > This uses libnl3 to communicate with the macsec module available on Linux. A > recent enough version of libnl is needed for the hardware offload support. > > Signed-off-by: Emeel Hakim <ehakim@nvidia.com> > --- > src/drivers/driver_macsec_linux.c | 34 +++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/src/drivers/driver_macsec_linux.c b/src/drivers/driver_macsec_linux.c > index b609bbf38..ac34d7ed9 100644 > --- a/src/drivers/driver_macsec_linux.c > +++ b/src/drivers/driver_macsec_linux.c > @@ -73,6 +73,9 @@ struct macsec_drv_data { > bool replay_protect; > bool replay_protect_set; > > + enum macsec_offload offload; > + bool offload_set; > + > u32 replay_window; > > u8 encoding_sa; > @@ -228,6 +231,14 @@ static int try_commit(struct macsec_drv_data *drv) > drv->replay_window); > } > > + if (drv->offload_set) { > + wpa_printf(MSG_DEBUG, DRV_PREFIX > + "%s: try_commit offload=%d", > + drv->ifname, drv->offload); > + rtnl_link_macsec_set_offload(drv->link, > + drv->offload); > + } > + > if (drv->encoding_sa_set) { > wpa_printf(MSG_DEBUG, DRV_PREFIX > "%s: try_commit encoding_sa=%d", > @@ -455,6 +466,28 @@ static int macsec_drv_set_replay_protect(void *priv, bool > enabled, } > > > +/** > + * macsec_drv_set_offload - Set offload status > + * @priv: Private driver interface data > + * @offload: 0 = MACSEC_OFFLOAD_OFF > + * 1 = MACSEC_OFFLOAD_PHY > + * 2 = MACSEC_OFFLOAD_MAC > + * Returns: 0 on success, -1 on failure (or if not supported) */ > +static int macsec_drv_set_offload(void *priv, u8 offload) { > + struct macsec_drv_data *drv = priv; > + > + > + wpa_printf(MSG_DEBUG, "%s -> %02" PRIx8, __func__, offload); > + > + drv->offload_set = true; > + drv->offload = offload; > + > + return try_commit(drv); > +} > + > + > /** > * macsec_drv_set_current_cipher_suite - Set current cipher suite > * @priv: Private driver interface data @@ -1648,6 +1681,7 @@ const struct > wpa_driver_ops wpa_driver_macsec_linux_ops = { > .enable_protect_frames = macsec_drv_enable_protect_frames, > .enable_encrypt = macsec_drv_enable_encrypt, > .set_replay_protect = macsec_drv_set_replay_protect, > + .set_offload = macsec_drv_set_offload, > .set_current_cipher_suite = macsec_drv_set_current_cipher_suite, > .enable_controlled_port = macsec_drv_enable_controlled_port, > .get_receive_lowest_pn = macsec_drv_get_receive_lowest_pn, > -- > 2.21.3
On Wed, Feb 15, 2023 at 08:01:15AM +0000, Emeel Hakim wrote: > > This uses libnl3 to communicate with the macsec module available on Linux. A > > recent enough version of libnl is needed for the hardware offload support. > > diff --git a/src/drivers/driver_macsec_linux.c b/src/drivers/driver_macsec_linux.c > > + rtnl_link_macsec_set_offload(drv->link, > > + drv->offload); This breaks the build for commonly used libnl versions and as such, needs some kind of conditional build option to avoid that. Maybe something based on LIBNL_VER_* unless there is a more convenient option.
On Tue, Feb 21, 2023 at 06:57:51PM +0200, Jouni Malinen wrote: > On Wed, Feb 15, 2023 at 08:01:15AM +0000, Emeel Hakim wrote: > > > This uses libnl3 to communicate with the macsec module available on Linux. A > > > recent enough version of libnl is needed for the hardware offload support. > > > > diff --git a/src/drivers/driver_macsec_linux.c b/src/drivers/driver_macsec_linux.c > > > + rtnl_link_macsec_set_offload(drv->link, > > > + drv->offload); > > This breaks the build for commonly used libnl versions and as such, > needs some kind of conditional build option to avoid that. Maybe > something based on LIBNL_VER_* unless there is a more convenient option. It was actually straightforward to do that with LIBNL_VER_NUM and LIBNL_VER(), so I applied these two patches with that conditional build support added.
> -----Original Message----- > From: Jouni Malinen <j@w1.fi> > Sent: Tuesday, 21 February 2023 19:50 > To: Emeel Hakim <ehakim@nvidia.com> > Cc: hostap@lists.infradead.org; sd@queasysnail.net > Subject: Re: [PATCH 2/2] macsec_linux: Add support for MACsec hardware offload > > External email: Use caution opening links or attachments > > > On Tue, Feb 21, 2023 at 06:57:51PM +0200, Jouni Malinen wrote: > > On Wed, Feb 15, 2023 at 08:01:15AM +0000, Emeel Hakim wrote: > > > > This uses libnl3 to communicate with the macsec module available > > > > on Linux. A recent enough version of libnl is needed for the hardware offload > support. > > > > > > diff --git a/src/drivers/driver_macsec_linux.c > > > > b/src/drivers/driver_macsec_linux.c > > > > + rtnl_link_macsec_set_offload(drv->link, > > > > + drv->offload); > > > > This breaks the build for commonly used libnl versions and as such, > > needs some kind of conditional build option to avoid that. Maybe > > something based on LIBNL_VER_* unless there is a more convenient option. > > It was actually straightforward to do that with LIBNL_VER_NUM and LIBNL_VER(), > so I applied these two patches with that conditional build support added. That’s great Thanks! > -- > Jouni Malinen PGP id EFC895FA
I provided a similar patch earlier where the question was asked "why does the user need to configure this?" which I found was a valid point so I made a second patch that would try first to enable HW offload and if that failed would fallback to SW MACsec. So why is it now a good idea to have this configurable? Fine with me, but I'm curious. /Benny On 21.02.2023 18.50, Jouni Malinen wrote: > On Tue, Feb 21, 2023 at 06:57:51PM +0200, Jouni Malinen wrote: >> On Wed, Feb 15, 2023 at 08:01:15AM +0000, Emeel Hakim wrote: >>>> This uses libnl3 to communicate with the macsec module available on Linux. A >>>> recent enough version of libnl is needed for the hardware offload support. >>>> diff --git a/src/drivers/driver_macsec_linux.c b/src/drivers/driver_macsec_linux.c >>>> + rtnl_link_macsec_set_offload(drv->link, >>>> + drv->offload); >> This breaks the build for commonly used libnl versions and as such, >> needs some kind of conditional build option to avoid that. Maybe >> something based on LIBNL_VER_* unless there is a more convenient option. > It was actually straightforward to do that with LIBNL_VER_NUM and > LIBNL_VER(), so I applied these two patches with that conditional build > support added. >
On Wed, Feb 22, 2023 at 09:15:29AM +0100, Benny Lønstrup Ammitzbøll wrote: > I provided a similar patch earlier where the question was asked "why does > the user need to configure this?" which I found was a valid point so I made > a second patch that would try first to enable HW offload and if that failed > would fallback to SW MACsec. > > So why is it now a good idea to have this configurable? Fine with me, but > I'm curious. Maybe due to not remembering and mixing two contributions and there being no pending patch in the queue for doing this differently.. I'm not convinced this is good to require/need configuration, but I have no convenient ways of testing this myself and I don't fully understand how and why the kernel interface for offload was designed in this manner for MACsec. As such, it felt safer to get things available for testing in this manner. Anyway, I'd welcome a followup patch to allow the offload mechanism to be enabled automatically based on kernel/driver/hardware capabilities.
diff --git a/src/drivers/driver_macsec_linux.c b/src/drivers/driver_macsec_linux.c index b609bbf38..ac34d7ed9 100644 --- a/src/drivers/driver_macsec_linux.c +++ b/src/drivers/driver_macsec_linux.c @@ -73,6 +73,9 @@ struct macsec_drv_data { bool replay_protect; bool replay_protect_set; + enum macsec_offload offload; + bool offload_set; + u32 replay_window; u8 encoding_sa; @@ -228,6 +231,14 @@ static int try_commit(struct macsec_drv_data *drv) drv->replay_window); } + if (drv->offload_set) { + wpa_printf(MSG_DEBUG, DRV_PREFIX + "%s: try_commit offload=%d", + drv->ifname, drv->offload); + rtnl_link_macsec_set_offload(drv->link, + drv->offload); + } + if (drv->encoding_sa_set) { wpa_printf(MSG_DEBUG, DRV_PREFIX "%s: try_commit encoding_sa=%d", @@ -455,6 +466,28 @@ static int macsec_drv_set_replay_protect(void *priv, bool enabled, } +/** + * macsec_drv_set_offload - Set offload status + * @priv: Private driver interface data + * @offload: 0 = MACSEC_OFFLOAD_OFF + * 1 = MACSEC_OFFLOAD_PHY + * 2 = MACSEC_OFFLOAD_MAC + * Returns: 0 on success, -1 on failure (or if not supported) + */ +static int macsec_drv_set_offload(void *priv, u8 offload) +{ + struct macsec_drv_data *drv = priv; + + + wpa_printf(MSG_DEBUG, "%s -> %02" PRIx8, __func__, offload); + + drv->offload_set = true; + drv->offload = offload; + + return try_commit(drv); +} + + /** * macsec_drv_set_current_cipher_suite - Set current cipher suite * @priv: Private driver interface data @@ -1648,6 +1681,7 @@ const struct wpa_driver_ops wpa_driver_macsec_linux_ops = { .enable_protect_frames = macsec_drv_enable_protect_frames, .enable_encrypt = macsec_drv_enable_encrypt, .set_replay_protect = macsec_drv_set_replay_protect, + .set_offload = macsec_drv_set_offload, .set_current_cipher_suite = macsec_drv_set_current_cipher_suite, .enable_controlled_port = macsec_drv_enable_controlled_port, .get_receive_lowest_pn = macsec_drv_get_receive_lowest_pn,
This uses libnl3 to communicate with the macsec module available on Linux. A recent enough version of libnl is needed for the hardware offload support. Signed-off-by: Emeel Hakim <ehakim@nvidia.com> --- src/drivers/driver_macsec_linux.c | 34 +++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)