Messages in this thread |  | | Date | Sun, 11 Dec 2022 14:25:31 +0900 | Subject | Re: [PATCH 0/3] KVM: arm64: Handle CCSIDR associativity mismatches | From | Akihiko Odaki <> |
| |
On 2022/12/04 23:57, Marc Zyngier wrote: > On Fri, 02 Dec 2022 09:55:24 +0000, > Akihiko Odaki <akihiko.odaki@gmail.com> wrote: >> >> On 2022/12/02 18:40, Marc Zyngier wrote: >>> On Fri, 02 Dec 2022 05:17:12 +0000, >>> Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >>>> >>>>>> On M2 MacBook Air, I have seen no other difference in standard ID >>>>>> registers and CCSIDRs are exceptions. Perhaps Apple designed this way >>>>>> so that macOS's Hypervisor can freely migrate vCPU, but I can't assure >>>>>> that without more analysis. This is still enough to migrate vCPU >>>>>> running Linux at least. >>>>> >>>>> I guess that MacOS hides more of the underlying HW than KVM does. And >>>>> KVM definitely doesn't hide the MIDR_EL1 registers, which *are* >>>>> different between the two clusters. >>>> >>>> It seems KVM stores a MIDR value of a CPU and reuse it as "invariant" >>>> value for ioctls while it exposes the MIDR value each physical CPU >>>> owns to vCPU. >>> >>> This only affects the VMM though, and not the guest which sees the >>> MIDR of the CPU it runs on. The problem is that at short of pinning >>> the vcpus, you don't know where they will run. So any value is fair >>> game. >> >> Yes, my concern is that VMM can be confused if it sees something >> different from what the guest on the vCPU sees. > > Well, this has been part of the ABI for about 10 years, since Rusty > introduced this notion of invariant, so userspace is already working > around it if that's an actual issue.
In that case, I think it is better to document that the interface is not working properly and deprecated.
> > This would be easily addressed though, and shouldn't result in any > issue. The following should do the trick (only lightly tested on an > M1).
This can be problematic when restoring vcpu state saved with the old kernel. A possible solution is to allow the userspace to overwrite MIDR_EL1 as proposed for CCSIDR_EL1.
Regards, Akihiko Odaki
> > Thanks, > > M. > > From f1caacb89eb8ae40dc38669160a2f081f87f4b15 Mon Sep 17 00:00:00 2001 > From: Marc Zyngier <maz@kernel.org> > Date: Sun, 4 Dec 2022 14:22:22 +0000 > Subject: [PATCH] KVM: arm64: Return MIDR_EL1 to userspace as seen on the vcpu > thread > > When booting, KVM sample the MIDR of the CPU it initialises on, > and keep this as the value that will forever be exposed to userspace. > > However, this has nothing to do with the value that the guest will > see. On an asymetric system, this can result in userspace observing > weird things, specially if it has pinned the vcpus on a *different* > set of CPUs. > > Instead, return the MIDR value for the vpcu we're currently on and > that the vcpu will observe if it has been pinned onto that CPU. > > For symmetric systems, this changes nothing. For asymmetric machines, > they will observe the correct MIDR value at the point of the call. > > Reported-by: Akihiko Odaki <akihiko.odaki@gmail.com> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/kvm/sys_regs.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index f4a7c5abcbca..f6bcf8ba9b2e 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1246,6 +1246,22 @@ static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > return 0; > } > > +static int get_midr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > + u64 *val) > +{ > + *val = read_sysreg(midr_el1); > + return 0; > +} > + > +static int set_midr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > + u64 val) > +{ > + if (val != read_sysreg(midr_el1)) > + return -EINVAL; > + > + return 0; > +} > + > static int get_raz_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd, > u64 *val) > { > @@ -1432,6 +1448,7 @@ static const struct sys_reg_desc sys_reg_descs[] = { > > { SYS_DESC(SYS_DBGVCR32_EL2), NULL, reset_val, DBGVCR32_EL2, 0 }, > > + { SYS_DESC(SYS_MIDR_EL1), .get_user = get_midr, .set_user = set_midr }, > { SYS_DESC(SYS_MPIDR_EL1), NULL, reset_mpidr, MPIDR_EL1 }, > > /* > @@ -2609,7 +2626,6 @@ id_to_sys_reg_desc(struct kvm_vcpu *vcpu, u64 id, > ((struct sys_reg_desc *)r)->val = read_sysreg(reg); \ > } > > -FUNCTION_INVARIANT(midr_el1) > FUNCTION_INVARIANT(revidr_el1) > FUNCTION_INVARIANT(clidr_el1) > FUNCTION_INVARIANT(aidr_el1) > @@ -2621,7 +2637,6 @@ static void get_ctr_el0(struct kvm_vcpu *v, const struct sys_reg_desc *r) > > /* ->val is filled in by kvm_sys_reg_table_init() */ > static struct sys_reg_desc invariant_sys_regs[] = { > - { SYS_DESC(SYS_MIDR_EL1), NULL, get_midr_el1 }, > { SYS_DESC(SYS_REVIDR_EL1), NULL, get_revidr_el1 }, > { SYS_DESC(SYS_CLIDR_EL1), NULL, get_clidr_el1 }, > { SYS_DESC(SYS_AIDR_EL1), NULL, get_aidr_el1 },
|  |