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

From: Nicholas Piggin <npiggin@gmail.com>
To: Claudio Carvalho <cclaudio@linux.ibm.com>,
	linuxppc-dev@ozlabs.org, Michael Ellerman <mpe@ellerman.id.au>
Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>,
	Michael Anderson <andmike@linux.ibm.com>,
	Ram Pai <linuxram@us.ibm.com>,
	kvm-ppc@vger.kernel.org, Bharata B Rao <bharata@linux.ibm.com>,
	Ryan Grimm <grimm@linux.ibm.com>,
	Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>,
	Thiago Bauermann <bauerman@linux.ibm.com>,
	Anshuman Khandual <khandual@linux.vnet.ibm.com>
Subject: Re: [PATCH v4 1/8] KVM: PPC: Ultravisor: Introduce the MSR_S bit
Date: Fri, 12 Jul 2019 10:59:54 +1000	[thread overview]
Message-ID: <1562893118.ys3k642fnf.astroid@bobo.none> (raw)
In-Reply-To: <87muhkg258.fsf@concordia.ellerman.id.au>

Michael Ellerman's on July 11, 2019 10:57 pm:
> Claudio Carvalho <cclaudio@linux.ibm.com> writes:
>> From: Sukadev Bhattiprolu <sukadev@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 are you trying to express with the 'x' value?
> 
> I guess you mean it as "either" or "don't care" - but then you have
> cases where it could only have one value, such as hypervisor. I think it
> would be clearer if you spelled it out more explicitly.
> 
>> The hypervisor doesn't (and can't) run with the MSR_S bit set, but a
>> secure guest and the ultravisor firmware do.
> 
> I know you're trying to be helpful, but this comment is really just
> confusing to someone who doesn't have all the documentation.
> 
> I'd really like to see something in Documentation/powerpc describing at
> least the outline of how the system works. I'm pretty sure most of that
> is public, so even if it's mostly a list of references to other
> documentations or presentations that would be fine. But I'm not really
> happy with a whole new processor mode appearing with zero documentation
> in the tree.
> 
>> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
>> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>> [ Update the commit message ]
> 
> It's normal to prefix these comments with your handle to make it clear
> who is saying it.
> 
>> Signed-off-by: Claudio Carvalho <cclaudio@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 */
> 
> I don't think that's the best description, because it's also the
> Ultravisor bit when MSR[HV] = 1.
> 
> So "Secure state" as you have below would be better IMO.

Ooops I see Michael covered everything I wrote, sorry for the noise
I missed the thread.

Thanks,
Nick