[PATCH] mac80211_hwsim: allow setting custom features/iftypes

[PATCH] mac80211_hwsim: allow setting custom features/iftypes

[James Prestwood]

The current driver hard codes various features, ext features and supported
interface types when a new radio is created. This patch adds several new
hwsim attributes so user space can specify specific features the radio should
have:

- HWSIM_ATTR_FEATURE_SUPPORT
	Bit field of nl80211_feature_flags flags
- HWSIM_ATTR_EXT_FEATURE_SUPPORT
	Variable length bit field where bits map to nl80211_ext_feature_index
	values.
- HWSIM_ATTR_IFTYPE_SUPPORT
	Nested attribute of flags containing all supported interface types.
	Each flag attribute sets support for an interface type.

Each attribute data format matches a corresponding NL80211 attribute:

HWSIM_ATTR_FEATURE_SUPPORT --> NL80211_ATTR_FEATURE_FLAGS
HWSIM_ATTR_EXT_FEATURE_SUPPORT --> NL80211_ATTR_EXT_FEATURES
HWSIM_ATTR_IFTYPE_SUPPORT --> NL80211_ATTR_SUPPORTED_IFTYPES
---
 drivers/net/wireless/mac80211_hwsim.c | 95 +++++++++++++++++++++++----
 drivers/net/wireless/mac80211_hwsim.h | 10 +++
 2 files changed, 91 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 18e819d964f1..727e61cf154c 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -641,8 +641,30 @@ static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] = {
 	[HWSIM_ATTR_NO_VIF] = { .type = NLA_FLAG },
 	[HWSIM_ATTR_FREQ] = { .type = NLA_U32 },
 	[HWSIM_ATTR_PERM_ADDR] = { .type = NLA_UNSPEC, .len = ETH_ALEN },
+	[HWSIM_ATTR_FEATURE_SUPPORT] = { .type = NLA_U32 },
+	[HWSIM_ATTR_EXT_FEATURE_SUPPORT] = { .type = NLA_BINARY },
+	[HWSIM_ATTR_IFTYPE_SUPPORT] = { .type = NLA_NESTED },
 };
 
+/* policy for iftype support flags */
+static const struct nla_policy
+hwsim_iftype_support_policy[NUM_NL80211_IFTYPES] = {
+	[NL80211_IFTYPE_UNSPECIFIED] = { .type = NLA_FLAG },
+	[NL80211_IFTYPE_ADHOC] = { .type = NLA_FLAG },
+	[NL80211_IFTYPE_STATION] = { .type = NLA_FLAG },
+	[NL80211_IFTYPE_AP] = { .type = NLA_FLAG },
+	[NL80211_IFTYPE_AP_VLAN] = { .type = NLA_FLAG },
+	[NL80211_IFTYPE_WDS] = { .type = NLA_FLAG },
+	[NL80211_IFTYPE_MONITOR] = { .type = NLA_FLAG },
+	[NL80211_IFTYPE_MESH_POINT] = { .type = NLA_FLAG },
+	[NL80211_IFTYPE_P2P_CLIENT] = { .type = NLA_FLAG },
+	[NL80211_IFTYPE_P2P_GO] = { .type = NLA_FLAG },
+	[NL80211_IFTYPE_P2P_DEVICE] = { .type = NLA_FLAG },
+	[NL80211_IFTYPE_OCB] = { .type = NLA_FLAG },
+	[NL80211_IFTYPE_NAN] = { .type = NLA_FLAG },
+};
+
+
 static void mac80211_hwsim_tx_frame(struct ieee80211_hw *hw,
 				    struct sk_buff *skb,
 				    struct ieee80211_channel *chan);
@@ -2413,6 +2435,9 @@ struct hwsim_new_radio_params {
 	const char *hwname;
 	bool no_vif;
 	const u8 *perm_addr;
+	u32 features;
+	u8 ext_features[DIV_ROUND_UP(NUM_NL80211_EXT_FEATURES, 8)];
+	u32 iftypes;
 };
 
 static void hwsim_mcast_config_msg(struct sk_buff *mcast_skb,
@@ -2631,12 +2656,8 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
 
 	hw->queues = 5;
 	hw->offchannel_tx_hw_queue = 4;
-	hw->wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION) |
-				     BIT(NL80211_IFTYPE_AP) |
-				     BIT(NL80211_IFTYPE_P2P_CLIENT) |
-				     BIT(NL80211_IFTYPE_P2P_GO) |
-				     BIT(NL80211_IFTYPE_ADHOC) |
-				     BIT(NL80211_IFTYPE_MESH_POINT);
+
+	hw->wiphy->interface_modes = param->iftypes;
 
 	if (param->p2p_device)
 		hw->wiphy->interface_modes |= BIT(NL80211_IFTYPE_P2P_DEVICE);
@@ -2658,12 +2679,10 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
 			    WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL |
 			    WIPHY_FLAG_AP_UAPSD |
 			    WIPHY_FLAG_HAS_CHANNEL_SWITCH;
-	hw->wiphy->features |= NL80211_FEATURE_ACTIVE_MONITOR |
-			       NL80211_FEATURE_AP_MODE_CHAN_WIDTH_CHANGE |
-			       NL80211_FEATURE_STATIC_SMPS |
-			       NL80211_FEATURE_DYNAMIC_SMPS |
-			       NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR;
-	wiphy_ext_feature_set(hw->wiphy, NL80211_EXT_FEATURE_VHT_IBSS);
+
+	hw->wiphy->features |= param->features;
+	memcpy(hw->wiphy->ext_features, param->ext_features,
+				DIV_ROUND_UP(NUM_NL80211_EXT_FEATURES, 8));
 
 	/* ask mac80211 to reserve space for magic */
 	hw->vif_data_size = sizeof(struct hwsim_vif_priv);
@@ -2767,8 +2786,6 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
 	if (param->no_vif)
 		ieee80211_hw_set(hw, NO_AUTO_VIF);
 
-	wiphy_ext_feature_set(hw->wiphy, NL80211_EXT_FEATURE_CQM_RSSI_LIST);
-
 	err = ieee80211_register_hw(hw);
 	if (err < 0) {
 		pr_debug("mac80211_hwsim: ieee80211_register_hw failed (%d)\n",
@@ -3244,6 +3261,56 @@ static int hwsim_new_radio_nl(struct sk_buff *msg, struct genl_info *info)
 		param.perm_addr = nla_data(info->attrs[HWSIM_ATTR_PERM_ADDR]);
 	}
 
+	if (info->attrs[HWSIM_ATTR_FEATURE_SUPPORT])
+		param.features = nla_get_u32(
+				info->attrs[HWSIM_ATTR_FEATURE_SUPPORT]);
+	else
+		param.features = NL80211_FEATURE_ACTIVE_MONITOR |
+				NL80211_FEATURE_AP_MODE_CHAN_WIDTH_CHANGE |
+				NL80211_FEATURE_STATIC_SMPS |
+				NL80211_FEATURE_DYNAMIC_SMPS |
+				NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR;
+
+	if (info->attrs[HWSIM_ATTR_EXT_FEATURE_SUPPORT]) {
+		if (nla_len(info->attrs[HWSIM_ATTR_EXT_FEATURE_SUPPORT]) !=
+				DIV_ROUND_UP(NUM_NL80211_EXT_FEATURES, 8))
+			return -EINVAL;
+
+		memcpy(param.ext_features,
+			nla_data(info->attrs[HWSIM_ATTR_EXT_FEATURE_SUPPORT]),
+			DIV_ROUND_UP(NUM_NL80211_EXT_FEATURES, 8));
+	} else {
+		param.ext_features[NL80211_EXT_FEATURE_VHT_IBSS / 8] |=
+				BIT(NL80211_EXT_FEATURE_VHT_IBSS % 8);
+		param.ext_features[NL80211_EXT_FEATURE_CQM_RSSI_LIST / 8] |=
+				BIT(NL80211_EXT_FEATURE_CQM_RSSI_LIST % 8);
+	}
+
+	if (info->attrs[HWSIM_ATTR_IFTYPE_SUPPORT]) {
+		int i;
+		struct nlattr *types[NUM_NL80211_IFTYPES];
+		int err = nla_parse_nested(types,
+					NUM_NL80211_IFTYPES - 1,
+					info->attrs[HWSIM_ATTR_IFTYPE_SUPPORT],
+					hwsim_iftype_support_policy,
+					info->extack);
+		if (err) {
+			kfree(hwname);
+			return err;
+		}
+
+		for (i = 0; i < NUM_NL80211_IFTYPES; i++) {
+			if (nla_get_flag(types[i]))
+				param.iftypes |= BIT(i);
+		}
+	} else
+		param.iftypes = BIT(NL80211_IFTYPE_STATION) |
+				BIT(NL80211_IFTYPE_AP) |
+				BIT(NL80211_IFTYPE_P2P_CLIENT) |
+				BIT(NL80211_IFTYPE_P2P_GO) |
+				BIT(NL80211_IFTYPE_ADHOC) |
+				BIT(NL80211_IFTYPE_MESH_POINT);
+
 	ret = mac80211_hwsim_new_radio(info, &param);
 	kfree(hwname);
 	return ret;
diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h
index 0fe3199f8c72..aea242eb4746 100644
--- a/drivers/net/wireless/mac80211_hwsim.h
+++ b/drivers/net/wireless/mac80211_hwsim.h
@@ -132,6 +132,13 @@ enum {
  * @HWSIM_ATTR_TX_INFO_FLAGS: additional flags for corresponding
  *	rates of %HWSIM_ATTR_TX_INFO
  * @HWSIM_ATTR_PERM_ADDR: permanent mac address of new radio
+ * @HWSIM_ATTR_FEATURE_SUPPORT: bit mask of supported features (u32 attribute)
+ * 	Matches data format of %NL80211_ATTR_FEATURE_FLAGS
+ * @HWSIM_ATTR_EXT_FEATURE_SUPPORT: variable length bit field. Matches data
+ * 	format of %NL80211_ATTR_EXT_FEATURES
+ * @HWSIM_ATTR_IFTYPE_SUPPORT: nested attribute containing flag attributes.
+ * 	Each flag either sets the support for each iftype. Matches format of
+ * 	%NL80211_ATTR_SUPPORTED_IFTYPES
  * @__HWSIM_ATTR_MAX: enum limit
  */
 
@@ -160,6 +167,9 @@ enum {
 	HWSIM_ATTR_PAD,
 	HWSIM_ATTR_TX_INFO_FLAGS,
 	HWSIM_ATTR_PERM_ADDR,
+	HWSIM_ATTR_FEATURE_SUPPORT,
+	HWSIM_ATTR_EXT_FEATURE_SUPPORT,
+	HWSIM_ATTR_IFTYPE_SUPPORT,
 	__HWSIM_ATTR_MAX,
 };
 #define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1)
-- 
2.17.1

Re: [PATCH] mac80211_hwsim: allow setting custom features/iftypes

[Johannes Berg]

On Tue, 2018-10-09 at 10:48 -0700, James Prestwood wrote:
> The current driver hard codes various features, ext features and supported
> interface types when a new radio is created. This patch adds several new
> hwsim attributes so user space can specify specific features the radio should
> have:
> 
> - HWSIM_ATTR_FEATURE_SUPPORT
> 	Bit field of nl80211_feature_flags flags
> - HWSIM_ATTR_EXT_FEATURE_SUPPORT
> 	Variable length bit field where bits map to nl80211_ext_feature_index
> 	values.
> - HWSIM_ATTR_IFTYPE_SUPPORT
> 	Nested attribute of flags containing all supported interface types.
> 	Each flag attribute sets support for an interface type.

This seems rather wrong, most (extended) features require the
driver/device to actually *do* something.

Let's say you enable NL80211_EXT_FEATURE_TXQS - but then nothing
actually happens when you try to configure those. Or let's say you
enable NL80211_FEATURE_TX_POWER_INSERTION but then nothing actually
happens when sending the frame...

johannes

Re: [PATCH] mac80211_hwsim: allow setting custom features/iftypes

[James Prestwood]

On Tue, 2018-10-09 at 21:41 +0200, Johannes Berg wrote:
> On Tue, 2018-10-09 at 10:48 -0700, James Prestwood wrote:
> > The current driver hard codes various features, ext features and
> > supported
> > interface types when a new radio is created. This patch adds
> > several new
> > hwsim attributes so user space can specify specific features the
> > radio should
> > have:
> > 
> > - HWSIM_ATTR_FEATURE_SUPPORT
> > 	Bit field of nl80211_feature_flags flags
> > - HWSIM_ATTR_EXT_FEATURE_SUPPORT
> > 	Variable length bit field where bits map to
> > nl80211_ext_feature_index
> > 	values.
> > - HWSIM_ATTR_IFTYPE_SUPPORT
> > 	Nested attribute of flags containing all supported interface
> > types.
> > 	Each flag attribute sets support for an interface type.
> 
> This seems rather wrong, most (extended) features require the
> driver/device to actually *do* something.
> 
> Let's say you enable NL80211_EXT_FEATURE_TXQS - but then nothing
> actually happens when you try to configure those. Or let's say you
> enable NL80211_FEATURE_TX_POWER_INSERTION but then nothing actually
> happens when sending the frame...

Ok, that makes sense. The intent here was to test logic for detecting
and handling supported driver features/iftypes, rather than actually
using the features. But I do understand it would be strange for anyone
trying to enable a feature, to find that all it does it set a feature
bit (although this is exactly what we want :)). Would it be acceptable
(for now at least) if we just included the iftype piece? I can pull
that out into a new patch if so.

> 
> johannes

Re: [PATCH] mac80211_hwsim: allow setting custom features/iftypes

[Johannes Berg]

On Tue, 2018-10-09 at 13:12 -0700, James Prestwood wrote:

> Ok, that makes sense. The intent here was to test logic for detecting
> and handling supported driver features/iftypes, rather than actually
> using the features. But I do understand it would be strange for anyone
> trying to enable a feature, to find that all it does it set a feature
> bit (although this is exactly what we want :)).

:-)

In general, I guess what would work is to be able to *restrict* the
advertised features over what's currently the case, but I suspect that's
not something that would be so very much useful for you (unless we also
add more features to hwsim).

> Would it be acceptable
> (for now at least) if we just included the iftype piece? I can pull
> that out into a new patch if so.

As written, it doesn't make much sense, but again you could restrict the
feature set? There are also the pure software feature types etc., and
mac80211 will add those in some cases.

Similar to the features though, you wouldn't want to advertise e.g. NAN
over hwsim, since that requires special operations to be implemented.

I guess here also I could see perhaps a way to restrict the interface
types could be worthwhile?

Though I'd do the implementation with a single u32 attribute with a
bitmap, rather than the massive nested flags. Which, btw, you should've
used the nested policy support for that I added recently in commit
9a659a35ba17 ("netlink: allow NLA_NESTED to specify nested policy to
validate"), but that point is of course moot if we don't want to do a
nested attribute :-)

johannes

Re: [PATCH] mac80211_hwsim: allow setting custom features/iftypes

[James Prestwood]

On Tue, 2018-10-09 at 22:13 +0200, Johannes Berg wrote:
> On Tue, 2018-10-09 at 13:12 -0700, James Prestwood wrote:
> 
> > Ok, that makes sense. The intent here was to test logic for
> > detecting
> > and handling supported driver features/iftypes, rather than
> > actually
> > using the features. But I do understand it would be strange for
> > anyone
> > trying to enable a feature, to find that all it does it set a
> > feature
> > bit (although this is exactly what we want :)).
> 
> :-)
> 
> In general, I guess what would work is to be able to *restrict* the
> advertised features over what's currently the case, but I suspect
> that's
> not something that would be so very much useful for you (unless we
> also
> add more features to hwsim).

Actually this would work perfectly. Just being able to disable/restrict
features will work fine. I can change the attribute to take a mask of
disabled/enabled features, but only features that hwsim actually
supports.

> 
> > Would it be acceptable
> > (for now at least) if we just included the iftype piece? I can pull
> > that out into a new patch if so.
> 
> As written, it doesn't make much sense, but again you could restrict
> the
> feature set? There are also the pure software feature types etc., and
> mac80211 will add those in some cases.
> 
> Similar to the features though, you wouldn't want to advertise e.g.
> NAN
> over hwsim, since that requires special operations to be implemented.
> 
> I guess here also I could see perhaps a way to restrict the interface
> types could be worthwhile?
> 
> Though I'd do the implementation with a single u32 attribute with a
> bitmap, rather than the massive nested flags. Which, btw, you
> should've
> used the nested policy support for that I added recently in commit
> 9a659a35ba17 ("netlink: allow NLA_NESTED to specify nested policy to
> validate"), but that point is of course moot if we don't want to do a
> nested attribute :-)

The reason I did it this way was to follow how NL80211 packaged up
iftype support (NL80211_ATTR_SUPPORTED_IFTYPES). We could model this
after how we want the feature support, where we only allow
disabling/enabling features/iftypes that are already supported in the
default configuration. So, for iftypes, my code could remain the same
where I build up a mask of iftypes, but then AND that with a the
default configuration.

Thanks,
James

> 
> johannes

Re: [PATCH] mac80211_hwsim: allow setting custom features/iftypes

[Johannes Berg]

On Tue, 2018-10-09 at 13:42 -0700, James Prestwood wrote:

> > In general, I guess what would work is to be able to *restrict* the
> > advertised features over what's currently the case, but I suspect
> > that's not something that would be so very much useful for you (unless we
> > also add more features to hwsim).
> 
> Actually this would work perfectly. Just being able to disable/restrict
> features will work fine. I can change the attribute to take a mask of
> disabled/enabled features, but only features that hwsim actually
> supports.

Well I guess you really only need *disabled* ones since the default
would remain to have all enabled, and you can't change them on the fly,
only set them at radio creation, right?

> The reason I did it this way was to follow how NL80211 packaged up
> iftype support (NL80211_ATTR_SUPPORTED_IFTYPES).

Ok, but I still think a bitmap would make more sense, back then for some
reason I/we didn't (like to) do bitmaps, but these days I think it's the
much better representation.

> We could model this
> after how we want the feature support, where we only allow
> disabling/enabling features/iftypes that are already supported in the
> default configuration. So, for iftypes, my code could remain the same
> where I build up a mask of iftypes, but then AND that with a the
> default configuration.

Sort of.

Yes, I think we would want to have a list/mask of *enabled* (extended)
features/interface types, unlike what I said above to just have a mask
of disabled features; if we do a mask of disabled ones then one
basically has to list all current and *future ones* to have predictable
output, that's not going to be feasible.

So yes, we want a list of *enabled* ones.

However, I think we want to reject configurations that list more enabled
features than we currently support, because otherwise again you end up
with unpredictability if the hwsim version changes underneath your tool
or test case.

So I think the algorithm should be:

valid_features = <current list>;
enabled_features = nla_get_whatever(...);

if (enabled_features & ~valid_features) {
	NL_SET_ERR_ATTR_MSG(...);
	return -EINVAL;
}

// use enabled_features instead of the current list later

johannes

Re: [PATCH] mac80211_hwsim: allow setting custom features/iftypes

[James Prestwood]

On Tue, 2018-10-09 at 22:47 +0200, Johannes Berg wrote:
> On Tue, 2018-10-09 at 13:42 -0700, James Prestwood wrote:
> 
> > > In general, I guess what would work is to be able to *restrict*
> > > the
> > > advertised features over what's currently the case, but I suspect
> > > that's not something that would be so very much useful for you
> > > (unless we
> > > also add more features to hwsim).
> > 
> > Actually this would work perfectly. Just being able to
> > disable/restrict
> > features will work fine. I can change the attribute to take a mask
> > of
> > disabled/enabled features, but only features that hwsim actually
> > supports.
> 
> Well I guess you really only need *disabled* ones since the default
> would remain to have all enabled, and you can't change them on the
> fly,
> only set them at radio creation, right?

Yes, disabling features is really what we needed ultimately. And
changing them on the fly is not needed.

> 
> > The reason I did it this way was to follow how NL80211 packaged up
> > iftype support (NL80211_ATTR_SUPPORTED_IFTYPES).
> 
> Ok, but I still think a bitmap would make more sense, back then for
> some
> reason I/we didn't (like to) do bitmaps, but these days I think it's
> the
> much better representation.
> 
> > We could model this
> > after how we want the feature support, where we only allow
> > disabling/enabling features/iftypes that are already supported in
> > the
> > default configuration. So, for iftypes, my code could remain the
> > same
> > where I build up a mask of iftypes, but then AND that with a the
> > default configuration.
> 
> Sort of.
> 
> Yes, I think we would want to have a list/mask of *enabled*
> (extended)
> features/interface types, unlike what I said above to just have a
> mask
> of disabled features; if we do a mask of disabled ones then one
> basically has to list all current and *future ones* to have
> predictable
> output, that's not going to be feasible.
> 
> So yes, we want a list of *enabled* ones.
> 
> However, I think we want to reject configurations that list more
> enabled
> features than we currently support, because otherwise again you end
> up
> with unpredictability if the hwsim version changes underneath your
> tool
> or test case.

Ok, yeah this makes sense.

> 
> So I think the algorithm should be:
> 
> valid_features = <current list>;
> enabled_features = nla_get_whatever(...);
> 
> if (enabled_features & ~valid_features) {
> 	NL_SET_ERR_ATTR_MSG(...);
> 	return -EINVAL;
> }
> 
> // use enabled_features instead of the current list later


Ok, thanks for your help/suggestions. Ill get a patch together with the
talked about changes.

> 
> johannes

Re: [PATCH] mac80211_hwsim: allow setting custom features/iftypes

[Kalle Valo]

James Prestwood <james.prestwood@linux.intel.com> writes:

> The current driver hard codes various features, ext features and supported
> interface types when a new radio is created. This patch adds several new
> hwsim attributes so user space can specify specific features the radio should
> have:
>
> - HWSIM_ATTR_FEATURE_SUPPORT
> 	Bit field of nl80211_feature_flags flags
> - HWSIM_ATTR_EXT_FEATURE_SUPPORT
> 	Variable length bit field where bits map to nl80211_ext_feature_index
> 	values.
> - HWSIM_ATTR_IFTYPE_SUPPORT
> 	Nested attribute of flags containing all supported interface types.
> 	Each flag attribute sets support for an interface type.
>
> Each attribute data format matches a corresponding NL80211 attribute:
>
> HWSIM_ATTR_FEATURE_SUPPORT --> NL80211_ATTR_FEATURE_FLAGS
> HWSIM_ATTR_EXT_FEATURE_SUPPORT --> NL80211_ATTR_EXT_FEATURES
> HWSIM_ATTR_IFTYPE_SUPPORT --> NL80211_ATTR_SUPPORTED_IFTYPES

Signed-off-by is missing.

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#signed-off-by_missing

-- 
Kalle Valo