Re: [PATCH v4 1/8] KVM: PPC: Ultravisor: Introduce the MSR_S bit
On 7/11/19 9:57 PM, Nicholas Piggin wrote: > Claudio Carvalho's on June 29, 2019 6:08 am: >> From: Sukadev Bhattiprolu >> >> 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 >> Signed-off-by: Ram Pai >> [ Update the commit message ] >> Signed-off-by: Claudio Carvalho >> --- >> 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_LG22 /* 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
Re: [PATCH v4 1/8] KVM: PPC: Ultravisor: Introduce the MSR_S bit
Nicholas Piggin writes: > Claudio Carvalho's on June 29, 2019 6:08 am: >> From: Sukadev Bhattiprolu >> >> 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, 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? I like that format. cheers
Re: [PATCH v4 1/8] KVM: PPC: Ultravisor: Introduce the MSR_S bit
Michael Ellerman's on July 11, 2019 10:57 pm: > Claudio Carvalho writes: >> From: Sukadev Bhattiprolu >> >> 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 >> Signed-off-by: Ram Pai >> [ 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 >> --- >> 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_LG22 /* 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
Re: [PATCH v4 1/8] KVM: PPC: Ultravisor: Introduce the MSR_S bit
Claudio Carvalho's on June 29, 2019 6:08 am: > From: Sukadev Bhattiprolu > > 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, 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? > > 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 > Signed-off-by: Ram Pai > [ Update the commit message ] > Signed-off-by: Claudio Carvalho > --- > 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_LG32 /* 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_LG18 /* 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?
Re: [PATCH v4 1/8] KVM: PPC: Ultravisor: Introduce the MSR_S bit
Claudio Carvalho writes: > From: Sukadev Bhattiprolu > > 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 > Signed-off-by: Ram Pai > [ 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 > --- > 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_LG32 /* 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_LG18 /* 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_S0 > #endif > > /* > -- > 2.20.1
Re: [PATCH v4 1/8] KVM: PPC: Ultravisor: Introduce the MSR_S bit
On 2019-06-28 15:08, Claudio Carvalho wrote: From: Sukadev Bhattiprolu 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 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 Signed-off-by: Ram Pai [ Update the commit message ] Signed-off-by: Claudio Carvalho Reviewed-by: Janani Janakiraman --- 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_LG17 /* 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_ISF0 #define MSR_HV 0 +#define MSR_S 0 #endif /*
[PATCH v4 1/8] KVM: PPC: Ultravisor: Introduce the MSR_S bit
From: Sukadev Bhattiprolu 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 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 Signed-off-by: Ram Pai [ Update the commit message ] Signed-off-by: Claudio Carvalho --- 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_LG17 /* 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_ISF0 #define MSR_HV 0 +#define MSR_S 0 #endif /* -- 2.20.1
