diff mbox

[v3,43/55] KVM: arm/arm64: vgic-new: vgic_kvm_device: access to VGIC registers

Message ID 1462531568-9799-44-git-send-email-andre.przywara@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andre Przywara May 6, 2016, 10:45 a.m. UTC
From: Eric Auger <eric.auger@linaro.org>

This patch implements the switches for KVM_DEV_ARM_VGIC_GRP_DIST_REGS
and KVM_DEV_ARM_VGIC_GRP_CPU_REGS API which allows the userspace to
access VGIC registers.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 virt/kvm/arm/vgic/vgic-kvm-device.c | 53 +++++++++++++++++++++++++++++++++++--
 virt/kvm/arm/vgic/vgic-mmio-v2.c    | 34 ++++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic.h            |  1 +
 3 files changed, 86 insertions(+), 2 deletions(-)

Comments

Christoffer Dall May 12, 2016, 6:30 p.m. UTC | #1
On Fri, May 06, 2016 at 11:45:56AM +0100, Andre Przywara wrote:
> From: Eric Auger <eric.auger@linaro.org>
> 
> This patch implements the switches for KVM_DEV_ARM_VGIC_GRP_DIST_REGS
> and KVM_DEV_ARM_VGIC_GRP_CPU_REGS API which allows the userspace to
> access VGIC registers.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-kvm-device.c | 53 +++++++++++++++++++++++++++++++++++--
>  virt/kvm/arm/vgic/vgic-mmio-v2.c    | 34 ++++++++++++++++++++++++
>  virt/kvm/arm/vgic/vgic.h            |  1 +
>  3 files changed, 86 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index 0189c13..c952f6f 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -252,6 +252,21 @@ void kvm_register_vgic_device(unsigned long type)
>  	}
>  }
>  
> +/** vgic_attr_regs_access: allows user space to read/write VGIC registers
> + *
> + * @dev: kvm device handle
> + * @attr: kvm device attribute
> + * @reg: address the value is read or written
> + * @is_write: write flag
> + *
> + */
> +static int vgic_attr_regs_access(struct kvm_device *dev,
> +				 struct kvm_device_attr *attr,
> +				 u32 *reg, bool is_write)
> +{
> +	return -ENXIO;
> +}
> +
>  /* V2 ops */
>  
>  static int vgic_v2_set_attr(struct kvm_device *dev,
> @@ -260,8 +275,23 @@ static int vgic_v2_set_attr(struct kvm_device *dev,
>  	int ret;
>  
>  	ret = vgic_set_common_attr(dev, attr);
> -	return ret;
> +	if (ret != -ENXIO)
> +		return ret;
> +
> +	switch (attr->group) {
> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> +	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: {
> +		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> +		u32 reg;
> +
> +		if (get_user(reg, uaddr))
> +			return -EFAULT;
>  
> +		return vgic_attr_regs_access(dev, attr, &reg, true);
> +	}
> +	}
> +
> +	return -ENXIO;
>  }
>  
>  static int vgic_v2_get_attr(struct kvm_device *dev,
> @@ -270,7 +300,23 @@ static int vgic_v2_get_attr(struct kvm_device *dev,
>  	int ret;
>  
>  	ret = vgic_get_common_attr(dev, attr);
> -	return ret;
> +	if (ret != -ENXIO)
> +		return ret;
> +
> +	switch (attr->group) {
> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> +	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: {
> +		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> +		u32 reg = 0;
> +
> +		ret = vgic_attr_regs_access(dev, attr, &reg, false);
> +		if (ret)
> +			return ret;
> +		return put_user(reg, uaddr);
> +	}
> +	}
> +
> +	return -ENXIO;
>  }
>  
>  static int vgic_v2_has_attr(struct kvm_device *dev,
> @@ -284,6 +330,9 @@ static int vgic_v2_has_attr(struct kvm_device *dev,
>  			return 0;
>  		}
>  		break;
> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> +	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
> +		return vgic_v2_has_attr_regs(dev, attr);
>  	case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
>  		return 0;
>  	case KVM_DEV_ARM_VGIC_GRP_CTRL:
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index 8006ac0..cf8fee9 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -246,3 +246,37 @@ unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev)
>  
>  	return SZ_4K;
>  }
> +
> +int vgic_v2_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
> +{
> +	int nr_irqs = dev->kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS;
> +	const struct vgic_register_region *regions;
> +	gpa_t addr;
> +	int nr_regions, i, len;
> +
> +	addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
> +
> +	switch (attr->group) {
> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> +		regions = vgic_v2_dist_registers;
> +		nr_regions = ARRAY_SIZE(vgic_v2_dist_registers);
> +		break;
> +	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
> +		return -ENXIO;		/* TODO: describe CPU i/f regs also */
> +	default:
> +		return -ENXIO;
> +	}
> +
> +	for (i = 0; i < nr_regions; i++) {
> +		if (regions[i].bits_per_irq)
> +			len = (regions[i].bits_per_irq * nr_irqs) / 8;
> +		else
> +			len = regions[i].len;
> +
> +		if (regions[i].reg_offset <= addr &&
> +		    regions[i].reg_offset + len > addr)
> +			return 0;

should we check if addr is word-aligned ?

> +	}
> +
> +	return -ENXIO;
> +}
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index c44ee01..a4397f9 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -36,6 +36,7 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu);
>  void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr);
>  void vgic_v2_clear_lr(struct kvm_vcpu *vcpu, int lr);
>  void vgic_v2_set_underflow(struct kvm_vcpu *vcpu);
> +int vgic_v2_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr);
>  int vgic_register_dist_iodev(struct kvm *kvm, gpa_t dist_base_address,
>  			     enum vgic_type);
>  
> -- 
> 2.7.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andre Przywara May 13, 2016, 12:24 p.m. UTC | #2
Hi,

On 12/05/16 19:30, Christoffer Dall wrote:
> On Fri, May 06, 2016 at 11:45:56AM +0100, Andre Przywara wrote:
>> From: Eric Auger <eric.auger@linaro.org>
>>
>> This patch implements the switches for KVM_DEV_ARM_VGIC_GRP_DIST_REGS
>> and KVM_DEV_ARM_VGIC_GRP_CPU_REGS API which allows the userspace to
>> access VGIC registers.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  virt/kvm/arm/vgic/vgic-kvm-device.c | 53 +++++++++++++++++++++++++++++++++++--
>>  virt/kvm/arm/vgic/vgic-mmio-v2.c    | 34 ++++++++++++++++++++++++
>>  virt/kvm/arm/vgic/vgic.h            |  1 +
>>  3 files changed, 86 insertions(+), 2 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
>> index 0189c13..c952f6f 100644
>> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
>> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
>> @@ -252,6 +252,21 @@ void kvm_register_vgic_device(unsigned long type)
>>  	}
>>  }
>>  
>> +/** vgic_attr_regs_access: allows user space to read/write VGIC registers
>> + *
>> + * @dev: kvm device handle
>> + * @attr: kvm device attribute
>> + * @reg: address the value is read or written
>> + * @is_write: write flag
>> + *
>> + */
>> +static int vgic_attr_regs_access(struct kvm_device *dev,
>> +				 struct kvm_device_attr *attr,
>> +				 u32 *reg, bool is_write)
>> +{
>> +	return -ENXIO;
>> +}
>> +
>>  /* V2 ops */
>>  
>>  static int vgic_v2_set_attr(struct kvm_device *dev,
>> @@ -260,8 +275,23 @@ static int vgic_v2_set_attr(struct kvm_device *dev,
>>  	int ret;
>>  
>>  	ret = vgic_set_common_attr(dev, attr);
>> -	return ret;
>> +	if (ret != -ENXIO)
>> +		return ret;
>> +
>> +	switch (attr->group) {
>> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
>> +	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: {
>> +		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
>> +		u32 reg;
>> +
>> +		if (get_user(reg, uaddr))
>> +			return -EFAULT;
>>  
>> +		return vgic_attr_regs_access(dev, attr, &reg, true);
>> +	}
>> +	}
>> +
>> +	return -ENXIO;
>>  }
>>  
>>  static int vgic_v2_get_attr(struct kvm_device *dev,
>> @@ -270,7 +300,23 @@ static int vgic_v2_get_attr(struct kvm_device *dev,
>>  	int ret;
>>  
>>  	ret = vgic_get_common_attr(dev, attr);
>> -	return ret;
>> +	if (ret != -ENXIO)
>> +		return ret;
>> +
>> +	switch (attr->group) {
>> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
>> +	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: {
>> +		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
>> +		u32 reg = 0;
>> +
>> +		ret = vgic_attr_regs_access(dev, attr, &reg, false);
>> +		if (ret)
>> +			return ret;
>> +		return put_user(reg, uaddr);
>> +	}
>> +	}
>> +
>> +	return -ENXIO;
>>  }
>>  
>>  static int vgic_v2_has_attr(struct kvm_device *dev,
>> @@ -284,6 +330,9 @@ static int vgic_v2_has_attr(struct kvm_device *dev,
>>  			return 0;
>>  		}
>>  		break;
>> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
>> +	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
>> +		return vgic_v2_has_attr_regs(dev, attr);
>>  	case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
>>  		return 0;
>>  	case KVM_DEV_ARM_VGIC_GRP_CTRL:
>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> index 8006ac0..cf8fee9 100644
>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>> @@ -246,3 +246,37 @@ unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev)
>>  
>>  	return SZ_4K;
>>  }
>> +
>> +int vgic_v2_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
>> +{
>> +	int nr_irqs = dev->kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS;
>> +	const struct vgic_register_region *regions;
>> +	gpa_t addr;
>> +	int nr_regions, i, len;
>> +
>> +	addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
>> +
>> +	switch (attr->group) {
>> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
>> +		regions = vgic_v2_dist_registers;
>> +		nr_regions = ARRAY_SIZE(vgic_v2_dist_registers);
>> +		break;
>> +	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
>> +		return -ENXIO;		/* TODO: describe CPU i/f regs also */
>> +	default:
>> +		return -ENXIO;
>> +	}
>> +
>> +	for (i = 0; i < nr_regions; i++) {
>> +		if (regions[i].bits_per_irq)
>> +			len = (regions[i].bits_per_irq * nr_irqs) / 8;
>> +		else
>> +			len = regions[i].len;
>> +
>> +		if (regions[i].reg_offset <= addr &&
>> +		    regions[i].reg_offset + len > addr)
>> +			return 0;
> 
> should we check if addr is word-aligned ?

Do we care here? This is just the function that says whether we support
this register or not, so I don't see so much benefit in checking here.
A check would be more useful in get/set_attr, if this isn't even
enforced before.

Cheers,
Andre.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoffer Dall May 13, 2016, 12:29 p.m. UTC | #3
On Fri, May 13, 2016 at 01:24:07PM +0100, Andre Przywara wrote:
> Hi,
> 
> On 12/05/16 19:30, Christoffer Dall wrote:
> > On Fri, May 06, 2016 at 11:45:56AM +0100, Andre Przywara wrote:
> >> From: Eric Auger <eric.auger@linaro.org>
> >>
> >> This patch implements the switches for KVM_DEV_ARM_VGIC_GRP_DIST_REGS
> >> and KVM_DEV_ARM_VGIC_GRP_CPU_REGS API which allows the userspace to
> >> access VGIC registers.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

[...]

> >> +int vgic_v2_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
> >> +{
> >> +	int nr_irqs = dev->kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS;
> >> +	const struct vgic_register_region *regions;
> >> +	gpa_t addr;
> >> +	int nr_regions, i, len;
> >> +
> >> +	addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
> >> +
> >> +	switch (attr->group) {
> >> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> >> +		regions = vgic_v2_dist_registers;
> >> +		nr_regions = ARRAY_SIZE(vgic_v2_dist_registers);
> >> +		break;
> >> +	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
> >> +		return -ENXIO;		/* TODO: describe CPU i/f regs also */
> >> +	default:
> >> +		return -ENXIO;
> >> +	}
> >> +
> >> +	for (i = 0; i < nr_regions; i++) {
> >> +		if (regions[i].bits_per_irq)
> >> +			len = (regions[i].bits_per_irq * nr_irqs) / 8;
> >> +		else
> >> +			len = regions[i].len;
> >> +
> >> +		if (regions[i].reg_offset <= addr &&
> >> +		    regions[i].reg_offset + len > addr)
> >> +			return 0;
> > 
> > should we check if addr is word-aligned ?
> 
> Do we care here? This is just the function that says whether we support
> this register or not, so I don't see so much benefit in checking here.
> A check would be more useful in get/set_attr, if this isn't even
> enforced before.
> 

It's kind of weird that you can ask 'do you have an attribute with
offset 0x2' and be told, 'yes', and then if you try to get it, you get
'this attribute doesn't exist'.

Thanks,
-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Zyngier May 13, 2016, 12:30 p.m. UTC | #4
On 13/05/16 13:24, Andre Przywara wrote:
> Hi,
> 
> On 12/05/16 19:30, Christoffer Dall wrote:
>> On Fri, May 06, 2016 at 11:45:56AM +0100, Andre Przywara wrote:
>>> From: Eric Auger <eric.auger@linaro.org>
>>>
>>> This patch implements the switches for KVM_DEV_ARM_VGIC_GRP_DIST_REGS
>>> and KVM_DEV_ARM_VGIC_GRP_CPU_REGS API which allows the userspace to
>>> access VGIC registers.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>> ---
>>>  virt/kvm/arm/vgic/vgic-kvm-device.c | 53 +++++++++++++++++++++++++++++++++++--
>>>  virt/kvm/arm/vgic/vgic-mmio-v2.c    | 34 ++++++++++++++++++++++++
>>>  virt/kvm/arm/vgic/vgic.h            |  1 +
>>>  3 files changed, 86 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
>>> index 0189c13..c952f6f 100644
>>> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
>>> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
>>> @@ -252,6 +252,21 @@ void kvm_register_vgic_device(unsigned long type)
>>>  	}
>>>  }
>>>  
>>> +/** vgic_attr_regs_access: allows user space to read/write VGIC registers
>>> + *
>>> + * @dev: kvm device handle
>>> + * @attr: kvm device attribute
>>> + * @reg: address the value is read or written
>>> + * @is_write: write flag
>>> + *
>>> + */
>>> +static int vgic_attr_regs_access(struct kvm_device *dev,
>>> +				 struct kvm_device_attr *attr,
>>> +				 u32 *reg, bool is_write)
>>> +{
>>> +	return -ENXIO;
>>> +}
>>> +
>>>  /* V2 ops */
>>>  
>>>  static int vgic_v2_set_attr(struct kvm_device *dev,
>>> @@ -260,8 +275,23 @@ static int vgic_v2_set_attr(struct kvm_device *dev,
>>>  	int ret;
>>>  
>>>  	ret = vgic_set_common_attr(dev, attr);
>>> -	return ret;
>>> +	if (ret != -ENXIO)
>>> +		return ret;
>>> +
>>> +	switch (attr->group) {
>>> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
>>> +	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: {
>>> +		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
>>> +		u32 reg;
>>> +
>>> +		if (get_user(reg, uaddr))
>>> +			return -EFAULT;
>>>  
>>> +		return vgic_attr_regs_access(dev, attr, &reg, true);
>>> +	}
>>> +	}
>>> +
>>> +	return -ENXIO;
>>>  }
>>>  
>>>  static int vgic_v2_get_attr(struct kvm_device *dev,
>>> @@ -270,7 +300,23 @@ static int vgic_v2_get_attr(struct kvm_device *dev,
>>>  	int ret;
>>>  
>>>  	ret = vgic_get_common_attr(dev, attr);
>>> -	return ret;
>>> +	if (ret != -ENXIO)
>>> +		return ret;
>>> +
>>> +	switch (attr->group) {
>>> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
>>> +	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: {
>>> +		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
>>> +		u32 reg = 0;
>>> +
>>> +		ret = vgic_attr_regs_access(dev, attr, &reg, false);
>>> +		if (ret)
>>> +			return ret;
>>> +		return put_user(reg, uaddr);
>>> +	}
>>> +	}
>>> +
>>> +	return -ENXIO;
>>>  }
>>>  
>>>  static int vgic_v2_has_attr(struct kvm_device *dev,
>>> @@ -284,6 +330,9 @@ static int vgic_v2_has_attr(struct kvm_device *dev,
>>>  			return 0;
>>>  		}
>>>  		break;
>>> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
>>> +	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
>>> +		return vgic_v2_has_attr_regs(dev, attr);
>>>  	case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
>>>  		return 0;
>>>  	case KVM_DEV_ARM_VGIC_GRP_CTRL:
>>> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>>> index 8006ac0..cf8fee9 100644
>>> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
>>> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
>>> @@ -246,3 +246,37 @@ unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev)
>>>  
>>>  	return SZ_4K;
>>>  }
>>> +
>>> +int vgic_v2_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
>>> +{
>>> +	int nr_irqs = dev->kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS;
>>> +	const struct vgic_register_region *regions;
>>> +	gpa_t addr;
>>> +	int nr_regions, i, len;
>>> +
>>> +	addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
>>> +
>>> +	switch (attr->group) {
>>> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
>>> +		regions = vgic_v2_dist_registers;
>>> +		nr_regions = ARRAY_SIZE(vgic_v2_dist_registers);
>>> +		break;
>>> +	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
>>> +		return -ENXIO;		/* TODO: describe CPU i/f regs also */
>>> +	default:
>>> +		return -ENXIO;
>>> +	}
>>> +
>>> +	for (i = 0; i < nr_regions; i++) {
>>> +		if (regions[i].bits_per_irq)
>>> +			len = (regions[i].bits_per_irq * nr_irqs) / 8;
>>> +		else
>>> +			len = regions[i].len;
>>> +
>>> +		if (regions[i].reg_offset <= addr &&
>>> +		    regions[i].reg_offset + len > addr)
>>> +			return 0;
>>
>> should we check if addr is word-aligned ?
> 
> Do we care here? This is just the function that says whether we support
> this register or not, so I don't see so much benefit in checking here.

There definitely is value in checking the alignment. When you reply OK
to a "has_attr" request, you form a contract with userspace that the
same value will can be used for a get or set operation.

Here, has_attr will succeed while get/set will fail, and that's not an
acceptable behaviour.

> A check would be more useful in get/set_attr, if this isn't even
> enforced before.

Don't we already have that check by virtue of using the same accessors
as the MMIO path?

Thanks,

	M.
diff mbox

Patch

diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
index 0189c13..c952f6f 100644
--- a/virt/kvm/arm/vgic/vgic-kvm-device.c
+++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
@@ -252,6 +252,21 @@  void kvm_register_vgic_device(unsigned long type)
 	}
 }
 
+/** vgic_attr_regs_access: allows user space to read/write VGIC registers
+ *
+ * @dev: kvm device handle
+ * @attr: kvm device attribute
+ * @reg: address the value is read or written
+ * @is_write: write flag
+ *
+ */
+static int vgic_attr_regs_access(struct kvm_device *dev,
+				 struct kvm_device_attr *attr,
+				 u32 *reg, bool is_write)
+{
+	return -ENXIO;
+}
+
 /* V2 ops */
 
 static int vgic_v2_set_attr(struct kvm_device *dev,
@@ -260,8 +275,23 @@  static int vgic_v2_set_attr(struct kvm_device *dev,
 	int ret;
 
 	ret = vgic_set_common_attr(dev, attr);
-	return ret;
+	if (ret != -ENXIO)
+		return ret;
+
+	switch (attr->group) {
+	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
+	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: {
+		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
+		u32 reg;
+
+		if (get_user(reg, uaddr))
+			return -EFAULT;
 
+		return vgic_attr_regs_access(dev, attr, &reg, true);
+	}
+	}
+
+	return -ENXIO;
 }
 
 static int vgic_v2_get_attr(struct kvm_device *dev,
@@ -270,7 +300,23 @@  static int vgic_v2_get_attr(struct kvm_device *dev,
 	int ret;
 
 	ret = vgic_get_common_attr(dev, attr);
-	return ret;
+	if (ret != -ENXIO)
+		return ret;
+
+	switch (attr->group) {
+	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
+	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS: {
+		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
+		u32 reg = 0;
+
+		ret = vgic_attr_regs_access(dev, attr, &reg, false);
+		if (ret)
+			return ret;
+		return put_user(reg, uaddr);
+	}
+	}
+
+	return -ENXIO;
 }
 
 static int vgic_v2_has_attr(struct kvm_device *dev,
@@ -284,6 +330,9 @@  static int vgic_v2_has_attr(struct kvm_device *dev,
 			return 0;
 		}
 		break;
+	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
+	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
+		return vgic_v2_has_attr_regs(dev, attr);
 	case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
 		return 0;
 	case KVM_DEV_ARM_VGIC_GRP_CTRL:
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index 8006ac0..cf8fee9 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -246,3 +246,37 @@  unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev)
 
 	return SZ_4K;
 }
+
+int vgic_v2_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
+{
+	int nr_irqs = dev->kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS;
+	const struct vgic_register_region *regions;
+	gpa_t addr;
+	int nr_regions, i, len;
+
+	addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
+
+	switch (attr->group) {
+	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
+		regions = vgic_v2_dist_registers;
+		nr_regions = ARRAY_SIZE(vgic_v2_dist_registers);
+		break;
+	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
+		return -ENXIO;		/* TODO: describe CPU i/f regs also */
+	default:
+		return -ENXIO;
+	}
+
+	for (i = 0; i < nr_regions; i++) {
+		if (regions[i].bits_per_irq)
+			len = (regions[i].bits_per_irq * nr_irqs) / 8;
+		else
+			len = regions[i].len;
+
+		if (regions[i].reg_offset <= addr &&
+		    regions[i].reg_offset + len > addr)
+			return 0;
+	}
+
+	return -ENXIO;
+}
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index c44ee01..a4397f9 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -36,6 +36,7 @@  void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu);
 void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr);
 void vgic_v2_clear_lr(struct kvm_vcpu *vcpu, int lr);
 void vgic_v2_set_underflow(struct kvm_vcpu *vcpu);
+int vgic_v2_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr);
 int vgic_register_dist_iodev(struct kvm *kvm, gpa_t dist_base_address,
 			     enum vgic_type);