Re: [RFC v1 0/8] vhost-vdpa: add support for iommufd

Re: [RFC v1 0/8] vhost-vdpa: add support for iommufd

[Jason Wang]

On Sat, Nov 4, 2023 at 1:16 AM Cindy Lu <lulu@redhat.com> wrote:
>
>
> Hi All
> This code provides the iommufd support for vdpa device
> This code fixes the bugs from the last version and also add the asid support. rebase on kernel
> v6,6-rc3
> Test passed in the physical device (vp_vdpa), but  there are still some problems in the emulated device (vdpa_sim_net),
> I will continue working on it
>
> The kernel code is
> https://gitlab.com/lulu6/vhost/-/tree/iommufdRFC_v1
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>

It would be better to have a change summary here.

Thanks

>
>
> Cindy Lu (8):
>   vhost/iommufd: Add the functions support iommufd
>   Kconfig: Add the new file vhost/iommufd
>   vhost: Add 3 new uapi to support iommufd
>   vdpa: Add new vdpa_config_ops to support iommufd
>   vdpa_sim :Add support for iommufd
>   vdpa: change the map/unmap process to support iommufd
>   vp_vdpa::Add support for iommufd
>   iommu: expose the function iommu_device_use_default_domain
>
>  drivers/iommu/iommu.c             |   2 +
>  drivers/vdpa/vdpa_sim/vdpa_sim.c  |   8 ++
>  drivers/vdpa/virtio_pci/vp_vdpa.c |   4 +
>  drivers/vhost/Kconfig             |   1 +
>  drivers/vhost/Makefile            |   1 +
>  drivers/vhost/iommufd.c           | 178 +++++++++++++++++++++++++
>  drivers/vhost/vdpa.c              | 210 +++++++++++++++++++++++++++++-
>  drivers/vhost/vhost.h             |  21 +++
>  include/linux/vdpa.h              |  38 +++++-
>  include/uapi/linux/vhost.h        |  66 ++++++++++
>  10 files changed, 525 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/vhost/iommufd.c
>
> --
> 2.34.3
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC v1 7/8] vp_vdpa::Add support for iommufd

[Jason Wang]

On Sat, Nov 4, 2023 at 1:17 AM Cindy Lu <lulu@redhat.com> wrote:
>
> Add new vdpa_config_ops function to support iommufd
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vdpa/virtio_pci/vp_vdpa.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
> index 281287fae89f..dd2c372d36a6 100644
> --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
> +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
> @@ -460,6 +460,10 @@ static const struct vdpa_config_ops vp_vdpa_ops = {
>         .set_config     = vp_vdpa_set_config,
>         .set_config_cb  = vp_vdpa_set_config_cb,
>         .get_vq_irq     = vp_vdpa_get_vq_irq,
> +       .bind_iommufd = vdpa_iommufd_physical_bind,
> +       .unbind_iommufd = vdpa_iommufd_physical_unbind,
> +       .attach_ioas = vdpa_iommufd_physical_attach_ioas,
> +       .detach_ioas = vdpa_iommufd_physical_detach_ioas,

For the device that depends on the platform IOMMU, any reason we still
bother a per device config ops here just as an indirection?

Thanks

>  };
>
>  static void vp_vdpa_free_irq_vectors(void *data)
> --
> 2.34.3
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC v1 8/8] iommu: expose the function iommu_device_use_default_domain

[Jason Wang]

On Sat, Nov 4, 2023 at 1:18 AM Cindy Lu <lulu@redhat.com> wrote:
>
> Expose the function iommu_device_use_default_domain() and
> iommu_device_unuse_default_domain()，
> While vdpa bind the iommufd device and detach the iommu device,
> vdpa need to call the function
> iommu_device_unuse_default_domain() to release the owner
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>

This is the end of the series, who is the user then?

Thanks

> ---
>  drivers/iommu/iommu.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 3bfc56df4f78..987cbf8c9a87 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3164,6 +3164,7 @@ int iommu_device_use_default_domain(struct device *dev)
>
>         return ret;
>  }
> +EXPORT_SYMBOL_GPL(iommu_device_use_default_domain);
>
>  /**
>   * iommu_device_unuse_default_domain() - Device driver stops handling device
> @@ -3187,6 +3188,7 @@ void iommu_device_unuse_default_domain(struct device *dev)
>         mutex_unlock(&group->mutex);
>         iommu_group_put(group);
>  }
> +EXPORT_SYMBOL_GPL(iommu_device_unuse_default_domain);
>
>  static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
>  {
> --
> 2.34.3
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC v1 3/8] vhost: Add 3 new uapi to support iommufd

[Jason Wang]

On Sat, Nov 4, 2023 at 1:17 AM Cindy Lu <lulu@redhat.com> wrote:
>
> VHOST_VDPA_SET_IOMMU_FD: bind the device to iommufd device
>
> VDPA_DEVICE_ATTACH_IOMMUFD_AS: Attach a vdpa device to an iommufd
> address space specified by IOAS id.
>
> VDPA_DEVICE_DETACH_IOMMUFD_AS: Detach a vdpa device
> from the iommufd address space
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>

As discussed in the previous version, any reason/advantages of this
compared to just having a single  VDPA_DEVICE_ATTACH_IOMMUFD_AS?

Thanks

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC v1 3/8] vhost: Add 3 new uapi to support iommufd

[Jason Wang]

On Sat, Nov 4, 2023 at 1:17 AM Cindy Lu <lulu@redhat.com> wrote:
>
> VHOST_VDPA_SET_IOMMU_FD: bind the device to iommufd device
>
> VDPA_DEVICE_ATTACH_IOMMUFD_AS: Attach a vdpa device to an iommufd
> address space specified by IOAS id.
>
> VDPA_DEVICE_DETACH_IOMMUFD_AS: Detach a vdpa device
> from the iommufd address space
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---

[...]

> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index f5c48b61ab62..07e1b2c443ca 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -219,4 +219,70 @@
>   */
>  #define VHOST_VDPA_RESUME              _IO(VHOST_VIRTIO, 0x7E)
>
> +/* vhost_vdpa_set_iommufd
> + * Input parameters:
> + * @iommufd: file descriptor from /dev/iommu; pass -1 to unset
> + * @iommufd_ioasid: IOAS identifier returned from ioctl(IOMMU_IOAS_ALLOC)
> + * Output parameters:
> + * @out_dev_id: device identifier
> + */
> +struct vhost_vdpa_set_iommufd {
> +       __s32 iommufd;
> +       __u32 iommufd_ioasid;
> +       __u32 out_dev_id;
> +};
> +
> +#define VHOST_VDPA_SET_IOMMU_FD \
> +       _IOW(VHOST_VIRTIO, 0x7F, struct vhost_vdpa_set_iommufd)
> +
> +/*
> + * VDPA_DEVICE_ATTACH_IOMMUFD_AS -
> + * _IOW(VHOST_VIRTIO, 0x7f, struct vdpa_device_attach_iommufd_as)
> + *
> + * Attach a vdpa device to an iommufd address space specified by IOAS
> + * id.
> + *
> + * Available only after a device has been bound to iommufd via
> + * VHOST_VDPA_SET_IOMMU_FD
> + *
> + * Undo by VDPA_DEVICE_DETACH_IOMMUFD_AS or device fd close.
> + *
> + * @argsz:     user filled size of this data.
> + * @flags:     must be 0.
> + * @ioas_id:   Input the target id which can represent an ioas
> + *             allocated via iommufd subsystem.
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +struct vdpa_device_attach_iommufd_as {
> +       __u32 argsz;
> +       __u32 flags;
> +       __u32 ioas_id;
> +};

I think we need to map ioas to vDPA AS, so there should be an ASID
from the view of vDPA?

Thanks

> +
> +#define VDPA_DEVICE_ATTACH_IOMMUFD_AS \
> +       _IOW(VHOST_VIRTIO, 0x82, struct vdpa_device_attach_iommufd_as)
> +
> +/*
> + * VDPA_DEVICE_DETACH_IOMMUFD_AS
> + *
> + * Detach a vdpa device from the iommufd address space it has been
> + * attached to. After it, device should be in a blocking DMA state.
> + *
> + * Available only after a device has been bound to iommufd via
> + * VHOST_VDPA_SET_IOMMU_FD
> + *
> + * @argsz:     user filled size of this data.
> + * @flags:     must be 0.
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +struct vdpa_device_detach_iommufd_as {
> +       __u32 argsz;
> +       __u32 flags;
> +};
> +
> +#define VDPA_DEVICE_DETACH_IOMMUFD_AS \
> +       _IOW(VHOST_VIRTIO, 0x83, struct vdpa_device_detach_iommufd_as)
> +
>  #endif
> --
> 2.34.3
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC v1 0/8] vhost-vdpa: add support for iommufd

[Michael S. Tsirkin]

On Sat, Nov 04, 2023 at 01:16:33AM +0800, Cindy Lu wrote:
> 
> Hi All
> This code provides the iommufd support for vdpa device
> This code fixes the bugs from the last version and also add the asid support. rebase on kernel
> v6,6-rc3
> Test passed in the physical device (vp_vdpa), but  there are still some problems in the emulated device (vdpa_sim_net), 

What kind of problems? Understanding that will make it easier
to figure out whether this is worth reviewing.

> I will continue working on it
> 
> The kernel code is
> https://gitlab.com/lulu6/vhost/-/tree/iommufdRFC_v1
> 
> Signed-off-by: Cindy Lu <lulu@redhat.com>

Please also Cc iommufd maintainers:

Jason Gunthorpe <jgg@ziepe.ca> (maintainer:IOMMUFD)
Kevin Tian <kevin.tian@intel.com> (maintainer:IOMMUFD)
Joerg Roedel <joro@8bytes.org> (maintainer:IOMMU SUBSYSTEM)
Will Deacon <will@kernel.org> (maintainer:IOMMU SUBSYSTEM)
Robin Murphy <robin.murphy@arm.com> (reviewer:IOMMU SUBSYSTEM)
iommu@lists.linux.dev (open list:IOMMUFD)
linux-kernel@vger.kernel.org (open list)

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC v1 0/8] vhost-vdpa: add support for iommufd

[Michael S. Tsirkin]

On Sat, Nov 04, 2023 at 01:16:33AM +0800, Cindy Lu wrote:
> Test passed in the physical device (vp_vdpa), but  there are still some problems in the emulated device (vdpa_sim_net), 

I'm not sure there's even value in bothering with iommufd for the
simulator. Just find a way to disable it and fail gracefully.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC v1 0/8] vhost-vdpa: add support for iommufd

[Michael S. Tsirkin]

On Tue, Nov 07, 2023 at 08:49:02AM -0400, Jason Gunthorpe wrote:
> On Tue, Nov 07, 2023 at 02:30:34AM -0500, Michael S. Tsirkin wrote:
> > On Sat, Nov 04, 2023 at 01:16:33AM +0800, Cindy Lu wrote:
> > > 
> > > Hi All
> > > This code provides the iommufd support for vdpa device
> > > This code fixes the bugs from the last version and also add the asid support. rebase on kernel
> > > v6,6-rc3
> > > Test passed in the physical device (vp_vdpa), but  there are still some problems in the emulated device (vdpa_sim_net), 
> > 
> > What kind of problems? Understanding that will make it easier
> > to figure out whether this is worth reviewing.
> 
> IMHO, this patch series needs to spend more time internally to Red Hat
> before it is presented to the community. It is too far away from
> something worth reviewing at this point.
> 
> Jason

I am always trying to convince people to post RFCs early
instead of working for months behind closed doors only
to be told to rewrite everything in Rust.

Why does it have to be internal to a specific company?
I see Yi Liu from Intel is helping Cindy get it into shape
and that's classic open source ethos.

I know some subsystems ignore the RFC tag but I didn't realize
iommu is one of these. Is that really true?

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC v1 0/8] vhost-vdpa: add support for iommufd

[Michael S. Tsirkin]

On Tue, Nov 07, 2023 at 10:12:37AM -0400, Jason Gunthorpe wrote:
> Big company's should take the responsibility to train and provide
> skill development for their own staff.

That would result in a beautiful cathedral of a patch. I know this is
how some companies work. We are doing more of a bazaar thing here,
though. In a bunch of subsystems it seems that you don't get the
necessary skills until you have been publically shouted at by
maintainers - better to start early ;). Not a nice environment for
novices, for sure.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC v1 0/8] vhost-vdpa: add support for iommufd

[Michael S. Tsirkin]

On Tue, Nov 07, 2023 at 08:49:02AM -0400, Jason Gunthorpe wrote:
> IMHO, this patch series needs to spend more time internally to Red Hat
> before it is presented to the community.

Just to add an example why I think this "internal review" is a bad idea
I seem to recall that someone internal to nvidia at some point
attempted to implement this already. The only output from that
work we have is that "it's tough" - no pointers to what's tough,
no code to study even as a bad path to follow.
And while Red Hat might be big, the virt team is rather smaller.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC v1 0/8] vhost-vdpa: add support for iommufd

[Michael S. Tsirkin]

On Tue, Nov 07, 2023 at 11:48:48AM -0400, Jason Gunthorpe wrote:
> On Tue, Nov 07, 2023 at 09:55:26AM -0500, Michael S. Tsirkin wrote:
> > On Tue, Nov 07, 2023 at 08:49:02AM -0400, Jason Gunthorpe wrote:
> > > IMHO, this patch series needs to spend more time internally to Red Hat
> > > before it is presented to the community.
> > 
> > Just to add an example why I think this "internal review" is a bad idea
> > I seem to recall that someone internal to nvidia at some point
> > attempted to implement this already. The only output from that
> > work we have is that "it's tough" - no pointers to what's tough,
> > no code to study even as a bad path to follow.
> > And while Red Hat might be big, the virt team is rather smaller.
> 
> I don't think Nicolin got to a presentable code point.
> 
> But you can start to see the issues even in this series, like
> simulator is complicated. mlx5 is complicated. Deciding to omit those
> is one path. Come with a proposal and justification to take it out,
> not a patch with an unexplained #ifdef.

Right. Simulator I don't think we need to support, or at least
not necessarily to get this merged - it does not really
benefit from any iommufd features.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RFC v1 3/8] vhost: Add 3 new uapi to support iommufd

[Jason Wang]

On Tue, Nov 7, 2023 at 2:57 PM Cindy Lu <lulu@redhat.com> wrote:
>
> On Mon, Nov 6, 2023 at 3:30 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Sat, Nov 4, 2023 at 1:17 AM Cindy Lu <lulu@redhat.com> wrote:
> > >
> > > VHOST_VDPA_SET_IOMMU_FD: bind the device to iommufd device
> > >
> > > VDPA_DEVICE_ATTACH_IOMMUFD_AS: Attach a vdpa device to an iommufd
> > > address space specified by IOAS id.
> > >
> > > VDPA_DEVICE_DETACH_IOMMUFD_AS: Detach a vdpa device
> > > from the iommufd address space
> > >
> > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > ---
> >
> > [...]
> >
> > > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > > index f5c48b61ab62..07e1b2c443ca 100644
> > > --- a/include/uapi/linux/vhost.h
> > > +++ b/include/uapi/linux/vhost.h
> > > @@ -219,4 +219,70 @@
> > >   */
> > >  #define VHOST_VDPA_RESUME              _IO(VHOST_VIRTIO, 0x7E)
> > >
> > > +/* vhost_vdpa_set_iommufd
> > > + * Input parameters:
> > > + * @iommufd: file descriptor from /dev/iommu; pass -1 to unset
> > > + * @iommufd_ioasid: IOAS identifier returned from ioctl(IOMMU_IOAS_ALLOC)
> > > + * Output parameters:
> > > + * @out_dev_id: device identifier
> > > + */
> > > +struct vhost_vdpa_set_iommufd {
> > > +       __s32 iommufd;
> > > +       __u32 iommufd_ioasid;
> > > +       __u32 out_dev_id;
> > > +};
> > > +
> > > +#define VHOST_VDPA_SET_IOMMU_FD \
> > > +       _IOW(VHOST_VIRTIO, 0x7F, struct vhost_vdpa_set_iommufd)
> > > +
> > > +/*
> > > + * VDPA_DEVICE_ATTACH_IOMMUFD_AS -
> > > + * _IOW(VHOST_VIRTIO, 0x7f, struct vdpa_device_attach_iommufd_as)
> > > + *
> > > + * Attach a vdpa device to an iommufd address space specified by IOAS
> > > + * id.
> > > + *
> > > + * Available only after a device has been bound to iommufd via
> > > + * VHOST_VDPA_SET_IOMMU_FD
> > > + *
> > > + * Undo by VDPA_DEVICE_DETACH_IOMMUFD_AS or device fd close.
> > > + *
> > > + * @argsz:     user filled size of this data.
> > > + * @flags:     must be 0.
> > > + * @ioas_id:   Input the target id which can represent an ioas
> > > + *             allocated via iommufd subsystem.
> > > + *
> > > + * Return: 0 on success, -errno on failure.
> > > + */
> > > +struct vdpa_device_attach_iommufd_as {
> > > +       __u32 argsz;
> > > +       __u32 flags;
> > > +       __u32 ioas_id;
> > > +};
> >
> > I think we need to map ioas to vDPA AS, so there should be an ASID
> > from the view of vDPA?
> >
> > Thanks
> >
> The qemu will have a structure save and  maintain this information,So
> I didn't add this
>  in kernel，we can add this but maybe only for check?

I meant for example, a simulator has two AS. How can we attach an ioas
to a specific AS with the above uAPI?

Thanks

> this in
> Thanks
> Cindy
> > > +
> > > +#define VDPA_DEVICE_ATTACH_IOMMUFD_AS \
> > > +       _IOW(VHOST_VIRTIO, 0x82, struct vdpa_device_attach_iommufd_as)
> > > +
> > > +/*
> > > + * VDPA_DEVICE_DETACH_IOMMUFD_AS
> > > + *
> > > + * Detach a vdpa device from the iommufd address space it has been
> > > + * attached to. After it, device should be in a blocking DMA state.
> > > + *
> > > + * Available only after a device has been bound to iommufd via
> > > + * VHOST_VDPA_SET_IOMMU_FD
> > > + *
> > > + * @argsz:     user filled size of this data.
> > > + * @flags:     must be 0.
> > > + *
> > > + * Return: 0 on success, -errno on failure.
> > > + */
> > > +struct vdpa_device_detach_iommufd_as {
> > > +       __u32 argsz;
> > > +       __u32 flags;
> > > +};
> > > +
> > > +#define VDPA_DEVICE_DETACH_IOMMUFD_AS \
> > > +       _IOW(VHOST_VIRTIO, 0x83, struct vdpa_device_detach_iommufd_as)
> > > +
> > >  #endif
> > > --
> > > 2.34.3
> > >
> >
>

Re: [RFC v1 8/8] iommu: expose the function iommu_device_use_default_domain

[Jason Wang]

On Tue, Nov 7, 2023 at 2:10 PM Cindy Lu <lulu@redhat.com> wrote:
>
> On Mon, Nov 6, 2023 at 3:26 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Sat, Nov 4, 2023 at 1:18 AM Cindy Lu <lulu@redhat.com> wrote:
> > >
> > > Expose the function iommu_device_use_default_domain() and
> > > iommu_device_unuse_default_domain()，
> > > While vdpa bind the iommufd device and detach the iommu device,
> > > vdpa need to call the function
> > > iommu_device_unuse_default_domain() to release the owner
> > >
> > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> >
> > This is the end of the series, who is the user then?
> >
> > Thanks
> >
> hi Jason
> These 2 functions was called in vhost_vdpa_iommufd_set_device(), Vdpa need to
> release the dma owner, otherwise, the function will fail when
> iommufd called iommu_device_claim_dma_owner() in iommufd_device_bind()
> I will change this sequence, Or maybe will find some other way to fix
> this problem
> thanks

I meant exporting helpers needs to be done before the real users.

Thanks

> cindy
>
>
> > > ---
> > >  drivers/iommu/iommu.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index 3bfc56df4f78..987cbf8c9a87 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -3164,6 +3164,7 @@ int iommu_device_use_default_domain(struct device *dev)
> > >
> > >         return ret;
> > >  }
> > > +EXPORT_SYMBOL_GPL(iommu_device_use_default_domain);
> > >
> > >  /**
> > >   * iommu_device_unuse_default_domain() - Device driver stops handling device
> > > @@ -3187,6 +3188,7 @@ void iommu_device_unuse_default_domain(struct device *dev)
> > >         mutex_unlock(&group->mutex);
> > >         iommu_group_put(group);
> > >  }
> > > +EXPORT_SYMBOL_GPL(iommu_device_unuse_default_domain);
> > >
> > >  static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
> > >  {
> > > --
> > > 2.34.3
> > >
> >
>

Re: [RFC v1 3/8] vhost: Add 3 new uapi to support iommufd

[Cindy Lu]

On Wed, Nov 8, 2023 at 11:03 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Tue, Nov 7, 2023 at 2:57 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > On Mon, Nov 6, 2023 at 3:30 PM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Sat, Nov 4, 2023 at 1:17 AM Cindy Lu <lulu@redhat.com> wrote:
> > > >
> > > > VHOST_VDPA_SET_IOMMU_FD: bind the device to iommufd device
> > > >
> > > > VDPA_DEVICE_ATTACH_IOMMUFD_AS: Attach a vdpa device to an iommufd
> > > > address space specified by IOAS id.
> > > >
> > > > VDPA_DEVICE_DETACH_IOMMUFD_AS: Detach a vdpa device
> > > > from the iommufd address space
> > > >
> > > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > > ---
> > >
> > > [...]
> > >
> > > > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > > > index f5c48b61ab62..07e1b2c443ca 100644
> > > > --- a/include/uapi/linux/vhost.h
> > > > +++ b/include/uapi/linux/vhost.h
> > > > @@ -219,4 +219,70 @@
> > > >   */
> > > >  #define VHOST_VDPA_RESUME              _IO(VHOST_VIRTIO, 0x7E)
> > > >
> > > > +/* vhost_vdpa_set_iommufd
> > > > + * Input parameters:
> > > > + * @iommufd: file descriptor from /dev/iommu; pass -1 to unset
> > > > + * @iommufd_ioasid: IOAS identifier returned from ioctl(IOMMU_IOAS_ALLOC)
> > > > + * Output parameters:
> > > > + * @out_dev_id: device identifier
> > > > + */
> > > > +struct vhost_vdpa_set_iommufd {
> > > > +       __s32 iommufd;
> > > > +       __u32 iommufd_ioasid;
> > > > +       __u32 out_dev_id;
> > > > +};
> > > > +
> > > > +#define VHOST_VDPA_SET_IOMMU_FD \
> > > > +       _IOW(VHOST_VIRTIO, 0x7F, struct vhost_vdpa_set_iommufd)
> > > > +
> > > > +/*
> > > > + * VDPA_DEVICE_ATTACH_IOMMUFD_AS -
> > > > + * _IOW(VHOST_VIRTIO, 0x7f, struct vdpa_device_attach_iommufd_as)
> > > > + *
> > > > + * Attach a vdpa device to an iommufd address space specified by IOAS
> > > > + * id.
> > > > + *
> > > > + * Available only after a device has been bound to iommufd via
> > > > + * VHOST_VDPA_SET_IOMMU_FD
> > > > + *
> > > > + * Undo by VDPA_DEVICE_DETACH_IOMMUFD_AS or device fd close.
> > > > + *
> > > > + * @argsz:     user filled size of this data.
> > > > + * @flags:     must be 0.
> > > > + * @ioas_id:   Input the target id which can represent an ioas
> > > > + *             allocated via iommufd subsystem.
> > > > + *
> > > > + * Return: 0 on success, -errno on failure.
> > > > + */
> > > > +struct vdpa_device_attach_iommufd_as {
> > > > +       __u32 argsz;
> > > > +       __u32 flags;
> > > > +       __u32 ioas_id;
> > > > +};
> > >
> > > I think we need to map ioas to vDPA AS, so there should be an ASID
> > > from the view of vDPA?
> > >
> > > Thanks
> > >
> > The qemu will have a structure save and  maintain this information,So
> > I didn't add this
> >  in kernel，we can add this but maybe only for check?
>
> I meant for example, a simulator has two AS. How can we attach an ioas
> to a specific AS with the above uAPI?
>
> Thank>
this   __u32 ioas_id here is alloc from the iommufd system. maybe I
need to change to new name iommuds_asid to
make this more clear
the process in qemu is

1) qemu want to use AS 0 (for example)
2) checking the existing asid. the asid 0 not used before
3 )alloc new asid from iommufd system, get new ioas_id (maybe 3 for example)
qemu will save this relation 3<-->0 in the driver.
4) setting the ioctl VDPA_DEVICE_ATTACH_IOMMUFD_AS to attach new ASID
to the kernel
5） while map the memory， qemu will use ASID 3 to map /umap
and use ASID 0 for legacy mode map/umap

So kernel here will not maintain the ioas_id from iommufd，
and this also make the code strange since there will 2 different asid
for the same AS, maybe we can save these information in the kernel
Thanks
cindy
> > Thanks
> > Cindy
> > > > +
> > > > +#define VDPA_DEVICE_ATTACH_IOMMUFD_AS \
> > > > +       _IOW(VHOST_VIRTIO, 0x82, struct vdpa_device_attach_iommufd_as)
> > > > +
> > > > +/*
> > > > + * VDPA_DEVICE_DETACH_IOMMUFD_AS
> > > > + *
> > > > + * Detach a vdpa device from the iommufd address space it has been
> > > > + * attached to. After it, device should be in a blocking DMA state.
> > > > + *
> > > > + * Available only after a device has been bound to iommufd via
> > > > + * VHOST_VDPA_SET_IOMMU_FD
> > > > + *
> > > > + * @argsz:     user filled size of this data.
> > > > + * @flags:     must be 0.
> > > > + *
> > > > + * Return: 0 on success, -errno on failure.
> > > > + */
> > > > +struct vdpa_device_detach_iommufd_as {
> > > > +       __u32 argsz;
> > > > +       __u32 flags;
> > > > +};
> > > > +
> > > > +#define VDPA_DEVICE_DETACH_IOMMUFD_AS \
> > > > +       _IOW(VHOST_VIRTIO, 0x83, struct vdpa_device_detach_iommufd_as)
> > > > +
> > > >  #endif
> > > > --
> > > > 2.34.3
> > > >
> > >
> >
>

Re: [RFC v1 8/8] iommu: expose the function iommu_device_use_default_domain

[Cindy Lu]

On Wed, Nov 8, 2023 at 11:04 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Tue, Nov 7, 2023 at 2:10 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > On Mon, Nov 6, 2023 at 3:26 PM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Sat, Nov 4, 2023 at 1:18 AM Cindy Lu <lulu@redhat.com> wrote:
> > > >
> > > > Expose the function iommu_device_use_default_domain() and
> > > > iommu_device_unuse_default_domain()，
> > > > While vdpa bind the iommufd device and detach the iommu device,
> > > > vdpa need to call the function
> > > > iommu_device_unuse_default_domain() to release the owner
> > > >
> > > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > >
> > > This is the end of the series, who is the user then?
> > >
> > > Thanks
> > >
> > hi Jason
> > These 2 functions was called in vhost_vdpa_iommufd_set_device(), Vdpa need to
> > release the dma owner, otherwise, the function will fail when
> > iommufd called iommu_device_claim_dma_owner() in iommufd_device_bind()
> > I will change this sequence, Or maybe will find some other way to fix
> > this problem
> > thanks
>
> I meant exporting helpers needs to be done before the real users.
>
> Thanks
>
sure will fix
Thanks
Cindy
> > cindy
> >
> >
> > > > ---
> > > >  drivers/iommu/iommu.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > > index 3bfc56df4f78..987cbf8c9a87 100644
> > > > --- a/drivers/iommu/iommu.c
> > > > +++ b/drivers/iommu/iommu.c
> > > > @@ -3164,6 +3164,7 @@ int iommu_device_use_default_domain(struct device *dev)
> > > >
> > > >         return ret;
> > > >  }
> > > > +EXPORT_SYMBOL_GPL(iommu_device_use_default_domain);
> > > >
> > > >  /**
> > > >   * iommu_device_unuse_default_domain() - Device driver stops handling device
> > > > @@ -3187,6 +3188,7 @@ void iommu_device_unuse_default_domain(struct device *dev)
> > > >         mutex_unlock(&group->mutex);
> > > >         iommu_group_put(group);
> > > >  }
> > > > +EXPORT_SYMBOL_GPL(iommu_device_unuse_default_domain);
> > > >
> > > >  static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
> > > >  {
> > > > --
> > > > 2.34.3
> > > >
> > >
> >
>

Re: [RFC v1 3/8] vhost: Add 3 new uapi to support iommufd

[Jason Wang]

On Wed, Nov 8, 2023 at 2:39 PM Cindy Lu <lulu@redhat.com> wrote:
>
> On Wed, Nov 8, 2023 at 11:03 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Tue, Nov 7, 2023 at 2:57 PM Cindy Lu <lulu@redhat.com> wrote:
> > >
> > > On Mon, Nov 6, 2023 at 3:30 PM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Sat, Nov 4, 2023 at 1:17 AM Cindy Lu <lulu@redhat.com> wrote:
> > > > >
> > > > > VHOST_VDPA_SET_IOMMU_FD: bind the device to iommufd device
> > > > >
> > > > > VDPA_DEVICE_ATTACH_IOMMUFD_AS: Attach a vdpa device to an iommufd
> > > > > address space specified by IOAS id.
> > > > >
> > > > > VDPA_DEVICE_DETACH_IOMMUFD_AS: Detach a vdpa device
> > > > > from the iommufd address space
> > > > >
> > > > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > > > ---
> > > >
> > > > [...]
> > > >
> > > > > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > > > > index f5c48b61ab62..07e1b2c443ca 100644
> > > > > --- a/include/uapi/linux/vhost.h
> > > > > +++ b/include/uapi/linux/vhost.h
> > > > > @@ -219,4 +219,70 @@
> > > > >   */
> > > > >  #define VHOST_VDPA_RESUME              _IO(VHOST_VIRTIO, 0x7E)
> > > > >
> > > > > +/* vhost_vdpa_set_iommufd
> > > > > + * Input parameters:
> > > > > + * @iommufd: file descriptor from /dev/iommu; pass -1 to unset
> > > > > + * @iommufd_ioasid: IOAS identifier returned from ioctl(IOMMU_IOAS_ALLOC)
> > > > > + * Output parameters:
> > > > > + * @out_dev_id: device identifier
> > > > > + */
> > > > > +struct vhost_vdpa_set_iommufd {
> > > > > +       __s32 iommufd;
> > > > > +       __u32 iommufd_ioasid;
> > > > > +       __u32 out_dev_id;
> > > > > +};
> > > > > +
> > > > > +#define VHOST_VDPA_SET_IOMMU_FD \
> > > > > +       _IOW(VHOST_VIRTIO, 0x7F, struct vhost_vdpa_set_iommufd)
> > > > > +
> > > > > +/*
> > > > > + * VDPA_DEVICE_ATTACH_IOMMUFD_AS -
> > > > > + * _IOW(VHOST_VIRTIO, 0x7f, struct vdpa_device_attach_iommufd_as)
> > > > > + *
> > > > > + * Attach a vdpa device to an iommufd address space specified by IOAS
> > > > > + * id.
> > > > > + *
> > > > > + * Available only after a device has been bound to iommufd via
> > > > > + * VHOST_VDPA_SET_IOMMU_FD
> > > > > + *
> > > > > + * Undo by VDPA_DEVICE_DETACH_IOMMUFD_AS or device fd close.
> > > > > + *
> > > > > + * @argsz:     user filled size of this data.
> > > > > + * @flags:     must be 0.
> > > > > + * @ioas_id:   Input the target id which can represent an ioas
> > > > > + *             allocated via iommufd subsystem.
> > > > > + *
> > > > > + * Return: 0 on success, -errno on failure.
> > > > > + */
> > > > > +struct vdpa_device_attach_iommufd_as {
> > > > > +       __u32 argsz;
> > > > > +       __u32 flags;
> > > > > +       __u32 ioas_id;
> > > > > +};
> > > >
> > > > I think we need to map ioas to vDPA AS, so there should be an ASID
> > > > from the view of vDPA?
> > > >
> > > > Thanks
> > > >
> > > The qemu will have a structure save and  maintain this information,So
> > > I didn't add this
> > >  in kernel，we can add this but maybe only for check?
> >
> > I meant for example, a simulator has two AS. How can we attach an ioas
> > to a specific AS with the above uAPI?
> >
> > Thank>
> this   __u32 ioas_id here is alloc from the iommufd system. maybe I
> need to change to new name iommuds_asid to
> make this more clear
> the process in qemu is
>
> 1) qemu want to use AS 0 (for example)
> 2) checking the existing asid. the asid 0 not used before
> 3 )alloc new asid from iommufd system, get new ioas_id (maybe 3 for example)
> qemu will save this relation 3<-->0 in the driver.
> 4) setting the ioctl VDPA_DEVICE_ATTACH_IOMMUFD_AS to attach new ASID
> to the kernel

So if we want to map IOMMUFD AS 3 to VDPA AS 0, how can it be done?

For example I didn't see a vDPA AS parameter in the above uAPI.

vhost_vdpa_set_iommufd has iommufd_ioasid which is obviously not the vDPA AS.

And ioas_id of vdpa_device_attach_iommufd_as (as you explained above)
is not vDPA AS.

Thanks


> 5） while map the memory， qemu will use ASID 3 to map /umap
> and use ASID 0 for legacy mode map/umap
>
> So kernel here will not maintain the ioas_id from iommufd，
> and this also make the code strange since there will 2 different asid
> for the same AS, maybe we can save these information in the kernel
> Thanks
> cindy
> > > Thanks
> > > Cindy
> > > > > +
> > > > > +#define VDPA_DEVICE_ATTACH_IOMMUFD_AS \
> > > > > +       _IOW(VHOST_VIRTIO, 0x82, struct vdpa_device_attach_iommufd_as)
> > > > > +
> > > > > +/*
> > > > > + * VDPA_DEVICE_DETACH_IOMMUFD_AS
> > > > > + *
> > > > > + * Detach a vdpa device from the iommufd address space it has been
> > > > > + * attached to. After it, device should be in a blocking DMA state.
> > > > > + *
> > > > > + * Available only after a device has been bound to iommufd via
> > > > > + * VHOST_VDPA_SET_IOMMU_FD
> > > > > + *
> > > > > + * @argsz:     user filled size of this data.
> > > > > + * @flags:     must be 0.
> > > > > + *
> > > > > + * Return: 0 on success, -errno on failure.
> > > > > + */
> > > > > +struct vdpa_device_detach_iommufd_as {
> > > > > +       __u32 argsz;
> > > > > +       __u32 flags;
> > > > > +};
> > > > > +
> > > > > +#define VDPA_DEVICE_DETACH_IOMMUFD_AS \
> > > > > +       _IOW(VHOST_VIRTIO, 0x83, struct vdpa_device_detach_iommufd_as)
> > > > > +
> > > > >  #endif
> > > > > --
> > > > > 2.34.3
> > > > >
> > > >
> > >
> >
>

Re: [RFC v1 0/8] vhost-vdpa: add support for iommufd

[Michael S. Tsirkin]

On Tue, Nov 07, 2023 at 11:52:17AM -0400, Jason Gunthorpe wrote:
> On Tue, Nov 07, 2023 at 09:30:21AM -0500, Michael S. Tsirkin wrote:
> > On Tue, Nov 07, 2023 at 10:12:37AM -0400, Jason Gunthorpe wrote:
> > > Big company's should take the responsibility to train and provide
> > > skill development for their own staff.
> > 
> > That would result in a beautiful cathedral of a patch. I know this is
> > how some companies work. We are doing more of a bazaar thing here,
> > though. In a bunch of subsystems it seems that you don't get the
> > necessary skills until you have been publically shouted at by
> > maintainers - better to start early ;). Not a nice environment for
> > novices, for sure.
> 
> In my view the "shouting from maintainers" is harmful to the people
> buidling skills and it is an unkind thing to dump employees into that
> kind of situation.
> 
> They should have help to establish the basic level of competence where
> they may do the wrong thing, but all the process and presentation of
> the wrong thing is top notch. You get a much better reception.
> 
> Jason

What - like e.g. mechanically fixing checkpatch warnings without
understanding? I actually very much dislike it when people take a bad
patch and just polish the presentation
- it is easier for me if I can judge patch quality quickly from the
presentation. Matter of taste I guess.

-- 
MST

Re: [RFC v1 3/8] vhost: Add 3 new uapi to support iommufd

[Jason Wang]

On Wed, Nov 8, 2023 at 3:09 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Nov 8, 2023 at 2:39 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > On Wed, Nov 8, 2023 at 11:03 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Tue, Nov 7, 2023 at 2:57 PM Cindy Lu <lulu@redhat.com> wrote:
> > > >
> > > > On Mon, Nov 6, 2023 at 3:30 PM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Sat, Nov 4, 2023 at 1:17 AM Cindy Lu <lulu@redhat.com> wrote:
> > > > > >
> > > > > > VHOST_VDPA_SET_IOMMU_FD: bind the device to iommufd device
> > > > > >
> > > > > > VDPA_DEVICE_ATTACH_IOMMUFD_AS: Attach a vdpa device to an iommufd
> > > > > > address space specified by IOAS id.
> > > > > >
> > > > > > VDPA_DEVICE_DETACH_IOMMUFD_AS: Detach a vdpa device
> > > > > > from the iommufd address space
> > > > > >
> > > > > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > > > > ---
> > > > >
> > > > > [...]
> > > > >
> > > > > > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > > > > > index f5c48b61ab62..07e1b2c443ca 100644
> > > > > > --- a/include/uapi/linux/vhost.h
> > > > > > +++ b/include/uapi/linux/vhost.h
> > > > > > @@ -219,4 +219,70 @@
> > > > > >   */
> > > > > >  #define VHOST_VDPA_RESUME              _IO(VHOST_VIRTIO, 0x7E)
> > > > > >
> > > > > > +/* vhost_vdpa_set_iommufd
> > > > > > + * Input parameters:
> > > > > > + * @iommufd: file descriptor from /dev/iommu; pass -1 to unset
> > > > > > + * @iommufd_ioasid: IOAS identifier returned from ioctl(IOMMU_IOAS_ALLOC)
> > > > > > + * Output parameters:
> > > > > > + * @out_dev_id: device identifier
> > > > > > + */
> > > > > > +struct vhost_vdpa_set_iommufd {
> > > > > > +       __s32 iommufd;
> > > > > > +       __u32 iommufd_ioasid;
> > > > > > +       __u32 out_dev_id;
> > > > > > +};
> > > > > > +
> > > > > > +#define VHOST_VDPA_SET_IOMMU_FD \
> > > > > > +       _IOW(VHOST_VIRTIO, 0x7F, struct vhost_vdpa_set_iommufd)
> > > > > > +
> > > > > > +/*
> > > > > > + * VDPA_DEVICE_ATTACH_IOMMUFD_AS -
> > > > > > + * _IOW(VHOST_VIRTIO, 0x7f, struct vdpa_device_attach_iommufd_as)
> > > > > > + *
> > > > > > + * Attach a vdpa device to an iommufd address space specified by IOAS
> > > > > > + * id.
> > > > > > + *
> > > > > > + * Available only after a device has been bound to iommufd via
> > > > > > + * VHOST_VDPA_SET_IOMMU_FD
> > > > > > + *
> > > > > > + * Undo by VDPA_DEVICE_DETACH_IOMMUFD_AS or device fd close.
> > > > > > + *
> > > > > > + * @argsz:     user filled size of this data.
> > > > > > + * @flags:     must be 0.
> > > > > > + * @ioas_id:   Input the target id which can represent an ioas
> > > > > > + *             allocated via iommufd subsystem.
> > > > > > + *
> > > > > > + * Return: 0 on success, -errno on failure.
> > > > > > + */
> > > > > > +struct vdpa_device_attach_iommufd_as {
> > > > > > +       __u32 argsz;
> > > > > > +       __u32 flags;
> > > > > > +       __u32 ioas_id;
> > > > > > +};
> > > > >
> > > > > I think we need to map ioas to vDPA AS, so there should be an ASID
> > > > > from the view of vDPA?
> > > > >
> > > > > Thanks
> > > > >
> > > > The qemu will have a structure save and  maintain this information,So
> > > > I didn't add this
> > > >  in kernel，we can add this but maybe only for check?
> > >
> > > I meant for example, a simulator has two AS. How can we attach an ioas
> > > to a specific AS with the above uAPI?
> > >
> > > Thank>
> > this   __u32 ioas_id here is alloc from the iommufd system. maybe I
> > need to change to new name iommuds_asid to
> > make this more clear
> > the process in qemu is
> >
> > 1) qemu want to use AS 0 (for example)
> > 2) checking the existing asid. the asid 0 not used before
> > 3 )alloc new asid from iommufd system, get new ioas_id (maybe 3 for example)
> > qemu will save this relation 3<-->0 in the driver.
> > 4) setting the ioctl VDPA_DEVICE_ATTACH_IOMMUFD_AS to attach new ASID
> > to the kernel
>
> So if we want to map IOMMUFD AS 3 to VDPA AS 0, how can it be done?
>
> For example I didn't see a vDPA AS parameter in the above uAPI.
>
> vhost_vdpa_set_iommufd has iommufd_ioasid which is obviously not the vDPA AS.
>
> And ioas_id of vdpa_device_attach_iommufd_as (as you explained above)
> is not vDPA AS.

For example, the simulator/mlx5e has two ASes. It needs to know the
mapping between vDPA AS and iommufd AS. Otherwise the translation will
be problematic.

Thanks

>
> Thanks
>
>
> > 5） while map the memory， qemu will use ASID 3 to map /umap
> > and use ASID 0 for legacy mode map/umap
> >
> > So kernel here will not maintain the ioas_id from iommufd，
> > and this also make the code strange since there will 2 different asid
> > for the same AS, maybe we can save these information in the kernel
> > Thanks
> > cindy
> > > > Thanks
> > > > Cindy
> > > > > > +
> > > > > > +#define VDPA_DEVICE_ATTACH_IOMMUFD_AS \
> > > > > > +       _IOW(VHOST_VIRTIO, 0x82, struct vdpa_device_attach_iommufd_as)
> > > > > > +
> > > > > > +/*
> > > > > > + * VDPA_DEVICE_DETACH_IOMMUFD_AS
> > > > > > + *
> > > > > > + * Detach a vdpa device from the iommufd address space it has been
> > > > > > + * attached to. After it, device should be in a blocking DMA state.
> > > > > > + *
> > > > > > + * Available only after a device has been bound to iommufd via
> > > > > > + * VHOST_VDPA_SET_IOMMU_FD
> > > > > > + *
> > > > > > + * @argsz:     user filled size of this data.
> > > > > > + * @flags:     must be 0.
> > > > > > + *
> > > > > > + * Return: 0 on success, -errno on failure.
> > > > > > + */
> > > > > > +struct vdpa_device_detach_iommufd_as {
> > > > > > +       __u32 argsz;
> > > > > > +       __u32 flags;
> > > > > > +};
> > > > > > +
> > > > > > +#define VDPA_DEVICE_DETACH_IOMMUFD_AS \
> > > > > > +       _IOW(VHOST_VIRTIO, 0x83, struct vdpa_device_detach_iommufd_as)
> > > > > > +
> > > > > >  #endif
> > > > > > --
> > > > > > 2.34.3
> > > > > >
> > > > >
> > > >
> > >
> >

Re: [RFC v1 3/8] vhost: Add 3 new uapi to support iommufd

[Cindy Lu]

Jason Wang <jasowang@redhat.com> 于2023年11月10日周五 10:32写道：
>
> On Wed, Nov 8, 2023 at 3:09 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Wed, Nov 8, 2023 at 2:39 PM Cindy Lu <lulu@redhat.com> wrote:
> > >
> > > On Wed, Nov 8, 2023 at 11:03 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Tue, Nov 7, 2023 at 2:57 PM Cindy Lu <lulu@redhat.com> wrote:
> > > > >
> > > > > On Mon, Nov 6, 2023 at 3:30 PM Jason Wang <jasowang@redhat.com> wrote:
> > > > > >
> > > > > > On Sat, Nov 4, 2023 at 1:17 AM Cindy Lu <lulu@redhat.com> wrote:
> > > > > > >
> > > > > > > VHOST_VDPA_SET_IOMMU_FD: bind the device to iommufd device
> > > > > > >
> > > > > > > VDPA_DEVICE_ATTACH_IOMMUFD_AS: Attach a vdpa device to an iommufd
> > > > > > > address space specified by IOAS id.
> > > > > > >
> > > > > > > VDPA_DEVICE_DETACH_IOMMUFD_AS: Detach a vdpa device
> > > > > > > from the iommufd address space
> > > > > > >
> > > > > > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > > > > > ---
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > > > > > > index f5c48b61ab62..07e1b2c443ca 100644
> > > > > > > --- a/include/uapi/linux/vhost.h
> > > > > > > +++ b/include/uapi/linux/vhost.h
> > > > > > > @@ -219,4 +219,70 @@
> > > > > > >   */
> > > > > > >  #define VHOST_VDPA_RESUME              _IO(VHOST_VIRTIO, 0x7E)
> > > > > > >
> > > > > > > +/* vhost_vdpa_set_iommufd
> > > > > > > + * Input parameters:
> > > > > > > + * @iommufd: file descriptor from /dev/iommu; pass -1 to unset
> > > > > > > + * @iommufd_ioasid: IOAS identifier returned from ioctl(IOMMU_IOAS_ALLOC)
> > > > > > > + * Output parameters:
> > > > > > > + * @out_dev_id: device identifier
> > > > > > > + */
> > > > > > > +struct vhost_vdpa_set_iommufd {
> > > > > > > +       __s32 iommufd;
> > > > > > > +       __u32 iommufd_ioasid;
> > > > > > > +       __u32 out_dev_id;
> > > > > > > +};
> > > > > > > +
> > > > > > > +#define VHOST_VDPA_SET_IOMMU_FD \
> > > > > > > +       _IOW(VHOST_VIRTIO, 0x7F, struct vhost_vdpa_set_iommufd)
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * VDPA_DEVICE_ATTACH_IOMMUFD_AS -
> > > > > > > + * _IOW(VHOST_VIRTIO, 0x7f, struct vdpa_device_attach_iommufd_as)
> > > > > > > + *
> > > > > > > + * Attach a vdpa device to an iommufd address space specified by IOAS
> > > > > > > + * id.
> > > > > > > + *
> > > > > > > + * Available only after a device has been bound to iommufd via
> > > > > > > + * VHOST_VDPA_SET_IOMMU_FD
> > > > > > > + *
> > > > > > > + * Undo by VDPA_DEVICE_DETACH_IOMMUFD_AS or device fd close.
> > > > > > > + *
> > > > > > > + * @argsz:     user filled size of this data.
> > > > > > > + * @flags:     must be 0.
> > > > > > > + * @ioas_id:   Input the target id which can represent an ioas
> > > > > > > + *             allocated via iommufd subsystem.
> > > > > > > + *
> > > > > > > + * Return: 0 on success, -errno on failure.
> > > > > > > + */
> > > > > > > +struct vdpa_device_attach_iommufd_as {
> > > > > > > +       __u32 argsz;
> > > > > > > +       __u32 flags;
> > > > > > > +       __u32 ioas_id;
> > > > > > > +};
> > > > > >
> > > > > > I think we need to map ioas to vDPA AS, so there should be an ASID
> > > > > > from the view of vDPA?
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > The qemu will have a structure save and  maintain this information,So
> > > > > I didn't add this
> > > > >  in kernel，we can add this but maybe only for check?
> > > >
> > > > I meant for example, a simulator has two AS. How can we attach an ioas
> > > > to a specific AS with the above uAPI?
> > > >
> > > > Thank>
> > > this   __u32 ioas_id here is alloc from the iommufd system. maybe I
> > > need to change to new name iommuds_asid to
> > > make this more clear
> > > the process in qemu is
> > >
> > > 1) qemu want to use AS 0 (for example)
> > > 2) checking the existing asid. the asid 0 not used before
> > > 3 )alloc new asid from iommufd system, get new ioas_id (maybe 3 for example)
> > > qemu will save this relation 3<-->0 in the driver.
> > > 4) setting the ioctl VDPA_DEVICE_ATTACH_IOMMUFD_AS to attach new ASID
> > > to the kernel
> >
> > So if we want to map IOMMUFD AS 3 to VDPA AS 0, how can it be done?
> >
> > For example I didn't see a vDPA AS parameter in the above uAPI.
> >
> > vhost_vdpa_set_iommufd has iommufd_ioasid which is obviously not the vDPA AS.
> >
> > And ioas_id of vdpa_device_attach_iommufd_as (as you explained above)
> > is not vDPA AS.
>
> For example, the simulator/mlx5e has two ASes. It needs to know the
> mapping between vDPA AS and iommufd AS. Otherwise the translation will
> be problematic.
>
> Thanks
>
Got it, thanks Jason. I will re-write this part
Thanks
Cindy
> >
> > Thanks
> >
> >
> > > 5） while map the memory， qemu will use ASID 3 to map /umap
> > > and use ASID 0 for legacy mode map/umap
> > >
> > > So kernel here will not maintain the ioas_id from iommufd，
> > > and this also make the code strange since there will 2 different asid
> > > for the same AS, maybe we can save these information in the kernel
> > > Thanks
> > > cindy
> > > > > Thanks
> > > > > Cindy
> > > > > > > +
> > > > > > > +#define VDPA_DEVICE_ATTACH_IOMMUFD_AS \
> > > > > > > +       _IOW(VHOST_VIRTIO, 0x82, struct vdpa_device_attach_iommufd_as)
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * VDPA_DEVICE_DETACH_IOMMUFD_AS
> > > > > > > + *
> > > > > > > + * Detach a vdpa device from the iommufd address space it has been
> > > > > > > + * attached to. After it, device should be in a blocking DMA state.
> > > > > > > + *
> > > > > > > + * Available only after a device has been bound to iommufd via
> > > > > > > + * VHOST_VDPA_SET_IOMMU_FD
> > > > > > > + *
> > > > > > > + * @argsz:     user filled size of this data.
> > > > > > > + * @flags:     must be 0.
> > > > > > > + *
> > > > > > > + * Return: 0 on success, -errno on failure.
> > > > > > > + */
> > > > > > > +struct vdpa_device_detach_iommufd_as {
> > > > > > > +       __u32 argsz;
> > > > > > > +       __u32 flags;
> > > > > > > +};
> > > > > > > +
> > > > > > > +#define VDPA_DEVICE_DETACH_IOMMUFD_AS \
> > > > > > > +       _IOW(VHOST_VIRTIO, 0x83, struct vdpa_device_detach_iommufd_as)
> > > > > > > +
> > > > > > >  #endif
> > > > > > > --
> > > > > > > 2.34.3
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
>

Re: [RFC v1 0/8] vhost-vdpa: add support for iommufd

[Jason Gunthorpe]

On Thu, Nov 09, 2023 at 06:48:46PM -0500, Michael S. Tsirkin wrote:
> On Tue, Nov 07, 2023 at 11:52:17AM -0400, Jason Gunthorpe wrote:
> > On Tue, Nov 07, 2023 at 09:30:21AM -0500, Michael S. Tsirkin wrote:
> > > On Tue, Nov 07, 2023 at 10:12:37AM -0400, Jason Gunthorpe wrote:
> > > > Big company's should take the responsibility to train and provide
> > > > skill development for their own staff.
> > > 
> > > That would result in a beautiful cathedral of a patch. I know this is
> > > how some companies work. We are doing more of a bazaar thing here,
> > > though. In a bunch of subsystems it seems that you don't get the
> > > necessary skills until you have been publically shouted at by
> > > maintainers - better to start early ;). Not a nice environment for
> > > novices, for sure.
> > 
> > In my view the "shouting from maintainers" is harmful to the people
> > buidling skills and it is an unkind thing to dump employees into that
> > kind of situation.
> > 
> > They should have help to establish the basic level of competence where
> > they may do the wrong thing, but all the process and presentation of
> > the wrong thing is top notch. You get a much better reception.
> 
> What - like e.g. mechanically fixing checkpatch warnings without
> understanding? 

No, not at all. I mean actually going through and explaining what the
idea is to another person and ensuing that the commit messages convey
that idea, that the patches reflect the idea, that everything is
convayed, and it isn't obviously internally illogical.

Like, why did this series have a giant block of #ifdef 0'd code with
no explanation at all? That isn't checkpatch nitpicks, that is not
meeting the minimum standard to convey an idea in an RFC.

Jason

Re: [RFC v1 0/8] vhost-vdpa: add support for iommufd

[Michael S. Tsirkin]

On Sat, Nov 04, 2023 at 01:16:33AM +0800, Cindy Lu wrote:
> 
> Hi All
> This code provides the iommufd support for vdpa device
> This code fixes the bugs from the last version and also add the asid support. rebase on kernel
> v6,6-rc3
> Test passed in the physical device (vp_vdpa), but  there are still some problems in the emulated device (vdpa_sim_net), 
> I will continue working on it
> 
> The kernel code is
> https://gitlab.com/lulu6/vhost/-/tree/iommufdRFC_v1
> 
> Signed-off-by: Cindy Lu <lulu@redhat.com>

Was this abandoned?

> 
> Cindy Lu (8):
>   vhost/iommufd: Add the functions support iommufd
>   Kconfig: Add the new file vhost/iommufd
>   vhost: Add 3 new uapi to support iommufd
>   vdpa: Add new vdpa_config_ops to support iommufd
>   vdpa_sim :Add support for iommufd
>   vdpa: change the map/unmap process to support iommufd
>   vp_vdpa::Add support for iommufd
>   iommu: expose the function iommu_device_use_default_domain
> 
>  drivers/iommu/iommu.c             |   2 +
>  drivers/vdpa/vdpa_sim/vdpa_sim.c  |   8 ++
>  drivers/vdpa/virtio_pci/vp_vdpa.c |   4 +
>  drivers/vhost/Kconfig             |   1 +
>  drivers/vhost/Makefile            |   1 +
>  drivers/vhost/iommufd.c           | 178 +++++++++++++++++++++++++
>  drivers/vhost/vdpa.c              | 210 +++++++++++++++++++++++++++++-
>  drivers/vhost/vhost.h             |  21 +++
>  include/linux/vdpa.h              |  38 +++++-
>  include/uapi/linux/vhost.h        |  66 ++++++++++
>  10 files changed, 525 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/vhost/iommufd.c
> 
> -- 
> 2.34.3

Re: [RFC v1 0/8] vhost-vdpa: add support for iommufd

[Cindy Lu]

On Thu, Jan 11, 2024 at 6:25 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sat, Nov 04, 2023 at 01:16:33AM +0800, Cindy Lu wrote:
> >
> > Hi All
> > This code provides the iommufd support for vdpa device
> > This code fixes the bugs from the last version and also add the asid support. rebase on kernel
> > v6,6-rc3
> > Test passed in the physical device (vp_vdpa), but  there are still some problems in the emulated device (vdpa_sim_net),
> > I will continue working on it
> >
> > The kernel code is
> > https://gitlab.com/lulu6/vhost/-/tree/iommufdRFC_v1
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
>
> Was this abandoned?
>
Thanks Micheal. I'm really sorry for the delay. I will continue working on this
Thanks
Cindy
> >
> > Cindy Lu (8):
> >   vhost/iommufd: Add the functions support iommufd
> >   Kconfig: Add the new file vhost/iommufd
> >   vhost: Add 3 new uapi to support iommufd
> >   vdpa: Add new vdpa_config_ops to support iommufd
> >   vdpa_sim :Add support for iommufd
> >   vdpa: change the map/unmap process to support iommufd
> >   vp_vdpa::Add support for iommufd
> >   iommu: expose the function iommu_device_use_default_domain
> >
> >  drivers/iommu/iommu.c             |   2 +
> >  drivers/vdpa/vdpa_sim/vdpa_sim.c  |   8 ++
> >  drivers/vdpa/virtio_pci/vp_vdpa.c |   4 +
> >  drivers/vhost/Kconfig             |   1 +
> >  drivers/vhost/Makefile            |   1 +
> >  drivers/vhost/iommufd.c           | 178 +++++++++++++++++++++++++
> >  drivers/vhost/vdpa.c              | 210 +++++++++++++++++++++++++++++-
> >  drivers/vhost/vhost.h             |  21 +++
> >  include/linux/vdpa.h              |  38 +++++-
> >  include/uapi/linux/vhost.h        |  66 ++++++++++
> >  10 files changed, 525 insertions(+), 4 deletions(-)
> >  create mode 100644 drivers/vhost/iommufd.c
> >
> > --
> > 2.34.3
>