| Message ID | 20201109113233.9012-8-dbrazdil@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Opt-in always-on nVHE hypervisor | expand |
On 2020-11-09 11:32, David Brazdil wrote: > When KVM starts validating host's PSCI requests, it will need to map > MPIDR back to the CPU ID. To this end, copy cpu_logical_map into nVHE > hyp memory when KVM is initialized. > > Only copy the information for CPUs that are online at the point of KVM > initialization so that KVM rejects CPUs whose features were not checked > against the finalized capabilities. > > Signed-off-by: David Brazdil <dbrazdil@google.com> > --- > arch/arm64/kvm/arm.c | 17 +++++++++++++++++ > arch/arm64/kvm/hyp/nvhe/percpu.c | 16 ++++++++++++++++ > 2 files changed, 33 insertions(+) > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index 9ba9db2aa7f8..b85b4294b72d 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -1481,6 +1481,21 @@ static inline void hyp_cpu_pm_exit(void) > } > #endif > > +static void init_cpu_logical_map(void) > +{ > + extern u64 kvm_nvhe_sym(__cpu_logical_map)[NR_CPUS]; > + int cpu; > + > + /* > + * Copy the MPIDR <-> logical CPU ID mapping to hyp. > + * Only copy the set of online CPUs whose features have been chacked > + * against the finalized system capabilities. The hypervisor will not > + * allow any other CPUs from the `possible` set to boot. > + */ > + for_each_online_cpu(cpu) > + CHOOSE_NVHE_SYM(__cpu_logical_map)[cpu] = cpu_logical_map(cpu); > +} > + > static int init_common_resources(void) > { > return kvm_set_ipa_limit(); > @@ -1659,6 +1674,8 @@ static int init_hyp_mode(void) > } > } > > + init_cpu_logical_map(); > + > return 0; > > out_err: > diff --git a/arch/arm64/kvm/hyp/nvhe/percpu.c > b/arch/arm64/kvm/hyp/nvhe/percpu.c > index 5fd0c5696907..d0b9dbc2df45 100644 > --- a/arch/arm64/kvm/hyp/nvhe/percpu.c > +++ b/arch/arm64/kvm/hyp/nvhe/percpu.c > @@ -8,6 +8,22 @@ > #include <asm/kvm_hyp.h> > #include <asm/kvm_mmu.h> > > +/* > + * nVHE copy of data structures tracking available CPU cores. > + * Only entries for CPUs that were online at KVM init are populated. > + * Other CPUs should not be allowed to boot because their features > were > + * not checked against the finalized system capabilities. > + */ > +u64 __ro_after_init __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] > = INVALID_HWID }; I'm not sure what __ro_after_init means once we get S2 isolation. > + > +u64 cpu_logical_map(int cpu) nit: is there any reason why "cpu" cannot be unsigned? The thought of a negative CPU number makes me shiver... > +{ > + if (cpu < 0 || cpu >= ARRAY_SIZE(__cpu_logical_map)) > + hyp_panic(); > + > + return __cpu_logical_map[cpu]; > +} > + > unsigned long __hyp_per_cpu_offset(unsigned int cpu) > { > unsigned long *cpu_base_array; Overall, this patch would make more sense closer it its use case (in patch 19). I also don't understand why this lives in percpu.c... Thanks, M.
> > +/* > > + * nVHE copy of data structures tracking available CPU cores. > > + * Only entries for CPUs that were online at KVM init are populated. > > + * Other CPUs should not be allowed to boot because their features were > > + * not checked against the finalized system capabilities. > > + */ > > +u64 __ro_after_init __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] > > = INVALID_HWID }; > > I'm not sure what __ro_after_init means once we get S2 isolation. It is stretching the definition of 'init' a bit, I know, but I don't see what your worry is about S2? The intention is to mark this read-only for .hyp.text at runtime. With S2, the host won't be able to write to it after KVM init. Obviously that's currently not the case. One thing we might change in the future is marking it RW for some initial setup in a HVC handler, then marking it RO for the rest of uptime. > > > + > > +u64 cpu_logical_map(int cpu) > > nit: is there any reason why "cpu" cannot be unsigned? The thought > of a negative CPU number makes me shiver... Same here. That's how it's defined in kernel proper, so I went with that. > > > +{ > > + if (cpu < 0 || cpu >= ARRAY_SIZE(__cpu_logical_map)) > > + hyp_panic(); > > + > > + return __cpu_logical_map[cpu]; > > +} > > + > > unsigned long __hyp_per_cpu_offset(unsigned int cpu) > > { > > unsigned long *cpu_base_array; > > Overall, this patch would make more sense closer it its use case > (in patch 19). I also don't understand why this lives in percpu.c... I didn't think it called for adding another C file for this. How about we rename this file to smp.c? Would that make sense for both?
On 2020-11-11 13:03, David Brazdil wrote: >> > +/* >> > + * nVHE copy of data structures tracking available CPU cores. >> > + * Only entries for CPUs that were online at KVM init are populated. >> > + * Other CPUs should not be allowed to boot because their features were >> > + * not checked against the finalized system capabilities. >> > + */ >> > +u64 __ro_after_init __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] >> > = INVALID_HWID }; >> >> I'm not sure what __ro_after_init means once we get S2 isolation. > > It is stretching the definition of 'init' a bit, I know, but I don't > see what > your worry is about S2? The intention is to mark this read-only for > .hyp.text > at runtime. With S2, the host won't be able to write to it after KVM > init. > Obviously that's currently not the case. More importantly, EL2 can write to it at any time, which is the bit I'm worried about, as it makes the annotation misleading. > One thing we might change in the future is marking it RW for some > initial > setup in a HVC handler, then marking it RO for the rest of uptime. That'd be a desirable outcome, and it would be consistent with the rest of the kernel. > >> >> > + >> > +u64 cpu_logical_map(int cpu) >> >> nit: is there any reason why "cpu" cannot be unsigned? The thought >> of a negative CPU number makes me shiver... > > Same here. That's how it's defined in kernel proper, so I went with > that. I'm happy to deviate from the kernel (give the function a different name if this clashes with existing include files). We can also fix the rest of the kernel (I've just written the trivial patch). >> >> > +{ >> > + if (cpu < 0 || cpu >= ARRAY_SIZE(__cpu_logical_map)) >> > + hyp_panic(); >> > + >> > + return __cpu_logical_map[cpu]; >> > +} >> > + >> > unsigned long __hyp_per_cpu_offset(unsigned int cpu) >> > { >> > unsigned long *cpu_base_array; >> >> Overall, this patch would make more sense closer it its use case >> (in patch 19). I also don't understand why this lives in percpu.c... > > I didn't think it called for adding another C file for this. How about > we > rename this file to smp.c? Would that make sense for both? Make that hyp-smp.c, please! M.
On Wed, Nov 11, 2020 at 01:29:29PM +0000, Marc Zyngier wrote: > On 2020-11-11 13:03, David Brazdil wrote: > > > > +/* > > > > + * nVHE copy of data structures tracking available CPU cores. > > > > + * Only entries for CPUs that were online at KVM init are populated. > > > > + * Other CPUs should not be allowed to boot because their features were > > > > + * not checked against the finalized system capabilities. > > > > + */ > > > > +u64 __ro_after_init __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] > > > > = INVALID_HWID }; > > > > > > I'm not sure what __ro_after_init means once we get S2 isolation. > > > > It is stretching the definition of 'init' a bit, I know, but I don't see > > what > > your worry is about S2? The intention is to mark this read-only for > > .hyp.text > > at runtime. With S2, the host won't be able to write to it after KVM > > init. > > Obviously that's currently not the case. > > More importantly, EL2 can write to it at any time, which is the bit I'm > worried > about, as it makes the annotation misleading. EL2 can't, at least not accidentally. The hyp memory mapping is PAGE_HYP_RO (see patch 05). Does this annotation have stronger guarantees in EL1? AFAICT, those variables are made PAGE_KERNEL_RO in mark_rodata_ro(). > > > One thing we might change in the future is marking it RW for some > > initial > > setup in a HVC handler, then marking it RO for the rest of uptime. > > That'd be a desirable outcome, and it would be consistent with the rest > of the kernel. > > > > > > > > > > + > > > > +u64 cpu_logical_map(int cpu) > > > > > > nit: is there any reason why "cpu" cannot be unsigned? The thought > > > of a negative CPU number makes me shiver... > > > > Same here. That's how it's defined in kernel proper, so I went with > > that. > > I'm happy to deviate from the kernel (give the function a different name > if this clashes with existing include files). > > We can also fix the rest of the kernel (I've just written the trivial > patch). Shouldn't clash with include files. Where fixing the kernel might clash is all the users of for_each_*_cpu that use an int for the iterator var.
On 2020-11-11 13:45, David Brazdil wrote: > On Wed, Nov 11, 2020 at 01:29:29PM +0000, Marc Zyngier wrote: >> On 2020-11-11 13:03, David Brazdil wrote: >> > > > +/* >> > > > + * nVHE copy of data structures tracking available CPU cores. >> > > > + * Only entries for CPUs that were online at KVM init are populated. >> > > > + * Other CPUs should not be allowed to boot because their features were >> > > > + * not checked against the finalized system capabilities. >> > > > + */ >> > > > +u64 __ro_after_init __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] >> > > > = INVALID_HWID }; >> > > >> > > I'm not sure what __ro_after_init means once we get S2 isolation. >> > >> > It is stretching the definition of 'init' a bit, I know, but I don't see >> > what >> > your worry is about S2? The intention is to mark this read-only for >> > .hyp.text >> > at runtime. With S2, the host won't be able to write to it after KVM >> > init. >> > Obviously that's currently not the case. >> >> More importantly, EL2 can write to it at any time, which is the bit >> I'm >> worried >> about, as it makes the annotation misleading. > > EL2 can't, at least not accidentally. The hyp memory mapping is > PAGE_HYP_RO > (see patch 05). Ah, I obviously overlooked that. Thanks for setting me straight. > Shouldn't clash with include files. Where fixing the kernel might clash > is > all the users of for_each_*_cpu that use an int for the iterator var. I don't think that's a problem (nobody expects that many CPUs). But if you are confident that we don't have a problem, no need to change the kernel itself. M.
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 9ba9db2aa7f8..b85b4294b72d 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -1481,6 +1481,21 @@ static inline void hyp_cpu_pm_exit(void) } #endif +static void init_cpu_logical_map(void) +{ + extern u64 kvm_nvhe_sym(__cpu_logical_map)[NR_CPUS]; + int cpu; + + /* + * Copy the MPIDR <-> logical CPU ID mapping to hyp. + * Only copy the set of online CPUs whose features have been chacked + * against the finalized system capabilities. The hypervisor will not + * allow any other CPUs from the `possible` set to boot. + */ + for_each_online_cpu(cpu) + CHOOSE_NVHE_SYM(__cpu_logical_map)[cpu] = cpu_logical_map(cpu); +} + static int init_common_resources(void) { return kvm_set_ipa_limit(); @@ -1659,6 +1674,8 @@ static int init_hyp_mode(void) } } + init_cpu_logical_map(); + return 0; out_err: diff --git a/arch/arm64/kvm/hyp/nvhe/percpu.c b/arch/arm64/kvm/hyp/nvhe/percpu.c index 5fd0c5696907..d0b9dbc2df45 100644 --- a/arch/arm64/kvm/hyp/nvhe/percpu.c +++ b/arch/arm64/kvm/hyp/nvhe/percpu.c @@ -8,6 +8,22 @@ #include <asm/kvm_hyp.h> #include <asm/kvm_mmu.h> +/* + * nVHE copy of data structures tracking available CPU cores. + * Only entries for CPUs that were online at KVM init are populated. + * Other CPUs should not be allowed to boot because their features were + * not checked against the finalized system capabilities. + */ +u64 __ro_after_init __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = INVALID_HWID }; + +u64 cpu_logical_map(int cpu) +{ + if (cpu < 0 || cpu >= ARRAY_SIZE(__cpu_logical_map)) + hyp_panic(); + + return __cpu_logical_map[cpu]; +} + unsigned long __hyp_per_cpu_offset(unsigned int cpu) { unsigned long *cpu_base_array;
When KVM starts validating host's PSCI requests, it will need to map MPIDR back to the CPU ID. To this end, copy cpu_logical_map into nVHE hyp memory when KVM is initialized. Only copy the information for CPUs that are online at the point of KVM initialization so that KVM rejects CPUs whose features were not checked against the finalized capabilities. Signed-off-by: David Brazdil <dbrazdil@google.com> --- arch/arm64/kvm/arm.c | 17 +++++++++++++++++ arch/arm64/kvm/hyp/nvhe/percpu.c | 16 ++++++++++++++++ 2 files changed, 33 insertions(+)