Re: [PATCH v4 17/19] spapr: Remove last pieces of SpaprIrq

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 9657 bytes --]

On Fri, 11 Oct 2019 08:13:33 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Fri, 11 Oct 2019 16:07:58 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Oct 10, 2019 at 10:33:04PM +0200, Greg Kurz wrote:
> > > On Thu, 10 Oct 2019 08:29:58 +0200
> > > Greg Kurz <groug@kaod.org> wrote:
> > > 
> > > > On Thu, 10 Oct 2019 13:02:09 +1100
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > 
> > > > > On Wed, Oct 09, 2019 at 07:02:15PM +0200, Greg Kurz wrote:
> > > > > > On Wed,  9 Oct 2019 17:08:16 +1100
> > > > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > > > 
> > > > > > > The only thing remaining in this structure are the flags to allow either
> > > > > > > XICS or XIVE to be present.  These actually make more sense as spapr
> > > > > > > capabilities - that way they can take advantage of the existing
> > > > > > > infrastructure to sanity check capability states across migration and so
> > > > > > > forth.
> > > > > > > 
> > > > > > 
> > > > > > The user can now choose the interrupt controller mode either through
> > > > > > ic-mode or through cap-xics/cap-xive. I guess it doesn't break anything
> > > > > > to expose another API to do the same thing but it raises some questions.
> > > > > > 
> > > > > > We should at least document somewhere that ic-mode is an alias to these
> > > > > > caps, and maybe state which is the preferred method (I personally vote
> > > > > > for the caps).
> > > > > > 
> > > > > > Also, we must keep ic-mode for the moment to stay compatible with the
> > > > > > existing pseries-4.0 and pseries-4.1 machine types, but will we
> > > > > > keep ic-mode forever ? If no, maybe start by not allowing it for
> > > > > > pseries-4.2 ?
> > > > > 
> > > > > I'm actually inclined to keep it for now, maybe even leave it as the
> > > > > suggested way to configure this.  The caps are nice from an internal
> > > > > organization point of view, but ic-mode is arguably a more user
> > > > > friendly way of configuring it.  The conversion of one to the other is
> > > > > straightforward, isolated ans small, so I'm not especially bothered by
> > > > > keeping it around.
> > > > > 
> > > > 
> > > > Fair enough.
> > > > 
> > > > Reviewed-by: Greg Kurz <groug@kaod.org>
> > > > 
> > > 
> > > But unfortunately this still requires care :-\
> > > 
> > > qemu-system-ppc64: cap-xive higher level (1) in incoming stream than on destination (0)
> > > qemu-system-ppc64: error while loading state for instance 0x0 of device 'spapr'
> > > qemu-system-ppc64: load of migration failed: Invalid argument
> > > 
> > > or
> > > 
> > > qemu-system-ppc64: cap-xics higher level (1) in incoming stream than on destination (0)
> > > qemu-system-ppc64: error while loading state for instance 0x0 of device 'spapr'
> > > qemu-system-ppc64: load of migration failed: Invalid argument
> > > 
> > > when migrating from QEMU 4.1 with ic-mode=xics and ic-mode=xive respectively.
> > > 
> > > This happens because the existing pseries-4.1 machine type doesn't send the
> > > new caps and the logic in spapr_caps_post_migration() wrongly assumes that
> > > the source has both caps set:
> > > 
> > >     srccaps = default_caps_with_cpu(spapr, MACHINE(spapr)->cpu_type);
> > >     for (i = 0; i < SPAPR_CAP_NUM; i++) {
> > >         /* If not default value then assume came in with the migration */
> > >         if (spapr->mig.caps[i] != spapr->def.caps[i]) {
> > > 
> > > spapr->mig.caps[SPAPR_CAP_XICS] = 0
> > > spapr->mig.caps[SPAPR_CAP_XIVE] = 0
> > > 
> > >             srccaps.caps[i] = spapr->mig.caps[i];
> > > 
> > > srcaps.caps[SPAPR_CAP_XICS] = 1
> > > srcaps.caps[SPAPR_CAP_XIVE] = 1
> > > 
> > >         }
> > >     }
> > > 
> > > and breaks
> > > 
> > >     for (i = 0; i < SPAPR_CAP_NUM; i++) {
> > >         SpaprCapabilityInfo *info = &capability_table[i];
> > > 
> > >         if (srccaps.caps[i] > dstcaps.caps[i]) {
> > > 
> > > srcaps.caps[SPAPR_CAP_XICS] = 0 when ic-mode=xive
> > > srcaps.caps[SPAPR_CAP_XIVE] = 0 when ic-mode=xics
> > > 
> > >             error_report("cap-%s higher level (%d) in incoming stream than on destination (%d)",
> > >                          info->name, srccaps.caps[i], dstcaps.caps[i]);
> > >             ok = false;
> > >         }
> > 
> > Ah.. right.  I thought there would be problems with backwards
> > migration, but I didn't think of this problem even with forward
> > migration.
> > 
> > > Maybe we shouldn't check capabilities that we know the source
> > > isn't supposed to send, eg. by having a smc->max_cap ?
> > 
> > Uh.. I'm not really sure what exactly you're suggesting here.
> > 
> 
> I'm suggesting to have a per-machine version smc->max_cap that
> contains the highest supported cap index, to be used instead of
> SPAPR_CAP_NUM in this functions, ie.
> 
> for (i = 0; i <= smc->max_cap; i++) {
>     ...
> }
> 
> where we would have
> 
> smc->max_cap = SPAPR_CAP_CCF_ASSIST for pseries-4.1
> 
> and
> 
> smc->max_cap = SPAPR_CAP_XIVE for psereis-4.2
> 
> > I think what we need here is a custom migrate_needed function, like we
> > already have for cap_hpt_maxpagesize, to exclude it from the migration
> > stream for machine versions before 4.2.
> > 
> 
> No, VMState needed() hooks are for outgoing migration only.
> 

Well we actually do need a needed() function to fix backward
migration, but it doesn't solve anything with forward migration.

I'm thinking about something like this to address both:

=======================================================================
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 66b68fdd5ef5..1342058c1aae 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -83,7 +83,12 @@ typedef enum {
 #define SPAPR_CAP_XICS                  0x0a
 /* XIVE interrupt controller */
 #define SPAPR_CAP_XIVE                  0x0b
-/* Num Caps */
+/*
+ * Num Caps.
+ *
+ * CAUTION: when new caps are being added, older machine types should
+ * set smc->mig_cap_num to the previous value of SPAPR_CAP_NUM.
+ */
 #define SPAPR_CAP_NUM                   (SPAPR_CAP_XIVE + 1)
 
 /*
@@ -135,6 +140,7 @@ struct SpaprMachineClass {
                           hwaddr *nv2atsd, Error **errp);
     SpaprResizeHpt resize_hpt_default;
     SpaprCapabilities default_caps;
+    int mig_cap_num; /* don't migrate newer capabilities */
 };
 
 /**
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index bf9fdb169303..fa81cedfbcc5 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4453,6 +4453,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     smc->dr_phb_enabled = true;
     smc->linux_pci_probe = true;
     smc->nr_xirqs = SPAPR_NR_XIRQS;
+    smc->mig_cap_num = SPAPR_CAP_NUM;
 }
 
 static const TypeInfo spapr_machine_info = {
@@ -4520,6 +4521,7 @@ static void spapr_machine_4_1_class_options(MachineClass *mc)
 
     spapr_machine_4_2_class_options(mc);
     smc->linux_pci_probe = false;
+    smc->mig_cap_num = SPAPR_CAP_CCF_ASSIST + 1;
     compat_props_add(mc->compat_props, hw_compat_4_1, hw_compat_4_1_len);
     compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat));
 }
diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index e06fd386f6ac..ba079f46e084 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -532,6 +532,13 @@ static void cap_xive_apply(SpaprMachineState *spapr, uint8_t val, Error **errp)
     }
 }
 
+static bool cap_xics_xive_migrate_needed(void *opaque)
+{
+    int mig_cap_num = SPAPR_MACHINE_GET_CLASS(opaque)->mig_cap_num;
+
+    return mig_cap_num > SPAPR_CAP_XIVE && mig_cap_num > SPAPR_CAP_XICS;
+}
+
 SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
     [SPAPR_CAP_HTM] = {
         .name = "htm",
@@ -639,6 +646,7 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
         .set = spapr_cap_set_bool,
         .type = "bool",
         .apply = cap_xics_apply,
+        .migrate_needed = cap_xics_xive_migrate_needed,
     },
     [SPAPR_CAP_XIVE] = {
         .name = "xive",
@@ -648,6 +656,7 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
         .set = spapr_cap_set_bool,
         .type = "bool",
         .apply = cap_xive_apply,
+        .migrate_needed = cap_xics_xive_migrate_needed,
     },
 };
 
@@ -729,20 +738,21 @@ int spapr_caps_pre_save(void *opaque)
  * caps on the destination */
 int spapr_caps_post_migration(SpaprMachineState *spapr)
 {
+    SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
     int i;
     bool ok = true;
     SpaprCapabilities dstcaps = spapr->eff;
     SpaprCapabilities srccaps;
 
     srccaps = default_caps_with_cpu(spapr, MACHINE(spapr)->cpu_type);
-    for (i = 0; i < SPAPR_CAP_NUM; i++) {
+    for (i = 0; i < smc->mig_cap_num; i++) {
         /* If not default value then assume came in with the migration */
         if (spapr->mig.caps[i] != spapr->def.caps[i]) {
             srccaps.caps[i] = spapr->mig.caps[i];
         }
     }
 
-    for (i = 0; i < SPAPR_CAP_NUM; i++) {
+    for (i = 0; i < smc->mig_cap_num; i++) {
         SpaprCapabilityInfo *info = &capability_table[i];
 
         if (srccaps.caps[i] > dstcaps.caps[i]) {
=======================================================================

> bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque)
> {
>     if (vmsd->needed && !vmsd->needed(opaque)) {
>         /* optional section not needed */
>         return false;
>     }
>     return true;
> }


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]