Re: [PATCH v2 2/3] vdpa: Add a device object for vdpa management device

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, May 17, 2022 at 02:29:38PM +0800, Jason Wang wrote:
> On Mon, May 16, 2022 at 7:54 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, May 16, 2022 at 06:34:36AM -0400, Michael S. Tsirkin wrote:
> > > On Mon, May 16, 2022 at 06:31:18PM +0800, Yongji Xie wrote:
> > > > On Mon, May 16, 2022 at 5:54 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Mon, May 16, 2022 at 05:31:27PM +0800, Yongji Xie wrote:
> > > > > > On Mon, May 16, 2022 at 5:14 PM Jason Wang <jasowang@redhat.com> wrote:
> > > > > > >
> > > > > > >
> > > > > > > 在 2022/5/16 14:03, Xie Yongji 写道:
> > > > > > > > Introduce a device object for vdpa management device to control
> > > > > > > > its lifecycle. And the device name will be used to match
> > > > > > > > VDPA_ATTR_MGMTDEV_DEV_NAME field of netlink message rather than
> > > > > > > > using parent device name.
> > > > > > > >
> > > > > > > > With this patch applied, drivers should use vdpa_mgmtdev_alloc()
> > > > > > > > or _vdpa_mgmtdev_alloc() to allocate a vDPA management device
> > > > > > > > before calling vdpa_mgmtdev_register(). And some buggy empty
> > > > > > > > release function can also be removed from the driver codes.
> > > > > > > >
> > > > > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > > > > > > ---
> > > > > > > >   drivers/vdpa/ifcvf/ifcvf_main.c      | 11 ++--
> > > > > > > >   drivers/vdpa/mlx5/net/mlx5_vnet.c    | 11 ++--
> > > > > > > >   drivers/vdpa/vdpa.c                  | 92 ++++++++++++++++++++++++----
> > > > > > > >   drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 39 ++++--------
> > > > > > > >   drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 46 +++++---------
> > > > > > > >   drivers/vdpa/vdpa_user/vduse_dev.c   | 38 ++++--------
> > > > > > > >   include/linux/vdpa.h                 | 38 +++++++++++-
> > > > > > > >   7 files changed, 168 insertions(+), 107 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
> > > > > > > > index 4366320fb68d..d4087c37cfdf 100644
> > > > > > > > --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> > > > > > > > +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> > > > > > > > @@ -821,10 +821,11 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > > > > > >       u32 dev_type;
> > > > > > > >       int ret;
> > > > > > > >
> > > > > > > > -     ifcvf_mgmt_dev = kzalloc(sizeof(struct ifcvf_vdpa_mgmt_dev), GFP_KERNEL);
> > > > > > > > -     if (!ifcvf_mgmt_dev) {
> > > > > > > > +     ifcvf_mgmt_dev = vdpa_mgmtdev_alloc(struct ifcvf_vdpa_mgmt_dev,
> > > > > > > > +                                         mdev, dev_name(dev), dev);
> > > > > > >
> > > > > > >
> > > > > > > Just wonder if it's better to make vDPA device a child of the mgmt
> > > > > > > device instead of the PCI device?
> > > > > > >
> > > > > > > (Currently we use PCI device as the parent of the vDPA device, or at
> > > > > > > least we can do this for the simulator which doesn't have a parent?)
> > > > > > >
> > > > > >
> > > > > > Make sense. I think we can do it for all vDPA drivers. Make sure the
> > > > > > parent of the vDPA device is the vDPA management device.
> > > > > >
> > > > > > Thanks,
> > > > > > Yongji
> > > > >
> > > > >
> > > > > that's an ABI change though isn't it? parent is exposed in sysfs,
> > > > > right?
> > > > >
> > > >
> > > > Hmm...yes. So it looks like we can't change it, right?
> > > >
> > > > Thanks,
> > > > Yongji
> > >
> > > Afraid so. a way to find the pci device already exists I think, right?
> >
> > You can change it, any tools should be going through the bus/device
> > links, not walking the sysfs tree directly, right?  That's what those
> > symlinks are for, in order to properly be able to enumerate different
> > device types.
> >
> > A specific topology in sysfs should not ever be assumed to be static
> > over time, that's not an accurate representation of how the kernel
> > works.
> >
> > So try it and see?  Odds are there are no tools that even care about
> > these devices, right?  Or is there?
> 
> I think there's no tool that depends on the sysfs tree now. The
> management layer is only expected to talk to the management device via
> vdpa(8) which is integrated in iproute2.

Great, then change away!
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization