| Message ID | 20220314164044.772709-5-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: vgic-v3: MMIO-based LPI invalidation and co | expand |
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@kernel.org> > --- > 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@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
On Tue, 15 Mar 2022 23:13:09 +0000, Oliver Upton <oupton@google.com> wrote: > > 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@kernel.org> > > --- > > 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? Good point. Having a non-zero implementation will serve the same purpose. The drawback is that we lose the documentation aspect of the field, but we can probably work around that. > > > + 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? Sure, as there will be two users now. Thanks, M.
Hi Oliver, On Tue, 15 Mar 2022 23:13:09 +0000, Oliver Upton <oupton@google.com> wrote: > > Hi Marc, > > On Mon, Mar 14, 2022 at 04:40:44PM +0000, Marc Zyngier wrote: > > @@ -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? [coming back to this] Now I remember why this doesn't work. The established behaviour is that it takes a write to IIDR to switch to the 'writable groups' mode. If we base the switch on the implementation version, we don't need a write anymore (we always allow groups to be writable), and old guests cannot be reliably restored. 32f8777ed92d has the gory details, and that's really not old enough that we can turn a blind eye to it, unfortunately. Thanks, M.
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; + 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) + 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;
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@kernel.org> --- 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(-)