All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/3] pseries: Reverse behaviour change for older machine types
@ 2017-05-18  5:45 David Gibson
  2017-05-18  5:45 ` [Qemu-devel] [PATCH 1/3] pseries: Split CAS PVR negotiation out into a separate function David Gibson
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: David Gibson @ 2017-05-18  5:45 UTC (permalink / raw)
  To: abologna, thuth, lvivier
  Cc: mdroth, groug, qemu-ppc, qemu-devel, 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.

Changes since RFC:
 * Corrected serious bug in vector address calculation which would
   have broken all the rest of the CAS process
 * Added a third patch with tracing cleanup
 * Correct error in commit message

David Gibson (3):
  pseries: Split CAS PVR negotiation out into a separate function
  pseries: Restore PVR negotiation logic for pre-2.9 machine types
  pseries: Improve tracing of CPU compatibility negotiation

 hw/ppc/spapr.c         |   3 ++
 hw/ppc/spapr_hcall.c   | 141 ++++++++++++++++++++++++++++++++++++++++++-------
 hw/ppc/trace-events    |   6 ++-
 include/hw/ppc/spapr.h |   1 +
 4 files changed, 131 insertions(+), 20 deletions(-)

-- 
2.9.4

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 1/3] pseries: Split CAS PVR negotiation out into a separate function
  2017-05-18  5:45 [Qemu-devel] [PATCH 0/3] pseries: Reverse behaviour change for older machine types David Gibson
@ 2017-05-18  5:45 ` David Gibson
  2017-05-18  7:46   ` Laurent Vivier
  2017-05-18  7:59   ` Greg Kurz
  2017-05-18  5:45 ` [Qemu-devel] [PATCH 2/3] pseries: Restore PVR negotiation logic for pre-2.9 machine types David Gibson
  2017-05-18  5:45 ` [Qemu-devel] [PATCH 3/3] pseries: Improve tracing of CPU compatibility negotiation David Gibson
  2 siblings, 2 replies; 9+ messages in thread
From: David Gibson @ 2017-05-18  5:45 UTC (permalink / raw)
  To: abologna, thuth, lvivier
  Cc: mdroth, groug, qemu-ppc, qemu-devel, 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 isn't a 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 | 49 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 2daace4..77d2d66 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1044,19 +1044,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 *addr,
+                              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
@@ -1066,9 +1060,9 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
     for (i = 0; i < 512; ++i) {
         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;
+        pvr_mask = ldl_be_phys(&address_space_memory, *addr);
+        pvr = ldl_be_phys(&address_space_memory, *addr + 4);
+        *addr += 8;
 
         if (~pvr_mask & pvr) {
             break; /* Terminator record */
@@ -1087,17 +1081,38 @@ 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;
+}
 
-        ppc_set_compat_all(best_compat, &local_err);
+static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
+                                                  sPAPRMachineState *spapr,
+                                                  target_ulong opcode,
+                                                  target_ulong *args)
+{
+    /* Working address in data buffer */
+    target_ulong addr = ppc64_phys_to_real(args[0]);
+    target_ulong ov_table;
+    uint32_t cas_pvr;
+    sPAPROptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
+    bool guest_radix;
+    Error *local_err = NULL;
+
+    cas_pvr = cas_check_pvr(cpu, &addr, &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;
@@ -1105,7 +1120,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
     }
 
     /* For the future use: here @ov_table points to the first option vector */
-    ov_table = list;
+    ov_table = addr;
 
     ov1_guest = spapr_ovec_parse_vector(ov_table, 1);
     ov5_guest = spapr_ovec_parse_vector(ov_table, 5);
-- 
2.9.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 2/3] pseries: Restore PVR negotiation logic for pre-2.9 machine types
  2017-05-18  5:45 [Qemu-devel] [PATCH 0/3] pseries: Reverse behaviour change for older machine types David Gibson
  2017-05-18  5:45 ` [Qemu-devel] [PATCH 1/3] pseries: Split CAS PVR negotiation out into a separate function David Gibson
@ 2017-05-18  5:45 ` David Gibson
  2017-05-18  8:01   ` Greg Kurz
  2017-05-18  5:45 ` [Qemu-devel] [PATCH 3/3] pseries: Improve tracing of CPU compatibility negotiation David Gibson
  2 siblings, 1 reply; 9+ messages in thread
From: David Gibson @ 2017-05-18  5:45 UTC (permalink / raw)
  To: abologna, thuth, lvivier
  Cc: mdroth, groug, qemu-ppc, qemu-devel, 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>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
---
 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 35dceb0..ad8ddf2 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3313,9 +3313,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 77d2d66..a790da7 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1044,6 +1044,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 *addr,
+                                      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, *addr);
+        pvr = ldl_be_phys(&address_space_memory, *addr + 4);
+        *addr += 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 *addr,
                               Error **errp)
 {
@@ -1096,6 +1179,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
                                                   target_ulong opcode,
                                                   target_ulong *args)
 {
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
     /* Working address in data buffer */
     target_ulong addr = ppc64_phys_to_real(args[0]);
     target_ulong ov_table;
@@ -1104,7 +1188,11 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
     bool guest_radix;
     Error *local_err = NULL;
 
-    cas_pvr = cas_check_pvr(cpu, &addr, &local_err);
+    if (smc->pre_2_9_cas_pvr) {
+        cas_pvr = cas_check_pvr_pre_2_9(cpu, &addr, &local_err);
+    } else {
+        cas_pvr = cas_check_pvr(cpu, &addr, &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 93c4cfc..f875dc4 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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 3/3] pseries: Improve tracing of CPU compatibility negotiation
  2017-05-18  5:45 [Qemu-devel] [PATCH 0/3] pseries: Reverse behaviour change for older machine types David Gibson
  2017-05-18  5:45 ` [Qemu-devel] [PATCH 1/3] pseries: Split CAS PVR negotiation out into a separate function David Gibson
  2017-05-18  5:45 ` [Qemu-devel] [PATCH 2/3] pseries: Restore PVR negotiation logic for pre-2.9 machine types David Gibson
@ 2017-05-18  5:45 ` David Gibson
  2017-05-18  7:47   ` Laurent Vivier
  2017-05-18  8:07   ` Greg Kurz
  2 siblings, 2 replies; 9+ messages in thread
From: David Gibson @ 2017-05-18  5:45 UTC (permalink / raw)
  To: abologna, thuth, lvivier
  Cc: mdroth, groug, qemu-ppc, qemu-devel, David Gibson

This makes some improvements to the debug tracepoints around the
negotiation of CPU compatibility mode during CAS.  The traces are
reorganized to emphasise what the inputs and outputs of the process are,
then distinguish the internal state of the two possible negotiation paths
(current and pre-2.8 machine type compatibility).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_hcall.c | 14 ++++++++------
 hw/ppc/trace-events  |  6 ++++--
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index a790da7..cea5d99 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1109,7 +1109,6 @@ static uint32_t cas_check_pvr_pre_2_9(PowerPCCPU *cpu, target_ulong *addr,
             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;
@@ -1120,10 +1119,10 @@ static uint32_t cas_check_pvr_pre_2_9(PowerPCCPU *cpu, target_ulong *addr,
         } else if (!cpu_match) {
             cas_handle_compat_cpu(pcc, pvr, max_lvl, &compat_lvl, &compat_pvr);
         }
+        trace_cas_check_pvr_pre_2_9(pvr, pvr_mask, cpu_match,
+                                    compat_lvl, compat_pvr);
     }
 
-    trace_spapr_cas_pvr(cpu->compat_pvr, cpu_match, compat_pvr);
-
     return compat_pvr;
 }
 
@@ -1158,6 +1157,7 @@ static uint32_t cas_check_pvr(PowerPCCPU *cpu, target_ulong *addr,
                 best_compat = pvr;
             }
         }
+        trace_cas_check_pvr(pvr, pvr_mask, explicit_match, best_compat);
     }
 
     if ((best_compat == 0) && (!explicit_match || max_compat)) {
@@ -1168,9 +1168,6 @@ static uint32_t cas_check_pvr(PowerPCCPU *cpu, target_ulong *addr,
         return 0;
     }
 
-    /* Parsing finished */
-    trace_spapr_cas_pvr(cpu->compat_pvr, explicit_match, best_compat);
-
     return best_compat;
 }
 
@@ -1188,6 +1185,9 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
     bool guest_radix;
     Error *local_err = NULL;
 
+    trace_cas_check_pvr_start(cpu->compat_pvr, cpu->max_compat,
+                              cpu->env.spr[SPR_PVR]);
+
     if (smc->pre_2_9_cas_pvr) {
         cas_pvr = cas_check_pvr_pre_2_9(cpu, &addr, &local_err);
     } else {
@@ -1198,6 +1198,8 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
         return H_HARDWARE;
     }
 
+    trace_cas_check_pvr_complete(cpu->compat_pvr, cas_pvr);
+
     /* Update CPUs */
     if (cpu->compat_pvr != cas_pvr) {
         ppc_set_compat_all(cas_pvr, &local_err);
diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
index 43d265f..5329740 100644
--- a/hw/ppc/trace-events
+++ b/hw/ppc/trace-events
@@ -14,8 +14,10 @@ spapr_cas_failed(unsigned long n) "DT diff buffer is too small: %ld bytes"
 spapr_cas_continue(unsigned long n) "Copy changes to the guest: %ld bytes"
 
 # hw/ppc/spapr_hcall.c
-spapr_cas_pvr_try(uint32_t pvr) "%x"
-spapr_cas_pvr(uint32_t cur_pvr, bool explicit_match, uint32_t new_pvr) "current=%x, explicit_match=%u, new=%x"
+cas_check_pvr_pre_2_9(uint32_t pvr, uint32_t mask, bool match, unsigned lvl, uint32_t compat_pvr) "0x%08x/0x%08x match=%d lvl=%d compat_pvr=0x%x"
+cas_check_pvr(uint32_t pvr, uint32_t pvr_mask, bool explicit_match, uint32_t best_compat) "0x%08x/0x%08x explicit_match=%d best_compat=0x%08x"
+cas_check_pvr_start(uint32_t compat_pvr, uint32_t max_compat, uint32_t phys_pvr) "Initial compat PVR 0x%08x, max compat 0x%08x (real PVR 0x%08x)"
+cas_check_pvr_complete(uint32_t old_pvr, uint32_t new_pvr) "Compatibility PVR was 0x%08x, now 0x%08x"
 
 # hw/ppc/spapr_iommu.c
 spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64
-- 
2.9.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 1/3] pseries: Split CAS PVR negotiation out into a separate function
  2017-05-18  5:45 ` [Qemu-devel] [PATCH 1/3] pseries: Split CAS PVR negotiation out into a separate function David Gibson
@ 2017-05-18  7:46   ` Laurent Vivier
  2017-05-18  7:59   ` Greg Kurz
  1 sibling, 0 replies; 9+ messages in thread
From: Laurent Vivier @ 2017-05-18  7:46 UTC (permalink / raw)
  To: David Gibson, abologna, thuth; +Cc: mdroth, groug, qemu-ppc, qemu-devel

On 18/05/2017 07:45, 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 isn't a CPU mode that's suitable for both guest and host).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

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

> ---
>  hw/ppc/spapr_hcall.c | 49 ++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 32 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 2daace4..77d2d66 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1044,19 +1044,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 *addr,
> +                              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
> @@ -1066,9 +1060,9 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>      for (i = 0; i < 512; ++i) {
>          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;
> +        pvr_mask = ldl_be_phys(&address_space_memory, *addr);
> +        pvr = ldl_be_phys(&address_space_memory, *addr + 4);
> +        *addr += 8;
>  
>          if (~pvr_mask & pvr) {
>              break; /* Terminator record */
> @@ -1087,17 +1081,38 @@ 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;
> +}
>  
> -        ppc_set_compat_all(best_compat, &local_err);
> +static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> +                                                  sPAPRMachineState *spapr,
> +                                                  target_ulong opcode,
> +                                                  target_ulong *args)
> +{
> +    /* Working address in data buffer */
> +    target_ulong addr = ppc64_phys_to_real(args[0]);
> +    target_ulong ov_table;
> +    uint32_t cas_pvr;
> +    sPAPROptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
> +    bool guest_radix;
> +    Error *local_err = NULL;
> +
> +    cas_pvr = cas_check_pvr(cpu, &addr, &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;
> @@ -1105,7 +1120,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>      }
>  
>      /* For the future use: here @ov_table points to the first option vector */
> -    ov_table = list;
> +    ov_table = addr;
>  
>      ov1_guest = spapr_ovec_parse_vector(ov_table, 1);
>      ov5_guest = spapr_ovec_parse_vector(ov_table, 5);
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] pseries: Improve tracing of CPU compatibility negotiation
  2017-05-18  5:45 ` [Qemu-devel] [PATCH 3/3] pseries: Improve tracing of CPU compatibility negotiation David Gibson
@ 2017-05-18  7:47   ` Laurent Vivier
  2017-05-18  8:07   ` Greg Kurz
  1 sibling, 0 replies; 9+ messages in thread
From: Laurent Vivier @ 2017-05-18  7:47 UTC (permalink / raw)
  To: David Gibson, abologna, thuth; +Cc: mdroth, groug, qemu-ppc, qemu-devel

On 18/05/2017 07:45, David Gibson wrote:
> This makes some improvements to the debug tracepoints around the
> negotiation of CPU compatibility mode during CAS.  The traces are
> reorganized to emphasise what the inputs and outputs of the process are,
> then distinguish the internal state of the two possible negotiation paths
> (current and pre-2.8 machine type compatibility).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

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

> ---
>  hw/ppc/spapr_hcall.c | 14 ++++++++------
>  hw/ppc/trace-events  |  6 ++++--
>  2 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index a790da7..cea5d99 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1109,7 +1109,6 @@ static uint32_t cas_check_pvr_pre_2_9(PowerPCCPU *cpu, target_ulong *addr,
>              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;
> @@ -1120,10 +1119,10 @@ static uint32_t cas_check_pvr_pre_2_9(PowerPCCPU *cpu, target_ulong *addr,
>          } else if (!cpu_match) {
>              cas_handle_compat_cpu(pcc, pvr, max_lvl, &compat_lvl, &compat_pvr);
>          }
> +        trace_cas_check_pvr_pre_2_9(pvr, pvr_mask, cpu_match,
> +                                    compat_lvl, compat_pvr);
>      }
>  
> -    trace_spapr_cas_pvr(cpu->compat_pvr, cpu_match, compat_pvr);
> -
>      return compat_pvr;
>  }
>  
> @@ -1158,6 +1157,7 @@ static uint32_t cas_check_pvr(PowerPCCPU *cpu, target_ulong *addr,
>                  best_compat = pvr;
>              }
>          }
> +        trace_cas_check_pvr(pvr, pvr_mask, explicit_match, best_compat);
>      }
>  
>      if ((best_compat == 0) && (!explicit_match || max_compat)) {
> @@ -1168,9 +1168,6 @@ static uint32_t cas_check_pvr(PowerPCCPU *cpu, target_ulong *addr,
>          return 0;
>      }
>  
> -    /* Parsing finished */
> -    trace_spapr_cas_pvr(cpu->compat_pvr, explicit_match, best_compat);
> -
>      return best_compat;
>  }
>  
> @@ -1188,6 +1185,9 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>      bool guest_radix;
>      Error *local_err = NULL;
>  
> +    trace_cas_check_pvr_start(cpu->compat_pvr, cpu->max_compat,
> +                              cpu->env.spr[SPR_PVR]);
> +
>      if (smc->pre_2_9_cas_pvr) {
>          cas_pvr = cas_check_pvr_pre_2_9(cpu, &addr, &local_err);
>      } else {
> @@ -1198,6 +1198,8 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>          return H_HARDWARE;
>      }
>  
> +    trace_cas_check_pvr_complete(cpu->compat_pvr, cas_pvr);
> +
>      /* Update CPUs */
>      if (cpu->compat_pvr != cas_pvr) {
>          ppc_set_compat_all(cas_pvr, &local_err);
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index 43d265f..5329740 100644
> --- a/hw/ppc/trace-events
> +++ b/hw/ppc/trace-events
> @@ -14,8 +14,10 @@ spapr_cas_failed(unsigned long n) "DT diff buffer is too small: %ld bytes"
>  spapr_cas_continue(unsigned long n) "Copy changes to the guest: %ld bytes"
>  
>  # hw/ppc/spapr_hcall.c
> -spapr_cas_pvr_try(uint32_t pvr) "%x"
> -spapr_cas_pvr(uint32_t cur_pvr, bool explicit_match, uint32_t new_pvr) "current=%x, explicit_match=%u, new=%x"
> +cas_check_pvr_pre_2_9(uint32_t pvr, uint32_t mask, bool match, unsigned lvl, uint32_t compat_pvr) "0x%08x/0x%08x match=%d lvl=%d compat_pvr=0x%x"
> +cas_check_pvr(uint32_t pvr, uint32_t pvr_mask, bool explicit_match, uint32_t best_compat) "0x%08x/0x%08x explicit_match=%d best_compat=0x%08x"
> +cas_check_pvr_start(uint32_t compat_pvr, uint32_t max_compat, uint32_t phys_pvr) "Initial compat PVR 0x%08x, max compat 0x%08x (real PVR 0x%08x)"
> +cas_check_pvr_complete(uint32_t old_pvr, uint32_t new_pvr) "Compatibility PVR was 0x%08x, now 0x%08x"
>  
>  # hw/ppc/spapr_iommu.c
>  spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 1/3] pseries: Split CAS PVR negotiation out into a separate function
  2017-05-18  5:45 ` [Qemu-devel] [PATCH 1/3] pseries: Split CAS PVR negotiation out into a separate function David Gibson
  2017-05-18  7:46   ` Laurent Vivier
@ 2017-05-18  7:59   ` Greg Kurz
  1 sibling, 0 replies; 9+ messages in thread
From: Greg Kurz @ 2017-05-18  7:59 UTC (permalink / raw)
  To: David Gibson; +Cc: abologna, thuth, lvivier, mdroth, qemu-ppc, qemu-devel

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

On Thu, 18 May 2017 15:45:20 +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 isn't a CPU mode that's suitable for both guest and host).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

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

>  hw/ppc/spapr_hcall.c | 49 ++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 32 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 2daace4..77d2d66 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1044,19 +1044,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 *addr,
> +                              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
> @@ -1066,9 +1060,9 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>      for (i = 0; i < 512; ++i) {
>          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;
> +        pvr_mask = ldl_be_phys(&address_space_memory, *addr);
> +        pvr = ldl_be_phys(&address_space_memory, *addr + 4);
> +        *addr += 8;
>  
>          if (~pvr_mask & pvr) {
>              break; /* Terminator record */
> @@ -1087,17 +1081,38 @@ 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;
> +}
>  
> -        ppc_set_compat_all(best_compat, &local_err);
> +static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
> +                                                  sPAPRMachineState *spapr,
> +                                                  target_ulong opcode,
> +                                                  target_ulong *args)
> +{
> +    /* Working address in data buffer */
> +    target_ulong addr = ppc64_phys_to_real(args[0]);
> +    target_ulong ov_table;
> +    uint32_t cas_pvr;
> +    sPAPROptionVector *ov1_guest, *ov5_guest, *ov5_cas_old, *ov5_updates;
> +    bool guest_radix;
> +    Error *local_err = NULL;
> +
> +    cas_pvr = cas_check_pvr(cpu, &addr, &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;
> @@ -1105,7 +1120,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>      }
>  
>      /* For the future use: here @ov_table points to the first option vector */
> -    ov_table = list;
> +    ov_table = addr;
>  
>      ov1_guest = spapr_ovec_parse_vector(ov_table, 1);
>      ov5_guest = spapr_ovec_parse_vector(ov_table, 5);


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 2/3] pseries: Restore PVR negotiation logic for pre-2.9 machine types
  2017-05-18  5:45 ` [Qemu-devel] [PATCH 2/3] pseries: Restore PVR negotiation logic for pre-2.9 machine types David Gibson
@ 2017-05-18  8:01   ` Greg Kurz
  0 siblings, 0 replies; 9+ messages in thread
From: Greg Kurz @ 2017-05-18  8:01 UTC (permalink / raw)
  To: David Gibson; +Cc: abologna, thuth, lvivier, mdroth, qemu-ppc, qemu-devel

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

On Thu, 18 May 2017 15:45:21 +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>
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> ---

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

>  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 35dceb0..ad8ddf2 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3313,9 +3313,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 77d2d66..a790da7 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1044,6 +1044,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 *addr,
> +                                      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, *addr);
> +        pvr = ldl_be_phys(&address_space_memory, *addr + 4);
> +        *addr += 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 *addr,
>                                Error **errp)
>  {
> @@ -1096,6 +1179,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>                                                    target_ulong opcode,
>                                                    target_ulong *args)
>  {
> +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>      /* Working address in data buffer */
>      target_ulong addr = ppc64_phys_to_real(args[0]);
>      target_ulong ov_table;
> @@ -1104,7 +1188,11 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>      bool guest_radix;
>      Error *local_err = NULL;
>  
> -    cas_pvr = cas_check_pvr(cpu, &addr, &local_err);
> +    if (smc->pre_2_9_cas_pvr) {
> +        cas_pvr = cas_check_pvr_pre_2_9(cpu, &addr, &local_err);
> +    } else {
> +        cas_pvr = cas_check_pvr(cpu, &addr, &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 93c4cfc..f875dc4 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 --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] pseries: Improve tracing of CPU compatibility negotiation
  2017-05-18  5:45 ` [Qemu-devel] [PATCH 3/3] pseries: Improve tracing of CPU compatibility negotiation David Gibson
  2017-05-18  7:47   ` Laurent Vivier
@ 2017-05-18  8:07   ` Greg Kurz
  1 sibling, 0 replies; 9+ messages in thread
From: Greg Kurz @ 2017-05-18  8:07 UTC (permalink / raw)
  To: David Gibson; +Cc: abologna, thuth, lvivier, mdroth, qemu-ppc, qemu-devel

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

On Thu, 18 May 2017 15:45:22 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> This makes some improvements to the debug tracepoints around the
> negotiation of CPU compatibility mode during CAS.  The traces are
> reorganized to emphasise what the inputs and outputs of the process are,
> then distinguish the internal state of the two possible negotiation paths
> (current and pre-2.8 machine type compatibility).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

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

>  hw/ppc/spapr_hcall.c | 14 ++++++++------
>  hw/ppc/trace-events  |  6 ++++--
>  2 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index a790da7..cea5d99 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1109,7 +1109,6 @@ static uint32_t cas_check_pvr_pre_2_9(PowerPCCPU *cpu, target_ulong *addr,
>              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;
> @@ -1120,10 +1119,10 @@ static uint32_t cas_check_pvr_pre_2_9(PowerPCCPU *cpu, target_ulong *addr,
>          } else if (!cpu_match) {
>              cas_handle_compat_cpu(pcc, pvr, max_lvl, &compat_lvl, &compat_pvr);
>          }
> +        trace_cas_check_pvr_pre_2_9(pvr, pvr_mask, cpu_match,
> +                                    compat_lvl, compat_pvr);
>      }
>  
> -    trace_spapr_cas_pvr(cpu->compat_pvr, cpu_match, compat_pvr);
> -
>      return compat_pvr;
>  }
>  
> @@ -1158,6 +1157,7 @@ static uint32_t cas_check_pvr(PowerPCCPU *cpu, target_ulong *addr,
>                  best_compat = pvr;
>              }
>          }
> +        trace_cas_check_pvr(pvr, pvr_mask, explicit_match, best_compat);
>      }
>  
>      if ((best_compat == 0) && (!explicit_match || max_compat)) {
> @@ -1168,9 +1168,6 @@ static uint32_t cas_check_pvr(PowerPCCPU *cpu, target_ulong *addr,
>          return 0;
>      }
>  
> -    /* Parsing finished */
> -    trace_spapr_cas_pvr(cpu->compat_pvr, explicit_match, best_compat);
> -
>      return best_compat;
>  }
>  
> @@ -1188,6 +1185,9 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>      bool guest_radix;
>      Error *local_err = NULL;
>  
> +    trace_cas_check_pvr_start(cpu->compat_pvr, cpu->max_compat,
> +                              cpu->env.spr[SPR_PVR]);
> +
>      if (smc->pre_2_9_cas_pvr) {
>          cas_pvr = cas_check_pvr_pre_2_9(cpu, &addr, &local_err);
>      } else {
> @@ -1198,6 +1198,8 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>          return H_HARDWARE;
>      }
>  
> +    trace_cas_check_pvr_complete(cpu->compat_pvr, cas_pvr);
> +
>      /* Update CPUs */
>      if (cpu->compat_pvr != cas_pvr) {
>          ppc_set_compat_all(cas_pvr, &local_err);
> diff --git a/hw/ppc/trace-events b/hw/ppc/trace-events
> index 43d265f..5329740 100644
> --- a/hw/ppc/trace-events
> +++ b/hw/ppc/trace-events
> @@ -14,8 +14,10 @@ spapr_cas_failed(unsigned long n) "DT diff buffer is too small: %ld bytes"
>  spapr_cas_continue(unsigned long n) "Copy changes to the guest: %ld bytes"
>  
>  # hw/ppc/spapr_hcall.c
> -spapr_cas_pvr_try(uint32_t pvr) "%x"
> -spapr_cas_pvr(uint32_t cur_pvr, bool explicit_match, uint32_t new_pvr) "current=%x, explicit_match=%u, new=%x"
> +cas_check_pvr_pre_2_9(uint32_t pvr, uint32_t mask, bool match, unsigned lvl, uint32_t compat_pvr) "0x%08x/0x%08x match=%d lvl=%d compat_pvr=0x%x"
> +cas_check_pvr(uint32_t pvr, uint32_t pvr_mask, bool explicit_match, uint32_t best_compat) "0x%08x/0x%08x explicit_match=%d best_compat=0x%08x"
> +cas_check_pvr_start(uint32_t compat_pvr, uint32_t max_compat, uint32_t phys_pvr) "Initial compat PVR 0x%08x, max compat 0x%08x (real PVR 0x%08x)"
> +cas_check_pvr_complete(uint32_t old_pvr, uint32_t new_pvr) "Compatibility PVR was 0x%08x, now 0x%08x"
>  
>  # hw/ppc/spapr_iommu.c
>  spapr_iommu_put(uint64_t liobn, uint64_t ioba, uint64_t tce, uint64_t ret) "liobn=%"PRIx64" ioba=0x%"PRIx64" tce=0x%"PRIx64" ret=%"PRId64


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-05-18  8:07 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-18  5:45 [Qemu-devel] [PATCH 0/3] pseries: Reverse behaviour change for older machine types David Gibson
2017-05-18  5:45 ` [Qemu-devel] [PATCH 1/3] pseries: Split CAS PVR negotiation out into a separate function David Gibson
2017-05-18  7:46   ` Laurent Vivier
2017-05-18  7:59   ` Greg Kurz
2017-05-18  5:45 ` [Qemu-devel] [PATCH 2/3] pseries: Restore PVR negotiation logic for pre-2.9 machine types David Gibson
2017-05-18  8:01   ` Greg Kurz
2017-05-18  5:45 ` [Qemu-devel] [PATCH 3/3] pseries: Improve tracing of CPU compatibility negotiation David Gibson
2017-05-18  7:47   ` Laurent Vivier
2017-05-18  8:07   ` Greg Kurz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.