[PATCH 0/3] Enable IAMR storage keys for radix

[PATCH 0/3] Enable IAMR storage keys for radix

[Balbir Singh]

This series follows up on
https://lists.ozlabs.org/pipermail/linuxppc-dev/2016-August/147840.html

The first patch sets up AMOR in hypervisor mode. AMOR
needs to be setup before IAMR (details of AMOR/IAMR in
each patch). The second patch enables detection of exceptions
generated due to instruction fetch violations caused
and OOPSs' the task. The third patch enables IAMR for
both hypervisor and guest kernels.

I've tested with patch series with a sample hack and
payload.

Chris Smart helped with the series, reviewing and
providing valuable feedback

Cc: Chris Smart <chris@distroguy.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Michael Neuling <mikey@neuling.org>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Cc: Paul Mackerras <paulus@samba.org>

Balbir Singh (3):
  Setup AMOR in HV mode
  Detect instruction fetch denied and report
  Enable storage keys for radix - user mode execution

 arch/powerpc/mm/fault.c         |  4 ++++
 arch/powerpc/mm/pgtable-radix.c | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

-- 
2.5.5

[PATCH 1/3] Setup AMOR in HV mode

[Balbir Singh]

AMOR should be setup in HV mode, we set it up once
and let the generic kernel handle IAMR. This patch is
used to enable storage keys in a following patch as
defined in ISA 3

Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Signed-off-by: Balbir Singh <bsingharora@gmail.com>
---
 arch/powerpc/mm/pgtable-radix.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
index ed7bddc..0fdd8ed 100644
--- a/arch/powerpc/mm/pgtable-radix.c
+++ b/arch/powerpc/mm/pgtable-radix.c
@@ -320,6 +320,25 @@ static void update_hid_for_radix(void)
 		cpu_relax();
 }
 
+/*
+ * In HV mode, we init AMOR so that the hypervisor
+ * and guest can setup IMAR, enable key 0 and set
+ * it to 1
+ * AMOR = 1100....00 (Mask for key 0 is 11)
+ */
+static void __init radix_init_amor(void)
+{
+	unsigned long amor_mask = 0xc000000000000000;
+	unsigned long amor = mfspr(SPRN_AMOR);
+
+	if (cpu_has_feature(CPU_FTR_POWER9_DD1))
+		return;
+
+	amor |= amor_mask;
+
+	mtspr(SPRN_AMOR, amor);
+}
+
 void __init radix__early_init_mmu(void)
 {
 	unsigned long lpcr;
@@ -376,6 +395,7 @@ void __init radix__early_init_mmu(void)
 		lpcr = mfspr(SPRN_LPCR);
 		mtspr(SPRN_LPCR, lpcr | LPCR_UPRT | LPCR_HR);
 		radix_init_partition_table();
+		radix_init_amor();
 	}
 
 	radix_init_pgtable();
-- 
2.5.5

Re: [PATCH 1/3] Setup AMOR in HV mode

[Aneesh Kumar K.V]

Balbir Singh <bsingharora@gmail.com> writes:

> AMOR should be setup in HV mode, we set it up once
> and let the generic kernel handle IAMR. This patch is
> used to enable storage keys in a following patch as
> defined in ISA 3
>
> Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> ---
>  arch/powerpc/mm/pgtable-radix.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
> index ed7bddc..0fdd8ed 100644
> --- a/arch/powerpc/mm/pgtable-radix.c
> +++ b/arch/powerpc/mm/pgtable-radix.c
> @@ -320,6 +320,25 @@ static void update_hid_for_radix(void)
>  		cpu_relax();
>  }
>  
> +/*
> + * In HV mode, we init AMOR so that the hypervisor
> + * and guest can setup IMAR, enable key 0 and set
> + * it to 1
> + * AMOR = 1100....00 (Mask for key 0 is 11)
> + */
> +static void __init radix_init_amor(void)
> +{
> +	unsigned long amor_mask = 0xc000000000000000;
> +	unsigned long amor = mfspr(SPRN_AMOR);
> +
> +	if (cpu_has_feature(CPU_FTR_POWER9_DD1))
> +		return;

Why is DD1 excluded ?

> +
> +	amor |= amor_mask;
> +

Can't we just init it with the constant 0xc000000000000000;
I can understand kvm code wanting to do the '|' above, but does the host
init code need to look at the previous value there ?


> +	mtspr(SPRN_AMOR, amor);
> +}
> +
>  void __init radix__early_init_mmu(void)
>  {
>  	unsigned long lpcr;
> @@ -376,6 +395,7 @@ void __init radix__early_init_mmu(void)
>  		lpcr = mfspr(SPRN_LPCR);
>  		mtspr(SPRN_LPCR, lpcr | LPCR_UPRT | LPCR_HR);
>  		radix_init_partition_table();
> +		radix_init_amor();
>  	}
>  
>  	radix_init_pgtable();

What about secondaries ? Don't we need to init AMOR there ?

-aneesh

Re: [PATCH 1/3] Setup AMOR in HV mode

[Balbir Singh]

On 09/11/16 02:13, Aneesh Kumar K.V wrote:
> Balbir Singh <bsingharora@gmail.com> writes:
> 
>> AMOR should be setup in HV mode, we set it up once
>> and let the generic kernel handle IAMR. This patch is
>> used to enable storage keys in a following patch as
>> defined in ISA 3
>>
>> Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
>> ---
>>  arch/powerpc/mm/pgtable-radix.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
>> index ed7bddc..0fdd8ed 100644
>> --- a/arch/powerpc/mm/pgtable-radix.c
>> +++ b/arch/powerpc/mm/pgtable-radix.c
>> @@ -320,6 +320,25 @@ static void update_hid_for_radix(void)
>>  		cpu_relax();
>>  }
>>  
>> +/*
>> + * In HV mode, we init AMOR so that the hypervisor
>> + * and guest can setup IMAR, enable key 0 and set
>> + * it to 1
>> + * AMOR = 1100....00 (Mask for key 0 is 11)
>> + */
>> +static void __init radix_init_amor(void)
>> +{
>> +	unsigned long amor_mask = 0xc000000000000000;
>> +	unsigned long amor = mfspr(SPRN_AMOR);
>> +
>> +	if (cpu_has_feature(CPU_FTR_POWER9_DD1))
>> +		return;
> 
> Why is DD1 excluded ?
> 
>> +
>> +	amor |= amor_mask;
>> +
> 
> Can't we just init it with the constant 0xc000000000000000;
> I can understand kvm code wanting to do the '|' above, but does the host
> init code need to look at the previous value there ?
> 
> 
>> +	mtspr(SPRN_AMOR, amor);
>> +}
>> +
>>  void __init radix__early_init_mmu(void)
>>  {
>>  	unsigned long lpcr;
>> @@ -376,6 +395,7 @@ void __init radix__early_init_mmu(void)
>>  		lpcr = mfspr(SPRN_LPCR);
>>  		mtspr(SPRN_LPCR, lpcr | LPCR_UPRT | LPCR_HR);
>>  		radix_init_partition_table();
>> +		radix_init_amor();
>>  	}
>>  
>>  	radix_init_pgtable();
> 
> What about secondaries ? Don't we need to init AMOR there ?

My bad, I should have checked and caught this

Balbir Singh

[PATCH 2/3] Detect instruction fetch denied and report

[Balbir Singh]

ISA 3 allows for prevention of instruction fetch and execution
of user mode pages. If such an error occurs, SRR1 bit 35
reports the error. We catch and report the error in do_page_fault()

Signed-off-by: Balbir Singh <bsingharora@gmail.com>
---
 arch/powerpc/mm/fault.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index d0b137d..835fd03 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -404,6 +404,10 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
 		    (cpu_has_feature(CPU_FTR_NOEXECUTE) ||
 		     !(vma->vm_flags & (VM_READ | VM_WRITE))))
 			goto bad_area;
+
+		if (radix_enabled() && (regs->msr & SRR1_ISI_N_OR_G))
+			goto bad_area;
+
 #ifdef CONFIG_PPC_STD_MMU
 		/*
 		 * protfault should only happen due to us
-- 
2.5.5

Re: [PATCH 2/3] Detect instruction fetch denied and report

[Aneesh Kumar K.V]

Balbir Singh <bsingharora@gmail.com> writes:

> ISA 3 allows for prevention of instruction fetch and execution
> of user mode pages. If such an error occurs, SRR1 bit 35
> reports the error. We catch and report the error in do_page_fault()
>
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> ---
>  arch/powerpc/mm/fault.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index d0b137d..835fd03 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -404,6 +404,10 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
>  		    (cpu_has_feature(CPU_FTR_NOEXECUTE) ||
>  		     !(vma->vm_flags & (VM_READ | VM_WRITE))))
>  			goto bad_area;
> +
> +		if (radix_enabled() && (regs->msr & SRR1_ISI_N_OR_G))
> +			goto bad_area;

Why is this within radix_enabled() ? Hash can also generate an
interrupt with that SRR1 value right ?


> +
>  #ifdef CONFIG_PPC_STD_MMU
>  		/*
>  		 * protfault should only happen due to us


-aneesh

Re: [PATCH 2/3] Detect instruction fetch denied and report

[Balbir Singh]

On 09/11/16 03:09, Aneesh Kumar K.V wrote:
> Balbir Singh <bsingharora@gmail.com> writes:
> 
>> ISA 3 allows for prevention of instruction fetch and execution
>> of user mode pages. If such an error occurs, SRR1 bit 35
>> reports the error. We catch and report the error in do_page_fault()
>>
>> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
>> ---
>>  arch/powerpc/mm/fault.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index d0b137d..835fd03 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>> @@ -404,6 +404,10 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
>>  		    (cpu_has_feature(CPU_FTR_NOEXECUTE) ||
>>  		     !(vma->vm_flags & (VM_READ | VM_WRITE))))
>>  			goto bad_area;
>> +
>> +		if (radix_enabled() && (regs->msr & SRR1_ISI_N_OR_G))
>> +			goto bad_area;
> 
> Why is this within radix_enabled() ? Hash can also generate an
> interrupt with that SRR1 value right ?
> 

Yes the bits are not specific to radix. I suspect the check above took
care of instruction exceptions, so we've never needed it before. I'll
remove the check

Balbir

Re: [PATCH 2/3] Detect instruction fetch denied and report

[Aneesh Kumar K.V]

Balbir Singh <bsingharora@gmail.com> writes:

> ISA 3 allows for prevention of instruction fetch and execution
> of user mode pages. If such an error occurs, SRR1 bit 35
> reports the error. We catch and report the error in do_page_fault()
>
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> ---
>  arch/powerpc/mm/fault.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index d0b137d..835fd03 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -404,6 +404,10 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
>  		    (cpu_has_feature(CPU_FTR_NOEXECUTE) ||
>  		     !(vma->vm_flags & (VM_READ | VM_WRITE))))
>  			goto bad_area;
> +
> +		if (radix_enabled() && (regs->msr & SRR1_ISI_N_OR_G))
> +			goto bad_area;
> +

Can we also do this check early, ie, before down_read(mmap_sem)

-aneesh

[PATCH 3/3] Enable storage keys for radix - user mode execution

[Balbir Singh]

ISA 3 defines new encoded access authority that allows instruction
access prevention in privileged mode and allows normal access
to problem state. This patch just enables IAMR (Instruction Authority
Mask Register), enabling AMR would require more work.

I've tested this with a buggy driver and a simple payload. The payload
is specific to the build I've tested.

Signed-off-by: Balbir Singh <bsingharora@gmail.com>
---
 arch/powerpc/mm/pgtable-radix.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
index 0fdd8ed..cd3d400 100644
--- a/arch/powerpc/mm/pgtable-radix.c
+++ b/arch/powerpc/mm/pgtable-radix.c
@@ -339,6 +339,24 @@ static void __init radix_init_amor(void)
 	mtspr(SPRN_AMOR, amor);
 }
 
+/*
+ * For radix page tables we setup, the IAMR values as follows
+ * IMAR = 0100...00 (key 0 is set to 1)
+ * AMR, UAMR, UAMOR are not affected
+ */
+static void __init radix_init_iamr(void)
+{
+	unsigned long iamr_mask = 0x4000000000000000;
+	unsigned long iamr = mfspr(SPRN_IAMR);
+
+	if (cpu_has_feature(CPU_FTR_POWER9_DD1))
+		return;
+
+	iamr |= iamr_mask;
+
+	mtspr(SPRN_IAMR, iamr);
+}
+
 void __init radix__early_init_mmu(void)
 {
 	unsigned long lpcr;
@@ -398,6 +416,7 @@ void __init radix__early_init_mmu(void)
 		radix_init_amor();
 	}
 
+	radix_init_iamr();
 	radix_init_pgtable();
 }
 
-- 
2.5.5

Re: [PATCH 3/3] Enable storage keys for radix - user mode execution

[Aneesh Kumar K.V]

Balbir Singh <bsingharora@gmail.com> writes:

> ISA 3 defines new encoded access authority that allows instruction
> access prevention in privileged mode and allows normal access
> to problem state. This patch just enables IAMR (Instruction Authority
> Mask Register), enabling AMR would require more work.


We may want to explain what the rules are with details like IAMR class 0
bit 1 controls the instruction access etc. Also we can metion that we
now setup user pages such that EAA[0] is set to 0

>
> I've tested this with a buggy driver and a simple payload. The payload
> is specific to the build I've tested.
>
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> ---
>  arch/powerpc/mm/pgtable-radix.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
> index 0fdd8ed..cd3d400 100644
> --- a/arch/powerpc/mm/pgtable-radix.c
> +++ b/arch/powerpc/mm/pgtable-radix.c
> @@ -339,6 +339,24 @@ static void __init radix_init_amor(void)
>  	mtspr(SPRN_AMOR, amor);
>  }
>  
> +/*
> + * For radix page tables we setup, the IAMR values as follows
> + * IMAR = 0100...00 (key 0 is set to 1)
> + * AMR, UAMR, UAMOR are not affected
> + */
> +static void __init radix_init_iamr(void)
> +{
> +	unsigned long iamr_mask = 0x4000000000000000;
> +	unsigned long iamr = mfspr(SPRN_IAMR);
> +
> +	if (cpu_has_feature(CPU_FTR_POWER9_DD1))
> +		return;

is this needed ?

> +
> +	iamr |= iamr_mask;
> +
> +	mtspr(SPRN_IAMR, iamr);
> +}

Why do '|'. Who else can set this ?

> +
>  void __init radix__early_init_mmu(void)
>  {
>  	unsigned long lpcr;
> @@ -398,6 +416,7 @@ void __init radix__early_init_mmu(void)
>  		radix_init_amor();
>  	}
>  
> +	radix_init_iamr();
>  	radix_init_pgtable();
>  }

What about secondary cpus ?

-aneesh

Re: [PATCH 3/3] Enable storage keys for radix - user mode execution

[Balbir Singh]

On 09/11/16 03:15, Aneesh Kumar K.V wrote:
> Balbir Singh <bsingharora@gmail.com> writes:
> 
>> ISA 3 defines new encoded access authority that allows instruction
>> access prevention in privileged mode and allows normal access
>> to problem state. This patch just enables IAMR (Instruction Authority
>> Mask Register), enabling AMR would require more work.
> 
> 
> We may want to explain what the rules are with details like IAMR class 0
> bit 1 controls the instruction access etc. Also we can metion that we
> now setup user pages such that EAA[0] is set to 0
> 
>>
>> I've tested this with a buggy driver and a simple payload. The payload
>> is specific to the build I've tested.
>>
>> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
>> ---
>>  arch/powerpc/mm/pgtable-radix.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
>> index 0fdd8ed..cd3d400 100644
>> --- a/arch/powerpc/mm/pgtable-radix.c
>> +++ b/arch/powerpc/mm/pgtable-radix.c
>> @@ -339,6 +339,24 @@ static void __init radix_init_amor(void)
>>  	mtspr(SPRN_AMOR, amor);
>>  }
>>  
>> +/*
>> + * For radix page tables we setup, the IAMR values as follows
>> + * IMAR = 0100...00 (key 0 is set to 1)
>> + * AMR, UAMR, UAMOR are not affected
>> + */
>> +static void __init radix_init_iamr(void)
>> +{
>> +	unsigned long iamr_mask = 0x4000000000000000;
>> +	unsigned long iamr = mfspr(SPRN_IAMR);
>> +
>> +	if (cpu_has_feature(CPU_FTR_POWER9_DD1))
>> +		return;
> 
> is this needed ?
> 

Yes, IAMR support is DD1+

>> +
>> +	iamr |= iamr_mask;
>> +
>> +	mtspr(SPRN_IAMR, iamr);
>> +}
> 
> Why do '|'. Who else can set this ?
> 
>> +
>>  void __init radix__early_init_mmu(void)
>>  {
>>  	unsigned long lpcr;
>> @@ -398,6 +416,7 @@ void __init radix__early_init_mmu(void)
>>  		radix_init_amor();
>>  	}
>>  
>> +	radix_init_iamr();
>>  	radix_init_pgtable();
>>  }
> 
> What about secondary cpus ?
> 

Oops..Yes, will do a v3. 

> -aneesh
>