On Tue, 15 Mar 2022 23:13:09 +0000,
Oliver Upton <oupton@xxxxxxxxxx> 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@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?
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.
--
Without deviation from the norm, progress is not possible.
