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

David Gibson posted 1 patch 4 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190517041823.23871-1-david@gibson.dropbear.id.au
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test checkpatch passed
Test asan passed
Maintainers: David Gibson <david@gibson.dropbear.id.au>
hw/ppc/spapr.c         | 1 +
hw/ppc/spapr_caps.c    | 1 +
include/hw/ppc/spapr.h | 1 +
3 files changed, 3 insertions(+)
[Qemu-devel] [PATCH] spapr: Add forgotten capability to migration stream
Posted by David Gibson 4 years, 11 months ago
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"

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 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;
-- 
2.21.0


Re: [Qemu-devel] [PATCH] spapr: Add forgotten capability to migration stream
Posted by Cédric Le Goater 4 years, 11 months ago
On 5/17/19 6:18 AM, David Gibson 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"
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>



Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  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;
> 


Re: [Qemu-devel] [PATCH] spapr: Add forgotten capability to migration stream
Posted by Greg Kurz 4 years, 11 months ago
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.

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 <groug@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;


Re: [Qemu-devel] [PATCH] spapr: Add forgotten capability to migration stream
Posted by David Gibson 4 years, 11 months ago
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
pages).  And migration even works as long as it's 24 at both ends,
although it emits a bogus warning.

> 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;
> +}
> +
>  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;
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Re: [Qemu-devel] [PATCH] spapr: Add forgotten capability to migration stream
Posted by Greg Kurz 4 years, 11 months ago
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;  
> >   
>