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,
>> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/arm/p2m.c;h=d43c3aa896ab857bd85387f09a3dfea05ca6bac1;hb=HEAD#l1461
>
> 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.
Jan
>
> Claudio, do you think we could have a similar issue on the TX2? It's
> asymmetric as well IIRC, but maybe with identical parange values.
>
>>
>>>
>>>>
>>>> The minimal value 40 needs to be used as cpu parange, if using 44,
>>>> A53 will be broken. See the error message:
>>>>
>>>> "
>>>> Initializing Jailhouse hypervisor v0.8 (67-gd1b70bbf-dirty) on CPU 4
>>>> Code location: 0x0000ffffc0200060 Page pool usage after early setup:
>>>> mem 43/4067, remap 96/131072 Initializing processors:
>>>> CPU 4... OK
>>>> CPU 5... OK
>>>> CPU 3... OK
>>>> CPU 1... OK
>>>> CPU 0... OK
>>>> CPU 2... OK
>>>> Page pool usage after late setup: mem 40/4067, remap 368/131072
>>>> FATAL: instruction abort at 0x816bbefc
>>>>
>>>> FATAL: forbidden access (exception class 0x20) Cell state before
>>>> exception:
>>>> pc: ffff000000cd0efc lr: ffff000000cd0efc spsr: 600001c5 EL1
>>>> sp: ffff8008fff22fb0 esr: 20 1 0000084
>>>> x0: 0000000000000000 x1: 0000000000000000 x2:
>>> 0000000000000000
>>>> x3: 0000000000000000 x4: 0000000000000000 x5:
>>> 0000000000000000
>>>> x6: 0000000000000000 x7: 0000000000000000 x8:
>>> 0000000000000000
>>>> x9: 0000000000000000 x10: 0000000000000000 x11:
>>> 0000000000000000
>>>> x12: 0000000000000000 x13: 0000000000000000 x14:
>>> 0000000000000000
>>>> x15: 0000000000000000 x16: 0000000000000000 x17:
>>> 0000000000000000
>>>> x18: 0000000000000000 x19: ffff000000cd4910 x20: 0000000000000000
>>>> x21: 0000000000000000 x22: 0000000000000001 x23: ffff8008f6137dd0
>>>> x24: ffff00000945700f x25: ffff8008fff1f0a0 x26: ffff8008fff23090
>>>> x27: ffff000009307000 x28: ffff8008f6134000 x29: ffff8008fff22fb0
>>>>
>>>> Parking CPU 2 (Cell: "imx8qm")
>>>> "
>>>>
>>>> Because get_cpu_parange only runs on the master cpu which first runs
>>>> into hypervisor initialization and A72 runs faster than A53 in
>>>> i.MX8QM, so A72 will be the master cpu and 44 used as cpu parange.
>>>>
>>>> In this patch, introduce a macro to define the cpu parange that could
>>>> be supported by different cpu types.
>>>>
>>>> Signed-off-by: Peng Fan <[email protected]>
>>>> ---
>>>> Documentation/hypervisor-configuration.md | 7 +++++++
>>>> hypervisor/arch/arm64/include/asm/paging.h | 8 +++++++-
>>>> 2 files changed, 14 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/hypervisor-configuration.md
>>>> b/Documentation/hypervisor-configuration.md
>>>> index 021093e7..4dc4738b 100644
>>>> --- a/Documentation/hypervisor-configuration.md
>>>> +++ b/Documentation/hypervisor-configuration.md
>>>> @@ -42,6 +42,13 @@ General configuration parameters
>>>> */
>>>> #define CONFIG_BARE_METAL 1
>>>>
>>>> + /*
>>>> + * To ARM Processors, that has different cpu types integrated, such
>>>> + * as Big.Little Processors, they have different PA range.
>>>> + * So define this macro to choose one value works for both cpu types.
>>>> + */
>>>> + #define CONFIG_CPU_PARANGE 40
>>>> +
>>>> ### Example board specific configurations
>>>>
>>>> #### ARM
>>>> diff --git a/hypervisor/arch/arm64/include/asm/paging.h
>>>> b/hypervisor/arch/arm64/include/asm/paging.h
>>>> index 0fe1429d..55ee2a44 100644
>>>> --- a/hypervisor/arch/arm64/include/asm/paging.h
>>>> +++ b/hypervisor/arch/arm64/include/asm/paging.h
>>>> @@ -183,6 +183,10 @@ extern unsigned int cpu_parange;
>>>> * cpu_parange for later reference */ static inline unsigned int
>>>> get_cpu_parange(void) {
>>>> +
>>>> +#ifdef CONFIG_CPU_PARANGE
>>>> + return CONFIG_CPU_PARANGE;
>>>> +#else
>>>> unsigned long id_aa64mmfr0;
>>>>
>>>> arm_read_sysreg(ID_AA64MMFR0_EL1, id_aa64mmfr0); @@ -203,6
>>> +207,7 @@
>>>> static inline unsigned int get_cpu_parange(void)
>>>> default:
>>>> return 0;
>>>> }
>>>> +#endif
>>>
>>> I dislike compile-time configurations like this. How about making this a
>>> platform
>>> parameter? If it's 0, we continue reading it from the first CPU we
>>> initialize,
>>> otherwise we use that system config parameter.
>>
>> You mean adding an entry in root cell file as the following?
>> .platform_info = {
>> .arm = {
>> Parange = 40,
>
> Yes, along this path.
>
> Jan
>
--
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.
