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.
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;
+
+ for (i = 0; i < hypervisor_header.online_cpus; i++) {
+ if ((per_cpu(i)->id_aa64mmfr0 & 0xf) < parange)
+ parange = per_cpu(i)->id_aa64mmfr0 & 0xf;
+ }
+
+ 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 (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.
>
> 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.
