Re: [PATCH v3 14/16] hw/ppc: Use hreg_store_msr for msr updates

From: Richard Henderson <richard.henderson@linaro.org>
To: "Cédric Le Goater" <clg@kaod.org>, qemu-devel@nongnu.org
Cc: ivan@vmfacility.fr, qemu-ppc@nongnu.org,
	Greg Kurz <groug@kaod.org>,
	david@gibson.dropbear.id.au
Subject: Re: [PATCH v3 14/16] hw/ppc: Use hreg_store_msr for msr updates
Date: Mon, 15 Mar 2021 07:47:35 -0600	[thread overview]
Message-ID: <f963e548-d2c0-d0a3-d8ed-bccc3c1d30e7@linaro.org> (raw)
In-Reply-To: <ea6b23c2-21db-a512-87b5-be5351ff0a74@kaod.org>

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.henderson@linaro.org>
>> ---
>> Cc: Cédric Le Goater <clg@kaod.org>
>> Cc: Greg Kurz <groug@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.

I'll just have these reset functions use hreg_recompute_hflags directly.

r~