Re: [PATCH 4/4] KVM: arm64: vgic-v3: Advertise GICR_CTLR.{IR, CES} as a new GICD_IIDR revision

[Marc Zyngier <maz@kernel.org>]

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.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm