>-----Original Message-----
>From: Eric Auger <eric.au...@redhat.com>
>Subject: Re: [PATCH rfcv1 1/6] backends/iommufd_device: introduce
>IOMMUFDDevice
>
>Hi Zhenzhong,
>
>On 1/15/24 11:13, Zhenzhong Duan wrote:
>> IOMMUFDDevice represents a device in iommufd and can be used as
>> a communication interface between devices (i.e., VFIO, VDPA) and
>> vIOMMU.
>>
>> Currently it includes iommufd handler and device id information
>iommufd handle
>> which could be used by vIOMMU to get hw IOMMU information.
>>
>> In future nested translation support, vIOMMU is going to have
>> more iommufd related operations like allocate hwpt for a device,
>> attach/detach hwpt, etc. So IOMMUFDDevice will be further expanded.
>>
>> IOMMUFDDevice is willingly not a QOM object because we don't want
>> it to be visible from the user interface.
>>
>> Introduce a helper iommufd_device_init to initialize IOMMUFDDevice.
>
>+ iommufd_device_get_info helper
Will do.
>>
>> Originally-by: Yi Liu <yi.l....@intel.com>
>> Signed-off-by: Yi Sun <yi.y....@linux.intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
>> ---
>> MAINTAINERS | 4 +--
>> include/sysemu/iommufd_device.h | 31 ++++++++++++++++++++
>> backends/iommufd_device.c | 50
>+++++++++++++++++++++++++++++++++
>> backends/meson.build | 2 +-
>> 4 files changed, 84 insertions(+), 3 deletions(-)
>> create mode 100644 include/sysemu/iommufd_device.h
>> create mode 100644 backends/iommufd_device.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 00ec1f7eca..606dfeb2b1 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2171,8 +2171,8 @@ M: Yi Liu <yi.l....@intel.com>
>> M: Eric Auger <eric.au...@redhat.com>
>> M: Zhenzhong Duan <zhenzhong.d...@intel.com>
>> S: Supported
>> -F: backends/iommufd.c
>> -F: include/sysemu/iommufd.h
>> +F: backends/iommufd*.c
>> +F: include/sysemu/iommufd*.h
>> F: include/qemu/chardev_open.h
>> F: util/chardev_open.c
>> F: docs/devel/vfio-iommufd.rst
>> diff --git a/include/sysemu/iommufd_device.h
>b/include/sysemu/iommufd_device.h
>> new file mode 100644
>> index 0000000000..795630324b
>> --- /dev/null
>> +++ b/include/sysemu/iommufd_device.h
>> @@ -0,0 +1,31 @@
>> +/*
>> + * IOMMUFD Device
>> + *
>> + * Copyright (C) 2024 Intel Corporation.
>> + *
>> + * Authors: Yi Liu <yi.l....@intel.com>
>> + * Zhenzhong Duan <zhenzhong.d...@intel.com>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#ifndef SYSEMU_IOMMUFD_DEVICE_H
>> +#define SYSEMU_IOMMUFD_DEVICE_H
>> +
>> +#include <linux/iommufd.h>
>> +#include "sysemu/iommufd.h"
>> +
>> +typedef struct IOMMUFDDevice IOMMUFDDevice;
>> +
>> +/* This is an abstraction of host IOMMUFD device */
>> +struct IOMMUFDDevice {
>> + IOMMUFDBackend *iommufd;
>> + uint32_t dev_id;
>> +};
>> +
>> +int iommufd_device_get_info(IOMMUFDDevice *idev,
>> + enum iommu_hw_info_type *type,
>> + uint32_t len, void *data);
>> +void iommufd_device_init(void *_idev, size_t instance_size,
>> + IOMMUFDBackend *iommufd, uint32_t dev_id);
>> +#endif
>> diff --git a/backends/iommufd_device.c b/backends/iommufd_device.c
>> new file mode 100644
>> index 0000000000..f6e7ca1dbf
>> --- /dev/null
>> +++ b/backends/iommufd_device.c
>> @@ -0,0 +1,50 @@
>> +/*
>> + * QEMU abstract of Host IOMMU
>it is the abstraction of the IOMMU or of any assigned device?
' QEMU abstract of Host IOMMUFD device' may be better.
>> + *
>> + * Copyright (C) 2024 Intel Corporation.
>> + *
>> + * Authors: Yi Liu <yi.l....@intel.com>
>> + * Zhenzhong Duan <zhenzhong.d...@intel.com>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#include <sys/ioctl.h>
>> +#include "qemu/osdep.h"
>> +#include "qemu/error-report.h"
>> +#include "sysemu/iommufd_device.h"
>> +
>> +int (IOMMUFDDevice *idev,
>> + enum iommu_hw_info_type *type,
>> + uint32_t len, void *data)
>> +{
>> + struct iommu_hw_info info = {
>> + .size = sizeof(info),
>> + .flags = 0,
>> + .dev_id = idev->dev_id,
>> + .data_len = len,
>> + .__reserved = 0,
>> + .data_uptr = (uintptr_t)data,
>> + };
>> + int ret;
>> +
>> + ret = ioctl(idev->iommufd->fd, IOMMU_GET_HW_INFO, &info);
>> + if (ret) {
>> + error_report("Failed to get info %m");
>you may prefer using errp instead of hard traces.
Good suggestion, will do.
>> + } else {
>> + *type = info.out_data_type;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +void iommufd_device_init(void *_idev, size_t instance_size,
>nit: why the "_"
To distinguish with local idev.
>> + IOMMUFDBackend *iommufd, uint32_t dev_id)
>> +{
>> + IOMMUFDDevice *idev = (IOMMUFDDevice *)_idev;
>> +
>> + g_assert(sizeof(IOMMUFDDevice) <= instance_size);
>at this stage of the reading it is not clear why you input the
>instance_size. worth to be clarified/documented.
VFIO or VDPA may have IOMMUFD related attributes for its own usages.
It looks VFIO doesn't need this for now. I'll remove it, then _idev can be
removed too.
Thanks
Zhenzhong
