[Qemu-devel] [PATCH 1/3] target/ppc: drop empty #if/#endif block

[Qemu-devel] [PATCH 1/3] target/ppc: drop empty #if/#endif block

[Greg Kurz]

Commit 9d6f106552fa moved the last line in this block to somewhere else,
but it forgot to remove the now useless #if/#endif.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 target/ppc/cpu.h |    2 --
 1 file changed, 2 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 0247c1f04c37..a91f1a8777eb 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1300,8 +1300,6 @@ void ppc_store_ptcr(CPUPPCState *env, target_ulong value);
 void ppc_store_msr (CPUPPCState *env, target_ulong value);
 
 void ppc_cpu_list (FILE *f, fprintf_function cpu_fprintf);
-#if defined(TARGET_PPC64)
-#endif
 
 /* Time-base and decrementer management */
 #ifndef NO_CPU_IO_DEFS

Re: [Qemu-devel] [PATCH 1/3] target/ppc: drop empty #if/#endif block

[Philippe Mathieu-Daudé]

On 06/12/2018 01:27 PM, Greg Kurz wrote:
> Commit 9d6f106552fa moved the last line in this block to somewhere else,
> but it forgot to remove the now useless #if/#endif.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/ppc/cpu.h |    2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 0247c1f04c37..a91f1a8777eb 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1300,8 +1300,6 @@ void ppc_store_ptcr(CPUPPCState *env, target_ulong value);
>  void ppc_store_msr (CPUPPCState *env, target_ulong value);
>  
>  void ppc_cpu_list (FILE *f, fprintf_function cpu_fprintf);
> -#if defined(TARGET_PPC64)
> -#endif
>  
>  /* Time-base and decrementer management */
>  #ifndef NO_CPU_IO_DEFS
> 
> 

[Qemu-devel] [PATCH 2/3] spapr: fix leak in h_client_architecture_support()

[Greg Kurz]

If the negotiated compat mode can't be set, but raw mode is supported,
we decide to ignore the error. An so, we should free it to prevent a
memory leak.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_hcall.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 022f6d810182..8b9a4b577fbf 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1547,6 +1547,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
                 error_report_err(local_err);
                 return H_HARDWARE;
             }
+            error_free(local_err);
             local_err = NULL;
         }
     }

[Qemu-devel] [PATCH 3/3] target/ppc: filter out non-zero PCR values when using TCG

[Greg Kurz]

Bits set in the PCR disable features of the processor. TCG currently
doesn't implement that, ie, we always act like if PCR is all zeros.

But it is still possible for the PCR to have a non-null value. This may
confuse the guest.

There are three distinct cases:

1) a powernv guest doing mtspr SPR_PCR

2) reset of a pseries guest if the max-cpu-compat machine property is set

3) CAS of a pseries guest

This patch adds a ppc_store_pcr() helper that ensures we cannot put
a non-null value in the PCR when using TCG. This helper also has
error propagation support, so that each case listed above can be
handled appropriately:

1) since the powernv machine is mostly used for OpenPOWER FW devel,
   we just print an error and let QEMU continue execution

2) an error is printed and QEMU exits, ie, same behaviour as when
   KVM doesn't support the requested compat mode

3) an error is printed and QEMU reports H_HARDWARE to the guest

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 target/ppc/compat.c      |   26 ++++++++++++++++++++++++--
 target/ppc/cpu.h         |    3 +++
 target/ppc/misc_helper.c |    9 ++++++---
 3 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/target/ppc/compat.c b/target/ppc/compat.c
index 807c906f6848..08aa99e6ad47 100644
--- a/target/ppc/compat.c
+++ b/target/ppc/compat.c
@@ -138,8 +138,8 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
 {
     const CompatInfo *compat = compat_by_pvr(compat_pvr);
     CPUPPCState *env = &cpu->env;
-    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     uint64_t pcr;
+    Error *local_err = NULL;
 
     if (!compat_pvr) {
         pcr = 0;
@@ -165,8 +165,30 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
         }
     }
 
+    ppc_store_pcr(env, pcr, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
     cpu->compat_pvr = compat_pvr;
-    env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
+}
+
+void ppc_store_pcr(CPUPPCState *env, target_ulong value, Error **errp)
+{
+    PowerPCCPU *cpu = ppc_env_get_cpu(env);
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+
+    /* TODO: this check should go away once we actually put the proper PCR
+     * checks in the various bits of TCG that should have them.
+     */
+    if (!kvm_enabled() && value != 0) {
+        error_setg(errp, "TCG doesn't support PCR value 0x"TARGET_FMT_lx,
+                   value);
+        return;
+    }
+
+    env->spr[SPR_PCR] = value & pcc->pcr_mask;
 }
 
 typedef struct {
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index a91f1a8777eb..fdaae34feffb 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1296,6 +1296,9 @@ int ppc_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, int size, int rw,
 #if !defined(CONFIG_USER_ONLY)
 void ppc_store_sdr1 (CPUPPCState *env, target_ulong value);
 void ppc_store_ptcr(CPUPPCState *env, target_ulong value);
+#if defined(TARGET_PPC64)
+void ppc_store_pcr(CPUPPCState *env, target_ulong value, Error **errp);
+#endif
 #endif /* !defined(CONFIG_USER_ONLY) */
 void ppc_store_msr (CPUPPCState *env, target_ulong value);
 
diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
index b88493009609..7a9b45a01453 100644
--- a/target/ppc/misc_helper.c
+++ b/target/ppc/misc_helper.c
@@ -21,6 +21,7 @@
 #include "exec/exec-all.h"
 #include "exec/helper-proto.h"
 #include "qemu/error-report.h"
+#include "qapi/error.h"
 
 #include "helper_regs.h"
 
@@ -102,10 +103,12 @@ void helper_store_ptcr(CPUPPCState *env, target_ulong val)
 
 void helper_store_pcr(CPUPPCState *env, target_ulong value)
 {
-    PowerPCCPU *cpu = ppc_env_get_cpu(env);
-    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+    Error *local_err = NULL;
 
-    env->spr[SPR_PCR] = value & pcc->pcr_mask;
+    ppc_store_pcr(env, value, &local_err);
+    if (local_err) {
+        error_report_err(local_err);
+    }
 }
 #endif /* defined(TARGET_PPC64) */
 

Re: [Qemu-devel] [PATCH 2/3] spapr: fix leak in h_client_architecture_support()

[Philippe Mathieu-Daudé]

On 06/12/2018 02:01 PM, Greg Kurz wrote:
> If the negotiated compat mode can't be set, but raw mode is supported,
> we decide to ignore the error. An so, we should free it to prevent a
> memory leak.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/ppc/spapr_hcall.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 022f6d810182..8b9a4b577fbf 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1547,6 +1547,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>                  error_report_err(local_err);
>                  return H_HARDWARE;
>              }
> +            error_free(local_err);
>              local_err = NULL;
>          }
>      }
> 
> 

Re: [Qemu-devel] [PATCH 1/3] target/ppc: drop empty #if/#endif block

[David Gibson]

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

On Tue, Jun 12, 2018 at 06:27:54PM +0200, Greg Kurz wrote:
> Commit 9d6f106552fa moved the last line in this block to somewhere else,
> but it forgot to remove the now useless #if/#endif.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-3.0, thanks.

> ---
>  target/ppc/cpu.h |    2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 0247c1f04c37..a91f1a8777eb 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1300,8 +1300,6 @@ void ppc_store_ptcr(CPUPPCState *env, target_ulong value);
>  void ppc_store_msr (CPUPPCState *env, target_ulong value);
>  
>  void ppc_cpu_list (FILE *f, fprintf_function cpu_fprintf);
> -#if defined(TARGET_PPC64)
> -#endif
>  
>  /* Time-base and decrementer management */
>  #ifndef NO_CPU_IO_DEFS
> 

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 2/3] spapr: fix leak in h_client_architecture_support()

[David Gibson]

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

On Tue, Jun 12, 2018 at 07:01:26PM +0200, Greg Kurz wrote:
> If the negotiated compat mode can't be set, but raw mode is supported,
> we decide to ignore the error. An so, we should free it to prevent a
> memory leak.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-3.0, thanks.

> ---
>  hw/ppc/spapr_hcall.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 022f6d810182..8b9a4b577fbf 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1547,6 +1547,7 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>                  error_report_err(local_err);
>                  return H_HARDWARE;
>              }
> +            error_free(local_err);
>              local_err = NULL;
>          }
>      }
> 

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 3/3] target/ppc: filter out non-zero PCR values when using TCG

[David Gibson]

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

On Tue, Jun 12, 2018 at 07:04:15PM +0200, Greg Kurz wrote:
> Bits set in the PCR disable features of the processor. TCG currently
> doesn't implement that, ie, we always act like if PCR is all zeros.
> 
> But it is still possible for the PCR to have a non-null value. This may
> confuse the guest.
> 
> There are three distinct cases:
> 
> 1) a powernv guest doing mtspr SPR_PCR
> 
> 2) reset of a pseries guest if the max-cpu-compat machine property is set
> 
> 3) CAS of a pseries guest
> 
> This patch adds a ppc_store_pcr() helper that ensures we cannot put
> a non-null value in the PCR when using TCG. This helper also has
> error propagation support, so that each case listed above can be
> handled appropriately:
> 
> 1) since the powernv machine is mostly used for OpenPOWER FW devel,
>    we just print an error and let QEMU continue execution
> 
> 2) an error is printed and QEMU exits, ie, same behaviour as when
>    KVM doesn't support the requested compat mode
> 
> 3) an error is printed and QEMU reports H_HARDWARE to the guest
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

I'm not really convinced this is a good idea.  Printing a (non fatal)
error if the guest attempts to write a non-zero value to the PCR
should be ok.  However, you're generating a fatal error if the machine
tries to set the PCR in TCG mode.  That could easily happen using,
e.g. the cap-htm flag on a TCG guest.  That would take TCG from mostly
working, to refusing to run at all.

> ---
>  target/ppc/compat.c      |   26 ++++++++++++++++++++++++--
>  target/ppc/cpu.h         |    3 +++
>  target/ppc/misc_helper.c |    9 ++++++---
>  3 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/target/ppc/compat.c b/target/ppc/compat.c
> index 807c906f6848..08aa99e6ad47 100644
> --- a/target/ppc/compat.c
> +++ b/target/ppc/compat.c
> @@ -138,8 +138,8 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
>  {
>      const CompatInfo *compat = compat_by_pvr(compat_pvr);
>      CPUPPCState *env = &cpu->env;
> -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      uint64_t pcr;
> +    Error *local_err = NULL;
>  
>      if (!compat_pvr) {
>          pcr = 0;
> @@ -165,8 +165,30 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
>          }
>      }
>  
> +    ppc_store_pcr(env, pcr, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
>      cpu->compat_pvr = compat_pvr;
> -    env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
> +}
> +
> +void ppc_store_pcr(CPUPPCState *env, target_ulong value, Error **errp)
> +{
> +    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +
> +    /* TODO: this check should go away once we actually put the proper PCR
> +     * checks in the various bits of TCG that should have them.
> +     */
> +    if (!kvm_enabled() && value != 0) {
> +        error_setg(errp, "TCG doesn't support PCR value 0x"TARGET_FMT_lx,
> +                   value);
> +        return;
> +    }
> +
> +    env->spr[SPR_PCR] = value & pcc->pcr_mask;
>  }
>  
>  typedef struct {
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index a91f1a8777eb..fdaae34feffb 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1296,6 +1296,9 @@ int ppc_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, int size, int rw,
>  #if !defined(CONFIG_USER_ONLY)
>  void ppc_store_sdr1 (CPUPPCState *env, target_ulong value);
>  void ppc_store_ptcr(CPUPPCState *env, target_ulong value);
> +#if defined(TARGET_PPC64)
> +void ppc_store_pcr(CPUPPCState *env, target_ulong value, Error **errp);
> +#endif
>  #endif /* !defined(CONFIG_USER_ONLY) */
>  void ppc_store_msr (CPUPPCState *env, target_ulong value);
>  
> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> index b88493009609..7a9b45a01453 100644
> --- a/target/ppc/misc_helper.c
> +++ b/target/ppc/misc_helper.c
> @@ -21,6 +21,7 @@
>  #include "exec/exec-all.h"
>  #include "exec/helper-proto.h"
>  #include "qemu/error-report.h"
> +#include "qapi/error.h"
>  
>  #include "helper_regs.h"
>  
> @@ -102,10 +103,12 @@ void helper_store_ptcr(CPUPPCState *env, target_ulong val)
>  
>  void helper_store_pcr(CPUPPCState *env, target_ulong value)
>  {
> -    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +    Error *local_err = NULL;
>  
> -    env->spr[SPR_PCR] = value & pcc->pcr_mask;
> +    ppc_store_pcr(env, value, &local_err);
> +    if (local_err) {
> +        error_report_err(local_err);
> +    }
>  }
>  #endif /* defined(TARGET_PPC64) */
>  
> 

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 3/3] target/ppc: filter out non-zero PCR values when using TCG

[Greg Kurz]

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

On Wed, 13 Jun 2018 10:45:06 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Jun 12, 2018 at 07:04:15PM +0200, Greg Kurz wrote:
> > Bits set in the PCR disable features of the processor. TCG currently
> > doesn't implement that, ie, we always act like if PCR is all zeros.
> > 
> > But it is still possible for the PCR to have a non-null value. This may
> > confuse the guest.
> > 
> > There are three distinct cases:
> > 
> > 1) a powernv guest doing mtspr SPR_PCR
> > 
> > 2) reset of a pseries guest if the max-cpu-compat machine property is set
> > 
> > 3) CAS of a pseries guest
> > 
> > This patch adds a ppc_store_pcr() helper that ensures we cannot put
> > a non-null value in the PCR when using TCG. This helper also has
> > error propagation support, so that each case listed above can be
> > handled appropriately:
> > 
> > 1) since the powernv machine is mostly used for OpenPOWER FW devel,
> >    we just print an error and let QEMU continue execution
> > 
> > 2) an error is printed and QEMU exits, ie, same behaviour as when
> >    KVM doesn't support the requested compat mode
> > 
> > 3) an error is printed and QEMU reports H_HARDWARE to the guest
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> I'm not really convinced this is a good idea.  Printing a (non fatal)
> error if the guest attempts to write a non-zero value to the PCR
> should be ok.  However, you're generating a fatal error if the machine
> tries to set the PCR in TCG mode.  That could easily happen using,
> e.g. the cap-htm flag on a TCG guest.  That would take TCG from mostly
> working, to refusing to run at all.
> 

I'm confused... I don't see anything related to HTM in TCG. Also we have
the following in cap_htm_apply():

    if (tcg_enabled()) {
        error_setg(errp,
                   "No Transactional Memory support in TCG, try cap-htm=off");

I'm probably missing something... can you enlighten me ?

> > ---
> >  target/ppc/compat.c      |   26 ++++++++++++++++++++++++--
> >  target/ppc/cpu.h         |    3 +++
> >  target/ppc/misc_helper.c |    9 ++++++---
> >  3 files changed, 33 insertions(+), 5 deletions(-)
> > 
> > diff --git a/target/ppc/compat.c b/target/ppc/compat.c
> > index 807c906f6848..08aa99e6ad47 100644
> > --- a/target/ppc/compat.c
> > +++ b/target/ppc/compat.c
> > @@ -138,8 +138,8 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
> >  {
> >      const CompatInfo *compat = compat_by_pvr(compat_pvr);
> >      CPUPPCState *env = &cpu->env;
> > -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> >      uint64_t pcr;
> > +    Error *local_err = NULL;
> >  
> >      if (!compat_pvr) {
> >          pcr = 0;
> > @@ -165,8 +165,30 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
> >          }
> >      }
> >  
> > +    ppc_store_pcr(env, pcr, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> >      cpu->compat_pvr = compat_pvr;
> > -    env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
> > +}
> > +
> > +void ppc_store_pcr(CPUPPCState *env, target_ulong value, Error **errp)
> > +{
> > +    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> > +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > +
> > +    /* TODO: this check should go away once we actually put the proper PCR
> > +     * checks in the various bits of TCG that should have them.
> > +     */
> > +    if (!kvm_enabled() && value != 0) {
> > +        error_setg(errp, "TCG doesn't support PCR value 0x"TARGET_FMT_lx,
> > +                   value);
> > +        return;
> > +    }
> > +
> > +    env->spr[SPR_PCR] = value & pcc->pcr_mask;
> >  }
> >  
> >  typedef struct {
> > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > index a91f1a8777eb..fdaae34feffb 100644
> > --- a/target/ppc/cpu.h
> > +++ b/target/ppc/cpu.h
> > @@ -1296,6 +1296,9 @@ int ppc_cpu_handle_mmu_fault(CPUState *cpu, vaddr address, int size, int rw,
> >  #if !defined(CONFIG_USER_ONLY)
> >  void ppc_store_sdr1 (CPUPPCState *env, target_ulong value);
> >  void ppc_store_ptcr(CPUPPCState *env, target_ulong value);
> > +#if defined(TARGET_PPC64)
> > +void ppc_store_pcr(CPUPPCState *env, target_ulong value, Error **errp);
> > +#endif
> >  #endif /* !defined(CONFIG_USER_ONLY) */
> >  void ppc_store_msr (CPUPPCState *env, target_ulong value);
> >  
> > diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> > index b88493009609..7a9b45a01453 100644
> > --- a/target/ppc/misc_helper.c
> > +++ b/target/ppc/misc_helper.c
> > @@ -21,6 +21,7 @@
> >  #include "exec/exec-all.h"
> >  #include "exec/helper-proto.h"
> >  #include "qemu/error-report.h"
> > +#include "qapi/error.h"
> >  
> >  #include "helper_regs.h"
> >  
> > @@ -102,10 +103,12 @@ void helper_store_ptcr(CPUPPCState *env, target_ulong val)
> >  
> >  void helper_store_pcr(CPUPPCState *env, target_ulong value)
> >  {
> > -    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> > -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > +    Error *local_err = NULL;
> >  
> > -    env->spr[SPR_PCR] = value & pcc->pcr_mask;
> > +    ppc_store_pcr(env, value, &local_err);
> > +    if (local_err) {
> > +        error_report_err(local_err);
> > +    }
> >  }
> >  #endif /* defined(TARGET_PPC64) */
> >  
> >   
> 


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

Re: [Qemu-devel] [PATCH 3/3] target/ppc: filter out non-zero PCR values when using TCG

[David Gibson]

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

On Wed, Jun 13, 2018 at 10:19:15AM +0200, Greg Kurz wrote:
> On Wed, 13 Jun 2018 10:45:06 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, Jun 12, 2018 at 07:04:15PM +0200, Greg Kurz wrote:
> > > Bits set in the PCR disable features of the processor. TCG currently
> > > doesn't implement that, ie, we always act like if PCR is all zeros.
> > > 
> > > But it is still possible for the PCR to have a non-null value. This may
> > > confuse the guest.
> > > 
> > > There are three distinct cases:
> > > 
> > > 1) a powernv guest doing mtspr SPR_PCR
> > > 
> > > 2) reset of a pseries guest if the max-cpu-compat machine property is set
> > > 
> > > 3) CAS of a pseries guest
> > > 
> > > This patch adds a ppc_store_pcr() helper that ensures we cannot put
> > > a non-null value in the PCR when using TCG. This helper also has
> > > error propagation support, so that each case listed above can be
> > > handled appropriately:
> > > 
> > > 1) since the powernv machine is mostly used for OpenPOWER FW devel,
> > >    we just print an error and let QEMU continue execution
> > > 
> > > 2) an error is printed and QEMU exits, ie, same behaviour as when
> > >    KVM doesn't support the requested compat mode
> > > 
> > > 3) an error is printed and QEMU reports H_HARDWARE to the guest
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>  
> > 
> > I'm not really convinced this is a good idea.  Printing a (non fatal)
> > error if the guest attempts to write a non-zero value to the PCR
> > should be ok.  However, you're generating a fatal error if the machine
> > tries to set the PCR in TCG mode.  That could easily happen using,
> > e.g. the cap-htm flag on a TCG guest.  That would take TCG from mostly
> > working, to refusing to run at all.
> > 
> 
> I'm confused... I don't see anything related to HTM in TCG. Also we have
> the following in cap_htm_apply():
> 
>     if (tcg_enabled()) {
>         error_setg(errp,
>                    "No Transactional Memory support in TCG, try cap-htm=off");
> 
> I'm probably missing something... can you enlighten me ?

Ok, so right now when cap-htm=off we don't actually enforce that, we
just don't advertise it to the guest.  We probably _should_ enforce
that, and the way we'd do it is to set the appropriate bit in the
PCR.  That'll do the right thing for KVM (well, once we update KVM to
actually pass on the PCR value) but would break TCG in conjunction
with your patch above.

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 3/3] target/ppc: filter out non-zero PCR values when using TCG

[Greg Kurz]

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

On Wed, 13 Jun 2018 22:05:02 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jun 13, 2018 at 10:19:15AM +0200, Greg Kurz wrote:
> > On Wed, 13 Jun 2018 10:45:06 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Tue, Jun 12, 2018 at 07:04:15PM +0200, Greg Kurz wrote:  
> > > > Bits set in the PCR disable features of the processor. TCG currently
> > > > doesn't implement that, ie, we always act like if PCR is all zeros.
> > > > 
> > > > But it is still possible for the PCR to have a non-null value. This may
> > > > confuse the guest.
> > > > 
> > > > There are three distinct cases:
> > > > 
> > > > 1) a powernv guest doing mtspr SPR_PCR
> > > > 
> > > > 2) reset of a pseries guest if the max-cpu-compat machine property is set
> > > > 
> > > > 3) CAS of a pseries guest
> > > > 
> > > > This patch adds a ppc_store_pcr() helper that ensures we cannot put
> > > > a non-null value in the PCR when using TCG. This helper also has
> > > > error propagation support, so that each case listed above can be
> > > > handled appropriately:
> > > > 
> > > > 1) since the powernv machine is mostly used for OpenPOWER FW devel,
> > > >    we just print an error and let QEMU continue execution
> > > > 
> > > > 2) an error is printed and QEMU exits, ie, same behaviour as when
> > > >    KVM doesn't support the requested compat mode
> > > > 
> > > > 3) an error is printed and QEMU reports H_HARDWARE to the guest
> > > > 
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>    
> > > 
> > > I'm not really convinced this is a good idea.  Printing a (non fatal)
> > > error if the guest attempts to write a non-zero value to the PCR
> > > should be ok.  However, you're generating a fatal error if the machine
> > > tries to set the PCR in TCG mode.  That could easily happen using,
> > > e.g. the cap-htm flag on a TCG guest.  That would take TCG from mostly
> > > working, to refusing to run at all.
> > >   
> > 
> > I'm confused... I don't see anything related to HTM in TCG. Also we have
> > the following in cap_htm_apply():
> > 
> >     if (tcg_enabled()) {
> >         error_setg(errp,
> >                    "No Transactional Memory support in TCG, try cap-htm=off");
> > 
> > I'm probably missing something... can you enlighten me ?  
> 
> Ok, so right now when cap-htm=off we don't actually enforce that, we
> just don't advertise it to the guest.  We probably _should_ enforce
> that, and the way we'd do it is to set the appropriate bit in the
> PCR.  That'll do the right thing for KVM (well, once we update KVM to
> actually pass on the PCR value) but would break TCG in conjunction
> with your patch above.
> 

Hmm... The granularity of the PCR bits is PowerISA versions, not individual
facilities AFAIK... or am I missing something again ?

Anyway, with the current code, if we start the guest with:

-machine accel=tcg,max-cpu-compat=power8 -cpu POWER9

We get this in the guest:

# grep ^cpu /proc/cpuinfo 
cpu             : POWER8 (architected), altivec supported

This is the expected result of the CAS negotiation, but
the guest can still execute PowerISA 3.0 instructions
that should cause an invalid instruction exception.

I agree this shouldn't cause QEMU to exit, but I guess we should
at least leave the PCR to all zeroes, so that the guest view
is consistent with what TGC does (ie, raw mode). And maybe an
error message to indicate that the PCR is ignored in TCG... but
since that could be guest triggered, and the user cannot do anything
about it, I'm wondering if this should rather be a trace actually.

Does it make sense for you ?

Cheers,

--
Greg

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

Re: [Qemu-devel] [PATCH 3/3] target/ppc: filter out non-zero PCR values when using TCG

[David Gibson]

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

On Wed, Jun 13, 2018 at 04:26:39PM +0200, Greg Kurz wrote:
> On Wed, 13 Jun 2018 22:05:02 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Jun 13, 2018 at 10:19:15AM +0200, Greg Kurz wrote:
> > > On Wed, 13 Jun 2018 10:45:06 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > >   
> > > > On Tue, Jun 12, 2018 at 07:04:15PM +0200, Greg Kurz wrote:  
> > > > > Bits set in the PCR disable features of the processor. TCG currently
> > > > > doesn't implement that, ie, we always act like if PCR is all zeros.
> > > > > 
> > > > > But it is still possible for the PCR to have a non-null value. This may
> > > > > confuse the guest.
> > > > > 
> > > > > There are three distinct cases:
> > > > > 
> > > > > 1) a powernv guest doing mtspr SPR_PCR
> > > > > 
> > > > > 2) reset of a pseries guest if the max-cpu-compat machine property is set
> > > > > 
> > > > > 3) CAS of a pseries guest
> > > > > 
> > > > > This patch adds a ppc_store_pcr() helper that ensures we cannot put
> > > > > a non-null value in the PCR when using TCG. This helper also has
> > > > > error propagation support, so that each case listed above can be
> > > > > handled appropriately:
> > > > > 
> > > > > 1) since the powernv machine is mostly used for OpenPOWER FW devel,
> > > > >    we just print an error and let QEMU continue execution
> > > > > 
> > > > > 2) an error is printed and QEMU exits, ie, same behaviour as when
> > > > >    KVM doesn't support the requested compat mode
> > > > > 
> > > > > 3) an error is printed and QEMU reports H_HARDWARE to the guest
> > > > > 
> > > > > Signed-off-by: Greg Kurz <groug@kaod.org>    
> > > > 
> > > > I'm not really convinced this is a good idea.  Printing a (non fatal)
> > > > error if the guest attempts to write a non-zero value to the PCR
> > > > should be ok.  However, you're generating a fatal error if the machine
> > > > tries to set the PCR in TCG mode.  That could easily happen using,
> > > > e.g. the cap-htm flag on a TCG guest.  That would take TCG from mostly
> > > > working, to refusing to run at all.
> > > >   
> > > 
> > > I'm confused... I don't see anything related to HTM in TCG. Also we have
> > > the following in cap_htm_apply():
> > > 
> > >     if (tcg_enabled()) {
> > >         error_setg(errp,
> > >                    "No Transactional Memory support in TCG, try cap-htm=off");
> > > 
> > > I'm probably missing something... can you enlighten me ?  
> > 
> > Ok, so right now when cap-htm=off we don't actually enforce that, we
> > just don't advertise it to the guest.  We probably _should_ enforce
> > that, and the way we'd do it is to set the appropriate bit in the
> > PCR.  That'll do the right thing for KVM (well, once we update KVM to
> > actually pass on the PCR value) but would break TCG in conjunction
> > with your patch above.
> 
> Hmm... The granularity of the PCR bits is PowerISA versions, not individual
> facilities AFAIK... or am I missing something again ?

Huh.. so.  In the 3.0 ISA, there are only ISA version bits.  But in
the 2.07 ISA, there are ISA version bits *and* a bit to control HTM.

I'm not quite sure what to make of that.  I kind of love the fact that
they incompatibly change the compatibility register.

> Anyway, with the current code, if we start the guest with:
> 
> -machine accel=tcg,max-cpu-compat=power8 -cpu POWER9
> 
> We get this in the guest:
> 
> # grep ^cpu /proc/cpuinfo 
> cpu             : POWER8 (architected), altivec supported
> 
> This is the expected result of the CAS negotiation, but
> the guest can still execute PowerISA 3.0 instructions
> that should cause an invalid instruction exception.

Right, this is a bug, albeit not a very high priority one.

> I agree this shouldn't cause QEMU to exit, but I guess we should
> at least leave the PCR to all zeroes, so that the guest view
> is consistent with what TGC does (ie, raw mode). And maybe an
> error message to indicate that the PCR is ignored in TCG... but
> since that could be guest triggered, and the user cannot do anything
> about it, I'm wondering if this should rather be a trace actually.
> 
> Does it make sense for you ?

TBH, I don't care all that much.  There are a bunch of cases where TCG
doesn't behave quite right, most without warnings.  One more or less
doesn't make a huge difference.

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 3/3] target/ppc: filter out non-zero PCR values when using TCG

[Richard Henderson]

On 06/12/2018 10:19 PM, Greg Kurz wrote:
> I'm confused... I don't see anything related to HTM in TCG. Also we have
> the following in cap_htm_apply():
> 
>     if (tcg_enabled()) {
>         error_setg(errp,
>                    "No Transactional Memory support in TCG, try cap-htm=off");
> 
> I'm probably missing something... can you enlighten me ?

One of the two IBM machines -- and I thought it was ppc not s390x, but the code
you quote seems to deny that memory -- has stub support for TM within TCG.

To wit, the instructions are recognized and transactions always fail.  Which is
not a bad way to test the required fallback paths that rarely fail on hardware.
 ;-)


r~

Re: [Qemu-devel] [PATCH 3/3] target/ppc: filter out non-zero PCR values when using TCG

[Greg Kurz]

On Thu, 14 Jun 2018 09:52:55 -1000
Richard Henderson <richard.henderson@linaro.org> wrote:

> On 06/12/2018 10:19 PM, Greg Kurz wrote:
> > I'm confused... I don't see anything related to HTM in TCG. Also we have
> > the following in cap_htm_apply():
> > 
> >     if (tcg_enabled()) {
> >         error_setg(errp,
> >                    "No Transactional Memory support in TCG, try cap-htm=off");
> > 
> > I'm probably missing something... can you enlighten me ?  
> 
> One of the two IBM machines -- and I thought it was ppc not s390x, but the code
> you quote seems to deny that memory -- has stub support for TM within TCG.
> 

Oh ? I didn't know and didn't check :)

> To wit, the instructions are recognized and transactions always fail.  Which is
> not a bad way to test the required fallback paths that rarely fail on hardware.
>  ;-)

If TM instructions don't cause an exception, I guess its reasonable to say
they're supported :)

--
G

> 
> 
> r~

Re: [Qemu-devel] [PATCH 3/3] target/ppc: filter out non-zero PCR values when using TCG

[David Gibson]

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

On Fri, Jun 15, 2018 at 12:00:20AM +0200, Greg Kurz wrote:
> On Thu, 14 Jun 2018 09:52:55 -1000
> Richard Henderson <richard.henderson@linaro.org> wrote:
> 
> > On 06/12/2018 10:19 PM, Greg Kurz wrote:
> > > I'm confused... I don't see anything related to HTM in TCG. Also we have
> > > the following in cap_htm_apply():
> > > 
> > >     if (tcg_enabled()) {
> > >         error_setg(errp,
> > >                    "No Transactional Memory support in TCG, try cap-htm=off");
> > > 
> > > I'm probably missing something... can you enlighten me ?  
> > 
> > One of the two IBM machines -- and I thought it was ppc not s390x, but the code
> > you quote seems to deny that memory -- has stub support for TM within TCG.
> 
> Oh ? I didn't know and didn't check :)

That is true, there are stub implementations of the TM instructions.

> 
> > To wit, the instructions are recognized and transactions always fail.  Which is
> > not a bad way to test the required fallback paths that rarely fail on hardware.
> >  ;-)
> 
> If TM instructions don't cause an exception, I guess its reasonable to say
> they're supported :)

That argument has come up before, and I disagree.  I don't think it's
reasonable to advertise TM support as available if the instructions
always fail.

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 3/3] target/ppc: filter out non-zero PCR values when using TCG

[Richard Henderson]

On 06/14/2018 03:45 PM, David Gibson wrote:
>>> To wit, the instructions are recognized and transactions always fail.  Which is
>>> not a bad way to test the required fallback paths that rarely fail on hardware.
>>>  ;-)
>>
>> If TM instructions don't cause an exception, I guess its reasonable to say
>> they're supported :)
> 
> That argument has come up before, and I disagree.  I don't think it's
> reasonable to advertise TM support as available if the instructions
> always fail.

Is it possible to turn it on from the command-line?
Like one can with -cpu foo,+opt on x86 or -cpu foo,opt=on on s390?


r~

Re: [Qemu-devel] [PATCH 3/3] target/ppc: filter out non-zero PCR values when using TCG

[David Gibson]

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

On Thu, Jun 14, 2018 at 05:38:16PM -1000, Richard Henderson wrote:
> On 06/14/2018 03:45 PM, David Gibson wrote:
> >>> To wit, the instructions are recognized and transactions always fail.  Which is
> >>> not a bad way to test the required fallback paths that rarely fail on hardware.
> >>>  ;-)
> >>
> >> If TM instructions don't cause an exception, I guess its reasonable to say
> >> they're supported :)
> > 
> > That argument has come up before, and I disagree.  I don't think it's
> > reasonable to advertise TM support as available if the instructions
> > always fail.
> 
> Is it possible to turn it on from the command-line?

Not at present.. attempting to is what gives the error message
discussed above.

> Like one can with -cpu foo,+opt on x86 or -cpu foo,opt=on on s390?
> 
> 
> r~
> 

-- 
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: 833 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/3] target/ppc: filter out non-zero PCR values when using TCG

[Greg Kurz]

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

On Fri, 15 Jun 2018 11:45:01 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Jun 15, 2018 at 12:00:20AM +0200, Greg Kurz wrote:
> > On Thu, 14 Jun 2018 09:52:55 -1000
> > Richard Henderson <richard.henderson@linaro.org> wrote:
> >   
> > > On 06/12/2018 10:19 PM, Greg Kurz wrote:  
> > > > I'm confused... I don't see anything related to HTM in TCG. Also we have
> > > > the following in cap_htm_apply():
> > > > 
> > > >     if (tcg_enabled()) {
> > > >         error_setg(errp,
> > > >                    "No Transactional Memory support in TCG, try cap-htm=off");
> > > > 
> > > > I'm probably missing something... can you enlighten me ?    
> > > 
> > > One of the two IBM machines -- and I thought it was ppc not s390x, but the code
> > > you quote seems to deny that memory -- has stub support for TM within TCG.  
> > 
> > Oh ? I didn't know and didn't check :)  
> 
> That is true, there are stub implementations of the TM instructions.
> 
> >   
> > > To wit, the instructions are recognized and transactions always fail.  Which is
> > > not a bad way to test the required fallback paths that rarely fail on hardware.
> > >  ;-)  
> > 
> > If TM instructions don't cause an exception, I guess its reasonable to say
> > they're supported :)  
> 
> That argument has come up before, and I disagree.  I don't think it's
> reasonable to advertise TM support as available if the instructions
> always fail.
> 

Fair enough! :)

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