| Message ID | 1485422381-29019-5-git-send-email-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
* 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
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 >
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
* 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
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
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;
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(-)