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

[Marc Zyngier <marc.zyngier@arm.com>]

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?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...