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 <gr...@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
signature.asc
Description: PGP signature
