Re: [Qemu-devel] [PATCH] spapr: Add forgotten capability to migration stream

[Greg Kurz <groug@kaod.org>]

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

On Mon, 20 May 2019 16:14:32 +1000
David Gibson <david@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 <david@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 <david@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 <groug@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;  
> >   
> 


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