On Fri, 11 Oct 2019 08:13:33 +0200 Greg Kurz wrote: > On Fri, 11 Oct 2019 16:07:58 +1100 > David Gibson 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 wrote: > > > > > > > On Thu, 10 Oct 2019 13:02:09 +1100 > > > > David Gibson 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 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 > > > > > > > > > > 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; > }