Check VTY config against features reported by BTS

* Don't copy features for osmo-bts and nanobts initially, wait until
  BTS reported its features
* Checks for BTS features in VTY cmds: pass if features are not known
  (not yet reported by the BTS), fail if the feature is missing
* Once BTS reports its features, check relevant VTY config parts again

Related: SYS#5922, OS#5538
Change-Id: I7fca42a39a4bc98a6ea8b9cfab28c4bad3a6a0aa

doc/bts-features.txt

@@ -0,0 +1,42 @@
+Notes about BTS feature check code
+---
+
+Feature reporting:
+- For most BTS we hardcode a list of assumed features in the BTS model's
+  _init() function, e.g. bts_model_bs11_init(). These features get copied to
+  bts->features once the BTS model is set.
+- nanobts and osmo-bts are special, they support reporting features during OML
+  bring up (features_get_reported set in struct_gsm_bts_model):
+  - For osmo-bts, we do not assume any features in the BTS model and just let
+    it report all available features.
+  - For nanobts, we wait for the reported features and then extend them with
+    the features set in the bts model. This is needed because the features enum
+    gets extended by us for osmo-bts, it may have features that nanobts does
+    not report but has implemented.
+- Once features are available (either through feature reporting or copied from
+  the bts model), features_known is true in struct gsm_bts.
+
+Implementing a feature check:
+- Check that features_known is true, in case the check may be done before the
+  BTS is connected and has reported its features (e.g. in VTY config parsing)
+- Use osmo_bts_has_feature()
+- Example:
+  if (bts->features_known && !osmo_bts_has_feature(&bts->features, BTS_FEAT_MULTI_TSC))
+
+VTY and feature checks:
+- Some VTY commands only make sense if a BTS supports a certain feature
+- Implement the following checks:
+  - In the VTY command, check if the BTS has the feature.
+  - In gsm_bts_check_cfg() (or called funcs like trx_has_valid_pchan_config),
+    check if the VTY command for the feature is set and if the BTS has the
+    feature.
+- In both cases, do not fail the checks if bts->features_known is false.
+
+Resulting functionality:
+- For BTS that do not support feature reporting, the VTY config is checked
+  against the hardcoded feature set as it gets parsed.
+- For BTS that do support feature reporting, the VTY config is checked when
+  features get reported. The BTS gets rejected if the config is invalid for the
+  available features.
+- Once a BTS is up and running, VTY commands changing the behavior check
+  against the available feature sets.

include/osmocom/bsc/bts.h

@@ -295,9 +295,12 @@ struct gsm_bts_model {
 
 	struct tlv_definition nm_att_tlvdef;
 
-	/* features of a given BTS model set via gsm_bts_model_register() locally */
+	/* features of a given BTS model set via gsm_bts_model_register()
+	 * locally, see doc/bts-features.txt */
 	struct bitvec features;
 	uint8_t _features_data[MAX_BTS_FEATURES/8];
+	/* BTS reports features during OML bring up */
+	bool features_get_reported;
 };
 
 struct gsm_gprs_cell {
@@ -334,9 +337,13 @@ struct gsm_bts {
 	char version[MAX_VERSION_LENGTH];
 	char sub_model[MAX_VERSION_LENGTH];
 
-	/* features of a given BTS set/reported via OML */
+	/* features of a given BTS either hardcoded or set/reported via OML,
+	 * see doc/bts-features.txt */
 	struct bitvec features;
 	uint8_t _features_data[MAX_BTS_FEATURES/8];
+	/* Features have been reported by the BTS or were copied from the BTS
+	 * model */
+	bool features_known;
 
 	/* Connected PCU version (if any) */
 	char pcu_version[MAX_VERSION_LENGTH];

src/osmo-bsc/abis_nm.c

@@ -600,6 +600,7 @@ static int parse_attr_resp_info_attr(struct gsm_bts *bts, const struct gsm_bts_t
 		}
 
 		memcpy(bts->_features_data, TLVP_VAL(tp, NM_ATT_MANUF_ID), len);
+		bts->features_known = true;
 
 		/* Log each BTS feature in the reported vector */
 		for (i = 0; i < len * 8; i++) {

src/osmo-bsc/bsc_vty.c

@@ -1700,6 +1700,7 @@ static int lchan_act_trx(struct vty *vty, struct gsm_bts_trx *trx, int activate)
 static int lchan_act_deact(struct vty *vty, const char **argv, int argc)
 {
 	struct gsm_bts_trx_ts *ts;
+	struct gsm_bts *bts;
 	struct gsm_lchan *lchan;
 	bool vamos = (strcmp(argv[3], "vamos-sub-slot") == 0);
 	int ss_nr = atoi(argv[4]);
@@ -1725,7 +1726,8 @@ static int lchan_act_deact(struct vty *vty, const char **argv, int argc)
 		return CMD_WARNING;
 	}
 
-	if (vamos && !osmo_bts_has_feature(&ts->trx->bts->features, BTS_FEAT_VAMOS)) {
+	bts = ts->trx->bts;
+	if (vamos && bts->features_known && !osmo_bts_has_feature(&bts->features, BTS_FEAT_VAMOS)) {
 		vty_out(vty, "BTS does not support VAMOS%s", VTY_NEWLINE);
 		return CMD_WARNING;
 	}
@@ -1955,6 +1957,7 @@ DEFUN(vamos_modify_lchan, vamos_modify_lchan_cmd,
       TSC_ARGS_DOC)
 {
 	struct gsm_bts_trx_ts *ts;
+	struct gsm_bts *bts;
 	struct gsm_lchan *lchan;
 	int ss_nr = atoi(argv[3]);
 	const char *vamos_str = argv[4];
@@ -1971,7 +1974,8 @@ DEFUN(vamos_modify_lchan, vamos_modify_lchan_cmd,
 		return CMD_WARNING;
 	}
 
-	if (!osmo_bts_has_feature(&ts->trx->bts->features, BTS_FEAT_VAMOS)) {
+	bts = ts->trx->bts;
+	if (bts->features_known && !osmo_bts_has_feature(&bts->features, BTS_FEAT_VAMOS)) {
 		vty_out(vty, "%% BTS does not support VAMOS%s", VTY_NEWLINE);
 		return CMD_WARNING;
 	}

src/osmo-bsc/bts.c

@@ -500,6 +500,14 @@ int gsm_bts_check_cfg(struct gsm_bts *bts)
 		return -EINVAL;
 	}
 
+	if (bts->features_known) {
+		if (!bts_gprs_mode_is_compat(bts, bts->gprs.mode)) {
+			LOGP(DNM, LOGL_ERROR, "(bts=%u) GPRS mode set to '%s', but BTS does not support it\n", bts->nr,
+			     bts_gprs_mode_name(bts->gprs.mode));
+			return -EINVAL;
+		}
+	}
+
 	/* Verify the physical channel mapping */
 	llist_for_each_entry(trx, &bts->trx_list, list) {
 		if (!trx_has_valid_pchan_config(trx)) {
@@ -621,11 +629,15 @@ int gsm_set_bts_model(struct gsm_bts *bts, struct gsm_bts_model *model)
 {
 	bts->model = model;
 
-	/* Copy hardcoded feature list from BTS model. For some BTS we support
-	 * reporting features at runtime (as of writing nanobts, OsmoBTS),
-	 * which will then replace this list. */
-	if (model)
+	/* Copy hardcoded feature list from BTS model, unless the BTS supports
+	 * reporting features at runtime (as of writing nanobts, OsmoBTS). */
+	if (!model || model->features_get_reported) {
+		memset(bts->_features_data, 0, sizeof(bts->_features_data));
+		bts->features_known = false;
+	} else {
 		memcpy(bts->_features_data, bts->model->_features_data, sizeof(bts->_features_data));
+		bts->features_known = true;
+	}
 
 	return 0;
 }

src/osmo-bsc/bts_ipaccess_nanobts.c

@@ -131,6 +131,7 @@ struct gsm_bts_model bts_model_nanobts = {
 			[NM_ATT_IPACC_REVOC_DATE] =	{ TLV_TYPE_TL16V },
 		},
 	},
+	.features_get_reported = true,
 };
 
 

src/osmo-bsc/bts_osmobts.c

@@ -205,13 +205,8 @@ int bts_model_osmobts_init(void)
 				sizeof(model_osmobts._features_data);
 	memset(model_osmobts.features.data, 0, model_osmobts.features.data_len);
 
-	/* Order alphabetically and remember to adjust bts_init/bts_model_init
-	 * in OsmoBTS to report new features. */
-	osmo_bts_set_feature(&model_osmobts.features, BTS_FEAT_CCN);
-	osmo_bts_set_feature(&model_osmobts.features, BTS_FEAT_EGPRS);
-	osmo_bts_set_feature(&model_osmobts.features, BTS_FEAT_GPRS);
-	osmo_bts_set_feature(&model_osmobts.features, BTS_FEAT_IPV6_NSVC);
-	osmo_bts_set_feature(&model_osmobts.features, BTS_FEAT_PAGING_COORDINATION);
+	/* Adjust bts_init/bts_model_init in OsmoBTS to report new features.
+	 * See also: doc/bts-features.txt */
 
 	model_osmobts.nm_att_tlvdef.def[NM_ATT_OSMO_NS_LINK_CFG].type = TLV_TYPE_TL16V;
 

src/osmo-bsc/bts_trx.c

@@ -338,6 +338,20 @@ bool trx_has_valid_pchan_config(const struct gsm_bts_trx *trx)
 				result = false;
 			}
 		}
+
+		if (trx->bts->features_known) {
+			const struct bitvec *ft = &trx->bts->features;
+
+			if (ts->hopping.enabled && !osmo_bts_has_feature(ft, BTS_FEAT_HOPPING)) {
+				LOGP(DNM, LOGL_ERROR, "TS%d has freq. hopping enabled, but BTS does not support it\n", i);
+				result = false;
+			}
+
+			if (ts->tsc != -1 && !osmo_bts_has_feature(ft, BTS_FEAT_MULTI_TSC)) {
+				LOGP(DNM, LOGL_ERROR, "TS%d has TSC != BCC, but BTS does not support it\n", i);
+				result = false;
+			}
+		}
 	}
 
 	return result;

src/osmo-bsc/bts_trx_vty.c

@@ -316,8 +316,9 @@ DEFUN_USRATTR(cfg_ts_tsc,
 	      "Training Sequence Code of the Timeslot\n" "TSC\n")
 {
 	struct gsm_bts_trx_ts *ts = vty->index;
+	const struct gsm_bts *bts = ts->trx->bts;
 
-	if (!osmo_bts_has_feature(&ts->trx->bts->features, BTS_FEAT_MULTI_TSC)) {
+	if (bts->features_known && !osmo_bts_has_feature(&bts->features, BTS_FEAT_MULTI_TSC)) {
 		vty_out(vty, "%% This BTS does not support a TSC != BCC, "
 			"falling back to BCC%s", VTY_NEWLINE);
 		ts->tsc = -1;
@@ -339,13 +340,12 @@ DEFUN_USRATTR(cfg_ts_hopping,
 	      "Disable frequency hopping\n" "Enable frequency hopping\n")
 {
 	struct gsm_bts_trx_ts *ts = vty->index;
+	const struct gsm_bts *bts = ts->trx->bts;
 	int enabled = atoi(argv[0]);
 
-	if (enabled && !osmo_bts_has_feature(&ts->trx->bts->features, BTS_FEAT_HOPPING)) {
-		vty_out(vty, "%% BTS model does not seem to support freq. hopping%s", VTY_NEWLINE);
-		/* Allow enabling frequency hopping anyway, because the BTS might not have
-		 * connected yet (thus not sent the feature vector), so we cannot know for
-		 * sure.  Jet print a warning and let it go. */
+	if (enabled && bts->features_known && !osmo_bts_has_feature(&bts->features, BTS_FEAT_HOPPING)) {
+		vty_out(vty, "%% BTS does not support freq. hopping%s", VTY_NEWLINE);
+		return CMD_WARNING;
 	}
 
 	ts->hopping.enabled = enabled;

src/osmo-bsc/bts_vty.c

@@ -1592,7 +1592,7 @@ DEFUN_USRATTR(cfg_bts_gprs_mode,
 	struct gsm_bts *bts = vty->index;
 	enum bts_gprs_mode mode = bts_gprs_mode_parse(argv[0], NULL);
 
-	if (!bts_gprs_mode_is_compat(bts, mode)) {
+	if (bts->features_known && !bts_gprs_mode_is_compat(bts, mode)) {
 		vty_out(vty, "%% This BTS type does not support %s%s", argv[0],
 			VTY_NEWLINE);
 		return CMD_WARNING;

tests/Makefile.am

@@ -43,6 +43,7 @@ EXTRA_DIST = \
 	power_ctrl.vty \
 	interf_meas.vty \
 	acch_overpower.vty \
+	bts_features.vty \
 	ctrl/osmo-bsc-neigh-test.cfg \
 	ctrl/osmo-bsc-apply-config-file.cfg \
 	ctrl/osmo-bsc-apply-config-file-invalid.cfg \

tests/bts_features.vty

@@ -0,0 +1,27 @@
+OsmoBSC> ### see doc/bts-features.txt
+
+OsmoBSC> enable
+OsmoBSC# configure terminal
+OsmoBSC(config)# network
+
+OsmoBSC(config-net)# ### osmo-bts: all feature checks pass before it is connected (features_get_reported is true)
+OsmoBSC(config-net)# bts 0
+OsmoBSC(config-net-bts)# gprs mode egprs
+OsmoBSC(config-net-bts)# trx 0
+OsmoBSC(config-net-bts-trx)# timeslot 2
+OsmoBSC(config-net-bts-trx-ts)# hopping enabled 1
+OsmoBSC(config-net-bts-trx-ts)# exit
+OsmoBSC(config-net-bts-trx)# exit
+OsmoBSC(config-net-bts)# exit
+
+OsmoBSC(config-net)# ### bs11: checks against hardcoded features (features_get_reported is false)
+OsmoBSC(config-net)# bts 1
+OsmoBSC(config-net-bts)# type bs11
+OsmoBSC(config-net-bts)# gprs mode egprs
+% This BTS type does not support egprs
+OsmoBSC(config-net-bts)# trx 0
+OsmoBSC(config-net-bts-trx)# timeslot 2
+OsmoBSC(config-net-bts-trx-ts)# hopping enabled 1
+OsmoBSC(config-net-bts-trx-ts)# exit
+OsmoBSC(config-net-bts-trx)# exit
+OsmoBSC(config-net-bts)# exit