Re: [PATCH v4 05/12] vfio/iommufd: Introduce auto domain creation

[Cédric Le Goater]

On 7/18/24 15:47, Joao Martins wrote:
> On 17/07/2024 10:31, Joao Martins wrote:
> > On 17/07/2024 10:28, Cédric Le Goater wrote:
> > > > > > > > > @@ -224,6 +300,11 @@ static void
> > > > > > iommufd_cdev_detach_container(VFIODevice *vbasedev,
> > > > > > > > >    {
> > > > > > > > >        Error *err = NULL;
> > > > > > > > >
> > > > > > > > > +    if (vbasedev->hwpt) {
> > > > > > > > > +        iommufd_cdev_autodomains_put(vbasedev, container);
> > > > > > > > > +        return;
> > > > > > > > Where do we detach the device from the hwpt?
> > > > > > > In iommufd_backend_free_id() for auto domains
> > > > > >
> > > > > > to clarify here I meant *userspace* auto domains
> > > > > >
> > > > > > *kernel* auto domains (mdev) goes via DETACH_IOMMUFD_PT
> > > > >
> > > > > If the device is still attached to the hwpt, will iommufd_backend_free_id()
> > > > > succeed?
> > > > > Have you tried the hot unplug?
> > > >
> > > > I have but I didn't see any errors. But I will check again for v5 as it could
> > > > also be my oversight.
> > > >
> > > > I was thinking about Eric's remark overnight and I think what I am doing is not
> > > > correct regardless of the above.
> > > >
> > > > I should be calling DETACH_IOMMUFD_PT pairing with ATTACH_IOMMUFD_PT, and the
> > > > iommufd_backend_free_id() is to drop the final reference pairing with
> > > > alloc_hwpt() when the device list is empty i.e. when there's no more devices in
> > > > that vdev::hwpt.
> > > >
> > > > DETACH_IOMMUFD_PT decrement the hwpt refcount and it doesn't differentiate
> > > > between auto domains vs manual domains.
> > > >
> > > > The code is already there anyhow it just has the order of
> > > > iommufd_cdev_autodomains_put vs detach invocation reversed; I'll fix that for
> > > > next version.
> > >
> > > While at it, could you please move these routines :
> > >
> > >    iommufd_cdev_detach_ioas_hwpt
> > >    iommufd_cdev_attach_ioas_hwpt
> > >   
> > > under backends/iommufd.c ? I think that's where they belong.
> >
> > OK
>
> At the first glance I thought this was a good idea. But these functions while
> they attach an IOMMUFD they do not really talk to an IOMMUFD backend, but to a
> VFIO device file descriptor. Now I think they are in the right place here and we
> would leave IOMMUFD uAPI things to backends/iommufd and VFIO APIs in hw/vfio/.

yep. I was misled by vbasedev->iommufd->fd which is only used in the trace 
event.
Let's keep things how they are. Thanks for looking,

C.



> It also uses a lot of VFIODevice* which requires some funny includes in
> sysemu/iommufd.h.
>
> Do you still want me to go ahead with it? Here's a snip below of the change
> involved:
>
> diff --git a/backends/iommufd.c b/backends/iommufd.c
> index 2b3d51af26d2..19d1e430ef48 100644
> --- a/backends/iommufd.c
> +++ b/backends/iommufd.c
> @@ -20,6 +20,7 @@
>   #include "trace.h"
>   #include <sys/ioctl.h>
>   #include <linux/iommufd.h>
> +#include <linux/vfio.h>
>
>   static void iommufd_backend_init(Object *obj)
>   {
> @@ -232,6 +233,46 @@ bool iommufd_backend_get_device_info(IOMMUFDBackend *be,
> uint32_t devid,
>       return true;
>   }
>
> +int iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, uint32_t id,
> +                                  Error **errp)
> +{
> +    int iommufd = vbasedev->iommufd->fd;
> +    struct vfio_device_attach_iommufd_pt attach_data = {
> +        .argsz = sizeof(attach_data),
> +        .flags = 0,
> +        .pt_id = id,
> +    };
> +
> +    /* Attach device to an IOAS or hwpt within iommufd */
> +    if (ioctl(vbasedev->fd, VFIO_DEVICE_ATTACH_IOMMUFD_PT, &attach_data)) {
> +        error_setg_errno(errp, errno,
> +                         "[iommufd=%d] error attach %s (%d) to id=%d",
> +                         iommufd, vbasedev->name, vbasedev->fd, id);
> +        return -errno;
> +    }
> +
> +    trace_iommufd_cdev_attach_ioas_hwpt(iommufd, vbasedev->name,
> +                                        vbasedev->fd, id);
> +    return 0;
> +}
> +
> +bool iommufd_cdev_detach_ioas_hwpt(VFIODevice *vbasedev, Error **errp)
> +{
> +    int iommufd = vbasedev->iommufd->fd;
> +    struct vfio_device_detach_iommufd_pt detach_data = {
> +        .argsz = sizeof(detach_data),
> +        .flags = 0,
> +    };
> +
> +    if (ioctl(vbasedev->fd, VFIO_DEVICE_DETACH_IOMMUFD_PT, &detach_data)) {
> +        error_setg_errno(errp, errno, "detach %s failed", vbasedev->name);
> +        return false;
> +    }
> +
> +    trace_iommufd_cdev_detach_ioas_hwpt(iommufd, vbasedev->name);
> +    return true;
> +}
> +
>   static int hiod_iommufd_get_cap(HostIOMMUDevice *hiod, int cap, Error **errp)
>   {
>       HostIOMMUDeviceCaps *caps = &hiod->caps;
> diff --git a/backends/trace-events b/backends/trace-events
> index 211e6f374adc..2fee8e0af20e 100644
> --- a/backends/trace-events
> +++ b/backends/trace-events
> @@ -15,3 +15,5 @@ iommufd_backend_unmap_dma_non_exist(int iommufd, uint32_t
> ioas, uint64_t iova, u
>   iommufd_backend_unmap_dma(int iommufd, uint32_t ioas, uint64_t iova, uint64_t
> size, int ret) " iommufd=%d ioas=%d iova=0x%"PRIx64" s
> ize=0x%"PRIx64" (%d)"
>   iommufd_backend_alloc_ioas(int iommufd, uint32_t ioas) " iommufd=%d ioas=%d"
>   iommufd_backend_free_id(int iommufd, uint32_t id, int ret) " iommufd=%d id=%d 
> (%d)"
> +iommufd_cdev_attach_ioas_hwpt(int iommufd, const char *name, int devfd, int id)
> " [iommufd=%d] Successfully attached device %s (%d)
> to id=%d"
> +iommufd_cdev_detach_ioas_hwpt(int iommufd, const char *name) " [iommufd=%d]
> Successfully detached %s"
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 077dea8f1b64..5a6d56c915e2 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -172,46 +172,6 @@ out:
>       return ret;
>   }
>
> -static int iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, uint32_t id,
> -                                         Error **errp)
> -{
> -    int iommufd = vbasedev->iommufd->fd;
> -    struct vfio_device_attach_iommufd_pt attach_data = {
> -        .argsz = sizeof(attach_data),
> -        .flags = 0,
> -        .pt_id = id,
> -    };
> -
> -    /* Attach device to an IOAS or hwpt within iommufd */
> -    if (ioctl(vbasedev->fd, VFIO_DEVICE_ATTACH_IOMMUFD_PT, &attach_data)) {
> -        error_setg_errno(errp, errno,
> -                         "[iommufd=%d] error attach %s (%d) to id=%d",
> -                         iommufd, vbasedev->name, vbasedev->fd, id);
> -        return -errno;
> -    }
> -
> -    trace_iommufd_cdev_attach_ioas_hwpt(iommufd, vbasedev->name,
> -                                        vbasedev->fd, id);
> -    return 0;
> -}
> -
> -static bool iommufd_cdev_detach_ioas_hwpt(VFIODevice *vbasedev, Error **errp)
> -{
> -    int iommufd = vbasedev->iommufd->fd;
> -    struct vfio_device_detach_iommufd_pt detach_data = {
> -        .argsz = sizeof(detach_data),
> -        .flags = 0,
> -    };
> -
> -    if (ioctl(vbasedev->fd, VFIO_DEVICE_DETACH_IOMMUFD_PT, &detach_data)) {
> -        error_setg_errno(errp, errno, "detach %s failed", vbasedev->name);
> -        return false;
> -    }
> -
> -    trace_iommufd_cdev_detach_ioas_hwpt(iommufd, vbasedev->name);
> -    return true;
> -}
> -
>   static bool iommufd_cdev_attach_container(VFIODevice *vbasedev,
>                                             VFIOIOMMUFDContainer *container,
>                                             Error **errp)
> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> index e16179b507ed..24fde6270112 100644
> --- a/hw/vfio/trace-events
> +++ b/hw/vfio/trace-events
> @@ -170,8 +170,6 @@ vfio_vmstate_change_prepare(const char *name, int running,
> const char *reason, c
>
>   iommufd_cdev_connect_and_bind(int iommufd, const char *name, int devfd, int
> devid) " [iommufd=%d] Successfully bound device %s (fd=%
> d): output devid=%d"
>   iommufd_cdev_getfd(const char *dev, int devfd) " %s (fd=%d)"
> -iommufd_cdev_attach_ioas_hwpt(int iommufd, const char *name, int devfd, int id)
> " [iommufd=%d] Successfully attached device %s (%d)
> to id=%d"
> -iommufd_cdev_detach_ioas_hwpt(int iommufd, const char *name) " [iommufd=%d]
> Successfully detached %s"
>   iommufd_cdev_fail_attach_existing_container(const char *msg) " %s"
>   iommufd_cdev_alloc_ioas(int iommufd, int ioas_id) " [iommufd=%d] new IOMMUFD
> container with ioasid=%d"
>   iommufd_cdev_device_info(char *name, int devfd, int num_irqs, int num_regions,
> int flags) " %s (%d) num_irqs=%d num_regions=%d flags
> =%d"
> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
> index 57d502a1c79a..89780669118f 100644
> --- a/include/sysemu/iommufd.h
> +++ b/include/sysemu/iommufd.h
> @@ -18,6 +18,8 @@
>   #include "exec/hwaddr.h"
>   #include "exec/cpu-common.h"
>   #include "sysemu/host_iommu_device.h"
> +#include "hw/vfio/vfio-common.h"
> +#include "hw/vfio/vfio-container-base.h"
>
>   #define TYPE_IOMMUFD_BACKEND "iommufd"
>   OBJECT_DECLARE_TYPE(IOMMUFDBackend, IOMMUFDBackendClass, IOMMUFD_BACKEND)
> @@ -51,5 +53,9 @@ bool iommufd_backend_get_device_info(IOMMUFDBackend *be,
> uint32_t devid,
>                                        uint32_t *type, void *data, uint32_t len,
>                                        uint64_t *caps, Error **errp);
>
> +bool iommufd_cdev_detach_ioas_hwpt(VFIODevice *vbasedev, Error **errp);
> +int iommufd_cdev_attach_ioas_hwpt(VFIODevice *vbasedev, uint32_t id,
> +                                  Error **errp);
> +
>   #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
>   #endif