[PATCH 0/3] KVM: arm64: Implement API for vGICv3 live migration

[PATCH 0/3] KVM: arm64: Implement API for vGICv3 live migration

[Pavel Fedin]

This patchset adds necessary userspace API in order to support vGICv3 live
migration. This includes accessing GIC distributor and redistributor memory
regions using device attribute ioctls, and system registers of
CPU interface using register get/set ioctls.

Pavel Fedin (3):
  KVM: arm64: Implement vGICv3 distributor and redistributor access from
    userspace
  KVM: arm64: Allow to use accessors in KVM_SET_ONE_REG and
    KVM_GET_ONE_REG
  KVM: arm64: Implement accessors for vGIC CPU interface registers

 arch/arm64/include/uapi/asm/kvm.h  |   1 +
 arch/arm64/kvm/sys_regs.c          | 223 ++++++++++++++++++++++++++++++++++++-
 include/linux/irqchip/arm-gic-v3.h |  18 ++-
 virt/kvm/arm/vgic-v3-emul.c        | 186 ++++++++++++++++++++++++++++---
 4 files changed, 405 insertions(+), 23 deletions(-)

-- 
2.4.4

[PATCH 1/3] KVM: arm64: Implement vGICv3 distributor and redistributor access from userspace

[Pavel Fedin]

The access is done similar to GICv2, using KVM_DEV_ARM_VGIC_GRP_DIST_REGS
and KVM_DEV_ARM_VGIC_GRP_REDIST_REGS with KVM_SET_DEVICE_ATTR and
KVM_GET_DEVICE_ATTR ioctls.

Registers are always assumed to be of their native size, 4 or 8 bytes.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 arch/arm64/include/uapi/asm/kvm.h |   1 +
 virt/kvm/arm/vgic-v3-emul.c       | 186 +++++++++++++++++++++++++++++++++++---
 2 files changed, 172 insertions(+), 15 deletions(-)

diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 0cd7b59..2936651 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -203,6 +203,7 @@ struct kvm_arch_memory_slot {
 #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS	3
 #define KVM_DEV_ARM_VGIC_GRP_CTRL	4
 #define   KVM_DEV_ARM_VGIC_CTRL_INIT	0
+#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
 
 /* KVM_IRQ_LINE irq field index values */
 #define KVM_ARM_IRQ_TYPE_SHIFT		24
diff --git a/virt/kvm/arm/vgic-v3-emul.c b/virt/kvm/arm/vgic-v3-emul.c
index e661e7f..b3847e1 100644
--- a/virt/kvm/arm/vgic-v3-emul.c
+++ b/virt/kvm/arm/vgic-v3-emul.c
@@ -39,6 +39,7 @@
 #include <linux/kvm.h>
 #include <linux/kvm_host.h>
 #include <linux/interrupt.h>
+#include <linux/uaccess.h>
 
 #include <linux/irqchip/arm-gic-v3.h>
 #include <kvm/arm_vgic.h>
@@ -990,6 +991,107 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
 		vgic_kick_vcpus(vcpu->kvm);
 }
 
+static int vgic_v3_attr_regs_access(struct kvm_device *dev,
+				 struct kvm_device_attr *attr,
+				 void *reg, u32 len, bool is_write)
+{
+	const struct vgic_io_range *r = NULL, *ranges;
+	phys_addr_t offset;
+	int ret, cpuid, c;
+	struct kvm_vcpu *vcpu, *tmp_vcpu;
+	struct vgic_dist *vgic;
+	struct kvm_exit_mmio mmio;
+	u64 data;
+
+	offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
+	cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
+		KVM_DEV_ARM_VGIC_CPUID_SHIFT;
+
+	mutex_lock(&dev->kvm->lock);
+
+	ret = vgic_init(dev->kvm);
+	if (ret)
+		goto out;
+
+	if (cpuid >= atomic_read(&dev->kvm->online_vcpus)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	vcpu = kvm_get_vcpu(dev->kvm, cpuid);
+	vgic = &dev->kvm->arch.vgic;
+
+	mmio.len = len;
+	mmio.is_write = is_write;
+	mmio.data = &data;
+	if (is_write) {
+		if (len == 8)
+			data = cpu_to_le64(*((u64 *)reg));
+		else
+			mmio_data_write(&mmio, ~0, *((u32 *)reg));
+	}
+	switch (attr->group) {
+	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
+		mmio.phys_addr = vgic->vgic_dist_base + offset;
+		ranges = vgic_v3_dist_ranges;
+		break;
+	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
+		mmio.phys_addr = vgic->vgic_redist_base + offset;
+		ranges = vgic_redist_ranges;
+		break;
+	default:
+		BUG();
+	}
+	r = vgic_find_range(ranges, 4, offset);
+
+	if (unlikely(!r || !r->handle_mmio)) {
+		ret = -ENXIO;
+		goto out;
+	}
+
+
+	spin_lock(&vgic->lock);
+
+	/*
+	 * Ensure that no other VCPU is running by checking the vcpu->cpu
+	 * field.  If no other VPCUs are running we can safely access the VGIC
+	 * state, because even if another VPU is run after this point, that
+	 * VCPU will not touch the vgic state, because it will block on
+	 * getting the vgic->lock in kvm_vgic_sync_hwstate().
+	 */
+	kvm_for_each_vcpu(c, tmp_vcpu, dev->kvm) {
+		if (unlikely(tmp_vcpu->cpu != -1)) {
+			ret = -EBUSY;
+			goto out_vgic_unlock;
+		}
+	}
+
+	/*
+	 * Move all pending IRQs from the LRs on all VCPUs so the pending
+	 * state can be properly represented in the register state accessible
+	 * through this API.
+	 */
+	kvm_for_each_vcpu(c, tmp_vcpu, dev->kvm)
+		vgic_unqueue_irqs(tmp_vcpu);
+
+	offset -= r->base;
+	r->handle_mmio(vcpu, &mmio, offset);
+
+	if (!is_write) {
+		if (len == 8)
+			*(u64 *)reg = le64_to_cpu(data);
+		else
+			*(u32 *)reg = mmio_data_read(&mmio, ~0);
+	}
+
+	ret = 0;
+out_vgic_unlock:
+	spin_unlock(&vgic->lock);
+out:
+	mutex_unlock(&dev->kvm->lock);
+	return ret;
+}
+
 static int vgic_v3_create(struct kvm_device *dev, u32 type)
 {
 	return kvm_vgic_create(dev->kvm, type);
@@ -1000,40 +1102,95 @@ static void vgic_v3_destroy(struct kvm_device *dev)
 	kfree(dev);
 }
 
+static u32 vgic_v3_get_reg_size(struct kvm_device_attr *attr)
+{
+	u32 offset;
+
+	switch (attr->group) {
+	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
+		offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
+		if (offset >= GICD_IROUTER && offset <= 0x7FD8)
+			return 8;
+		else
+			return 4;
+		break;
+
+	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
+		offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
+		if ((offset == GICR_TYPER) ||
+		    (offset >= GICR_SETLPIR && offset <= GICR_INVALLR))
+			return 8;
+		else
+			return 4;
+		break;
+
+	default:
+		return -ENXIO;
+	}
+}
+
 static int vgic_v3_set_attr(struct kvm_device *dev,
 			    struct kvm_device_attr *attr)
 {
-	int ret;
+	int ret, len;
+	u64 reg64;
+	u32 reg;
+	void *data;
 
 	ret = vgic_set_common_attr(dev, attr);
 	if (ret != -ENXIO)
 		return ret;
 
-	switch (attr->group) {
-	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
-	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
-		return -ENXIO;
+	len = vgic_v3_get_reg_size(attr);
+	if (len < 0)
+		return len;
+
+	if (len == 8) {
+		u64 __user *uaddr = (u64 __user *)(long)attr->addr;
+
+		ret = get_user(reg64, uaddr);
+		data = &reg64;
+	} else {
+		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
+
+		ret = get_user(reg, uaddr);
+		data = &reg;
 	}
+	if (ret)
+		return -EFAULT;
 
-	return -ENXIO;
+	return vgic_v3_attr_regs_access(dev, attr, data, len, true);
 }
 
 static int vgic_v3_get_attr(struct kvm_device *dev,
 			    struct kvm_device_attr *attr)
 {
-	int ret;
+	int ret, len;
+	u64 reg64;
+	u32 reg;
 
 	ret = vgic_get_common_attr(dev, attr);
 	if (ret != -ENXIO)
 		return ret;
 
-	switch (attr->group) {
-	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
-	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
-		return -ENXIO;
-	}
+	len = vgic_v3_get_reg_size(attr);
+	if (len < 0)
+		return len;
 
-	return -ENXIO;
+	ret = vgic_v3_attr_regs_access(dev, attr, (len == 8) ? (void *)&reg64 :
+					(void *)&reg, len, false);
+	if (ret)
+		return ret;
+
+	if (len == 8) {
+		u64 __user *uaddr = (u64 __user *)(long)attr->addr;
+
+		return put_user(reg64, uaddr);
+	} else {
+		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
+
+		return put_user(reg, uaddr);
+	}
 }
 
 static int vgic_v3_has_attr(struct kvm_device *dev,
@@ -1051,8 +1208,7 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
 		}
 		break;
 	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
-	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
-		return -ENXIO;
+	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
 	case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
 		return 0;
 	case KVM_DEV_ARM_VGIC_GRP_CTRL:
-- 
2.4.4

Re: [PATCH 1/3] KVM: arm64: Implement vGICv3 distributor and redistributor access from userspace

[Christoffer Dall]

On Fri, Aug 28, 2015 at 03:56:10PM +0300, Pavel Fedin wrote:
> The access is done similar to GICv2, using KVM_DEV_ARM_VGIC_GRP_DIST_REGS
> and KVM_DEV_ARM_VGIC_GRP_REDIST_REGS with KVM_SET_DEVICE_ATTR and
> KVM_GET_DEVICE_ATTR ioctls.
> 
> Registers are always assumed to be of their native size, 4 or 8 bytes.
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  arch/arm64/include/uapi/asm/kvm.h |   1 +
>  virt/kvm/arm/vgic-v3-emul.c       | 186 +++++++++++++++++++++++++++++++++++---
>  2 files changed, 172 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 0cd7b59..2936651 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -203,6 +203,7 @@ struct kvm_arch_memory_slot {
>  #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS	3
>  #define KVM_DEV_ARM_VGIC_GRP_CTRL	4
>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT	0
> +#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
>  
>  /* KVM_IRQ_LINE irq field index values */
>  #define KVM_ARM_IRQ_TYPE_SHIFT		24
> diff --git a/virt/kvm/arm/vgic-v3-emul.c b/virt/kvm/arm/vgic-v3-emul.c
> index e661e7f..b3847e1 100644
> --- a/virt/kvm/arm/vgic-v3-emul.c
> +++ b/virt/kvm/arm/vgic-v3-emul.c
> @@ -39,6 +39,7 @@
>  #include <linux/kvm.h>
>  #include <linux/kvm_host.h>
>  #include <linux/interrupt.h>
> +#include <linux/uaccess.h>
>  
>  #include <linux/irqchip/arm-gic-v3.h>
>  #include <kvm/arm_vgic.h>
> @@ -990,6 +991,107 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
>  		vgic_kick_vcpus(vcpu->kvm);
>  }
>  
> +static int vgic_v3_attr_regs_access(struct kvm_device *dev,
> +				 struct kvm_device_attr *attr,
> +				 void *reg, u32 len, bool is_write)

using a void pointer for the register with variable length here is
likely to cause endianness headaches.  Can we use a typed pointer here?

> +{
> +	const struct vgic_io_range *r = NULL, *ranges;
> +	phys_addr_t offset;
> +	int ret, cpuid, c;
> +	struct kvm_vcpu *vcpu, *tmp_vcpu;
> +	struct vgic_dist *vgic;
> +	struct kvm_exit_mmio mmio;
> +	u64 data;
> +
> +	offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
> +	cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
> +		KVM_DEV_ARM_VGIC_CPUID_SHIFT;
> +
> +	mutex_lock(&dev->kvm->lock);
> +
> +	ret = vgic_init(dev->kvm);
> +	if (ret)
> +		goto out;
> +
> +	if (cpuid >= atomic_read(&dev->kvm->online_vcpus)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	vcpu = kvm_get_vcpu(dev->kvm, cpuid);
> +	vgic = &dev->kvm->arch.vgic;
> +
> +	mmio.len = len;
> +	mmio.is_write = is_write;
> +	mmio.data = &data;
> +	if (is_write) {
> +		if (len == 8)
> +			data = cpu_to_le64(*((u64 *)reg));
> +		else
> +			mmio_data_write(&mmio, ~0, *((u32 *)reg));
> +	}
> +	switch (attr->group) {
> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> +		mmio.phys_addr = vgic->vgic_dist_base + offset;
> +		ranges = vgic_v3_dist_ranges;
> +		break;
> +	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
> +		mmio.phys_addr = vgic->vgic_redist_base + offset;
> +		ranges = vgic_redist_ranges;
> +		break;
> +	default:
> +		BUG();
> +	}
> +	r = vgic_find_range(ranges, 4, offset);
> +
> +	if (unlikely(!r || !r->handle_mmio)) {
> +		ret = -ENXIO;
> +		goto out;
> +	}
> +
> +
> +	spin_lock(&vgic->lock);
> +
> +	/*
> +	 * Ensure that no other VCPU is running by checking the vcpu->cpu
> +	 * field.  If no other VPCUs are running we can safely access the VGIC
> +	 * state, because even if another VPU is run after this point, that
> +	 * VCPU will not touch the vgic state, because it will block on
> +	 * getting the vgic->lock in kvm_vgic_sync_hwstate().
> +	 */
> +	kvm_for_each_vcpu(c, tmp_vcpu, dev->kvm) {
> +		if (unlikely(tmp_vcpu->cpu != -1)) {
> +			ret = -EBUSY;
> +			goto out_vgic_unlock;
> +		}
> +	}
> +
> +	/*
> +	 * Move all pending IRQs from the LRs on all VCPUs so the pending
> +	 * state can be properly represented in the register state accessible
> +	 * through this API.
> +	 */
> +	kvm_for_each_vcpu(c, tmp_vcpu, dev->kvm)
> +		vgic_unqueue_irqs(tmp_vcpu);
> +
> +	offset -= r->base;
> +	r->handle_mmio(vcpu, &mmio, offset);
> +
> +	if (!is_write) {
> +		if (len == 8)
> +			*(u64 *)reg = le64_to_cpu(data);
> +		else
> +			*(u32 *)reg = mmio_data_read(&mmio, ~0);
> +	}
> +
> +	ret = 0;
> +out_vgic_unlock:
> +	spin_unlock(&vgic->lock);
> +out:
> +	mutex_unlock(&dev->kvm->lock);
> +	return ret;

I feel like there's a lot of reused code with the v2 vgic here.  Can you
look at reusing some of the logic?

> +}
> +
>  static int vgic_v3_create(struct kvm_device *dev, u32 type)
>  {
>  	return kvm_vgic_create(dev->kvm, type);
> @@ -1000,40 +1102,95 @@ static void vgic_v3_destroy(struct kvm_device *dev)
>  	kfree(dev);
>  }
>  
> +static u32 vgic_v3_get_reg_size(struct kvm_device_attr *attr)
> +{
> +	u32 offset;
> +
> +	switch (attr->group) {
> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> +		offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
> +		if (offset >= GICD_IROUTER && offset <= 0x7FD8)

eh, 0x7FD8 ?

> +			return 8;
> +		else
> +			return 4;
> +		break;
> +
> +	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
> +		offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
> +		if ((offset == GICR_TYPER) ||
> +		    (offset >= GICR_SETLPIR && offset <= GICR_INVALLR))
> +			return 8;
> +		else
> +			return 4;
> +		break;
> +
> +	default:
> +		return -ENXIO;
> +	}
> +}

this feels wrong.

How about encoding the userspace requested access size in the reserved
bits of the attr field similarly to how the register indicies for the
SET_ONE/GET_ONE ioctls work and then you can enforce specific access
length restrictions in the individual register access functions.

> +
>  static int vgic_v3_set_attr(struct kvm_device *dev,
>  			    struct kvm_device_attr *attr)
>  {
> -	int ret;
> +	int ret, len;
> +	u64 reg64;
> +	u32 reg;
> +	void *data;
>  
>  	ret = vgic_set_common_attr(dev, attr);
>  	if (ret != -ENXIO)
>  		return ret;
>  
> -	switch (attr->group) {
> -	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> -	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
> -		return -ENXIO;
> +	len = vgic_v3_get_reg_size(attr);
> +	if (len < 0)
> +		return len;
> +
> +	if (len == 8) {
> +		u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> +
> +		ret = get_user(reg64, uaddr);
> +		data = &reg64;
> +	} else {
> +		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> +
> +		ret = get_user(reg, uaddr);
> +		data = &reg;
>  	}
> +	if (ret)
> +		return -EFAULT;
>  
> -	return -ENXIO;
> +	return vgic_v3_attr_regs_access(dev, attr, data, len, true);
>  }
>  
>  static int vgic_v3_get_attr(struct kvm_device *dev,
>  			    struct kvm_device_attr *attr)
>  {
> -	int ret;
> +	int ret, len;
> +	u64 reg64;
> +	u32 reg;
>  
>  	ret = vgic_get_common_attr(dev, attr);
>  	if (ret != -ENXIO)
>  		return ret;
>  
> -	switch (attr->group) {
> -	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> -	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
> -		return -ENXIO;
> -	}
> +	len = vgic_v3_get_reg_size(attr);
> +	if (len < 0)
> +		return len;
>  
> -	return -ENXIO;
> +	ret = vgic_v3_attr_regs_access(dev, attr, (len == 8) ? (void *)&reg64 :
> +					(void *)&reg, len, false);

this use of the ternary operator is terrible, but it should be solved if
you always use a u64 for the reg parameter.

> +	if (ret)
> +		return ret;
> +
> +	if (len == 8) {
> +		u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> +
> +		return put_user(reg64, uaddr);
> +	} else {
> +		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> +
> +		return put_user(reg, uaddr);
> +	}
>  }
>  
>  static int vgic_v3_has_attr(struct kvm_device *dev,
> @@ -1051,8 +1208,7 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
>  		}
>  		break;
>  	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> -	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
> -		return -ENXIO;
> +	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
>  	case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
>  		return 0;
>  	case KVM_DEV_ARM_VGIC_GRP_CTRL:
> -- 
> 2.4.4
> 

RE: [PATCH 1/3] KVM: arm64: Implement vGICv3 distributor and redistributor access from userspace

[Pavel Fedin]

 Hello!

> > +	len = vgic_v3_get_reg_size(attr);
> > +	if (len < 0)
> > +		return len;
> >
> > -	return -ENXIO;
> > +	ret = vgic_v3_attr_regs_access(dev, attr, (len == 8) ? (void *)&reg64 :
> > +					(void *)&reg, len, false);
> 
> this use of the ternary operator is terrible, but it should be solved if
> you always use a u64 for the reg parameter.

 I also dislike this, but this is the best thing i could invent. This is dictated by put_user() and
get_user(), which rely on typeof() of their arguments. Well, i could do some castings, but they are
no less ugly, and would give more headache to bigendian systems.
However, what about doing the same thing as GET/SET_ONE_REG does by just assuming that everything is
64-bit wide? This would automatically resolve two other issues you have commented on. By the way,
handling it in userspace would also be simpler.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH 1/3] KVM: arm64: Implement vGICv3 distributor and redistributor access from userspace

[Christoffer Dall]

On Mon, Aug 31, 2015 at 10:35:05AM +0300, Pavel Fedin wrote:
>  Hello!
> 
> > > +	len = vgic_v3_get_reg_size(attr);
> > > +	if (len < 0)
> > > +		return len;
> > >
> > > -	return -ENXIO;
> > > +	ret = vgic_v3_attr_regs_access(dev, attr, (len == 8) ? (void *)&reg64 :
> > > +					(void *)&reg, len, false);
> > 
> > this use of the ternary operator is terrible, but it should be solved if
> > you always use a u64 for the reg parameter.
> 
>  I also dislike this, but this is the best thing i could invent. This is dictated by put_user() and
> get_user(), which rely on typeof() of their arguments. Well, i could do some castings, but they are
> no less ugly, and would give more headache to bigendian systems.
> However, what about doing the same thing as GET/SET_ONE_REG does by just assuming that everything is
> 64-bit wide? This would automatically resolve two other issues you have commented on. By the way,
> handling it in userspace would also be simpler.
> 
Sounds fine to me, definitely if you must do a cast doing it in a
function between typed variables is strictly preferred to passing void *
values between functions.

-Christoffer

Re: [PATCH 1/3] KVM: arm64: Implement vGICv3 distributor and redistributor access from userspace

[Andre Przywara]

Hi Pavel,

...

>> diff --git a/virt/kvm/arm/vgic-v3-emul.c b/virt/kvm/arm/vgic-v3-emul.c
>> index e661e7f..b3847e1 100644
>> --- a/virt/kvm/arm/vgic-v3-emul.c
>> +++ b/virt/kvm/arm/vgic-v3-emul.c
...
>> @@ -1000,40 +1102,95 @@ static void vgic_v3_destroy(struct kvm_device *dev)
>>  	kfree(dev);
>>  }
>>  
>> +static u32 vgic_v3_get_reg_size(struct kvm_device_attr *attr)
>> +{
>> +	u32 offset;
>> +
>> +	switch (attr->group) {
>> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
>> +		offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
>> +		if (offset >= GICD_IROUTER && offset <= 0x7FD8)
> 
> eh, 0x7FD8 ?
> 
>> +			return 8;
>> +		else
>> +			return 4;
>> +		break;
>> +
>> +	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
>> +		offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
>> +		if ((offset == GICR_TYPER) ||
>> +		    (offset >= GICR_SETLPIR && offset <= GICR_INVALLR))
>> +			return 8;
>> +		else
>> +			return 4;
>> +		break;
>> +
>> +	default:
>> +		return -ENXIO;
>> +	}
>> +}
> 
> this feels wrong.

I agree on this, actually I consider this dangerous. Currently the
memory behind addr in QEMU (hw/intc/arm_gic_kvm.c:kvm_arm_gic_get() for
instance) is only uint32_t, so you have to take care to provide uint64_t
backing for those registers, which means that there must be a match
between the register size the kernel knows and the size userland thinks
of. So I'd rather see the access size controlled by userland, probably
using Christoffer's suggestion below.

Also the GIC specification says that everything must be accessible with
32-bit accesses. Correct me if I am wrong on this, but vCPUs are not
supposed to run while you are getting/setting VGIC registers, right? So
there shouldn't be any issues with non-atomic accesses to 64-bit
registers, which means you could just go ahead and do everything in
32-bit only. This would also help with supporting 32-bit userland and/or
kernel later.

Cheers,
Andre.

RE: [PATCH 1/3] KVM: arm64: Implement vGICv3 distributor and redistributor access from userspace

[Pavel Fedin]

 Hello!

> I agree on this, actually I consider this dangerous. Currently the
> memory behind addr in QEMU (hw/intc/arm_gic_kvm.c:kvm_arm_gic_get() for
> instance) is only uint32_t, so you have to take care to provide uint64_t
> backing for those registers, which means that there must be a match
> between the register size the kernel knows and the size userland thinks
> of. So I'd rather see the access size controlled by userland

 Ok, i will implement it this way.

> Also the GIC specification says that everything must be accessible with
> 32-bit accesses. Correct me if I am wrong on this, but vCPUs are not
> supposed to run while you are getting/setting VGIC registers, right?

 Right.

> So there shouldn't be any issues with non-atomic accesses to 64-bit
> registers, which means you could just go ahead and do everything in
> 32-bit only.

 I thought about it too, it's inconvenient. In the userland you would have to do two accesses and
merge the result. It's just tedious. After all this API is not emulating guest behavior, it's just
for reading/writing GIC state.
 So on next respin i'll add size bit.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH 1/3] KVM: arm64: Implement vGICv3 distributor and redistributor access from userspace

[Peter Maydell]

On 1 September 2015 at 14:52, Andre Przywara <andre.przywara@arm.com> wrote:
> Also the GIC specification says that everything must be accessible with
> 32-bit accesses. Correct me if I am wrong on this, but vCPUs are not
> supposed to run while you are getting/setting VGIC registers, right? So
> there shouldn't be any issues with non-atomic accesses to 64-bit
> registers, which means you could just go ahead and do everything in
> 32-bit only. This would also help with supporting 32-bit userland and/or
> kernel later.

We should design the userspace API based on the natural size
of the registers in the GICv3 spec, not on what happens to
be convenient for the kernel to implement. There's only one
kernel but there can be multiple userspace consumers of the API...

I don't see any reason why a 32-bit userland wouldn't be able
to handle 64-bit accesses via the KVM_SET/GET_DEVICE_ATTR
ioctls, or am I missing something?

thanks
-- PMM

[PATCH 2/3] KVM: arm64: Allow to use accessors in KVM_SET_ONE_REG and KVM_GET_ONE_REG

[Pavel Fedin]

Call accessor functions if the register is defined in the list but does
not have accociated cell in CPU context.

This allows to implement accessors for vGIC CPU interface, necessary for
live migration.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 arch/arm64/kvm/sys_regs.c | 47 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 41 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index b41607d..8cc4a5e 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1306,10 +1306,6 @@ static const struct sys_reg_desc *index_to_sys_reg_desc(struct kvm_vcpu *vcpu,
 	if (!r)
 		r = find_reg(&params, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
 
-	/* Not saved in the sys_reg array? */
-	if (r && !r->reg)
-		r = NULL;
-
 	return r;
 }
 
@@ -1533,6 +1529,7 @@ int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
 {
 	const struct sys_reg_desc *r;
 	void __user *uaddr = (void __user *)(unsigned long)reg->addr;
+	u64 val;
 
 	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
 		return demux_c15_get(reg->id, uaddr);
@@ -1547,13 +1544,31 @@ int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
 	if (r->get_user)
 		return (r->get_user)(vcpu, r, reg, uaddr);
 
-	return reg_to_user(uaddr, &vcpu_sys_reg(vcpu, r->reg), reg->id);
+	if (r->reg) {
+		val = vcpu_sys_reg(vcpu, r->reg);
+	} else {
+		struct sys_reg_params params;
+
+		if (!index_to_params(reg->id, &params))
+			return -ENOENT;
+
+		params.is_write = false;
+		params.is_aarch32 = false;
+		params.is_32bit = false;
+
+		if (!r->access(vcpu, &params, r))
+			return -EINVAL;
+	}
+
+	return reg_to_user(uaddr, &val, reg->id);
 }
 
 int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	const struct sys_reg_desc *r;
 	void __user *uaddr = (void __user *)(unsigned long)reg->addr;
+	u64 val;
+	int ret;
 
 	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
 		return demux_c15_set(reg->id, uaddr);
@@ -1568,7 +1583,27 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
 	if (r->set_user)
 		return (r->set_user)(vcpu, r, reg, uaddr);
 
-	return reg_from_user(&vcpu_sys_reg(vcpu, r->reg), uaddr, reg->id);
+	ret = reg_from_user(&val, uaddr, reg->id);
+	if (ret)
+		return ret;
+
+	if (r->reg) {
+		vcpu_sys_reg(vcpu, r->reg) = val;
+	} else {
+		struct sys_reg_params params;
+
+		if (!index_to_params(reg->id, &params))
+			return -ENOENT;
+
+		params.is_write = true;
+		params.is_aarch32 = false;
+		params.is_32bit = false;
+
+		if (!r->access(vcpu, &params, r))
+			return -EINVAL;
+	}
+
+	return 0;
 }
 
 static unsigned int num_demux_regs(void)
-- 
2.4.4

[PATCH 3/3] KVM: arm64: Implement accessors for vGIC CPU interface registers

[Pavel Fedin]

This commit adds accessors for all registers, being part of saved vGIC
context in the form of ICH_VMCR_EL2. This is necessary for enabling vGICv3
live migration.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 arch/arm64/kvm/sys_regs.c          | 176 +++++++++++++++++++++++++++++++++++++
 include/linux/irqchip/arm-gic-v3.h |  18 +++-
 2 files changed, 192 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 8cc4a5e..7a4f982 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -23,6 +23,7 @@
 #include <linux/kvm_host.h>
 #include <linux/mm.h>
 #include <linux/uaccess.h>
+#include <linux/irqchip/arm-gic-v3.h>
 
 #include <asm/cacheflush.h>
 #include <asm/cputype.h>
@@ -136,6 +137,162 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+static bool access_gic_ctlr(struct kvm_vcpu *vcpu,
+			    const struct sys_reg_params *p,
+			    const struct sys_reg_desc *r)
+{
+	u64 val;
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
+		return false;
+
+	if (p->is_write) {
+		val = *vcpu_reg(vcpu, p->Rt);
+
+		vgicv3->vgic_vmcr &= ~(ICH_VMCR_CBPR|ICH_VMCR_EOIM);
+		vgicv3->vgic_vmcr |= (val << (ICH_VMCR_CBPR_SHIFT -
+						ICC_CTLR_EL1_CBPR_SHIFT)) &
+					ICH_VMCR_CBPR;
+		vgicv3->vgic_vmcr |= (val << (ICH_VMCR_EOIM_SHIFT -
+						ICC_CTLR_EL1_EOImode_SHIFT)) &
+					ICH_VMCR_EOIM;
+	} else {
+		asm volatile("mrs_s %0," __stringify(ICC_IAR1_EL1)
+			     : "=r" (val));
+		val &= (ICC_CTLR_EL1_A3V | ICC_CTLR_EL1_SEIS |
+			ICC_CTLR_EL1_IDbits_MASK | ICC_CTLR_EL1_PRIbits_MASK);
+		val |= (vgicv3->vgic_vmcr & ICH_VMCR_CBPR) >>
+			(ICH_VMCR_CBPR_SHIFT - ICC_CTLR_EL1_CBPR_SHIFT);
+		val |= (vgicv3->vgic_vmcr & ICH_VMCR_EOIM) >>
+			(ICH_VMCR_EOIM_SHIFT - ICC_CTLR_EL1_EOImode_SHIFT);
+
+		*vcpu_reg(vcpu, p->Rt) = val;
+	}
+
+	return true;
+}
+
+static bool access_gic_pmr(struct kvm_vcpu *vcpu,
+			   const struct sys_reg_params *p,
+			   const struct sys_reg_desc *r)
+{
+	u64 val;
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
+		return false;
+
+	if (p->is_write) {
+		val = *vcpu_reg(vcpu, p->Rt);
+		vgicv3->vgic_vmcr &= ~ICH_VMCR_PMR_MASK;
+		vgicv3->vgic_vmcr |= (val << ICH_VMCR_PMR_SHIFT) &
+					ICH_VMCR_PMR_MASK;
+	} else {
+		val = (vgicv3->vgic_vmcr & ICH_VMCR_PMR_MASK) >>
+			ICH_VMCR_PMR_SHIFT;
+		*vcpu_reg(vcpu, p->Rt) = val;
+	}
+
+	return true;
+}
+
+static bool access_gic_bpr0(struct kvm_vcpu *vcpu,
+			    const struct sys_reg_params *p,
+			    const struct sys_reg_desc *r)
+{
+	u64 val;
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
+		return false;
+
+	if (p->is_write) {
+		val = *vcpu_reg(vcpu, p->Rt);
+		vgicv3->vgic_vmcr &= ~ICH_VMCR_BPR0_MASK;
+		vgicv3->vgic_vmcr |= (val << ICH_VMCR_BPR0_SHIFT) &
+					ICH_VMCR_BPR0_MASK;
+	} else {
+		val = (vgicv3->vgic_vmcr & ICH_VMCR_BPR0_MASK) >>
+			ICH_VMCR_BPR0_SHIFT;
+		*vcpu_reg(vcpu, p->Rt) = val;
+	}
+
+	return true;
+}
+
+static bool access_gic_bpr1(struct kvm_vcpu *vcpu,
+			    const struct sys_reg_params *p,
+			    const struct sys_reg_desc *r)
+{
+	u64 val;
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
+		return false;
+
+	if (p->is_write) {
+		val = *vcpu_reg(vcpu, p->Rt);
+		vgicv3->vgic_vmcr &= ~ICH_VMCR_BPR1_MASK;
+		vgicv3->vgic_vmcr |= (val << ICH_VMCR_BPR1_SHIFT) &
+					ICH_VMCR_BPR1_MASK;
+	} else {
+		val = (vgicv3->vgic_vmcr & ICH_VMCR_BPR1_MASK) >>
+			ICH_VMCR_BPR1_SHIFT;
+		*vcpu_reg(vcpu, p->Rt) = val;
+	}
+
+	return true;
+}
+
+static bool access_gic_grpen0(struct kvm_vcpu *vcpu,
+			      const struct sys_reg_params *p,
+			      const struct sys_reg_desc *r)
+{
+	u64 val;
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
+		return false;
+
+	if (p->is_write) {
+		val = *vcpu_reg(vcpu, p->Rt);
+		vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG0;
+		vgicv3->vgic_vmcr |= (val << ICH_VMCR_ENG0_SHIFT) &
+					ICH_VMCR_ENG0;
+	} else {
+		val = (vgicv3->vgic_vmcr & ICH_VMCR_ENG0) >>
+			ICH_VMCR_ENG0_SHIFT;
+		*vcpu_reg(vcpu, p->Rt) = val;
+	}
+
+	return true;
+}
+
+static bool access_gic_grpen1(struct kvm_vcpu *vcpu,
+			      const struct sys_reg_params *p,
+			      const struct sys_reg_desc *r)
+{
+	u64 val;
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
+		return false;
+
+	if (p->is_write) {
+		val = *vcpu_reg(vcpu, p->Rt);
+		vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG1;
+		vgicv3->vgic_vmcr |= (val << ICH_VMCR_ENG1_SHIFT) &
+					ICH_VMCR_ENG1;
+	} else {
+		val = (vgicv3->vgic_vmcr & ICH_VMCR_ENG1) >>
+			ICH_VMCR_ENG1_SHIFT;
+		*vcpu_reg(vcpu, p->Rt) = val;
+	}
+
+	return true;
+}
+
 static bool trap_raz_wi(struct kvm_vcpu *vcpu,
 			const struct sys_reg_params *p,
 			const struct sys_reg_desc *r)
@@ -579,6 +736,10 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ Op0(0b11), Op1(0b000), CRn(0b0010), CRm(0b0000), Op2(0b010),
 	  access_vm_reg, reset_val, TCR_EL1, 0 },
 
+	/* ICC_PMR_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b0100), CRm(0b0110), Op2(0b000),
+	  access_gic_pmr },
+
 	/* AFSR0_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b0101), CRm(0b0001), Op2(0b000),
 	  access_vm_reg, reset_unknown, AFSR0_EL1 },
@@ -613,12 +774,27 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b0000), Op2(0b000),
 	  NULL, reset_val, VBAR_EL1, 0 },
 
+	/* ICC_BPR0_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1000), Op2(0b011),
+	  access_gic_bpr0 },
 	/* ICC_SGI1R_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1011), Op2(0b101),
 	  access_gic_sgi },
+	/* ICC_BPR1_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b011),
+	  access_gic_bpr1 },
+	/* ICC_CTLR_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b100),
+	  access_gic_ctlr },
 	/* ICC_SRE_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b101),
 	  trap_raz_wi },
+	/* ICC_IGRPEN0_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b110),
+	  access_gic_grpen0 },
+	/* ICC_GRPEN1_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b111),
+	  access_gic_grpen1 },
 
 	/* CONTEXTIDR_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b1101), CRm(0b0000), Op2(0b001),
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index ed0fc9f..7e9fc16 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -257,8 +257,14 @@
 /*
  * CPU interface registers
  */
-#define ICC_CTLR_EL1_EOImode_drop_dir	(0U << 1)
-#define ICC_CTLR_EL1_EOImode_drop	(1U << 1)
+#define ICC_CTLR_EL1_CBPR_SHIFT		0
+#define ICC_CTLR_EL1_EOImode_SHIFT	1
+#define ICC_CTLR_EL1_EOImode_drop_dir	(0U << ICC_CTLR_EL1_EOImode_SHIFT)
+#define ICC_CTLR_EL1_EOImode_drop	(1U << ICC_CTLR_EL1_EOImode_SHIFT)
+#define ICC_CTLR_EL1_PRIbits_MASK	(7U << 8)
+#define ICC_CTLR_EL1_IDbits_MASK	(7U << 11)
+#define ICC_CTLR_EL1_SEIS		(1U << 14)
+#define ICC_CTLR_EL1_A3V		(1U << 15)
 #define ICC_SRE_EL1_SRE			(1U << 0)
 
 /*
@@ -283,6 +289,14 @@
 
 #define ICH_VMCR_CTLR_SHIFT		0
 #define ICH_VMCR_CTLR_MASK		(0x21f << ICH_VMCR_CTLR_SHIFT)
+#define ICH_VMCR_ENG0_SHIFT		0
+#define ICH_VMCR_ENG0			(1 << ICH_VMCR_ENG0_SHIFT)
+#define ICH_VMCR_ENG1_SHIFT		1
+#define ICH_VMCR_ENG1			(1 << ICH_VMCR_ENG1_SHIFT)
+#define ICH_VMCR_CBPR_SHIFT		4
+#define ICH_VMCR_CBPR			(1 << ICH_VMCR_CBPR_SHIFT)
+#define ICH_VMCR_EOIM_SHIFT		9
+#define ICH_VMCR_EOIM			(1 << ICH_VMCR_EOIM_SHIFT)
 #define ICH_VMCR_BPR1_SHIFT		18
 #define ICH_VMCR_BPR1_MASK		(7 << ICH_VMCR_BPR1_SHIFT)
 #define ICH_VMCR_BPR0_SHIFT		21
-- 
2.4.4

Re: [PATCH 3/3] KVM: arm64: Implement accessors for vGIC CPU interface registers

[Christoffer Dall]

On Fri, Aug 28, 2015 at 03:56:12PM +0300, Pavel Fedin wrote:
> This commit adds accessors for all registers, being part of saved vGIC
> context in the form of ICH_VMCR_EL2. This is necessary for enabling vGICv3
> live migration.
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  arch/arm64/kvm/sys_regs.c          | 176 +++++++++++++++++++++++++++++++++++++
>  include/linux/irqchip/arm-gic-v3.h |  18 +++-
>  2 files changed, 192 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 8cc4a5e..7a4f982 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -23,6 +23,7 @@
>  #include <linux/kvm_host.h>
>  #include <linux/mm.h>
>  #include <linux/uaccess.h>
> +#include <linux/irqchip/arm-gic-v3.h>
>  
>  #include <asm/cacheflush.h>
>  #include <asm/cputype.h>
> @@ -136,6 +137,162 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> +static bool access_gic_ctlr(struct kvm_vcpu *vcpu,
> +			    const struct sys_reg_params *p,
> +			    const struct sys_reg_desc *r)
> +{
> +	u64 val;
> +	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> +	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
> +		return false;
> +
> +	if (p->is_write) {
> +		val = *vcpu_reg(vcpu, p->Rt);
> +
> +		vgicv3->vgic_vmcr &= ~(ICH_VMCR_CBPR|ICH_VMCR_EOIM);
> +		vgicv3->vgic_vmcr |= (val << (ICH_VMCR_CBPR_SHIFT -
> +						ICC_CTLR_EL1_CBPR_SHIFT)) &
> +					ICH_VMCR_CBPR;
> +		vgicv3->vgic_vmcr |= (val << (ICH_VMCR_EOIM_SHIFT -
> +						ICC_CTLR_EL1_EOImode_SHIFT)) &
> +					ICH_VMCR_EOIM;
> +	} else {
> +		asm volatile("mrs_s %0," __stringify(ICC_IAR1_EL1)
> +			     : "=r" (val));
> +		val &= (ICC_CTLR_EL1_A3V | ICC_CTLR_EL1_SEIS |
> +			ICC_CTLR_EL1_IDbits_MASK | ICC_CTLR_EL1_PRIbits_MASK);
> +		val |= (vgicv3->vgic_vmcr & ICH_VMCR_CBPR) >>
> +			(ICH_VMCR_CBPR_SHIFT - ICC_CTLR_EL1_CBPR_SHIFT);
> +		val |= (vgicv3->vgic_vmcr & ICH_VMCR_EOIM) >>
> +			(ICH_VMCR_EOIM_SHIFT - ICC_CTLR_EL1_EOImode_SHIFT);
> +
> +		*vcpu_reg(vcpu, p->Rt) = val;
> +	}
> +
> +	return true;
> +}
> +
> +static bool access_gic_pmr(struct kvm_vcpu *vcpu,
> +			   const struct sys_reg_params *p,
> +			   const struct sys_reg_desc *r)
> +{
> +	u64 val;
> +	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> +	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
> +		return false;
> +
> +	if (p->is_write) {
> +		val = *vcpu_reg(vcpu, p->Rt);
> +		vgicv3->vgic_vmcr &= ~ICH_VMCR_PMR_MASK;
> +		vgicv3->vgic_vmcr |= (val << ICH_VMCR_PMR_SHIFT) &
> +					ICH_VMCR_PMR_MASK;
> +	} else {
> +		val = (vgicv3->vgic_vmcr & ICH_VMCR_PMR_MASK) >>
> +			ICH_VMCR_PMR_SHIFT;
> +		*vcpu_reg(vcpu, p->Rt) = val;
> +	}
> +
> +	return true;
> +}
> +
> +static bool access_gic_bpr0(struct kvm_vcpu *vcpu,
> +			    const struct sys_reg_params *p,
> +			    const struct sys_reg_desc *r)
> +{
> +	u64 val;
> +	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> +	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
> +		return false;
> +
> +	if (p->is_write) {
> +		val = *vcpu_reg(vcpu, p->Rt);
> +		vgicv3->vgic_vmcr &= ~ICH_VMCR_BPR0_MASK;
> +		vgicv3->vgic_vmcr |= (val << ICH_VMCR_BPR0_SHIFT) &
> +					ICH_VMCR_BPR0_MASK;
> +	} else {
> +		val = (vgicv3->vgic_vmcr & ICH_VMCR_BPR0_MASK) >>
> +			ICH_VMCR_BPR0_SHIFT;
> +		*vcpu_reg(vcpu, p->Rt) = val;
> +	}
> +
> +	return true;
> +}
> +
> +static bool access_gic_bpr1(struct kvm_vcpu *vcpu,
> +			    const struct sys_reg_params *p,
> +			    const struct sys_reg_desc *r)
> +{
> +	u64 val;
> +	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> +	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
> +		return false;
> +
> +	if (p->is_write) {
> +		val = *vcpu_reg(vcpu, p->Rt);
> +		vgicv3->vgic_vmcr &= ~ICH_VMCR_BPR1_MASK;
> +		vgicv3->vgic_vmcr |= (val << ICH_VMCR_BPR1_SHIFT) &
> +					ICH_VMCR_BPR1_MASK;
> +	} else {
> +		val = (vgicv3->vgic_vmcr & ICH_VMCR_BPR1_MASK) >>
> +			ICH_VMCR_BPR1_SHIFT;
> +		*vcpu_reg(vcpu, p->Rt) = val;
> +	}
> +
> +	return true;
> +}
> +
> +static bool access_gic_grpen0(struct kvm_vcpu *vcpu,
> +			      const struct sys_reg_params *p,
> +			      const struct sys_reg_desc *r)
> +{
> +	u64 val;
> +	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> +	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
> +		return false;
> +
> +	if (p->is_write) {
> +		val = *vcpu_reg(vcpu, p->Rt);
> +		vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG0;
> +		vgicv3->vgic_vmcr |= (val << ICH_VMCR_ENG0_SHIFT) &
> +					ICH_VMCR_ENG0;
> +	} else {
> +		val = (vgicv3->vgic_vmcr & ICH_VMCR_ENG0) >>
> +			ICH_VMCR_ENG0_SHIFT;
> +		*vcpu_reg(vcpu, p->Rt) = val;
> +	}
> +
> +	return true;
> +}
> +
> +static bool access_gic_grpen1(struct kvm_vcpu *vcpu,
> +			      const struct sys_reg_params *p,
> +			      const struct sys_reg_desc *r)
> +{
> +	u64 val;
> +	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> +	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
> +		return false;
> +
> +	if (p->is_write) {
> +		val = *vcpu_reg(vcpu, p->Rt);
> +		vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG1;
> +		vgicv3->vgic_vmcr |= (val << ICH_VMCR_ENG1_SHIFT) &
> +					ICH_VMCR_ENG1;
> +	} else {
> +		val = (vgicv3->vgic_vmcr & ICH_VMCR_ENG1) >>
> +			ICH_VMCR_ENG1_SHIFT;
> +		*vcpu_reg(vcpu, p->Rt) = val;
> +	}
> +
> +	return true;
> +}
> +
>  static bool trap_raz_wi(struct kvm_vcpu *vcpu,
>  			const struct sys_reg_params *p,
>  			const struct sys_reg_desc *r)
> @@ -579,6 +736,10 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	{ Op0(0b11), Op1(0b000), CRn(0b0010), CRm(0b0000), Op2(0b010),
>  	  access_vm_reg, reset_val, TCR_EL1, 0 },
>  
> +	/* ICC_PMR_EL1 */
> +	{ Op0(0b11), Op1(0b000), CRn(0b0100), CRm(0b0110), Op2(0b000),
> +	  access_gic_pmr },
> +
>  	/* AFSR0_EL1 */
>  	{ Op0(0b11), Op1(0b000), CRn(0b0101), CRm(0b0001), Op2(0b000),
>  	  access_vm_reg, reset_unknown, AFSR0_EL1 },
> @@ -613,12 +774,27 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b0000), Op2(0b000),
>  	  NULL, reset_val, VBAR_EL1, 0 },
>  
> +	/* ICC_BPR0_EL1 */
> +	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1000), Op2(0b011),
> +	  access_gic_bpr0 },
>  	/* ICC_SGI1R_EL1 */
>  	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1011), Op2(0b101),
>  	  access_gic_sgi },
> +	/* ICC_BPR1_EL1 */
> +	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b011),
> +	  access_gic_bpr1 },
> +	/* ICC_CTLR_EL1 */
> +	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b100),
> +	  access_gic_ctlr },
>  	/* ICC_SRE_EL1 */
>  	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b101),
>  	  trap_raz_wi },
> +	/* ICC_IGRPEN0_EL1 */
> +	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b110),
> +	  access_gic_grpen0 },
> +	/* ICC_GRPEN1_EL1 */
> +	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b111),
> +	  access_gic_grpen1 },
>  

I had imagined we would encode the GICv3 register accesses through the
device API and not through the system register API, since I'm not crazy
about polluting the general system register handling logic with GIC
registers solely for the purposes of migration.

I'd much rather have this logic locally to the gic files.

Have you thought about proper locking/serializing of access to the GIC
state in these accessor functions?  Seems like this deserves a comment
in the commit log at least and will probably be easier to understand in
the context of the vgic code.

-Christoffer

Re: [PATCH 3/3] KVM: arm64: Implement accessors for vGIC CPU interface registers

[Peter Maydell]

On 30 August 2015 at 17:50, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> I had imagined we would encode the GICv3 register accesses through the
> device API and not through the system register API, since I'm not crazy
> about polluting the general system register handling logic with GIC
> registers solely for the purposes of migration.

There's an interesting design question lurking under this
about the extent to which you expose the h/w design split
between the CPU interface and the GIC proper as part of the
KVM APIs. I'm inclined to agree that it's better to for our
purposes treat both bits as just part of an irqchip device,
but I haven't given it a great deal of thought.

(Similarly in the QEMU emulated-GICv3 case you could also
split the CPU i/f more formally, or not. The kernel's choice
would have implications for which way QEMU ends up going,
I think.)

thanks
-- PMM

RE: [PATCH 3/3] KVM: arm64: Implement accessors for vGIC CPU interface registers

[Pavel Fedin]

 Hello!

> > I had imagined we would encode the GICv3 register accesses through the
> > device API and not through the system register API, since I'm not crazy
> > about polluting the general system register handling logic with GIC
> > registers solely for the purposes of migration.
> 
> There's an interesting design question lurking under this
> about the extent to which you expose the h/w design split
> between the CPU interface and the GIC proper as part of the
> KVM APIs.

 I could split up handling logic from access logic. So that in sys_regs.c we would have something like:

static bool access_gic_ctlr(struct kvm_vcpu *vcpu,
			    const struct sys_reg_params *p,
			    const struct sys_reg_desc *r)
{
	return vgicv3_access_ctlr(vcpu, vcpu_reg(vcpu, p->Rt), p->is_write);
}

 And in vgic-v3-emul.c we would have the handler itself with the prototype:

bool vgicv3_access_ctlr(struct kvm_vcpu *vcpu, u64 *val, bool write);

 Would this be OK?
 In my personal opinion system register access API fits perfectly well for this task, because after all these are system registers. And implementing this as device attribute would, i guess, give no difference from code's point of view. We would have to encode system register numbers into attribute, then perform table lookup, actually duplicating our system register access code. Does it worth that?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH 3/3] KVM: arm64: Implement accessors for vGIC CPU interface registers

[Christoffer Dall]

On Mon, Aug 31, 2015 at 10:43:27AM +0300, Pavel Fedin wrote:
>  Hello!
> 
> > > I had imagined we would encode the GICv3 register accesses through the
> > > device API and not through the system register API, since I'm not crazy
> > > about polluting the general system register handling logic with GIC
> > > registers solely for the purposes of migration.
> > 
> > There's an interesting design question lurking under this
> > about the extent to which you expose the h/w design split
> > between the CPU interface and the GIC proper as part of the
> > KVM APIs.
> 
>  I could split up handling logic from access logic. So that in sys_regs.c we would have something like:
> 
> static bool access_gic_ctlr(struct kvm_vcpu *vcpu,
> 			    const struct sys_reg_params *p,
> 			    const struct sys_reg_desc *r)
> {
> 	return vgicv3_access_ctlr(vcpu, vcpu_reg(vcpu, p->Rt), p->is_write);
> }
> 
>  And in vgic-v3-emul.c we would have the handler itself with the prototype:
> 
> bool vgicv3_access_ctlr(struct kvm_vcpu *vcpu, u64 *val, bool write);
> 
>  Would this be OK?
>  In my personal opinion system register access API fits perfectly well for this task, because after all these are system registers. And implementing this as device attribute would, i guess, give no difference from code's point of view. We would have to encode system register numbers into attribute, then perform table lookup, actually duplicating our system register access code. Does it worth that?
> 
I think it's worth moving the thing to device attributes, yes,
especially given that I never expect us to trap and emulate GICv3 system
register accesses from a guest in KVM.  Is that correct?

However, if there's a strong argument that this is really CPU state and
not state associated with the GIC device, I'd be willing to reconsider.

-Christoffer

RE: [PATCH 3/3] KVM: arm64: Implement accessors for vGIC CPU interface registers

[Pavel Fedin]

 Hello!

> I think it's worth moving the thing to device attributes, yes,
> especially given that I never expect us to trap and emulate GICv3 system
> register accesses from a guest in KVM.  Is that correct?

 Yes, but nevertheless, for GICv2 attributes we reuse the same code which is expected to trap MMIO
accesses from guest. And there we also have MMIO handlers for the CPU interface, which are also
never trapped from guest. So why cannot we do the same for GICv3 CPU interface, and simply reuse
existing APIs?
 I am currently working on full support in qemu, and it's not difficult to deal with CPU fd's.
Because anyway you have to iterate through all VCPUs in order to save state correctly.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH 3/3] KVM: arm64: Implement accessors for vGIC CPU interface registers

[Christoffer Dall]

On Sun, Aug 30, 2015 at 07:39:05PM +0100, Peter Maydell wrote:
> On 30 August 2015 at 17:50, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> > I had imagined we would encode the GICv3 register accesses through the
> > device API and not through the system register API, since I'm not crazy
> > about polluting the general system register handling logic with GIC
> > registers solely for the purposes of migration.
> 
> There's an interesting design question lurking under this
> about the extent to which you expose the h/w design split
> between the CPU interface and the GIC proper as part of the
> KVM APIs. I'm inclined to agree that it's better to for our
> purposes treat both bits as just part of an irqchip device,
> but I haven't given it a great deal of thought.

Me neither, and I intended to start a discussion around this with my
e-mail.

But as I stated above, I think it will be easier to manage if the state
belongs to the device, in general, but I have fairly little insight into
how this is going to be implemented in QEMU at this point.  But for the
GICv2 implementation, it certainly made a lot of sense to only deal with
one device and file descriptor when getting/setting the state.

> 
> (Similarly in the QEMU emulated-GICv3 case you could also
> split the CPU i/f more formally, or not. The kernel's choice
> would have implications for which way QEMU ends up going,
> I think.)
> 

Thanks,
-Christoffer

RE: [PATCH 3/3] KVM: arm64: Implement accessors for vGIC CPU interface registers

[Pavel Fedin]

 Hello!

> Have you thought about proper locking/serializing of access to the GIC
> state in these accessor functions?  

 I am in the process of rewriting the whole thing, and i came to this point.
 What kind of locking  would you expect ? It's a CPU interface, it does not affect state of any
other vCPUs. And, since i am getting/setting its registers, i assume that the vCPU is not running.
Well, i added the check. What next?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH 3/3] KVM: arm64: Implement accessors for vGIC CPU interface registers

[Christoffer Dall]

On Tue, Sep 01, 2015 at 04:09:18PM +0300, Pavel Fedin wrote:
>  Hello!
> 
> > Have you thought about proper locking/serializing of access to the GIC
> > state in these accessor functions?  
> 
>  I am in the process of rewriting the whole thing, and i came to this point.
>  What kind of locking  would you expect ? It's a CPU interface, it does not affect state of any
> other vCPUs. And, since i am getting/setting its registers, i assume that the vCPU is not running.
> Well, i added the check. What next?
> 
I think we make some assumptions throughout the vgic code that only the
vcpu itself touches the state of the registers.  Maybe there is no need
for additional locking, but I'd sleep better at night if I knew that
whoever implemented save/restore logic had thought about concurrency.

-Christoffer

Re: [PATCH 0/3] KVM: arm64: Implement API for vGICv3 live migration

[Christoffer Dall]

On Fri, Aug 28, 2015 at 03:56:09PM +0300, Pavel Fedin wrote:
> This patchset adds necessary userspace API in order to support vGICv3 live
> migration. This includes accessing GIC distributor and redistributor memory
> regions using device attribute ioctls, and system registers of
> CPU interface using register get/set ioctls.

This obviously lacks a clear description of the API in
Documentation/virtual/kvm/devices/arm-vgic.txt

> 
> Pavel Fedin (3):
>   KVM: arm64: Implement vGICv3 distributor and redistributor access from
>     userspace
>   KVM: arm64: Allow to use accessors in KVM_SET_ONE_REG and
>     KVM_GET_ONE_REG
>   KVM: arm64: Implement accessors for vGIC CPU interface registers
> 
>  arch/arm64/include/uapi/asm/kvm.h  |   1 +
>  arch/arm64/kvm/sys_regs.c          | 223 ++++++++++++++++++++++++++++++++++++-
>  include/linux/irqchip/arm-gic-v3.h |  18 ++-
>  virt/kvm/arm/vgic-v3-emul.c        | 186 ++++++++++++++++++++++++++++---
>  4 files changed, 405 insertions(+), 23 deletions(-)
> 
> -- 
> 2.4.4
>