On 3/15/21 2:47 PM, Richard Henderson wrote: > On 3/15/21 4:23 AM, Cédric Le Goater wrote: >> On 3/14/21 6:59 PM, Richard Henderson wrote: >>> Only one of the three places in hw/ppc that modify msr updated >>> hflags. Even in that case, use the official interface instead >>> of a direct call to hreg_compute_hflags. >> >> ppc_store_msr() is the interface to use. >> >>> Signed-off-by: Richard Henderson <richard.hender...@linaro.org> >>> --- >>> Cc: Cédric Le Goater <c...@kaod.org> >>> Cc: Greg Kurz <gr...@kaod.org> >>> --- >>> hw/ppc/pnv_core.c | 3 ++- >>> hw/ppc/spapr_hcall.c | 3 +-- >>> hw/ppc/spapr_rtas.c | 3 ++- >>> 3 files changed, 5 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c >>> index bd2bf2e044..31f041b9c7 100644 >>> --- a/hw/ppc/pnv_core.c >>> +++ b/hw/ppc/pnv_core.c >>> @@ -29,6 +29,7 @@ >>> #include "hw/ppc/pnv_xscom.h" >>> #include "hw/ppc/xics.h" >>> #include "hw/qdev-properties.h" >>> +#include "helper_regs.h" >>> static const char *pnv_core_cpu_typename(PnvCore *pc) >>> { >>> @@ -54,7 +55,7 @@ static void pnv_core_cpu_reset(PnvCore *pc, PowerPCCPU >>> *cpu) >>> */ >>> env->gpr[3] = PNV_FDT_ADDR; >>> env->nip = 0x10; >>> - env->msr |= MSR_HVB; /* Hypervisor mode */ >>> + hreg_store_msr(env, env->msr | MSR_HVB, true); /* Hypervisor mode */ >> >> >> This is going to have the opposite effect of not setting the HV bit in the >> PowerNV machine. See the comment in powerpc_set_excp_state(). >> >> May be commit 1c953ba57ada ("ppc: Fix hreg_store_msr() so that non-HV >> mode cannot alter MSR:HV") needs a fix first. > > Hmm. I mis-read the code and assumed "allow_hv" allowed hv to be changed. > There must be some kind of quirkyness here that I don't understand. This part was added ~14 years ago by commit a4f30719a8cd ("PowerPC hypervisor mode is not fundamentally available only for PowerPC 64. Remove TARGET_PPC64 dependency and add code provision to be able to define a fake 32 bits CPU with hypervisor feature support.")
I am afraid we kept adding stuff on top of it. > I'll just have these reset functions use hreg_recompute_hflags directly. Yes. That should be ok. Thanks, C.