[RFC PATCH] kvmppc: protection keys fix

[RFC PATCH] kvmppc: protection keys fix

[Ram Pai]

kvmppc_do_h_enter() hcall, clobbers the high-order two bits of
the protection key in the lower pte (ptel). Hence
any pkey  number above 7 fails to behave.

The following patch, preserves the pkey bits.

Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
 arch/powerpc/include/asm/book3s/64/mmu-hash.h |    1 +
 arch/powerpc/kvm/book3s_hv_rm_mmu.c           |    2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
index 369f9ff..f07a1c0 100644
--- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h
+++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
@@ -109,6 +109,7 @@
 #define HPTE_R_KEY_BIT2		ASM_CONST(0x0000000000000800)
 #define HPTE_R_KEY_BIT3		ASM_CONST(0x0000000000000400)
 #define HPTE_R_KEY_BIT4		ASM_CONST(0x0000000000000200)
+#define HPTE_R_KEY		(HPTE_R_KEY_LO | HPTE_R_KEY_HI)
 
 #define HPTE_V_1TB_SEG		ASM_CONST(0x4000000000000000)
 #define HPTE_V_VRMA_MASK	ASM_CONST(0x4001ffffff000000)
diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
index ce6f212..d9462ab 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
@@ -268,7 +268,7 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
 	if (!realmode)
 		local_irq_restore(irq_flags);
 
-	ptel &= ~(HPTE_R_PP0 - psize);
+	ptel &= HPTE_R_KEY | HPTE_R_PP0 | (psize-1);
 	ptel |= pa;
 
 	if (pa)
-- 
1.7.1

Re: [RFC PATCH] kvmppc: protection keys fix

[Paul Mackerras]

On Fri, Jul 28, 2017 at 09:22:58AM -0700, Ram Pai wrote:
> kvmppc_do_h_enter() hcall, clobbers the high-order two bits of
> the protection key in the lower pte (ptel). Hence
> any pkey  number above 7 fails to behave.
> 
> The following patch, preserves the pkey bits.
> 
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>

Seems right.

Reviewed-by: Paul Mackerras <paulus@ozlabs.org>

Re: [RFC PATCH] kvmppc: protection keys fix

[Balbir Singh]

On Sat, Jul 29, 2017 at 2:22 AM, Ram Pai <linuxram@us.ibm.com> wrote:
> kvmppc_do_h_enter() hcall, clobbers the high-order two bits of
> the protection key in the lower pte (ptel). Hence
> any pkey  number above 7 fails to behave.

I am not sure behave is the right word.

>
> The following patch, preserves the pkey bits.
>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---

Yep, Looks right. I would reword the change log though

Acked-by: Balbir Singh <bsingharora@gmail.com>

Re: [RFC PATCH] kvmppc: protection keys fix

[Aneesh Kumar K.V]

Ram Pai <linuxram@us.ibm.com> writes:

> kvmppc_do_h_enter() hcall, clobbers the high-order two bits of
> the protection key in the lower pte (ptel). Hence
> any pkey  number above 7 fails to behave.
>
> The following patch, preserves the pkey bits.
>
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> ---
>  arch/powerpc/include/asm/book3s/64/mmu-hash.h |    1 +
>  arch/powerpc/kvm/book3s_hv_rm_mmu.c           |    2 +-
>  2 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> index 369f9ff..f07a1c0 100644
> --- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> +++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> @@ -109,6 +109,7 @@
>  #define HPTE_R_KEY_BIT2		ASM_CONST(0x0000000000000800)
>  #define HPTE_R_KEY_BIT3		ASM_CONST(0x0000000000000400)
>  #define HPTE_R_KEY_BIT4		ASM_CONST(0x0000000000000200)
> +#define HPTE_R_KEY		(HPTE_R_KEY_LO | HPTE_R_KEY_HI)
>
>  #define HPTE_V_1TB_SEG		ASM_CONST(0x4000000000000000)
>  #define HPTE_V_VRMA_MASK	ASM_CONST(0x4001ffffff000000)
> diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> index ce6f212..d9462ab 100644
> --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> @@ -268,7 +268,7 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
>  	if (!realmode)
>  		local_irq_restore(irq_flags);
>
> -	ptel &= ~(HPTE_R_PP0 - psize);
> +	ptel &= HPTE_R_KEY | HPTE_R_PP0 | (psize-1);

Is this all needed to get keys to work on kvm ? We use key value 31 and
key value 0 along with absent and invalid hpte config to indicate mmio
and host swapped pages ? Won't that need to be fixed ?


>  	ptel |= pa;
>
>  	if (pa)
> -- 
> 1.7.1

-aneesh

Re: [RFC PATCH] kvmppc: protection keys fix

[Paul Mackerras]

On Wed, Aug 02, 2017 at 02:18:42PM +0530, Aneesh Kumar K.V wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > kvmppc_do_h_enter() hcall, clobbers the high-order two bits of
> > the protection key in the lower pte (ptel). Hence
> > any pkey  number above 7 fails to behave.
> >
> > The following patch, preserves the pkey bits.
> >
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > ---
> >  arch/powerpc/include/asm/book3s/64/mmu-hash.h |    1 +
> >  arch/powerpc/kvm/book3s_hv_rm_mmu.c           |    2 +-
> >  2 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> > index 369f9ff..f07a1c0 100644
> > --- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> > +++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> > @@ -109,6 +109,7 @@
> >  #define HPTE_R_KEY_BIT2		ASM_CONST(0x0000000000000800)
> >  #define HPTE_R_KEY_BIT3		ASM_CONST(0x0000000000000400)
> >  #define HPTE_R_KEY_BIT4		ASM_CONST(0x0000000000000200)
> > +#define HPTE_R_KEY		(HPTE_R_KEY_LO | HPTE_R_KEY_HI)
> >
> >  #define HPTE_V_1TB_SEG		ASM_CONST(0x4000000000000000)
> >  #define HPTE_V_VRMA_MASK	ASM_CONST(0x4001ffffff000000)
> > diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > index ce6f212..d9462ab 100644
> > --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > @@ -268,7 +268,7 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
> >  	if (!realmode)
> >  		local_irq_restore(irq_flags);
> >
> > -	ptel &= ~(HPTE_R_PP0 - psize);
> > +	ptel &= HPTE_R_KEY | HPTE_R_PP0 | (psize-1);
> 
> Is this all needed to get keys to work on kvm ? We use key value 31 and
> key value 0 along with absent and invalid hpte config to indicate mmio
> and host swapped pages ? Won't that need to be fixed ?

It should be fine, though if you want to audit the code go ahead.

Yong Ji fixed it last year to check for key 31 only when the HPTE is
absent, to clear the key when making a HPTE absent unless it is an
MMIO HPTE (and then it's set to 31), and to set the key to the
guest-requested key when making the HPTE present.

Paul.

Re: [RFC PATCH] kvmppc: protection keys fix

[Ram Pai]

On Wed, Aug 02, 2017 at 02:18:42PM +0530, Aneesh Kumar K.V wrote:
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > kvmppc_do_h_enter() hcall, clobbers the high-order two bits of
> > the protection key in the lower pte (ptel). Hence
> > any pkey  number above 7 fails to behave.
> >
> > The following patch, preserves the pkey bits.
> >
> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> > ---
> >  arch/powerpc/include/asm/book3s/64/mmu-hash.h |    1 +
> >  arch/powerpc/kvm/book3s_hv_rm_mmu.c           |    2 +-
> >  2 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> > index 369f9ff..f07a1c0 100644
> > --- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> > +++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> > @@ -109,6 +109,7 @@
> >  #define HPTE_R_KEY_BIT2		ASM_CONST(0x0000000000000800)
> >  #define HPTE_R_KEY_BIT3		ASM_CONST(0x0000000000000400)
> >  #define HPTE_R_KEY_BIT4		ASM_CONST(0x0000000000000200)
> > +#define HPTE_R_KEY		(HPTE_R_KEY_LO | HPTE_R_KEY_HI)
> >
> >  #define HPTE_V_1TB_SEG		ASM_CONST(0x4000000000000000)
> >  #define HPTE_V_VRMA_MASK	ASM_CONST(0x4001ffffff000000)
> > diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > index ce6f212..d9462ab 100644
> > --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> > @@ -268,7 +268,7 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
> >  	if (!realmode)
> >  		local_irq_restore(irq_flags);
> >
> > -	ptel &= ~(HPTE_R_PP0 - psize);
> > +	ptel &= HPTE_R_KEY | HPTE_R_PP0 | (psize-1);
> 
> Is this all needed to get keys to work on kvm ? We use key value 31 and
> key value 0 along with absent and invalid hpte config to indicate mmio
> and host swapped pages ? Won't that need to be fixed ?

The OS assumes key 0 and 1 are reserved and hence does not let
applications use it.  And the device tree tells us if key 31 is used or
not. If it is used the OS does not let application use it either.

Now, the device tree for KVM guests do not expose the key property yet.
That needs to be fixed.

RP

Re: [RFC PATCH] kvmppc: protection keys fix

[Thiago Jung Bauermann]

Ram Pai <linuxram@us.ibm.com> writes:

> On Wed, Aug 02, 2017 at 02:18:42PM +0530, Aneesh Kumar K.V wrote:
>> Ram Pai <linuxram@us.ibm.com> writes:
>> 
>> > kvmppc_do_h_enter() hcall, clobbers the high-order two bits of
>> > the protection key in the lower pte (ptel). Hence
>> > any pkey  number above 7 fails to behave.
>> >
>> > The following patch, preserves the pkey bits.
>> >
>> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>> > ---
>> >  arch/powerpc/include/asm/book3s/64/mmu-hash.h |    1 +
>> >  arch/powerpc/kvm/book3s_hv_rm_mmu.c           |    2 +-
>> >  2 files changed, 2 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
>> > index 369f9ff..f07a1c0 100644
>> > --- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h
>> > +++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
>> > @@ -109,6 +109,7 @@
>> >  #define HPTE_R_KEY_BIT2		ASM_CONST(0x0000000000000800)
>> >  #define HPTE_R_KEY_BIT3		ASM_CONST(0x0000000000000400)
>> >  #define HPTE_R_KEY_BIT4		ASM_CONST(0x0000000000000200)
>> > +#define HPTE_R_KEY		(HPTE_R_KEY_LO | HPTE_R_KEY_HI)
>> >
>> >  #define HPTE_V_1TB_SEG		ASM_CONST(0x4000000000000000)
>> >  #define HPTE_V_VRMA_MASK	ASM_CONST(0x4001ffffff000000)
>> > diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
>> > index ce6f212..d9462ab 100644
>> > --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
>> > +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
>> > @@ -268,7 +268,7 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
>> >  	if (!realmode)
>> >  		local_irq_restore(irq_flags);
>> >
>> > -	ptel &= ~(HPTE_R_PP0 - psize);
>> > +	ptel &= HPTE_R_KEY | HPTE_R_PP0 | (psize-1);
>> 
>> Is this all needed to get keys to work on kvm ? We use key value 31 and
>> key value 0 along with absent and invalid hpte config to indicate mmio
>> and host swapped pages ? Won't that need to be fixed ?
>
> The OS assumes key 0 and 1 are reserved and hence does not let
> applications use it.  And the device tree tells us if key 31 is used or
> not. If it is used the OS does not let application use it either.
>
> Now, the device tree for KVM guests do not expose the key property yet.
> That needs to be fixed.

I'm currently preparing a patch for that. One question: did I understand
correctly that Paul Mackerras' answer on this thread¹ means that KVM
doesn't need to reserve any keys for itself and can pass on 32 keys to
the guest?

-- 
Thiago Jung Bauermann
IBM Linux Technology Center

¹ https://www.spinics.net/lists/kvm-ppc/msg12765.html

Re: [RFC PATCH] kvmppc: protection keys fix

[Ram Pai]

On Thu, Aug 17, 2017 at 05:54:45PM -0300, Thiago Jung Bauermann wrote:
> 
> Ram Pai <linuxram@us.ibm.com> writes:
> 
> > On Wed, Aug 02, 2017 at 02:18:42PM +0530, Aneesh Kumar K.V wrote:
> >> Ram Pai <linuxram@us.ibm.com> writes:
> >> 
> >> > kvmppc_do_h_enter() hcall, clobbers the high-order two bits of
> >> > the protection key in the lower pte (ptel). Hence
> >> > any pkey  number above 7 fails to behave.
> >> >
> >> > The following patch, preserves the pkey bits.
> >> >
> >> > Signed-off-by: Ram Pai <linuxram@us.ibm.com>
> >> > ---
> >> >  arch/powerpc/include/asm/book3s/64/mmu-hash.h |    1 +
> >> >  arch/powerpc/kvm/book3s_hv_rm_mmu.c           |    2 +-
> >> >  2 files changed, 2 insertions(+), 1 deletions(-)
> >> >
> >> > diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> >> > index 369f9ff..f07a1c0 100644
> >> > --- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> >> > +++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> >> > @@ -109,6 +109,7 @@
> >> >  #define HPTE_R_KEY_BIT2		ASM_CONST(0x0000000000000800)
> >> >  #define HPTE_R_KEY_BIT3		ASM_CONST(0x0000000000000400)
> >> >  #define HPTE_R_KEY_BIT4		ASM_CONST(0x0000000000000200)
> >> > +#define HPTE_R_KEY		(HPTE_R_KEY_LO | HPTE_R_KEY_HI)
> >> >
> >> >  #define HPTE_V_1TB_SEG		ASM_CONST(0x4000000000000000)
> >> >  #define HPTE_V_VRMA_MASK	ASM_CONST(0x4001ffffff000000)
> >> > diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> >> > index ce6f212..d9462ab 100644
> >> > --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> >> > +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> >> > @@ -268,7 +268,7 @@ long kvmppc_do_h_enter(struct kvm *kvm, unsigned long flags,
> >> >  	if (!realmode)
> >> >  		local_irq_restore(irq_flags);
> >> >
> >> > -	ptel &= ~(HPTE_R_PP0 - psize);
> >> > +	ptel &= HPTE_R_KEY | HPTE_R_PP0 | (psize-1);
> >> 
> >> Is this all needed to get keys to work on kvm ? We use key value 31 and
> >> key value 0 along with absent and invalid hpte config to indicate mmio
> >> and host swapped pages ? Won't that need to be fixed ?
> >
> > The OS assumes key 0 and 1 are reserved and hence does not let
> > applications use it.  And the device tree tells us if key 31 is used or
> > not. If it is used the OS does not let application use it either.
> >
> > Now, the device tree for KVM guests do not expose the key property yet.
> > That needs to be fixed.
> 
> I'm currently preparing a patch for that. One question: did I understand
> correctly that Paul Mackerras' answer on this thread¹ means that KVM
> doesn't need to reserve any keys for itself and can pass on 32 keys to
> the guest?

No. KVM does use key 31 for emulated MMIO.

if (!(memslot && !(memslot->flags & KVM_MEMSLOT_INVALID))) {
	/* Emulated MMIO - mark this with key1 */
	pteh |= HPTE_V_ABSENT;
	ptel |= HPTE_R_KEY_HI |
	HPTE_R_KEY_LO;
	goto do_insert;
}


The device tree has to say 31 and not 32.

RP.

Re: [RFC PATCH] kvmppc: protection keys fix

[Paul Mackerras]

On Thu, Aug 17, 2017 at 03:01:31PM -0700, Ram Pai wrote:
> On Thu, Aug 17, 2017 at 05:54:45PM -0300, Thiago Jung Bauermann wrote:
[snip]
> > 
> > I'm currently preparing a patch for that. One question: did I understand
> > correctly that Paul Mackerras' answer on this thread¹ means that KVM
> > doesn't need to reserve any keys for itself and can pass on 32 keys to
> > the guest?
> 
> No. KVM does use key 31 for emulated MMIO.
> 
> if (!(memslot && !(memslot->flags & KVM_MEMSLOT_INVALID))) {
> 	/* Emulated MMIO - mark this with key1 */
> 	pteh |= HPTE_V_ABSENT;
> 	ptel |= HPTE_R_KEY_HI |
> 	HPTE_R_KEY_LO;
> 	goto do_insert;
> }

Actually, the guest *can* use all 32 keys.  We only use key 31 on
absent HPTEs (see the HPTE_V_ABSENT flag above - these are HPTEs that
the guest thinks are valid but are not actually mapped in the real
HPT).

Paul.

Re: [RFC PATCH] kvmppc: protection keys fix

[Ram Pai]

On Fri, Aug 18, 2017 at 10:02:01AM +1000, Paul Mackerras wrote:
> On Thu, Aug 17, 2017 at 03:01:31PM -0700, Ram Pai wrote:
> > On Thu, Aug 17, 2017 at 05:54:45PM -0300, Thiago Jung Bauermann wrote:
> [snip]
> > > 
> > > I'm currently preparing a patch for that. One question: did I understand
> > > correctly that Paul Mackerras' answer on this thread¹ means that KVM
> > > doesn't need to reserve any keys for itself and can pass on 32 keys to
> > > the guest?
> > 
> > No. KVM does use key 31 for emulated MMIO.
> > 
> > if (!(memslot && !(memslot->flags & KVM_MEMSLOT_INVALID))) {
> > 	/* Emulated MMIO - mark this with key1 */
> > 	pteh |= HPTE_V_ABSENT;
> > 	ptel |= HPTE_R_KEY_HI |
> > 	HPTE_R_KEY_LO;
> > 	goto do_insert;
> > }
> 
> Actually, the guest *can* use all 32 keys.  We only use key 31 on
> absent HPTEs (see the HPTE_V_ABSENT flag above - these are HPTEs that
> the guest thinks are valid but are not actually mapped in the real
> HPT).

Ok. looks like you use the HPTE_V_ABSENT flag to determine if a key fault
on key 31, needs to be redirected to the guest or handled internally.

RP

Re: [RFC PATCH] kvmppc: protection keys fix

[Paul Mackerras]

On Fri, Jul 28, 2017 at 09:22:58AM -0700, Ram Pai wrote:
> kvmppc_do_h_enter() hcall, clobbers the high-order two bits of
> the protection key in the lower pte (ptel). Hence
> any pkey  number above 7 fails to behave.
> 
> The following patch, preserves the pkey bits.
> 
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>

Thanks, patch applied to my kvm-ppc-next branch (with rewritten patch
description).

Paul.