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

[Cédric Le Goater]

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.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.
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.