Hi Marc,
On Mon, Mar 14, 2022 at 04:40:44PM +0000, Marc Zyngier wrote:
> Since adversising GICR_CTLR.{IC,CES} is directly observable from
> a guest, we need to make it selectable from userspace.
>
> For that, bump the default GICD_IIDR revision and let userspace
> downgrade it to the previous default. For GICv2, the two distributor
> revisions are strictly equivalent.
>
> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> ---
> arch/arm64/kvm/vgic/vgic-init.c | 7 ++++++-
> arch/arm64/kvm/vgic/vgic-mmio-v2.c | 18 +++++++++++++++---
> arch/arm64/kvm/vgic/vgic-mmio-v3.c | 23 +++++++++++++++++++++--
> include/kvm/arm_vgic.h | 3 +++
> 4 files changed, 45 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index fc00304fe7d8..f84e04f334c6 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -319,7 +319,12 @@ int vgic_init(struct kvm *kvm)
>
> vgic_debug_init(kvm);
>
> - dist->implementation_rev = 2;
> + /*
> + * If userspace didn't set the GIC implementation revision,
> + * default to the latest and greatest. You know want it.
> + */
> + if (!dist->implementation_rev)
> + dist->implementation_rev = KVM_VGIC_IMP_REV_LATEST;
> dist->initialized = true;
>
> out:
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v2.c b/arch/arm64/kvm/vgic/vgic-mmio-v2.c
> index 12e4c223e6b8..f2246c4ca812 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v2.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v2.c
> @@ -73,9 +73,13 @@ static int vgic_mmio_uaccess_write_v2_misc(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val)
> {
> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> + u32 reg;
> +
> switch (addr & 0x0c) {
> case GIC_DIST_IIDR:
> - if (val != vgic_mmio_read_v2_misc(vcpu, addr, len))
> + reg = vgic_mmio_read_v2_misc(vcpu, addr, len);
> + if ((reg ^ val) & ~GICD_IIDR_REVISION_MASK)
> return -EINVAL;
>
> /*
> @@ -87,8 +91,16 @@ static int vgic_mmio_uaccess_write_v2_misc(struct kvm_vcpu *vcpu,
> * migration from old kernels to new kernels with legacy
> * userspace.
> */
> - vcpu->kvm->arch.vgic.v2_groups_user_writable = true;
> - return 0;
> + reg = FIELD_GET(GICD_IIDR_REVISION_MASK, reg);
> + switch (reg) {
> + case KVM_VGIC_IMP_REV_2:
> + case KVM_VGIC_IMP_REV_3:
> + dist->v2_groups_user_writable = true;
Could you eliminate this bool and just pivot off of the implementation
version?
> + dist->implementation_rev = reg;
> + return 0;
> + default:
> + return -EINVAL;
> + }
> }
>
> vgic_mmio_write_v2_misc(vcpu, addr, len, val);
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index a6be403996c6..4c8e4f83e3d1 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -155,13 +155,27 @@ static int vgic_mmio_uaccess_write_v3_misc(struct kvm_vcpu *vcpu,
> unsigned long val)
> {
> struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> + u32 reg;
>
> switch (addr & 0x0c) {
> case GICD_TYPER2:
> - case GICD_IIDR:
> if (val != vgic_mmio_read_v3_misc(vcpu, addr, len))
> return -EINVAL;
> return 0;
> + case GICD_IIDR:
> + reg = vgic_mmio_read_v3_misc(vcpu, addr, len);
> + if ((reg ^ val) & ~GICD_IIDR_REVISION_MASK)
> + return -EINVAL;
> +
> + reg = FIELD_GET(GICD_IIDR_REVISION_MASK, reg);
> + switch (reg) {
> + case KVM_VGIC_IMP_REV_2:
> + case KVM_VGIC_IMP_REV_3:
> + dist->implementation_rev = reg;
> + return 0;
> + default:
> + return -EINVAL;
> + }
> case GICD_CTLR:
> /* Not a GICv4.1? No HW SGIs */
> if (!kvm_vgic_global_state.has_gicv4_1)
> @@ -232,8 +246,13 @@ static unsigned long vgic_mmio_read_v3r_ctlr(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len)
> {
> struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> + unsigned long val;
> +
> + val = atomic_read(&vgic_cpu->ctlr);
> + if (vcpu->kvm->arch.vgic.implementation_rev >= KVM_VGIC_IMP_REV_3)
That's a lot of indirection :) Could you make a helper for getting at
the implementation revision from a vCPU pointer?
> + val |= GICR_CTLR_IR | GICR_CTLR_CES;
>
> - return vgic_cpu->lpis_enabled ? GICR_CTLR_ENABLE_LPIS : 0;
> + return val;
> }
>
> static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu,
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 401236f97cf2..2d8f2e90edc2 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -231,6 +231,9 @@ struct vgic_dist {
>
> /* Implementation revision as reported in the GICD_IIDR */
> u32 implementation_rev;
> +#define KVM_VGIC_IMP_REV_2 2 /* GICv2 restorable groups */
> +#define KVM_VGIC_IMP_REV_3 3 /* GICv3 GICR_CTLR.{IW,CES,RWP} */
> +#define KVM_VGIC_IMP_REV_LATEST KVM_VGIC_IMP_REV_3
>
> /* Userspace can write to GICv2 IGROUPR */
> bool v2_groups_user_writable;
> --
> 2.34.1
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
