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

[David Gibson]

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 <address@hidden>

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

signature.asc
Description: PGP signature