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