[RFC PATCH net-next 0/2] Add PCP selector and new APPTRUST attribute

[RFC PATCH net-next 0/2] Add PCP selector and new APPTRUST attribute

[Daniel Machon]

This patch series adds support for offloading PCP-based queue
classification and introduces a new APPTRUST extension attribute to the
8021Qaz APP managed object.  Prior to implemenation, it has been
discussed on the netdev mailing list here:

https://lore.kernel.org/netdev/Yv9VO1DYAxNduw6A@DEN-LT-70577/

In summary: there currently exist no conveinent way to offload per-port
PCP-based queue classification to hardware. Similarly, there is no way
to indicate the notion of trust for APP table selectors. This patch series
addresses both topics.

PCP based queue classification:

	8021Q standardizes the Priority Code Point table (see 6.9.3 of
	IEEE Std 802.1Q-2018).  This patch series makes it possible, to
	offload the PCP classification to said table.  The new PCP
	selector is not a standard part of the APP managed object,
	therefore it has been assigned a value of 255 to avoid any
	clashes with future DCB standard extensions.

Selector trust:

	ASIC's often has the notion of trust DSCP and trust PCP. This
	new object makes it possible to specify a trust order of app
	selectors, which drivers can then react on.

Patch #1 introduces a new PCP selector to the APP object, which makes it
possible to encode PCP and DEI in the app triplet and offload it to the
PCP table of the ASIC.

Patch #2 Introduces the new extension attribute APPTRUST, which is a
list of trusted app selectors. Lower indexes has higher precedence.
selectors are stored consecutively, starting from index zero. With a
maximum number of 255 unique selectors, the list has the same maximum
size.

The userspace part of this will be posted in a separate patch series.

Daniel Machon (2):
  net: dcb: add new pcp selector to app object
  net: dcb: add new apptrust attribute

 include/net/dcbnl.h        |  2 ++
 include/uapi/linux/dcbnl.h | 15 +++++++++++++++
 net/dcb/dcbnl.c            | 17 +++++++++++++++++
 3 files changed, 34 insertions(+)

--
2.34.1

[RFC PATCH net-next 1/2] net: dcb: add new pcp selector to app object

[Daniel Machon]

Add new PCP selector for the 8021Qaz APP managed object.

The purpose of adding the PCP selector, is to be able to offload
PCP-based queue classification to the 8021Q Priority Code Point table,
see 6.9.3 of IEEE Std 802.1Q-2018.

PCP and DEI is encoded in the protocol field as 8*dei+pcp, so that a
mapping of PCP 2 and DEI 1 to priority 3 is encoded as {255, 10, 3}.

While PCP is not a standard 8021Qaz selector, it seems very convenient
to add it to the APP object, as this is where similar priority mapping
is handled, and it perfectly fits the {selector, protocol, priority}
triplet.

Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
 include/uapi/linux/dcbnl.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/dcbnl.h b/include/uapi/linux/dcbnl.h
index a791a94013a6..8eab16e5bc13 100644
--- a/include/uapi/linux/dcbnl.h
+++ b/include/uapi/linux/dcbnl.h
@@ -217,6 +217,7 @@ struct cee_pfc {
 #define IEEE_8021QAZ_APP_SEL_DGRAM	3
 #define IEEE_8021QAZ_APP_SEL_ANY	4
 #define IEEE_8021QAZ_APP_SEL_DSCP       5
+#define IEEE_8021QAZ_APP_SEL_PCP	255
 
 /* This structure contains the IEEE 802.1Qaz APP managed object. This
  * object is also used for the CEE std as well.
-- 
2.34.1

[RFC PATCH net-next 2/2] net: dcb: add new apptrust attribute

[Daniel Machon]

Add a new apptrust extension attribute to the 8021Qaz APP managed
object.

The new attribute is meant to allow drivers, whose hw supports the
notion of trust, to be able to set whether a particular app selector is
to be trusted - and also the order of precedence of selectors.

A new structure ieee_apptrust has been created, which contains an array
of selectors, where lower indexes has higher precedence.

Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
 include/net/dcbnl.h        |  2 ++
 include/uapi/linux/dcbnl.h | 14 ++++++++++++++
 net/dcb/dcbnl.c            | 17 +++++++++++++++++
 3 files changed, 33 insertions(+)

diff --git a/include/net/dcbnl.h b/include/net/dcbnl.h
index 2b2d86fb3131..0c4b0107981d 100644
--- a/include/net/dcbnl.h
+++ b/include/net/dcbnl.h
@@ -61,6 +61,8 @@ struct dcbnl_rtnl_ops {
 	int (*ieee_getapp) (struct net_device *, struct dcb_app *);
 	int (*ieee_setapp) (struct net_device *, struct dcb_app *);
 	int (*ieee_delapp) (struct net_device *, struct dcb_app *);
+	int (*ieee_setapptrust)  (struct net_device *, struct ieee_apptrust *);
+	int (*ieee_getapptrust)  (struct net_device *, struct ieee_apptrust *);
 	int (*ieee_peer_getets) (struct net_device *, struct ieee_ets *);
 	int (*ieee_peer_getpfc) (struct net_device *, struct ieee_pfc *);
 
diff --git a/include/uapi/linux/dcbnl.h b/include/uapi/linux/dcbnl.h
index 8eab16e5bc13..833466dec096 100644
--- a/include/uapi/linux/dcbnl.h
+++ b/include/uapi/linux/dcbnl.h
@@ -248,6 +248,19 @@ struct dcb_app {
 	__u16	protocol;
 };
 
+#define IEEE_8021QAZ_APP_SEL_MAX 255
+
+/* This structure contains trust order extension to the IEEE 802.1Qaz APP
+ * managed object.
+ *
+ * @order: contains trust ordering of selector values for the IEEE 802.1Qaz
+ *               APP managed object. Lower indexes has higher trust.
+ */
+struct ieee_apptrust {
+	__u8 num;
+	__u8 order[IEEE_8021QAZ_APP_SEL_MAX];
+};
+
 /**
  * struct dcb_peer_app_info - APP feature information sent by the peer
  *
@@ -419,6 +432,7 @@ enum ieee_attrs {
 	DCB_ATTR_IEEE_QCN,
 	DCB_ATTR_IEEE_QCN_STATS,
 	DCB_ATTR_DCB_BUFFER,
+	DCB_ATTR_IEEE_APP_TRUST,
 	__DCB_ATTR_IEEE_MAX
 };
 #define DCB_ATTR_IEEE_MAX (__DCB_ATTR_IEEE_MAX - 1)
diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c
index dc4fb699b56c..e87f0128c3bd 100644
--- a/net/dcb/dcbnl.c
+++ b/net/dcb/dcbnl.c
@@ -162,6 +162,7 @@ static const struct nla_policy dcbnl_ieee_policy[DCB_ATTR_IEEE_MAX + 1] = {
 	[DCB_ATTR_IEEE_ETS]	    = {.len = sizeof(struct ieee_ets)},
 	[DCB_ATTR_IEEE_PFC]	    = {.len = sizeof(struct ieee_pfc)},
 	[DCB_ATTR_IEEE_APP_TABLE]   = {.type = NLA_NESTED},
+	[DCB_ATTR_IEEE_APP_TRUST]   = {.len = sizeof(struct ieee_apptrust)},
 	[DCB_ATTR_IEEE_MAXRATE]   = {.len = sizeof(struct ieee_maxrate)},
 	[DCB_ATTR_IEEE_QCN]         = {.len = sizeof(struct ieee_qcn)},
 	[DCB_ATTR_IEEE_QCN_STATS]   = {.len = sizeof(struct ieee_qcn_stats)},
@@ -1133,6 +1134,14 @@ static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev)
 	spin_unlock_bh(&dcb_lock);
 	nla_nest_end(skb, app);
 
+	if (ops->ieee_getapptrust) {
+		struct ieee_apptrust trust;
+		memset(&trust, 0, sizeof(trust));
+		err = ops->ieee_getapptrust(netdev, &trust);
+		if (!err && nla_put(skb, DCB_ATTR_IEEE_APP_TRUST, sizeof(trust), &trust))
+			return -EMSGSIZE;
+	}
+
 	/* get peer info if available */
 	if (ops->ieee_peer_getets) {
 		struct ieee_ets ets;
@@ -1513,6 +1522,14 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh,
 		}
 	}
 
+	if (ieee[DCB_ATTR_IEEE_APP_TRUST] && ops->ieee_setapptrust) {
+		struct ieee_apptrust *trust =
+			nla_data(ieee[DCB_ATTR_IEEE_APP_TRUST]);
+		err = ops->ieee_setapptrust(netdev, trust);
+		if (err)
+			goto err;
+	}
+
 err:
 	err = nla_put_u8(skb, DCB_ATTR_IEEE, err);
 	dcbnl_ieee_notify(netdev, RTM_SETDCB, DCB_CMD_IEEE_SET, seq, 0);
-- 
2.34.1

Re: [RFC PATCH net-next 2/2] net: dcb: add new apptrust attribute

[Vladimir Oltean]

Hi Daniel,

On Thu, Sep 08, 2022 at 02:04:42PM +0200, Daniel Machon wrote:
> Add a new apptrust extension attribute to the 8021Qaz APP managed
> object.
> 
> The new attribute is meant to allow drivers, whose hw supports the
> notion of trust, to be able to set whether a particular app selector is
> to be trusted - and also the order of precedence of selectors.
> 
> A new structure ieee_apptrust has been created, which contains an array
> of selectors, where lower indexes has higher precedence.
> 
> Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> ---

Let's say I have a switch which only looks at VLAN PCP/DEI if the bridge
vlan_filtering setting is enabled (otherwise, the switch is completely
VLAN unaware, including for QoS purposes).

Would it be ok to report through ieee_getapptrust() that the PCP
selector is trusted when under a vlan_filtering bridge, not trusted when
not under a vlan_filtering bridge, and deny changes to ieee_setapptrust()
for the PCP selector? I see the return value is not cached anywhere
within the kernel, just passed to the user.

Re: [RFC PATCH net-next 2/2] net: dcb: add new apptrust attribute

[Daniel.Machon]

Den Fri, Sep 09, 2022 at 12:29:50PM +0000 skrev Vladimir Oltean:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Daniel,
> 
> On Thu, Sep 08, 2022 at 02:04:42PM +0200, Daniel Machon wrote:
> > Add a new apptrust extension attribute to the 8021Qaz APP managed
> > object.
> >
> > The new attribute is meant to allow drivers, whose hw supports the
> > notion of trust, to be able to set whether a particular app selector is
> > to be trusted - and also the order of precedence of selectors.
> >
> > A new structure ieee_apptrust has been created, which contains an array
> > of selectors, where lower indexes has higher precedence.
> >
> > Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> > ---
> 
> Let's say I have a switch which only looks at VLAN PCP/DEI if the bridge
> vlan_filtering setting is enabled (otherwise, the switch is completely
> VLAN unaware, including for QoS purposes).
> 
> Would it be ok to report through ieee_getapptrust() that the PCP
> selector is trusted when under a vlan_filtering bridge, not trusted when
> not under a vlan_filtering bridge, and deny changes to ieee_setapptrust()
> for the PCP selector? I see the return value is not cached anywhere
> within the kernel, just passed to the user.

There *might* be a distinction between enabled and trusted, disabled and not-trusted.
For instance, sparx5 switch has this distinction (at least for dscp) - but really that is 
hw dependent. Therefore, in your particular case, with the vlan_filtering on/off, 
yes that would be OK IMO. Any concerns?

This patch merely provides the means for drivers to implement a user-specified trust
order and report it back to the user, just like with many of the other dcb attributes 
(maxrate, buffer etc.).

Re: [RFC PATCH net-next 2/2] net: dcb: add new apptrust attribute

[Petr Machata]

Daniel Machon <daniel.machon@microchip.com> writes:

> Add a new apptrust extension attribute to the 8021Qaz APP managed
> object.
>
> The new attribute is meant to allow drivers, whose hw supports the
> notion of trust, to be able to set whether a particular app selector is
> to be trusted - and also the order of precedence of selectors.
>
> A new structure ieee_apptrust has been created, which contains an array
> of selectors, where lower indexes has higher precedence.
>
> Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> ---
>  include/net/dcbnl.h        |  2 ++
>  include/uapi/linux/dcbnl.h | 14 ++++++++++++++
>  net/dcb/dcbnl.c            | 17 +++++++++++++++++
>  3 files changed, 33 insertions(+)
>
> diff --git a/include/net/dcbnl.h b/include/net/dcbnl.h
> index 2b2d86fb3131..0c4b0107981d 100644
> --- a/include/net/dcbnl.h
> +++ b/include/net/dcbnl.h
> @@ -61,6 +61,8 @@ struct dcbnl_rtnl_ops {
>  	int (*ieee_getapp) (struct net_device *, struct dcb_app *);
>  	int (*ieee_setapp) (struct net_device *, struct dcb_app *);
>  	int (*ieee_delapp) (struct net_device *, struct dcb_app *);
> +	int (*ieee_setapptrust)  (struct net_device *, struct ieee_apptrust *);
> +	int (*ieee_getapptrust)  (struct net_device *, struct ieee_apptrust *);
>  	int (*ieee_peer_getets) (struct net_device *, struct ieee_ets *);
>  	int (*ieee_peer_getpfc) (struct net_device *, struct ieee_pfc *);
>  
> diff --git a/include/uapi/linux/dcbnl.h b/include/uapi/linux/dcbnl.h
> index 8eab16e5bc13..833466dec096 100644
> --- a/include/uapi/linux/dcbnl.h
> +++ b/include/uapi/linux/dcbnl.h
> @@ -248,6 +248,19 @@ struct dcb_app {
>  	__u16	protocol;
>  };
>  
> +#define IEEE_8021QAZ_APP_SEL_MAX 255
> +
> +/* This structure contains trust order extension to the IEEE 802.1Qaz APP
> + * managed object.
> + *
> + * @order: contains trust ordering of selector values for the IEEE 802.1Qaz
> + *               APP managed object. Lower indexes has higher trust.
> + */
> +struct ieee_apptrust {

Trust level setting is not standard, so this should be something like
dcbnl_apptrust.

Ditto for DCB_ATTR_IEEE_APP_TRUST below. I believe DCB_ATTR_DCB_BUFFER
is in the DCB namespace for that reason, and the trust level artifacts
should be as well. Likewise with the DCB ops callbacks, which should
IMHO be dcbnl_get/setapptrust.

> +	__u8 num;

Is this supposed to be a count of the "order" items?
If yes, I'd call it "count", because then it's clear the value is not
just a number, but actually a number of items.

> +	__u8 order[IEEE_8021QAZ_APP_SEL_MAX];

Should be +1 IMHO, in case the whole u8 selector space is allocated. (In
particular, there's no guarantee that IEEE won't ever allocate the
selector 0.)

But of course this will never get anywhere close to that. We will end up
passing maybe one, two entries. So the UAPI seems excessive in how it
hands around this large array.

I wonder if it would be better to make the DCB_ATTR_IEEE_APP_TABLE
payload be an array of bytes, each byte a selector? Or something similar
to DCB_ATTR_IEEE_APP_TABLE / DCB_ATTR_IEEE_APP, a nest and an array of
payload attributes?

> +};
> +
>  /**
>   * struct dcb_peer_app_info - APP feature information sent by the peer
>   *
> @@ -419,6 +432,7 @@ enum ieee_attrs {
>  	DCB_ATTR_IEEE_QCN,
>  	DCB_ATTR_IEEE_QCN_STATS,
>  	DCB_ATTR_DCB_BUFFER,
> +	DCB_ATTR_IEEE_APP_TRUST,
>  	__DCB_ATTR_IEEE_MAX
>  };
>  #define DCB_ATTR_IEEE_MAX (__DCB_ATTR_IEEE_MAX - 1)
> diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c
> index dc4fb699b56c..e87f0128c3bd 100644
> --- a/net/dcb/dcbnl.c
> +++ b/net/dcb/dcbnl.c
> @@ -162,6 +162,7 @@ static const struct nla_policy dcbnl_ieee_policy[DCB_ATTR_IEEE_MAX + 1] = {
>  	[DCB_ATTR_IEEE_ETS]	    = {.len = sizeof(struct ieee_ets)},
>  	[DCB_ATTR_IEEE_PFC]	    = {.len = sizeof(struct ieee_pfc)},
>  	[DCB_ATTR_IEEE_APP_TABLE]   = {.type = NLA_NESTED},
> +	[DCB_ATTR_IEEE_APP_TRUST]   = {.len = sizeof(struct ieee_apptrust)},
>  	[DCB_ATTR_IEEE_MAXRATE]   = {.len = sizeof(struct ieee_maxrate)},
>  	[DCB_ATTR_IEEE_QCN]         = {.len = sizeof(struct ieee_qcn)},
>  	[DCB_ATTR_IEEE_QCN_STATS]   = {.len = sizeof(struct ieee_qcn_stats)},
> @@ -1133,6 +1134,14 @@ static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev)
>  	spin_unlock_bh(&dcb_lock);
>  	nla_nest_end(skb, app);
>  
> +	if (ops->ieee_getapptrust) {
> +		struct ieee_apptrust trust;

I believe checkpatch warns if there's no empty line between the variable
declaration block and the rest of the code.

Maybe it's just because this is an RFC, but for the final submission
please make sure you run checkpatch.pl --max-line-length=80. The
80-character limit is not really a hard requirement anymore, but it's
still strongly preferred in net.

> +		memset(&trust, 0, sizeof(trust));
> +		err = ops->ieee_getapptrust(netdev, &trust);
> +		if (!err && nla_put(skb, DCB_ATTR_IEEE_APP_TRUST, sizeof(trust), &trust))
> +			return -EMSGSIZE;
> +	}
> +
>  	/* get peer info if available */
>  	if (ops->ieee_peer_getets) {
>  		struct ieee_ets ets;
> @@ -1513,6 +1522,14 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh,
>  		}
>  	}
>  
> +	if (ieee[DCB_ATTR_IEEE_APP_TRUST] && ops->ieee_setapptrust) {

Hmm, weird that none of the set requests are bounced if there's no set
callback. That way the request appears to succeed but doesn't actually
set anything. I find this very strange.

Drivers that do not even have any DCB ops give -EOPNOTSUPP as expected.
I think lack of a particular op should do this as well. We can't change
this for the existing ones, but IMHO the new OPs should be like that.

> +		struct ieee_apptrust *trust =
> +			nla_data(ieee[DCB_ATTR_IEEE_APP_TRUST]);

Besides invoking the OP, this should validate the payload. E.g. no
driver is supposed to accept trust policies that contain invalid
selectors. Pretty sure there's no value in repeated entries either.

> +		err = ops->ieee_setapptrust(netdev, trust);
> +		if (err)
> +			goto err;
> +	}
> +
>  err:
>  	err = nla_put_u8(skb, DCB_ATTR_IEEE, err);
>  	dcbnl_ieee_notify(netdev, RTM_SETDCB, DCB_CMD_IEEE_SET, seq, 0);

Re: [RFC PATCH net-next 1/2] net: dcb: add new pcp selector to app object

[Petr Machata]

Daniel Machon <daniel.machon@microchip.com> writes:

> Add new PCP selector for the 8021Qaz APP managed object.
>
> The purpose of adding the PCP selector, is to be able to offload
> PCP-based queue classification to the 8021Q Priority Code Point table,
> see 6.9.3 of IEEE Std 802.1Q-2018.
>
> PCP and DEI is encoded in the protocol field as 8*dei+pcp, so that a
> mapping of PCP 2 and DEI 1 to priority 3 is encoded as {255, 10, 3}.
>
> While PCP is not a standard 8021Qaz selector, it seems very convenient
> to add it to the APP object, as this is where similar priority mapping
> is handled, and it perfectly fits the {selector, protocol, priority}
> triplet.
>
> Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> ---
>  include/uapi/linux/dcbnl.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/uapi/linux/dcbnl.h b/include/uapi/linux/dcbnl.h
> index a791a94013a6..8eab16e5bc13 100644
> --- a/include/uapi/linux/dcbnl.h
> +++ b/include/uapi/linux/dcbnl.h
> @@ -217,6 +217,7 @@ struct cee_pfc {
>  #define IEEE_8021QAZ_APP_SEL_DGRAM	3
>  #define IEEE_8021QAZ_APP_SEL_ANY	4
>  #define IEEE_8021QAZ_APP_SEL_DSCP       5
> +#define IEEE_8021QAZ_APP_SEL_PCP	255
>  
>  /* This structure contains the IEEE 802.1Qaz APP managed object. This
>   * object is also used for the CEE std as well.

I'm thinking how to further isolate this from the IEEE standard values.
I think it would be better to pass the non-standard APP contributions in
a different attribute. IIUIC, this is how the APP table is passed:

DCB_ATTR_IEEE_APP_TABLE {
    DCB_ATTR_IEEE_APP {
        struct dcb_app { ... };
    }
    DCB_ATTR_IEEE_APP {
        struct dcb_app { ... };
    }
}

Well, instead, the non-standard stuff could be passed in a different
attribute:

DCB_ATTR_IEEE_APP_TABLE {
    DCB_ATTR_IEEE_APP {
        struct dcb_app { ... }; // standard contribution to APP table
    }
    DCB_ATTR_DCB_APP {
        struct dcb_app { ... }; // non-standard contribution
    }
}

The new selector could still stay as 255. This will allow us to keep the
internal bookkeeping simple for the likely case that 255 never becomes a
valid IEEE selector. But if it ever does, the UAPI can stay the same,
just the internals will need to be updated.

Re: [RFC PATCH net-next 2/2] net: dcb: add new apptrust attribute

[Petr Machata]

<Daniel.Machon@microchip.com> writes:

> Den Fri, Sep 09, 2022 at 12:29:50PM +0000 skrev Vladimir Oltean:
>
>> Let's say I have a switch which only looks at VLAN PCP/DEI if the bridge
>> vlan_filtering setting is enabled (otherwise, the switch is completely
>> VLAN unaware, including for QoS purposes).
>> 
>> Would it be ok to report through ieee_getapptrust() that the PCP
>> selector is trusted when under a vlan_filtering bridge, not trusted when
>> not under a vlan_filtering bridge, and deny changes to ieee_setapptrust()
>> for the PCP selector? I see the return value is not cached anywhere
>> within the kernel, just passed to the user.
>
> Therefore, in your particular case, with the vlan_filtering on/off,
> yes that would be OK IMO. Any concerns?

Yeah, it would make sense to me. With the 802.1q bridge, the reported
trust level would be [PCP], with 802.1d it would be [].

As a service to the user, I would accept set requests that just reassert
the only valid configuration, but otherwise it sounds OK to me.

Re: [RFC PATCH net-next 2/2] net: dcb: add new apptrust attribute

[Vladimir Oltean]

On Mon, Sep 12, 2022 at 06:30:08PM +0200, Petr Machata wrote:
> 
> <Daniel.Machon@microchip.com> writes:
> 
> > Den Fri, Sep 09, 2022 at 12:29:50PM +0000 skrev Vladimir Oltean:
> >
> >> Let's say I have a switch which only looks at VLAN PCP/DEI if the bridge
> >> vlan_filtering setting is enabled (otherwise, the switch is completely
> >> VLAN unaware, including for QoS purposes).
> >> 
> >> Would it be ok to report through ieee_getapptrust() that the PCP
> >> selector is trusted when under a vlan_filtering bridge, not trusted when
> >> not under a vlan_filtering bridge, and deny changes to ieee_setapptrust()
> >> for the PCP selector? I see the return value is not cached anywhere
> >> within the kernel, just passed to the user.
> >
> > Therefore, in your particular case, with the vlan_filtering on/off,
> > yes that would be OK IMO. Any concerns?
> 
> Yeah, it would make sense to me. With the 802.1q bridge, the reported
> trust level would be [PCP], with 802.1d it would be [].
> 
> As a service to the user, I would accept set requests that just reassert
> the only valid configuration, but otherwise it sounds OK to me.

This sounds good to me too.

Re: [RFC PATCH net-next 1/2] net: dcb: add new pcp selector to app object

[Daniel.Machon]

> > The purpose of adding the PCP selector, is to be able to offload
> > PCP-based queue classification to the 8021Q Priority Code Point table,
> > see 6.9.3 of IEEE Std 802.1Q-2018.
> >
> > PCP and DEI is encoded in the protocol field as 8*dei+pcp, so that a
> > mapping of PCP 2 and DEI 1 to priority 3 is encoded as {255, 10, 3}.
> >
> > While PCP is not a standard 8021Qaz selector, it seems very convenient
> > to add it to the APP object, as this is where similar priority mapping
> > is handled, and it perfectly fits the {selector, protocol, priority}
> > triplet.
> >
> > Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> > ---
> >  include/uapi/linux/dcbnl.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/include/uapi/linux/dcbnl.h b/include/uapi/linux/dcbnl.h
> > index a791a94013a6..8eab16e5bc13 100644
> > --- a/include/uapi/linux/dcbnl.h
> > +++ b/include/uapi/linux/dcbnl.h
> > @@ -217,6 +217,7 @@ struct cee_pfc {
> >  #define IEEE_8021QAZ_APP_SEL_DGRAM   3
> >  #define IEEE_8021QAZ_APP_SEL_ANY     4
> >  #define IEEE_8021QAZ_APP_SEL_DSCP       5
> > +#define IEEE_8021QAZ_APP_SEL_PCP     255
> >
> >  /* This structure contains the IEEE 802.1Qaz APP managed object. This
> >   * object is also used for the CEE std as well.
> 
> I'm thinking how to further isolate this from the IEEE standard values.
> I think it would be better to pass the non-standard APP contributions in
> a different attribute. IIUIC, this is how the APP table is passed:
> 
> DCB_ATTR_IEEE_APP_TABLE {
>     DCB_ATTR_IEEE_APP {
>         struct dcb_app { ... };
>     }
>     DCB_ATTR_IEEE_APP {
>         struct dcb_app { ... };
>     }
> }
> 
> Well, instead, the non-standard stuff could be passed in a different
> attribute:
> 
> DCB_ATTR_IEEE_APP_TABLE {
>     DCB_ATTR_IEEE_APP {
>         struct dcb_app { ... }; // standard contribution to APP table
>     }
>     DCB_ATTR_DCB_APP {
>         struct dcb_app { ... }; // non-standard contribution
>     }
> }
> 
> The new selector could still stay as 255. This will allow us to keep the
> internal bookkeeping simple for the likely case that 255 never becomes a
> valid IEEE selector. But if it ever does, the UAPI can stay the same,
> just the internals will need to be updated.

I get your sentiment, but it seems a little far-fetched to me. The 
trade-off will be extra code, in trade for something that IMO very likely 
will not happen. Like you said earlier - how many selectors could one
possibly prioritize on?

Re: [RFC PATCH net-next 2/2] net: dcb: add new apptrust attribute

[Daniel.Machon]

Hi Petr,
Thank you for your feedback.

> Daniel Machon <daniel.machon@microchip.com> writes:
> 
> > Add a new apptrust extension attribute to the 8021Qaz APP managed
> > object.
> >
> > The new attribute is meant to allow drivers, whose hw supports the
> > notion of trust, to be able to set whether a particular app selector is
> > to be trusted - and also the order of precedence of selectors.
> >
> > A new structure ieee_apptrust has been created, which contains an array
> > of selectors, where lower indexes has higher precedence.
> >
> > Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> > ---
> >  include/net/dcbnl.h        |  2 ++
> >  include/uapi/linux/dcbnl.h | 14 ++++++++++++++
> >  net/dcb/dcbnl.c            | 17 +++++++++++++++++
> >  3 files changed, 33 insertions(+)
> >
> > diff --git a/include/net/dcbnl.h b/include/net/dcbnl.h
> > index 2b2d86fb3131..0c4b0107981d 100644
> > --- a/include/net/dcbnl.h
> > +++ b/include/net/dcbnl.h
> > @@ -61,6 +61,8 @@ struct dcbnl_rtnl_ops {
> >       int (*ieee_getapp) (struct net_device *, struct dcb_app *);
> >       int (*ieee_setapp) (struct net_device *, struct dcb_app *);
> >       int (*ieee_delapp) (struct net_device *, struct dcb_app *);
> > +     int (*ieee_setapptrust)  (struct net_device *, struct ieee_apptrust *);
> > +     int (*ieee_getapptrust)  (struct net_device *, struct ieee_apptrust *);
> >       int (*ieee_peer_getets) (struct net_device *, struct ieee_ets *);
> >       int (*ieee_peer_getpfc) (struct net_device *, struct ieee_pfc *);
> >
> > diff --git a/include/uapi/linux/dcbnl.h b/include/uapi/linux/dcbnl.h
> > index 8eab16e5bc13..833466dec096 100644
> > --- a/include/uapi/linux/dcbnl.h
> > +++ b/include/uapi/linux/dcbnl.h
> > @@ -248,6 +248,19 @@ struct dcb_app {
> >       __u16   protocol;
> >  };
> >
> > +#define IEEE_8021QAZ_APP_SEL_MAX 255
> > +
> > +/* This structure contains trust order extension to the IEEE 802.1Qaz APP
> > + * managed object.
> > + *
> > + * @order: contains trust ordering of selector values for the IEEE 802.1Qaz
> > + *               APP managed object. Lower indexes has higher trust.
> > + */
> > +struct ieee_apptrust {
> 
> Trust level setting is not standard, so this should be something like
> dcbnl_apptrust.

Ack.
 
> Ditto for DCB_ATTR_IEEE_APP_TRUST below. I believe DCB_ATTR_DCB_BUFFER
> is in the DCB namespace for that reason, and the trust level artifacts
> should be as well. Likewise with the DCB ops callbacks, which should
> IMHO be dcbnl_get/setapptrust.
> 
> > +     __u8 num;
> 
> Is this supposed to be a count of the "order" items?
> If yes, I'd call it "count", because then it's clear the value is not
> just a number, but actually a number of items.

Yes, this is the number of selector-occupied indexes. I dont have any strongs
feelings on num vs count - we can go with count.

> 
> > +     __u8 order[IEEE_8021QAZ_APP_SEL_MAX];
> 
> Should be +1 IMHO, in case the whole u8 selector space is allocated. (In
> particular, there's no guarantee that IEEE won't ever allocate the
> selector 0.)

Ack.

> 
> But of course this will never get anywhere close to that. We will end up
> passing maybe one, two entries. So the UAPI seems excessive in how it
> hands around this large array.
> 
> I wonder if it would be better to make the DCB_ATTR_IEEE_APP_TABLE
> payload be an array of bytes, each byte a selector? Or something similar
> to DCB_ATTR_IEEE_APP_TABLE / DCB_ATTR_IEEE_APP, a nest and an array of
> payload attributes?

Hmm. It might seem excessive, but a quick few thoughts on your proposed solution:
  - We need more code to define and parse the new DCB_ATTR_IEEE_APP_TRUST_TABLE /
    DCB_ATTR_IEEE_APP_TRUST attributes.
  - If the selectors are passed individually to the driver, we need a 
    dcbnl_delapptrust(), because now, the driver have to add and del from the
    driver maintained array. You could of course accumulate selectors in an array
    before passing them to the driver, but then why go away from the array in the
    first place.

I kind of like the 'set' nature more than the 'add' 'del' nature. What do you think?

> 
> > +};
> > +
> >  /**
> >   * struct dcb_peer_app_info - APP feature information sent by the peer
> >   *
> > @@ -419,6 +432,7 @@ enum ieee_attrs {
> >       DCB_ATTR_IEEE_QCN,
> >       DCB_ATTR_IEEE_QCN_STATS,
> >       DCB_ATTR_DCB_BUFFER,
> > +     DCB_ATTR_IEEE_APP_TRUST,
> >       __DCB_ATTR_IEEE_MAX
> >  };
> >  #define DCB_ATTR_IEEE_MAX (__DCB_ATTR_IEEE_MAX - 1)
> > diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c
> > index dc4fb699b56c..e87f0128c3bd 100644
> > --- a/net/dcb/dcbnl.c
> > +++ b/net/dcb/dcbnl.c
> > @@ -162,6 +162,7 @@ static const struct nla_policy dcbnl_ieee_policy[DCB_ATTR_IEEE_MAX + 1] = {
> >       [DCB_ATTR_IEEE_ETS]         = {.len = sizeof(struct ieee_ets)},
> >       [DCB_ATTR_IEEE_PFC]         = {.len = sizeof(struct ieee_pfc)},
> >       [DCB_ATTR_IEEE_APP_TABLE]   = {.type = NLA_NESTED},
> > +     [DCB_ATTR_IEEE_APP_TRUST]   = {.len = sizeof(struct ieee_apptrust)},
> >       [DCB_ATTR_IEEE_MAXRATE]   = {.len = sizeof(struct ieee_maxrate)},
> >       [DCB_ATTR_IEEE_QCN]         = {.len = sizeof(struct ieee_qcn)},
> >       [DCB_ATTR_IEEE_QCN_STATS]   = {.len = sizeof(struct ieee_qcn_stats)},
> > @@ -1133,6 +1134,14 @@ static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev)
> >       spin_unlock_bh(&dcb_lock);
> >       nla_nest_end(skb, app);
> >
> > +     if (ops->ieee_getapptrust) {
> > +             struct ieee_apptrust trust;
> 
> I believe checkpatch warns if there's no empty line between the variable
> declaration block and the rest of the code.

It does give a warning. No empty lines on any of the other declarations though,
but lets indeed add one here.

> 
> Maybe it's just because this is an RFC, but for the final submission
> please make sure you run checkpatch.pl --max-line-length=80. The
> 80-character limit is not really a hard requirement anymore, but it's
> still strongly preferred in net.
> 
> > +             memset(&trust, 0, sizeof(trust));
> > +             err = ops->ieee_getapptrust(netdev, &trust);
> > +             if (!err && nla_put(skb, DCB_ATTR_IEEE_APP_TRUST, sizeof(trust), &trust))
> > +                     return -EMSGSIZE;
> > +     }
> > +
> >       /* get peer info if available */
> >       if (ops->ieee_peer_getets) {
> >               struct ieee_ets ets;
> > @@ -1513,6 +1522,14 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh,
> >               }
> >       }
> >
> > +     if (ieee[DCB_ATTR_IEEE_APP_TRUST] && ops->ieee_setapptrust) {
> 
> Hmm, weird that none of the set requests are bounced if there's no set
> callback. That way the request appears to succeed but doesn't actually
> set anything. I find this very strange.
> 
> Drivers that do not even have any DCB ops give -EOPNOTSUPP as expected.
> I think lack of a particular op should do this as well. We can't change
> this for the existing ones, but IMHO the new OPs should be like that.

Agree.

> 
> > +             struct ieee_apptrust *trust =
> > +                     nla_data(ieee[DCB_ATTR_IEEE_APP_TRUST]);
> 
> Besides invoking the OP, this should validate the payload. E.g. no
> driver is supposed to accept trust policies that contain invalid
> selectors. Pretty sure there's no value in repeated entries either.

Validation (bogus input and unique selectors) is done in userspace (dcb-apptrust).

> 
> > +             err = ops->ieee_setapptrust(netdev, trust);
> > +             if (err)
> > +                     goto err;
> > +     }
> > +
> >  err:
> >       err = nla_put_u8(skb, DCB_ATTR_IEEE, err);
> >       dcbnl_ieee_notify(netdev, RTM_SETDCB, DCB_CMD_IEEE_SET, seq, 0);
> 

Re: [RFC PATCH net-next 2/2] net: dcb: add new apptrust attribute

[Petr Machata]

<Daniel.Machon@microchip.com> writes:
>
> Petr Machata <petrm@nvidia.com> writes:
>>
>> But of course this will never get anywhere close to that. We will end up
>> passing maybe one, two entries. So the UAPI seems excessive in how it
>> hands around this large array.
>> 
>> I wonder if it would be better to make the DCB_ATTR_IEEE_APP_TABLE
>> payload be an array of bytes, each byte a selector? Or something similar
>> to DCB_ATTR_IEEE_APP_TABLE / DCB_ATTR_IEEE_APP, a nest and an array of
>> payload attributes?
>
> Hmm. It might seem excessive, but a quick few thoughts on your proposed solution:
>   - We need more code to define and parse the new DCB_ATTR_IEEE_APP_TRUST_TABLE /
>     DCB_ATTR_IEEE_APP_TRUST attributes.

Yes, a bit. But it's not too bad IMHO. Am I forgetting something here?

	u8 selectors[256];
	int nselectors;
	int rem;

	nla_for_each_nested(attr, ieee[DCB_ATTR_DCB_APP_TRUST_TABLE], rem) {
		if (nla_type(attr) != DCB_ATTR_DCB_APP_TRUST ||
		    nla_len(attr) != 1 ||
		    nselectors >= sizeof(selectors)) {
			err = -EINVAL;
			goto err;
		}

		selectors[nselectors++] = nla_get_u8(attr);
	}

... and you have reconstructed the array.

>   - If the selectors are passed individually to the driver, we need a 
>     dcbnl_delapptrust(), because now, the driver have to add and del from the
>     driver maintained array. You could of course accumulate selectors in an array
>     before passing them to the driver, but then why go away from the array in the
>     first place.

I have no problem with using an array for the in-kernel API. There it's
easy to change. UAPI can't ever change.

>> > +             struct ieee_apptrust *trust =
>> > +                     nla_data(ieee[DCB_ATTR_IEEE_APP_TRUST]);
>> 
>> Besides invoking the OP, this should validate the payload. E.g. no
>> driver is supposed to accept trust policies that contain invalid
>> selectors. Pretty sure there's no value in repeated entries either.
>
> Validation (bogus input and unique selectors) is done in userspace (dcb-apptrust).

Using iproute2 dcb is not mandatory, the UAPI is client-agnostic. The
kernel needs to bounce bogons as well. Otherwise they will become part
of the UAPI with the meaning "this doesn't do anything".

And yeah, drivers will validate supported configurations. But still the
requests that go to the driver should already be sanitized, so that the
driver code doesn't need to worry about this.

Re: [RFC PATCH net-next 2/2] net: dcb: add new apptrust attribute

[Daniel.Machon]

Hi Petr,

> <Daniel.Machon@microchip.com> writes:
> >
> > Petr Machata <petrm@nvidia.com> writes:
> >>
> >> But of course this will never get anywhere close to that. We will end up
> >> passing maybe one, two entries. So the UAPI seems excessive in how it
> >> hands around this large array.
> >>
> >> I wonder if it would be better to make the DCB_ATTR_IEEE_APP_TABLE
> >> payload be an array of bytes, each byte a selector? Or something similar
> >> to DCB_ATTR_IEEE_APP_TABLE / DCB_ATTR_IEEE_APP, a nest and an array of
> >> payload attributes?
> >
> > Hmm. It might seem excessive, but a quick few thoughts on your proposed solution:
> >   - We need more code to define and parse the new DCB_ATTR_IEEE_APP_TRUST_TABLE /
> >     DCB_ATTR_IEEE_APP_TRUST attributes.
> 
> Yes, a bit. But it's not too bad IMHO. Am I forgetting something here?
> 
>         u8 selectors[256];
>         int nselectors;
>         int rem;
> 
>         nla_for_each_nested(attr, ieee[DCB_ATTR_DCB_APP_TRUST_TABLE], rem) {
>                 if (nla_type(attr) != DCB_ATTR_DCB_APP_TRUST ||
>                     nla_len(attr) != 1 ||
>                     nselectors >= sizeof(selectors)) {
>                         err = -EINVAL;
>                         goto err;
>                 }
> 
>                 selectors[nselectors++] = nla_get_u8(attr);
>         }
> 
> ... and you have reconstructed the array.

LGTM.

> 
> >   - If the selectors are passed individually to the driver, we need a
> >     dcbnl_delapptrust(), because now, the driver have to add and del from the
> >     driver maintained array. You could of course accumulate selectors in an array
> >     before passing them to the driver, but then why go away from the array in the
> >     first place.
> 
> I have no problem with using an array for the in-kernel API. There it's
> easy to change. UAPI can't ever change.
> 
> >> > +             struct ieee_apptrust *trust =
> >> > +                     nla_data(ieee[DCB_ATTR_IEEE_APP_TRUST]);
> >>
> >> Besides invoking the OP, this should validate the payload. E.g. no
> >> driver is supposed to accept trust policies that contain invalid
> >> selectors. Pretty sure there's no value in repeated entries either.
> >
> > Validation (bogus input and unique selectors) is done in userspace (dcb-apptrust).
> 
> Using iproute2 dcb is not mandatory, the UAPI is client-agnostic. The
> kernel needs to bounce bogons as well. Otherwise they will become part
> of the UAPI with the meaning "this doesn't do anything".
> 
> And yeah, drivers will validate supported configurations. But still the
> requests that go to the driver should already be sanitized, so that the
> driver code doesn't need to worry about this.

Good point.

Will prepare a v2 with suggested changes.