[Qemu-devel] [RFC 0/2] pseries: Correct behaviour change for older machine types

[Qemu-devel] [RFC 0/2] pseries: Correct behaviour change for older machine types

[David Gibson]

152ef80 "pseries: Rewrite CAS PVR compatibility logic" incorrectly
introduced a guest-visible behaviour change into existing versioned
machine type.  Patch 2/2 corrects this change, while 1/2 is a
preliminary clean up to make that easier.

Unfortunately, this bug is already in the released qemu-2.9.0.  Once
this is ready to merge for master, I'll make a backport to apply for
2.9.1 as well.

David Gibson (2):
  pseries: Split CAS PVR negotiation out into a separate function
  pseries: Restore PVR negotiation logic for pre-2.9 machine types

 hw/ppc/spapr.c         |   3 ++
 hw/ppc/spapr_hcall.c   | 131 +++++++++++++++++++++++++++++++++++++++++++------
 include/hw/ppc/spapr.h |   1 +
 3 files changed, 119 insertions(+), 16 deletions(-)

-- 
2.9.4

[Qemu-devel] [RFC 1/2] pseries: Split CAS PVR negotiation out into a separate function

[David Gibson]

Guests of the qemu machine type go through a feature negotiation process
known as "client architecture support" (CAS) during early boot.  This does
a number of things, one of which is finding a CPU compatibility mode which
can be supported by both guest and host.

In fact the CPU negotiation is probably the single most complex part of the
CAS process, so this splits it out into a helper function.  We've recently
made some mistakes in maintaining backward compatibility for old machine
types here.  Splitting this out will also make it easier to fix this.

This also adds a possibly useful error message if the negotiation fails
(i.e. if there is CPU mode that's suitable for both guest and host).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_hcall.c | 43 +++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 0d608d6..007ae8d 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1047,19 +1047,13 @@ static target_ulong h_signal_sys_reset(PowerPCCPU *cpu,
     }
 }
 
-static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
-                                                  sPAPRMachineState *spapr,
-                                                  target_ulong opcode,
-                                                  target_ulong *args)
+static uint32_t cas_check_pvr(PowerPCCPU *cpu, target_ulong list,
+                              Error **errp)
 {
-    target_ulong list = ppc64_phys_to_real(args[0]);
-    target_ulong ov_table;
     bool explicit_match = false; /* Matched the CPU's real PVR */
     uint32_t max_compat = cpu->max_compat;
     uint32_t best_compat = 0;
     int i;
-    sPAPROptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
-    bool guest_radix;
 
     /*
      * We scan the supplied table of PVRs looking for two things
@@ -1090,26 +1084,43 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
         /* We couldn't find a suitable compatibility mode, and either
          * the guest doesn't support "raw" mode for this CPU, or raw
          * mode is disabled because a maximum compat mode is set */
-        return H_HARDWARE;
+        error_setg(errp, "Couldn't negotiate a suitable PVR during CAS");
+        return 0;
     }
 
     /* Parsing finished */
     trace_spapr_cas_pvr(cpu->compat_pvr, explicit_match, best_compat);
 
-    /* Update CPUs */
-    if (cpu->compat_pvr != best_compat) {
-        Error *local_err = NULL;
+    return best_compat;
+}
+
+static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
+                                                  sPAPRMachineState *spapr,
+                                                  target_ulong opcode,
+                                                  target_ulong *args)
+{
+    /* @ov_table points to the first option vector */
+    target_ulong ov_table = ppc64_phys_to_real(args[0]);
+    uint32_t cas_pvr;
+    sPAPROptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
+    bool guest_radix;
+    Error *local_err = NULL;
 
-        ppc_set_compat_all(best_compat, &local_err);
+    cas_pvr = cas_check_pvr(cpu, ov_table, &local_err);
+    if (local_err) {
+        error_report_err(local_err);
+        return H_HARDWARE;
+    }
+
+    /* Update CPUs */
+    if (cpu->compat_pvr != cas_pvr) {
+        ppc_set_compat_all(cas_pvr, &local_err);
         if (local_err) {
             error_report_err(local_err);
             return H_HARDWARE;
         }
     }
 
-    /* For the future use: here @ov_table points to the first option vector */
-    ov_table = list;
-
     ov1_guest = spapr_ovec_parse_vector(ov_table, 1);
     ov5_guest = spapr_ovec_parse_vector(ov_table, 5);
     if (spapr_ovec_test(ov5_guest, OV5_MMU_BOTH)) {
-- 
2.9.4

[Qemu-devel] [RFC 2/2] pseries: Restore PVR negotiation logic for pre-2.9 machine types

[David Gibson]

"pseries" guests go through a hypervisor<->guest feature negotiation during
early boot.  Part of this is finding a CPU compatibility mode which works
for both.

In 152ef80 "pseries: Rewrite CAS PVR compatibility logic" this logic was
changed to strongly prefer architecture defined CPU compatibility modes,
rather than CPU "raw" modes.  However, this change was made universally,
which introduces a guest visible change for the previously existing machine
types (pseries-2.8 and earlier).  That's never supposed to happen.

This corrects the behaviour, reverting to the old PVR negotiation logic
for machine types prior to pseries-2.9.

Fixes: 152ef803ceb1959e2380a1da7736b935b109222e
Reported-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c         |  3 ++
 hw/ppc/spapr_hcall.c   | 90 +++++++++++++++++++++++++++++++++++++++++++++++++-
 include/hw/ppc/spapr.h |  1 +
 3 files changed, 93 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index a9471b9..913355f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3308,9 +3308,12 @@ static void spapr_machine_2_8_instance_options(MachineState *machine)
 
 static void spapr_machine_2_8_class_options(MachineClass *mc)
 {
+    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
+
     spapr_machine_2_9_class_options(mc);
     SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_8);
     mc->numa_mem_align_shift = 23;
+    smc->pre_2_9_cas_pvr = true;
 }
 
 DEFINE_SPAPR_MACHINE(2_8, "2.8", false);
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 007ae8d..4937019 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1047,6 +1047,89 @@ static target_ulong h_signal_sys_reset(PowerPCCPU *cpu,
     }
 }
 
+/*
+ * Old logic for PVR negotiation, used old <2.9 machine types for
+ * compatibility with old qemu versions
+ */
+#define get_compat_level(cpuver) (                  \
+        ((cpuver) == CPU_POWERPC_LOGICAL_2_05) ? 2050 : \
+        ((cpuver) == CPU_POWERPC_LOGICAL_2_06) ? 2060 :  \
+        ((cpuver) == CPU_POWERPC_LOGICAL_2_06_PLUS) ? 2061 :    \
+        ((cpuver) == CPU_POWERPC_LOGICAL_2_07) ? 2070 : 0)
+
+static void cas_handle_compat_cpu(PowerPCCPUClass *pcc, uint32_t pvr,
+                                  unsigned max_lvl, unsigned *compat_lvl,
+                                  unsigned *compat_pvr)
+{
+    unsigned lvl = get_compat_level(pvr);
+    bool is205, is206, is207;
+
+    if (!lvl) {
+        return;
+    }
+
+    /* If it is a logical PVR, try to determine the highest level */
+    is205 = (pcc->pcr_supported & PCR_COMPAT_2_05) &&
+        (lvl == get_compat_level(CPU_POWERPC_LOGICAL_2_05));
+    is206 = (pcc->pcr_supported & PCR_COMPAT_2_06) &&
+        ((lvl == get_compat_level(CPU_POWERPC_LOGICAL_2_06)) ||
+         (lvl == get_compat_level(CPU_POWERPC_LOGICAL_2_06_PLUS)));
+    is207 = (pcc->pcr_supported & PCR_COMPAT_2_07) &&
+        (lvl == get_compat_level(CPU_POWERPC_LOGICAL_2_07));
+
+    if (is205 || is206 || is207) {
+        if (!max_lvl) {
+            /* User did not set the level, choose the highest */
+            if (*compat_lvl <= lvl) {
+                *compat_lvl = lvl;
+                *compat_pvr = pvr;
+            }
+        } else if (max_lvl >= lvl) {
+            /* User chose the level, don't set higher than this */
+            *compat_lvl = lvl;
+            *compat_pvr = pvr;
+        }
+    }
+}
+
+static uint32_t cas_check_pvr_pre_2_9(PowerPCCPU *cpu, target_ulong list,
+                                      Error **errp)
+{
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+    int counter;
+    unsigned max_lvl = get_compat_level(cpu->max_compat);
+    bool cpu_match = false;
+    unsigned compat_lvl = 0, compat_pvr = 0;
+
+    for (counter = 0; counter < 512; ++counter) {
+        uint32_t pvr, pvr_mask;
+
+        pvr_mask = ldl_be_phys(&address_space_memory, list);
+        pvr = ldl_be_phys(&address_space_memory, list + 4);
+        list += 8;
+
+        if (~pvr_mask & pvr) {
+            break; /* Terminator record */
+        }
+
+        trace_spapr_cas_pvr_try(pvr);
+        if (!max_lvl &&
+            ((cpu->env.spr[SPR_PVR] & pvr_mask) == (pvr & pvr_mask))) {
+            cpu_match = true;
+            compat_pvr = 0;
+        } else if (pvr == cpu->compat_pvr) {
+            cpu_match = true;
+            compat_pvr = cpu->compat_pvr;
+        } else if (!cpu_match) {
+            cas_handle_compat_cpu(pcc, pvr, max_lvl, &compat_lvl, &compat_pvr);
+        }
+    }
+
+    trace_spapr_cas_pvr(cpu->compat_pvr, cpu_match, compat_pvr);
+
+    return compat_pvr;
+}
+
 static uint32_t cas_check_pvr(PowerPCCPU *cpu, target_ulong list,
                               Error **errp)
 {
@@ -1099,6 +1182,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
                                                   target_ulong opcode,
                                                   target_ulong *args)
 {
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
     /* @ov_table points to the first option vector */
     target_ulong ov_table = ppc64_phys_to_real(args[0]);
     uint32_t cas_pvr;
@@ -1106,7 +1190,11 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
     bool guest_radix;
     Error *local_err = NULL;
 
-    cas_pvr = cas_check_pvr(cpu, ov_table, &local_err);
+    if (smc->pre_2_9_cas_pvr) {
+        cas_pvr = cas_check_pvr_pre_2_9(cpu, ov_table, &local_err);
+    } else {
+        cas_pvr = cas_check_pvr(cpu, ov_table, &local_err);
+    }
     if (local_err) {
         error_report_err(local_err);
         return H_HARDWARE;
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 5802f88..0863a41 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -52,6 +52,7 @@ struct sPAPRMachineClass {
     /*< public >*/
     bool dr_lmb_enabled;       /* enable dynamic-reconfig/hotplug of LMBs */
     bool use_ohci_by_default;  /* use USB-OHCI instead of XHCI */
+    bool pre_2_9_cas_pvr;      /* Use old logic for PVR compat negotiation */
     const char *tcg_default_cpu; /* which (TCG) CPU to simulate by default */
     void (*phb_placement)(sPAPRMachineState *spapr, uint32_t index,
                           uint64_t *buid, hwaddr *pio, 
-- 
2.9.4

Re: [Qemu-devel] [RFC 0/2] pseries: Correct behaviour change for older machine types

[Andrea Bolognani]

On Wed, 2017-05-17 at 16:35 +1000, David Gibson wrote:
> 152ef80 "pseries: Rewrite CAS PVR compatibility logic" incorrectly
> introduced a guest-visible behaviour change into existing versioned
> machine type.  Patch 2/2 corrects this change, while 1/2 is a
> preliminary clean up to make that easier.
> 
> Unfortunately, this bug is already in the released qemu-2.9.0.  Once
> this is ready to merge for master, I'll make a backport to apply for
> 2.9.1 as well.
> 
> David Gibson (2):
>   pseries: Split CAS PVR negotiation out into a separate function
>   pseries: Restore PVR negotiation logic for pre-2.9 machine types
> 
>  hw/ppc/spapr.c         |   3 ++
>  hw/ppc/spapr_hcall.c   | 131 +++++++++++++++++++++++++++++++++++++++++++------
>  include/hw/ppc/spapr.h |   1 +
>  3 files changed, 119 insertions(+), 16 deletions(-)

I gave it a spin and I can no longer reproduce the issue,
so for the entire series:

Tested-by: Andrea Bolognani <abologna@redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [Qemu-devel] [RFC 1/2] pseries: Split CAS PVR negotiation out into a separate function

[Laurent Vivier]

On 17/05/2017 08:35, David Gibson wrote:
> Guests of the qemu machine type go through a feature negotiation process
> known as "client architecture support" (CAS) during early boot.  This does
> a number of things, one of which is finding a CPU compatibility mode which
> can be supported by both guest and host.
> 
> In fact the CPU negotiation is probably the single most complex part of the
> CAS process, so this splits it out into a helper function.  We've recently
> made some mistakes in maintaining backward compatibility for old machine
> types here.  Splitting this out will also make it easier to fix this.
> 
> This also adds a possibly useful error message if the negotiation fails
> (i.e. if there is CPU mode that's suitable for both guest and host).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_hcall.c | 43 +++++++++++++++++++++++++++----------------
>  1 file changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 0d608d6..007ae8d 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1047,19 +1047,13 @@ static target_ulong h_signal_sys_reset(PowerPCCPU *cpu,
>      }
>  }
>  
> -static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> -                                                  sPAPRMachineState *spapr,
> -                                                  target_ulong opcode,
> -                                                  target_ulong *args)
> +static uint32_t cas_check_pvr(PowerPCCPU *cpu, target_ulong list,
> +                              Error **errp)
>  {
> -    target_ulong list = ppc64_phys_to_real(args[0]);
> -    target_ulong ov_table;
>      bool explicit_match = false; /* Matched the CPU's real PVR */
>      uint32_t max_compat = cpu->max_compat;
>      uint32_t best_compat = 0;
>      int i;
> -    sPAPROptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
> -    bool guest_radix;
>  
>      /*
>       * We scan the supplied table of PVRs looking for two things
> @@ -1090,26 +1084,43 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>          /* We couldn't find a suitable compatibility mode, and either
>           * the guest doesn't support "raw" mode for this CPU, or raw
>           * mode is disabled because a maximum compat mode is set */
> -        return H_HARDWARE;
> +        error_setg(errp, "Couldn't negotiate a suitable PVR during CAS");
> +        return 0;
>      }
>  
>      /* Parsing finished */
>      trace_spapr_cas_pvr(cpu->compat_pvr, explicit_match, best_compat);
>  
> -    /* Update CPUs */
> -    if (cpu->compat_pvr != best_compat) {
> -        Error *local_err = NULL;
> +    return best_compat;
> +}
> +
> +static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> +                                                  sPAPRMachineState *spapr,
> +                                                  target_ulong opcode,
> +                                                  target_ulong *args)
> +{
> +    /* @ov_table points to the first option vector */
> +    target_ulong ov_table = ppc64_phys_to_real(args[0]);
> +    uint32_t cas_pvr;
> +    sPAPROptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
> +    bool guest_radix;
> +    Error *local_err = NULL;
>  
> -        ppc_set_compat_all(best_compat, &local_err);
> +    cas_pvr = cas_check_pvr(cpu, ov_table, &local_err);
> +    if (local_err) {
> +        error_report_err(local_err);
> +        return H_HARDWARE;
> +    }
> +
> +    /* Update CPUs */
> +    if (cpu->compat_pvr != cas_pvr) {
> +        ppc_set_compat_all(cas_pvr, &local_err);
>          if (local_err) {
>              error_report_err(local_err);
>              return H_HARDWARE;
>          }
>      }
>  
> -    /* For the future use: here @ov_table points to the first option vector */
> -    ov_table = list;
> -

I think there is a change here: now ov_table is always
ppc64_phys_to_real(args[0]), as it is not updated by cas_check() as list
was by h_client_architecture_support().

>      ov1_guest = spapr_ovec_parse_vector(ov_table, 1);
>      ov5_guest = spapr_ovec_parse_vector(ov_table, 5);
>      if (spapr_ovec_test(ov5_guest, OV5_MMU_BOTH)) {

Laurent

Re: [Qemu-devel] [RFC 2/2] pseries: Restore PVR negotiation logic for pre-2.9 machine types

[Laurent Vivier]

On 17/05/2017 08:35, David Gibson wrote:
> "pseries" guests go through a hypervisor<->guest feature negotiation during
> early boot.  Part of this is finding a CPU compatibility mode which works
> for both.
> 
> In 152ef80 "pseries: Rewrite CAS PVR compatibility logic" this logic was
> changed to strongly prefer architecture defined CPU compatibility modes,
> rather than CPU "raw" modes.  However, this change was made universally,
> which introduces a guest visible change for the previously existing machine
> types (pseries-2.8 and earlier).  That's never supposed to happen.
> 
> This corrects the behaviour, reverting to the old PVR negotiation logic
> for machine types prior to pseries-2.9.
> 
> Fixes: 152ef803ceb1959e2380a1da7736b935b109222e
> Reported-by: Andrea Bolognani <abologna@redhat.com>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c         |  3 ++
>  hw/ppc/spapr_hcall.c   | 90 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  include/hw/ppc/spapr.h |  1 +
>  3 files changed, 93 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a9471b9..913355f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3308,9 +3308,12 @@ static void spapr_machine_2_8_instance_options(MachineState *machine)
>  
>  static void spapr_machine_2_8_class_options(MachineClass *mc)
>  {
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> +
>      spapr_machine_2_9_class_options(mc);
>      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_8);
>      mc->numa_mem_align_shift = 23;
> +    smc->pre_2_9_cas_pvr = true;
>  }
>  
>  DEFINE_SPAPR_MACHINE(2_8, "2.8", false);
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 007ae8d..4937019 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1047,6 +1047,89 @@ static target_ulong h_signal_sys_reset(PowerPCCPU *cpu,
>      }
>  }
>  
> +/*
> + * Old logic for PVR negotiation, used old <2.9 machine types for
> + * compatibility with old qemu versions
> + */
> +#define get_compat_level(cpuver) (                  \
> +        ((cpuver) == CPU_POWERPC_LOGICAL_2_05) ? 2050 : \
> +        ((cpuver) == CPU_POWERPC_LOGICAL_2_06) ? 2060 :  \
> +        ((cpuver) == CPU_POWERPC_LOGICAL_2_06_PLUS) ? 2061 :    \
> +        ((cpuver) == CPU_POWERPC_LOGICAL_2_07) ? 2070 : 0)
> +
> +static void cas_handle_compat_cpu(PowerPCCPUClass *pcc, uint32_t pvr,
> +                                  unsigned max_lvl, unsigned *compat_lvl,
> +                                  unsigned *compat_pvr)
> +{
> +    unsigned lvl = get_compat_level(pvr);
> +    bool is205, is206, is207;
> +
> +    if (!lvl) {
> +        return;
> +    }
> +
> +    /* If it is a logical PVR, try to determine the highest level */
> +    is205 = (pcc->pcr_supported & PCR_COMPAT_2_05) &&
> +        (lvl == get_compat_level(CPU_POWERPC_LOGICAL_2_05));
> +    is206 = (pcc->pcr_supported & PCR_COMPAT_2_06) &&
> +        ((lvl == get_compat_level(CPU_POWERPC_LOGICAL_2_06)) ||
> +         (lvl == get_compat_level(CPU_POWERPC_LOGICAL_2_06_PLUS)));
> +    is207 = (pcc->pcr_supported & PCR_COMPAT_2_07) &&
> +        (lvl == get_compat_level(CPU_POWERPC_LOGICAL_2_07));
> +
> +    if (is205 || is206 || is207) {
> +        if (!max_lvl) {
> +            /* User did not set the level, choose the highest */
> +            if (*compat_lvl <= lvl) {
> +                *compat_lvl = lvl;
> +                *compat_pvr = pvr;
> +            }
> +        } else if (max_lvl >= lvl) {
> +            /* User chose the level, don't set higher than this */
> +            *compat_lvl = lvl;
> +            *compat_pvr = pvr;
> +        }
> +    }
> +}
> +
> +static uint32_t cas_check_pvr_pre_2_9(PowerPCCPU *cpu, target_ulong list,
> +                                      Error **errp)
> +{
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +    int counter;
> +    unsigned max_lvl = get_compat_level(cpu->max_compat);
> +    bool cpu_match = false;
> +    unsigned compat_lvl = 0, compat_pvr = 0;
> +
> +    for (counter = 0; counter < 512; ++counter) {
> +        uint32_t pvr, pvr_mask;
> +
> +        pvr_mask = ldl_be_phys(&address_space_memory, list);
> +        pvr = ldl_be_phys(&address_space_memory, list + 4);
> +        list += 8;
> +
> +        if (~pvr_mask & pvr) {
> +            break; /* Terminator record */
> +        }
> +
> +        trace_spapr_cas_pvr_try(pvr);
> +        if (!max_lvl &&
> +            ((cpu->env.spr[SPR_PVR] & pvr_mask) == (pvr & pvr_mask))) {
> +            cpu_match = true;
> +            compat_pvr = 0;
> +        } else if (pvr == cpu->compat_pvr) {
> +            cpu_match = true;
> +            compat_pvr = cpu->compat_pvr;
> +        } else if (!cpu_match) {
> +            cas_handle_compat_cpu(pcc, pvr, max_lvl, &compat_lvl, &compat_pvr);
> +        }
> +    }
> +
> +    trace_spapr_cas_pvr(cpu->compat_pvr, cpu_match, compat_pvr);

Perhaps you can add a new function or a parameter to show which path
(pre_2_9 or not) we use to compute the CAS PVR?

anyway:

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Laurent

Re: [Qemu-devel] [Qemu-ppc] [RFC 1/2] pseries: Split CAS PVR negotiation out into a separate function

[Greg Kurz]

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

On Wed, 17 May 2017 16:35:46 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Guests of the qemu machine type go through a feature negotiation process
> known as "client architecture support" (CAS) during early boot.  This does
> a number of things, one of which is finding a CPU compatibility mode which
> can be supported by both guest and host.
> 
> In fact the CPU negotiation is probably the single most complex part of the
> CAS process, so this splits it out into a helper function.  We've recently
> made some mistakes in maintaining backward compatibility for old machine
> types here.  Splitting this out will also make it easier to fix this.
> 
> This also adds a possibly useful error message if the negotiation fails
> (i.e. if there is CPU mode that's suitable for both guest and host).

s/if there is/if there isn't a/ ?

> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_hcall.c | 43 +++++++++++++++++++++++++++----------------
>  1 file changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 0d608d6..007ae8d 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1047,19 +1047,13 @@ static target_ulong h_signal_sys_reset(PowerPCCPU *cpu,
>      }
>  }
>  
> -static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> -                                                  sPAPRMachineState *spapr,
> -                                                  target_ulong opcode,
> -                                                  target_ulong *args)
> +static uint32_t cas_check_pvr(PowerPCCPU *cpu, target_ulong list,
> +                              Error **errp)
>  {
> -    target_ulong list = ppc64_phys_to_real(args[0]);
> -    target_ulong ov_table;
>      bool explicit_match = false; /* Matched the CPU's real PVR */
>      uint32_t max_compat = cpu->max_compat;
>      uint32_t best_compat = 0;
>      int i;
> -    sPAPROptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
> -    bool guest_radix;
>  
>      /*
>       * We scan the supplied table of PVRs looking for two things
> @@ -1090,26 +1084,43 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>          /* We couldn't find a suitable compatibility mode, and either
>           * the guest doesn't support "raw" mode for this CPU, or raw
>           * mode is disabled because a maximum compat mode is set */
> -        return H_HARDWARE;
> +        error_setg(errp, "Couldn't negotiate a suitable PVR during CAS");
> +        return 0;
>      }
>  
>      /* Parsing finished */
>      trace_spapr_cas_pvr(cpu->compat_pvr, explicit_match, best_compat);
>  
> -    /* Update CPUs */
> -    if (cpu->compat_pvr != best_compat) {
> -        Error *local_err = NULL;
> +    return best_compat;
> +}
> +
> +static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> +                                                  sPAPRMachineState *spapr,
> +                                                  target_ulong opcode,
> +                                                  target_ulong *args)
> +{
> +    /* @ov_table points to the first option vector */
> +    target_ulong ov_table = ppc64_phys_to_real(args[0]);
> +    uint32_t cas_pvr;
> +    sPAPROptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
> +    bool guest_radix;
> +    Error *local_err = NULL;
>  
> -        ppc_set_compat_all(best_compat, &local_err);
> +    cas_pvr = cas_check_pvr(cpu, ov_table, &local_err);

Shouldn't cas_check_pvr() take a target_ulong * so that we can get
the address of the first option vector...

> +    if (local_err) {
> +        error_report_err(local_err);
> +        return H_HARDWARE;
> +    }
> +
> +    /* Update CPUs */
> +    if (cpu->compat_pvr != cas_pvr) {
> +        ppc_set_compat_all(cas_pvr, &local_err);
>          if (local_err) {
>              error_report_err(local_err);
>              return H_HARDWARE;
>          }
>      }
>  
> -    /* For the future use: here @ov_table points to the first option vector */
> -    ov_table = list;
> -

... and address Laurent's comment ?

>      ov1_guest = spapr_ovec_parse_vector(ov_table, 1);
>      ov5_guest = spapr_ovec_parse_vector(ov_table, 5);
>      if (spapr_ovec_test(ov5_guest, OV5_MMU_BOTH)) {


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

Re: [Qemu-devel] [Qemu-ppc] [RFC 2/2] pseries: Restore PVR negotiation logic for pre-2.9 machine types

[Greg Kurz]

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

On Wed, 17 May 2017 16:35:47 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> "pseries" guests go through a hypervisor<->guest feature negotiation during
> early boot.  Part of this is finding a CPU compatibility mode which works
> for both.
> 
> In 152ef80 "pseries: Rewrite CAS PVR compatibility logic" this logic was
> changed to strongly prefer architecture defined CPU compatibility modes,
> rather than CPU "raw" modes.  However, this change was made universally,
> which introduces a guest visible change for the previously existing machine
> types (pseries-2.8 and earlier).  That's never supposed to happen.
> 
> This corrects the behaviour, reverting to the old PVR negotiation logic
> for machine types prior to pseries-2.9.
> 
> Fixes: 152ef803ceb1959e2380a1da7736b935b109222e
> Reported-by: Andrea Bolognani <abologna@redhat.com>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr.c         |  3 ++
>  hw/ppc/spapr_hcall.c   | 90 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  include/hw/ppc/spapr.h |  1 +
>  3 files changed, 93 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a9471b9..913355f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3308,9 +3308,12 @@ static void spapr_machine_2_8_instance_options(MachineState *machine)
>  
>  static void spapr_machine_2_8_class_options(MachineClass *mc)
>  {
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
> +
>      spapr_machine_2_9_class_options(mc);
>      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_8);
>      mc->numa_mem_align_shift = 23;
> +    smc->pre_2_9_cas_pvr = true;
>  }
>  
>  DEFINE_SPAPR_MACHINE(2_8, "2.8", false);
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 007ae8d..4937019 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1047,6 +1047,89 @@ static target_ulong h_signal_sys_reset(PowerPCCPU *cpu,
>      }
>  }
>  
> +/*
> + * Old logic for PVR negotiation, used old <2.9 machine types for

s/used old/used by old/

> + * compatibility with old qemu versions
> + */
> +#define get_compat_level(cpuver) (                  \
> +        ((cpuver) == CPU_POWERPC_LOGICAL_2_05) ? 2050 : \
> +        ((cpuver) == CPU_POWERPC_LOGICAL_2_06) ? 2060 :  \
> +        ((cpuver) == CPU_POWERPC_LOGICAL_2_06_PLUS) ? 2061 :    \
> +        ((cpuver) == CPU_POWERPC_LOGICAL_2_07) ? 2070 : 0)
> +
> +static void cas_handle_compat_cpu(PowerPCCPUClass *pcc, uint32_t pvr,
> +                                  unsigned max_lvl, unsigned *compat_lvl,
> +                                  unsigned *compat_pvr)
> +{
> +    unsigned lvl = get_compat_level(pvr);
> +    bool is205, is206, is207;
> +
> +    if (!lvl) {
> +        return;
> +    }
> +
> +    /* If it is a logical PVR, try to determine the highest level */
> +    is205 = (pcc->pcr_supported & PCR_COMPAT_2_05) &&
> +        (lvl == get_compat_level(CPU_POWERPC_LOGICAL_2_05));
> +    is206 = (pcc->pcr_supported & PCR_COMPAT_2_06) &&
> +        ((lvl == get_compat_level(CPU_POWERPC_LOGICAL_2_06)) ||
> +         (lvl == get_compat_level(CPU_POWERPC_LOGICAL_2_06_PLUS)));
> +    is207 = (pcc->pcr_supported & PCR_COMPAT_2_07) &&
> +        (lvl == get_compat_level(CPU_POWERPC_LOGICAL_2_07));
> +
> +    if (is205 || is206 || is207) {
> +        if (!max_lvl) {
> +            /* User did not set the level, choose the highest */
> +            if (*compat_lvl <= lvl) {
> +                *compat_lvl = lvl;
> +                *compat_pvr = pvr;
> +            }
> +        } else if (max_lvl >= lvl) {
> +            /* User chose the level, don't set higher than this */
> +            *compat_lvl = lvl;
> +            *compat_pvr = pvr;
> +        }
> +    }
> +}
> +
> +static uint32_t cas_check_pvr_pre_2_9(PowerPCCPU *cpu, target_ulong list,

Same remark as for cas_check_pvr() in patch 1.

> +                                      Error **errp)
> +{
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +    int counter;
> +    unsigned max_lvl = get_compat_level(cpu->max_compat);
> +    bool cpu_match = false;
> +    unsigned compat_lvl = 0, compat_pvr = 0;
> +
> +    for (counter = 0; counter < 512; ++counter) {
> +        uint32_t pvr, pvr_mask;
> +
> +        pvr_mask = ldl_be_phys(&address_space_memory, list);
> +        pvr = ldl_be_phys(&address_space_memory, list + 4);
> +        list += 8;
> +
> +        if (~pvr_mask & pvr) {
> +            break; /* Terminator record */
> +        }
> +
> +        trace_spapr_cas_pvr_try(pvr);
> +        if (!max_lvl &&
> +            ((cpu->env.spr[SPR_PVR] & pvr_mask) == (pvr & pvr_mask))) {
> +            cpu_match = true;
> +            compat_pvr = 0;
> +        } else if (pvr == cpu->compat_pvr) {
> +            cpu_match = true;
> +            compat_pvr = cpu->compat_pvr;
> +        } else if (!cpu_match) {
> +            cas_handle_compat_cpu(pcc, pvr, max_lvl, &compat_lvl, &compat_pvr);
> +        }
> +    }
> +
> +    trace_spapr_cas_pvr(cpu->compat_pvr, cpu_match, compat_pvr);
> +
> +    return compat_pvr;
> +}
> +
>  static uint32_t cas_check_pvr(PowerPCCPU *cpu, target_ulong list,
>                                Error **errp)
>  {
> @@ -1099,6 +1182,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>                                                    target_ulong opcode,
>                                                    target_ulong *args)
>  {
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>      /* @ov_table points to the first option vector */
>      target_ulong ov_table = ppc64_phys_to_real(args[0]);
>      uint32_t cas_pvr;
> @@ -1106,7 +1190,11 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>      bool guest_radix;
>      Error *local_err = NULL;
>  
> -    cas_pvr = cas_check_pvr(cpu, ov_table, &local_err);
> +    if (smc->pre_2_9_cas_pvr) {
> +        cas_pvr = cas_check_pvr_pre_2_9(cpu, ov_table, &local_err);
> +    } else {
> +        cas_pvr = cas_check_pvr(cpu, ov_table, &local_err);
> +    }
>      if (local_err) {
>          error_report_err(local_err);
>          return H_HARDWARE;
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 5802f88..0863a41 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -52,6 +52,7 @@ struct sPAPRMachineClass {
>      /*< public >*/
>      bool dr_lmb_enabled;       /* enable dynamic-reconfig/hotplug of LMBs */
>      bool use_ohci_by_default;  /* use USB-OHCI instead of XHCI */
> +    bool pre_2_9_cas_pvr;      /* Use old logic for PVR compat negotiation */
>      const char *tcg_default_cpu; /* which (TCG) CPU to simulate by default */
>      void (*phb_placement)(sPAPRMachineState *spapr, uint32_t index,
>                            uint64_t *buid, hwaddr *pio, 


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

Re: [Qemu-devel] [Qemu-ppc] [RFC 1/2] pseries: Split CAS PVR negotiation out into a separate function

[David Gibson]

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

On Wed, May 17, 2017 at 05:56:57PM +0200, Greg Kurz wrote:
> On Wed, 17 May 2017 16:35:46 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Guests of the qemu machine type go through a feature negotiation process
> > known as "client architecture support" (CAS) during early boot.  This does
> > a number of things, one of which is finding a CPU compatibility mode which
> > can be supported by both guest and host.
> > 
> > In fact the CPU negotiation is probably the single most complex part of the
> > CAS process, so this splits it out into a helper function.  We've recently
> > made some mistakes in maintaining backward compatibility for old machine
> > types here.  Splitting this out will also make it easier to fix this.
> > 
> > This also adds a possibly useful error message if the negotiation fails
> > (i.e. if there is CPU mode that's suitable for both guest and host).
> 
> s/if there is/if there isn't a/ ?

Oops, I'll correct that.

> 
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_hcall.c | 43 +++++++++++++++++++++++++++----------------
> >  1 file changed, 27 insertions(+), 16 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index 0d608d6..007ae8d 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -1047,19 +1047,13 @@ static target_ulong h_signal_sys_reset(PowerPCCPU *cpu,
> >      }
> >  }
> >  
> > -static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> > -                                                  sPAPRMachineState *spapr,
> > -                                                  target_ulong opcode,
> > -                                                  target_ulong *args)
> > +static uint32_t cas_check_pvr(PowerPCCPU *cpu, target_ulong list,
> > +                              Error **errp)
> >  {
> > -    target_ulong list = ppc64_phys_to_real(args[0]);
> > -    target_ulong ov_table;
> >      bool explicit_match = false; /* Matched the CPU's real PVR */
> >      uint32_t max_compat = cpu->max_compat;
> >      uint32_t best_compat = 0;
> >      int i;
> > -    sPAPROptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
> > -    bool guest_radix;
> >  
> >      /*
> >       * We scan the supplied table of PVRs looking for two things
> > @@ -1090,26 +1084,43 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> >          /* We couldn't find a suitable compatibility mode, and either
> >           * the guest doesn't support "raw" mode for this CPU, or raw
> >           * mode is disabled because a maximum compat mode is set */
> > -        return H_HARDWARE;
> > +        error_setg(errp, "Couldn't negotiate a suitable PVR during CAS");
> > +        return 0;
> >      }
> >  
> >      /* Parsing finished */
> >      trace_spapr_cas_pvr(cpu->compat_pvr, explicit_match, best_compat);
> >  
> > -    /* Update CPUs */
> > -    if (cpu->compat_pvr != best_compat) {
> > -        Error *local_err = NULL;
> > +    return best_compat;
> > +}
> > +
> > +static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> > +                                                  sPAPRMachineState *spapr,
> > +                                                  target_ulong opcode,
> > +                                                  target_ulong *args)
> > +{
> > +    /* @ov_table points to the first option vector */
> > +    target_ulong ov_table = ppc64_phys_to_real(args[0]);
> > +    uint32_t cas_pvr;
> > +    sPAPROptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
> > +    bool guest_radix;
> > +    Error *local_err = NULL;
> >  
> > -        ppc_set_compat_all(best_compat, &local_err);
> > +    cas_pvr = cas_check_pvr(cpu, ov_table, &local_err);
> 
> Shouldn't cas_check_pvr() take a target_ulong * so that we can get
> the address of the first option vector...
> 
> > +    if (local_err) {
> > +        error_report_err(local_err);
> > +        return H_HARDWARE;
> > +    }
> > +
> > +    /* Update CPUs */
> > +    if (cpu->compat_pvr != cas_pvr) {
> > +        ppc_set_compat_all(cas_pvr, &local_err);
> >          if (local_err) {
> >              error_report_err(local_err);
> >              return H_HARDWARE;
> >          }
> >      }
> >  
> > -    /* For the future use: here @ov_table points to the first option vector */
> > -    ov_table = list;
> > -
> 
> ... and address Laurent's comment ?

Yes, I completely misread what was happening with the pvr_list vs. the
option vectors.  I'll correct that.

> 
> >      ov1_guest = spapr_ovec_parse_vector(ov_table, 1);
> >      ov5_guest = spapr_ovec_parse_vector(ov_table, 5);
> >      if (spapr_ovec_test(ov5_guest, OV5_MMU_BOTH)) {
> 



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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC 0/2] pseries: Correct behaviour change for older machine types

[David Gibson]

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

On Wed, May 17, 2017 at 10:52:05AM +0200, Andrea Bolognani wrote:
> On Wed, 2017-05-17 at 16:35 +1000, David Gibson wrote:
> > 152ef80 "pseries: Rewrite CAS PVR compatibility logic" incorrectly
> > introduced a guest-visible behaviour change into existing versioned
> > machine type.  Patch 2/2 corrects this change, while 1/2 is a
> > preliminary clean up to make that easier.
> > 
> > Unfortunately, this bug is already in the released qemu-2.9.0.  Once
> > this is ready to merge for master, I'll make a backport to apply for
> > 2.9.1 as well.
> > 
> > David Gibson (2):
> >   pseries: Split CAS PVR negotiation out into a separate function
> >   pseries: Restore PVR negotiation logic for pre-2.9 machine types
> > 
> >  hw/ppc/spapr.c         |   3 ++
> >  hw/ppc/spapr_hcall.c   | 131 +++++++++++++++++++++++++++++++++++++++++++------
> >  include/hw/ppc/spapr.h |   1 +
> >  3 files changed, 119 insertions(+), 16 deletions(-)
> 
> I gave it a spin and I can no longer reproduce the issue,
> so for the entire series:
> 
> Tested-by: Andrea Bolognani <abologna@redhat.com>

I'm actually kind of astonished it worked at all given the bug that
Laurent pointed out.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC 0/2] pseries: Correct behaviour change for older machine types

[Andrea Bolognani]

On Thu, 2017-05-18 at 14:51 +1000, David Gibson wrote:
> > I gave it a spin and I can no longer reproduce the issue,
> > so for the entire series:
> > 
> > Tested-by: Andrea Bolognani <abologna@redhat.com>
> 
> I'm actually kind of astonished it worked at all given the bug that
> Laurent pointed out.

¯\_(ツ)_/¯

-- 
Andrea Bolognani / Red Hat / Virtualization