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

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

Hi Dave,

On 26/01/2017 11:06, Dr. David Alan Gilbert wrote:
> * 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.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> <snip>
> 
>> diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
>> index 3f8017d..7f81d33 100644
>> --- a/hw/intc/arm_gicv3_its_kvm.c
>> +++ b/hw/intc/arm_gicv3_its_kvm.c
>> @@ -95,8 +95,12 @@ static void kvm_arm_its_realize(DeviceState *dev, Error **errp)
>>       * 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);
>> +    if (!kvm_device_check_attr(s->dev_fd, KVM_DEV_ARM_VGIC_GRP_ITS_REGS,
>> +                               GITS_CTLR)) {
>> +        error_setg(&s->migration_blocker, "This operating system kernel does "
>> +                                          "not support vITS migration");
>> +        migrate_add_blocker(s->migration_blocker);
>> +    }
> 
> Watch out, a change went in to the parameters/return value of migrate_add_blocker
> earlier in the week - it can now fail.
OK thanks for the notice
> 
>>      kvm_msi_use_devid = true;
>>      kvm_gsi_direct_mapping = false;
>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> index 1a22887..ebd755c 100644
>> --- a/include/migration/vmstate.h
>> +++ b/include/migration/vmstate.h
>> @@ -188,6 +188,8 @@ enum VMStateFlags {
>>  
>>  typedef enum {
>>      MIG_PRI_DEFAULT = 0,
>> +    MIG_PRI_GICV3_ITS,
>> +    MIG_PRI_GICV3,
>>      MIG_PRI_MAX,
> 
> Can we keep this commented so it's trivially easy to see the order, something like:
> 
>  typedef enum {
>      MIG_PRI_DEFAULT = 0,
> +    MIG_PRI_GICV3_ITS,    /* Needs to be before PCI devices */
> +    MIG_PRI_GICV3,        /* Must be before ITS */
Sure

Thanks!

Eric
>      MIG_PRI_MAX,
>  } MigrationPriority;
> 
> Dave
> 
>>  
>> -- 
>> 2.5.5
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>