[RFC,v3,2/3] hw/intc/arm_gicv3_its: Implement state save/restore
diff mbox

Message ID 1490608126-22187-3-git-send-email-eric.auger@redhat.com
State New
Headers show

Commit Message

Auger Eric March 27, 2017, 9:48 a.m. UTC
We need to handle both registers and ITS tables. While
register handling is standard, ITS table handling is more
challenging since the kernel API is devised so that the
tables are flushed into guest RAM and not in vmstate buffers.

Flushing the ITS tables on device pre_save() is too late
since the guest RAM is already saved at this point.

Table flushing needs to happen when we are sure the vcpus
are stopped and before the last dirty page saving. The
right point is RUN_STATE_FINISH_MIGRATE but sometimes the
VM gets stopped before migration launch so let's simply
flush the tables each time the VM gets stopped.

For regular ITS registers we just can use vmstate pre_save()
and post_load() callbacks.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v2 -> v3:
- take into account Peter's comments:
  - add iidr save/restore
  - reword comments
  - remove s->ctlr = extract64(reg, 0, 32);
  - rename get()/put() function into pre_save()/post_load()
- do not execute put() if iidr == 0
---
 hw/intc/arm_gicv3_its_common.c         |  9 ++++
 hw/intc/arm_gicv3_its_kvm.c            | 96 ++++++++++++++++++++++++++++++++++
 include/hw/intc/arm_gicv3_its_common.h |  8 +++
 3 files changed, 113 insertions(+)

Comments

Juan Quintela March 28, 2017, 7:45 p.m. UTC | #1
Eric Auger <eric.auger@redhat.com> wrote:
> We need to handle both registers and ITS tables. While
> register handling is standard, ITS table handling is more
> challenging since the kernel API is devised so that the
> tables are flushed into guest RAM and not in vmstate buffers.
>
> Flushing the ITS tables on device pre_save() is too late
> since the guest RAM is already saved at this point.

We need to put a way to register handlers for this.

> Table flushing needs to happen when we are sure the vcpus
> are stopped and before the last dirty page saving. The
> right point is RUN_STATE_FINISH_MIGRATE but sometimes the
> VM gets stopped before migration launch so let's simply
> flush the tables each time the VM gets stopped.

Just curious, how slow is doing that in all stops?


No comments in the rest of the patch


>  static void kvm_arm_its_init(Object *obj)
> @@ -102,6 +122,80 @@ static void kvm_arm_its_init(Object *obj)
>                               &error_abort);
>  }
>  
> +/**
> + * kvm_arm_its_pre_save - handles the saving of ITS registers.
> + * ITS tables are flushed into guest RAM separately and earlier,
> + * through the VM change state handler, since at the moment pre_save()
> + * is called, the guest RAM has already been saved.
> + */
> +static void kvm_arm_its_pre_save(GICv3ITSState *s)
> +{

...

> +}
> +
> +/**
> + * kvm_arm_its_post_load - Restore both the ITS registers and tables
> + */
> +static void kvm_arm_its_post_load(GICv3ITSState *s)
> +{

...

> +}
> +

I assume that two functions are right.  I have no clue about ARM.

> @@ -109,6 +203,8 @@ static void kvm_arm_its_class_init(ObjectClass *klass, void *data)
>  
>      dc->realize = kvm_arm_its_realize;
>      icc->send_msi = kvm_its_send_msi;
> +    icc->pre_save = kvm_arm_its_pre_save;
> +    icc->post_load = kvm_arm_its_post_load;
>  }

Let me see if I understood this correctly.

We have an ARM_GICV3_ITS_COMMON.  And that has some fields.
In particular:

struct GICv3ITSState {
    /* Registers */
    uint32_t ctlr;
    uint64_t cbaser;
    uint64_t cwriter;
    uint64_t creadr;
    uint64_t baser[8];
    /* lots of things removed */
};



We have this in arm_gicv3_its_common.c  (it is exactly the same for
post_load, so we forgot about it by now).


static void gicv3_its_pre_save(void *opaque)
{
    GICv3ITSState *s = (GICv3ITSState *)opaque; (*)
                                                   /* nitpit: the cast
                                                   is useless */
    GICv3ITSCommonClass *c = ARM_GICV3_ITS_COMMON_GET_CLASS(s);

    if (c->pre_save) {
        c->pre_save(s);
    }
}

And then we have in the patch:


> @@ -109,6 +203,8 @@ static void kvm_arm_its_class_init(ObjectClass *klass, void *data)
>  
>      dc->realize = kvm_arm_its_realize;
>      icc->send_msi = kvm_its_send_msi;
> +    icc->pre_save = kvm_arm_its_pre_save;
> +    icc->post_load = kvm_arm_its_post_load;
>  }


struct GICv3ITSCommonClass {
....
    void (*pre_save)(GICv3ITSState *s);
    void (*post_load)(GICv3ITSState *s);
};


Notice that I have only found one user of this on the tree, so I don't
know if there is a good reason for this.


static void gicv3_its_common_class_init(ObjectClass *klass, void *data)
{
    DeviceClass *dc = DEVICE_CLASS(klass);

    dc->reset = gicv3_its_common_reset;
    dc->vmsd = &vmstate_its;
}

So, what if we change:

const VMSField vmstate_its_fields[] = {
     VMSTATE_UINT32(ctlr, GICv3ITSState),
     VMSTATE_UINT32(iidr, GICv3ITSState),
     VMSTATE_UINT64(cbaser, GICv3ITSState),
     VMSTATE_UINT64(cwriter, GICv3ITSState),
     VMSTATE_UINT64(creadr, GICv3ITSState),
     VMSTATE_UINT64_ARRAY(baser, GICv3ITSState, 8),
     VMSTATE_END_OF_LIST()
};



Remove the dc->vmsd = &vmstate_its; from gicv3_its_common_class_init();

And we add in arm_gicv3_its_kvm.c


static const VMStateDescription vmstate_its_kvm = {
    .name = "arm_gicv3_its",
    .pre_save = kvm_arm_its_pre_save,
    .post_load = kvm_arm_its_post_load,
    .fields = &vmsate_its_fields;
    },
};

And add the:

dc->vmstate = &vmastet_its_kvm;

into kvm_arm_its_class_init()?

And be with it?  Or it is too late by then?

I am assuming that there is some reason why we want to call
arm_gicv3_its either for kvm or for anything else.  But IMHO, you are
making things more complicated that they need to be.

My understanding:
- We have GICv3 ITS state
- We want to have several implementations
- We want to be able to migration from one to another


Or have I missed something?

Notice that I like more this other approach, but as far as I can see,
yours should also work.

Thanks, Juan.
Peter Maydell March 28, 2017, 9:08 p.m. UTC | #2
On 28 March 2017 at 20:45, Juan Quintela <quintela@redhat.com> wrote:
> Let me see if I understood this correctly.
>
> We have an ARM_GICV3_ITS_COMMON.  And that has some fields.
> In particular:
>
> struct GICv3ITSState {
>     /* Registers */
>     uint32_t ctlr;
>     uint64_t cbaser;
>     uint64_t cwriter;
>     uint64_t creadr;
>     uint64_t baser[8];
>     /* lots of things removed */
> };
>
>
>
> We have this in arm_gicv3_its_common.c  (it is exactly the same for
> post_load, so we forgot about it by now).
>
>
> static void gicv3_its_pre_save(void *opaque)
> {
>     GICv3ITSState *s = (GICv3ITSState *)opaque; (*)
>                                                    /* nitpit: the cast
>                                                    is useless */
>     GICv3ITSCommonClass *c = ARM_GICV3_ITS_COMMON_GET_CLASS(s);
>
>     if (c->pre_save) {
>         c->pre_save(s);
>     }
> }
>
> And then we have in the patch:
>
>
>> @@ -109,6 +203,8 @@ static void kvm_arm_its_class_init(ObjectClass *klass, void *data)
>>
>>      dc->realize = kvm_arm_its_realize;
>>      icc->send_msi = kvm_its_send_msi;
>> +    icc->pre_save = kvm_arm_its_pre_save;
>> +    icc->post_load = kvm_arm_its_post_load;
>>  }
>
>
> struct GICv3ITSCommonClass {
> ....
>     void (*pre_save)(GICv3ITSState *s);
>     void (*post_load)(GICv3ITSState *s);
> };
>
>
> Notice that I have only found one user of this on the tree, so I don't
> know if there is a good reason for this.

This is just following the existing pattern we have for
the GICv3 itself (and the GICv2, for that matter).
At some point we'll implement the emulated ITS which
will share the base class (and the vmstate).

thanks
-- PMM
Auger Eric March 29, 2017, 7:59 a.m. UTC | #3
Hi Juan,

On 28/03/2017 21:45, Juan Quintela wrote:
> Eric Auger <eric.auger@redhat.com> wrote:
>> We need to handle both registers and ITS tables. While
>> register handling is standard, ITS table handling is more
>> challenging since the kernel API is devised so that the
>> tables are flushed into guest RAM and not in vmstate buffers.
>>
>> Flushing the ITS tables on device pre_save() is too late
>> since the guest RAM is already saved at this point.
> 
> We need to put a way to register handlers for this.
Do you want this to happen in this series or do you think this can be
added later - but I guess this will depend on answer to your next
question ;-) -
> 
>> Table flushing needs to happen when we are sure the vcpus
>> are stopped and before the last dirty page saving. The
>> right point is RUN_STATE_FINISH_MIGRATE but sometimes the
>> VM gets stopped before migration launch so let's simply
>> flush the tables each time the VM gets stopped.
> 
> Just curious, how slow is doing that in all stops?
I will provide figures by the end of the week.
> 
> 
> No comments in the rest of the patch
> 
> 
>>  static void kvm_arm_its_init(Object *obj)
>> @@ -102,6 +122,80 @@ static void kvm_arm_its_init(Object *obj)
>>                               &error_abort);
>>  }
>>  
>> +/**
>> + * kvm_arm_its_pre_save - handles the saving of ITS registers.
>> + * ITS tables are flushed into guest RAM separately and earlier,
>> + * through the VM change state handler, since at the moment pre_save()
>> + * is called, the guest RAM has already been saved.
>> + */
>> +static void kvm_arm_its_pre_save(GICv3ITSState *s)
>> +{
> 
> ...
> 
>> +}
>> +
>> +/**
>> + * kvm_arm_its_post_load - Restore both the ITS registers and tables
>> + */
>> +static void kvm_arm_its_post_load(GICv3ITSState *s)
>> +{
> 
> ...
> 
>> +}
>> +
> 
> I assume that two functions are right.  I have no clue about ARM.
> 
>> @@ -109,6 +203,8 @@ static void kvm_arm_its_class_init(ObjectClass *klass, void *data)
>>  
>>      dc->realize = kvm_arm_its_realize;
>>      icc->send_msi = kvm_its_send_msi;
>> +    icc->pre_save = kvm_arm_its_pre_save;
>> +    icc->post_load = kvm_arm_its_post_load;
>>  }
> 
> Let me see if I understood this correctly.
> 
> We have an ARM_GICV3_ITS_COMMON.  And that has some fields.
> In particular:
> 
> struct GICv3ITSState {
>     /* Registers */
>     uint32_t ctlr;
>     uint64_t cbaser;
>     uint64_t cwriter;
>     uint64_t creadr;
>     uint64_t baser[8];
>     /* lots of things removed */
> };
> 
> 
> 
> We have this in arm_gicv3_its_common.c  (it is exactly the same for
> post_load, so we forgot about it by now).
> 
> 
> static void gicv3_its_pre_save(void *opaque)
> {
>     GICv3ITSState *s = (GICv3ITSState *)opaque; (*)
>                                                    /* nitpit: the cast
>                                                    is useless */
>     GICv3ITSCommonClass *c = ARM_GICV3_ITS_COMMON_GET_CLASS(s);
> 
>     if (c->pre_save) {
>         c->pre_save(s);
>     }
> }
> 
> And then we have in the patch:
> 
> 
>> @@ -109,6 +203,8 @@ static void kvm_arm_its_class_init(ObjectClass *klass, void *data)
>>  
>>      dc->realize = kvm_arm_its_realize;
>>      icc->send_msi = kvm_its_send_msi;
>> +    icc->pre_save = kvm_arm_its_pre_save;
>> +    icc->post_load = kvm_arm_its_post_load;
>>  }
> 
> 
> struct GICv3ITSCommonClass {
> ....
>     void (*pre_save)(GICv3ITSState *s);
>     void (*post_load)(GICv3ITSState *s);
> };
> 
> 
> Notice that I have only found one user of this on the tree, so I don't
> know if there is a good reason for this.
> 
> 
> static void gicv3_its_common_class_init(ObjectClass *klass, void *data)
> {
>     DeviceClass *dc = DEVICE_CLASS(klass);
> 
>     dc->reset = gicv3_its_common_reset;
>     dc->vmsd = &vmstate_its;
> }
> 
> So, what if we change:
> 
> const VMSField vmstate_its_fields[] = {
>      VMSTATE_UINT32(ctlr, GICv3ITSState),
>      VMSTATE_UINT32(iidr, GICv3ITSState),
>      VMSTATE_UINT64(cbaser, GICv3ITSState),
>      VMSTATE_UINT64(cwriter, GICv3ITSState),
>      VMSTATE_UINT64(creadr, GICv3ITSState),
>      VMSTATE_UINT64_ARRAY(baser, GICv3ITSState, 8),
>      VMSTATE_END_OF_LIST()
> };
> 
> 
> 
> Remove the dc->vmsd = &vmstate_its; from gicv3_its_common_class_init();
> 
> And we add in arm_gicv3_its_kvm.c
> 
> 
> static const VMStateDescription vmstate_its_kvm = {
>     .name = "arm_gicv3_its",
>     .pre_save = kvm_arm_its_pre_save,
>     .post_load = kvm_arm_its_post_load,
>     .fields = &vmsate_its_fields;
>     },
> };
> 
> And add the:
> 
> dc->vmstate = &vmastet_its_kvm;
> 
> into kvm_arm_its_class_init()?
> 
> And be with it?  Or it is too late by then?
> 
> I am assuming that there is some reason why we want to call
> arm_gicv3_its either for kvm or for anything else.  But IMHO, you are
> making things more complicated that they need to be.
> 
> My understanding:
> - We have GICv3 ITS state
> - We want to have several implementations
> - We want to be able to migration from one to another
> 
> 
> Or have I missed something?
> 
> Notice that I like more this other approach, but as far as I can see,
> yours should also work.

Yes at the moment it may look over-complicated but as Peter answered
this prepares for TCG ITS MSI controler model.

Thanks

Eric
> 
> Thanks, Juan.
> 
> 
> 
>
Auger Eric March 31, 2017, 10:11 a.m. UTC | #4
Hi Juan,

On 28/03/2017 21:45, Juan Quintela wrote:
> Eric Auger <eric.auger@redhat.com> wrote:
>> We need to handle both registers and ITS tables. While
>> register handling is standard, ITS table handling is more
>> challenging since the kernel API is devised so that the
>> tables are flushed into guest RAM and not in vmstate buffers.
>>
>> Flushing the ITS tables on device pre_save() is too late
>> since the guest RAM is already saved at this point.
> 
> We need to put a way to register handlers for this.
> 
>> Table flushing needs to happen when we are sure the vcpus
>> are stopped and before the last dirty page saving. The
>> right point is RUN_STATE_FINISH_MIGRATE but sometimes the
>> VM gets stopped before migration launch so let's simply
>> flush the tables each time the VM gets stopped.
> 
> Just curious, how slow is doing that in all stops?
The flush of the ITS tables takes about 40 microseconds in my use case
(measured with gettimeofday).

Thanks

Eric
> 
> 
> No comments in the rest of the patch
> 
> 
>>  static void kvm_arm_its_init(Object *obj)
>> @@ -102,6 +122,80 @@ static void kvm_arm_its_init(Object *obj)
>>                               &error_abort);
>>  }
>>  
>> +/**
>> + * kvm_arm_its_pre_save - handles the saving of ITS registers.
>> + * ITS tables are flushed into guest RAM separately and earlier,
>> + * through the VM change state handler, since at the moment pre_save()
>> + * is called, the guest RAM has already been saved.
>> + */
>> +static void kvm_arm_its_pre_save(GICv3ITSState *s)
>> +{
> 
> ...
> 
>> +}
>> +
>> +/**
>> + * kvm_arm_its_post_load - Restore both the ITS registers and tables
>> + */
>> +static void kvm_arm_its_post_load(GICv3ITSState *s)
>> +{
> 
> ...
> 
>> +}
>> +
> 
> I assume that two functions are right.  I have no clue about ARM.
> 
>> @@ -109,6 +203,8 @@ static void kvm_arm_its_class_init(ObjectClass *klass, void *data)
>>  
>>      dc->realize = kvm_arm_its_realize;
>>      icc->send_msi = kvm_its_send_msi;
>> +    icc->pre_save = kvm_arm_its_pre_save;
>> +    icc->post_load = kvm_arm_its_post_load;
>>  }
> 
> Let me see if I understood this correctly.
> 
> We have an ARM_GICV3_ITS_COMMON.  And that has some fields.
> In particular:
> 
> struct GICv3ITSState {
>     /* Registers */
>     uint32_t ctlr;
>     uint64_t cbaser;
>     uint64_t cwriter;
>     uint64_t creadr;
>     uint64_t baser[8];
>     /* lots of things removed */
> };
> 
> 
> 
> We have this in arm_gicv3_its_common.c  (it is exactly the same for
> post_load, so we forgot about it by now).
> 
> 
> static void gicv3_its_pre_save(void *opaque)
> {
>     GICv3ITSState *s = (GICv3ITSState *)opaque; (*)
>                                                    /* nitpit: the cast
>                                                    is useless */
>     GICv3ITSCommonClass *c = ARM_GICV3_ITS_COMMON_GET_CLASS(s);
> 
>     if (c->pre_save) {
>         c->pre_save(s);
>     }
> }
> 
> And then we have in the patch:
> 
> 
>> @@ -109,6 +203,8 @@ static void kvm_arm_its_class_init(ObjectClass *klass, void *data)
>>  
>>      dc->realize = kvm_arm_its_realize;
>>      icc->send_msi = kvm_its_send_msi;
>> +    icc->pre_save = kvm_arm_its_pre_save;
>> +    icc->post_load = kvm_arm_its_post_load;
>>  }
> 
> 
> struct GICv3ITSCommonClass {
> ....
>     void (*pre_save)(GICv3ITSState *s);
>     void (*post_load)(GICv3ITSState *s);
> };
> 
> 
> Notice that I have only found one user of this on the tree, so I don't
> know if there is a good reason for this.
> 
> 
> static void gicv3_its_common_class_init(ObjectClass *klass, void *data)
> {
>     DeviceClass *dc = DEVICE_CLASS(klass);
> 
>     dc->reset = gicv3_its_common_reset;
>     dc->vmsd = &vmstate_its;
> }
> 
> So, what if we change:
> 
> const VMSField vmstate_its_fields[] = {
>      VMSTATE_UINT32(ctlr, GICv3ITSState),
>      VMSTATE_UINT32(iidr, GICv3ITSState),
>      VMSTATE_UINT64(cbaser, GICv3ITSState),
>      VMSTATE_UINT64(cwriter, GICv3ITSState),
>      VMSTATE_UINT64(creadr, GICv3ITSState),
>      VMSTATE_UINT64_ARRAY(baser, GICv3ITSState, 8),
>      VMSTATE_END_OF_LIST()
> };
> 
> 
> 
> Remove the dc->vmsd = &vmstate_its; from gicv3_its_common_class_init();
> 
> And we add in arm_gicv3_its_kvm.c
> 
> 
> static const VMStateDescription vmstate_its_kvm = {
>     .name = "arm_gicv3_its",
>     .pre_save = kvm_arm_its_pre_save,
>     .post_load = kvm_arm_its_post_load,
>     .fields = &vmsate_its_fields;
>     },
> };
> 
> And add the:
> 
> dc->vmstate = &vmastet_its_kvm;
> 
> into kvm_arm_its_class_init()?
> 
> And be with it?  Or it is too late by then?
> 
> I am assuming that there is some reason why we want to call
> arm_gicv3_its either for kvm or for anything else.  But IMHO, you are
> making things more complicated that they need to be.
> 
> My understanding:
> - We have GICv3 ITS state
> - We want to have several implementations
> - We want to be able to migration from one to another
> 
> 
> Or have I missed something?
> 
> Notice that I like more this other approach, but as far as I can see,
> yours should also work.
> 
> Thanks, Juan.
> 
> 
> 
>

Patch
diff mbox

diff --git a/hw/intc/arm_gicv3_its_common.c b/hw/intc/arm_gicv3_its_common.c
index 9d67c5c..efab8c7 100644
--- a/hw/intc/arm_gicv3_its_common.c
+++ b/hw/intc/arm_gicv3_its_common.c
@@ -49,6 +49,15 @@  static const VMStateDescription vmstate_its = {
     .pre_save = gicv3_its_pre_save,
     .post_load = gicv3_its_post_load,
     .unmigratable = true,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(ctlr, GICv3ITSState),
+        VMSTATE_UINT32(iidr, GICv3ITSState),
+        VMSTATE_UINT64(cbaser, GICv3ITSState),
+        VMSTATE_UINT64(cwriter, GICv3ITSState),
+        VMSTATE_UINT64(creadr, GICv3ITSState),
+        VMSTATE_UINT64_ARRAY(baser, GICv3ITSState, 8),
+        VMSTATE_END_OF_LIST()
+    },
 };
 
 static MemTxResult gicv3_its_trans_read(void *opaque, hwaddr offset,
diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
index bd4f3aa..1732831 100644
--- a/hw/intc/arm_gicv3_its_kvm.c
+++ b/hw/intc/arm_gicv3_its_kvm.c
@@ -53,6 +53,24 @@  static int kvm_its_send_msi(GICv3ITSState *s, uint32_t value, uint16_t devid)
     return kvm_vm_ioctl(kvm_state, KVM_SIGNAL_MSI, &msi);
 }
 
+/**
+ * vm_change_state_handler - VM change state callback aiming at flushing
+ * ITS tables into guest RAM
+ *
+ * The tables get flushed to guest RAM whenever the VM gets stopped.
+ */
+static void vm_change_state_handler(void *opaque, int running,
+                                    RunState state)
+{
+    GICv3ITSState *s = (GICv3ITSState *)opaque;
+
+    if (running) {
+        return;
+    }
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_TABLES,
+                      0, NULL, false);
+}
+
 static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
 {
     GICv3ITSState *s = ARM_GICV3_ITS_COMMON(dev);
@@ -89,6 +107,8 @@  static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
     kvm_msi_use_devid = true;
     kvm_gsi_direct_mapping = false;
     kvm_msi_via_irqfd_allowed = kvm_irqfds_enabled();
+
+    qemu_add_vm_change_state_handler(vm_change_state_handler, s);
 }
 
 static void kvm_arm_its_init(Object *obj)
@@ -102,6 +122,80 @@  static void kvm_arm_its_init(Object *obj)
                              &error_abort);
 }
 
+/**
+ * kvm_arm_its_pre_save - handles the saving of ITS registers.
+ * ITS tables are flushed into guest RAM separately and earlier,
+ * through the VM change state handler, since at the moment pre_save()
+ * is called, the guest RAM has already been saved.
+ */
+static void kvm_arm_its_pre_save(GICv3ITSState *s)
+{
+    uint64_t reg;
+    int i;
+
+    for (i = 0; i < 8; i++) {
+        kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                          GITS_BASER + i * 8, &s->baser[i], false);
+    }
+
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                      GITS_CTLR, &s->ctlr, false);
+
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                      GITS_CBASER, &s->cbaser, false);
+
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                      GITS_CREADR, &s->creadr, false);
+
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                      GITS_CWRITER, &s->cwriter, false);
+
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                      GITS_IIDR, &s->iidr, false);
+}
+
+/**
+ * kvm_arm_its_post_load - Restore both the ITS registers and tables
+ */
+static void kvm_arm_its_post_load(GICv3ITSState *s)
+{
+    uint64_t reg;
+    int i;
+
+    if (!s->iidr) {
+        return;
+    }
+
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                      GITS_IIDR, &s->iidr, true);
+
+    /*
+     * must be written before GITS_CREADR since GITS_CBASER write
+     * access resets GITS_CREADR.
+     */
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                      GITS_CBASER, &s->cbaser, true);
+
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                      GITS_CREADR, &s->creadr, true);
+
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                      GITS_CWRITER, &s->cwriter, true);
+
+
+    for (i = 0; i < 8; i++) {
+        kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                          GITS_BASER + i * 8, &s->baser[i], true);
+    }
+
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_TABLES,
+                      0, NULL, true);
+
+    reg = s->ctlr;
+    kvm_device_access(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
+                      GITS_CTLR, &reg, true);
+}
+
 static void kvm_arm_its_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -109,6 +203,8 @@  static void kvm_arm_its_class_init(ObjectClass *klass, void *data)
 
     dc->realize = kvm_arm_its_realize;
     icc->send_msi = kvm_its_send_msi;
+    icc->pre_save = kvm_arm_its_pre_save;
+    icc->post_load = kvm_arm_its_post_load;
 }
 
 static const TypeInfo kvm_arm_its_info = {
diff --git a/include/hw/intc/arm_gicv3_its_common.h b/include/hw/intc/arm_gicv3_its_common.h
index 1ba1894..fd1fe64 100644
--- a/include/hw/intc/arm_gicv3_its_common.h
+++ b/include/hw/intc/arm_gicv3_its_common.h
@@ -28,6 +28,13 @@ 
 #define ITS_TRANS_SIZE   0x10000
 #define ITS_SIZE         (ITS_CONTROL_SIZE + ITS_TRANS_SIZE)
 
+#define GITS_CTLR        0x0
+#define GITS_IIDR        0x4
+#define GITS_CBASER      0x80
+#define GITS_CWRITER     0x88
+#define GITS_CREADR      0x90
+#define GITS_BASER       0x100
+
 struct GICv3ITSState {
     SysBusDevice parent_obj;
 
@@ -43,6 +50,7 @@  struct GICv3ITSState {
 
     /* Registers */
     uint32_t ctlr;
+    uint32_t iidr;
     uint64_t cbaser;
     uint64_t cwriter;
     uint64_t creadr;