[PATCH v4 1/8] KVM: PPC: Ultravisor: Introduce the MSR_S bit

[Claudio Carvalho]

On 7/11/19 9:57 PM, Nicholas Piggin wrote:
> Claudio Carvalho's on June 29, 2019 6:08 am:
>> From: Sukadev Bhattiprolu <sukadev at linux.vnet.ibm.com>
>>
>> The ultravisor processor mode is introduced in POWER platforms that
>> supports the Protected Execution Facility (PEF). Ultravisor is higher
>> privileged than hypervisor mode.
>>
>> In PEF enabled platforms, the MSR_S bit is used to indicate if the
>> thread is in secure state. With the MSR_S bit, the privilege state of
>> the thread is now determined by MSR_S, MSR_HV and MSR_PR, as follows:
>>
>> S   HV  PR
>> -----------------------
>> 0   x   1   problem
>> 1   0   1   problem
>> x   x   0   privileged
>> x   1   0   hypervisor
>> 1   1   0   ultravisor
>> 1   1   1   reserved
> What does this table mean? I thought 'x' meant either


Yes, it means either. The table was arranged that way to say that:
- hypervisor state is also a privileged state,
- ultravisor state is also a hypervisor state.


> , but in that
> case there are several states that can apply to the same
> combination of bits.
>
> Would it be clearer to rearrange the table so the columns are the HV
> and PR bits we know and love, plus the effect of S=1 on each of them?
>
>       HV  PR  S=0         S=1
>       ---------------------------------------------
>       0   0   privileged  privileged (secure guest kernel)
>       0   1   problem     problem (secure guest userspace)
>       1   0   hypervisor  ultravisor
>       1   1   problem     reserved
>
> Is that accurate?

Yes, it is. I also like this format. I will consider it.


>
>
>> The hypervisor doesn't (and can't) run with the MSR_S bit set, but a
>> secure guest and the ultravisor firmware do.
>>
>> Signed-off-by: Sukadev Bhattiprolu <sukadev at linux.vnet.ibm.com>
>> Signed-off-by: Ram Pai <linuxram at us.ibm.com>
>> [ Update the commit message ]
>> Signed-off-by: Claudio Carvalho <cclaudio at linux.ibm.com>
>> ---
>>  arch/powerpc/include/asm/reg.h | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
>> index 10caa145f98b..39b4c0a519f5 100644
>> --- a/arch/powerpc/include/asm/reg.h
>> +++ b/arch/powerpc/include/asm/reg.h
>> @@ -38,6 +38,7 @@
>>  #define MSR_TM_LG	32		/* Trans Mem Available */
>>  #define MSR_VEC_LG	25	        /* Enable AltiVec */
>>  #define MSR_VSX_LG	23		/* Enable VSX */
>> +#define MSR_S_LG	22		/* Secure VM bit */
>>  #define MSR_POW_LG	18		/* Enable Power Management */
>>  #define MSR_WE_LG	18		/* Wait State Enable */
>>  #define MSR_TGPR_LG	17		/* TLB Update registers in use */
>> @@ -71,11 +72,13 @@
>>  #define MSR_SF		__MASK(MSR_SF_LG)	/* Enable 64 bit mode */
>>  #define MSR_ISF		__MASK(MSR_ISF_LG)	/* Interrupt 64b mode valid on 630 */
>>  #define MSR_HV 		__MASK(MSR_HV_LG)	/* Hypervisor state */
>> +#define MSR_S		__MASK(MSR_S_LG)	/* Secure state */
> This is a real nitpick, but why two different comments for the bit 
> number and the mask?

Fixed for the next version. Both comments will be /* Secure state */

Thanks
Claudio