[RFC PATCH v2 0/4] arm/arm64: vgic-new: Implement API for vGICv3 live migration

[RFC PATCH v2 0/4] arm/arm64: vgic-new: Implement API for vGICv3 live migration

[vijay.kilari]

From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>

This patchset adds API for saving and restoring
of VGICv3 registers to support live migration with new vgic feature.
This API definition is as per version of VGICv3 specification
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html

To test live migration with QEMU, use below patch series
https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01444.html

The patch 3 & 4 are picked from the Pavel's previous implementation.
http://www.spinics.net/lists/kvm/msg122040.html

v1 => v2:
 - The init sequence change patch is no more required.
   Fixed in patch 2 by using static vgic_io_dev regions structure instead
   of using dynamic allocation pointer.
 - Updated commit message of patch 4.
 - Dropped usage of union to manage 32-bit and 64-bit access in patch 1.
   Used local variable for 32-bit access.
 - Updated macro __ARM64_SYS_REG and ARM64_SYS_REG in 
   arch/arm64/include/uapi/asm/kvm.h as per qemu requirements.

Vijaya Kumar K (4):
  arm/arm64: vgic-new: Introduce 64-bit reg access support
  arm/arm64: vgic-new: Add distributor and redistributor access
  arm/arm64: vgic-new: Introduce find_reg_by_id()
  arm/arm64: vgic-new: Implement VGICv3 CPU interface access

 arch/arm64/include/uapi/asm/kvm.h   |  18 ++-
 arch/arm64/kvm/Makefile             |   1 +
 arch/arm64/kvm/sys_regs.c           |  22 ++--
 arch/arm64/kvm/sys_regs.h           |   4 +
 include/linux/irqchip/arm-gic-v3.h  |   4 +
 virt/kvm/arm/vgic/vgic-kvm-device.c | 138 +++++++++++++++++++---
 virt/kvm/arm/vgic/vgic-mmio-v2.c    |   4 +-
 virt/kvm/arm/vgic/vgic-mmio-v3.c    | 119 +++++++++++++++++++
 virt/kvm/arm/vgic/vgic-mmio.c       |   2 +-
 virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 225 ++++++++++++++++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic.h            |  14 +++
 11 files changed, 525 insertions(+), 26 deletions(-)
 create mode 100644 virt/kvm/arm/vgic/vgic-sys-reg-v3.c

-- 
1.9.1

[RFC PATCH v2 0/4] arm/arm64: vgic-new: Implement API for vGICv3 live migration

[vijay.kilari at gmail.com]

From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>

This patchset adds API for saving and restoring
of VGICv3 registers to support live migration with new vgic feature.
This API definition is as per version of VGICv3 specification
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html

To test live migration with QEMU, use below patch series
https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01444.html

The patch 3 & 4 are picked from the Pavel's previous implementation.
http://www.spinics.net/lists/kvm/msg122040.html

v1 => v2:
 - The init sequence change patch is no more required.
   Fixed in patch 2 by using static vgic_io_dev regions structure instead
   of using dynamic allocation pointer.
 - Updated commit message of patch 4.
 - Dropped usage of union to manage 32-bit and 64-bit access in patch 1.
   Used local variable for 32-bit access.
 - Updated macro __ARM64_SYS_REG and ARM64_SYS_REG in 
   arch/arm64/include/uapi/asm/kvm.h as per qemu requirements.

Vijaya Kumar K (4):
  arm/arm64: vgic-new: Introduce 64-bit reg access support
  arm/arm64: vgic-new: Add distributor and redistributor access
  arm/arm64: vgic-new: Introduce find_reg_by_id()
  arm/arm64: vgic-new: Implement VGICv3 CPU interface access

 arch/arm64/include/uapi/asm/kvm.h   |  18 ++-
 arch/arm64/kvm/Makefile             |   1 +
 arch/arm64/kvm/sys_regs.c           |  22 ++--
 arch/arm64/kvm/sys_regs.h           |   4 +
 include/linux/irqchip/arm-gic-v3.h  |   4 +
 virt/kvm/arm/vgic/vgic-kvm-device.c | 138 +++++++++++++++++++---
 virt/kvm/arm/vgic/vgic-mmio-v2.c    |   4 +-
 virt/kvm/arm/vgic/vgic-mmio-v3.c    | 119 +++++++++++++++++++
 virt/kvm/arm/vgic/vgic-mmio.c       |   2 +-
 virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 225 ++++++++++++++++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic.h            |  14 +++
 11 files changed, 525 insertions(+), 26 deletions(-)
 create mode 100644 virt/kvm/arm/vgic/vgic-sys-reg-v3.c

-- 
1.9.1

[RFC PATCH v2 1/4] arm/arm64: vgic-new: Introduce 64-bit reg access support

[vijay.kilari]

From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>

vgic_attr_regs_access() handles only 32-bit register
value. Pass u64 as parameter and locally handle 32-bit
reads and writes depending on attribute group.

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
---
 virt/kvm/arm/vgic/vgic-kvm-device.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
index 0130c4b..06de322 100644
--- a/virt/kvm/arm/vgic/vgic-kvm-device.c
+++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
@@ -236,12 +236,13 @@ void kvm_register_vgic_device(unsigned long type)
  */
 static int vgic_attr_regs_access(struct kvm_device *dev,
 				 struct kvm_device_attr *attr,
-				 u32 *reg, bool is_write)
+				 u64 *reg, bool is_write)
 {
 	gpa_t addr;
 	int cpuid, ret, c;
 	struct kvm_vcpu *vcpu, *tmp_vcpu;
 	int vcpu_lock_idx = -1;
+	u32 tmp32;
 
 	cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
 		 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
@@ -272,12 +273,19 @@ static int vgic_attr_regs_access(struct kvm_device *dev,
 		vcpu_lock_idx = c;
 	}
 
+	if (is_write)
+		tmp32 = *reg;
+
 	switch (attr->group) {
 	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
-		ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, reg);
+		ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, &tmp32);
+		if (!is_write)
+			*reg = tmp32;
 		break;
 	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
-		ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, reg);
+		ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, &tmp32);
+		if (!is_write)
+			*reg = tmp32;
 		break;
 	default:
 		ret = -EINVAL;
@@ -309,11 +317,13 @@ static int vgic_v2_set_attr(struct kvm_device *dev,
 	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;
+		u32 tmp32;
+		u64 reg;
 
-		if (get_user(reg, uaddr))
+		if (get_user(tmp32, uaddr))
 			return -EFAULT;
 
+		reg = tmp32;
 		return vgic_attr_regs_access(dev, attr, &reg, true);
 	}
 	}
@@ -334,12 +344,14 @@ static int vgic_v2_get_attr(struct kvm_device *dev,
 	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;
+		u32 tmp32;
+		u64 reg;
 
 		ret = vgic_attr_regs_access(dev, attr, &reg, false);
 		if (ret)
 			return ret;
-		return put_user(reg, uaddr);
+		tmp32 = reg;
+		return put_user(tmp32, uaddr);
 	}
 	}
 
-- 
1.9.1

[RFC PATCH v2 1/4] arm/arm64: vgic-new: Introduce 64-bit reg access support

[vijay.kilari at gmail.com]

From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>

vgic_attr_regs_access() handles only 32-bit register
value. Pass u64 as parameter and locally handle 32-bit
reads and writes depending on attribute group.

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
---
 virt/kvm/arm/vgic/vgic-kvm-device.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
index 0130c4b..06de322 100644
--- a/virt/kvm/arm/vgic/vgic-kvm-device.c
+++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
@@ -236,12 +236,13 @@ void kvm_register_vgic_device(unsigned long type)
  */
 static int vgic_attr_regs_access(struct kvm_device *dev,
 				 struct kvm_device_attr *attr,
-				 u32 *reg, bool is_write)
+				 u64 *reg, bool is_write)
 {
 	gpa_t addr;
 	int cpuid, ret, c;
 	struct kvm_vcpu *vcpu, *tmp_vcpu;
 	int vcpu_lock_idx = -1;
+	u32 tmp32;
 
 	cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
 		 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
@@ -272,12 +273,19 @@ static int vgic_attr_regs_access(struct kvm_device *dev,
 		vcpu_lock_idx = c;
 	}
 
+	if (is_write)
+		tmp32 = *reg;
+
 	switch (attr->group) {
 	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
-		ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, reg);
+		ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, &tmp32);
+		if (!is_write)
+			*reg = tmp32;
 		break;
 	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
-		ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, reg);
+		ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, &tmp32);
+		if (!is_write)
+			*reg = tmp32;
 		break;
 	default:
 		ret = -EINVAL;
@@ -309,11 +317,13 @@ static int vgic_v2_set_attr(struct kvm_device *dev,
 	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;
+		u32 tmp32;
+		u64 reg;
 
-		if (get_user(reg, uaddr))
+		if (get_user(tmp32, uaddr))
 			return -EFAULT;
 
+		reg = tmp32;
 		return vgic_attr_regs_access(dev, attr, &reg, true);
 	}
 	}
@@ -334,12 +344,14 @@ static int vgic_v2_get_attr(struct kvm_device *dev,
 	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;
+		u32 tmp32;
+		u64 reg;
 
 		ret = vgic_attr_regs_access(dev, attr, &reg, false);
 		if (ret)
 			return ret;
-		return put_user(reg, uaddr);
+		tmp32 = reg;
+		return put_user(tmp32, uaddr);
 	}
 	}
 
-- 
1.9.1

[RFC PATCH v2 2/4] arm/arm64: vgic-new: Add distributor and redistributor access

[vijay.kilari]

From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>

VGICv3 Distributor and Redistributor registers are accessed using
KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_DIST_REGS
with KVM_SET_DEVICE_ATTR and KVM_GET_DEVICE_ATTR ioctls.
These registers are accessed as 32-bit and cpu mpidr
value passed along with register offset is used to identify the
cpu for redistributor registers access.

The version of VGIC v3 specification is define here
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
---
 arch/arm64/include/uapi/asm/kvm.h   |   3 +
 virt/kvm/arm/vgic/vgic-kvm-device.c |  81 ++++++++++++++++++++++++--
 virt/kvm/arm/vgic/vgic-mmio-v3.c    | 113 ++++++++++++++++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic-mmio.c       |   2 +-
 virt/kvm/arm/vgic/vgic.h            |   8 +++
 5 files changed, 200 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index f209ea1..a6b996e 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -199,10 +199,13 @@ struct kvm_arch_memory_slot {
 #define KVM_DEV_ARM_VGIC_GRP_CPU_REGS	2
 #define   KVM_DEV_ARM_VGIC_CPUID_SHIFT	32
 #define   KVM_DEV_ARM_VGIC_CPUID_MASK	(0xffULL << KVM_DEV_ARM_VGIC_CPUID_SHIFT)
+#define   KVM_DEV_ARM_VGIC_V3_CPUID_MASK \
+				(0xffffffffULL << KVM_DEV_ARM_VGIC_CPUID_SHIFT)
 #define   KVM_DEV_ARM_VGIC_OFFSET_SHIFT	0
 #define   KVM_DEV_ARM_VGIC_OFFSET_MASK	(0xffffffffULL << KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
 #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS	3
 #define KVM_DEV_ARM_VGIC_GRP_CTRL	4
+#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
 #define   KVM_DEV_ARM_VGIC_CTRL_INIT	0
 
 /* Device Control API on vcpu fd */
diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
index 06de322..986f8e1 100644
--- a/virt/kvm/arm/vgic/vgic-kvm-device.c
+++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
@@ -243,10 +243,19 @@ static int vgic_attr_regs_access(struct kvm_device *dev,
 	struct kvm_vcpu *vcpu, *tmp_vcpu;
 	int vcpu_lock_idx = -1;
 	u32 tmp32;
+	struct vgic_dist *vgic = &dev->kvm->arch.vgic;
 
-	cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
-		 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
-	vcpu = kvm_get_vcpu(dev->kvm, cpuid);
+	if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2) {
+		cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
+			 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
+		vcpu = kvm_get_vcpu(dev->kvm, cpuid);
+	}
+	else
+	{
+		cpuid = (attr->attr & KVM_DEV_ARM_VGIC_V3_CPUID_MASK) >>
+			 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
+		vcpu = kvm_mpidr_to_vcpu(dev->kvm, cpuid);
+	}
 	addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
 
 	mutex_lock(&dev->kvm->lock);
@@ -283,10 +292,25 @@ static int vgic_attr_regs_access(struct kvm_device *dev,
 			*reg = tmp32;
 		break;
 	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
-		ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, &tmp32);
+		if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
+			ret = vgic_v2_dist_uaccess(vcpu, is_write, addr,
+						   &tmp32);
+		else
+			ret = vgic_v3_dist_uaccess(vcpu, is_write, addr,
+						   &tmp32);
 		if (!is_write)
 			*reg = tmp32;
 		break;
+	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
+		if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
+			ret = vgic_v3_redist_uaccess(vcpu, is_write, addr,
+						     &tmp32);
+			if (!is_write)
+				*reg = tmp32;
+		} else {
+			ret = -EINVAL;
+		}
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -399,13 +423,55 @@ struct kvm_device_ops kvm_arm_vgic_v2_ops = {
 static int vgic_v3_set_attr(struct kvm_device *dev,
 			    struct kvm_device_attr *attr)
 {
-	return vgic_set_common_attr(dev, attr);
+	int ret;
+
+	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_REDIST_REGS: {
+		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
+		u32 tmp32;
+		u64 reg;
+
+		if (get_user(tmp32, uaddr))
+			return -EFAULT;
+
+		reg = tmp32;
+		return vgic_attr_regs_access(dev, attr, &reg, true);
+	}
+	}
+	return -ENXIO;
 }
 
 static int vgic_v3_get_attr(struct kvm_device *dev,
 			    struct kvm_device_attr *attr)
 {
-	return vgic_get_common_attr(dev, attr);
+	int ret;
+
+	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_REDIST_REGS: {
+		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
+		u64 reg;
+		u32 tmp32;
+
+		ret = vgic_attr_regs_access(dev, attr, &reg, false);
+		if (ret)
+			return ret;
+		tmp32 = reg;
+		ret = put_user(tmp32, uaddr);
+		return ret;
+	}
+	}
+
+	return -ENXIO;
 }
 
 static int vgic_v3_has_attr(struct kvm_device *dev,
@@ -419,6 +485,9 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
 			return 0;
 		}
 		break;
+	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
+	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
+		return vgic_v3_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-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index a0c515a..36b4882 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -18,6 +18,8 @@
 #include <kvm/arm_vgic.h>
 
 #include <asm/kvm_emulate.h>
+#include <asm/kvm_arm.h>
+#include <asm/kvm_mmu.h>
 
 #include "vgic.h"
 #include "vgic-mmio.h"
@@ -226,6 +228,9 @@ static const struct vgic_register_region vgic_v3_rdbase_registers[] = {
 	REGISTER_DESC_WITH_LENGTH(GICR_TYPER,
 		vgic_mmio_read_v3r_typer, vgic_mmio_write_wi, 8,
 		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
+	REGISTER_DESC_WITH_LENGTH(GICR_WAKER,
+		vgic_mmio_read_raz, vgic_mmio_write_wi, 8,
+		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_LENGTH(GICR_PROPBASER,
 		vgic_mmio_read_raz, vgic_mmio_write_wi, 8,
 		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
@@ -348,6 +353,52 @@ int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t redist_base_address)
 	return ret;
 }
 
+int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
+{
+	struct kvm_vcpu *vcpu;
+	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, cpuid;
+
+	addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
+	cpuid = (attr->attr & KVM_DEV_ARM_VGIC_V3_CPUID_MASK) >>
+		 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
+	vcpu = kvm_mpidr_to_vcpu(dev->kvm, cpuid);
+
+	switch (attr->group) {
+	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
+		regions = vgic_v3_dist_registers;
+		nr_regions = ARRAY_SIZE(vgic_v3_dist_registers);
+		break;
+	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:{
+		struct vgic_io_device *devices;
+		struct vgic_io_device *rd_dev;
+
+		devices = dev->kvm->arch.vgic.redist_iodevs;
+		rd_dev = &devices[vcpu->vcpu_id * 2];
+
+		regions = rd_dev->regions;
+		nr_regions = rd_dev->nr_regions;
+		break;
+	}
+	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;
+}
 /*
  * Compare a given affinity (level 1-3 and a level 0 mask, from the SGI
  * generation register ICC_SGI1R_EL1) with a given VCPU.
@@ -453,3 +504,65 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
 		vgic_queue_irq_unlock(vcpu->kvm, irq);
 	}
 }
+
+/*
+ * When userland tries to access the VGIC register handlers, we need to
+ * create a usable struct vgic_io_device to be passed to the handlers and we
+ * have to set up a buffer similar to what would have happened if a guest MMIO
+ * access occurred, including doing endian conversions on BE systems.
+ */
+static int vgic_v3_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
+			   bool is_write, int offset, u32 *val)
+{
+	unsigned int len = 4;
+	u8 buf[4];
+	int ret;
+
+	if (is_write) {
+		vgic_data_host_to_mmio_bus(buf, len, *val);
+		ret = kvm_io_gic_ops.write(vcpu, &dev->dev,
+					   dev->base_addr + offset, len, buf);
+	} else {
+		ret = kvm_io_gic_ops.read(vcpu, &dev->dev,
+					  dev->base_addr + offset, len, buf);
+		if (!ret)
+			*val = vgic_data_mmio_bus_to_host(buf, len);
+	}
+
+	return ret;
+}
+
+int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
+			 int offset, u32 *val)
+{
+	struct vgic_io_device dev = {
+		.regions = vgic_v3_dist_registers,
+		.nr_regions = ARRAY_SIZE(vgic_v3_dist_registers),
+	};
+
+	return vgic_v3_uaccess(vcpu, &dev, is_write, offset, val);
+}
+
+int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
+			   int offset, u32 *val)
+{
+	struct vgic_io_device *dev;
+	const struct vgic_register_region *region;
+
+	struct vgic_io_device rd_dev = {
+		.regions = vgic_v3_rdbase_registers,
+		.nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers),
+	};
+
+	struct vgic_io_device sgi_dev = {
+		.regions = vgic_v3_sgibase_registers,
+		.nr_regions = ARRAY_SIZE(vgic_v3_sgibase_registers),
+	};
+
+	dev = &sgi_dev;
+	region = vgic_find_mmio_region(dev->regions, dev->nr_regions, offset);
+	if (region == NULL)
+		dev = &rd_dev;
+
+	return vgic_v3_uaccess(vcpu, dev, is_write, offset, val);
+}
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index 9f6fab7..f583959 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -363,7 +363,7 @@ static int match_region(const void *key, const void *elt)
 }
 
 /* Find the proper register handler entry given a certain address offset. */
-static const struct vgic_register_region *
+const struct vgic_register_region *
 vgic_find_mmio_region(const struct vgic_register_region *region, int nr_regions,
 		      unsigned int offset)
 {
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 7b300ca..8637690 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -59,6 +59,9 @@ int vgic_v2_map_resources(struct kvm *kvm);
 int vgic_register_dist_iodev(struct kvm *kvm, gpa_t dist_base_address,
 			     enum vgic_type);
 
+const struct vgic_register_region *
+	vgic_find_mmio_region(const struct vgic_register_region *region,
+			      int nr_regions, unsigned int offset);
 #ifdef CONFIG_KVM_ARM_VGIC_V3
 void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu);
 void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu);
@@ -71,6 +74,11 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu);
 int vgic_v3_probe(const struct gic_kvm_info *info);
 int vgic_v3_map_resources(struct kvm *kvm);
 int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t dist_base_address);
+int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr);
+int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
+			 int offset, u32 *val);
+int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
+			 int offset, u32 *val);
 #else
 static inline void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu)
 {
-- 
1.9.1

[RFC PATCH v2 2/4] arm/arm64: vgic-new: Add distributor and redistributor access

[vijay.kilari at gmail.com]

From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>

VGICv3 Distributor and Redistributor registers are accessed using
KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_DIST_REGS
with KVM_SET_DEVICE_ATTR and KVM_GET_DEVICE_ATTR ioctls.
These registers are accessed as 32-bit and cpu mpidr
value passed along with register offset is used to identify the
cpu for redistributor registers access.

The version of VGIC v3 specification is define here
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
---
 arch/arm64/include/uapi/asm/kvm.h   |   3 +
 virt/kvm/arm/vgic/vgic-kvm-device.c |  81 ++++++++++++++++++++++++--
 virt/kvm/arm/vgic/vgic-mmio-v3.c    | 113 ++++++++++++++++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic-mmio.c       |   2 +-
 virt/kvm/arm/vgic/vgic.h            |   8 +++
 5 files changed, 200 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index f209ea1..a6b996e 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -199,10 +199,13 @@ struct kvm_arch_memory_slot {
 #define KVM_DEV_ARM_VGIC_GRP_CPU_REGS	2
 #define   KVM_DEV_ARM_VGIC_CPUID_SHIFT	32
 #define   KVM_DEV_ARM_VGIC_CPUID_MASK	(0xffULL << KVM_DEV_ARM_VGIC_CPUID_SHIFT)
+#define   KVM_DEV_ARM_VGIC_V3_CPUID_MASK \
+				(0xffffffffULL << KVM_DEV_ARM_VGIC_CPUID_SHIFT)
 #define   KVM_DEV_ARM_VGIC_OFFSET_SHIFT	0
 #define   KVM_DEV_ARM_VGIC_OFFSET_MASK	(0xffffffffULL << KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
 #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS	3
 #define KVM_DEV_ARM_VGIC_GRP_CTRL	4
+#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
 #define   KVM_DEV_ARM_VGIC_CTRL_INIT	0
 
 /* Device Control API on vcpu fd */
diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
index 06de322..986f8e1 100644
--- a/virt/kvm/arm/vgic/vgic-kvm-device.c
+++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
@@ -243,10 +243,19 @@ static int vgic_attr_regs_access(struct kvm_device *dev,
 	struct kvm_vcpu *vcpu, *tmp_vcpu;
 	int vcpu_lock_idx = -1;
 	u32 tmp32;
+	struct vgic_dist *vgic = &dev->kvm->arch.vgic;
 
-	cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
-		 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
-	vcpu = kvm_get_vcpu(dev->kvm, cpuid);
+	if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2) {
+		cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
+			 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
+		vcpu = kvm_get_vcpu(dev->kvm, cpuid);
+	}
+	else
+	{
+		cpuid = (attr->attr & KVM_DEV_ARM_VGIC_V3_CPUID_MASK) >>
+			 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
+		vcpu = kvm_mpidr_to_vcpu(dev->kvm, cpuid);
+	}
 	addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
 
 	mutex_lock(&dev->kvm->lock);
@@ -283,10 +292,25 @@ static int vgic_attr_regs_access(struct kvm_device *dev,
 			*reg = tmp32;
 		break;
 	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
-		ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, &tmp32);
+		if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
+			ret = vgic_v2_dist_uaccess(vcpu, is_write, addr,
+						   &tmp32);
+		else
+			ret = vgic_v3_dist_uaccess(vcpu, is_write, addr,
+						   &tmp32);
 		if (!is_write)
 			*reg = tmp32;
 		break;
+	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
+		if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
+			ret = vgic_v3_redist_uaccess(vcpu, is_write, addr,
+						     &tmp32);
+			if (!is_write)
+				*reg = tmp32;
+		} else {
+			ret = -EINVAL;
+		}
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -399,13 +423,55 @@ struct kvm_device_ops kvm_arm_vgic_v2_ops = {
 static int vgic_v3_set_attr(struct kvm_device *dev,
 			    struct kvm_device_attr *attr)
 {
-	return vgic_set_common_attr(dev, attr);
+	int ret;
+
+	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_REDIST_REGS: {
+		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
+		u32 tmp32;
+		u64 reg;
+
+		if (get_user(tmp32, uaddr))
+			return -EFAULT;
+
+		reg = tmp32;
+		return vgic_attr_regs_access(dev, attr, &reg, true);
+	}
+	}
+	return -ENXIO;
 }
 
 static int vgic_v3_get_attr(struct kvm_device *dev,
 			    struct kvm_device_attr *attr)
 {
-	return vgic_get_common_attr(dev, attr);
+	int ret;
+
+	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_REDIST_REGS: {
+		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
+		u64 reg;
+		u32 tmp32;
+
+		ret = vgic_attr_regs_access(dev, attr, &reg, false);
+		if (ret)
+			return ret;
+		tmp32 = reg;
+		ret = put_user(tmp32, uaddr);
+		return ret;
+	}
+	}
+
+	return -ENXIO;
 }
 
 static int vgic_v3_has_attr(struct kvm_device *dev,
@@ -419,6 +485,9 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
 			return 0;
 		}
 		break;
+	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
+	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
+		return vgic_v3_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-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index a0c515a..36b4882 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -18,6 +18,8 @@
 #include <kvm/arm_vgic.h>
 
 #include <asm/kvm_emulate.h>
+#include <asm/kvm_arm.h>
+#include <asm/kvm_mmu.h>
 
 #include "vgic.h"
 #include "vgic-mmio.h"
@@ -226,6 +228,9 @@ static const struct vgic_register_region vgic_v3_rdbase_registers[] = {
 	REGISTER_DESC_WITH_LENGTH(GICR_TYPER,
 		vgic_mmio_read_v3r_typer, vgic_mmio_write_wi, 8,
 		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
+	REGISTER_DESC_WITH_LENGTH(GICR_WAKER,
+		vgic_mmio_read_raz, vgic_mmio_write_wi, 8,
+		VGIC_ACCESS_32bit),
 	REGISTER_DESC_WITH_LENGTH(GICR_PROPBASER,
 		vgic_mmio_read_raz, vgic_mmio_write_wi, 8,
 		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
@@ -348,6 +353,52 @@ int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t redist_base_address)
 	return ret;
 }
 
+int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
+{
+	struct kvm_vcpu *vcpu;
+	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, cpuid;
+
+	addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
+	cpuid = (attr->attr & KVM_DEV_ARM_VGIC_V3_CPUID_MASK) >>
+		 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
+	vcpu = kvm_mpidr_to_vcpu(dev->kvm, cpuid);
+
+	switch (attr->group) {
+	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
+		regions = vgic_v3_dist_registers;
+		nr_regions = ARRAY_SIZE(vgic_v3_dist_registers);
+		break;
+	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:{
+		struct vgic_io_device *devices;
+		struct vgic_io_device *rd_dev;
+
+		devices = dev->kvm->arch.vgic.redist_iodevs;
+		rd_dev = &devices[vcpu->vcpu_id * 2];
+
+		regions = rd_dev->regions;
+		nr_regions = rd_dev->nr_regions;
+		break;
+	}
+	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;
+}
 /*
  * Compare a given affinity (level 1-3 and a level 0 mask, from the SGI
  * generation register ICC_SGI1R_EL1) with a given VCPU.
@@ -453,3 +504,65 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
 		vgic_queue_irq_unlock(vcpu->kvm, irq);
 	}
 }
+
+/*
+ * When userland tries to access the VGIC register handlers, we need to
+ * create a usable struct vgic_io_device to be passed to the handlers and we
+ * have to set up a buffer similar to what would have happened if a guest MMIO
+ * access occurred, including doing endian conversions on BE systems.
+ */
+static int vgic_v3_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
+			   bool is_write, int offset, u32 *val)
+{
+	unsigned int len = 4;
+	u8 buf[4];
+	int ret;
+
+	if (is_write) {
+		vgic_data_host_to_mmio_bus(buf, len, *val);
+		ret = kvm_io_gic_ops.write(vcpu, &dev->dev,
+					   dev->base_addr + offset, len, buf);
+	} else {
+		ret = kvm_io_gic_ops.read(vcpu, &dev->dev,
+					  dev->base_addr + offset, len, buf);
+		if (!ret)
+			*val = vgic_data_mmio_bus_to_host(buf, len);
+	}
+
+	return ret;
+}
+
+int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
+			 int offset, u32 *val)
+{
+	struct vgic_io_device dev = {
+		.regions = vgic_v3_dist_registers,
+		.nr_regions = ARRAY_SIZE(vgic_v3_dist_registers),
+	};
+
+	return vgic_v3_uaccess(vcpu, &dev, is_write, offset, val);
+}
+
+int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
+			   int offset, u32 *val)
+{
+	struct vgic_io_device *dev;
+	const struct vgic_register_region *region;
+
+	struct vgic_io_device rd_dev = {
+		.regions = vgic_v3_rdbase_registers,
+		.nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers),
+	};
+
+	struct vgic_io_device sgi_dev = {
+		.regions = vgic_v3_sgibase_registers,
+		.nr_regions = ARRAY_SIZE(vgic_v3_sgibase_registers),
+	};
+
+	dev = &sgi_dev;
+	region = vgic_find_mmio_region(dev->regions, dev->nr_regions, offset);
+	if (region == NULL)
+		dev = &rd_dev;
+
+	return vgic_v3_uaccess(vcpu, dev, is_write, offset, val);
+}
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index 9f6fab7..f583959 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -363,7 +363,7 @@ static int match_region(const void *key, const void *elt)
 }
 
 /* Find the proper register handler entry given a certain address offset. */
-static const struct vgic_register_region *
+const struct vgic_register_region *
 vgic_find_mmio_region(const struct vgic_register_region *region, int nr_regions,
 		      unsigned int offset)
 {
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 7b300ca..8637690 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -59,6 +59,9 @@ int vgic_v2_map_resources(struct kvm *kvm);
 int vgic_register_dist_iodev(struct kvm *kvm, gpa_t dist_base_address,
 			     enum vgic_type);
 
+const struct vgic_register_region *
+	vgic_find_mmio_region(const struct vgic_register_region *region,
+			      int nr_regions, unsigned int offset);
 #ifdef CONFIG_KVM_ARM_VGIC_V3
 void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu);
 void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu);
@@ -71,6 +74,11 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu);
 int vgic_v3_probe(const struct gic_kvm_info *info);
 int vgic_v3_map_resources(struct kvm *kvm);
 int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t dist_base_address);
+int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr);
+int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
+			 int offset, u32 *val);
+int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
+			 int offset, u32 *val);
 #else
 static inline void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu)
 {
-- 
1.9.1

[RFC PATCH v2 3/4] arm/arm64: vgic-new: Introduce find_reg_by_id()

[vijay.kilari]

From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>

In order to implement vGICv3 CPU interface access, we will need to perform
table lookup of system registers. We would need both index_to_params() and
find_reg() exported for that purpose, but instead we export a single
function which combines them both.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
---
 arch/arm64/kvm/sys_regs.c | 22 +++++++++++++++-------
 arch/arm64/kvm/sys_regs.h |  4 ++++
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index a57d650..16c33ca 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1802,6 +1802,17 @@ static bool index_to_params(u64 id, struct sys_reg_params *params)
 	}
 }
 
+const struct sys_reg_desc *find_reg_by_id(u64 id,
+					  struct sys_reg_params *params,
+					  const struct sys_reg_desc table[],
+					  unsigned int num)
+{
+	if (!index_to_params(id, params))
+		return NULL;
+
+	return find_reg(params, table, num);
+}
+
 /* Decode an index value, and find the sys_reg_desc entry. */
 static const struct sys_reg_desc *index_to_sys_reg_desc(struct kvm_vcpu *vcpu,
 						    u64 id)
@@ -1929,10 +1940,8 @@ static int get_invariant_sys_reg(u64 id, void __user *uaddr)
 	struct sys_reg_params params;
 	const struct sys_reg_desc *r;
 
-	if (!index_to_params(id, &params))
-		return -ENOENT;
-
-	r = find_reg(&params, invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs));
+	r = find_reg_by_id(id, &params, invariant_sys_regs,
+			   ARRAY_SIZE(invariant_sys_regs));
 	if (!r)
 		return -ENOENT;
 
@@ -1946,9 +1955,8 @@ static int set_invariant_sys_reg(u64 id, void __user *uaddr)
 	int err;
 	u64 val = 0; /* Make sure high bits are 0 for 32-bit regs */
 
-	if (!index_to_params(id, &params))
-		return -ENOENT;
-	r = find_reg(&params, invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs));
+	r = find_reg_by_id(id, &params, invariant_sys_regs,
+			   ARRAY_SIZE(invariant_sys_regs));
 	if (!r)
 		return -ENOENT;
 
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index dbbb01c..9c6ffd0 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -136,6 +136,10 @@ static inline int cmp_sys_reg(const struct sys_reg_desc *i1,
 	return i1->Op2 - i2->Op2;
 }
 
+const struct sys_reg_desc *find_reg_by_id(u64 id,
+					  struct sys_reg_params *params,
+					  const struct sys_reg_desc table[],
+					  unsigned int num);
 
 #define Op0(_x) 	.Op0 = _x
 #define Op1(_x) 	.Op1 = _x
-- 
1.9.1

[RFC PATCH v2 3/4] arm/arm64: vgic-new: Introduce find_reg_by_id()

[vijay.kilari at gmail.com]

From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>

In order to implement vGICv3 CPU interface access, we will need to perform
table lookup of system registers. We would need both index_to_params() and
find_reg() exported for that purpose, but instead we export a single
function which combines them both.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
---
 arch/arm64/kvm/sys_regs.c | 22 +++++++++++++++-------
 arch/arm64/kvm/sys_regs.h |  4 ++++
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index a57d650..16c33ca 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1802,6 +1802,17 @@ static bool index_to_params(u64 id, struct sys_reg_params *params)
 	}
 }
 
+const struct sys_reg_desc *find_reg_by_id(u64 id,
+					  struct sys_reg_params *params,
+					  const struct sys_reg_desc table[],
+					  unsigned int num)
+{
+	if (!index_to_params(id, params))
+		return NULL;
+
+	return find_reg(params, table, num);
+}
+
 /* Decode an index value, and find the sys_reg_desc entry. */
 static const struct sys_reg_desc *index_to_sys_reg_desc(struct kvm_vcpu *vcpu,
 						    u64 id)
@@ -1929,10 +1940,8 @@ static int get_invariant_sys_reg(u64 id, void __user *uaddr)
 	struct sys_reg_params params;
 	const struct sys_reg_desc *r;
 
-	if (!index_to_params(id, &params))
-		return -ENOENT;
-
-	r = find_reg(&params, invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs));
+	r = find_reg_by_id(id, &params, invariant_sys_regs,
+			   ARRAY_SIZE(invariant_sys_regs));
 	if (!r)
 		return -ENOENT;
 
@@ -1946,9 +1955,8 @@ static int set_invariant_sys_reg(u64 id, void __user *uaddr)
 	int err;
 	u64 val = 0; /* Make sure high bits are 0 for 32-bit regs */
 
-	if (!index_to_params(id, &params))
-		return -ENOENT;
-	r = find_reg(&params, invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs));
+	r = find_reg_by_id(id, &params, invariant_sys_regs,
+			   ARRAY_SIZE(invariant_sys_regs));
 	if (!r)
 		return -ENOENT;
 
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index dbbb01c..9c6ffd0 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -136,6 +136,10 @@ static inline int cmp_sys_reg(const struct sys_reg_desc *i1,
 	return i1->Op2 - i2->Op2;
 }
 
+const struct sys_reg_desc *find_reg_by_id(u64 id,
+					  struct sys_reg_params *params,
+					  const struct sys_reg_desc table[],
+					  unsigned int num);
 
 #define Op0(_x) 	.Op0 = _x
 #define Op1(_x) 	.Op1 = _x
-- 
1.9.1

[RFC PATCH v2 4/4] arm/arm64: vgic-new: Implement VGICv3 CPU interface access

[vijay.kilari]

From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>

VGICv3 CPU interface registers are accessed using
KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
as 64-bit. The cpu MPIDR value is passed along with register id.
is used to identify the cpu for registers access.

The version of VGIC v3 specification is define here
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
---
 arch/arm64/include/uapi/asm/kvm.h   |  15 ++-
 arch/arm64/kvm/Makefile             |   1 +
 include/linux/irqchip/arm-gic-v3.h  |   4 +
 virt/kvm/arm/vgic/vgic-kvm-device.c |  31 +++++
 virt/kvm/arm/vgic/vgic-mmio-v2.c    |   4 +-
 virt/kvm/arm/vgic/vgic-mmio-v3.c    |   6 +
 virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 225 ++++++++++++++++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic.h            |   6 +
 8 files changed, 287 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index a6b996e..85829fb 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -180,14 +180,14 @@ struct kvm_arch_memory_slot {
 	KVM_REG_ARM64_SYSREG_ ## n ## _MASK)
 
 #define __ARM64_SYS_REG(op0,op1,crn,crm,op2) \
-	(KVM_REG_ARM64 | KVM_REG_ARM64_SYSREG | \
-	ARM64_SYS_REG_SHIFT_MASK(op0, OP0) | \
+	(ARM64_SYS_REG_SHIFT_MASK(op0, OP0) | \
 	ARM64_SYS_REG_SHIFT_MASK(op1, OP1) | \
 	ARM64_SYS_REG_SHIFT_MASK(crn, CRN) | \
 	ARM64_SYS_REG_SHIFT_MASK(crm, CRM) | \
 	ARM64_SYS_REG_SHIFT_MASK(op2, OP2))
 
-#define ARM64_SYS_REG(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U64)
+#define ARM64_SYS_REG(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U64 | \
+			    KVM_REG_ARM64 | KVM_REG_ARM64_SYSREG)
 
 #define KVM_REG_ARM_TIMER_CTL		ARM64_SYS_REG(3, 3, 14, 3, 1)
 #define KVM_REG_ARM_TIMER_CNT		ARM64_SYS_REG(3, 3, 14, 3, 2)
@@ -206,7 +206,16 @@ 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_GRP_REDIST_REGS 5
+#define KVM_DEV_ARM_VGIC_CPU_SYSREGS    6
+
 #define   KVM_DEV_ARM_VGIC_CTRL_INIT	0
+#define   KVM_DEV_ARM_VGIC_SYSREG_MASK (KVM_REG_ARM64_SYSREG_OP0_MASK | \
+					KVM_REG_ARM64_SYSREG_OP1_MASK | \
+					KVM_REG_ARM64_SYSREG_CRN_MASK | \
+					KVM_REG_ARM64_SYSREG_CRM_MASK | \
+					KVM_REG_ARM64_SYSREG_OP2_MASK)
+#define   KVM_DEV_ARM_VGIC_SYSREG(op0, op1, crn, crm, op2) \
+					__ARM64_SYS_REG(op0, op1, crn, crm, op2)
 
 /* Device Control API on vcpu fd */
 #define KVM_ARM_VCPU_PMU_V3_CTRL	0
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index a7a958c..a962525 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -30,6 +30,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio.o
 kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v2.o
 kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v3.o
 kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
+kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-sys-reg-v3.o
 else
 kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic.o
 kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic-v2.o
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index bfbd707..ae84cde 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -288,6 +288,10 @@
 
 #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_BPR1_SHIFT		18
 #define ICH_VMCR_BPR1_MASK		(7 << ICH_VMCR_BPR1_SHIFT)
 #define ICH_VMCR_BPR0_SHIFT		21
diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
index 986f8e1..b7ca929 100644
--- a/virt/kvm/arm/vgic/vgic-kvm-device.c
+++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
@@ -243,6 +243,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev,
 	struct kvm_vcpu *vcpu, *tmp_vcpu;
 	int vcpu_lock_idx = -1;
 	u32 tmp32;
+	u64 regid;
 	struct vgic_dist *vgic = &dev->kvm->arch.vgic;
 
 	if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2) {
@@ -311,6 +312,16 @@ static int vgic_attr_regs_access(struct kvm_device *dev,
 			ret = -EINVAL;
 		}
 		break;
+	case KVM_DEV_ARM_VGIC_CPU_SYSREGS:
+		if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
+			regid = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_MASK) |
+				KVM_REG_SIZE_U64;
+			ret = vgic_v3_cpu_sysregs_uaccess(vcpu, is_write,
+							  regid, reg);
+		} else {
+			ret = -EINVAL;
+		}
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -442,6 +453,15 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
 		reg = tmp32;
 		return vgic_attr_regs_access(dev, attr, &reg, true);
 	}
+	case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
+		u64 __user *uaddr = (u64 __user *)(long)attr->addr;
+		u64 reg;
+
+		if (get_user(reg, uaddr))
+			return -EFAULT;
+
+		return vgic_attr_regs_access(dev, attr, &reg, true);
+	}
 	}
 	return -ENXIO;
 }
@@ -469,6 +489,16 @@ static int vgic_v3_get_attr(struct kvm_device *dev,
 		ret = put_user(tmp32, uaddr);
 		return ret;
 	}
+	case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
+		u64 __user *uaddr = (u64 __user *)(long)attr->addr;
+		u64 reg;
+
+		ret = vgic_attr_regs_access(dev, attr, &reg, false);
+		if (ret)
+			return ret;
+		ret = put_user(reg, uaddr);
+		return ret;
+	}
 	}
 
 	return -ENXIO;
@@ -487,6 +517,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_REDIST_REGS:
+	case KVM_DEV_ARM_VGIC_CPU_SYSREGS:
 		return vgic_v3_has_attr_regs(dev, attr);
 	case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
 		return 0;
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index a213936..97d94b2 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -204,7 +204,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu,
 	}
 }
 
-static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
+void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
 {
 	if (kvm_vgic_global_state.type == VGIC_V2)
 		vgic_v2_set_vmcr(vcpu, vmcr);
@@ -212,7 +212,7 @@ static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
 		vgic_v3_set_vmcr(vcpu, vmcr);
 }
 
-static void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
+void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
 {
 	if (kvm_vgic_global_state.type == VGIC_V2)
 		vgic_v2_get_vmcr(vcpu, vmcr);
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 36b4882..0c6994a 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -23,6 +23,7 @@
 
 #include "vgic.h"
 #include "vgic-mmio.h"
+#include "sys_regs.h"
 
 /* extract @num bytes at @offset bytes offset in data */
 static unsigned long extract_bytes(unsigned long data, unsigned int offset,
@@ -382,6 +383,11 @@ int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
 		nr_regions = rd_dev->nr_regions;
 		break;
 	}
+	case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
+		u64 reg;
+
+		return vgic_v3_has_cpu_sysregs_attr(vcpu, 0, cpuid, &reg);
+	}
 	default:
 		return -ENXIO;
 	}
diff --git a/virt/kvm/arm/vgic/vgic-sys-reg-v3.c b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
new file mode 100644
index 0000000..d877cd8
--- /dev/null
+++ b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
@@ -0,0 +1,225 @@
+#include <linux/irqchip/arm-gic-v3.h>
+#include <linux/kvm.h>
+#include <linux/kvm_host.h>
+#include <kvm/iodev.h>
+#include <kvm/arm_vgic.h>
+#include <asm/kvm_emulate.h>
+#include <asm/kvm_arm.h>
+#include <asm/kvm_mmu.h>
+
+#include "vgic.h"
+#include "vgic-mmio.h"
+#include "sys_regs.h"
+
+static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+			    const struct sys_reg_desc *r)
+{
+	struct vgic_vmcr vmcr;
+
+	vgic_get_vmcr(vcpu, &vmcr);
+	if (p->is_write) {
+		vmcr.ctlr = (u32)p->regval;
+		vgic_set_vmcr(vcpu, &vmcr);
+	} else {
+		p->regval = vmcr.ctlr;
+	}
+
+	return true;
+}
+
+static bool access_gic_pmr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+			   const struct sys_reg_desc *r)
+{
+	struct vgic_vmcr vmcr;
+
+	vgic_get_vmcr(vcpu, &vmcr);
+	if (p->is_write) {
+		vmcr.pmr = (u32)p->regval;
+		vgic_set_vmcr(vcpu, &vmcr);
+	} else {
+		p->regval = vmcr.pmr;
+	}
+
+	return true;
+}
+
+static bool access_gic_bpr0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+			    const struct sys_reg_desc *r)
+{
+	struct vgic_vmcr vmcr;
+
+	vgic_get_vmcr(vcpu, &vmcr);
+	if (p->is_write) {
+		vmcr.bpr = (u32)p->regval;
+		vgic_set_vmcr(vcpu, &vmcr);
+	} else {
+		p->regval = vmcr.bpr;
+	}
+
+	return true;
+}
+
+static bool access_gic_bpr1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+			    const struct sys_reg_desc *r)
+{
+	struct vgic_vmcr vmcr;
+
+	vgic_get_vmcr(vcpu, &vmcr);
+	if (p->is_write) {
+		vmcr.abpr = (u32)p->regval;
+		vgic_set_vmcr(vcpu, &vmcr);
+	} else {
+		p->regval = vmcr.abpr;
+	}
+
+	return true;
+}
+
+static bool access_gic_grpen0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+			      const struct sys_reg_desc *r)
+{
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (p->is_write) {
+		vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG0;
+		vgicv3->vgic_vmcr |= (p->regval << ICH_VMCR_ENG0_SHIFT) &
+				      ICH_VMCR_ENG0;
+	} else {
+		p->regval = (vgicv3->vgic_vmcr & ICH_VMCR_ENG0) >>
+			     ICH_VMCR_ENG0_SHIFT;
+	}
+
+	return true;
+}
+
+static bool access_gic_grpen1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+			      const struct sys_reg_desc *r)
+{
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (p->is_write) {
+		vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG1;
+		vgicv3->vgic_vmcr |= (p->regval << ICH_VMCR_ENG1_SHIFT) &
+				      ICH_VMCR_ENG1;
+	} else {
+		p->regval = (vgicv3->vgic_vmcr & ICH_VMCR_ENG1) >>
+			     ICH_VMCR_ENG1_SHIFT;
+	}
+
+	return true;
+}
+
+static bool access_gic_ap0r(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+			    const struct sys_reg_desc *r)
+{
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+	u8 idx = r->Op2 & 3;
+
+	if (p->is_write)
+		vgicv3->vgic_ap0r[idx] = p->regval;
+	else
+		p->regval = vgicv3->vgic_ap0r[idx];
+
+	return true;
+}
+
+static bool access_gic_ap1r(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+			    const struct sys_reg_desc *r)
+{
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+	u8 idx = r->Op2 & 3;
+
+	if (p->is_write)
+		vgicv3->vgic_ap1r[idx] = p->regval;
+	else
+		p->regval = vgicv3->vgic_ap1r[idx];
+
+	return true;
+}
+
+static const struct sys_reg_desc gic_v3_icc_reg_descs[] = {
+	/* ICC_PMR_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b0100), CRm(0b0110), Op2(0b000),
+	  access_gic_pmr },
+	/* ICC_BPR0_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1000), Op2(0b011),
+	  access_gic_bpr0 },
+	/* ICC_AP0R0_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1000), Op2(0b100),
+	  access_gic_ap0r },
+	/* ICC_AP0R1_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1000), Op2(0b101),
+	  access_gic_ap0r },
+	/* ICC_AP0R2_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1000), Op2(0b110),
+	  access_gic_ap0r },
+	/* ICC_AP0R3_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1000), Op2(0b111),
+	  access_gic_ap0r },
+	/* ICC_AP1R0_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1001), Op2(0b000),
+	  access_gic_ap1r },
+	/* ICC_AP1R1_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1001), Op2(0b001),
+	  access_gic_ap1r },
+	/* ICC_AP1R2_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1001), Op2(0b010),
+	  access_gic_ap1r },
+	/* ICC_AP1R3_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1001), Op2(0b011),
+	  access_gic_ap1r },
+	/* 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_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 },
+};
+
+int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
+				u64 *reg)
+{
+	struct sys_reg_params params;
+
+	params.regval = le64_to_cpu(*reg);
+	params.is_write = is_write;
+	params.is_aarch32 = false;
+	params.is_32bit = false;
+
+	return find_reg_by_id(id, &params, gic_v3_icc_reg_descs,
+			      ARRAY_SIZE(gic_v3_icc_reg_descs)) ?
+		0 : -ENXIO;
+}
+
+int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, u64 id,
+				u64 *reg)
+{
+	struct sys_reg_params params;
+	const struct sys_reg_desc *r;
+
+	if (is_write)
+		params.regval = le64_to_cpu(*reg);
+	params.is_write = is_write;
+	params.is_aarch32 = false;
+	params.is_32bit = false;
+
+	r = find_reg_by_id(id, &params, gic_v3_icc_reg_descs,
+			   ARRAY_SIZE(gic_v3_icc_reg_descs));
+	if (!r)
+		return -ENXIO;
+
+	if (!r->access(vcpu, &params, r))
+		return -EINVAL;
+
+	if (!is_write)
+		*reg = cpu_to_le64(params.regval);
+
+	return 0;
+}
+
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 8637690..23db38d 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -79,6 +79,10 @@ int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
 			 int offset, u32 *val);
 int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
 			 int offset, u32 *val);
+int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write,
+			 u64 id, u64 *val);
+int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
+				u64 *reg);
 #else
 static inline void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu)
 {
@@ -132,6 +136,8 @@ static inline int vgic_register_redist_iodevs(struct kvm *kvm,
 }
 #endif
 
+void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
+void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
 void kvm_register_vgic_device(unsigned long type);
 int vgic_lazy_init(struct kvm *kvm);
 int vgic_init(struct kvm *kvm);
-- 
1.9.1

[RFC PATCH v2 4/4] arm/arm64: vgic-new: Implement VGICv3 CPU interface access

[vijay.kilari at gmail.com]

From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>

VGICv3 CPU interface registers are accessed using
KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
as 64-bit. The cpu MPIDR value is passed along with register id.
is used to identify the cpu for registers access.

The version of VGIC v3 specification is define here
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
---
 arch/arm64/include/uapi/asm/kvm.h   |  15 ++-
 arch/arm64/kvm/Makefile             |   1 +
 include/linux/irqchip/arm-gic-v3.h  |   4 +
 virt/kvm/arm/vgic/vgic-kvm-device.c |  31 +++++
 virt/kvm/arm/vgic/vgic-mmio-v2.c    |   4 +-
 virt/kvm/arm/vgic/vgic-mmio-v3.c    |   6 +
 virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 225 ++++++++++++++++++++++++++++++++++++
 virt/kvm/arm/vgic/vgic.h            |   6 +
 8 files changed, 287 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index a6b996e..85829fb 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -180,14 +180,14 @@ struct kvm_arch_memory_slot {
 	KVM_REG_ARM64_SYSREG_ ## n ## _MASK)
 
 #define __ARM64_SYS_REG(op0,op1,crn,crm,op2) \
-	(KVM_REG_ARM64 | KVM_REG_ARM64_SYSREG | \
-	ARM64_SYS_REG_SHIFT_MASK(op0, OP0) | \
+	(ARM64_SYS_REG_SHIFT_MASK(op0, OP0) | \
 	ARM64_SYS_REG_SHIFT_MASK(op1, OP1) | \
 	ARM64_SYS_REG_SHIFT_MASK(crn, CRN) | \
 	ARM64_SYS_REG_SHIFT_MASK(crm, CRM) | \
 	ARM64_SYS_REG_SHIFT_MASK(op2, OP2))
 
-#define ARM64_SYS_REG(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U64)
+#define ARM64_SYS_REG(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U64 | \
+			    KVM_REG_ARM64 | KVM_REG_ARM64_SYSREG)
 
 #define KVM_REG_ARM_TIMER_CTL		ARM64_SYS_REG(3, 3, 14, 3, 1)
 #define KVM_REG_ARM_TIMER_CNT		ARM64_SYS_REG(3, 3, 14, 3, 2)
@@ -206,7 +206,16 @@ 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_GRP_REDIST_REGS 5
+#define KVM_DEV_ARM_VGIC_CPU_SYSREGS    6
+
 #define   KVM_DEV_ARM_VGIC_CTRL_INIT	0
+#define   KVM_DEV_ARM_VGIC_SYSREG_MASK (KVM_REG_ARM64_SYSREG_OP0_MASK | \
+					KVM_REG_ARM64_SYSREG_OP1_MASK | \
+					KVM_REG_ARM64_SYSREG_CRN_MASK | \
+					KVM_REG_ARM64_SYSREG_CRM_MASK | \
+					KVM_REG_ARM64_SYSREG_OP2_MASK)
+#define   KVM_DEV_ARM_VGIC_SYSREG(op0, op1, crn, crm, op2) \
+					__ARM64_SYS_REG(op0, op1, crn, crm, op2)
 
 /* Device Control API on vcpu fd */
 #define KVM_ARM_VCPU_PMU_V3_CTRL	0
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index a7a958c..a962525 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -30,6 +30,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio.o
 kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v2.o
 kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v3.o
 kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
+kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-sys-reg-v3.o
 else
 kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic.o
 kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic-v2.o
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index bfbd707..ae84cde 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -288,6 +288,10 @@
 
 #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_BPR1_SHIFT		18
 #define ICH_VMCR_BPR1_MASK		(7 << ICH_VMCR_BPR1_SHIFT)
 #define ICH_VMCR_BPR0_SHIFT		21
diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
index 986f8e1..b7ca929 100644
--- a/virt/kvm/arm/vgic/vgic-kvm-device.c
+++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
@@ -243,6 +243,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev,
 	struct kvm_vcpu *vcpu, *tmp_vcpu;
 	int vcpu_lock_idx = -1;
 	u32 tmp32;
+	u64 regid;
 	struct vgic_dist *vgic = &dev->kvm->arch.vgic;
 
 	if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2) {
@@ -311,6 +312,16 @@ static int vgic_attr_regs_access(struct kvm_device *dev,
 			ret = -EINVAL;
 		}
 		break;
+	case KVM_DEV_ARM_VGIC_CPU_SYSREGS:
+		if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
+			regid = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_MASK) |
+				KVM_REG_SIZE_U64;
+			ret = vgic_v3_cpu_sysregs_uaccess(vcpu, is_write,
+							  regid, reg);
+		} else {
+			ret = -EINVAL;
+		}
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -442,6 +453,15 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
 		reg = tmp32;
 		return vgic_attr_regs_access(dev, attr, &reg, true);
 	}
+	case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
+		u64 __user *uaddr = (u64 __user *)(long)attr->addr;
+		u64 reg;
+
+		if (get_user(reg, uaddr))
+			return -EFAULT;
+
+		return vgic_attr_regs_access(dev, attr, &reg, true);
+	}
 	}
 	return -ENXIO;
 }
@@ -469,6 +489,16 @@ static int vgic_v3_get_attr(struct kvm_device *dev,
 		ret = put_user(tmp32, uaddr);
 		return ret;
 	}
+	case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
+		u64 __user *uaddr = (u64 __user *)(long)attr->addr;
+		u64 reg;
+
+		ret = vgic_attr_regs_access(dev, attr, &reg, false);
+		if (ret)
+			return ret;
+		ret = put_user(reg, uaddr);
+		return ret;
+	}
 	}
 
 	return -ENXIO;
@@ -487,6 +517,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_REDIST_REGS:
+	case KVM_DEV_ARM_VGIC_CPU_SYSREGS:
 		return vgic_v3_has_attr_regs(dev, attr);
 	case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
 		return 0;
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index a213936..97d94b2 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -204,7 +204,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu,
 	}
 }
 
-static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
+void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
 {
 	if (kvm_vgic_global_state.type == VGIC_V2)
 		vgic_v2_set_vmcr(vcpu, vmcr);
@@ -212,7 +212,7 @@ static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
 		vgic_v3_set_vmcr(vcpu, vmcr);
 }
 
-static void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
+void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
 {
 	if (kvm_vgic_global_state.type == VGIC_V2)
 		vgic_v2_get_vmcr(vcpu, vmcr);
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 36b4882..0c6994a 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -23,6 +23,7 @@
 
 #include "vgic.h"
 #include "vgic-mmio.h"
+#include "sys_regs.h"
 
 /* extract @num bytes at @offset bytes offset in data */
 static unsigned long extract_bytes(unsigned long data, unsigned int offset,
@@ -382,6 +383,11 @@ int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
 		nr_regions = rd_dev->nr_regions;
 		break;
 	}
+	case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
+		u64 reg;
+
+		return vgic_v3_has_cpu_sysregs_attr(vcpu, 0, cpuid, &reg);
+	}
 	default:
 		return -ENXIO;
 	}
diff --git a/virt/kvm/arm/vgic/vgic-sys-reg-v3.c b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
new file mode 100644
index 0000000..d877cd8
--- /dev/null
+++ b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
@@ -0,0 +1,225 @@
+#include <linux/irqchip/arm-gic-v3.h>
+#include <linux/kvm.h>
+#include <linux/kvm_host.h>
+#include <kvm/iodev.h>
+#include <kvm/arm_vgic.h>
+#include <asm/kvm_emulate.h>
+#include <asm/kvm_arm.h>
+#include <asm/kvm_mmu.h>
+
+#include "vgic.h"
+#include "vgic-mmio.h"
+#include "sys_regs.h"
+
+static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+			    const struct sys_reg_desc *r)
+{
+	struct vgic_vmcr vmcr;
+
+	vgic_get_vmcr(vcpu, &vmcr);
+	if (p->is_write) {
+		vmcr.ctlr = (u32)p->regval;
+		vgic_set_vmcr(vcpu, &vmcr);
+	} else {
+		p->regval = vmcr.ctlr;
+	}
+
+	return true;
+}
+
+static bool access_gic_pmr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+			   const struct sys_reg_desc *r)
+{
+	struct vgic_vmcr vmcr;
+
+	vgic_get_vmcr(vcpu, &vmcr);
+	if (p->is_write) {
+		vmcr.pmr = (u32)p->regval;
+		vgic_set_vmcr(vcpu, &vmcr);
+	} else {
+		p->regval = vmcr.pmr;
+	}
+
+	return true;
+}
+
+static bool access_gic_bpr0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+			    const struct sys_reg_desc *r)
+{
+	struct vgic_vmcr vmcr;
+
+	vgic_get_vmcr(vcpu, &vmcr);
+	if (p->is_write) {
+		vmcr.bpr = (u32)p->regval;
+		vgic_set_vmcr(vcpu, &vmcr);
+	} else {
+		p->regval = vmcr.bpr;
+	}
+
+	return true;
+}
+
+static bool access_gic_bpr1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+			    const struct sys_reg_desc *r)
+{
+	struct vgic_vmcr vmcr;
+
+	vgic_get_vmcr(vcpu, &vmcr);
+	if (p->is_write) {
+		vmcr.abpr = (u32)p->regval;
+		vgic_set_vmcr(vcpu, &vmcr);
+	} else {
+		p->regval = vmcr.abpr;
+	}
+
+	return true;
+}
+
+static bool access_gic_grpen0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+			      const struct sys_reg_desc *r)
+{
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (p->is_write) {
+		vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG0;
+		vgicv3->vgic_vmcr |= (p->regval << ICH_VMCR_ENG0_SHIFT) &
+				      ICH_VMCR_ENG0;
+	} else {
+		p->regval = (vgicv3->vgic_vmcr & ICH_VMCR_ENG0) >>
+			     ICH_VMCR_ENG0_SHIFT;
+	}
+
+	return true;
+}
+
+static bool access_gic_grpen1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+			      const struct sys_reg_desc *r)
+{
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (p->is_write) {
+		vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG1;
+		vgicv3->vgic_vmcr |= (p->regval << ICH_VMCR_ENG1_SHIFT) &
+				      ICH_VMCR_ENG1;
+	} else {
+		p->regval = (vgicv3->vgic_vmcr & ICH_VMCR_ENG1) >>
+			     ICH_VMCR_ENG1_SHIFT;
+	}
+
+	return true;
+}
+
+static bool access_gic_ap0r(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+			    const struct sys_reg_desc *r)
+{
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+	u8 idx = r->Op2 & 3;
+
+	if (p->is_write)
+		vgicv3->vgic_ap0r[idx] = p->regval;
+	else
+		p->regval = vgicv3->vgic_ap0r[idx];
+
+	return true;
+}
+
+static bool access_gic_ap1r(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+			    const struct sys_reg_desc *r)
+{
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+	u8 idx = r->Op2 & 3;
+
+	if (p->is_write)
+		vgicv3->vgic_ap1r[idx] = p->regval;
+	else
+		p->regval = vgicv3->vgic_ap1r[idx];
+
+	return true;
+}
+
+static const struct sys_reg_desc gic_v3_icc_reg_descs[] = {
+	/* ICC_PMR_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b0100), CRm(0b0110), Op2(0b000),
+	  access_gic_pmr },
+	/* ICC_BPR0_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1000), Op2(0b011),
+	  access_gic_bpr0 },
+	/* ICC_AP0R0_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1000), Op2(0b100),
+	  access_gic_ap0r },
+	/* ICC_AP0R1_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1000), Op2(0b101),
+	  access_gic_ap0r },
+	/* ICC_AP0R2_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1000), Op2(0b110),
+	  access_gic_ap0r },
+	/* ICC_AP0R3_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1000), Op2(0b111),
+	  access_gic_ap0r },
+	/* ICC_AP1R0_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1001), Op2(0b000),
+	  access_gic_ap1r },
+	/* ICC_AP1R1_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1001), Op2(0b001),
+	  access_gic_ap1r },
+	/* ICC_AP1R2_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1001), Op2(0b010),
+	  access_gic_ap1r },
+	/* ICC_AP1R3_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1001), Op2(0b011),
+	  access_gic_ap1r },
+	/* 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_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 },
+};
+
+int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
+				u64 *reg)
+{
+	struct sys_reg_params params;
+
+	params.regval = le64_to_cpu(*reg);
+	params.is_write = is_write;
+	params.is_aarch32 = false;
+	params.is_32bit = false;
+
+	return find_reg_by_id(id, &params, gic_v3_icc_reg_descs,
+			      ARRAY_SIZE(gic_v3_icc_reg_descs)) ?
+		0 : -ENXIO;
+}
+
+int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, u64 id,
+				u64 *reg)
+{
+	struct sys_reg_params params;
+	const struct sys_reg_desc *r;
+
+	if (is_write)
+		params.regval = le64_to_cpu(*reg);
+	params.is_write = is_write;
+	params.is_aarch32 = false;
+	params.is_32bit = false;
+
+	r = find_reg_by_id(id, &params, gic_v3_icc_reg_descs,
+			   ARRAY_SIZE(gic_v3_icc_reg_descs));
+	if (!r)
+		return -ENXIO;
+
+	if (!r->access(vcpu, &params, r))
+		return -EINVAL;
+
+	if (!is_write)
+		*reg = cpu_to_le64(params.regval);
+
+	return 0;
+}
+
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 8637690..23db38d 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -79,6 +79,10 @@ int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
 			 int offset, u32 *val);
 int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
 			 int offset, u32 *val);
+int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write,
+			 u64 id, u64 *val);
+int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
+				u64 *reg);
 #else
 static inline void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu)
 {
@@ -132,6 +136,8 @@ static inline int vgic_register_redist_iodevs(struct kvm *kvm,
 }
 #endif
 
+void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
+void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
 void kvm_register_vgic_device(unsigned long type);
 int vgic_lazy_init(struct kvm *kvm);
 int vgic_init(struct kvm *kvm);
-- 
1.9.1

Re: [RFC PATCH v2 0/4] arm/arm64: vgic-new: Implement API for vGICv3 live migration

[Peter Maydell]

On 9 August 2016 at 11:58,  <vijay.kilari@gmail.com> wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> This patchset adds API for saving and restoring
> of VGICv3 registers to support live migration with new vgic feature.
> This API definition is as per version of VGICv3 specification
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>
> To test live migration with QEMU, use below patch series
> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01444.html
>
> The patch 3 & 4 are picked from the Pavel's previous implementation.
> http://www.spinics.net/lists/kvm/msg122040.html
>
> v1 => v2:
>  - The init sequence change patch is no more required.
>    Fixed in patch 2 by using static vgic_io_dev regions structure instead
>    of using dynamic allocation pointer.
>  - Updated commit message of patch 4.
>  - Dropped usage of union to manage 32-bit and 64-bit access in patch 1.
>    Used local variable for 32-bit access.
>  - Updated macro __ARM64_SYS_REG and ARM64_SYS_REG in
>    arch/arm64/include/uapi/asm/kvm.h as per qemu requirements.

I only scanned briefly through this patchset, but I didn't
see any code implementing:
 * KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO
 * the different behaviour for accesses to GICD_STATUSR, GICR_STATUSR,
   GICD_ISPENDR, GICR_ISPENDR0, GICD_ICPENDR, and GICR_ICPENDR0, which
   don't act the same via this API as for a guest access to the register

Did I miss something?

thanks
-- PMM

[RFC PATCH v2 0/4] arm/arm64: vgic-new: Implement API for vGICv3 live migration

[Peter Maydell]

On 9 August 2016 at 11:58,  <vijay.kilari@gmail.com> wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> This patchset adds API for saving and restoring
> of VGICv3 registers to support live migration with new vgic feature.
> This API definition is as per version of VGICv3 specification
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>
> To test live migration with QEMU, use below patch series
> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01444.html
>
> The patch 3 & 4 are picked from the Pavel's previous implementation.
> http://www.spinics.net/lists/kvm/msg122040.html
>
> v1 => v2:
>  - The init sequence change patch is no more required.
>    Fixed in patch 2 by using static vgic_io_dev regions structure instead
>    of using dynamic allocation pointer.
>  - Updated commit message of patch 4.
>  - Dropped usage of union to manage 32-bit and 64-bit access in patch 1.
>    Used local variable for 32-bit access.
>  - Updated macro __ARM64_SYS_REG and ARM64_SYS_REG in
>    arch/arm64/include/uapi/asm/kvm.h as per qemu requirements.

I only scanned briefly through this patchset, but I didn't
see any code implementing:
 * KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO
 * the different behaviour for accesses to GICD_STATUSR, GICR_STATUSR,
   GICD_ISPENDR, GICR_ISPENDR0, GICD_ICPENDR, and GICR_ICPENDR0, which
   don't act the same via this API as for a guest access to the register

Did I miss something?

thanks
-- PMM

Re: [RFC PATCH v2 0/4] arm/arm64: vgic-new: Implement API for vGICv3 live migration

[Vijay Kilari]

On Tue, Aug 9, 2016 at 5:22 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 9 August 2016 at 11:58,  <vijay.kilari@gmail.com> wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>
>> This patchset adds API for saving and restoring
>> of VGICv3 registers to support live migration with new vgic feature.
>> This API definition is as per version of VGICv3 specification
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>>
>> To test live migration with QEMU, use below patch series
>> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01444.html
>>
>> The patch 3 & 4 are picked from the Pavel's previous implementation.
>> http://www.spinics.net/lists/kvm/msg122040.html
>>
>> v1 => v2:
>>  - The init sequence change patch is no more required.
>>    Fixed in patch 2 by using static vgic_io_dev regions structure instead
>>    of using dynamic allocation pointer.
>>  - Updated commit message of patch 4.
>>  - Dropped usage of union to manage 32-bit and 64-bit access in patch 1.
>>    Used local variable for 32-bit access.
>>  - Updated macro __ARM64_SYS_REG and ARM64_SYS_REG in
>>    arch/arm64/include/uapi/asm/kvm.h as per qemu requirements.
>
> I only scanned briefly through this patchset, but I didn't
> see any code implementing:
>  * KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO

If irq->pending is updated by kernel based on irq->line_level when interrupt
is asserted by device or guest. Do we still need to extract
irq->line_level using
this ioctl and while writing back GIC{D|R}_ISPENDR with line_level
+(OR) GIC{D|R}_ISPENDR?

>  * the different behaviour for accesses to GICD_STATUSR, GICR_STATUSR,

QEMU is saving and restoring this register, but kernel implementation
is missing. Kernel is silently returning zero. So could not catch. I
will fix it.

However, Specification says as below for STATUSR.

"    The GICD_STATUSR and GICR_STATUSR registers are architecturally
defined such
     that a write of a clear bit has no effect, whereas a write with a set bit
     clears that value.  To allow userspace to freely set the values
of these two
     registers, setting the attributes with the register offsets for these two
     registers simply sets the non-reserved bits to the value written."

Question is during restore, the set bit will clear the value STATUSR.
So it will reset the STATUSR after migrating the VM.

>    GICD_ISPENDR, GICR_ISPENDR0, GICD_ICPENDR, and GICR_ICPENDR0, which
>    don't act the same via this API as for a guest access to the register
>
> Did I miss something?

In kernel (as shown in below code snippet),
 GICD_ISPENDR, GICR_ISPENDR0, GICD_ICPENDR, and GICR_ICPENDR0
all register access using KVM_DEV_ARM_VGIC_GRP_{RE|DIST}_REGS ioctl
is accessing irq->pending state.

unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
                                     gpa_t addr, unsigned int len)
{
        u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
        u32 value = 0;
        int i;

        /* Loop over all IRQs affected by this read */
        for (i = 0; i < len * 8; i++) {
                struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);

                if (irq->pending)
                        value |= (1U << i);
        }

  ...
}
>
> thanks
> -- PMM

[RFC PATCH v2 0/4] arm/arm64: vgic-new: Implement API for vGICv3 live migration

[Vijay Kilari]

On Tue, Aug 9, 2016 at 5:22 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 9 August 2016 at 11:58,  <vijay.kilari@gmail.com> wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>
>> This patchset adds API for saving and restoring
>> of VGICv3 registers to support live migration with new vgic feature.
>> This API definition is as per version of VGICv3 specification
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>>
>> To test live migration with QEMU, use below patch series
>> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01444.html
>>
>> The patch 3 & 4 are picked from the Pavel's previous implementation.
>> http://www.spinics.net/lists/kvm/msg122040.html
>>
>> v1 => v2:
>>  - The init sequence change patch is no more required.
>>    Fixed in patch 2 by using static vgic_io_dev regions structure instead
>>    of using dynamic allocation pointer.
>>  - Updated commit message of patch 4.
>>  - Dropped usage of union to manage 32-bit and 64-bit access in patch 1.
>>    Used local variable for 32-bit access.
>>  - Updated macro __ARM64_SYS_REG and ARM64_SYS_REG in
>>    arch/arm64/include/uapi/asm/kvm.h as per qemu requirements.
>
> I only scanned briefly through this patchset, but I didn't
> see any code implementing:
>  * KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO

If irq->pending is updated by kernel based on irq->line_level when interrupt
is asserted by device or guest. Do we still need to extract
irq->line_level using
this ioctl and while writing back GIC{D|R}_ISPENDR with line_level
+(OR) GIC{D|R}_ISPENDR?

>  * the different behaviour for accesses to GICD_STATUSR, GICR_STATUSR,

QEMU is saving and restoring this register, but kernel implementation
is missing. Kernel is silently returning zero. So could not catch. I
will fix it.

However, Specification says as below for STATUSR.

"    The GICD_STATUSR and GICR_STATUSR registers are architecturally
defined such
     that a write of a clear bit has no effect, whereas a write with a set bit
     clears that value.  To allow userspace to freely set the values
of these two
     registers, setting the attributes with the register offsets for these two
     registers simply sets the non-reserved bits to the value written."

Question is during restore, the set bit will clear the value STATUSR.
So it will reset the STATUSR after migrating the VM.

>    GICD_ISPENDR, GICR_ISPENDR0, GICD_ICPENDR, and GICR_ICPENDR0, which
>    don't act the same via this API as for a guest access to the register
>
> Did I miss something?

In kernel (as shown in below code snippet),
 GICD_ISPENDR, GICR_ISPENDR0, GICD_ICPENDR, and GICR_ICPENDR0
all register access using KVM_DEV_ARM_VGIC_GRP_{RE|DIST}_REGS ioctl
is accessing irq->pending state.

unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
                                     gpa_t addr, unsigned int len)
{
        u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
        u32 value = 0;
        int i;

        /* Loop over all IRQs affected by this read */
        for (i = 0; i < len * 8; i++) {
                struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);

                if (irq->pending)
                        value |= (1U << i);
        }

  ...
}
>
> thanks
> -- PMM

Re: [RFC PATCH v2 0/4] arm/arm64: vgic-new: Implement API for vGICv3 live migration

[Peter Maydell]

On 11 August 2016 at 06:29, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> On Tue, Aug 9, 2016 at 5:22 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 9 August 2016 at 11:58,  <vijay.kilari@gmail.com> wrote:
>>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>>
>>> This patchset adds API for saving and restoring
>>> of VGICv3 registers to support live migration with new vgic feature.
>>> This API definition is as per version of VGICv3 specification
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>>>
>>> To test live migration with QEMU, use below patch series
>>> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01444.html
>>>
>>> The patch 3 & 4 are picked from the Pavel's previous implementation.
>>> http://www.spinics.net/lists/kvm/msg122040.html
>>>
>>> v1 => v2:
>>>  - The init sequence change patch is no more required.
>>>    Fixed in patch 2 by using static vgic_io_dev regions structure instead
>>>    of using dynamic allocation pointer.
>>>  - Updated commit message of patch 4.
>>>  - Dropped usage of union to manage 32-bit and 64-bit access in patch 1.
>>>    Used local variable for 32-bit access.
>>>  - Updated macro __ARM64_SYS_REG and ARM64_SYS_REG in
>>>    arch/arm64/include/uapi/asm/kvm.h as per qemu requirements.
>>
>> I only scanned briefly through this patchset, but I didn't
>> see any code implementing:
>>  * KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO
>
> If irq->pending is updated by kernel based on irq->line_level when interrupt
> is asserted by device or guest. Do we still need to extract
> irq->line_level using
> this ioctl and while writing back GIC{D|R}_ISPENDR with line_level
> +(OR) GIC{D|R}_ISPENDR?

The level and the pending status are different things;
the API docs have an explanation of this. The API access
to the ISPENDR registers should return only the pending
latch status (which is not the same as what these registers
return if you read them from the guest).

>>  * the different behaviour for accesses to GICD_STATUSR, GICR_STATUSR,
>
> QEMU is saving and restoring this register, but kernel implementation
> is missing. Kernel is silently returning zero. So could not catch. I
> will fix it.
>
> However, Specification says as below for STATUSR.
>
> "    The GICD_STATUSR and GICR_STATUSR registers are architecturally
> defined such
>      that a write of a clear bit has no effect, whereas a write with a set bit
>      clears that value.  To allow userspace to freely set the values
> of these two
>      registers, setting the attributes with the register offsets for these two
>      registers simply sets the non-reserved bits to the value written."
>
> Question is during restore, the set bit will clear the value STATUSR.
> So it will reset the STATUSR after migrating the VM.

The text you quote above says that setting the attribute via
the API "sets the non-reserved bits to the value written".
This is the point -- it does not have the write-1-to-clear
behaviour that a guest access to the register does.

>>    GICD_ISPENDR, GICR_ISPENDR0, GICD_ICPENDR, and GICR_ICPENDR0, which
>>    don't act the same via this API as for a guest access to the register
>>
>> Did I miss something?
>
> In kernel (as shown in below code snippet),
>  GICD_ISPENDR, GICR_ISPENDR0, GICD_ICPENDR, and GICR_ICPENDR0
> all register access using KVM_DEV_ARM_VGIC_GRP_{RE|DIST}_REGS ioctl
> is accessing irq->pending state.
>
> unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
>                                      gpa_t addr, unsigned int len)
> {
>         u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
>         u32 value = 0;
>         int i;
>
>         /* Loop over all IRQs affected by this read */
>         for (i = 0; i < len * 8; i++) {
>                 struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>
>                 if (irq->pending)
>                         value |= (1U << i);
>         }
>
>   ...
> }

This is the code for handling a guest access to this register.
The behaviour for access from userspace via this API has
to be different, and therefore it must not use this code.
The API doc specifies how it must differ.

thanks
-- PMM

[RFC PATCH v2 0/4] arm/arm64: vgic-new: Implement API for vGICv3 live migration

[Peter Maydell]

On 11 August 2016 at 06:29, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> On Tue, Aug 9, 2016 at 5:22 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 9 August 2016 at 11:58,  <vijay.kilari@gmail.com> wrote:
>>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>>
>>> This patchset adds API for saving and restoring
>>> of VGICv3 registers to support live migration with new vgic feature.
>>> This API definition is as per version of VGICv3 specification
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>>>
>>> To test live migration with QEMU, use below patch series
>>> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01444.html
>>>
>>> The patch 3 & 4 are picked from the Pavel's previous implementation.
>>> http://www.spinics.net/lists/kvm/msg122040.html
>>>
>>> v1 => v2:
>>>  - The init sequence change patch is no more required.
>>>    Fixed in patch 2 by using static vgic_io_dev regions structure instead
>>>    of using dynamic allocation pointer.
>>>  - Updated commit message of patch 4.
>>>  - Dropped usage of union to manage 32-bit and 64-bit access in patch 1.
>>>    Used local variable for 32-bit access.
>>>  - Updated macro __ARM64_SYS_REG and ARM64_SYS_REG in
>>>    arch/arm64/include/uapi/asm/kvm.h as per qemu requirements.
>>
>> I only scanned briefly through this patchset, but I didn't
>> see any code implementing:
>>  * KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO
>
> If irq->pending is updated by kernel based on irq->line_level when interrupt
> is asserted by device or guest. Do we still need to extract
> irq->line_level using
> this ioctl and while writing back GIC{D|R}_ISPENDR with line_level
> +(OR) GIC{D|R}_ISPENDR?

The level and the pending status are different things;
the API docs have an explanation of this. The API access
to the ISPENDR registers should return only the pending
latch status (which is not the same as what these registers
return if you read them from the guest).

>>  * the different behaviour for accesses to GICD_STATUSR, GICR_STATUSR,
>
> QEMU is saving and restoring this register, but kernel implementation
> is missing. Kernel is silently returning zero. So could not catch. I
> will fix it.
>
> However, Specification says as below for STATUSR.
>
> "    The GICD_STATUSR and GICR_STATUSR registers are architecturally
> defined such
>      that a write of a clear bit has no effect, whereas a write with a set bit
>      clears that value.  To allow userspace to freely set the values
> of these two
>      registers, setting the attributes with the register offsets for these two
>      registers simply sets the non-reserved bits to the value written."
>
> Question is during restore, the set bit will clear the value STATUSR.
> So it will reset the STATUSR after migrating the VM.

The text you quote above says that setting the attribute via
the API "sets the non-reserved bits to the value written".
This is the point -- it does not have the write-1-to-clear
behaviour that a guest access to the register does.

>>    GICD_ISPENDR, GICR_ISPENDR0, GICD_ICPENDR, and GICR_ICPENDR0, which
>>    don't act the same via this API as for a guest access to the register
>>
>> Did I miss something?
>
> In kernel (as shown in below code snippet),
>  GICD_ISPENDR, GICR_ISPENDR0, GICD_ICPENDR, and GICR_ICPENDR0
> all register access using KVM_DEV_ARM_VGIC_GRP_{RE|DIST}_REGS ioctl
> is accessing irq->pending state.
>
> unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
>                                      gpa_t addr, unsigned int len)
> {
>         u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
>         u32 value = 0;
>         int i;
>
>         /* Loop over all IRQs affected by this read */
>         for (i = 0; i < len * 8; i++) {
>                 struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>
>                 if (irq->pending)
>                         value |= (1U << i);
>         }
>
>   ...
> }

This is the code for handling a guest access to this register.
The behaviour for access from userspace via this API has
to be different, and therefore it must not use this code.
The API doc specifies how it must differ.

thanks
-- PMM

Re: [RFC PATCH v2 0/4] arm/arm64: vgic-new: Implement API for vGICv3 live migration

[Vijay Kilari]

On Thu, Aug 11, 2016 at 1:15 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 11 August 2016 at 06:29, Vijay Kilari <vijay.kilari@gmail.com> wrote:
>> On Tue, Aug 9, 2016 at 5:22 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 9 August 2016 at 11:58,  <vijay.kilari@gmail.com> wrote:
>>>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>>>
>>>> This patchset adds API for saving and restoring
>>>> of VGICv3 registers to support live migration with new vgic feature.
>>>> This API definition is as per version of VGICv3 specification
>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>>>>
>>>> To test live migration with QEMU, use below patch series
>>>> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01444.html
>>>>
>>>> The patch 3 & 4 are picked from the Pavel's previous implementation.
>>>> http://www.spinics.net/lists/kvm/msg122040.html
>>>>
>>>> v1 => v2:
>>>>  - The init sequence change patch is no more required.
>>>>    Fixed in patch 2 by using static vgic_io_dev regions structure instead
>>>>    of using dynamic allocation pointer.
>>>>  - Updated commit message of patch 4.
>>>>  - Dropped usage of union to manage 32-bit and 64-bit access in patch 1.
>>>>    Used local variable for 32-bit access.
>>>>  - Updated macro __ARM64_SYS_REG and ARM64_SYS_REG in
>>>>    arch/arm64/include/uapi/asm/kvm.h as per qemu requirements.
>>>
>>> I only scanned briefly through this patchset, but I didn't
>>> see any code implementing:
>>>  * KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO
>>
>> If irq->pending is updated by kernel based on irq->line_level when interrupt
>> is asserted by device or guest. Do we still need to extract
>> irq->line_level using
>> this ioctl and while writing back GIC{D|R}_ISPENDR with line_level
>> +(OR) GIC{D|R}_ISPENDR?
>
> The level and the pending status are different things;
> the API docs have an explanation of this. The API access
> to the ISPENDR registers should return only the pending
> latch status (which is not the same as what these registers
> return if you read them from the guest).
>
>>>  * the different behaviour for accesses to GICD_STATUSR, GICR_STATUSR,
>>
>> QEMU is saving and restoring this register, but kernel implementation
>> is missing. Kernel is silently returning zero. So could not catch. I
>> will fix it.
>>
>> However, Specification says as below for STATUSR.
>>
>> "    The GICD_STATUSR and GICR_STATUSR registers are architecturally
>> defined such
>>      that a write of a clear bit has no effect, whereas a write with a set bit
>>      clears that value.  To allow userspace to freely set the values
>> of these two
>>      registers, setting the attributes with the register offsets for these two
>>      registers simply sets the non-reserved bits to the value written."
>>
>> Question is during restore, the set bit will clear the value STATUSR.
>> So it will reset the STATUSR after migrating the VM.
>
> The text you quote above says that setting the attribute via
> the API "sets the non-reserved bits to the value written".
> This is the point -- it does not have the write-1-to-clear
> behaviour that a guest access to the register does.
>
>>>    GICD_ISPENDR, GICR_ISPENDR0, GICD_ICPENDR, and GICR_ICPENDR0, which
>>>    don't act the same via this API as for a guest access to the register
>>>
>>> Did I miss something?
>>
>> In kernel (as shown in below code snippet),
>>  GICD_ISPENDR, GICR_ISPENDR0, GICD_ICPENDR, and GICR_ICPENDR0
>> all register access using KVM_DEV_ARM_VGIC_GRP_{RE|DIST}_REGS ioctl
>> is accessing irq->pending state.
>>
>> unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
>>                                      gpa_t addr, unsigned int len)
>> {
>>         u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
>>         u32 value = 0;
>>         int i;
>>
>>         /* Loop over all IRQs affected by this read */
>>         for (i = 0; i < len * 8; i++) {
>>                 struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>>
>>                 if (irq->pending)
>>                         value |= (1U << i);
>>         }
>>
>>   ...
>> }
>
> This is the code for handling a guest access to this register.
> The behaviour for access from userspace via this API has
> to be different, and therefore it must not use this code.
> The API doc specifies how it must differ.

API doc says,

"For a level triggered interrupt the value accessed
here is that of the latch which is set by ISPENDR and cleared by ICPENDR or
interrupt activation"

Kernel maintains only irq->pending for all interrupts.
By going through the code, there is no separate variable that holds purely
ISPENDR value. With assumption that irq->pending is purely ISPENDR for
level triggerred
interrupt, userspace access to ISPENDR for level triggerred interrupts
can be irq->pending & (~ICPENDR[irq_bit]) | irq->active?.

[RFC PATCH v2 0/4] arm/arm64: vgic-new: Implement API for vGICv3 live migration

[Vijay Kilari]

On Thu, Aug 11, 2016 at 1:15 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 11 August 2016 at 06:29, Vijay Kilari <vijay.kilari@gmail.com> wrote:
>> On Tue, Aug 9, 2016 at 5:22 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 9 August 2016 at 11:58,  <vijay.kilari@gmail.com> wrote:
>>>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>>>
>>>> This patchset adds API for saving and restoring
>>>> of VGICv3 registers to support live migration with new vgic feature.
>>>> This API definition is as per version of VGICv3 specification
>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>>>>
>>>> To test live migration with QEMU, use below patch series
>>>> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01444.html
>>>>
>>>> The patch 3 & 4 are picked from the Pavel's previous implementation.
>>>> http://www.spinics.net/lists/kvm/msg122040.html
>>>>
>>>> v1 => v2:
>>>>  - The init sequence change patch is no more required.
>>>>    Fixed in patch 2 by using static vgic_io_dev regions structure instead
>>>>    of using dynamic allocation pointer.
>>>>  - Updated commit message of patch 4.
>>>>  - Dropped usage of union to manage 32-bit and 64-bit access in patch 1.
>>>>    Used local variable for 32-bit access.
>>>>  - Updated macro __ARM64_SYS_REG and ARM64_SYS_REG in
>>>>    arch/arm64/include/uapi/asm/kvm.h as per qemu requirements.
>>>
>>> I only scanned briefly through this patchset, but I didn't
>>> see any code implementing:
>>>  * KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO
>>
>> If irq->pending is updated by kernel based on irq->line_level when interrupt
>> is asserted by device or guest. Do we still need to extract
>> irq->line_level using
>> this ioctl and while writing back GIC{D|R}_ISPENDR with line_level
>> +(OR) GIC{D|R}_ISPENDR?
>
> The level and the pending status are different things;
> the API docs have an explanation of this. The API access
> to the ISPENDR registers should return only the pending
> latch status (which is not the same as what these registers
> return if you read them from the guest).
>
>>>  * the different behaviour for accesses to GICD_STATUSR, GICR_STATUSR,
>>
>> QEMU is saving and restoring this register, but kernel implementation
>> is missing. Kernel is silently returning zero. So could not catch. I
>> will fix it.
>>
>> However, Specification says as below for STATUSR.
>>
>> "    The GICD_STATUSR and GICR_STATUSR registers are architecturally
>> defined such
>>      that a write of a clear bit has no effect, whereas a write with a set bit
>>      clears that value.  To allow userspace to freely set the values
>> of these two
>>      registers, setting the attributes with the register offsets for these two
>>      registers simply sets the non-reserved bits to the value written."
>>
>> Question is during restore, the set bit will clear the value STATUSR.
>> So it will reset the STATUSR after migrating the VM.
>
> The text you quote above says that setting the attribute via
> the API "sets the non-reserved bits to the value written".
> This is the point -- it does not have the write-1-to-clear
> behaviour that a guest access to the register does.
>
>>>    GICD_ISPENDR, GICR_ISPENDR0, GICD_ICPENDR, and GICR_ICPENDR0, which
>>>    don't act the same via this API as for a guest access to the register
>>>
>>> Did I miss something?
>>
>> In kernel (as shown in below code snippet),
>>  GICD_ISPENDR, GICR_ISPENDR0, GICD_ICPENDR, and GICR_ICPENDR0
>> all register access using KVM_DEV_ARM_VGIC_GRP_{RE|DIST}_REGS ioctl
>> is accessing irq->pending state.
>>
>> unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
>>                                      gpa_t addr, unsigned int len)
>> {
>>         u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
>>         u32 value = 0;
>>         int i;
>>
>>         /* Loop over all IRQs affected by this read */
>>         for (i = 0; i < len * 8; i++) {
>>                 struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
>>
>>                 if (irq->pending)
>>                         value |= (1U << i);
>>         }
>>
>>   ...
>> }
>
> This is the code for handling a guest access to this register.
> The behaviour for access from userspace via this API has
> to be different, and therefore it must not use this code.
> The API doc specifies how it must differ.

API doc says,

"For a level triggered interrupt the value accessed
here is that of the latch which is set by ISPENDR and cleared by ICPENDR or
interrupt activation"

Kernel maintains only irq->pending for all interrupts.
By going through the code, there is no separate variable that holds purely
ISPENDR value. With assumption that irq->pending is purely ISPENDR for
level triggerred
interrupt, userspace access to ISPENDR for level triggerred interrupts
can be irq->pending & (~ICPENDR[irq_bit]) | irq->active?.

Re: [RFC PATCH v2 0/4] arm/arm64: vgic-new: Implement API for vGICv3 live migration

[Christoffer Dall]

On Fri, Aug 12, 2016 at 01:08:12PM +0530, Vijay Kilari wrote:
> On Thu, Aug 11, 2016 at 1:15 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 11 August 2016 at 06:29, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> >> On Tue, Aug 9, 2016 at 5:22 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>> On 9 August 2016 at 11:58,  <vijay.kilari@gmail.com> wrote:
> >>>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> >>>>
> >>>> This patchset adds API for saving and restoring
> >>>> of VGICv3 registers to support live migration with new vgic feature.
> >>>> This API definition is as per version of VGICv3 specification
> >>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
> >>>>
> >>>> To test live migration with QEMU, use below patch series
> >>>> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01444.html
> >>>>
> >>>> The patch 3 & 4 are picked from the Pavel's previous implementation.
> >>>> http://www.spinics.net/lists/kvm/msg122040.html
> >>>>
> >>>> v1 => v2:
> >>>>  - The init sequence change patch is no more required.
> >>>>    Fixed in patch 2 by using static vgic_io_dev regions structure instead
> >>>>    of using dynamic allocation pointer.
> >>>>  - Updated commit message of patch 4.
> >>>>  - Dropped usage of union to manage 32-bit and 64-bit access in patch 1.
> >>>>    Used local variable for 32-bit access.
> >>>>  - Updated macro __ARM64_SYS_REG and ARM64_SYS_REG in
> >>>>    arch/arm64/include/uapi/asm/kvm.h as per qemu requirements.
> >>>
> >>> I only scanned briefly through this patchset, but I didn't
> >>> see any code implementing:
> >>>  * KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO
> >>
> >> If irq->pending is updated by kernel based on irq->line_level when interrupt
> >> is asserted by device or guest. Do we still need to extract
> >> irq->line_level using
> >> this ioctl and while writing back GIC{D|R}_ISPENDR with line_level
> >> +(OR) GIC{D|R}_ISPENDR?
> >
> > The level and the pending status are different things;
> > the API docs have an explanation of this. The API access
> > to the ISPENDR registers should return only the pending
> > latch status (which is not the same as what these registers
> > return if you read them from the guest).
> >
> >>>  * the different behaviour for accesses to GICD_STATUSR, GICR_STATUSR,
> >>
> >> QEMU is saving and restoring this register, but kernel implementation
> >> is missing. Kernel is silently returning zero. So could not catch. I
> >> will fix it.
> >>
> >> However, Specification says as below for STATUSR.
> >>
> >> "    The GICD_STATUSR and GICR_STATUSR registers are architecturally
> >> defined such
> >>      that a write of a clear bit has no effect, whereas a write with a set bit
> >>      clears that value.  To allow userspace to freely set the values
> >> of these two
> >>      registers, setting the attributes with the register offsets for these two
> >>      registers simply sets the non-reserved bits to the value written."
> >>
> >> Question is during restore, the set bit will clear the value STATUSR.
> >> So it will reset the STATUSR after migrating the VM.
> >
> > The text you quote above says that setting the attribute via
> > the API "sets the non-reserved bits to the value written".
> > This is the point -- it does not have the write-1-to-clear
> > behaviour that a guest access to the register does.
> >
> >>>    GICD_ISPENDR, GICR_ISPENDR0, GICD_ICPENDR, and GICR_ICPENDR0, which
> >>>    don't act the same via this API as for a guest access to the register
> >>>
> >>> Did I miss something?
> >>
> >> In kernel (as shown in below code snippet),
> >>  GICD_ISPENDR, GICR_ISPENDR0, GICD_ICPENDR, and GICR_ICPENDR0
> >> all register access using KVM_DEV_ARM_VGIC_GRP_{RE|DIST}_REGS ioctl
> >> is accessing irq->pending state.
> >>
> >> unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
> >>                                      gpa_t addr, unsigned int len)
> >> {
> >>         u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> >>         u32 value = 0;
> >>         int i;
> >>
> >>         /* Loop over all IRQs affected by this read */
> >>         for (i = 0; i < len * 8; i++) {
> >>                 struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> >>
> >>                 if (irq->pending)
> >>                         value |= (1U << i);
> >>         }
> >>
> >>   ...
> >> }
> >
> > This is the code for handling a guest access to this register.
> > The behaviour for access from userspace via this API has
> > to be different, and therefore it must not use this code.
> > The API doc specifies how it must differ.
> 
> API doc says,
> 
> "For a level triggered interrupt the value accessed
> here is that of the latch which is set by ISPENDR and cleared by ICPENDR or
> interrupt activation"
> 
> Kernel maintains only irq->pending for all interrupts.

no, the kernel also maintains irq->soft_pending.

> By going through the code, there is no separate variable that holds purely
> ISPENDR value. With assumption that irq->pending is purely ISPENDR for
> level triggerred

it is not; irq->pending is always an OR of the line_level with the
soft_pending field for level-triggered interrupts.

You can read and understand the semantics of the soft_pending field by
taking a look at vgic_mmio_write_spending and grepping for soft_pending
in the kernel and vgic code in general.


> interrupt, userspace access to ISPENDR for level triggerred interrupts
> can be irq->pending & (~ICPENDR[irq_bit]) | irq->active?.

I don't understand what the active state has to do with userspace
reading the ISPENDR?

Hope this helps,
-Christoffer

[RFC PATCH v2 0/4] arm/arm64: vgic-new: Implement API for vGICv3 live migration

[Christoffer Dall]

On Fri, Aug 12, 2016 at 01:08:12PM +0530, Vijay Kilari wrote:
> On Thu, Aug 11, 2016 at 1:15 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 11 August 2016 at 06:29, Vijay Kilari <vijay.kilari@gmail.com> wrote:
> >> On Tue, Aug 9, 2016 at 5:22 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>> On 9 August 2016 at 11:58,  <vijay.kilari@gmail.com> wrote:
> >>>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> >>>>
> >>>> This patchset adds API for saving and restoring
> >>>> of VGICv3 registers to support live migration with new vgic feature.
> >>>> This API definition is as per version of VGICv3 specification
> >>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
> >>>>
> >>>> To test live migration with QEMU, use below patch series
> >>>> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01444.html
> >>>>
> >>>> The patch 3 & 4 are picked from the Pavel's previous implementation.
> >>>> http://www.spinics.net/lists/kvm/msg122040.html
> >>>>
> >>>> v1 => v2:
> >>>>  - The init sequence change patch is no more required.
> >>>>    Fixed in patch 2 by using static vgic_io_dev regions structure instead
> >>>>    of using dynamic allocation pointer.
> >>>>  - Updated commit message of patch 4.
> >>>>  - Dropped usage of union to manage 32-bit and 64-bit access in patch 1.
> >>>>    Used local variable for 32-bit access.
> >>>>  - Updated macro __ARM64_SYS_REG and ARM64_SYS_REG in
> >>>>    arch/arm64/include/uapi/asm/kvm.h as per qemu requirements.
> >>>
> >>> I only scanned briefly through this patchset, but I didn't
> >>> see any code implementing:
> >>>  * KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO
> >>
> >> If irq->pending is updated by kernel based on irq->line_level when interrupt
> >> is asserted by device or guest. Do we still need to extract
> >> irq->line_level using
> >> this ioctl and while writing back GIC{D|R}_ISPENDR with line_level
> >> +(OR) GIC{D|R}_ISPENDR?
> >
> > The level and the pending status are different things;
> > the API docs have an explanation of this. The API access
> > to the ISPENDR registers should return only the pending
> > latch status (which is not the same as what these registers
> > return if you read them from the guest).
> >
> >>>  * the different behaviour for accesses to GICD_STATUSR, GICR_STATUSR,
> >>
> >> QEMU is saving and restoring this register, but kernel implementation
> >> is missing. Kernel is silently returning zero. So could not catch. I
> >> will fix it.
> >>
> >> However, Specification says as below for STATUSR.
> >>
> >> "    The GICD_STATUSR and GICR_STATUSR registers are architecturally
> >> defined such
> >>      that a write of a clear bit has no effect, whereas a write with a set bit
> >>      clears that value.  To allow userspace to freely set the values
> >> of these two
> >>      registers, setting the attributes with the register offsets for these two
> >>      registers simply sets the non-reserved bits to the value written."
> >>
> >> Question is during restore, the set bit will clear the value STATUSR.
> >> So it will reset the STATUSR after migrating the VM.
> >
> > The text you quote above says that setting the attribute via
> > the API "sets the non-reserved bits to the value written".
> > This is the point -- it does not have the write-1-to-clear
> > behaviour that a guest access to the register does.
> >
> >>>    GICD_ISPENDR, GICR_ISPENDR0, GICD_ICPENDR, and GICR_ICPENDR0, which
> >>>    don't act the same via this API as for a guest access to the register
> >>>
> >>> Did I miss something?
> >>
> >> In kernel (as shown in below code snippet),
> >>  GICD_ISPENDR, GICR_ISPENDR0, GICD_ICPENDR, and GICR_ICPENDR0
> >> all register access using KVM_DEV_ARM_VGIC_GRP_{RE|DIST}_REGS ioctl
> >> is accessing irq->pending state.
> >>
> >> unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
> >>                                      gpa_t addr, unsigned int len)
> >> {
> >>         u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> >>         u32 value = 0;
> >>         int i;
> >>
> >>         /* Loop over all IRQs affected by this read */
> >>         for (i = 0; i < len * 8; i++) {
> >>                 struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> >>
> >>                 if (irq->pending)
> >>                         value |= (1U << i);
> >>         }
> >>
> >>   ...
> >> }
> >
> > This is the code for handling a guest access to this register.
> > The behaviour for access from userspace via this API has
> > to be different, and therefore it must not use this code.
> > The API doc specifies how it must differ.
> 
> API doc says,
> 
> "For a level triggered interrupt the value accessed
> here is that of the latch which is set by ISPENDR and cleared by ICPENDR or
> interrupt activation"
> 
> Kernel maintains only irq->pending for all interrupts.

no, the kernel also maintains irq->soft_pending.

> By going through the code, there is no separate variable that holds purely
> ISPENDR value. With assumption that irq->pending is purely ISPENDR for
> level triggerred

it is not; irq->pending is always an OR of the line_level with the
soft_pending field for level-triggered interrupts.

You can read and understand the semantics of the soft_pending field by
taking a look at vgic_mmio_write_spending and grepping for soft_pending
in the kernel and vgic code in general.


> interrupt, userspace access to ISPENDR for level triggerred interrupts
> can be irq->pending & (~ICPENDR[irq_bit]) | irq->active?.

I don't understand what the active state has to do with userspace
reading the ISPENDR?

Hope this helps,
-Christoffer

Re: [RFC PATCH v2 1/4] arm/arm64: vgic-new: Introduce 64-bit reg access support

[Christoffer Dall]

On Tue, Aug 09, 2016 at 04:28:43PM +0530, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> 
> vgic_attr_regs_access() handles only 32-bit register
> value. Pass u64 as parameter and locally handle 32-bit
> reads and writes depending on attribute group.
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
>  virt/kvm/arm/vgic/vgic-kvm-device.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index 0130c4b..06de322 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -236,12 +236,13 @@ void kvm_register_vgic_device(unsigned long type)
>   */
>  static int vgic_attr_regs_access(struct kvm_device *dev,
>  				 struct kvm_device_attr *attr,
> -				 u32 *reg, bool is_write)
> +				 u64 *reg, bool is_write)
>  {
>  	gpa_t addr;
>  	int cpuid, ret, c;
>  	struct kvm_vcpu *vcpu, *tmp_vcpu;
>  	int vcpu_lock_idx = -1;
> +	u32 tmp32;
>  
>  	cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
>  		 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
> @@ -272,12 +273,19 @@ static int vgic_attr_regs_access(struct kvm_device *dev,
>  		vcpu_lock_idx = c;
>  	}
>  
> +	if (is_write)
> +		tmp32 = *reg;
> +

I'm not a fan of this, from seeing that you do the read conversion
inside the case statements I gather you put this here so you only have
to have it once, even though you throw it away if you're doing 64-bit
accesses?

But a greater concern is the vgic_init() call above, which you don't
handle.

I thought we were supposed to get rid of all this lazy vgic init stuff.

Let me send you a patch series of how to rework this vgic_attr function
so that you can reuse some of the functionality and implement a new
gicv3 function on top of that.

Thanks,
-Christoffer

>  	switch (attr->group) {
>  	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
> -		ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, reg);
> +		ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, &tmp32);
> +		if (!is_write)
> +			*reg = tmp32;
>  		break;
>  	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> -		ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, reg);
> +		ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, &tmp32);
> +		if (!is_write)
> +			*reg = tmp32;


>  		break;
>  	default:
>  		ret = -EINVAL;
> @@ -309,11 +317,13 @@ static int vgic_v2_set_attr(struct kvm_device *dev,
>  	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;
> +		u32 tmp32;
> +		u64 reg;
>  
> -		if (get_user(reg, uaddr))
> +		if (get_user(tmp32, uaddr))
>  			return -EFAULT;
>  
> +		reg = tmp32;
>  		return vgic_attr_regs_access(dev, attr, &reg, true);
>  	}
>  	}
> @@ -334,12 +344,14 @@ static int vgic_v2_get_attr(struct kvm_device *dev,
>  	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;
> +		u32 tmp32;
> +		u64 reg;
>  
>  		ret = vgic_attr_regs_access(dev, attr, &reg, false);
>  		if (ret)
>  			return ret;
> -		return put_user(reg, uaddr);
> +		tmp32 = reg;
> +		return put_user(tmp32, uaddr);
>  	}
>  	}
>  
> -- 
> 1.9.1
> 

[RFC PATCH v2 1/4] arm/arm64: vgic-new: Introduce 64-bit reg access support

[Christoffer Dall]

On Tue, Aug 09, 2016 at 04:28:43PM +0530, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> 
> vgic_attr_regs_access() handles only 32-bit register
> value. Pass u64 as parameter and locally handle 32-bit
> reads and writes depending on attribute group.
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
>  virt/kvm/arm/vgic/vgic-kvm-device.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index 0130c4b..06de322 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -236,12 +236,13 @@ void kvm_register_vgic_device(unsigned long type)
>   */
>  static int vgic_attr_regs_access(struct kvm_device *dev,
>  				 struct kvm_device_attr *attr,
> -				 u32 *reg, bool is_write)
> +				 u64 *reg, bool is_write)
>  {
>  	gpa_t addr;
>  	int cpuid, ret, c;
>  	struct kvm_vcpu *vcpu, *tmp_vcpu;
>  	int vcpu_lock_idx = -1;
> +	u32 tmp32;
>  
>  	cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
>  		 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
> @@ -272,12 +273,19 @@ static int vgic_attr_regs_access(struct kvm_device *dev,
>  		vcpu_lock_idx = c;
>  	}
>  
> +	if (is_write)
> +		tmp32 = *reg;
> +

I'm not a fan of this, from seeing that you do the read conversion
inside the case statements I gather you put this here so you only have
to have it once, even though you throw it away if you're doing 64-bit
accesses?

But a greater concern is the vgic_init() call above, which you don't
handle.

I thought we were supposed to get rid of all this lazy vgic init stuff.

Let me send you a patch series of how to rework this vgic_attr function
so that you can reuse some of the functionality and implement a new
gicv3 function on top of that.

Thanks,
-Christoffer

>  	switch (attr->group) {
>  	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
> -		ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, reg);
> +		ret = vgic_v2_cpuif_uaccess(vcpu, is_write, addr, &tmp32);
> +		if (!is_write)
> +			*reg = tmp32;
>  		break;
>  	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> -		ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, reg);
> +		ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, &tmp32);
> +		if (!is_write)
> +			*reg = tmp32;


>  		break;
>  	default:
>  		ret = -EINVAL;
> @@ -309,11 +317,13 @@ static int vgic_v2_set_attr(struct kvm_device *dev,
>  	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;
> +		u32 tmp32;
> +		u64 reg;
>  
> -		if (get_user(reg, uaddr))
> +		if (get_user(tmp32, uaddr))
>  			return -EFAULT;
>  
> +		reg = tmp32;
>  		return vgic_attr_regs_access(dev, attr, &reg, true);
>  	}
>  	}
> @@ -334,12 +344,14 @@ static int vgic_v2_get_attr(struct kvm_device *dev,
>  	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;
> +		u32 tmp32;
> +		u64 reg;
>  
>  		ret = vgic_attr_regs_access(dev, attr, &reg, false);
>  		if (ret)
>  			return ret;
> -		return put_user(reg, uaddr);
> +		tmp32 = reg;
> +		return put_user(tmp32, uaddr);
>  	}
>  	}
>  
> -- 
> 1.9.1
> 

Re: [RFC PATCH v2 2/4] arm/arm64: vgic-new: Add distributor and redistributor access

[Christoffer Dall]

On Tue, Aug 09, 2016 at 04:28:44PM +0530, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> 
> VGICv3 Distributor and Redistributor registers are accessed using
> KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_DIST_REGS
> with KVM_SET_DEVICE_ATTR and KVM_GET_DEVICE_ATTR ioctls.
> These registers are accessed as 32-bit and cpu mpidr
> value passed along with register offset is used to identify the
> cpu for redistributor registers access.
> 
> The version of VGIC v3 specification is define here
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
>  arch/arm64/include/uapi/asm/kvm.h   |   3 +
>  virt/kvm/arm/vgic/vgic-kvm-device.c |  81 ++++++++++++++++++++++++--
>  virt/kvm/arm/vgic/vgic-mmio-v3.c    | 113 ++++++++++++++++++++++++++++++++++++
>  virt/kvm/arm/vgic/vgic-mmio.c       |   2 +-
>  virt/kvm/arm/vgic/vgic.h            |   8 +++
>  5 files changed, 200 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index f209ea1..a6b996e 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -199,10 +199,13 @@ struct kvm_arch_memory_slot {
>  #define KVM_DEV_ARM_VGIC_GRP_CPU_REGS	2
>  #define   KVM_DEV_ARM_VGIC_CPUID_SHIFT	32
>  #define   KVM_DEV_ARM_VGIC_CPUID_MASK	(0xffULL << KVM_DEV_ARM_VGIC_CPUID_SHIFT)
> +#define   KVM_DEV_ARM_VGIC_V3_CPUID_MASK \
> +				(0xffffffffULL << KVM_DEV_ARM_VGIC_CPUID_SHIFT)
>  #define   KVM_DEV_ARM_VGIC_OFFSET_SHIFT	0
>  #define   KVM_DEV_ARM_VGIC_OFFSET_MASK	(0xffffffffULL << KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
>  #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS	3
>  #define KVM_DEV_ARM_VGIC_GRP_CTRL	4
> +#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT	0
>  
>  /* Device Control API on vcpu fd */
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index 06de322..986f8e1 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -243,10 +243,19 @@ static int vgic_attr_regs_access(struct kvm_device *dev,
>  	struct kvm_vcpu *vcpu, *tmp_vcpu;
>  	int vcpu_lock_idx = -1;
>  	u32 tmp32;
> +	struct vgic_dist *vgic = &dev->kvm->arch.vgic;
>  
> -	cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
> -		 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
> -	vcpu = kvm_get_vcpu(dev->kvm, cpuid);
> +	if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2) {
> +		cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
> +			 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
> +		vcpu = kvm_get_vcpu(dev->kvm, cpuid);
> +	}
> +	else
> +	{

coding style issues

> +		cpuid = (attr->attr & KVM_DEV_ARM_VGIC_V3_CPUID_MASK) >>
> +			 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
> +		vcpu = kvm_mpidr_to_vcpu(dev->kvm, cpuid);
> +	}

regardless, this function is getting way too bloated.

>  	addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
>  
>  	mutex_lock(&dev->kvm->lock);
> @@ -283,10 +292,25 @@ static int vgic_attr_regs_access(struct kvm_device *dev,
>  			*reg = tmp32;
>  		break;
>  	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> -		ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, &tmp32);
> +		if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
> +			ret = vgic_v2_dist_uaccess(vcpu, is_write, addr,
> +						   &tmp32);
> +		else
> +			ret = vgic_v3_dist_uaccess(vcpu, is_write, addr,
> +						   &tmp32);
>  		if (!is_write)
>  			*reg = tmp32;
>  		break;
> +	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
> +		if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
> +			ret = vgic_v3_redist_uaccess(vcpu, is_write, addr,
> +						     &tmp32);
> +			if (!is_write)
> +				*reg = tmp32;
> +		} else {
> +			ret = -EINVAL;
> +		}
> +		break;
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -399,13 +423,55 @@ struct kvm_device_ops kvm_arm_vgic_v2_ops = {
>  static int vgic_v3_set_attr(struct kvm_device *dev,
>  			    struct kvm_device_attr *attr)
>  {
> -	return vgic_set_common_attr(dev, attr);
> +	int ret;
> +
> +	ret = vgic_set_common_attr(dev, attr);
> +	if (ret != -ENXIO)
> +		return ret;
> +

I think you need to check for (!vgic_initialized()) here, no?

> +	switch (attr->group) {
> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> +	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS: {
> +		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> +		u32 tmp32;
> +		u64 reg;
> +
> +		if (get_user(tmp32, uaddr))
> +			return -EFAULT;
> +
> +		reg = tmp32;
> +		return vgic_attr_regs_access(dev, attr, &reg, true);
> +	}
> +	}
> +	return -ENXIO;
>  }
>  
>  static int vgic_v3_get_attr(struct kvm_device *dev,
>  			    struct kvm_device_attr *attr)
>  {
> -	return vgic_get_common_attr(dev, attr);
> +	int ret;
> +
> +	ret = vgic_get_common_attr(dev, attr);
> +	if (ret != -ENXIO)
> +		return ret;

same?

> +
> +	switch (attr->group) {
> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> +	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS: {
> +		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> +		u64 reg;
> +		u32 tmp32;
> +
> +		ret = vgic_attr_regs_access(dev, attr, &reg, false);
> +		if (ret)
> +			return ret;
> +		tmp32 = reg;
> +		ret = put_user(tmp32, uaddr);
> +		return ret;
> +	}
> +	}
> +
> +	return -ENXIO;
>  }
>  
>  static int vgic_v3_has_attr(struct kvm_device *dev,
> @@ -419,6 +485,9 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
>  			return 0;
>  		}
>  		break;
> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> +	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
> +		return vgic_v3_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-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index a0c515a..36b4882 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -18,6 +18,8 @@
>  #include <kvm/arm_vgic.h>
>  
>  #include <asm/kvm_emulate.h>
> +#include <asm/kvm_arm.h>
> +#include <asm/kvm_mmu.h>
>  
>  #include "vgic.h"
>  #include "vgic-mmio.h"
> @@ -226,6 +228,9 @@ static const struct vgic_register_region vgic_v3_rdbase_registers[] = {
>  	REGISTER_DESC_WITH_LENGTH(GICR_TYPER,
>  		vgic_mmio_read_v3r_typer, vgic_mmio_write_wi, 8,
>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
> +	REGISTER_DESC_WITH_LENGTH(GICR_WAKER,
> +		vgic_mmio_read_raz, vgic_mmio_write_wi, 8,
> +		VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_LENGTH(GICR_PROPBASER,
>  		vgic_mmio_read_raz, vgic_mmio_write_wi, 8,
>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
> @@ -348,6 +353,52 @@ int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t redist_base_address)
>  	return ret;
>  }
>  
> +int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
> +{
> +	struct kvm_vcpu *vcpu;
> +	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, cpuid;
> +
> +	addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
> +	cpuid = (attr->attr & KVM_DEV_ARM_VGIC_V3_CPUID_MASK) >>
> +		 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
> +	vcpu = kvm_mpidr_to_vcpu(dev->kvm, cpuid);

this is also duplicating a lot of logic

> +
> +	switch (attr->group) {
> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> +		regions = vgic_v3_dist_registers;
> +		nr_regions = ARRAY_SIZE(vgic_v3_dist_registers);
> +		break;
> +	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:{
> +		struct vgic_io_device *devices;
> +		struct vgic_io_device *rd_dev;
> +
> +		devices = dev->kvm->arch.vgic.redist_iodevs;
> +		rd_dev = &devices[vcpu->vcpu_id * 2];
> +
> +		regions = rd_dev->regions;
> +		nr_regions = rd_dev->nr_regions;
> +		break;
> +	}
> +	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;
> +}
>  /*
>   * Compare a given affinity (level 1-3 and a level 0 mask, from the SGI
>   * generation register ICC_SGI1R_EL1) with a given VCPU.
> @@ -453,3 +504,65 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
>  		vgic_queue_irq_unlock(vcpu->kvm, irq);
>  	}
>  }
> +
> +/*
> + * When userland tries to access the VGIC register handlers, we need to
> + * create a usable struct vgic_io_device to be passed to the handlers and we
> + * have to set up a buffer similar to what would have happened if a guest MMIO
> + * access occurred, including doing endian conversions on BE systems.
> + */
> +static int vgic_v3_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
> +			   bool is_write, int offset, u32 *val)
> +{
> +	unsigned int len = 4;
> +	u8 buf[4];
> +	int ret;
> +
> +	if (is_write) {
> +		vgic_data_host_to_mmio_bus(buf, len, *val);
> +		ret = kvm_io_gic_ops.write(vcpu, &dev->dev,
> +					   dev->base_addr + offset, len, buf);
> +	} else {
> +		ret = kvm_io_gic_ops.read(vcpu, &dev->dev,
> +					  dev->base_addr + offset, len, buf);
> +		if (!ret)
> +			*val = vgic_data_mmio_bus_to_host(buf, len);
> +	}
> +
> +	return ret;
> +}
> +
> +int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> +			 int offset, u32 *val)
> +{
> +	struct vgic_io_device dev = {
> +		.regions = vgic_v3_dist_registers,
> +		.nr_regions = ARRAY_SIZE(vgic_v3_dist_registers),
> +	};
> +
> +	return vgic_v3_uaccess(vcpu, &dev, is_write, offset, val);
> +}
> +
> +int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> +			   int offset, u32 *val)
> +{
> +	struct vgic_io_device *dev;
> +	const struct vgic_register_region *region;
> +
> +	struct vgic_io_device rd_dev = {
> +		.regions = vgic_v3_rdbase_registers,
> +		.nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers),
> +	};
> +
> +	struct vgic_io_device sgi_dev = {
> +		.regions = vgic_v3_sgibase_registers,
> +		.nr_regions = ARRAY_SIZE(vgic_v3_sgibase_registers),
> +	};
> +
> +	dev = &sgi_dev;
> +	region = vgic_find_mmio_region(dev->regions, dev->nr_regions, offset);
> +	if (region == NULL)
> +		dev = &rd_dev;
> +
> +	return vgic_v3_uaccess(vcpu, dev, is_write, offset, val);
> +}
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index 9f6fab7..f583959 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -363,7 +363,7 @@ static int match_region(const void *key, const void *elt)
>  }
>  
>  /* Find the proper register handler entry given a certain address offset. */
> -static const struct vgic_register_region *
> +const struct vgic_register_region *
>  vgic_find_mmio_region(const struct vgic_register_region *region, int nr_regions,
>  		      unsigned int offset)
>  {
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index 7b300ca..8637690 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -59,6 +59,9 @@ int vgic_v2_map_resources(struct kvm *kvm);
>  int vgic_register_dist_iodev(struct kvm *kvm, gpa_t dist_base_address,
>  			     enum vgic_type);
>  
> +const struct vgic_register_region *
> +	vgic_find_mmio_region(const struct vgic_register_region *region,
> +			      int nr_regions, unsigned int offset);
>  #ifdef CONFIG_KVM_ARM_VGIC_V3
>  void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu);
>  void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu);
> @@ -71,6 +74,11 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu);
>  int vgic_v3_probe(const struct gic_kvm_info *info);
>  int vgic_v3_map_resources(struct kvm *kvm);
>  int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t dist_base_address);
> +int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr);
> +int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> +			 int offset, u32 *val);
> +int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> +			 int offset, u32 *val);
>  #else
>  static inline void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu)
>  {
> -- 
> 1.9.1
> 

[RFC PATCH v2 2/4] arm/arm64: vgic-new: Add distributor and redistributor access

[Christoffer Dall]

On Tue, Aug 09, 2016 at 04:28:44PM +0530, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> 
> VGICv3 Distributor and Redistributor registers are accessed using
> KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_DIST_REGS
> with KVM_SET_DEVICE_ATTR and KVM_GET_DEVICE_ATTR ioctls.
> These registers are accessed as 32-bit and cpu mpidr
> value passed along with register offset is used to identify the
> cpu for redistributor registers access.
> 
> The version of VGIC v3 specification is define here
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
>  arch/arm64/include/uapi/asm/kvm.h   |   3 +
>  virt/kvm/arm/vgic/vgic-kvm-device.c |  81 ++++++++++++++++++++++++--
>  virt/kvm/arm/vgic/vgic-mmio-v3.c    | 113 ++++++++++++++++++++++++++++++++++++
>  virt/kvm/arm/vgic/vgic-mmio.c       |   2 +-
>  virt/kvm/arm/vgic/vgic.h            |   8 +++
>  5 files changed, 200 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index f209ea1..a6b996e 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -199,10 +199,13 @@ struct kvm_arch_memory_slot {
>  #define KVM_DEV_ARM_VGIC_GRP_CPU_REGS	2
>  #define   KVM_DEV_ARM_VGIC_CPUID_SHIFT	32
>  #define   KVM_DEV_ARM_VGIC_CPUID_MASK	(0xffULL << KVM_DEV_ARM_VGIC_CPUID_SHIFT)
> +#define   KVM_DEV_ARM_VGIC_V3_CPUID_MASK \
> +				(0xffffffffULL << KVM_DEV_ARM_VGIC_CPUID_SHIFT)
>  #define   KVM_DEV_ARM_VGIC_OFFSET_SHIFT	0
>  #define   KVM_DEV_ARM_VGIC_OFFSET_MASK	(0xffffffffULL << KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
>  #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS	3
>  #define KVM_DEV_ARM_VGIC_GRP_CTRL	4
> +#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT	0
>  
>  /* Device Control API on vcpu fd */
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index 06de322..986f8e1 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -243,10 +243,19 @@ static int vgic_attr_regs_access(struct kvm_device *dev,
>  	struct kvm_vcpu *vcpu, *tmp_vcpu;
>  	int vcpu_lock_idx = -1;
>  	u32 tmp32;
> +	struct vgic_dist *vgic = &dev->kvm->arch.vgic;
>  
> -	cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
> -		 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
> -	vcpu = kvm_get_vcpu(dev->kvm, cpuid);
> +	if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2) {
> +		cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
> +			 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
> +		vcpu = kvm_get_vcpu(dev->kvm, cpuid);
> +	}
> +	else
> +	{

coding style issues

> +		cpuid = (attr->attr & KVM_DEV_ARM_VGIC_V3_CPUID_MASK) >>
> +			 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
> +		vcpu = kvm_mpidr_to_vcpu(dev->kvm, cpuid);
> +	}

regardless, this function is getting way too bloated.

>  	addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
>  
>  	mutex_lock(&dev->kvm->lock);
> @@ -283,10 +292,25 @@ static int vgic_attr_regs_access(struct kvm_device *dev,
>  			*reg = tmp32;
>  		break;
>  	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> -		ret = vgic_v2_dist_uaccess(vcpu, is_write, addr, &tmp32);
> +		if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V2)
> +			ret = vgic_v2_dist_uaccess(vcpu, is_write, addr,
> +						   &tmp32);
> +		else
> +			ret = vgic_v3_dist_uaccess(vcpu, is_write, addr,
> +						   &tmp32);
>  		if (!is_write)
>  			*reg = tmp32;
>  		break;
> +	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
> +		if (vgic->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
> +			ret = vgic_v3_redist_uaccess(vcpu, is_write, addr,
> +						     &tmp32);
> +			if (!is_write)
> +				*reg = tmp32;
> +		} else {
> +			ret = -EINVAL;
> +		}
> +		break;
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -399,13 +423,55 @@ struct kvm_device_ops kvm_arm_vgic_v2_ops = {
>  static int vgic_v3_set_attr(struct kvm_device *dev,
>  			    struct kvm_device_attr *attr)
>  {
> -	return vgic_set_common_attr(dev, attr);
> +	int ret;
> +
> +	ret = vgic_set_common_attr(dev, attr);
> +	if (ret != -ENXIO)
> +		return ret;
> +

I think you need to check for (!vgic_initialized()) here, no?

> +	switch (attr->group) {
> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> +	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS: {
> +		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> +		u32 tmp32;
> +		u64 reg;
> +
> +		if (get_user(tmp32, uaddr))
> +			return -EFAULT;
> +
> +		reg = tmp32;
> +		return vgic_attr_regs_access(dev, attr, &reg, true);
> +	}
> +	}
> +	return -ENXIO;
>  }
>  
>  static int vgic_v3_get_attr(struct kvm_device *dev,
>  			    struct kvm_device_attr *attr)
>  {
> -	return vgic_get_common_attr(dev, attr);
> +	int ret;
> +
> +	ret = vgic_get_common_attr(dev, attr);
> +	if (ret != -ENXIO)
> +		return ret;

same?

> +
> +	switch (attr->group) {
> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> +	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS: {
> +		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> +		u64 reg;
> +		u32 tmp32;
> +
> +		ret = vgic_attr_regs_access(dev, attr, &reg, false);
> +		if (ret)
> +			return ret;
> +		tmp32 = reg;
> +		ret = put_user(tmp32, uaddr);
> +		return ret;
> +	}
> +	}
> +
> +	return -ENXIO;
>  }
>  
>  static int vgic_v3_has_attr(struct kvm_device *dev,
> @@ -419,6 +485,9 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
>  			return 0;
>  		}
>  		break;
> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> +	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
> +		return vgic_v3_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-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index a0c515a..36b4882 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -18,6 +18,8 @@
>  #include <kvm/arm_vgic.h>
>  
>  #include <asm/kvm_emulate.h>
> +#include <asm/kvm_arm.h>
> +#include <asm/kvm_mmu.h>
>  
>  #include "vgic.h"
>  #include "vgic-mmio.h"
> @@ -226,6 +228,9 @@ static const struct vgic_register_region vgic_v3_rdbase_registers[] = {
>  	REGISTER_DESC_WITH_LENGTH(GICR_TYPER,
>  		vgic_mmio_read_v3r_typer, vgic_mmio_write_wi, 8,
>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
> +	REGISTER_DESC_WITH_LENGTH(GICR_WAKER,
> +		vgic_mmio_read_raz, vgic_mmio_write_wi, 8,
> +		VGIC_ACCESS_32bit),
>  	REGISTER_DESC_WITH_LENGTH(GICR_PROPBASER,
>  		vgic_mmio_read_raz, vgic_mmio_write_wi, 8,
>  		VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
> @@ -348,6 +353,52 @@ int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t redist_base_address)
>  	return ret;
>  }
>  
> +int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
> +{
> +	struct kvm_vcpu *vcpu;
> +	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, cpuid;
> +
> +	addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
> +	cpuid = (attr->attr & KVM_DEV_ARM_VGIC_V3_CPUID_MASK) >>
> +		 KVM_DEV_ARM_VGIC_CPUID_SHIFT;
> +	vcpu = kvm_mpidr_to_vcpu(dev->kvm, cpuid);

this is also duplicating a lot of logic

> +
> +	switch (attr->group) {
> +	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> +		regions = vgic_v3_dist_registers;
> +		nr_regions = ARRAY_SIZE(vgic_v3_dist_registers);
> +		break;
> +	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:{
> +		struct vgic_io_device *devices;
> +		struct vgic_io_device *rd_dev;
> +
> +		devices = dev->kvm->arch.vgic.redist_iodevs;
> +		rd_dev = &devices[vcpu->vcpu_id * 2];
> +
> +		regions = rd_dev->regions;
> +		nr_regions = rd_dev->nr_regions;
> +		break;
> +	}
> +	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;
> +}
>  /*
>   * Compare a given affinity (level 1-3 and a level 0 mask, from the SGI
>   * generation register ICC_SGI1R_EL1) with a given VCPU.
> @@ -453,3 +504,65 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
>  		vgic_queue_irq_unlock(vcpu->kvm, irq);
>  	}
>  }
> +
> +/*
> + * When userland tries to access the VGIC register handlers, we need to
> + * create a usable struct vgic_io_device to be passed to the handlers and we
> + * have to set up a buffer similar to what would have happened if a guest MMIO
> + * access occurred, including doing endian conversions on BE systems.
> + */
> +static int vgic_v3_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
> +			   bool is_write, int offset, u32 *val)
> +{
> +	unsigned int len = 4;
> +	u8 buf[4];
> +	int ret;
> +
> +	if (is_write) {
> +		vgic_data_host_to_mmio_bus(buf, len, *val);
> +		ret = kvm_io_gic_ops.write(vcpu, &dev->dev,
> +					   dev->base_addr + offset, len, buf);
> +	} else {
> +		ret = kvm_io_gic_ops.read(vcpu, &dev->dev,
> +					  dev->base_addr + offset, len, buf);
> +		if (!ret)
> +			*val = vgic_data_mmio_bus_to_host(buf, len);
> +	}
> +
> +	return ret;
> +}
> +
> +int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> +			 int offset, u32 *val)
> +{
> +	struct vgic_io_device dev = {
> +		.regions = vgic_v3_dist_registers,
> +		.nr_regions = ARRAY_SIZE(vgic_v3_dist_registers),
> +	};
> +
> +	return vgic_v3_uaccess(vcpu, &dev, is_write, offset, val);
> +}
> +
> +int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> +			   int offset, u32 *val)
> +{
> +	struct vgic_io_device *dev;
> +	const struct vgic_register_region *region;
> +
> +	struct vgic_io_device rd_dev = {
> +		.regions = vgic_v3_rdbase_registers,
> +		.nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers),
> +	};
> +
> +	struct vgic_io_device sgi_dev = {
> +		.regions = vgic_v3_sgibase_registers,
> +		.nr_regions = ARRAY_SIZE(vgic_v3_sgibase_registers),
> +	};
> +
> +	dev = &sgi_dev;
> +	region = vgic_find_mmio_region(dev->regions, dev->nr_regions, offset);
> +	if (region == NULL)
> +		dev = &rd_dev;
> +
> +	return vgic_v3_uaccess(vcpu, dev, is_write, offset, val);
> +}
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index 9f6fab7..f583959 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -363,7 +363,7 @@ static int match_region(const void *key, const void *elt)
>  }
>  
>  /* Find the proper register handler entry given a certain address offset. */
> -static const struct vgic_register_region *
> +const struct vgic_register_region *
>  vgic_find_mmio_region(const struct vgic_register_region *region, int nr_regions,
>  		      unsigned int offset)
>  {
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index 7b300ca..8637690 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -59,6 +59,9 @@ int vgic_v2_map_resources(struct kvm *kvm);
>  int vgic_register_dist_iodev(struct kvm *kvm, gpa_t dist_base_address,
>  			     enum vgic_type);
>  
> +const struct vgic_register_region *
> +	vgic_find_mmio_region(const struct vgic_register_region *region,
> +			      int nr_regions, unsigned int offset);
>  #ifdef CONFIG_KVM_ARM_VGIC_V3
>  void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu);
>  void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu);
> @@ -71,6 +74,11 @@ void vgic_v3_enable(struct kvm_vcpu *vcpu);
>  int vgic_v3_probe(const struct gic_kvm_info *info);
>  int vgic_v3_map_resources(struct kvm *kvm);
>  int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t dist_base_address);
> +int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr);
> +int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> +			 int offset, u32 *val);
> +int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> +			 int offset, u32 *val);
>  #else
>  static inline void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu)
>  {
> -- 
> 1.9.1
> 

Re: [RFC PATCH v2 0/4] arm/arm64: vgic-new: Implement API for vGICv3 live migration

[Christoffer Dall]

Hi Vijaya,

On Tue, Aug 09, 2016 at 04:28:42PM +0530, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> 
> This patchset adds API for saving and restoring
> of VGICv3 registers to support live migration with new vgic feature.
> This API definition is as per version of VGICv3 specification
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
> 
> To test live migration with QEMU, use below patch series
> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01444.html
> 
> The patch 3 & 4 are picked from the Pavel's previous implementation.
> http://www.spinics.net/lists/kvm/msg122040.html
> 
> v1 => v2:
>  - The init sequence change patch is no more required.
>    Fixed in patch 2 by using static vgic_io_dev regions structure instead
>    of using dynamic allocation pointer.
>  - Updated commit message of patch 4.
>  - Dropped usage of union to manage 32-bit and 64-bit access in patch 1.
>    Used local variable for 32-bit access.
>  - Updated macro __ARM64_SYS_REG and ARM64_SYS_REG in 
>    arch/arm64/include/uapi/asm/kvm.h as per qemu requirements.
> 
I think you should have enough to go on by now for a new revision.  You
can include the two patches I just sent with the series '[PATCH v2]
Rework vgic_attr_regs_access' in the beginning of your series and then
base your work on that, assuming nobody else screams about this.

If you use that approach, and you address the missing soft pending state
etc. that Peter pointed out, then we should be ready for a detailed
review.

Looking forward to the next version.

Thanks,
-Christoffer

[RFC PATCH v2 0/4] arm/arm64: vgic-new: Implement API for vGICv3 live migration

[Christoffer Dall]

Hi Vijaya,

On Tue, Aug 09, 2016 at 04:28:42PM +0530, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> 
> This patchset adds API for saving and restoring
> of VGICv3 registers to support live migration with new vgic feature.
> This API definition is as per version of VGICv3 specification
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
> 
> To test live migration with QEMU, use below patch series
> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01444.html
> 
> The patch 3 & 4 are picked from the Pavel's previous implementation.
> http://www.spinics.net/lists/kvm/msg122040.html
> 
> v1 => v2:
>  - The init sequence change patch is no more required.
>    Fixed in patch 2 by using static vgic_io_dev regions structure instead
>    of using dynamic allocation pointer.
>  - Updated commit message of patch 4.
>  - Dropped usage of union to manage 32-bit and 64-bit access in patch 1.
>    Used local variable for 32-bit access.
>  - Updated macro __ARM64_SYS_REG and ARM64_SYS_REG in 
>    arch/arm64/include/uapi/asm/kvm.h as per qemu requirements.
> 
I think you should have enough to go on by now for a new revision.  You
can include the two patches I just sent with the series '[PATCH v2]
Rework vgic_attr_regs_access' in the beginning of your series and then
base your work on that, assuming nobody else screams about this.

If you use that approach, and you address the missing soft pending state
etc. that Peter pointed out, then we should be ready for a detailed
review.

Looking forward to the next version.

Thanks,
-Christoffer

Re: [RFC PATCH v2 0/4] arm/arm64: vgic-new: Implement API for vGICv3 live migration

[Christoffer Dall]

On Tue, Aug 16, 2016 at 07:37:47PM +0200, Christoffer Dall wrote:
> Hi Vijaya,
> 
> On Tue, Aug 09, 2016 at 04:28:42PM +0530, vijay.kilari@gmail.com wrote:
> > From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> > 
> > This patchset adds API for saving and restoring
> > of VGICv3 registers to support live migration with new vgic feature.
> > This API definition is as per version of VGICv3 specification
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
> > 
> > To test live migration with QEMU, use below patch series
> > https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01444.html
> > 
> > The patch 3 & 4 are picked from the Pavel's previous implementation.
> > http://www.spinics.net/lists/kvm/msg122040.html
> > 
> > v1 => v2:
> >  - The init sequence change patch is no more required.
> >    Fixed in patch 2 by using static vgic_io_dev regions structure instead
> >    of using dynamic allocation pointer.
> >  - Updated commit message of patch 4.
> >  - Dropped usage of union to manage 32-bit and 64-bit access in patch 1.
> >    Used local variable for 32-bit access.
> >  - Updated macro __ARM64_SYS_REG and ARM64_SYS_REG in 
> >    arch/arm64/include/uapi/asm/kvm.h as per qemu requirements.
> > 
> I think you should have enough to go on by now for a new revision.  You
> can include the two patches I just sent with the series '[PATCH v2]
> Rework vgic_attr_regs_access' in the beginning of your series and then
> base your work on that, assuming nobody else screams about this.
> 
> If you use that approach, and you address the missing soft pending state
> etc. that Peter pointed out, then we should be ready for a detailed
> review.
> 
> Looking forward to the next version.
> 
FYI: I pushed the API doc and the refactoring patches to the 'queue'
branch of the kvmarm tree:

git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git

It would be good if you could base your patches on there for the next
revision.

Thanks,
-Christoffer

[RFC PATCH v2 0/4] arm/arm64: vgic-new: Implement API for vGICv3 live migration

[Christoffer Dall]

On Tue, Aug 16, 2016 at 07:37:47PM +0200, Christoffer Dall wrote:
> Hi Vijaya,
> 
> On Tue, Aug 09, 2016 at 04:28:42PM +0530, vijay.kilari at gmail.com wrote:
> > From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> > 
> > This patchset adds API for saving and restoring
> > of VGICv3 registers to support live migration with new vgic feature.
> > This API definition is as per version of VGICv3 specification
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
> > 
> > To test live migration with QEMU, use below patch series
> > https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01444.html
> > 
> > The patch 3 & 4 are picked from the Pavel's previous implementation.
> > http://www.spinics.net/lists/kvm/msg122040.html
> > 
> > v1 => v2:
> >  - The init sequence change patch is no more required.
> >    Fixed in patch 2 by using static vgic_io_dev regions structure instead
> >    of using dynamic allocation pointer.
> >  - Updated commit message of patch 4.
> >  - Dropped usage of union to manage 32-bit and 64-bit access in patch 1.
> >    Used local variable for 32-bit access.
> >  - Updated macro __ARM64_SYS_REG and ARM64_SYS_REG in 
> >    arch/arm64/include/uapi/asm/kvm.h as per qemu requirements.
> > 
> I think you should have enough to go on by now for a new revision.  You
> can include the two patches I just sent with the series '[PATCH v2]
> Rework vgic_attr_regs_access' in the beginning of your series and then
> base your work on that, assuming nobody else screams about this.
> 
> If you use that approach, and you address the missing soft pending state
> etc. that Peter pointed out, then we should be ready for a detailed
> review.
> 
> Looking forward to the next version.
> 
FYI: I pushed the API doc and the refactoring patches to the 'queue'
branch of the kvmarm tree:

git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git

It would be good if you could base your patches on there for the next
revision.

Thanks,
-Christoffer

Re: [RFC PATCH v2 0/4] arm/arm64: vgic-new: Implement API for vGICv3 live migration

[Vijay Kilari]

On Thu, Aug 11, 2016 at 1:15 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 11 August 2016 at 06:29, Vijay Kilari <vijay.kilari@gmail.com> wrote:
>> On Tue, Aug 9, 2016 at 5:22 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 9 August 2016 at 11:58,  <vijay.kilari@gmail.com> wrote:
>>>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>>>
>>>> This patchset adds API for saving and restoring
>>>> of VGICv3 registers to support live migration with new vgic feature.
>>>> This API definition is as per version of VGICv3 specification
>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>>>>
>>>> To test live migration with QEMU, use below patch series
>>>> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01444.html
>>>>
>>>> The patch 3 & 4 are picked from the Pavel's previous implementation.
>>>> http://www.spinics.net/lists/kvm/msg122040.html
>>>>
>>>> v1 => v2:
>>>>  - The init sequence change patch is no more required.
>>>>    Fixed in patch 2 by using static vgic_io_dev regions structure instead
>>>>    of using dynamic allocation pointer.
>>>>  - Updated commit message of patch 4.
>>>>  - Dropped usage of union to manage 32-bit and 64-bit access in patch 1.
>>>>    Used local variable for 32-bit access.
>>>>  - Updated macro __ARM64_SYS_REG and ARM64_SYS_REG in
>>>>    arch/arm64/include/uapi/asm/kvm.h as per qemu requirements.
>>>
>>> I only scanned briefly through this patchset, but I didn't
>>> see any code implementing:
>>>  * KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO
>>
>> If irq->pending is updated by kernel based on irq->line_level when interrupt
>> is asserted by device or guest. Do we still need to extract
>> irq->line_level using
>> this ioctl and while writing back GIC{D|R}_ISPENDR with line_level
>> +(OR) GIC{D|R}_ISPENDR?
>
> The level and the pending status are different things;
> the API docs have an explanation of this. The API access
> to the ISPENDR registers should return only the pending
> latch status (which is not the same as what these registers
> return if you read them from the guest).
>

OK. I have implemented separate api for ISPENDR userspace access to
read soft_pending for level triggered interrupts. This needs kernel
implementation
to support separate api's for guest and userspace access.

>>>  * the different behaviour for accesses to GICD_STATUSR, GICR_STATUSR,
>>
>> QEMU is saving and restoring this register, but kernel implementation
>> is missing. Kernel is silently returning zero. So could not catch. I
>> will fix it.
>>
>> However, Specification says as below for STATUSR.
>>
>> "    The GICD_STATUSR and GICR_STATUSR registers are architecturally
>> defined such
>>      that a write of a clear bit has no effect, whereas a write with a set bit
>>      clears that value.  To allow userspace to freely set the values
>> of these two
>>      registers, setting the attributes with the register offsets for these two
>>      registers simply sets the non-reserved bits to the value written."
>>
>> Question is during restore, the set bit will clear the value STATUSR.
>> So it will reset the STATUSR after migrating the VM.
>
> The text you quote above says that setting the attribute via
> the API "sets the non-reserved bits to the value written".
> This is the point -- it does not have the write-1-to-clear
> behaviour that a guest access to the register does.
>

 In the current implementation of vgic in kernel I could not find
any implement/support for GICD_STATUSR register value.
Should I leave this as RAZ / WI for now?.

[RFC PATCH v2 0/4] arm/arm64: vgic-new: Implement API for vGICv3 live migration

[Vijay Kilari]

On Thu, Aug 11, 2016 at 1:15 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 11 August 2016 at 06:29, Vijay Kilari <vijay.kilari@gmail.com> wrote:
>> On Tue, Aug 9, 2016 at 5:22 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 9 August 2016 at 11:58,  <vijay.kilari@gmail.com> wrote:
>>>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>>>>
>>>> This patchset adds API for saving and restoring
>>>> of VGICv3 registers to support live migration with new vgic feature.
>>>> This API definition is as per version of VGICv3 specification
>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>>>>
>>>> To test live migration with QEMU, use below patch series
>>>> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg01444.html
>>>>
>>>> The patch 3 & 4 are picked from the Pavel's previous implementation.
>>>> http://www.spinics.net/lists/kvm/msg122040.html
>>>>
>>>> v1 => v2:
>>>>  - The init sequence change patch is no more required.
>>>>    Fixed in patch 2 by using static vgic_io_dev regions structure instead
>>>>    of using dynamic allocation pointer.
>>>>  - Updated commit message of patch 4.
>>>>  - Dropped usage of union to manage 32-bit and 64-bit access in patch 1.
>>>>    Used local variable for 32-bit access.
>>>>  - Updated macro __ARM64_SYS_REG and ARM64_SYS_REG in
>>>>    arch/arm64/include/uapi/asm/kvm.h as per qemu requirements.
>>>
>>> I only scanned briefly through this patchset, but I didn't
>>> see any code implementing:
>>>  * KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO
>>
>> If irq->pending is updated by kernel based on irq->line_level when interrupt
>> is asserted by device or guest. Do we still need to extract
>> irq->line_level using
>> this ioctl and while writing back GIC{D|R}_ISPENDR with line_level
>> +(OR) GIC{D|R}_ISPENDR?
>
> The level and the pending status are different things;
> the API docs have an explanation of this. The API access
> to the ISPENDR registers should return only the pending
> latch status (which is not the same as what these registers
> return if you read them from the guest).
>

OK. I have implemented separate api for ISPENDR userspace access to
read soft_pending for level triggered interrupts. This needs kernel
implementation
to support separate api's for guest and userspace access.

>>>  * the different behaviour for accesses to GICD_STATUSR, GICR_STATUSR,
>>
>> QEMU is saving and restoring this register, but kernel implementation
>> is missing. Kernel is silently returning zero. So could not catch. I
>> will fix it.
>>
>> However, Specification says as below for STATUSR.
>>
>> "    The GICD_STATUSR and GICR_STATUSR registers are architecturally
>> defined such
>>      that a write of a clear bit has no effect, whereas a write with a set bit
>>      clears that value.  To allow userspace to freely set the values
>> of these two
>>      registers, setting the attributes with the register offsets for these two
>>      registers simply sets the non-reserved bits to the value written."
>>
>> Question is during restore, the set bit will clear the value STATUSR.
>> So it will reset the STATUSR after migrating the VM.
>
> The text you quote above says that setting the attribute via
> the API "sets the non-reserved bits to the value written".
> This is the point -- it does not have the write-1-to-clear
> behaviour that a guest access to the register does.
>

 In the current implementation of vgic in kernel I could not find
any implement/support for GICD_STATUSR register value.
Should I leave this as RAZ / WI for now?.