On Wed, 13 Jun 2018 10:45:06 +1000 David Gibson <da...@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 <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.
>
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) */
> >
> >
>
pgpI2guGTujzw.pgp
Description: OpenPGP digital signature
