Claudio Carvalho <cclaudio@xxxxxxxxxxxxx> writes: > From: Sukadev Bhattiprolu <sukadev@xxxxxxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxx> > Signed-off-by: Ram Pai <linuxram@xxxxxxxxxx> > [ 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@xxxxxxxxxxxxx> > --- > 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. cheers > #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 */ > #else > /* so tests for these bits fail on 32-bit */ > #define MSR_SF 0 > #define MSR_ISF 0 > #define MSR_HV 0 > +#define MSR_S 0 > #endif > > /* > -- > 2.20.1
