On 09/07/2024 10:13, Joao Martins wrote:
> On 09/07/2024 08:05, Cédric Le Goater wrote:
>> On 7/8/24 4:34 PM, Joao Martins wrote:
>>> ioctl(iommufd, IOMMU_HWPT_GET_DIRTY_BITMAP, arg) is the UAPI
>>> that fetches the bitmap that tells what was dirty in an IOVA
>>> range.
>>>
>>> A single bitmap is allocated and used across all the hwpts
>>> sharing an IOAS which is then used in log_sync() to set Qemu
>>> global bitmaps.
>>>
>>> Signed-off-by: Joao Martins <joao.m.mart...@oracle.com>
>>> ---
>>> include/sysemu/iommufd.h | 3 +++
>>> backends/iommufd.c | 26 ++++++++++++++++++++++++++
>>> hw/vfio/iommufd.c | 34 ++++++++++++++++++++++++++++++++++
>>> backends/trace-events | 1 +
>>> 4 files changed, 64 insertions(+)
>>>
>>> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
>>> index 1470377f55ba..223f1ea14e84 100644
>>> --- a/include/sysemu/iommufd.h
>>> +++ b/include/sysemu/iommufd.h
>>> @@ -56,6 +56,9 @@ int iommufd_backend_alloc_hwpt(IOMMUFDBackend *be,
>>> uint32_t
>>> dev_id,
>>> void *data_ptr, uint32_t *out_hwpt);
>>> int iommufd_backend_set_dirty_tracking(IOMMUFDBackend *be, uint32_t
>>> hwpt_id,
>>> bool start);
>>> +int iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be, uint32_t hwpt_id,
>>> + uint64_t iova, ram_addr_t size,
>>> + uint64_t page_size, uint64_t *data);
>>> #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
>>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>>> index 69daabc27473..b2d3bbd7c31b 100644
>>> --- a/backends/iommufd.c
>>> +++ b/backends/iommufd.c
>>> @@ -257,6 +257,32 @@ int iommufd_backend_set_dirty_tracking(IOMMUFDBackend
>>> *be, uint32_t hwpt_id,
>>> return ret;
>>> }
>>> +int iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be, uint32_t
>>> hwpt_id,
>>> + uint64_t iova, ram_addr_t size,
>>> + uint64_t page_size, uint64_t *data)
>>> +{
>>> + int ret;
>>> + struct iommu_hwpt_get_dirty_bitmap get_dirty_bitmap = {
>>> + .size = sizeof(get_dirty_bitmap),
>>> + .hwpt_id = hwpt_id,
>>> + .iova = iova,
>>> + .length = size,
>>> + .page_size = page_size,
>>> + .data = (uintptr_t)data,
>>> + };
>>> +
>>> + ret = ioctl(be->fd, IOMMU_HWPT_GET_DIRTY_BITMAP, &get_dirty_bitmap);
>>> + trace_iommufd_backend_get_dirty_bitmap(be->fd, hwpt_id, iova, size,
>>> + page_size, ret);
>>> + if (ret) {
>>> + error_report("IOMMU_HWPT_GET_DIRTY_BITMAP (iova: 0x%"PRIx64
>>> + " size: 0x%"PRIx64") failed: %s", iova,
>>> + size, strerror(errno));
>>> + }
>>> +
>>> + return !ret ? 0 : -errno;
>>> +}
>>> +
>>> bool iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid,
>>> uint32_t *type, void *data, uint32_t
>>> len,
>>> uint64_t *caps, Error **errp)
>>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>>> index 158a98cb3b12..9fad47baed9e 100644
>>> --- a/hw/vfio/iommufd.c
>>> +++ b/hw/vfio/iommufd.c
>>> @@ -25,6 +25,7 @@
>>> #include "qemu/cutils.h"
>>> #include "qemu/chardev_open.h"
>>> #include "pci.h"
>>> +#include "exec/ram_addr.h"
>>> static int iommufd_cdev_map(const VFIOContainerBase *bcontainer, hwaddr
>>> iova,
>>> ram_addr_t size, void *vaddr, bool readonly)
>>> @@ -152,6 +153,38 @@ err:
>>> return ret;
>>> }
>>> +static int iommufd_query_dirty_bitmap(const VFIOContainerBase
>>> *bcontainer,
>>> + VFIOBitmap *vbmap, hwaddr iova,
>>> + hwaddr size, Error **errp)
>>> +{
>>> + VFIOIOMMUFDContainer *container = container_of(bcontainer,
>>> + VFIOIOMMUFDContainer,
>>> + bcontainer);
>>> + int ret;
>>> + VFIOIOASHwpt *hwpt;
>>> + unsigned long page_size;
>>> +
>>> + page_size = qemu_real_host_page_size();
>>> + QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
>>> + if (!iommufd_hwpt_dirty_tracking(hwpt)) {
>>> + continue;
>>> + }
>>> +
>>> + ret = iommufd_backend_get_dirty_bitmap(container->be,
>>> hwpt->hwpt_id,
>>> + iova, size, page_size,
>>> + vbmap->bitmap);
>>> + if (ret) {
>>> + error_setg_errno(errp, ret,
>>> + "Failed to get dirty bitmap report, hwpt_id
>>> %u,
>>> iova: "
>>> + "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx,
>>> + hwpt->hwpt_id, iova, size);
>>
>> This error looks redundant with the one printed out in
>> iommufd_backend_get_dirty_bitmap(). Couldn't we add an 'Error **'
>> parameter and simply return a bool ?
>>
>
> I'll add it.
>
> Just as a sidebar: This is a odd pattern which seems somewhat spread, that
> somehow we only care about @errno as something to put on a message, rather
> then
> letting the user know what exact error code it had returned programmatically.
> Here in this series it is only important for the device attach so likely
> doesn't
> justify a Error structure enhancement, hence the rest of functions I
> introduced
> here can just adopt bool+errp.
>
>
Looks like this. A bit odd as the container ops (and legacy backend) expect an
errno. But I am assuming that you intended that way.
-int iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be, uint32_t hwpt_id,
- uint64_t iova, ram_addr_t size,
- uint64_t page_size, uint64_t *data)
+bool iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be,
+ uint32_t hwpt_id,
+ uint64_t iova, ram_addr_t size,
+ uint64_t page_size, uint64_t *data,
+ Error **errp)
{
int ret;
struct iommu_hwpt_get_dirty_bitmap get_dirty_bitmap = {
@@ -273,14 +278,15 @@ int iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be,
uint32_t hwpt_id,
ret = ioctl(be->fd, IOMMU_HWPT_GET_DIRTY_BITMAP, &get_dirty_bitmap);
trace_iommufd_backend_get_dirty_bitmap(be->fd, hwpt_id, iova, size,
- page_size, ret);
+ page_size, ret ? errno : 0);
if (ret) {
- error_report("IOMMU_HWPT_GET_DIRTY_BITMAP (iova: 0x%"PRIx64
- " size: 0x%"PRIx64") failed: %s", iova,
- size, strerror(errno));
+ error_setg_errno(errp, errno,
+ "IOMMU_HWPT_GET_DIRTY_BITMAP (iova: 0x%"HWADDR_PRIx
+ " size: 0x%"HWADDR_PRIx") failed", iova, size);
+ return false;
}
- return !ret ? 0 : -errno;
+ return true;
}
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 35cf8da22ef7..4369df34a6ac 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -160,25 +154,18 @@ static int iommufd_query_dirty_bitmap(const
VFIOContainerBase *bcontainer,
VFIOIOMMUFDContainer *container = container_of(bcontainer,
VFIOIOMMUFDContainer,
bcontainer);
- int ret;
+ unsigned long page_size = qemu_real_host_page_size();
VFIOIOASHwpt *hwpt;
- unsigned long page_size;
- page_size = qemu_real_host_page_size();
QLIST_FOREACH(hwpt, &container->hwpt_list, next) {
if (!iommufd_hwpt_dirty_tracking(hwpt)) {
continue;
}
- ret = iommufd_backend_get_dirty_bitmap(container->be, hwpt->hwpt_id,
- iova, size, page_size,
- vbmap->bitmap);
- if (ret) {
- error_setg_errno(errp, ret,
- "Failed to get dirty bitmap report, hwpt_id %u,
iova: "
- "0x%" HWADDR_PRIx ", size: 0x%" HWADDR_PRIx,
- hwpt->hwpt_id, iova, size);
- return ret;
+ if (!iommufd_backend_get_dirty_bitmap(container->be, hwpt->hwpt_id,
+ iova, size, page_size,
+ vbmap->bitmap)) {
+ return -EINVAL;
}
}
@@ -287,24 +274,11 @@ static bool iommufd_cdev_detach_ioas_hwpt(VFIODevice
*vbasedev, Error **errp)
return true;
}
(...)
