On Tue, Sep 27, 2022 at 6:01 PM Si-Wei Liu <[email protected]> wrote: > > > > On 9/26/2022 9:07 PM, Jason Wang wrote: > > On Tue, Sep 27, 2022 at 11:59 AM Jason Wang <[email protected]> wrote: > > On Tue, Sep 27, 2022 at 9:02 AM Si-Wei Liu <[email protected]> wrote: > > > On 9/26/2022 12:11 AM, Jason Wang wrote: > > On Sat, Sep 24, 2022 at 4:01 AM Si-Wei Liu <[email protected]> wrote: > > > On 9/21/2022 7:43 PM, Jason Wang wrote: > > This patch implements features provisioning for vdpa_sim_net. > > 1) validating the provisioned features to be a subset of the parent > features. > 2) clearing the features that is not wanted by the userspace > > For example: > > # vdpa mgmtdev show > vdpasim_net: > supported_classes net > max_supported_vqs 3 > dev_features MTU MAC CTRL_VQ CTRL_MAC_ADDR ANY_LAYOUT VERSION_1 > ACCESS_PLATFORM > > Sighs, not to blame any one and it's perhaps too late, but this > "dev_features" attr in "mgmtdev show" command output should have been > called "supported_features" in the first place. > > Not sure I get this, but I guess this is the negotiated features actually. > > Actually no, that is why I said the name is a bit confusing and > "supported_features" might sound better. > > You're right, it's an mgmtdev show actually. > > This attribute in the parent device (mgmtdev) denotes the real device > capability for what virtio features can be supported by the parent device. > Any unprivileged user can check into this field to know parent device's > capability without having to create a child vDPA device at all. The features > that child vDPA device may support should be a subset of, or at most up to > what the parent device offers. For e.g. the vdpa device dev1 you created > below can expose less or equal device_features bit than 0x308820028 (MTU MAC > CTRL_VQ CTRL_MAC_ADDR ANY_LAYOUT VERSION_1 ACCESS_PLATFORM), but shouldn't be > no more than what the parent device can actually support. > > Yes, I didn't see anything wrong with "dev_features", it aligns to the > virtio spec which means the features could be used to create a vdpa > device. But if everyone agree on the renaming, I'm fine. > > > I think Ling Shan is working on reporting both negotiated features > with the device features. > > Does it imply this series is connected to another work in parallel? Is it > possible to add a reference in the cover letter? > > I'm not sure, I remember Ling Shan did some work to not block the > config show in this commit: > > commit a34bed37fc9d3da319bb75dfbf02a7d3e95e12de > Author: Zhu Lingshan <[email protected]> > Date: Fri Jul 22 19:53:07 2022 +0800 > > vDPA: !FEATURES_OK should not block querying device config space > > We need some changes in the vdpa tool to show device_features > unconditionally in the "dev config show" command. > > Ok, Lingshan post an example here: > > https://lore.kernel.org/netdev/[email protected]/T/#u > > As I explained in the other email, it's incorrect to count on config space > fields to export vDPA attributes for live migration. If anyone thinks that is > not true, think again. > > Additionally Parav already repeatedly pointed out quite a few times, we have > a lot of (quasi-)duplicated attributes with similar names, > > VDPA_ATTR_DEV_SUPPORTED_FEATURES > > Lingshan's series will add: > > VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES > > and with this series, now we have one more: > > VDPA_ATTR_DEV_FEATURES > > Do we really need to maintain so many? I'm pretty sure at least one of them > can be eliminated.
I think VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES and VDPA_ATTR_DEV_FEATURES are equivalent, we can rebase on each other if it is needed. Thanks > > -Siwei > > > Thanks > > > 1) provision vDPA device with all features that are supported by the > net simulator > > # vdpa dev add name dev1 mgmtdev vdpasim_net > # vdpa dev config show > dev1: mac 00:00:00:00:00:00 link up link_announce false mtu 1500 > negotiated_features MTU MAC CTRL_VQ CTRL_MAC_ADDR VERSION_1 ACCESS_PLATFORM > > Maybe not in this patch, but for completeness for the whole series, > could we also add device_features to the output? > > Lingshan, could you please share your thoughts or patch on this? > > Noted here the device_features argument specified during vdpa creation is > introduced by this series itself, it somehow slightly changed the original > semantics of what device_features used to be. > > I'm not sure I get this, we don't support device_features in the past > and it is used to provision device features to the vDPA device which > seems to be fine. > > > When simply look at the "vdpa dev config show" output, I cannot really > tell the actual device_features that was used in vdpa creation. For e.g. > there is a missing feature ANY_LAYOUT from negotiated_features compared > with supported_features in mgmtdev, but the orchestration software > couldn't tell if the vdpa device on destination host should be created > with or without the ANY_LAYOUT feature. > > I think VERSION_1 implies ANY_LAYOUT. > > Right, ANY_LAYOUT is a bad example. A good example might be that, I knew the > parent mgmtdev on migration source node supports CTRL_MAC_ADDR, but I don't > find it in negotiated_features. > > I think we should use the features that we got from "mgmtdev show" > instead of "negotiated features". > > On the migration destination node, the parent device does support all > features as the source offers, including CTRL_MAC_ADDR. What device features > you would expect the mgmt software to create destination vdpa device with, if > not otherwise requiring mgmt software to remember all the arguments on device > creation? > > So in this example, we need use "dev_features" so we get exact the > same features after and operation as either src or dst. > > SOURCE# vdpa mgmtdev show > vdpasim_net: > supported_classes net > max_supported_vqs 3 > dev_features MTU MAC CTRL_VQ CTRL_MAC_ADDR ANY_LAYOUT VERSION_1 > ACCESS_PLATFORM > SOURCE# vdpa dev config show > dev1: mac 00:00:00:00:00:00 link up link_announce false mtu 1500 > negotiated_features MTU MAC CTRL_VQ VERSION_1 ACCESS_PLATFORM > > DESTINATION# vdpa mgmtdev show > vdpasim_net: > supported_classes net > max_supported_vqs 3 > dev_features MTU MAC CTRL_VQ CTRL_MAC_ADDR ANY_LAYOUT VERSION_1 > ACCESS_PLATFORM > > But it should be sufficient to > use features_src & feature_dst in this case. Actually, it should work > similar as to the cpu flags, the management software should introduce > the concept of cluster which means the maximal set of common features > is calculated and provisioned during device creation to allow > migration among the nodes inside the cluster. > > Yes, this is one way mgmt software may implement, but I am not sure if it's > the only way. For e.g. for cpu flags, mgmt software can infer the guest cpus > features in use from all qemu command line arguments and host cpu > features/capability, which doesn't need to remember creation arguments and is > easy to recover from failure without having to make the VM config persistent > in data store. I thought it would be great if vdpa CLI design could offer the > same. > > One minor difference is that we have cpu model abstraction, so we can > have things like: > > ./qemu-system-x86_64 -cpu EPYC > > Which implies the cpu features/flags where vDPA doesn't have. But > consider it's just a 64bit (or 128 in the future), it doesn't seems to > be too complex for the management to know, we probably need to start > from this and then we can try to introduce some generation/model after > it is agreed on most of the vendors. > > Thanks > > Thanks, > -Siwei > > > Thanks > > Thanks, > -Siwei > > > 2) provision vDPA device with a subset of the features > > # vdpa dev add name dev1 mgmtdev vdpasim_net device_features 0x300020000 > # vdpa dev config show > dev1: mac 00:00:00:00:00:00 link up link_announce false mtu 1500 > negotiated_features CTRL_VQ VERSION_1 ACCESS_PLATFORM > > Reviewed-by: Eli Cohen <[email protected]> > Signed-off-by: Jason Wang <[email protected]> > --- > drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > index 886449e88502..a9ba02be378b 100644 > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > @@ -254,6 +254,14 @@ static int vdpasim_net_dev_add(struct vdpa_mgmt_dev > *mdev, const char *name, > dev_attr.work_fn = vdpasim_net_work; > dev_attr.buffer_size = PAGE_SIZE; > > + if (config->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) { > + if (config->device_features & > + ~dev_attr.supported_features) > + return -EINVAL; > + dev_attr.supported_features &= > + config->device_features; > + } > + > simdev = vdpasim_create(&dev_attr); > if (IS_ERR(simdev)) > return PTR_ERR(simdev); > @@ -294,7 +302,8 @@ static struct vdpa_mgmt_dev mgmt_dev = { > .id_table = id_table, > .ops = &vdpasim_net_mgmtdev_ops, > .config_attr_mask = (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR | > - 1 << VDPA_ATTR_DEV_NET_CFG_MTU), > + 1 << VDPA_ATTR_DEV_NET_CFG_MTU | > + 1 << VDPA_ATTR_DEV_FEATURES), > .max_supported_vqs = VDPASIM_NET_VQ_NUM, > .supported_features = VDPASIM_NET_FEATURES, > }; > > > _______________________________________________ Virtualization mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/virtualization
