Message ID | 152882305541.114463.3137854902721347235.stgit@bahia.lan |
---|---|
State | New |
Headers | show |
Series | [1/3] target/ppc: drop empty #if/#endif block | expand |
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) */ > >
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) */ > > > > >
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.
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
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.
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~
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~
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.
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~
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~ >
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! :)
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) */
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(-)