On 2018-04-27 09:01, Peng Fan wrote:
> Hi Jans,
>
>> -----Original Message-----
>> From: Jan Kiszka [mailto:[email protected]]
>> Sent: 2018年4月26日 15:06
>> To: Peng Fan <[email protected]>; [email protected]; Claudio
>> Scordino <[email protected]>
>> Cc: [email protected]
>> Subject: Re: [PATCH] arm64: introduce CONFIG_CPU_PARANGE
>>
>> On 2018-04-20 12:08, Jan Kiszka wrote:
>>> On 2018-04-20 11:20, Peng Fan wrote:
>>>> Hi Jan,
>>>>
>>>>> -----Original Message-----
>>>>> From: Jan Kiszka [mailto:[email protected]]
>>>>> Sent: 2018年4月20日 16:19
>>>>> To: Peng Fan <[email protected]>; [email protected]
>>>>> Cc: [email protected]
>>>>> Subject: Re: [PATCH] arm64: introduce CONFIG_CPU_PARANGE
>>>>>
>>>>> On 2018-04-20 08:56, Peng Fan wrote:
>>>>>> In i.MX8QM, there are two types of Cortex-A CPUs, Cortex-A53 and
>>>>> Cortex-A72.
>>>>>> A53 ID_AA64MMFR0_EL1 shows its physical address range supported is
>>>>> 40bits.
>>>>>> A72 ID_AA64MMFR0_EL1 shows its physical address range supported is
>>>>> 44bits.
>>>>>
>>>>> So, such platforms will naturally be restricted to 40 bits PA for
>>>>> all cores and never attach resources at higher addresses, correct?
>>>>> Or are there asymmetric setups imaginable where only the A72 cores would
>> use such upper resources?
>>>>
>>>> This is not to set TCR_EL2.PS to 40, but restrict VTCR_EL2.PS to 40.
>>>> The issue is jailhouse use the PA to configure VTCR_EL2, it uses PA
>>>> range on A72 to configure A53, this is wrong.
>>>> My understanding is the VTCR_EL2 on all the cpus should use same
>>>> value and the PS field should use the minimal value supported by all the
>>>> cpu
>> types.
>>>>
>>>> This link includes how xen handles this, but introducing such logic
>>>> in jailhouse is not that straight forward,
>>>> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fxen
>>>>
>> bits.xen.org%2Fgitweb%2F%3Fp%3Dxen.git%3Ba%3Dblob%3Bf%3Dxen%2Farc
>> h%2F
>>>>
>> arm%2Fp2m.c%3Bh%3Dd43c3aa896ab857bd85387f09a3dfea05ca6bac1%3Bhb
>> %3DHEA
>>>>
>> D%23l1461&data=02%7C01%7Cpeng.fan%40nxp.com%7C39218e06782444bbd
>> c2808d
>>>>
>> 5ab4425e6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63660323
>> 154382
>>>>
>> 8207&sdata=k6IYzTIGVU%2FTLaRL%2BFJ5nmgRnFNbApeHprBo3rUQC3k%3D&
>> reserve
>>>> d=0
>>>
>>> Sure, we would have to collect the configuration on each CPU first and
>>> only then start laying out the guest page tables. But Jailhouse does
>>> all this early on the first CPU that is initialized. Need to think if
>>> that could be changed (to the last CPU), but it would be a major
>>> overhaul, if possible at all.
>>
>> Thought about this again: What we could easily do without reworking the whole
>> beast (the parange information is already be used right after
>> paging_init) is to add another barrier:
>>
>> diff --git a/hypervisor/setup.c b/hypervisor/setup.c index
>> 9eaac20ff..cb0ca7b14
>> 100644
>> --- a/hypervisor/setup.c
>> +++ b/hypervisor/setup.c
>> @@ -28,7 +28,7 @@ static const __attribute__((aligned(PAGE_SIZE))) u8
>> empty_page[PAGE_SIZE];
>>
>> static DEFINE_SPINLOCK(init_lock);
>> static unsigned int master_cpu_id = -1; -static volatile unsigned int
>> initialized_cpus;
>> +static volatile unsigned int entered_cpus, initialized_cpus;
>> static volatile int error;
>>
>> static void init_early(unsigned int cpu_id) @@ -177,6 +177,21 @@ int
>> entry(unsigned int cpu_id, struct per_cpu *cpu_data)
>>
>> spin_lock(&init_lock);
>>
>> + /*
>> + * If this CPU is last, make sure everything was committed before we
>> + * signal the other CPUs spinning on entered_cpus that they can
>> + * continue.
>> + */
>> + memory_barrier();
>> + entered_cpus++;
>> +
>> + spin_unlock(&init_lock);
>> +
>> + while (entered_cpus < hypervisor_header.online_cpus)
>> + cpu_relax();
>> +
>> + spin_lock(&init_lock);
>> +
>> if (master_cpu_id == -1) {
>> /* Only the master CPU, the first to enter this
>> * function, performs system-wide initializations. */
>>
>> ...and write parange info during arch_entry to the per-cpu struct. Then
>> arch_paging_init could collect all value and use the minimum.
>
> Based on your patch, I created a patch as you said above and tested on
> i.MX8QM,
> It works. I add a new id_aa64mmfr0 in per_cpu, not the lower 4 bits for
> parange,
> I think other fields of id_aa64mmfr0 might be needed in future.
Perfect!
>
> diff --git a/hypervisor/arch/arm-common/paging.c
> b/hypervisor/arch/arm-common/paging.c
> index 2ba7da6f..6e3af91c 100644
> --- a/hypervisor/arch/arm-common/paging.c
> +++ b/hypervisor/arch/arm-common/paging.c
> @@ -11,6 +11,7 @@
> */
>
> #include <jailhouse/paging.h>
> +#include <asm/percpu.h>
>
> unsigned int cpu_parange = 0;
>
> @@ -218,7 +219,37 @@ const struct paging *cell_paging;
>
> void arch_paging_init(void)
> {
> - cpu_parange = get_cpu_parange();
> + int parange = 0x10;
> + int i;
Both are used unsigned.
> +
> + for (i = 0; i < hypervisor_header.online_cpus; i++) {
> + if ((per_cpu(i)->id_aa64mmfr0 & 0xf) < parange)
> + parange = per_cpu(i)->id_aa64mmfr0 & 0xf;
> + }
0xf may deserve some explanatory BITMASK constant.
> +
> + switch (parange) {
> + case PARANGE_32B:
> + cpu_parange = 32;
> + break;
> + case PARANGE_36B:
> + cpu_parange = 36;
> + break;
> + case PARANGE_40B:
> + cpu_parange = 40;
> + break;
> + case PARANGE_42B:
> + cpu_parange = 42;
> + break;
> + case PARANGE_44B:
> + cpu_parange = 44;
> + break;
> + case PARANGE_48B:
> + cpu_parange = 48;
> + break;
> + default:
> + cpu_parange = 0;
> + break;
> + }
If this translation is irregular (no calculation possible), how about
using a table?
>
> if (cpu_parange < 44)
> /* 4 level page tables not supported for stage 2.
> diff --git a/hypervisor/arch/arm64/asm-defines.c
> b/hypervisor/arch/arm64/asm-defines.c
> index 58669a4b..81141dc9 100644
> --- a/hypervisor/arch/arm64/asm-defines.c
> +++ b/hypervisor/arch/arm64/asm-defines.c
> @@ -27,6 +27,7 @@ void common(void)
> debug_console.address);
> OFFSET(SYSCONFIG_HYPERVISOR_PHYS, jailhouse_system,
> hypervisor_memory.phys_start);
> + OFFSET(PERCPU_ID_AA64MMFR0, per_cpu, id_aa64mmfr0);
> BLANK();
>
> DEFINE(PERCPU_STACK_END,
> diff --git a/hypervisor/arch/arm64/entry.S b/hypervisor/arch/arm64/entry.S
> index 61c5dc49..d5b34f17 100644
> --- a/hypervisor/arch/arm64/entry.S
> +++ b/hypervisor/arch/arm64/entry.S
> @@ -157,6 +157,9 @@ el2_entry:
> */
> sub sp, sp, 20 * 8
>
> + mrs x29, id_aa64mmfr0_el1
> + str x29, [x1, #PERCPU_ID_AA64MMFR0]
> +
> mov x29, xzr /* reset fp,lr */
> mov x30, xzr
>
> diff --git a/hypervisor/arch/arm64/include/asm/percpu.h
> b/hypervisor/arch/arm64/include/asm/percpu.h
> index a88ed477..f8b9f77b 100644
> --- a/hypervisor/arch/arm64/include/asm/percpu.h
> +++ b/hypervisor/arch/arm64/include/asm/percpu.h
> @@ -34,6 +34,7 @@ struct per_cpu {
> u32 stats[JAILHOUSE_NUM_CPU_STATS];
> int shutdown_state;
> bool failed;
> + unsigned long id_aa64mmfr0;
>
> struct pending_irqs pending_irqs;
>
> -Peng.
>
Looking forward to regular patches!
Jan
--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux
--
You received this message because you are subscribed to the Google Groups
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
