[RFC,4/4] hw/intc/arm_gicv3_its: Allow save/restore
diff mbox

Message ID 1485422381-29019-5-git-send-email-eric.auger@redhat.com
State New
Headers show

Commit Message

Auger Eric Jan. 26, 2017, 9:19 a.m. UTC
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>
---
 hw/intc/arm_gicv3_common.c     | 1 +
 hw/intc/arm_gicv3_its_common.c | 3 ++-
 hw/intc/arm_gicv3_its_kvm.c    | 8 ++++++--
 include/migration/vmstate.h    | 2 ++
 4 files changed, 11 insertions(+), 3 deletions(-)

Comments

Dr. David Alan Gilbert Jan. 26, 2017, 10:06 a.m. UTC | #1
* 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.

>      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 */
     MIG_PRI_MAX,
 } MigrationPriority;

Dave

>  
> -- 
> 2.5.5
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Auger Eric Jan. 26, 2017, 1:30 p.m. UTC | #2
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
>
Peter Xu Feb. 3, 2017, 9:55 a.m. UTC | #3
On Thu, Jan 26, 2017 at 02:30:17PM +0100, Auger Eric wrote:

[...]

> >> 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!

Besides above: is it possible that in the future other platforms
(rather than ARM) can leverage these new introduced priority? If so,
would it be nicer that we use general names (like, e.g., INTCxxx? or
better?) rather than platform-specific names (like, GICxxx)?

Thanks,

-- peterx
Dr. David Alan Gilbert Feb. 3, 2017, 9:57 a.m. UTC | #4
* Peter Xu (peterx@redhat.com) wrote:
> On Thu, Jan 26, 2017 at 02:30:17PM +0100, Auger Eric wrote:
> 
> [...]
> 
> > >> 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!
> 
> Besides above: is it possible that in the future other platforms
> (rather than ARM) can leverage these new introduced priority? If so,
> would it be nicer that we use general names (like, e.g., INTCxxx? or
> better?) rather than platform-specific names (like, GICxxx)?

Yes, but the ordering rules on other platforms might be subtly different.

Dave

> Thanks,
> 
> -- peterx
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Peter Xu Feb. 3, 2017, 11:38 a.m. UTC | #5
On Fri, Feb 03, 2017 at 09:57:05AM +0000, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > On Thu, Jan 26, 2017 at 02:30:17PM +0100, Auger Eric wrote:
> > 
> > [...]
> > 
> > > >> 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!
> > 
> > Besides above: is it possible that in the future other platforms
> > (rather than ARM) can leverage these new introduced priority? If so,
> > would it be nicer that we use general names (like, e.g., INTCxxx? or
> > better?) rather than platform-specific names (like, GICxxx)?
> 
> Yes, but the ordering rules on other platforms might be subtly different.

I see. Then I have no problem in either way - we can rearrange the
defines until one day it is really needed. Thanks,

-- peterx

Patch
diff mbox

diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index 0f8c4b8..f80e60d 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -84,6 +84,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,
+    .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 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);
+    }
 
     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,
 } MigrationPriority;