Eric Auger <eric.au...@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.
