On Fri, 17 May 2019 14:18:23 +1000 David Gibson <da...@gibson.dropbear.id.au> wrote:
> spapr machine capabilities are supposed to be sent in the migration stream > so that we can sanity check the source and destination have compatible > configuration. Unfortunately, when we added the hpt-max-page-size > capability, we forgot to add it to the migration state. This means that we > can generate spurious warnings when both ends are configured for large > pages, or potentially fail to warn if the source is configured for huge > pages, but the destination is not. > > Fixes: 2309832afda "spapr: Maximum (HPT) pagesize property" > Sorry I didn't spot that during review :-\ > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au> > --- This breaks backward migration if the cap is set to a non-default value, since older QEMUs don't expect the "spapr/cap/hpt_maxpagesize" subsection. This being said, I'm not sure any other value but the current default (16) actually works, so maybe we don't care. If so, Reviewed-by: Greg Kurz <gr...@kaod.org> Otherwise, I was thinking about something like this: 8<============================================================================= diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index 9fc91c8f5eac..4f5becf1f3cc 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -119,6 +119,7 @@ struct SpaprMachineClass { bool pre_2_10_has_unused_icps; bool legacy_irq_allocation; bool broken_host_serial_model; /* present real host info to the guest */ + bool pre_4_1_migration; /* don't migrate hpt-max-page-size */ void (*phb_placement)(SpaprMachineState *spapr, uint32_t index, uint64_t *buid, hwaddr *pio, diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index bcae30ad26c3..c8b3cccd5375 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -4413,8 +4413,12 @@ DEFINE_SPAPR_MACHINE(4_1, "4.1", true); */ static void spapr_machine_4_0_class_options(MachineClass *mc) { + SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc); + spapr_machine_4_1_class_options(mc); compat_props_add(mc->compat_props, hw_compat_4_0, hw_compat_4_0_len); + + smc->pre_4_1_migration = true; } DEFINE_SPAPR_MACHINE(4_0, "4.0", false); diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c index 658eb15a147b..8a77bbdcf322 100644 --- a/hw/ppc/spapr_caps.c +++ b/hw/ppc/spapr_caps.c @@ -64,6 +64,7 @@ typedef struct SpaprCapabilityInfo { void (*apply)(SpaprMachineState *spapr, uint8_t val, Error **errp); void (*cpu_apply)(SpaprMachineState *spapr, PowerPCCPU *cpu, uint8_t val, Error **errp); + bool (*migrate_needed)(void *opaque); } SpaprCapabilityInfo; static void spapr_cap_get_bool(Object *obj, Visitor *v, const char *name, @@ -350,6 +351,11 @@ static void cap_hpt_maxpagesize_apply(SpaprMachineState *spapr, spapr_check_pagesize(spapr, qemu_minrampagesize(), errp); } +static bool cap_hpt_maxpagesize_migrate_needed(void *opaque) +{ + return SPAPR_MACHINE_CLASS(opaque)->pre_4_1_migration; +} + static bool spapr_pagesize_cb(void *opaque, uint32_t seg_pshift, uint32_t pshift) { @@ -542,6 +548,7 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = { .type = "int", .apply = cap_hpt_maxpagesize_apply, .cpu_apply = cap_hpt_maxpagesize_cpu_apply, + .migrate_needed = cap_hpt_maxpagesize_migrate_needed, }, [SPAPR_CAP_NESTED_KVM_HV] = { .name = "nested-hv", @@ -679,8 +686,11 @@ int spapr_caps_post_migration(SpaprMachineState *spapr) static bool spapr_cap_##sname##_needed(void *opaque) \ { \ SpaprMachineState *spapr = opaque; \ + bool (*needed)(void *opaque) = \ + capability_table[cap].migrate_needed; \ \ - return spapr->cmd_line_caps[cap] && \ + return needed ? needed(opaque) : true && \ + spapr->cmd_line_caps[cap] && \ (spapr->eff.caps[cap] != \ spapr->def.caps[cap]); \ } \ 8<============================================================================ > hw/ppc/spapr.c | 1 + > hw/ppc/spapr_caps.c | 1 + > include/hw/ppc/spapr.h | 1 + > 3 files changed, 3 insertions(+) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 8580a8dc67..bcae30ad26 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2125,6 +2125,7 @@ static const VMStateDescription vmstate_spapr = { > &vmstate_spapr_cap_cfpc, > &vmstate_spapr_cap_sbbc, > &vmstate_spapr_cap_ibs, > + &vmstate_spapr_cap_hpt_maxpagesize, > &vmstate_spapr_irq_map, > &vmstate_spapr_cap_nested_kvm_hv, > &vmstate_spapr_dtb, > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c > index 9b1c10baa6..658eb15a14 100644 > --- a/hw/ppc/spapr_caps.c > +++ b/hw/ppc/spapr_caps.c > @@ -703,6 +703,7 @@ SPAPR_CAP_MIG_STATE(dfp, SPAPR_CAP_DFP); > SPAPR_CAP_MIG_STATE(cfpc, SPAPR_CAP_CFPC); > SPAPR_CAP_MIG_STATE(sbbc, SPAPR_CAP_SBBC); > SPAPR_CAP_MIG_STATE(ibs, SPAPR_CAP_IBS); > +SPAPR_CAP_MIG_STATE(hpt_maxpagesize, SPAPR_CAP_HPT_MAXPAGESIZE); > SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV); > SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER); > SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST); > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index 7e32f309c2..9fc91c8f5e 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -849,6 +849,7 @@ extern const VMStateDescription vmstate_spapr_cap_dfp; > extern const VMStateDescription vmstate_spapr_cap_cfpc; > extern const VMStateDescription vmstate_spapr_cap_sbbc; > extern const VMStateDescription vmstate_spapr_cap_ibs; > +extern const VMStateDescription vmstate_spapr_cap_hpt_maxpagesize; > extern const VMStateDescription vmstate_spapr_cap_nested_kvm_hv; > extern const VMStateDescription vmstate_spapr_cap_large_decr; > extern const VMStateDescription vmstate_spapr_cap_ccf_assist;