> -----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?
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
--
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.
