Re: [PATCH 5/5] KVM: arm/arm64: vgic: Get rid of struct vmcr for GICv2

[Christoffer Dall <cdall@linaro.org>]

On Tue, Mar 21, 2017 at 02:36:00PM +0000, Marc Zyngier wrote:
> On 21/03/17 11:05, Christoffer Dall wrote:
> > There is no common code in the VGIC that uses the VMCR, so we have no
> > use of the intermediate architecture-agnostic representation of the VMCR
> > and might as well manipulate the bits specifically using the logic for
> > the version of the GIC that the code supports.
> > 
> > For GICv2, this means translating between the GICH_VMCR register format
> > stored in memory and the GICC_X registers exported to user space, with
> > the annoying quirk around the format of the GICC_PMR, where we export
> > the 8 bit prority field using the lower 5 bits and always assuming
> > bits[2:0] are clear.
> > 
> > This now lets us get completely rid of struct vmcr and its common
> > accessor functions.
> > 
> > Signed-off-by: Christoffer Dall <cdall@linaro.org>
> > ---
> >  virt/kvm/arm/vgic/vgic-mmio-v2.c | 51 +++++++++++++++++++++++++++++-----------
> >  virt/kvm/arm/vgic/vgic-mmio.c    | 16 -------------
> >  virt/kvm/arm/vgic/vgic-v2.c      | 29 -----------------------
> >  virt/kvm/arm/vgic/vgic.h         | 14 -----------
> >  4 files changed, 37 insertions(+), 73 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > index a3ad7ff..1bdb990 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > @@ -219,23 +219,33 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu,
> >  static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu,
> >  					   gpa_t addr, unsigned int len)
> >  {
> > -	struct vgic_vmcr vmcr;
> > +	u32 vmcr = vcpu->arch.vgic_cpu.vgic_v2.vgic_vmcr;
> >  	u32 val;
> >  
> > -	vgic_get_vmcr(vcpu, &vmcr);
> >  
> >  	switch (addr & 0xff) {
> >  	case GIC_CPU_CTRL:
> > -		val = vmcr.ctlr;
> > +		val = (vmcr & GICH_VMCR_CTRL_MASK) >>
> > +			GICH_VMCR_CTRL_SHIFT;
> >  		break;
> >  	case GIC_CPU_PRIMASK:
> > -		val = vmcr.pmr;
> > +		/*
> > +		 * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the
> > +		 * the PMR field as GICH_VMCR.VMPriMask rather than
> > +		 * GICC_PMR.Priority, so we expose the upper five bits of
> > +		 * priority mask to userspace using the lower bits in the
> > +		 * 32-bit word provided by userspace.
> > +		 */
> > +		val = (vmcr & GICH_VMCR_PRIMASK_MASK) >>
> > +			GICH_VMCR_PRIMASK_SHIFT;
> >  		break;
> >  	case GIC_CPU_BINPOINT:
> > -		val = vmcr.bpr;
> > +		val = (vmcr & GICH_VMCR_BINPOINT_MASK) >>
> > +			GICH_VMCR_BINPOINT_SHIFT;
> >  		break;
> >  	case GIC_CPU_ALIAS_BINPOINT:
> > -		val = vmcr.abpr;
> > +		val = (vmcr & GICH_VMCR_ALIAS_BINPOINT_MASK) >>
> > +			GICH_VMCR_ALIAS_BINPOINT_SHIFT;
> >  		break;
> >  	case GIC_CPU_IDENT:
> >  		val = ((PRODUCT_ID_KVM << 20) |
> > @@ -253,26 +263,39 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu,
> >  				   gpa_t addr, unsigned int len,
> >  				   unsigned long val)
> >  {
> > -	struct vgic_vmcr vmcr;
> > -
> > -	vgic_get_vmcr(vcpu, &vmcr);
> > +	u32 *vmcr = &vcpu->arch.vgic_cpu.vgic_v2.vgic_vmcr;
> > +	u32 mask, field;
> >  
> >  	switch (addr & 0xff) {
> >  	case GIC_CPU_CTRL:
> > -		vmcr.ctlr = val;
> > +		mask = GICH_VMCR_CTRL_MASK;
> > +		field = val << GICH_VMCR_CTRL_SHIFT;
> >  		break;
> >  	case GIC_CPU_PRIMASK:
> > -		vmcr.pmr = val;
> > +		/*
> > +		 * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the
> > +		 * the PMR field as GICH_VMCR.VMPriMask rather than
> > +		 * GICC_PMR.Priority, so we obtain the upper five bits of
> > +		 * priority mask from userspace using the lower bits in the
> > +		 * 32-bit word provided by userspace.
> > +		 */
> > +		mask = GICH_VMCR_PRIMASK_MASK;
> > +		field = val << GICH_VMCR_PRIMASK_SHIFT;
> >  		break;
> >  	case GIC_CPU_BINPOINT:
> > -		vmcr.bpr = val;
> > +		mask = GICH_VMCR_BINPOINT_MASK;
> > +		field = val << GICH_VMCR_BINPOINT_SHIFT;
> >  		break;
> >  	case GIC_CPU_ALIAS_BINPOINT:
> > -		vmcr.abpr = val;
> > +		mask = GICH_VMCR_ALIAS_BINPOINT_MASK;
> > +		field = val << GICH_VMCR_ALIAS_BINPOINT_SHIFT;
> >  		break;
> > +	default:
> > +		return;
> 
> 
> Something seems fishy here. If I have a GICv2-on-GICv3, my vmcr is still
> that of a GICv3, not of a GICv2. Here, you're cramming everything into a
> v2 representation, which is not going to work on GICv3.
> 
> Or am I missing something?
> 
No, you're right, that is the reason for having the indirection, which I
competely missed.  I'll drop this series and revert back to my original
patch for the VMCR.

Thanks for having a look and sorry about the noise.
-Christoffer