RE: [PATCH v2 3/7] vdpa: validate provisioned device features against specified attribute
> From: Si-Wei Liu
> Sent: Thursday, February 2, 2023 4:59 PM
>
> On 2/1/2023 9:05 PM, Parav Pandit wrote:
> >
> >> From: Si-Wei Liu
> >> Sent: Tuesday, January 31, 2023 6:22 PM
> >>
> >> With device feature provisioning, there's a chance for
> >> misconfiguration that the vdpa feature attribute supplied in 'vdpa
> >> dev add' command doesn't get selected on the device_features to be
> >> provisioned. For instance, when a @mac attribute is specified, the
> >> corresponding feature bit _F_MAC in device_features should be set for
> >> consistency. If there's conflict on provisioned features against the
> >> attribute,
> it should be treated as an error to fail the ambiguous command.
> >> Noted the opposite is not necessarily true, for e.g. it's okay to
> >> have _F_MAC set in device_features without providing a corresponding
> >> @mac attribute, in which case the vdpa vendor driver could load
> >> certain default value for attribute that is not explicitly specified.
> >>
> >> Generalize this check in vdpa core so that there's no duplicate code
> >> in each vendor driver.
> >>
> >> Signed-off-by: Si-Wei Liu
> >> ---
> >> drivers/vdpa/vdpa.c | 18 ++
> >> 1 file changed, 18 insertions(+)
> >>
> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> >> 21c8aa3..1eba978
> >> 100644
> >> --- a/drivers/vdpa/vdpa.c
> >> +++ b/drivers/vdpa/vdpa.c
> >> @@ -601,8 +601,26 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct
> >> sk_buff *skb, struct genl_info *i
> >>config.mask |=
> >> BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
> >>}
> >>if (nl_attrs[VDPA_ATTR_DEV_FEATURES]) {
> >> + u64 missing = 0x0ULL;
> >> +
> >>config.device_features =
> >>nla_get_u64(nl_attrs[VDPA_ATTR_DEV_FEATURES]);
> >> + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR] &&
> >> + !(config.device_features & BIT_ULL(VIRTIO_NET_F_MAC)))
> >> + missing |= BIT_ULL(VIRTIO_NET_F_MAC);
> >> + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU] &&
> >> + !(config.device_features & BIT_ULL(VIRTIO_NET_F_MTU)))
> >> + missing |= BIT_ULL(VIRTIO_NET_F_MTU);
> >> + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP] &&
> >> + config.net.max_vq_pairs > 1 &&
> >> + !(config.device_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> >> + missing |= BIT_ULL(VIRTIO_NET_F_MQ);
> >> + if (missing) {
> >> + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> >> + "Missing features 0x%llx for
> >> provided attributes",
> >> + missing);
> >> + return -EINVAL;
> >> + }
> >>config.mask |= BIT_ULL(VDPA_ATTR_DEV_FEATURES);
> >>}
> >>
> >> --
> >> 1.8.3.1
> > Vdpa this layer can likely derive the feature bits for the supplied config
> > fields
> so that user doesn't need to keep track of both.
> > Only those feature bits which are unrelated to any config, is what user
> > should
> be setting.
> It's not I can't do this, but Jason wanted to have clear semantics around
> migration compatibility for the driver, and for that users have to explicitly
> provide device_features that we may define new driver behavior (rather that
> inheritance which is implicit and not uniformly define across drivers) for
> compatibility using the new uAPI.
Make sense to explicitly tell, just requires more careful plumbing on the user
space side.
Eventually it will get orchestrated by non user, so it should be fine to
explicitly define it.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 3/7] vdpa: validate provisioned device features against specified attribute
On 2/1/2023 9:05 PM, Parav Pandit wrote:
From: Si-Wei Liu
Sent: Tuesday, January 31, 2023 6:22 PM
With device feature provisioning, there's a chance for misconfiguration that the
vdpa feature attribute supplied in 'vdpa dev add' command doesn't get
selected on the device_features to be provisioned. For instance, when a @mac
attribute is specified, the corresponding feature bit _F_MAC in device_features
should be set for consistency. If there's conflict on provisioned features
against
the attribute, it should be treated as an error to fail the ambiguous command.
Noted the opposite is not necessarily true, for e.g. it's okay to have _F_MAC
set
in device_features without providing a corresponding @mac attribute, in which
case the vdpa vendor driver could load certain default value for attribute that
is
not explicitly specified.
Generalize this check in vdpa core so that there's no duplicate code in each
vendor driver.
Signed-off-by: Si-Wei Liu
---
drivers/vdpa/vdpa.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index 21c8aa3..1eba978
100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -601,8 +601,26 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct
sk_buff *skb, struct genl_info *i
config.mask |=
BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
}
if (nl_attrs[VDPA_ATTR_DEV_FEATURES]) {
+ u64 missing = 0x0ULL;
+
config.device_features =
nla_get_u64(nl_attrs[VDPA_ATTR_DEV_FEATURES]);
+ if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR] &&
+ !(config.device_features & BIT_ULL(VIRTIO_NET_F_MAC)))
+ missing |= BIT_ULL(VIRTIO_NET_F_MAC);
+ if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU] &&
+ !(config.device_features & BIT_ULL(VIRTIO_NET_F_MTU)))
+ missing |= BIT_ULL(VIRTIO_NET_F_MTU);
+ if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP] &&
+ config.net.max_vq_pairs > 1 &&
+ !(config.device_features & BIT_ULL(VIRTIO_NET_F_MQ)))
+ missing |= BIT_ULL(VIRTIO_NET_F_MQ);
+ if (missing) {
+ NL_SET_ERR_MSG_FMT_MOD(info->extack,
+ "Missing features 0x%llx for
provided attributes",
+ missing);
+ return -EINVAL;
+ }
config.mask |= BIT_ULL(VDPA_ATTR_DEV_FEATURES);
}
--
1.8.3.1
Vdpa this layer can likely derive the feature bits for the supplied config
fields so that user doesn't need to keep track of both.
Only those feature bits which are unrelated to any config, is what user should
be setting.
It's not I can't do this, but Jason wanted to have clear semantics
around migration compatibility for the driver, and for that users have
to explicitly provide device_features that we may define new driver
behavior (rather that inheritance which is implicit and not uniformly
define across drivers) for compatibility using the new uAPI.
Thanks,
-Siwei
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH v2 3/7] vdpa: validate provisioned device features against specified attribute
> From: Si-Wei Liu
> Sent: Tuesday, January 31, 2023 6:22 PM
>
> With device feature provisioning, there's a chance for misconfiguration that
> the
> vdpa feature attribute supplied in 'vdpa dev add' command doesn't get
> selected on the device_features to be provisioned. For instance, when a @mac
> attribute is specified, the corresponding feature bit _F_MAC in
> device_features
> should be set for consistency. If there's conflict on provisioned features
> against
> the attribute, it should be treated as an error to fail the ambiguous command.
> Noted the opposite is not necessarily true, for e.g. it's okay to have _F_MAC
> set
> in device_features without providing a corresponding @mac attribute, in which
> case the vdpa vendor driver could load certain default value for attribute
> that is
> not explicitly specified.
>
> Generalize this check in vdpa core so that there's no duplicate code in each
> vendor driver.
>
> Signed-off-by: Si-Wei Liu
> ---
> drivers/vdpa/vdpa.c | 18 ++
> 1 file changed, 18 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index 21c8aa3..1eba978
> 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -601,8 +601,26 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct
> sk_buff *skb, struct genl_info *i
> config.mask |=
> BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
> }
> if (nl_attrs[VDPA_ATTR_DEV_FEATURES]) {
> + u64 missing = 0x0ULL;
> +
> config.device_features =
> nla_get_u64(nl_attrs[VDPA_ATTR_DEV_FEATURES]);
> + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR] &&
> + !(config.device_features & BIT_ULL(VIRTIO_NET_F_MAC)))
> + missing |= BIT_ULL(VIRTIO_NET_F_MAC);
> + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU] &&
> + !(config.device_features & BIT_ULL(VIRTIO_NET_F_MTU)))
> + missing |= BIT_ULL(VIRTIO_NET_F_MTU);
> + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP] &&
> + config.net.max_vq_pairs > 1 &&
> + !(config.device_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> + missing |= BIT_ULL(VIRTIO_NET_F_MQ);
> + if (missing) {
> + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> +"Missing features 0x%llx for
> provided attributes",
> +missing);
> + return -EINVAL;
> + }
> config.mask |= BIT_ULL(VDPA_ATTR_DEV_FEATURES);
> }
>
> --
> 1.8.3.1
Vdpa this layer can likely derive the feature bits for the supplied config
fields so that user doesn't need to keep track of both.
Only those feature bits which are unrelated to any config, is what user should
be setting.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
