| Message ID | 20240820100349.3544850-4-maz@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: arm64: Handle the lack of GICv3 exposed to a guest | expand |
Hey, On Tue, Aug 20, 2024 at 11:03:40AM +0100, Marc Zyngier wrote: > We so far only write the ICH_HCR_EL2 config in two situations: > > - when we need to emulate the GICv3 CPU interface due to HW bugs > > - when we do direct injection, as the virtual CPU interface needs > to be enabled > > This is all good. But it also means that we don't do anything special > when we emulate a GICv2, or that there is no GIC at all. > > What happens in this case when the guest uses the GICv3 system > registers? The *guest* gets a trap for a sysreg access (EC=0x18) > while we'd really like it to get an UNDEF. > > Fixing this is a bit involved: > > - we need to set all the required trap bits (TC, TALL0, TALL1, TDIR) > > - for these traps to take effect, we need to (counter-intuitively) > set ICC_SRE_EL1.SRE to 1 so that the above traps take priority. > > Note that doesn't fully work when GICv2 emulation is enabled, as > we cannot set ICC_SRE_EL1.SRE to 1 (it breaks Group0 delivery as > IRQ). Just to make sure I'm following completely, GICv2-on-GICv3 guest sees the (barf) architected behavior of sysreg traps going to EL1. > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/kvm/hyp/vgic-v3-sr.c | 22 ++++++++++++++++------ > arch/arm64/kvm/vgic/vgic-v3.c | 5 ++++- > 2 files changed, 20 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c > index 7b397fad26f2..a184def8f5ad 100644 > --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c > +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c > @@ -268,8 +268,16 @@ void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if) > * starting to mess with the rest of the GIC, and VMCR_EL2 in > * particular. This logic must be called before > * __vgic_v3_restore_state(). > + * > + * However, if the vgic is disabled (ICH_HCR_EL2.EN==0), no GIC is > + * provisionned at all. In order to prevent illegal accesses to the typo: provisioned > + * system registers to trap to EL1 (duh), force ICC_SRE_EL1.SRE to 1 > + * so that the trap bits can take effect. Yes, we *loves* the GIC. > */ > - if (!cpu_if->vgic_sre) { > + if (!(cpu_if->vgic_hcr & ICH_HCR_EN)) { > + write_gicreg(ICC_SRE_EL1_SRE, ICC_SRE_EL1); > + isb(); > + } else if (!cpu_if->vgic_sre) { > write_gicreg(0, ICC_SRE_EL1); > isb(); > write_gicreg(cpu_if->vgic_vmcr, ICH_VMCR_EL2); > @@ -288,8 +296,9 @@ void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if) > } > > /* > - * Prevent the guest from touching the GIC system registers if > - * SRE isn't enabled for GICv3 emulation. > + * Prevent the guest from touching the ICC_SRE_EL1 system > + * register. Note that this may not have any effect, as > + * ICC_SRE_EL2.Enable being RAO/WI is a valid implementation. So this behavior is weird but still 'safe' as, ICC_SRE_EL1.SRE is also RAO/WI and the HCR traps are still effective. Right? > */ > write_gicreg(read_gicreg(ICC_SRE_EL2) & ~ICC_SRE_EL2_ENABLE, > ICC_SRE_EL2); > @@ -297,10 +306,11 @@ void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if) > /* > * If we need to trap system registers, we must write > * ICH_HCR_EL2 anyway, even if no interrupts are being > - * injected, > + * injected. Note that this also applies if we don't expect > + * any system register access (GICv2 or no vgic at all). We don't expect the traps to come in the GICv2 case, though, right? Looks alright to me otherwise, but blech!
On Wed, 21 Aug 2024 00:19:32 +0100, Oliver Upton <oliver.upton@linux.dev> wrote: > > Hey, > > On Tue, Aug 20, 2024 at 11:03:40AM +0100, Marc Zyngier wrote: > > We so far only write the ICH_HCR_EL2 config in two situations: > > > > - when we need to emulate the GICv3 CPU interface due to HW bugs > > > > - when we do direct injection, as the virtual CPU interface needs > > to be enabled > > > > This is all good. But it also means that we don't do anything special > > when we emulate a GICv2, or that there is no GIC at all. > > > > What happens in this case when the guest uses the GICv3 system > > registers? The *guest* gets a trap for a sysreg access (EC=0x18) > > while we'd really like it to get an UNDEF. > > > > Fixing this is a bit involved: > > > > - we need to set all the required trap bits (TC, TALL0, TALL1, TDIR) > > > > - for these traps to take effect, we need to (counter-intuitively) > > set ICC_SRE_EL1.SRE to 1 so that the above traps take priority. > > > > Note that doesn't fully work when GICv2 emulation is enabled, as > > we cannot set ICC_SRE_EL1.SRE to 1 (it breaks Group0 delivery as > > IRQ). > > Just to make sure I'm following completely, GICv2-on-GICv3 guest sees > the (barf) architected behavior of sysreg traps going to EL1. Indeed. There isn't anything we can do about that, short of changing the behaviour of GICv2 emulation. I'll add something to that effect in the commit message. > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > arch/arm64/kvm/hyp/vgic-v3-sr.c | 22 ++++++++++++++++------ > > arch/arm64/kvm/vgic/vgic-v3.c | 5 ++++- > > 2 files changed, 20 insertions(+), 7 deletions(-) > > > > diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c > > index 7b397fad26f2..a184def8f5ad 100644 > > --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c > > +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c > > @@ -268,8 +268,16 @@ void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if) > > * starting to mess with the rest of the GIC, and VMCR_EL2 in > > * particular. This logic must be called before > > * __vgic_v3_restore_state(). > > + * > > + * However, if the vgic is disabled (ICH_HCR_EL2.EN==0), no GIC is > > + * provisionned at all. In order to prevent illegal accesses to the > > typo: provisioned > > > + * system registers to trap to EL1 (duh), force ICC_SRE_EL1.SRE to 1 > > + * so that the trap bits can take effect. Yes, we *loves* the GIC. > > */ > > - if (!cpu_if->vgic_sre) { > > + if (!(cpu_if->vgic_hcr & ICH_HCR_EN)) { > > + write_gicreg(ICC_SRE_EL1_SRE, ICC_SRE_EL1); > > + isb(); > > + } else if (!cpu_if->vgic_sre) { > > write_gicreg(0, ICC_SRE_EL1); > > isb(); > > write_gicreg(cpu_if->vgic_vmcr, ICH_VMCR_EL2); > > @@ -288,8 +296,9 @@ void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if) > > } > > > > /* > > - * Prevent the guest from touching the GIC system registers if > > - * SRE isn't enabled for GICv3 emulation. > > + * Prevent the guest from touching the ICC_SRE_EL1 system > > + * register. Note that this may not have any effect, as > > + * ICC_SRE_EL2.Enable being RAO/WI is a valid implementation. > > So this behavior is weird but still 'safe' as, ICC_SRE_EL1.SRE is also RAO/WI > and the HCR traps are still effective. Right? Exactly. All the traps are working, *except* for ICC_SRE_EL1 itself. A guest can then infer that it runs under a hypervisor, but not much more. > > > */ > > write_gicreg(read_gicreg(ICC_SRE_EL2) & ~ICC_SRE_EL2_ENABLE, > > ICC_SRE_EL2); > > @@ -297,10 +306,11 @@ void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if) > > /* > > * If we need to trap system registers, we must write > > * ICH_HCR_EL2 anyway, even if no interrupts are being > > - * injected, > > + * injected. Note that this also applies if we don't expect > > + * any system register access (GICv2 or no vgic at all). > > We don't expect the traps to come in the GICv2 case, though, right? Yup. I'll fix the comment, as it is misleading. > Looks alright to me otherwise, but blech! Yeah. It's the sort of code that makes you feel dirty just looking at it. Thanks, M.
diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c index 7b397fad26f2..a184def8f5ad 100644 --- a/arch/arm64/kvm/hyp/vgic-v3-sr.c +++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c @@ -268,8 +268,16 @@ void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if) * starting to mess with the rest of the GIC, and VMCR_EL2 in * particular. This logic must be called before * __vgic_v3_restore_state(). + * + * However, if the vgic is disabled (ICH_HCR_EL2.EN==0), no GIC is + * provisionned at all. In order to prevent illegal accesses to the + * system registers to trap to EL1 (duh), force ICC_SRE_EL1.SRE to 1 + * so that the trap bits can take effect. Yes, we *loves* the GIC. */ - if (!cpu_if->vgic_sre) { + if (!(cpu_if->vgic_hcr & ICH_HCR_EN)) { + write_gicreg(ICC_SRE_EL1_SRE, ICC_SRE_EL1); + isb(); + } else if (!cpu_if->vgic_sre) { write_gicreg(0, ICC_SRE_EL1); isb(); write_gicreg(cpu_if->vgic_vmcr, ICH_VMCR_EL2); @@ -288,8 +296,9 @@ void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if) } /* - * Prevent the guest from touching the GIC system registers if - * SRE isn't enabled for GICv3 emulation. + * Prevent the guest from touching the ICC_SRE_EL1 system + * register. Note that this may not have any effect, as + * ICC_SRE_EL2.Enable being RAO/WI is a valid implementation. */ write_gicreg(read_gicreg(ICC_SRE_EL2) & ~ICC_SRE_EL2_ENABLE, ICC_SRE_EL2); @@ -297,10 +306,11 @@ void __vgic_v3_activate_traps(struct vgic_v3_cpu_if *cpu_if) /* * If we need to trap system registers, we must write * ICH_HCR_EL2 anyway, even if no interrupts are being - * injected, + * injected. Note that this also applies if we don't expect + * any system register access (GICv2 or no vgic at all). */ if (static_branch_unlikely(&vgic_v3_cpuif_trap) || - cpu_if->its_vpe.its_vm) + cpu_if->its_vpe.its_vm || !cpu_if->vgic_sre) write_gicreg(cpu_if->vgic_hcr, ICH_HCR_EL2); } @@ -326,7 +336,7 @@ void __vgic_v3_deactivate_traps(struct vgic_v3_cpu_if *cpu_if) * no interrupts were being injected, and we disable it again here. */ if (static_branch_unlikely(&vgic_v3_cpuif_trap) || - cpu_if->its_vpe.its_vm) + cpu_if->its_vpe.its_vm || !cpu_if->vgic_sre) write_gicreg(0, ICH_HCR_EL2); } diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c index 11718412921f..b217b256853c 100644 --- a/arch/arm64/kvm/vgic/vgic-v3.c +++ b/arch/arm64/kvm/vgic/vgic-v3.c @@ -298,8 +298,11 @@ void vcpu_set_ich_hcr(struct kvm_vcpu *vcpu) { struct vgic_v3_cpu_if *vgic_v3 = &vcpu->arch.vgic_cpu.vgic_v3; - if (!kvm_has_gicv3(vcpu->kvm)) + /* Hide GICv3 sysreg if necessary */ + if (!kvm_has_gicv3(vcpu->kvm)) { + vgic_v3->vgic_hcr |= ICH_HCR_TALL0 | ICH_HCR_TALL1 | ICH_HCR_TC; return; + } if (group0_trap) vgic_v3->vgic_hcr |= ICH_HCR_TALL0;
We so far only write the ICH_HCR_EL2 config in two situations: - when we need to emulate the GICv3 CPU interface due to HW bugs - when we do direct injection, as the virtual CPU interface needs to be enabled This is all good. But it also means that we don't do anything special when we emulate a GICv2, or that there is no GIC at all. What happens in this case when the guest uses the GICv3 system registers? The *guest* gets a trap for a sysreg access (EC=0x18) while we'd really like it to get an UNDEF. Fixing this is a bit involved: - we need to set all the required trap bits (TC, TALL0, TALL1, TDIR) - for these traps to take effect, we need to (counter-intuitively) set ICC_SRE_EL1.SRE to 1 so that the above traps take priority. Note that doesn't fully work when GICv2 emulation is enabled, as we cannot set ICC_SRE_EL1.SRE to 1 (it breaks Group0 delivery as IRQ). Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/kvm/hyp/vgic-v3-sr.c | 22 ++++++++++++++++------ arch/arm64/kvm/vgic/vgic-v3.c | 5 ++++- 2 files changed, 20 insertions(+), 7 deletions(-)