[PATCH v4 1/8] KVM: PPC: Ultravisor: Introduce the MSR_S bit
mpe at ellerman.id.au
Thu Jul 11 22:57:07 AEST 2019
Claudio Carvalho <cclaudio at linux.ibm.com> writes:
> From: Sukadev Bhattiprolu <sukadev at 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 at linux.vnet.ibm.com>
> Signed-off-by: Ram Pai <linuxram at 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 at 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.
> #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 */
> /* 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
More information about the Linuxppc-dev