On 20/02/2024 12:51, Joao Martins wrote:
External email: Use caution opening links or attachments
On 19/02/2024 09:03, Avihai Horon wrote:
Hi Joao,
On 12/02/2024 15:56, Joao Martins wrote:
External email: Use caution opening links or attachments
Probe hardware dirty tracking support by querying device hw capabilities
via IOMMUFD_GET_HW_INFO.
In preparation to using the dirty tracking UAPI, request dirty tracking in
the HWPT flags when the device doesn't support dirty page tracking or has
it disabled; or when support when the VF backing IOMMU supports dirty
tracking. The latter is in the possibility of a device being attached
that doesn't have a dirty tracker.
Signed-off-by: Joao Martins <joao.m.mart...@oracle.com>
---
hw/vfio/common.c | 18 ++++++++++++++++++
hw/vfio/iommufd.c | 25 ++++++++++++++++++++++++-
include/hw/vfio/vfio-common.h | 2 ++
3 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index f7f85160be88..d8fc7077f839 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -216,6 +216,24 @@ bool vfio_devices_all_device_dirty_tracking(const
VFIOContainerBase *bcontainer)
return true;
}
+bool vfio_device_migration_supported(VFIODevice *vbasedev)
+{
+ if (!vbasedev->migration) {
+ return false;
+ }
+
+ return vbasedev->migration->mig_flags & VFIO_MIGRATION_STOP_COPY;
I think this is redundant, as (vbasedev->migration != NULL) implies
(vbasedev->migration->mig_flags & VFIO_MIGRATION_STOP_COPY) == true.
The check was there to prevent a null-deref in case the device didn't support
migration.
I meant that "vbasedev->migration->mig_flags & VFIO_MIGRATION_STOP_COPY"
check is redundant because if vbasedev->migration != NULL then
VFIO_MIGRATION_STOP_COPY is supported (it's already checked in
vfio_migration_init()).
But never mind, given what you wrote below.
+}
+
+bool vfio_device_dirty_pages_supported(VFIODevice *vbasedev)
+{
+ if (vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF) {
+ return false;
+ }
+
+ return !vbasedev->dirty_pages_supported;
+}
+
/*
* Check if all VFIO devices are running and migration is active, which is
* essentially equivalent to the migration being in pre-copy phase.
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index ca7ec45e725c..edacb6d72748 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -219,11 +219,26 @@ static int iommufd_cdev_detach_ioas_hwpt(VFIODevice
*vbasedev, Error **errp)
return ret;
}
+static bool iommufd_dirty_pages_supported(IOMMUFDDevice *iommufd_dev,
+ Error **errp)
+{
+ uint64_t caps;
+ int r;
+
+ r = iommufd_device_get_hw_capabilities(iommufd_dev, &caps, errp);
+ if (r) {
+ return false;
+ }
+
+ return caps & IOMMU_HW_CAP_DIRTY_TRACKING;
The false return value of this function is overloaded, it can indicate both
error and lack of DPT support.
Should we fail iommufd_cdev_autodomains_get() if iommufd_dirty_pages_supported()
fails?
Definitely not.
Otherwise, errp argument of iommufd_dirty_pages_supported() is redundant and we
can handle iommufd_device_get_hw_capabilities() error locally.
I'll handle locally.
+}
+
static int iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
VFIOIOMMUFDContainer *container,
Error **errp)
{
int iommufd = vbasedev->iommufd_dev.iommufd->fd;
+ uint32_t flags = 0;
VFIOIOASHwpt *hwpt;
Error *err = NULL;
int ret = -EINVAL;
@@ -245,9 +260,15 @@ static int iommufd_cdev_autodomains_get(VFIODevice
*vbasedev,
}
}
+ if ((vfio_device_migration_supported(vbasedev) &&
+ !vfio_device_dirty_pages_supported(vbasedev)) ||
+ iommufd_dirty_pages_supported(&vbasedev->iommufd_dev, &err)) {
I think it's too early to check vfio_device_migration_supported() and
vfio_device_dirty_pages_supported() here, as vfio_migration_init() hasn't been
called yet so vbasedev->migration and vbasedev->dirty_pages_supported are not
initialized.
I should replace with its own vfio device probing but the next point invalidates
this
Why do we need to check this? Can't we simply request IOMMUFD DPT if it's
supported?
There's no point in force requesting dpt in the domain if the device doesn't do
migration that was my thinking here; but otoh as past hotplug bug fixes have
shown it needs to proof against a new device getting add up that supports
migration while and the unsupported one be removed. So I guess we might not have
another option but to always ask for it if supported.
Thanks.
+ flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
+ }
+
ret = iommufd_backend_alloc_hwpt(iommufd,
vbasedev->iommufd_dev.devid,
- container->ioas_id, 0, 0, 0,
+ container->ioas_id, flags, 0, 0,
NULL, &hwpt_id);
if (ret) {
error_append_hint(&err,
@@ -271,6 +292,8 @@ static int iommufd_cdev_autodomains_get(VFIODevice
*vbasedev,
vbasedev->hwpt = hwpt;
QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next);
QLIST_INSERT_HEAD(&container->hwpt_list, hwpt, next);
+ container->bcontainer.dirty_pages_supported =
+ (flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING);
return 0;
}
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 7f7d823221e2..a3e691c126c6 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -271,6 +271,8 @@ bool
vfio_devices_all_running_and_mig_active(const VFIOContainerBase *bcontainer);
bool
vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer);
+bool vfio_device_migration_supported(VFIODevice *vbasedev);
+bool vfio_device_dirty_pages_supported(VFIODevice *vbasedev);
int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
VFIOBitmap *vbmap, hwaddr iova,
hwaddr size);
--
2.39.3
