Re: [PATCH 03/10] KVM: arm-vgic: Set base addr through device API

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

On 12/12/13 19:55, Christoffer Dall wrote:
> Support setting the distributor and cpu interface base addresses in the
> VM physical address space through the KVM_{SET,GET}_DEVICE_ATTR API
> in addition to the ARM specific API.
> 
> This has the added benefit of being able to share more code in user
> space and do things in a uniform manner.
> 
> Also deprecate the older API at the same time, but backwards
> compatibility will be maintained.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.

> ---
> Changelog[v4]:
>  - Move VGIC device control API defines to later patch where they are
>    used
>  - Remove unused variable declaration
> 
> Changelog[v3]:
>  - Spelling nits
>  - Fix error codes from vgic_get_attr
> 
> Changelog[v2]:
>  - None
> 
>  Documentation/virtual/kvm/api.txt              |  7 ++-
>  Documentation/virtual/kvm/devices/arm-vgic.txt | 11 ++++
>  arch/arm/include/uapi/asm/kvm.h                |  2 +
>  arch/arm/kvm/arm.c                             |  2 +-
>  include/kvm/arm_vgic.h                         |  2 +-
>  virt/kvm/arm/vgic.c                            | 87 ++++++++++++++++++++++----
>  6 files changed, 96 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index a30035d..867112f 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2391,7 +2391,8 @@ struct kvm_reg_list {
>  This ioctl returns the guest registers that are supported for the
>  KVM_GET_ONE_REG/KVM_SET_ONE_REG calls.
>  
> -4.85 KVM_ARM_SET_DEVICE_ADDR
> +
> +4.85 KVM_ARM_SET_DEVICE_ADDR (deprecated)
>  
>  Capability: KVM_CAP_ARM_SET_DEVICE_ADDR
>  Architectures: arm, arm64
> @@ -2429,6 +2430,10 @@ must be called after calling KVM_CREATE_IRQCHIP, but before calling
>  KVM_RUN on any of the VCPUs.  Calling this ioctl twice for any of the
>  base addresses will return -EEXIST.
>  
> +Note, this IOCTL is deprecated and the more flexible SET/GET_DEVICE_ATTR API
> +should be used instead.
> +
> +
>  4.86 KVM_PPC_RTAS_DEFINE_TOKEN
>  
>  Capability: KVM_CAP_PPC_RTAS
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
> index 38f27f7..c9febb2 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> @@ -8,3 +8,14 @@ Only one VGIC instance may be instantiated through either this API or the
>  legacy KVM_CREATE_IRQCHIP api.  The created VGIC will act as the VM interrupt
>  controller, requiring emulated user-space devices to inject interrupts to the
>  VGIC instead of directly to CPUs.
> +
> +Groups:
> +  KVM_DEV_ARM_VGIC_GRP_ADDR
> +  Attributes:
> +    KVM_VGIC_V2_ADDR_TYPE_DIST (rw, 64-bit)
> +      Base address in the guest physical address space of the GIC distributor
> +      register mappings.
> +
> +    KVM_VGIC_V2_ADDR_TYPE_CPU (rw, 64-bit)
> +      Base address in the guest physical address space of the GIC virtual cpu
> +      interface register mappings.
> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> index 835b867..76a7427 100644
> --- a/arch/arm/include/uapi/asm/kvm.h
> +++ b/arch/arm/include/uapi/asm/kvm.h
> @@ -163,6 +163,8 @@ struct kvm_arch_memory_slot {
>  #define KVM_REG_ARM_VFP_FPINST		0x1009
>  #define KVM_REG_ARM_VFP_FPINST2		0x100A
>  
> +/* Device Control API: ARM VGIC */
> +#define KVM_DEV_ARM_VGIC_GRP_ADDR	0
>  
>  /* KVM_IRQ_LINE irq field index values */
>  #define KVM_ARM_IRQ_TYPE_SHIFT		24
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index cc7c41a..f290b22 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -776,7 +776,7 @@ static int kvm_vm_ioctl_set_device_addr(struct kvm *kvm,
>  	case KVM_ARM_DEVICE_VGIC_V2:
>  		if (!vgic_present)
>  			return -ENXIO;
> -		return kvm_vgic_set_addr(kvm, type, dev_addr->addr);
> +		return kvm_vgic_addr(kvm, type, &dev_addr->addr, true);
>  	default:
>  		return -ENODEV;
>  	}
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 7e2d158..be85127 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -144,7 +144,7 @@ struct kvm_run;
>  struct kvm_exit_mmio;
>  
>  #ifdef CONFIG_KVM_ARM_VGIC
> -int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr);
> +int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write);
>  int kvm_vgic_hyp_init(void);
>  int kvm_vgic_init(struct kvm *kvm);
>  int kvm_vgic_create(struct kvm *kvm);
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index b15d6c1..45db48d 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1495,6 +1495,12 @@ static int vgic_ioaddr_assign(struct kvm *kvm, phys_addr_t *ioaddr,
>  {
>  	int ret;
>  
> +	if (addr & ~KVM_PHYS_MASK)
> +		return -E2BIG;
> +
> +	if (addr & (SZ_4K - 1))
> +		return -EINVAL;
> +
>  	if (!IS_VGIC_ADDR_UNDEF(*ioaddr))
>  		return -EEXIST;
>  	if (addr + size < addr)
> @@ -1507,26 +1513,41 @@ static int vgic_ioaddr_assign(struct kvm *kvm, phys_addr_t *ioaddr,
>  	return ret;
>  }
>  
> -int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr)
> +/**
> + * kvm_vgic_addr - set or get vgic VM base addresses
> + * @kvm:   pointer to the vm struct
> + * @type:  the VGIC addr type, one of KVM_VGIC_V2_ADDR_TYPE_XXX
> + * @addr:  pointer to address value
> + * @write: if true set the address in the VM address space, if false read the
> + *          address
> + *
> + * Set or get the vgic base addresses for the distributor and the virtual CPU
> + * interface in the VM physical address space.  These addresses are properties
> + * of the emulated core/SoC and therefore user space initially knows this
> + * information.
> + */
> +int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write)
>  {
>  	int r = 0;
>  	struct vgic_dist *vgic = &kvm->arch.vgic;
>  
> -	if (addr & ~KVM_PHYS_MASK)
> -		return -E2BIG;
> -
> -	if (addr & (SZ_4K - 1))
> -		return -EINVAL;
> -
>  	mutex_lock(&kvm->lock);
>  	switch (type) {
>  	case KVM_VGIC_V2_ADDR_TYPE_DIST:
> -		r = vgic_ioaddr_assign(kvm, &vgic->vgic_dist_base,
> -				       addr, KVM_VGIC_V2_DIST_SIZE);
> +		if (write) {
> +			r = vgic_ioaddr_assign(kvm, &vgic->vgic_dist_base,
> +					       *addr, KVM_VGIC_V2_DIST_SIZE);
> +		} else {
> +			*addr = vgic->vgic_dist_base;
> +		}
>  		break;
>  	case KVM_VGIC_V2_ADDR_TYPE_CPU:
> -		r = vgic_ioaddr_assign(kvm, &vgic->vgic_cpu_base,
> -				       addr, KVM_VGIC_V2_CPU_SIZE);
> +		if (write) {
> +			r = vgic_ioaddr_assign(kvm, &vgic->vgic_cpu_base,
> +					       *addr, KVM_VGIC_V2_CPU_SIZE);
> +		} else {
> +			*addr = vgic->vgic_cpu_base;
> +		}
>  		break;
>  	default:
>  		r = -ENODEV;
> @@ -1538,16 +1559,58 @@ int kvm_vgic_set_addr(struct kvm *kvm, unsigned long type, u64 addr)
>  
>  static int vgic_set_attr(struct kvm_device *dev, struct kvm_device_attr *attr)
>  {
> +	int r;
> +
> +	switch (attr->group) {
> +	case KVM_DEV_ARM_VGIC_GRP_ADDR: {
> +		u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> +		u64 addr;
> +		unsigned long type = (unsigned long)attr->attr;
> +
> +		if (copy_from_user(&addr, uaddr, sizeof(addr)))
> +			return -EFAULT;
> +
> +		r = kvm_vgic_addr(dev->kvm, type, &addr, true);
> +		return (r == -ENODEV) ? -ENXIO : r;
> +	}
> +	}
> +
>  	return -ENXIO;
>  }
>  
>  static int vgic_get_attr(struct kvm_device *dev, struct kvm_device_attr *attr)
>  {
> -	return -ENXIO;
> +	int r = -ENXIO;
> +
> +	switch (attr->group) {
> +	case KVM_DEV_ARM_VGIC_GRP_ADDR: {
> +		u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> +		u64 addr;
> +		unsigned long type = (unsigned long)attr->attr;
> +
> +		r = kvm_vgic_addr(dev->kvm, type, &addr, false);
> +		if (r)
> +			return (r == -ENODEV) ? -ENXIO : r;
> +
> +		if (copy_to_user(uaddr, &addr, sizeof(addr)))
> +			return -EFAULT;
> +	}
> +	}
> +
> +	return r;
>  }
>  
>  static int vgic_has_attr(struct kvm_device *dev, struct kvm_device_attr *attr)
>  {
> +	switch (attr->group) {
> +	case KVM_DEV_ARM_VGIC_GRP_ADDR:
> +		switch (attr->attr) {
> +		case KVM_VGIC_V2_ADDR_TYPE_DIST:
> +		case KVM_VGIC_V2_ADDR_TYPE_CPU:
> +			return 0;
> +		}
> +		break;
> +	}
>  	return -ENXIO;
>  }
>  
> 


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