| Message ID | 20210924082542.2766170-2-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Assorted vgic-v3 fixes | expand |
Hi Marc, On 9/24/21 09:25, Marc Zyngier wrote: > Until now, we always let ID_AA64PFR0_EL1.GIC reflect the value > visible on the host, even if we were running a GICv2-enabled VM > on a GICv3+compat host. > > That's fine, but we also now have the case of a host that does not > expose ID_AA64PFR0_EL1.GIC==1 despite having a vGIC. Yes, this is > confusing. Thank you M1. > > Let's go back to first principles and expose ID_AA64PFR0_EL1.GIC=1 > when a GICv3 is exposed to the guest. This also hides a GICv4.1 > CPU interface from the guest which has no business knowing about > the v4.1 extension. Had a look at the gic-v3 driver, and as far as I can tell it does not check that a GICv3 is advertised in ID_AA64PFR0_EL1. If I didn't get this wrong, then this patch is to ensure architectural compliance for a guest even if the hardware is not necessarily compliant, right? GICv4.1 is an extension to GICv4 (which itself was an extension to GICv3) to add support for virtualization features (virtual SGIs), so I don't see any harm in hiding it from the guest, since the guest cannot virtual SGIs. Thanks, Alex > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/kvm/sys_regs.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 1d46e185f31e..0e8fc29df19c 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1075,6 +1075,11 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, > val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2), (u64)vcpu->kvm->arch.pfr0_csv2); > val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3); > val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3), (u64)vcpu->kvm->arch.pfr0_csv3); > + if (irqchip_in_kernel(vcpu->kvm) && > + vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) { > + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); > + val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), 1); > + } > break; > case SYS_ID_AA64PFR1_EL1: > val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_MTE);
Hi Alex, On Wed, 29 Sep 2021 16:29:09 +0100, Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > Hi Marc, > > On 9/24/21 09:25, Marc Zyngier wrote: > > Until now, we always let ID_AA64PFR0_EL1.GIC reflect the value > > visible on the host, even if we were running a GICv2-enabled VM > > on a GICv3+compat host. > > > > That's fine, but we also now have the case of a host that does not > > expose ID_AA64PFR0_EL1.GIC==1 despite having a vGIC. Yes, this is > > confusing. Thank you M1. > > > > Let's go back to first principles and expose ID_AA64PFR0_EL1.GIC=1 > > when a GICv3 is exposed to the guest. This also hides a GICv4.1 > > CPU interface from the guest which has no business knowing about > > the v4.1 extension. > > Had a look at the gic-v3 driver, and as far as I can tell it does > not check that a GICv3 is advertised in ID_AA64PFR0_EL1. If I didn't > get this wrong, then this patch is to ensure architectural > compliance for a guest even if the hardware is not necessarily > compliant, right? Indeed. Not having this made some of my own tests fail on M1 as they rely on ID_AA64PFR0_EL1.GIC being correct. I also pondered setting it to 0 when emulating a GICv2, but that'd be a change in behaviour, and I want to think a bit more about the effects of that. > > GICv4.1 is an extension to GICv4 (which itself was an extension to > GICv3) to add support for virtualization features (virtual SGIs), so > I don't see any harm in hiding it from the guest, since the guest > cannot virtual SGIs. Indeed. The guest already has another way to look into this by checking whether the distributor allows active-less SGIs. Thanks, M.
Hi Marc, On 9/29/21 17:04, Marc Zyngier wrote: > Hi Alex, > > On Wed, 29 Sep 2021 16:29:09 +0100, > Alexandru Elisei <alexandru.elisei@arm.com> wrote: >> Hi Marc, >> >> On 9/24/21 09:25, Marc Zyngier wrote: >>> Until now, we always let ID_AA64PFR0_EL1.GIC reflect the value >>> visible on the host, even if we were running a GICv2-enabled VM >>> on a GICv3+compat host. >>> >>> That's fine, but we also now have the case of a host that does not >>> expose ID_AA64PFR0_EL1.GIC==1 despite having a vGIC. Yes, this is >>> confusing. Thank you M1. >>> >>> Let's go back to first principles and expose ID_AA64PFR0_EL1.GIC=1 >>> when a GICv3 is exposed to the guest. This also hides a GICv4.1 >>> CPU interface from the guest which has no business knowing about >>> the v4.1 extension. >> Had a look at the gic-v3 driver, and as far as I can tell it does >> not check that a GICv3 is advertised in ID_AA64PFR0_EL1. If I didn't >> get this wrong, then this patch is to ensure architectural >> compliance for a guest even if the hardware is not necessarily >> compliant, right? > Indeed. Not having this made some of my own tests fail on M1 as they > rely on ID_AA64PFR0_EL1.GIC being correct. I also pondered setting it > to 0 when emulating a GICv2, but that'd be a change in behaviour, and > I want to think a bit more about the effects of that. > >> GICv4.1 is an extension to GICv4 (which itself was an extension to >> GICv3) to add support for virtualization features (virtual SGIs), so >> I don't see any harm in hiding it from the guest, since the guest >> cannot virtual SGIs. > Indeed. The guest already has another way to look into this by > checking whether the distributor allows active-less SGIs. Thank you for the clarification, the patch looks good to me: Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com> Thanks, Alex > > Thanks, > > M. >
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 1d46e185f31e..0e8fc29df19c 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1075,6 +1075,11 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV2), (u64)vcpu->kvm->arch.pfr0_csv2); val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3); val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_CSV3), (u64)vcpu->kvm->arch.pfr0_csv3); + if (irqchip_in_kernel(vcpu->kvm) && + vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) { + val &= ~ARM64_FEATURE_MASK(ID_AA64PFR0_GIC); + val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64PFR0_GIC), 1); + } break; case SYS_ID_AA64PFR1_EL1: val &= ~ARM64_FEATURE_MASK(ID_AA64PFR1_MTE);
Until now, we always let ID_AA64PFR0_EL1.GIC reflect the value visible on the host, even if we were running a GICv2-enabled VM on a GICv3+compat host. That's fine, but we also now have the case of a host that does not expose ID_AA64PFR0_EL1.GIC==1 despite having a vGIC. Yes, this is confusing. Thank you M1. Let's go back to first principles and expose ID_AA64PFR0_EL1.GIC=1 when a GICv3 is exposed to the guest. This also hides a GICv4.1 CPU interface from the guest which has no business knowing about the v4.1 extension. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/kvm/sys_regs.c | 5 +++++ 1 file changed, 5 insertions(+)