On 7/11/19 9:57 PM, Nicholas Piggin wrote: > Claudio Carvalho's on June 29, 2019 6:08 am: >> 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 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 <suka...@linux.vnet.ibm.com> >> Signed-off-by: Ram Pai <linux...@us.ibm.com> >> [ Update the commit message ] >> 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 */ >> #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
