[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH] spapr: Add forgotten capability to migration stre
From: |
Greg Kurz |
Subject: |
Re: [Qemu-ppc] [PATCH] spapr: Add forgotten capability to migration stream |
Date: |
Mon, 20 May 2019 11:12:20 +0200 |
On Mon, 20 May 2019 16:14:32 +1000
David Gibson <address@hidden> 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 <address@hidden> 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 <address@hidden>
> > > ---
> >
> > 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 <address@hidden>
> >
> > 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;
> >
>
pgpq0JgGau3ki.pgp
Description: OpenPGP digital signature