[1/3] arm64: mm: Support Common Not Private translations

Commit Message

Vladimir Murzin Oct. 10, 2017, 12:50 p.m. UTC

Hi James,

On 09/10/17 17:48, James Morse wrote:
> Hi Catalin, Vladimir,
> 
> On 09/10/17 16:23, Catalin Marinas wrote:
>> On Mon, Oct 09, 2017 at 01:55:32PM +0100, Vladimir Murzin wrote:
>>> This patch adds support for Common Not Private translations on
>>> different exceptions levels:
> 
>>> (2) For EL1 we postpone setting CNP till all cpus are up and rely on
>>>     cpufeature framework to 1) patch the code which is sensitive to
>>>     CNP and 2) update TTBR1_EL1 with CNP bit set. The only case where
>>>     TTBR1_EL1 can be reprogrammed is hibirnation, so the code there is
>>>     changed to save raw TTBR1_EL1 and blindly restore it on resume.
> 
>> Even if you do this when all the CPUs are up, that's not always true.
>> Starting with maxcpus=1 allows something like systemd to bring up new
>> CPUs once user space starts.
> 
> For secondary CPUs cpu_enable_cnp() will be called to set CNP on TTBR1_EL1. But
> what about cpuidle? This also resets the TTBR1_EL1 value.

Good point! I've missed it because reset happens via __enable_mmu, which has 
no idea about CnP.

Would something like below be sufficient?



> 
> 
>> The problem we have is that we don't know
>> what the firmware is doing, whether it's setting CnP or not. Maybe we
>> should add some statement in Documentation/arm64/booting.txt that
>> firmware must not use CnP at all.
> 
> 
>>> diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
>>> index 095d3c1..1d056f3 100644
>>> --- a/arch/arm64/kernel/hibernate.c
>>> +++ b/arch/arm64/kernel/hibernate.c
>>> @@ -124,7 +124,7 @@ int arch_hibernation_header_save(void *addr, unsigned int max_size)
>>>  		return -EOVERFLOW;
>>>  
>>>  	arch_hdr_invariants(&hdr->invariants);
>>> -	hdr->ttbr1_el1		= __pa_symbol(swapper_pg_dir);
>>> +	hdr->ttbr1_el1		= read_sysreg(ttbr1_el1);
>>>  	hdr->reenter_kernel	= _cpu_resume;
>>>  
>>>  	/* We can't use __hyp_get_vectors() because kvm may still be loaded */
> 
>> Are all the CPUs up when coming out of hibernation and restoring
>> ttbr1_el1?
> 
> 'nonboot' CPUs are powered off around hibernate, so this only runs on one CPU.
> 
> Restoring with the CNP set like this will share all the TTBR1 mappings using the
> reserved ASID out of TTBR0. Hibernate then calls cpu_uninstall_idmap() via
> __cpu_suspend_exit(), which will restore the original ttbr0 value and CNP bit.
> 

Thanks for explanation!

Vladimir

> 
> Thanks,
> 
> James
>

Comments

James Morse Oct. 10, 2017, 3:19 p.m. UTC | #1

Hi Vladimir,

On 10/10/17 13:50, Vladimir Murzin wrote:
> On 09/10/17 17:48, James Morse wrote:
>> On 09/10/17 16:23, Catalin Marinas wrote:
>>> On Mon, Oct 09, 2017 at 01:55:32PM +0100, Vladimir Murzin wrote:
>>>> This patch adds support for Common Not Private translations on
>>>> different exceptions levels:
>>
>>>> (2) For EL1 we postpone setting CNP till all cpus are up and rely on
>>>>     cpufeature framework to 1) patch the code which is sensitive to
>>>>     CNP and 2) update TTBR1_EL1 with CNP bit set. The only case where
>>>>     TTBR1_EL1 can be reprogrammed is hibirnation, so the code there is
>>>>     changed to save raw TTBR1_EL1 and blindly restore it on resume.
>>
>>> Even if you do this when all the CPUs are up, that's not always true.
>>> Starting with maxcpus=1 allows something like systemd to bring up new
>>> CPUs once user space starts.
>>
>> For secondary CPUs cpu_enable_cnp() will be called to set CNP on TTBR1_EL1. But
>> what about cpuidle? This also resets the TTBR1_EL1 value.
> 
> Good point! I've missed it because reset happens via __enable_mmu, which has 
> no idea about CnP.
> 
> Would something like below be sufficient?
> 
> 
> diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
> index 1e3be90..03a02c4 100644
> --- a/arch/arm64/kernel/suspend.c
> +++ b/arch/arm64/kernel/suspend.c
> @@ -46,6 +46,10 @@ void notrace __cpu_suspend_exit(void)
>  	 */
>  	cpu_uninstall_idmap();
>  
> +#ifdef CONFIG_ARM64_CNP
> +	/* Restore CnP bit in TTBR1_EL1 */
> +	cpu_replace_ttbr1(swapper_pg_dir);
> +#endif
>  	/*
>  	 * PSTATE was not saved over suspend/resume, re-enable any detected
>  	 * features that might not have been set correctly.
> 

This re-install -> uninstalls the idmap, and if we don't actually have CNP
support, it wouldn't have changed anything. How about:

> if (cpus_have_const_cap(ARM64_HAS_CNP)
> 	cpu_replace_ttbr1(lm_alias(swapper_pg_dir));

We could look at having a combined helper that is called with the idmap
installed and does the uninstall.


hibernate uses these cpu_suspend helpers to save/restore the CPU registers, so
if we fix cpu-idle, you don't need the hibernate hunk anymore.


Thanks,

James

Patch

diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
index 1e3be90..03a02c4 100644
--- a/arch/arm64/kernel/suspend.c
+++ b/arch/arm64/kernel/suspend.c
@@ -46,6 +46,10 @@  void notrace __cpu_suspend_exit(void)
 	 */
 	cpu_uninstall_idmap();
 
+#ifdef CONFIG_ARM64_CNP
+	/* Restore CnP bit in TTBR1_EL1 */
+	cpu_replace_ttbr1(swapper_pg_dir);
+#endif
 	/*
 	 * PSTATE was not saved over suspend/resume, re-enable any detected
 	 * features that might not have been set correctly.