| Message ID | 20240820100349.3544850-7-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Handle the lack of GICv3 exposed to a guest | expand |
On Tue, Aug 20, 2024 at 11:03:43AM +0100, Marc Zyngier wrote: > In order to be consistent, we shouldn't advertise a GICv3 when none > is actually usable by the guest. > > Wipe the feature when these conditions apply, and allow the field > to be written from userspace. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/kvm/sys_regs.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index bc2d54da3827..7d00d7e359e1 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -2365,7 +2365,6 @@ static const struct sys_reg_desc sys_reg_descs[] = { > ID_AA64PFR0_EL1_MPAM | > ID_AA64PFR0_EL1_SVE | > ID_AA64PFR0_EL1_RAS | > - ID_AA64PFR0_EL1_GIC | > ID_AA64PFR0_EL1_AdvSIMD | > ID_AA64PFR0_EL1_FP), }, > ID_SANITISED(ID_AA64PFR1_EL1), > @@ -4634,6 +4633,11 @@ int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu) > > guard(mutex)(&kvm->arch.config_lock); > > + if (!kvm_has_gicv3(kvm)) { > + kvm->arch.id_regs[IDREG_IDX(SYS_ID_AA64PFR0_EL1)] &= ~ID_AA64PFR0_EL1_GIC_MASK; > + kvm->arch.id_regs[IDREG_IDX(SYS_ID_PFR1_EL1)] &= ~ID_PFR1_EL1_GIC_MASK; > + } > + Hmm, should we use the ID register field as the source of truth for kvm_has_gicv3() at this point? I think what you have in patch #1 makes good sense for a stable backport. Using the ID register from this point forward would make the behavior consistent for a stupid userspace instantiated GICv3 for the VM but clobbered it from the ID register. AFAICT all other usage of kvm_has_gicv3() happens after kvm_finalize_sys_regs(), so it should take this last-minute fixup into account.
On Wed, 21 Aug 2024 00:50:18 +0100, Oliver Upton <oliver.upton@linux.dev> wrote: > > On Tue, Aug 20, 2024 at 11:03:43AM +0100, Marc Zyngier wrote: > > In order to be consistent, we shouldn't advertise a GICv3 when none > > is actually usable by the guest. > > > > Wipe the feature when these conditions apply, and allow the field > > to be written from userspace. > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > arch/arm64/kvm/sys_regs.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > > index bc2d54da3827..7d00d7e359e1 100644 > > --- a/arch/arm64/kvm/sys_regs.c > > +++ b/arch/arm64/kvm/sys_regs.c > > @@ -2365,7 +2365,6 @@ static const struct sys_reg_desc sys_reg_descs[] = { > > ID_AA64PFR0_EL1_MPAM | > > ID_AA64PFR0_EL1_SVE | > > ID_AA64PFR0_EL1_RAS | > > - ID_AA64PFR0_EL1_GIC | > > ID_AA64PFR0_EL1_AdvSIMD | > > ID_AA64PFR0_EL1_FP), }, > > ID_SANITISED(ID_AA64PFR1_EL1), > > @@ -4634,6 +4633,11 @@ int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu) > > > > guard(mutex)(&kvm->arch.config_lock); > > > > + if (!kvm_has_gicv3(kvm)) { > > + kvm->arch.id_regs[IDREG_IDX(SYS_ID_AA64PFR0_EL1)] &= ~ID_AA64PFR0_EL1_GIC_MASK; > > + kvm->arch.id_regs[IDREG_IDX(SYS_ID_PFR1_EL1)] &= ~ID_PFR1_EL1_GIC_MASK; > > + } > > + > > Hmm, should we use the ID register field as the source of truth for > kvm_has_gicv3() at this point? > > I think what you have in patch #1 makes good sense for a stable > backport. Using the ID register from this point forward would make the > behavior consistent for a stupid userspace instantiated GICv3 for the VM > but clobbered it from the ID register. > > AFAICT all other usage of kvm_has_gicv3() happens after > kvm_finalize_sys_regs(), so it should take this last-minute fixup into > account. Right, so changing the definition of kvm_has_gicv3() in this patch, and using the "old" definition here only. Yup, this seems like a reasonable thing to do. Thanks, M.
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index bc2d54da3827..7d00d7e359e1 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -2365,7 +2365,6 @@ static const struct sys_reg_desc sys_reg_descs[] = { ID_AA64PFR0_EL1_MPAM | ID_AA64PFR0_EL1_SVE | ID_AA64PFR0_EL1_RAS | - ID_AA64PFR0_EL1_GIC | ID_AA64PFR0_EL1_AdvSIMD | ID_AA64PFR0_EL1_FP), }, ID_SANITISED(ID_AA64PFR1_EL1), @@ -4634,6 +4633,11 @@ int kvm_finalize_sys_regs(struct kvm_vcpu *vcpu) guard(mutex)(&kvm->arch.config_lock); + if (!kvm_has_gicv3(kvm)) { + kvm->arch.id_regs[IDREG_IDX(SYS_ID_AA64PFR0_EL1)] &= ~ID_AA64PFR0_EL1_GIC_MASK; + kvm->arch.id_regs[IDREG_IDX(SYS_ID_PFR1_EL1)] &= ~ID_PFR1_EL1_GIC_MASK; + } + if (vcpu_has_nv(vcpu)) { int ret = kvm_init_nv_sysregs(kvm); if (ret)
In order to be consistent, we shouldn't advertise a GICv3 when none is actually usable by the guest. Wipe the feature when these conditions apply, and allow the field to be written from userspace. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/kvm/sys_regs.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)