On 2018-04-27 10:22, Peng Fan wrote:
>
>
>> -----Original Message-----
>> From: Jan Kiszka [mailto:[email protected]]
>> Sent: 2018年4月27日 16:01
>> 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-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%2Fx
>>>>>> en
>>>>>>
>>>>
>> 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?
>
> This is a partial copy of get_cpu_parange in
> hypervisor/arch/arm64/include/asm/paging.h.
>
> I thought modify get_cpu_parange(void) to get_cpu_parange(int cpu),
> and get info using get_cpu_parange(x) from arch_paging_init, but putting
> per_cpu in file paging will cause
> a lot of complication failures.
> percpu.h requires some macros defined in paging.h.
>
> Use a table in arch_paging_cpu and leave get_cpu_parange not changed, or find
> a way
> to restruct get_cpu_parange?
It's fine to move get_cpu_parange into an arm64-specific module,
un-inlining it for that arch. ARM can stick with a simple inline version.
Jan
>
> What do you think?
>
> -Peng.
>
>>
>>>
>>> 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
--
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.
