Michael Ellerman's on July 11, 2019 10:57 pm: > Claudio Carvalho <cclau...@linux.ibm.com> writes: >> From: Sukadev Bhattiprolu <suka...@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 <suka...@linux.vnet.ibm.com> >> Signed-off-by: Ram Pai <linux...@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 <cclau...@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
