diff mbox series

[1/7] spapr: Simplify handling of pre ISA 3.0 guest workaround handling

Message ID 20190911040452.8341-2-david@gibson.dropbear.id.au
State New, archived
Headers show
Series spapr: CAS and reset cleanup preliminaries | expand

Commit Message

David Gibson Sept. 11, 2019, 4:04 a.m. UTC
Certain old guest versions don't understand the radix MMU introduced with
POWER ISA 3.0, but incorrectly select it if presented with the option at
CAS time.  We workaround this in qemu by explicitly excluding the radix
(and other ISA 3.0 linked) options if the guest doesn't explicitly note
support for ISA 3.0.

This is handled by the 'cas_legacy_guest_workaround' flag, which is pretty
vague.  Rename it to 'cas_pre_isa3_guest' to be clearer about what it's for.

In addition, we unnecessarily call spapr_populate_pa_features() with
different options when initially constructing the device tree and when
adjusting it at CAS time.  At the initial construct time cas_pre_isa3_guest
is already false, so we can still use the flag, rather than explicitly
overriding it to be false at the callsite.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c         | 10 ++++------
 hw/ppc/spapr_hcall.c   |  3 +--
 include/hw/ppc/spapr.h |  2 +-
 3 files changed, 6 insertions(+), 9 deletions(-)

Comments

Cédric Le Goater Sept. 11, 2019, 7:09 a.m. UTC | #1
On 11/09/2019 06:04, David Gibson wrote:
> Certain old guest versions don't understand the radix MMU introduced with
> POWER ISA 3.0, but incorrectly select it if presented with the option at
> CAS time.  We workaround this in qemu by explicitly excluding the radix
> (and other ISA 3.0 linked) options if the guest doesn't explicitly note
> support for ISA 3.0.
> 
> This is handled by the 'cas_legacy_guest_workaround' flag, which is pretty
> vague.  Rename it to 'cas_pre_isa3_guest' to be clearer about what it's for.
> 
> In addition, we unnecessarily call spapr_populate_pa_features() with
> different options when initially constructing the device tree and when
> adjusting it at CAS time.  At the initial construct time cas_pre_isa3_guest
> is already false, so we can still use the flag, rather than explicitly
> overriding it to be false at the callsite.
> 
> 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         | 10 ++++------
>  hw/ppc/spapr_hcall.c   |  3 +--
>  include/hw/ppc/spapr.h |  2 +-
>  3 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7124053b43..c551001f86 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -218,8 +218,7 @@ static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, PowerPCCPU *cpu)
>  /* Populate the "ibm,pa-features" property */
>  static void spapr_populate_pa_features(SpaprMachineState *spapr,
>                                         PowerPCCPU *cpu,
> -                                       void *fdt, int offset,
> -                                       bool legacy_guest)
> +                                       void *fdt, int offset)
>  {
>      uint8_t pa_features_206[] = { 6, 0,
>          0xf6, 0x1f, 0xc7, 0x00, 0x80, 0xc0 };
> @@ -285,7 +284,7 @@ static void spapr_populate_pa_features(SpaprMachineState *spapr,
>      if ((spapr_get_cap(spapr, SPAPR_CAP_HTM) != 0) && pa_size > 24) {
>          pa_features[24] |= 0x80;    /* Transactional memory support */
>      }
> -    if (legacy_guest && pa_size > 40) {
> +    if (spapr->cas_pre_isa3_guest && pa_size > 40) {
>          /* Workaround for broken kernels that attempt (guest) radix
>           * mode when they can't handle it, if they see the radix bit set
>           * in pa-features. So hide it from them. */
> @@ -348,8 +347,7 @@ static int spapr_fixup_cpu_dt(void *fdt, SpaprMachineState *spapr)
>              return ret;
>          }
>  
> -        spapr_populate_pa_features(spapr, cpu, fdt, offset,
> -                                   spapr->cas_legacy_guest_workaround);
> +        spapr_populate_pa_features(spapr, cpu, fdt, offset);
>      }
>      return ret;
>  }
> @@ -551,7 +549,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>                            page_sizes_prop, page_sizes_prop_size)));
>      }
>  
> -    spapr_populate_pa_features(spapr, cpu, fdt, offset, false);
> +    spapr_populate_pa_features(spapr, cpu, fdt, offset);
>  
>      _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
>                             cs->cpu_index / vcpus_per_socket)));
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 23e4bdb829..3d3a67149a 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1765,8 +1765,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>              exit(EXIT_FAILURE);
>          }
>      }
> -    spapr->cas_legacy_guest_workaround = !spapr_ovec_test(ov1_guest,
> -                                                          OV1_PPC_3_00);
> +    spapr->cas_pre_isa3_guest = !spapr_ovec_test(ov1_guest, OV1_PPC_3_00);
>      spapr_ovec_cleanup(ov1_guest);
>      if (!spapr->cas_reboot) {
>          /* If spapr_machine_reset() did not set up a HPT but one is necessary
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 03111fd55b..dfec8e8e76 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -175,7 +175,7 @@ struct SpaprMachineState {
>  
>      /* ibm,client-architecture-support option negotiation */
>      bool cas_reboot;
> -    bool cas_legacy_guest_workaround;
> +    bool cas_pre_isa3_guest;
>      SpaprOptionVector *ov5;         /* QEMU-supported option vectors */
>      SpaprOptionVector *ov5_cas;     /* negotiated (via CAS) option vectors */
>      uint32_t max_compat_pvr;
>
Greg Kurz Sept. 11, 2019, 7:26 a.m. UTC | #2
On Wed, 11 Sep 2019 14:04:46 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Certain old guest versions don't understand the radix MMU introduced with
> POWER ISA 3.0, but incorrectly select it if presented with the option at
> CAS time.  We workaround this in qemu by explicitly excluding the radix
> (and other ISA 3.0 linked) options if the guest doesn't explicitly note
> support for ISA 3.0.
> 
> This is handled by the 'cas_legacy_guest_workaround' flag, which is pretty
> vague.  Rename it to 'cas_pre_isa3_guest' to be clearer about what it's for.
> 
> In addition, we unnecessarily call spapr_populate_pa_features() with
> different options when initially constructing the device tree and when
> adjusting it at CAS time.  At the initial construct time cas_pre_isa3_guest
> is already false, so we can still use the flag, rather than explicitly
> overriding it to be false at the callsite.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr.c         | 10 ++++------
>  hw/ppc/spapr_hcall.c   |  3 +--
>  include/hw/ppc/spapr.h |  2 +-
>  3 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7124053b43..c551001f86 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -218,8 +218,7 @@ static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, PowerPCCPU *cpu)
>  /* Populate the "ibm,pa-features" property */
>  static void spapr_populate_pa_features(SpaprMachineState *spapr,
>                                         PowerPCCPU *cpu,
> -                                       void *fdt, int offset,
> -                                       bool legacy_guest)
> +                                       void *fdt, int offset)
>  {
>      uint8_t pa_features_206[] = { 6, 0,
>          0xf6, 0x1f, 0xc7, 0x00, 0x80, 0xc0 };
> @@ -285,7 +284,7 @@ static void spapr_populate_pa_features(SpaprMachineState *spapr,
>      if ((spapr_get_cap(spapr, SPAPR_CAP_HTM) != 0) && pa_size > 24) {
>          pa_features[24] |= 0x80;    /* Transactional memory support */
>      }
> -    if (legacy_guest && pa_size > 40) {
> +    if (spapr->cas_pre_isa3_guest && pa_size > 40) {
>          /* Workaround for broken kernels that attempt (guest) radix
>           * mode when they can't handle it, if they see the radix bit set
>           * in pa-features. So hide it from them. */
> @@ -348,8 +347,7 @@ static int spapr_fixup_cpu_dt(void *fdt, SpaprMachineState *spapr)
>              return ret;
>          }
>  
> -        spapr_populate_pa_features(spapr, cpu, fdt, offset,
> -                                   spapr->cas_legacy_guest_workaround);
> +        spapr_populate_pa_features(spapr, cpu, fdt, offset);
>      }
>      return ret;
>  }
> @@ -551,7 +549,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>                            page_sizes_prop, page_sizes_prop_size)));
>      }
>  
> -    spapr_populate_pa_features(spapr, cpu, fdt, offset, false);
> +    spapr_populate_pa_features(spapr, cpu, fdt, offset);
>  
>      _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
>                             cs->cpu_index / vcpus_per_socket)));
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 23e4bdb829..3d3a67149a 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1765,8 +1765,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>              exit(EXIT_FAILURE);
>          }
>      }
> -    spapr->cas_legacy_guest_workaround = !spapr_ovec_test(ov1_guest,
> -                                                          OV1_PPC_3_00);
> +    spapr->cas_pre_isa3_guest = !spapr_ovec_test(ov1_guest, OV1_PPC_3_00);
>      spapr_ovec_cleanup(ov1_guest);
>      if (!spapr->cas_reboot) {
>          /* If spapr_machine_reset() did not set up a HPT but one is necessary
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 03111fd55b..dfec8e8e76 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -175,7 +175,7 @@ struct SpaprMachineState {
>  
>      /* ibm,client-architecture-support option negotiation */
>      bool cas_reboot;
> -    bool cas_legacy_guest_workaround;
> +    bool cas_pre_isa3_guest;
>      SpaprOptionVector *ov5;         /* QEMU-supported option vectors */
>      SpaprOptionVector *ov5_cas;     /* negotiated (via CAS) option vectors */
>      uint32_t max_compat_pvr;
Alexey Kardashevskiy Sept. 11, 2019, 7:37 a.m. UTC | #3
On 11/09/2019 14:04, David Gibson wrote:
> Certain old guest versions don't understand the radix MMU introduced with
> POWER ISA 3.0, but incorrectly select it if presented with the option at
> CAS time.  We workaround this in qemu by explicitly excluding the radix
> (and other ISA 3.0 linked) options if the guest doesn't explicitly note
> support for ISA 3.0.
> 
> This is handled by the 'cas_legacy_guest_workaround' flag, which is pretty
> vague.  Rename it to 'cas_pre_isa3_guest' to be clearer about what it's for.
> 
> In addition, we unnecessarily call spapr_populate_pa_features() with
> different options when initially constructing the device tree and when
> adjusting it at CAS time.  At the initial construct time cas_pre_isa3_guest
> is already false, so we can still use the flag, rather than explicitly
> overriding it to be false at the callsite.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>



Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>




> ---
>  hw/ppc/spapr.c         | 10 ++++------
>  hw/ppc/spapr_hcall.c   |  3 +--
>  include/hw/ppc/spapr.h |  2 +-
>  3 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7124053b43..c551001f86 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -218,8 +218,7 @@ static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, PowerPCCPU *cpu)
>  /* Populate the "ibm,pa-features" property */
>  static void spapr_populate_pa_features(SpaprMachineState *spapr,
>                                         PowerPCCPU *cpu,
> -                                       void *fdt, int offset,
> -                                       bool legacy_guest)
> +                                       void *fdt, int offset)
>  {
>      uint8_t pa_features_206[] = { 6, 0,
>          0xf6, 0x1f, 0xc7, 0x00, 0x80, 0xc0 };
> @@ -285,7 +284,7 @@ static void spapr_populate_pa_features(SpaprMachineState *spapr,
>      if ((spapr_get_cap(spapr, SPAPR_CAP_HTM) != 0) && pa_size > 24) {
>          pa_features[24] |= 0x80;    /* Transactional memory support */
>      }
> -    if (legacy_guest && pa_size > 40) {
> +    if (spapr->cas_pre_isa3_guest && pa_size > 40) {
>          /* Workaround for broken kernels that attempt (guest) radix
>           * mode when they can't handle it, if they see the radix bit set
>           * in pa-features. So hide it from them. */
> @@ -348,8 +347,7 @@ static int spapr_fixup_cpu_dt(void *fdt, SpaprMachineState *spapr)
>              return ret;
>          }
>  
> -        spapr_populate_pa_features(spapr, cpu, fdt, offset,
> -                                   spapr->cas_legacy_guest_workaround);
> +        spapr_populate_pa_features(spapr, cpu, fdt, offset);
>      }
>      return ret;
>  }
> @@ -551,7 +549,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>                            page_sizes_prop, page_sizes_prop_size)));
>      }
>  
> -    spapr_populate_pa_features(spapr, cpu, fdt, offset, false);
> +    spapr_populate_pa_features(spapr, cpu, fdt, offset);
>  
>      _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
>                             cs->cpu_index / vcpus_per_socket)));
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 23e4bdb829..3d3a67149a 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1765,8 +1765,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>              exit(EXIT_FAILURE);
>          }
>      }
> -    spapr->cas_legacy_guest_workaround = !spapr_ovec_test(ov1_guest,
> -                                                          OV1_PPC_3_00);
> +    spapr->cas_pre_isa3_guest = !spapr_ovec_test(ov1_guest, OV1_PPC_3_00);
>      spapr_ovec_cleanup(ov1_guest);
>      if (!spapr->cas_reboot) {
>          /* If spapr_machine_reset() did not set up a HPT but one is necessary
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 03111fd55b..dfec8e8e76 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -175,7 +175,7 @@ struct SpaprMachineState {
>  
>      /* ibm,client-architecture-support option negotiation */
>      bool cas_reboot;
> -    bool cas_legacy_guest_workaround;
> +    bool cas_pre_isa3_guest;
>      SpaprOptionVector *ov5;         /* QEMU-supported option vectors */
>      SpaprOptionVector *ov5_cas;     /* negotiated (via CAS) option vectors */
>      uint32_t max_compat_pvr;
>
diff mbox series

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 7124053b43..c551001f86 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -218,8 +218,7 @@  static int spapr_fixup_cpu_numa_dt(void *fdt, int offset, PowerPCCPU *cpu)
 /* Populate the "ibm,pa-features" property */
 static void spapr_populate_pa_features(SpaprMachineState *spapr,
                                        PowerPCCPU *cpu,
-                                       void *fdt, int offset,
-                                       bool legacy_guest)
+                                       void *fdt, int offset)
 {
     uint8_t pa_features_206[] = { 6, 0,
         0xf6, 0x1f, 0xc7, 0x00, 0x80, 0xc0 };
@@ -285,7 +284,7 @@  static void spapr_populate_pa_features(SpaprMachineState *spapr,
     if ((spapr_get_cap(spapr, SPAPR_CAP_HTM) != 0) && pa_size > 24) {
         pa_features[24] |= 0x80;    /* Transactional memory support */
     }
-    if (legacy_guest && pa_size > 40) {
+    if (spapr->cas_pre_isa3_guest && pa_size > 40) {
         /* Workaround for broken kernels that attempt (guest) radix
          * mode when they can't handle it, if they see the radix bit set
          * in pa-features. So hide it from them. */
@@ -348,8 +347,7 @@  static int spapr_fixup_cpu_dt(void *fdt, SpaprMachineState *spapr)
             return ret;
         }
 
-        spapr_populate_pa_features(spapr, cpu, fdt, offset,
-                                   spapr->cas_legacy_guest_workaround);
+        spapr_populate_pa_features(spapr, cpu, fdt, offset);
     }
     return ret;
 }
@@ -551,7 +549,7 @@  static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
                           page_sizes_prop, page_sizes_prop_size)));
     }
 
-    spapr_populate_pa_features(spapr, cpu, fdt, offset, false);
+    spapr_populate_pa_features(spapr, cpu, fdt, offset);
 
     _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
                            cs->cpu_index / vcpus_per_socket)));
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 23e4bdb829..3d3a67149a 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1765,8 +1765,7 @@  static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
             exit(EXIT_FAILURE);
         }
     }
-    spapr->cas_legacy_guest_workaround = !spapr_ovec_test(ov1_guest,
-                                                          OV1_PPC_3_00);
+    spapr->cas_pre_isa3_guest = !spapr_ovec_test(ov1_guest, OV1_PPC_3_00);
     spapr_ovec_cleanup(ov1_guest);
     if (!spapr->cas_reboot) {
         /* If spapr_machine_reset() did not set up a HPT but one is necessary
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 03111fd55b..dfec8e8e76 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -175,7 +175,7 @@  struct SpaprMachineState {
 
     /* ibm,client-architecture-support option negotiation */
     bool cas_reboot;
-    bool cas_legacy_guest_workaround;
+    bool cas_pre_isa3_guest;
     SpaprOptionVector *ov5;         /* QEMU-supported option vectors */
     SpaprOptionVector *ov5_cas;     /* negotiated (via CAS) option vectors */
     uint32_t max_compat_pvr;