On Mon, 20 May 2019 16:14:32 +1000 David Gibson <da...@gibson.dropbear.id.au> wrote:
> On Fri, May 17, 2019 at 07:14:30PM +0200, Greg Kurz wrote: > > 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. > > Ah, crud, that's a serious pain. > > > 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, > > Alas, it really does work with value 24 (giving you POWER8 16MiB My bad... you're right :) > pages). And migration even works as long as it's 24 at both ends, > although it emits a bogus warning. > Yeah I saw that... no big deal I guess. BTW, since dst >= src is legal, do we really need to keep this warning around for future releases ? > > Reviewed-by: Greg Kurz <gr...@kaod.org> > > > > Otherwise, I was thinking about something like this: > > Yeah, I think something like the below is the best we can do. > > > 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; And, of course, this should rather be: return !SPAPR_MACHINE_GET_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; > > >
pgpc8rqNEheOB.pgp
Description: OpenPGP digital signature