Add Migration support. We rely on recently added gtree and qlist
migration. Besides, we have to fixup end point <-> domain link.
Indeed each domain has a list of endpoints attached to it. And each
endpoint has a pointer to its domain.
Raw gtree and qlist migration cannot handle this as it re-allocates
all the nodes while reconstructing the trees/lists.
So in post_load we re-construct the relationship between endpoints
and domains.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
hw/virtio/virtio-iommu.c | 127 ++++++++++++++++++++++++++++++++++++---
1 file changed, 117 insertions(+), 10 deletions(-)
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index c5b202fab7..4e92fc0c95 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -692,16 +692,6 @@ static void virtio_iommu_set_features(VirtIODevice *vdev, uint64_t val)
trace_virtio_iommu_set_features(dev->acked_features);
}
-/*
- * Migration is not yet supported: most of the state consists
- * of balanced binary trees which are not yet ready for getting
- * migrated
- */
-static const VMStateDescription vmstate_virtio_iommu_device = {
- .name = "virtio-iommu-device",
- .unmigratable = 1,
-};
-
static gint int_cmp(gconstpointer a, gconstpointer b, gpointer user_data)
{
uint ua = GPOINTER_TO_UINT(a);
@@ -778,6 +768,123 @@ static void virtio_iommu_instance_init(Object *obj)
{
}
+#define VMSTATE_INTERVAL \
+{ \
+ .name = "interval", \
+ .version_id = 1, \
+ .minimum_version_id = 1, \
+ .fields = (VMStateField[]) { \
+ VMSTATE_UINT64(low, viommu_interval), \
+ VMSTATE_UINT64(high, viommu_interval), \
+ VMSTATE_END_OF_LIST() \
+ } \
+}
+
+#define VMSTATE_MAPPING \
+{ \
+ .name = "mapping", \
+ .version_id = 1, \
+ .minimum_version_id = 1, \
+ .fields = (VMStateField[]) { \
+ VMSTATE_UINT64(phys_addr, viommu_mapping), \
+ VMSTATE_UINT32(flags, viommu_mapping), \
+ VMSTATE_END_OF_LIST() \
+ }, \
+}
+
+static const VMStateDescription vmstate_interval_mapping[2] = {
+ VMSTATE_MAPPING, /* value */
+ VMSTATE_INTERVAL /* key */
+};
+
+static int domain_preload(void *opaque)
+{
+ viommu_domain *domain = opaque;
+
+ domain->mappings = g_tree_new_full((GCompareDataFunc)interval_cmp,
+ NULL, g_free, g_free);
+ return 0;
+}
+
+static const VMStateDescription vmstate_endpoint = {
+ .name = "endpoint",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(id, viommu_endpoint),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static const VMStateDescription vmstate_domain = {
+ .name = "domain",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .pre_load = domain_preload,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(id, viommu_domain),
+ VMSTATE_GTREE_V(mappings, viommu_domain, 1,
+ vmstate_interval_mapping,
+ viommu_interval, viommu_mapping),
+ VMSTATE_QLIST_V(endpoint_list, viommu_domain, 1,
+ vmstate_endpoint, viommu_endpoint, next),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static gboolean reconstruct_ep_domain_link(gpointer key, gpointer value,
+ gpointer data)
+{
+ viommu_domain *d = (viommu_domain *)value;
+ viommu_endpoint *iter, *tmp;
+ viommu_endpoint *ep = (viommu_endpoint *)data;
+
+ QLIST_FOREACH_SAFE(iter, &d->endpoint_list, next, tmp) {
+ if (iter->id == ep->id) {
+ /* remove the ep */
+ QLIST_REMOVE(iter, next);
+ g_free(iter);
+ /* replace it with the good one */
+ QLIST_INSERT_HEAD(&d->endpoint_list, ep, next);
+ /* update the domain */
+ ep->domain = d;
+ return true; /* stop the search */
+ }
+ }
+ return false; /* continue the traversal */
+}
+
+static gboolean fix_endpoint(gpointer key, gpointer value, gpointer data)
+{
+ VirtIOIOMMU *s = (VirtIOIOMMU *)data;
+
+ g_tree_foreach(s->domains, reconstruct_ep_domain_link, value);
+ return false;
+}
+
+static int iommu_post_load(void *opaque, int version_id)
+{
+ VirtIOIOMMU *s = opaque;
+
+ g_tree_foreach(s->endpoints, fix_endpoint, s);
+ return 0;
+}
+
+static const VMStateDescription vmstate_virtio_iommu_device = {
+ .name = "virtio-iommu-device",
+ .minimum_version_id = 1,
+ .version_id = 1,
+ .post_load = iommu_post_load,
+ .fields = (VMStateField[]) {
+ VMSTATE_GTREE_DIRECT_KEY_V(domains, VirtIOIOMMU, 1,
+ &vmstate_domain, viommu_domain),
+ VMSTATE_GTREE_DIRECT_KEY_V(endpoints, VirtIOIOMMU, 1,
+ &vmstate_endpoint, viommu_endpoint),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
+
static const VMStateDescription vmstate_virtio_iommu = {
.name = "virtio-iommu",
.minimum_version_id = 1,
--
2.20.1
* Eric Auger (eric.auger@redhat.com) wrote:
> Add Migration support. We rely on recently added gtree and qlist
> migration. Besides, we have to fixup end point <-> domain link.
>
> Indeed each domain has a list of endpoints attached to it. And each
> endpoint has a pointer to its domain.
>
> Raw gtree and qlist migration cannot handle this as it re-allocates
> all the nodes while reconstructing the trees/lists.
>
> So in post_load we re-construct the relationship between endpoints
> and domains.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
From the migration side of things,
Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> hw/virtio/virtio-iommu.c | 127 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 117 insertions(+), 10 deletions(-)
>
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index c5b202fab7..4e92fc0c95 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -692,16 +692,6 @@ static void virtio_iommu_set_features(VirtIODevice *vdev, uint64_t val)
> trace_virtio_iommu_set_features(dev->acked_features);
> }
>
> -/*
> - * Migration is not yet supported: most of the state consists
> - * of balanced binary trees which are not yet ready for getting
> - * migrated
> - */
> -static const VMStateDescription vmstate_virtio_iommu_device = {
> - .name = "virtio-iommu-device",
> - .unmigratable = 1,
> -};
> -
> static gint int_cmp(gconstpointer a, gconstpointer b, gpointer user_data)
> {
> uint ua = GPOINTER_TO_UINT(a);
> @@ -778,6 +768,123 @@ static void virtio_iommu_instance_init(Object *obj)
> {
> }
>
> +#define VMSTATE_INTERVAL \
> +{ \
> + .name = "interval", \
> + .version_id = 1, \
> + .minimum_version_id = 1, \
> + .fields = (VMStateField[]) { \
> + VMSTATE_UINT64(low, viommu_interval), \
> + VMSTATE_UINT64(high, viommu_interval), \
> + VMSTATE_END_OF_LIST() \
> + } \
> +}
> +
> +#define VMSTATE_MAPPING \
> +{ \
> + .name = "mapping", \
> + .version_id = 1, \
> + .minimum_version_id = 1, \
> + .fields = (VMStateField[]) { \
> + VMSTATE_UINT64(phys_addr, viommu_mapping), \
> + VMSTATE_UINT32(flags, viommu_mapping), \
> + VMSTATE_END_OF_LIST() \
> + }, \
> +}
> +
> +static const VMStateDescription vmstate_interval_mapping[2] = {
> + VMSTATE_MAPPING, /* value */
> + VMSTATE_INTERVAL /* key */
> +};
> +
> +static int domain_preload(void *opaque)
> +{
> + viommu_domain *domain = opaque;
> +
> + domain->mappings = g_tree_new_full((GCompareDataFunc)interval_cmp,
> + NULL, g_free, g_free);
> + return 0;
> +}
> +
> +static const VMStateDescription vmstate_endpoint = {
> + .name = "endpoint",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32(id, viommu_endpoint),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static const VMStateDescription vmstate_domain = {
> + .name = "domain",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .pre_load = domain_preload,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32(id, viommu_domain),
> + VMSTATE_GTREE_V(mappings, viommu_domain, 1,
> + vmstate_interval_mapping,
> + viommu_interval, viommu_mapping),
> + VMSTATE_QLIST_V(endpoint_list, viommu_domain, 1,
> + vmstate_endpoint, viommu_endpoint, next),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static gboolean reconstruct_ep_domain_link(gpointer key, gpointer value,
> + gpointer data)
> +{
> + viommu_domain *d = (viommu_domain *)value;
> + viommu_endpoint *iter, *tmp;
> + viommu_endpoint *ep = (viommu_endpoint *)data;
> +
> + QLIST_FOREACH_SAFE(iter, &d->endpoint_list, next, tmp) {
> + if (iter->id == ep->id) {
> + /* remove the ep */
> + QLIST_REMOVE(iter, next);
> + g_free(iter);
> + /* replace it with the good one */
> + QLIST_INSERT_HEAD(&d->endpoint_list, ep, next);
> + /* update the domain */
> + ep->domain = d;
> + return true; /* stop the search */
> + }
> + }
> + return false; /* continue the traversal */
> +}
> +
> +static gboolean fix_endpoint(gpointer key, gpointer value, gpointer data)
> +{
> + VirtIOIOMMU *s = (VirtIOIOMMU *)data;
> +
> + g_tree_foreach(s->domains, reconstruct_ep_domain_link, value);
> + return false;
> +}
> +
> +static int iommu_post_load(void *opaque, int version_id)
> +{
> + VirtIOIOMMU *s = opaque;
> +
> + g_tree_foreach(s->endpoints, fix_endpoint, s);
> + return 0;
> +}
> +
> +static const VMStateDescription vmstate_virtio_iommu_device = {
> + .name = "virtio-iommu-device",
> + .minimum_version_id = 1,
> + .version_id = 1,
> + .post_load = iommu_post_load,
> + .fields = (VMStateField[]) {
> + VMSTATE_GTREE_DIRECT_KEY_V(domains, VirtIOIOMMU, 1,
> + &vmstate_domain, viommu_domain),
> + VMSTATE_GTREE_DIRECT_KEY_V(endpoints, VirtIOIOMMU, 1,
> + &vmstate_endpoint, viommu_endpoint),
> + VMSTATE_END_OF_LIST()
> + },
> +};
> +
> +
> static const VMStateDescription vmstate_virtio_iommu = {
> .name = "virtio-iommu",
> .minimum_version_id = 1,
> --
> 2.20.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, Nov 22, 2019 at 07:29:41PM +0100, Eric Auger wrote:
> +static const VMStateDescription vmstate_virtio_iommu_device = {
> + .name = "virtio-iommu-device",
> + .minimum_version_id = 1,
> + .version_id = 1,
> + .post_load = iommu_post_load,
> + .fields = (VMStateField[]) {
> + VMSTATE_GTREE_DIRECT_KEY_V(domains, VirtIOIOMMU, 1,
> + &vmstate_domain, viommu_domain),
> + VMSTATE_GTREE_DIRECT_KEY_V(endpoints, VirtIOIOMMU, 1,
> + &vmstate_endpoint, viommu_endpoint),
So if I understand correctly these fields are state that is modified by
the guest? We don't need to save/load fields that cannot be modified by
the guest, static information that is created from the QEMU command-line.
I think the above covers everything we need to migrate in VirtIOIOMMU
then, except for acked_features, which (as I pointed out on another patch)
seems redundant anyway since there is vdev->guest_features.
Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> + VMSTATE_END_OF_LIST()
> + },
> +};
> +
> +
> static const VMStateDescription vmstate_virtio_iommu = {
> .name = "virtio-iommu",
> .minimum_version_id = 1,
> --
> 2.20.1
>
>
Hi Jean,
On 12/10/19 5:50 PM, Jean-Philippe Brucker wrote:
> On Fri, Nov 22, 2019 at 07:29:41PM +0100, Eric Auger wrote:
>> +static const VMStateDescription vmstate_virtio_iommu_device = {
>> + .name = "virtio-iommu-device",
>> + .minimum_version_id = 1,
>> + .version_id = 1,
>> + .post_load = iommu_post_load,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_GTREE_DIRECT_KEY_V(domains, VirtIOIOMMU, 1,
>> + &vmstate_domain, viommu_domain),
>> + VMSTATE_GTREE_DIRECT_KEY_V(endpoints, VirtIOIOMMU, 1,
>> + &vmstate_endpoint, viommu_endpoint),
>
> So if I understand correctly these fields are state that is modified by
> the guest? We don't need to save/load fields that cannot be modified by
> the guest, static information that is created from the QEMU command-line.
Yes that's correct.
>
> I think the above covers everything we need to migrate in VirtIOIOMMU
> then, except for acked_features, which (as I pointed out on another patch)
> seems redundant anyway since there is vdev->guest_features.
you're right, acked features were not properly migrated.
>
> Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Thanks!
Eric
>
>> + VMSTATE_END_OF_LIST()
>> + },
>> +};
>> +
>> +
>> static const VMStateDescription vmstate_virtio_iommu = {
>> .name = "virtio-iommu",
>> .minimum_version_id = 1,
>> --
>> 2.20.1
>>
>>
>
On Fri, Nov 22, 2019 at 07:29:41PM +0100, Eric Auger wrote:
> +static const VMStateDescription vmstate_virtio_iommu_device = {
> + .name = "virtio-iommu-device",
> + .minimum_version_id = 1,
> + .version_id = 1,
> + .post_load = iommu_post_load,
> + .fields = (VMStateField[]) {
> + VMSTATE_GTREE_DIRECT_KEY_V(domains, VirtIOIOMMU, 1,
> + &vmstate_domain, viommu_domain),
> + VMSTATE_GTREE_DIRECT_KEY_V(endpoints, VirtIOIOMMU, 1,
> + &vmstate_endpoint, viommu_endpoint),
IIUC vmstate_domain already contains all the endpoint information (in
endpoint_list of vmstate_domain), but here we migrate it twice. I
suppose that's why now we need reconstruct_ep_domain_link() to fixup
the duplicated migration?
Then I'll instead ask whether we can skip migrating here? Then in
post_load we simply:
foreach(domain)
foreach(endpoint in domain)
g_tree_insert(s->endpoints);
It might help to avoid the reconstruct_ep_domain_link ugliness?
And besides, I also agree with Jean that the endpoint data structure
could be reused with IOMMUDevice somehow.
Thanks,
--
Peter Xu
Hi Peter,
On 12/10/19 9:01 PM, Peter Xu wrote:
> On Fri, Nov 22, 2019 at 07:29:41PM +0100, Eric Auger wrote:
>> +static const VMStateDescription vmstate_virtio_iommu_device = {
>> + .name = "virtio-iommu-device",
>> + .minimum_version_id = 1,
>> + .version_id = 1,
>> + .post_load = iommu_post_load,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_GTREE_DIRECT_KEY_V(domains, VirtIOIOMMU, 1,
>> + &vmstate_domain, viommu_domain),
>> + VMSTATE_GTREE_DIRECT_KEY_V(endpoints, VirtIOIOMMU, 1,
>> + &vmstate_endpoint, viommu_endpoint),
>
> IIUC vmstate_domain already contains all the endpoint information (in
> endpoint_list of vmstate_domain), but here we migrate it twice.
I migrated both because at that time I considered we could have
endpoints not attached to any domains but I think I can now simplify
based on the fact any EP is attached.
I
> suppose that's why now we need reconstruct_ep_domain_link() to fixup
> the duplicated migration?
Even if I only migrate the domain gtree, I need to reconstruct the
ep->domain which was not migrated, on purpose, as it pointed to the old
domain in the origin.
>
> Then I'll instead ask whether we can skip migrating here? Then in
> post_load we simply:
>
> foreach(domain)
> foreach(endpoint in domain)
> g_tree_insert(s->endpoints);
>
> It might help to avoid the reconstruct_ep_domain_link ugliness?
I agree that it is simpler. Also need to update the ep->domain as
mentionned above. Thank you for the suggestion.
>
> And besides, I also agree with Jean that the endpoint data structure
> could be reused with IOMMUDevice somehow.
As I replied to Jean, I think it makes sense to keep both structures as
endpoints are not indexed by the same key and the bus number is resolved
later.
Thanks
Eric
>
> Thanks,
>
© 2016 - 2026 Red Hat, Inc.