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 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
