Re: [Qemu-devel] [RFC v2 3/3] hw/intc/arm_gicv3_its: Allow save/restore

[Auger Eric <eric.auger@redhat.com>]

Hi Peter,

On 13/03/2017 19:03, Peter Maydell wrote:
> On 6 March 2017 at 12:48, Eric Auger <eric.auger@redhat.com> wrote:
>> We change the restoration priority of both the GICv3 and ITS. The
>> GICv3 must be restored before the ITS and the ITS needs to be restored
>> before PCIe devices since it translates their MSI transactions.
> 
>> We typically observe the virtio-pci-net device sending MSI transactions
>> very early (even before the first vcpu run) which looks weird. It
>> appears that not servicing those transactions cause the virtio-pci-net
>> to stall.
> 
> This does seem rather weird and worth closer investigation.
> A stopped VM ought IMHO to be completely stopped, not still
> doing things...
Yes I will take time to investigate what does happen.

> 
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> v1 -> v2:
>> - handle case where migrate_add_blocker fails
>> - add comments along with ITS and GICv3 migration priorities
>> ---
>>  hw/intc/arm_gicv3_common.c     |  1 +
>>  hw/intc/arm_gicv3_its_common.c |  3 ++-
>>  hw/intc/arm_gicv3_its_kvm.c    | 23 +++++++++++------------
>>  include/migration/vmstate.h    |  2 ++
>>  4 files changed, 16 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
>> index c6493d6..4228b7c 100644
>> --- a/hw/intc/arm_gicv3_common.c
>> +++ b/hw/intc/arm_gicv3_common.c
>> @@ -145,6 +145,7 @@ static const VMStateDescription vmstate_gicv3 = {
>>      .minimum_version_id = 1,
>>      .pre_save = gicv3_pre_save,
>>      .post_load = gicv3_post_load,
>> +    .priority = MIG_PRI_GICV3,
>>      .fields = (VMStateField[]) {
>>          VMSTATE_UINT32(gicd_ctlr, GICv3State),
>>          VMSTATE_UINT32_ARRAY(gicd_statusr, GICv3State, 2),
>> diff --git a/hw/intc/arm_gicv3_its_common.c b/hw/intc/arm_gicv3_its_common.c
>> index 75b9f04..854709f 100644
>> --- a/hw/intc/arm_gicv3_its_common.c
>> +++ b/hw/intc/arm_gicv3_its_common.c
>> @@ -48,7 +48,8 @@ static const VMStateDescription vmstate_its = {
>>      .name = "arm_gicv3_its",
>>      .pre_save = gicv3_its_pre_save,
>>      .post_load = gicv3_its_post_load,
>> -    .unmigratable = true,
>> +    .unmigratable = false,
> 
> unmigratable = false is the default, so we can just delete the line.
OK thanks
> 
>> +    .priority = MIG_PRI_GICV3_ITS,
>>      .fields = (VMStateField[]) {
>>          VMSTATE_UINT32(ctlr, GICv3ITSState),
>>          VMSTATE_UINT64(cbaser, GICv3ITSState),
>> diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
>> index 45e57d6..3bd2873 100644
>> --- a/hw/intc/arm_gicv3_its_kvm.c
>> +++ b/hw/intc/arm_gicv3_its_kvm.c
>> @@ -76,18 +76,6 @@ static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
>>      GICv3ITSState *s = ARM_GICV3_ITS_COMMON(dev);
>>      Error *local_err = NULL;
>>
>> -    /*
>> -     * Block migration of a KVM GICv3 ITS device: the API for saving and
>> -     * restoring the state in the kernel is not yet available
>> -     */
>> -    error_setg(&s->migration_blocker, "vITS migration is not implemented");
>> -    migrate_add_blocker(s->migration_blocker, &local_err);
>> -    if (local_err) {
>> -        error_propagate(errp, local_err);
>> -        error_free(s->migration_blocker);
>> -        return;
>> -    }
>> -
>>      s->dev_fd = kvm_create_device(kvm_state, KVM_DEV_TYPE_ARM_VGIC_ITS, false);
>>      if (s->dev_fd < 0) {
>>          error_setg_errno(errp, -s->dev_fd, "error creating in-kernel ITS");
>> @@ -104,6 +92,17 @@ static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
>>
>>      gicv3_its_init_mmio(s, NULL);
>>
>> +    if (!kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
>> +        GITS_CTLR)) {
>> +        error_setg(&s->migration_blocker, "vITS migration is not implemented");
> 
> I think we should specifically say that it's the host kernel
> that doesn't support ITS migration (ie not QEMU that's missing
> support).
> 
> The GICv3 uses the message
>  "This operating system kernel does not support vGICv3 migration"
> which is slightly odd phrasing but I guess we should follow that.
Sure I will use that message.

Thank you for the review

Best Regards

Eric
> 
>> +        migrate_add_blocker(s->migration_blocker, &local_err);
>> +        if (local_err) {
>> +            error_propagate(errp, local_err);
>> +            error_free(s->migration_blocker);
>> +            return;
>> +        }
>> +    }
>> +
>>      kvm_msi_use_devid = true;
>>      kvm_gsi_direct_mapping = false;
>>      kvm_msi_via_irqfd_allowed = kvm_irqfds_enabled();
>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> index f2dbf84..8dab9c7 100644
>> --- a/include/migration/vmstate.h
>> +++ b/include/migration/vmstate.h
>> @@ -198,6 +198,8 @@ enum VMStateFlags {
>>  typedef enum {
>>      MIG_PRI_DEFAULT = 0,
>>      MIG_PRI_IOMMU,              /* Must happen before PCI devices */
>> +    MIG_PRI_GICV3_ITS,          /* Must happen before PCI devices */
>> +    MIG_PRI_GICV3,              /* Must happen before the ITS */
>>      MIG_PRI_MAX,
>>  } MigrationPriority;
>>
>> --
>> 2.5.5
>>
> 
> thanks
> -- PMM
>