Skip to contentSkip to conversation

KVM: arm64: Implement API for vGICv3 live migration

PATCH Superseded Pavel Fedin 4 participants 19 messages Aug 28, 2015 – Sep 1, 2015

Subsystems (4)
KVM arm64 KVM arm64 Arm generic interrupt controller drivers

v1 of 7 Superseded Latest is v7 →

lore
Share & tools
lore

Pavel Fedin

author
#0 lore raw

cover KVM: arm64: Implement API for vGICv3 live migration

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

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

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

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

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

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

Signed-off-by Pavel Fedin <[email protected]>
---
 arch/arm64/include/uapi/asm/kvm.h |   1 +
 virt/kvm/arm/vgic-v3-emul.c       | 186 +++++++++++++++++++++++++++++++++++---
 2 files changed, 172 insertions(+), 15 deletions(-)
arch/arm64/include/uapi/asm/kvm.h +1 -0
index 0cd7b59..2936651 100644
@@ -203,6 +203,7 @@ struct kvm_arch_memory_slot {
 #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS	3
 #define KVM_DEV_ARM_VGIC_GRP_CTRL	4
 #define   KVM_DEV_ARM_VGIC_CTRL_INIT	0
+#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
 
 /* KVM_IRQ_LINE irq field index values */
 #define KVM_ARM_IRQ_TYPE_SHIFT		24
virt/kvm/arm/vgic-v3-emul.c +171 -15

231 lines not on this page · raw message

Signature · 1 line
2.4.4
#2 lore raw

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

Quoted 37 lines · Pavel Fedin

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

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

Signed-off-by Pavel Fedin <[email protected]>
---
 arch/arm64/include/uapi/asm/kvm.h |   1 +
 virt/kvm/arm/vgic-v3-emul.c       | 186 +++++++++++++++++++++++++++++++++++---
 2 files changed, 172 insertions(+), 15 deletions(-)
arch/arm64/include/uapi/asm/kvm.h +1 -0
index 0cd7b59..2936651 100644
@@ -203,6 +203,7 @@ struct kvm_arch_memory_slot {
 #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS	3
 #define KVM_DEV_ARM_VGIC_GRP_CTRL	4
 #define   KVM_DEV_ARM_VGIC_CTRL_INIT	0
+#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
 
 /* KVM_IRQ_LINE irq field index values */
 #define KVM_ARM_IRQ_TYPE_SHIFT		24
virt/kvm/arm/vgic-v3-emul.c +1 -0
index e661e7f..b3847e1 100644
@@ -39,6 +39,7 @@
 #include <linux/kvm.h>
 #include <linux/kvm_host.h>
 #include <linux/interrupt.h>
+#include <linux/uaccess.h>
 
 #include <linux/irqchip/arm-gic-v3.h>
 #include <kvm/arm_vgic.h>

virt/kvm/arm/vgic-v3-emul.c void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg) · line 996

Quoted 6 lines
virt/kvm/arm/vgic-v3-emul.c +3 -0
@@ -990,6 +991,107 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
 		vgic_kick_vcpus(vcpu->kvm);
 }
 
+static int vgic_v3_attr_regs_access(struct kvm_device *dev,
+				 struct kvm_device_attr *attr,
+				 void *reg, u32 len, bool is_write)

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

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

virt/kvm/arm/vgic-v3-emul.c void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg) · line 1092

Quoted 10 lines
virt/kvm/arm/vgic-v3-emul.c +10 -0
+		else
+			*(u32 *)reg = mmio_data_read(&mmio, ~0);
+	}
+
+	ret = 0;
+out_vgic_unlock:
+	spin_unlock(&vgic->lock);
+out:
+	mutex_unlock(&dev->kvm->lock);
+	return ret;

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

Quoted 7 lines
virt/kvm/arm/vgic-v3-emul.c +2 -0
+}
+
 static int vgic_v3_create(struct kvm_device *dev, u32 type)
 {
 	return kvm_vgic_create(dev->kvm, type);

@@ -1000,40 +1102,95 @@ static void vgic_v3_destroy(struct kvm_device *dev)
 	kfree(dev);

virt/kvm/arm/vgic-v3-emul.c static void vgic_v3_destroy(struct kvm_device *dev) · line 1112

Quoted 10 lines
virt/kvm/arm/vgic-v3-emul.c +8 -0
 }
 
+static u32 vgic_v3_get_reg_size(struct kvm_device_attr *attr)
+{
+	u32 offset;
+
+	switch (attr->group) {
+	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
+		offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
+		if (offset >= GICD_IROUTER && offset <= 0x7FD8)

eh, 0x7FD8 ?

Quoted 8 lines
virt/kvm/arm/vgic-v3-emul.c +8 -0
+			return 8;
+		else
+			return 4;
+		break;
+
+	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
+		offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
+		if ((offset == GICR_TYPER) ||

virt/kvm/arm/vgic-v3-emul.c static void vgic_v3_destroy(struct kvm_device *dev) · line 1130

Quoted 10 lines
virt/kvm/arm/vgic-v3-emul.c +10 -0
+		    (offset >= GICR_SETLPIR && offset <= GICR_INVALLR))
+			return 8;
+		else
+			return 4;
+		break;
+
+	default:
+		return -ENXIO;
+	}
+}

this feels wrong.

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

Quoted 54 lines
virt/kvm/arm/vgic-v3-emul.c +25 -9
+
 static int vgic_v3_set_attr(struct kvm_device *dev,
 			    struct kvm_device_attr *attr)
 {
-	int ret;
+	int ret, len;
+	u64 reg64;
+	u32 reg;
+	void *data;
 
 	ret = vgic_set_common_attr(dev, attr);
 	if (ret != -ENXIO)
 		return ret;
 
-	switch (attr->group) {
-	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
-	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
-		return -ENXIO;
+	len = vgic_v3_get_reg_size(attr);
+	if (len < 0)
+		return len;
+
+	if (len == 8) {
+		u64 __user *uaddr = (u64 __user *)(long)attr->addr;
+
+		ret = get_user(reg64, uaddr);
+		data = &reg64;
+	} else {
+		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
+
+		ret = get_user(reg, uaddr);
+		data = &reg;
 	}
+	if (ret)
+		return -EFAULT;
 
-	return -ENXIO;
+	return vgic_v3_attr_regs_access(dev, attr, data, len, true);
 }
 
 static int vgic_v3_get_attr(struct kvm_device *dev,
 			    struct kvm_device_attr *attr)
 {
-	int ret;
+	int ret, len;
+	u64 reg64;
+	u32 reg;
 
 	ret = vgic_get_common_attr(dev, attr);
 	if (ret != -ENXIO)
 		return ret;
 
-	switch (attr->group) {
-	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:

virt/kvm/arm/vgic-v3-emul.c static void vgic_v3_destroy(struct kvm_device *dev) · line 1181

Quoted 10 lines
virt/kvm/arm/vgic-v3-emul.c +5 -4
-	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
-		return -ENXIO;
-	}
+	len = vgic_v3_get_reg_size(attr);
+	if (len < 0)
+		return len;
 
-	return -ENXIO;
+	ret = vgic_v3_attr_regs_access(dev, attr, (len == 8) ? (void *)&reg64 :
+					(void *)&reg, len, false);

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

Quoted 28 lines
virt/kvm/arm/vgic-v3-emul.c +13 -2
+	if (ret)
+		return ret;
+
+	if (len == 8) {
+		u64 __user *uaddr = (u64 __user *)(long)attr->addr;
+
+		return put_user(reg64, uaddr);
+	} else {
+		u32 __user *uaddr = (u32 __user *)(long)attr->addr;
+
+		return put_user(reg, uaddr);
+	}
 }
 
 static int vgic_v3_has_attr(struct kvm_device *dev,

@@ -1051,8 +1208,7 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
 		}
 		break;
 	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
-	case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
-		return -ENXIO;
+	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
 	case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
 		return 0;
 	case KVM_DEV_ARM_VGIC_GRP_CTRL:
Signature · 1 line
2.4.4
author
#3 lore raw

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

Hello!

Quoted 10 lines

virt/kvm/arm/vgic-v3-emul.c static void vgic_v3_destroy(struct kvm_device *dev) · line 1181

Quoted 7 lines
virt/kvm/arm/vgic-v3-emul.c +5 -1
+	len = vgic_v3_get_reg_size(attr);
+	if (len < 0)
+		return len;
 
-	return -ENXIO;
+	ret = vgic_v3_attr_regs_access(dev, attr, (len == 8) ? (void *)&reg64 :
+					(void *)&reg, len, false);

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

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

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

#4 lore raw

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

Quoted 13 lines · Pavel Fedin

Hello!

Quoted 10 lines

virt/kvm/arm/vgic-v3-emul.c static void vgic_v3_destroy(struct kvm_device *dev) · line 1181

Quoted 7 lines
virt/kvm/arm/vgic-v3-emul.c +5 -1
+	len = vgic_v3_get_reg_size(attr);
+	if (len < 0)
+		return len;
 
-	return -ENXIO;
+	ret = vgic_v3_attr_regs_access(dev, attr, (len == 8) ? (void *)&reg64 :
+					(void *)&reg, len, false);

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

Quoted 7 lines

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

Sounds fine to me, definitely if you must do a cast doing it in a function between typed variables is strictly preferred to passing void * values between functions.

-Christoffer

#5 lore raw

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

In reply to Christoffer Dall

Hi Pavel,

...

Quoted 4 lines
Quoted 4 lines
virt/kvm/arm/vgic-v3-emul.c +0 -0
index e661e7f..b3847e1 100644

...

Quoted 27 lines

virt/kvm/arm/vgic-v3-emul.c static void vgic_v3_destroy(struct kvm_device *dev) · line 1112

Quoted 12 lines
virt/kvm/arm/vgic-v3-emul.c +8 -0
@@ -1000,40 +1102,95 @@ static void vgic_v3_destroy(struct kvm_device *dev)
 	kfree(dev);
 }
 
+static u32 vgic_v3_get_reg_size(struct kvm_device_attr *attr)
+{
+	u32 offset;
+
+	switch (attr->group) {
+	case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
+		offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
+		if (offset >= GICD_IROUTER && offset <= 0x7FD8)

eh, 0x7FD8 ?

Quoted 12 lines
virt/kvm/arm/vgic-v3-emul.c +12 -0
+			return 8;
+		else
+			return 4;
+		break;
+
+	case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
+		offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
+		if ((offset == GICR_TYPER) ||
+		    (offset >= GICR_SETLPIR && offset <= GICR_INVALLR))
+			return 8;
+		else
+			return 4;
Quoted 8 lines

virt/kvm/arm/vgic-v3-emul.c static void vgic_v3_destroy(struct kvm_device *dev) · line 1130

Quoted 6 lines
virt/kvm/arm/vgic-v3-emul.c +6 -0
+		break;
+
+	default:
+		return -ENXIO;
+	}
+}

this feels wrong.

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

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

Cheers,
Andre.

author
#6 lore raw

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

Hello!

Quoted 6 lines

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

Ok, i will implement it this way.

Quoted 3 lines

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

Right.

Quoted 3 lines

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

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

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

#7 lore raw

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

In reply to Andre Przywara
Quoted 7 lines · Andre Przywara

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

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

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

thanks
-- PMM

author
#8 lore raw

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

In reply to Pavel Fedin

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

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

Signed-off-by Pavel Fedin <[email protected]>
---
 arch/arm64/kvm/sys_regs.c | 47 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 41 insertions(+), 6 deletions(-)
arch/arm64/kvm/sys_regs.c +41 -6
index b41607d..8cc4a5e 100644
@@ -1306,10 +1306,6 @@ static const struct sys_reg_desc *index_to_sys_reg_desc(struct kvm_vcpu *vcpu,
 	if (!r)
 		r = find_reg(&params, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
 
-	/* Not saved in the sys_reg array? */
-	if (r && !r->reg)
-		r = NULL;
-
 	return r;
 }
 

@@ -1533,6 +1529,7 @@ int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
 {
 	const struct sys_reg_desc *r;
 	void __user *uaddr = (void __user *)(unsigned long)reg->addr;
+	u64 val;
 
 	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
 		return demux_c15_get(reg->id, uaddr);

@@ -1547,13 +1544,31 @@ int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
 	if (r->get_user)
 		return (r->get_user)(vcpu, r, reg, uaddr);
 
-	return reg_to_user(uaddr, &vcpu_sys_reg(vcpu, r->reg), reg->id);
+	if (r->reg) {
+		val = vcpu_sys_reg(vcpu, r->reg);
+	} else {
+		struct sys_reg_params params;
+
+		if (!index_to_params(reg->id, &params))
+			return -ENOENT;
+
+		params.is_write = false;
+		params.is_aarch32 = false;
+		params.is_32bit = false;
+
+		if (!r->access(vcpu, &params, r))
+			return -EINVAL;
+	}
+
+	return reg_to_user(uaddr, &val, reg->id);
 }
 
 int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
 	const struct sys_reg_desc *r;
 	void __user *uaddr = (void __user *)(unsigned long)reg->addr;
+	u64 val;
+	int ret;
 
 	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
 		return demux_c15_set(reg->id, uaddr);

@@ -1568,7 +1583,27 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
 	if (r->set_user)
 		return (r->set_user)(vcpu, r, reg, uaddr);
 
-	return reg_from_user(&vcpu_sys_reg(vcpu, r->reg), uaddr, reg->id);
+	ret = reg_from_user(&val, uaddr, reg->id);
+	if (ret)
+		return ret;
+
+	if (r->reg) {
+		vcpu_sys_reg(vcpu, r->reg) = val;
+	} else {
+		struct sys_reg_params params;
+
+		if (!index_to_params(reg->id, &params))
+			return -ENOENT;
+
+		params.is_write = true;
+		params.is_aarch32 = false;
+		params.is_32bit = false;
+
+		if (!r->access(vcpu, &params, r))
+			return -EINVAL;
+	}
+
+	return 0;
 }
 
 static unsigned int num_demux_regs(void)
Signature · 1 line
2.4.4
author
#9 lore raw

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

In reply to Pavel Fedin

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

Signed-off-by Pavel Fedin <[email protected]>
---
 arch/arm64/kvm/sys_regs.c          | 176 +++++++++++++++++++++++++++++++++++++
 include/linux/irqchip/arm-gic-v3.h |  18 +++-
 2 files changed, 192 insertions(+), 2 deletions(-)
arch/arm64/kvm/sys_regs.c +176 -0
index 8cc4a5e..7a4f982 100644
@@ -23,6 +23,7 @@
 #include <linux/kvm_host.h>
 #include <linux/mm.h>
 #include <linux/uaccess.h>
+#include <linux/irqchip/arm-gic-v3.h>
 
 #include <asm/cacheflush.h>
 #include <asm/cputype.h>

@@ -136,6 +137,162 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+static bool access_gic_ctlr(struct kvm_vcpu *vcpu,
+			    const struct sys_reg_params *p,
+			    const struct sys_reg_desc *r)
+{
+	u64 val;
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
+		return false;
+
+	if (p->is_write) {
+		val = *vcpu_reg(vcpu, p->Rt);
+
+		vgicv3->vgic_vmcr &= ~(ICH_VMCR_CBPR|ICH_VMCR_EOIM);
+		vgicv3->vgic_vmcr |= (val << (ICH_VMCR_CBPR_SHIFT -
+						ICC_CTLR_EL1_CBPR_SHIFT)) &
+					ICH_VMCR_CBPR;
+		vgicv3->vgic_vmcr |= (val << (ICH_VMCR_EOIM_SHIFT -
+						ICC_CTLR_EL1_EOImode_SHIFT)) &
+					ICH_VMCR_EOIM;
+	} else {
+		asm volatile("mrs_s %0," __stringify(ICC_IAR1_EL1)
+			     : "=r" (val));
+		val &= (ICC_CTLR_EL1_A3V | ICC_CTLR_EL1_SEIS |
+			ICC_CTLR_EL1_IDbits_MASK | ICC_CTLR_EL1_PRIbits_MASK);
+		val |= (vgicv3->vgic_vmcr & ICH_VMCR_CBPR) >>
+			(ICH_VMCR_CBPR_SHIFT - ICC_CTLR_EL1_CBPR_SHIFT);
+		val |= (vgicv3->vgic_vmcr & ICH_VMCR_EOIM) >>
+			(ICH_VMCR_EOIM_SHIFT - ICC_CTLR_EL1_EOImode_SHIFT);
+
+		*vcpu_reg(vcpu, p->Rt) = val;
+	}
+
+	return true;
+}
+
+static bool access_gic_pmr(struct kvm_vcpu *vcpu,
+			   const struct sys_reg_params *p,
+			   const struct sys_reg_desc *r)
+{
+	u64 val;
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
+		return false;
+
+	if (p->is_write) {
+		val = *vcpu_reg(vcpu, p->Rt);
+		vgicv3->vgic_vmcr &= ~ICH_VMCR_PMR_MASK;
+		vgicv3->vgic_vmcr |= (val << ICH_VMCR_PMR_SHIFT) &
+					ICH_VMCR_PMR_MASK;
+	} else {
+		val = (vgicv3->vgic_vmcr & ICH_VMCR_PMR_MASK) >>
+			ICH_VMCR_PMR_SHIFT;
+		*vcpu_reg(vcpu, p->Rt) = val;
+	}
+
+	return true;
+}
+
+static bool access_gic_bpr0(struct kvm_vcpu *vcpu,
+			    const struct sys_reg_params *p,
+			    const struct sys_reg_desc *r)
+{
+	u64 val;
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
+		return false;
+
+	if (p->is_write) {
+		val = *vcpu_reg(vcpu, p->Rt);
+		vgicv3->vgic_vmcr &= ~ICH_VMCR_BPR0_MASK;
+		vgicv3->vgic_vmcr |= (val << ICH_VMCR_BPR0_SHIFT) &
+					ICH_VMCR_BPR0_MASK;
+	} else {
+		val = (vgicv3->vgic_vmcr & ICH_VMCR_BPR0_MASK) >>
+			ICH_VMCR_BPR0_SHIFT;
+		*vcpu_reg(vcpu, p->Rt) = val;
+	}
+
+	return true;
+}
+
+static bool access_gic_bpr1(struct kvm_vcpu *vcpu,
+			    const struct sys_reg_params *p,
+			    const struct sys_reg_desc *r)
+{
+	u64 val;
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
+		return false;
+
+	if (p->is_write) {
+		val = *vcpu_reg(vcpu, p->Rt);
+		vgicv3->vgic_vmcr &= ~ICH_VMCR_BPR1_MASK;
+		vgicv3->vgic_vmcr |= (val << ICH_VMCR_BPR1_SHIFT) &
+					ICH_VMCR_BPR1_MASK;
+	} else {
+		val = (vgicv3->vgic_vmcr & ICH_VMCR_BPR1_MASK) >>
+			ICH_VMCR_BPR1_SHIFT;
+		*vcpu_reg(vcpu, p->Rt) = val;
+	}
+
+	return true;
+}
+
+static bool access_gic_grpen0(struct kvm_vcpu *vcpu,
+			      const struct sys_reg_params *p,
+			      const struct sys_reg_desc *r)
+{
+	u64 val;
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
+		return false;
+
+	if (p->is_write) {
+		val = *vcpu_reg(vcpu, p->Rt);
+		vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG0;
+		vgicv3->vgic_vmcr |= (val << ICH_VMCR_ENG0_SHIFT) &
+					ICH_VMCR_ENG0;
+	} else {
+		val = (vgicv3->vgic_vmcr & ICH_VMCR_ENG0) >>
+			ICH_VMCR_ENG0_SHIFT;
+		*vcpu_reg(vcpu, p->Rt) = val;
+	}
+
+	return true;
+}
+
+static bool access_gic_grpen1(struct kvm_vcpu *vcpu,
+			      const struct sys_reg_params *p,
+			      const struct sys_reg_desc *r)
+{
+	u64 val;
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
+		return false;
+
+	if (p->is_write) {
+		val = *vcpu_reg(vcpu, p->Rt);
+		vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG1;
+		vgicv3->vgic_vmcr |= (val << ICH_VMCR_ENG1_SHIFT) &
+					ICH_VMCR_ENG1;
+	} else {
+		val = (vgicv3->vgic_vmcr & ICH_VMCR_ENG1) >>
+			ICH_VMCR_ENG1_SHIFT;
+		*vcpu_reg(vcpu, p->Rt) = val;
+	}
+
+	return true;
+}
+
 static bool trap_raz_wi(struct kvm_vcpu *vcpu,
 			const struct sys_reg_params *p,
 			const struct sys_reg_desc *r)

@@ -579,6 +736,10 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ Op0(0b11), Op1(0b000), CRn(0b0010), CRm(0b0000), Op2(0b010),
 	  access_vm_reg, reset_val, TCR_EL1, 0 },
 
+	/* ICC_PMR_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b0100), CRm(0b0110), Op2(0b000),
+	  access_gic_pmr },
+
 	/* AFSR0_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b0101), CRm(0b0001), Op2(0b000),
 	  access_vm_reg, reset_unknown, AFSR0_EL1 },

@@ -613,12 +774,27 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b0000), Op2(0b000),
 	  NULL, reset_val, VBAR_EL1, 0 },
 
+	/* ICC_BPR0_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1000), Op2(0b011),
+	  access_gic_bpr0 },
 	/* ICC_SGI1R_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1011), Op2(0b101),
 	  access_gic_sgi },
+	/* ICC_BPR1_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b011),
+	  access_gic_bpr1 },
+	/* ICC_CTLR_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b100),
+	  access_gic_ctlr },
 	/* ICC_SRE_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b101),
 	  trap_raz_wi },
+	/* ICC_IGRPEN0_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b110),
+	  access_gic_grpen0 },
+	/* ICC_GRPEN1_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b111),
+	  access_gic_grpen1 },
 
 	/* CONTEXTIDR_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b1101), CRm(0b0000), Op2(0b001),
include/linux/irqchip/arm-gic-v3.h +16 -2
index ed0fc9f..7e9fc16 100644
@@ -257,8 +257,14 @@
 /*
  * CPU interface registers
  */
-#define ICC_CTLR_EL1_EOImode_drop_dir	(0U << 1)
-#define ICC_CTLR_EL1_EOImode_drop	(1U << 1)
+#define ICC_CTLR_EL1_CBPR_SHIFT		0
+#define ICC_CTLR_EL1_EOImode_SHIFT	1
+#define ICC_CTLR_EL1_EOImode_drop_dir	(0U << ICC_CTLR_EL1_EOImode_SHIFT)
+#define ICC_CTLR_EL1_EOImode_drop	(1U << ICC_CTLR_EL1_EOImode_SHIFT)
+#define ICC_CTLR_EL1_PRIbits_MASK	(7U << 8)
+#define ICC_CTLR_EL1_IDbits_MASK	(7U << 11)
+#define ICC_CTLR_EL1_SEIS		(1U << 14)
+#define ICC_CTLR_EL1_A3V		(1U << 15)
 #define ICC_SRE_EL1_SRE			(1U << 0)
 
 /*

@@ -283,6 +289,14 @@
 
 #define ICH_VMCR_CTLR_SHIFT		0
 #define ICH_VMCR_CTLR_MASK		(0x21f << ICH_VMCR_CTLR_SHIFT)
+#define ICH_VMCR_ENG0_SHIFT		0
+#define ICH_VMCR_ENG0			(1 << ICH_VMCR_ENG0_SHIFT)
+#define ICH_VMCR_ENG1_SHIFT		1
+#define ICH_VMCR_ENG1			(1 << ICH_VMCR_ENG1_SHIFT)
+#define ICH_VMCR_CBPR_SHIFT		4
+#define ICH_VMCR_CBPR			(1 << ICH_VMCR_CBPR_SHIFT)
+#define ICH_VMCR_EOIM_SHIFT		9
+#define ICH_VMCR_EOIM			(1 << ICH_VMCR_EOIM_SHIFT)
 #define ICH_VMCR_BPR1_SHIFT		18
 #define ICH_VMCR_BPR1_MASK		(7 << ICH_VMCR_BPR1_SHIFT)
 #define ICH_VMCR_BPR0_SHIFT		21
Signature · 1 line
2.4.4
#10 lore raw

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

Quoted 212 lines · Pavel Fedin

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

Signed-off-by Pavel Fedin <[email protected]>
---
 arch/arm64/kvm/sys_regs.c          | 176 +++++++++++++++++++++++++++++++++++++
 include/linux/irqchip/arm-gic-v3.h |  18 +++-
 2 files changed, 192 insertions(+), 2 deletions(-)
arch/arm64/kvm/sys_regs.c +170 -0
index 8cc4a5e..7a4f982 100644
@@ -23,6 +23,7 @@
 #include <linux/kvm_host.h>
 #include <linux/mm.h>
 #include <linux/uaccess.h>
+#include <linux/irqchip/arm-gic-v3.h>
 
 #include <asm/cacheflush.h>
 #include <asm/cputype.h>

@@ -136,6 +137,162 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+static bool access_gic_ctlr(struct kvm_vcpu *vcpu,
+			    const struct sys_reg_params *p,
+			    const struct sys_reg_desc *r)
+{
+	u64 val;
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
+		return false;
+
+	if (p->is_write) {
+		val = *vcpu_reg(vcpu, p->Rt);
+
+		vgicv3->vgic_vmcr &= ~(ICH_VMCR_CBPR|ICH_VMCR_EOIM);
+		vgicv3->vgic_vmcr |= (val << (ICH_VMCR_CBPR_SHIFT -
+						ICC_CTLR_EL1_CBPR_SHIFT)) &
+					ICH_VMCR_CBPR;
+		vgicv3->vgic_vmcr |= (val << (ICH_VMCR_EOIM_SHIFT -
+						ICC_CTLR_EL1_EOImode_SHIFT)) &
+					ICH_VMCR_EOIM;
+	} else {
+		asm volatile("mrs_s %0," __stringify(ICC_IAR1_EL1)
+			     : "=r" (val));
+		val &= (ICC_CTLR_EL1_A3V | ICC_CTLR_EL1_SEIS |
+			ICC_CTLR_EL1_IDbits_MASK | ICC_CTLR_EL1_PRIbits_MASK);
+		val |= (vgicv3->vgic_vmcr & ICH_VMCR_CBPR) >>
+			(ICH_VMCR_CBPR_SHIFT - ICC_CTLR_EL1_CBPR_SHIFT);
+		val |= (vgicv3->vgic_vmcr & ICH_VMCR_EOIM) >>
+			(ICH_VMCR_EOIM_SHIFT - ICC_CTLR_EL1_EOImode_SHIFT);
+
+		*vcpu_reg(vcpu, p->Rt) = val;
+	}
+
+	return true;
+}
+
+static bool access_gic_pmr(struct kvm_vcpu *vcpu,
+			   const struct sys_reg_params *p,
+			   const struct sys_reg_desc *r)
+{
+	u64 val;
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
+		return false;
+
+	if (p->is_write) {
+		val = *vcpu_reg(vcpu, p->Rt);
+		vgicv3->vgic_vmcr &= ~ICH_VMCR_PMR_MASK;
+		vgicv3->vgic_vmcr |= (val << ICH_VMCR_PMR_SHIFT) &
+					ICH_VMCR_PMR_MASK;
+	} else {
+		val = (vgicv3->vgic_vmcr & ICH_VMCR_PMR_MASK) >>
+			ICH_VMCR_PMR_SHIFT;
+		*vcpu_reg(vcpu, p->Rt) = val;
+	}
+
+	return true;
+}
+
+static bool access_gic_bpr0(struct kvm_vcpu *vcpu,
+			    const struct sys_reg_params *p,
+			    const struct sys_reg_desc *r)
+{
+	u64 val;
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
+		return false;
+
+	if (p->is_write) {
+		val = *vcpu_reg(vcpu, p->Rt);
+		vgicv3->vgic_vmcr &= ~ICH_VMCR_BPR0_MASK;
+		vgicv3->vgic_vmcr |= (val << ICH_VMCR_BPR0_SHIFT) &
+					ICH_VMCR_BPR0_MASK;
+	} else {
+		val = (vgicv3->vgic_vmcr & ICH_VMCR_BPR0_MASK) >>
+			ICH_VMCR_BPR0_SHIFT;
+		*vcpu_reg(vcpu, p->Rt) = val;
+	}
+
+	return true;
+}
+
+static bool access_gic_bpr1(struct kvm_vcpu *vcpu,
+			    const struct sys_reg_params *p,
+			    const struct sys_reg_desc *r)
+{
+	u64 val;
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
+		return false;
+
+	if (p->is_write) {
+		val = *vcpu_reg(vcpu, p->Rt);
+		vgicv3->vgic_vmcr &= ~ICH_VMCR_BPR1_MASK;
+		vgicv3->vgic_vmcr |= (val << ICH_VMCR_BPR1_SHIFT) &
+					ICH_VMCR_BPR1_MASK;
+	} else {
+		val = (vgicv3->vgic_vmcr & ICH_VMCR_BPR1_MASK) >>
+			ICH_VMCR_BPR1_SHIFT;
+		*vcpu_reg(vcpu, p->Rt) = val;
+	}
+
+	return true;
+}
+
+static bool access_gic_grpen0(struct kvm_vcpu *vcpu,
+			      const struct sys_reg_params *p,
+			      const struct sys_reg_desc *r)
+{
+	u64 val;
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
+		return false;
+
+	if (p->is_write) {
+		val = *vcpu_reg(vcpu, p->Rt);
+		vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG0;
+		vgicv3->vgic_vmcr |= (val << ICH_VMCR_ENG0_SHIFT) &
+					ICH_VMCR_ENG0;
+	} else {
+		val = (vgicv3->vgic_vmcr & ICH_VMCR_ENG0) >>
+			ICH_VMCR_ENG0_SHIFT;
+		*vcpu_reg(vcpu, p->Rt) = val;
+	}
+
+	return true;
+}
+
+static bool access_gic_grpen1(struct kvm_vcpu *vcpu,
+			      const struct sys_reg_params *p,
+			      const struct sys_reg_desc *r)
+{
+	u64 val;
+	struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+	if (vcpu->kvm->arch.vgic.vgic_model != KVM_DEV_TYPE_ARM_VGIC_V3)
+		return false;
+
+	if (p->is_write) {
+		val = *vcpu_reg(vcpu, p->Rt);
+		vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG1;
+		vgicv3->vgic_vmcr |= (val << ICH_VMCR_ENG1_SHIFT) &
+					ICH_VMCR_ENG1;
+	} else {
+		val = (vgicv3->vgic_vmcr & ICH_VMCR_ENG1) >>
+			ICH_VMCR_ENG1_SHIFT;
+		*vcpu_reg(vcpu, p->Rt) = val;
+	}
+
+	return true;
+}
+
 static bool trap_raz_wi(struct kvm_vcpu *vcpu,
 			const struct sys_reg_params *p,
 			const struct sys_reg_desc *r)

@@ -579,6 +736,10 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ Op0(0b11), Op1(0b000), CRn(0b0010), CRm(0b0000), Op2(0b010),
 	  access_vm_reg, reset_val, TCR_EL1, 0 },
 
+	/* ICC_PMR_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b0100), CRm(0b0110), Op2(0b000),
+	  access_gic_pmr },
+
 	/* AFSR0_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b0101), CRm(0b0001), Op2(0b000),
 	  access_vm_reg, reset_unknown, AFSR0_EL1 },

@@ -613,12 +774,27 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b0000), Op2(0b000),
 	  NULL, reset_val, VBAR_EL1, 0 },
 
+	/* ICC_BPR0_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1000), Op2(0b011),
+	  access_gic_bpr0 },
 	/* ICC_SGI1R_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1011), Op2(0b101),
 	  access_gic_sgi },
+	/* ICC_BPR1_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b011),
+	  access_gic_bpr1 },
+	/* ICC_CTLR_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b100),
+	  access_gic_ctlr },

arch/arm64/kvm/sys_regs.c static const struct sys_reg_desc sys_reg_descs[] = { · line 798

Quoted 10 lines
arch/arm64/kvm/sys_regs.c +6 -0
 	/* ICC_SRE_EL1 */
 	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b101),
 	  trap_raz_wi },
+	/* ICC_IGRPEN0_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b110),
+	  access_gic_grpen0 },
+	/* ICC_GRPEN1_EL1 */
+	{ Op0(0b11), Op1(0b000), CRn(0b1100), CRm(0b1100), Op2(0b111),
+	  access_gic_grpen1 },

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

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

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

-Christoffer

#11 lore raw

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

Quoted 4 lines · Christoffer Dall

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

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

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

thanks
-- PMM

author
#12 lore raw

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

Hello!

Quoted 9 lines
Quoted 4 lines

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

There's an interesting design question lurking under this about the extent to which you expose the h/w design split between the CPU interface and the GIC proper as part of the KVM APIs.

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

static bool access_gic_ctlr(struct kvm_vcpu *vcpu,
const struct sys_reg_params *p, const struct sys_reg_desc *r)

{
	return vgicv3_access_ctlr(vcpu, vcpu_reg(vcpu, p->Rt), p->is_write);
}

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

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

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

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

#13 lore raw

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

Quoted 21 lines · Pavel Fedin

Hello!

Quoted 9 lines
Quoted 4 lines

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

There's an interesting design question lurking under this about the extent to which you expose the h/w design split between the CPU interface and the GIC proper as part of the KVM APIs.

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

static bool access_gic_ctlr(struct kvm_vcpu *vcpu,
const struct sys_reg_params *p, const struct sys_reg_desc *r)

{
	return vgicv3_access_ctlr(vcpu, vcpu_reg(vcpu, p->Rt), p->is_write);
}
Quoted 7 lines

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

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

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

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

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

-Christoffer

author
#14 lore raw

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

Hello!

Quoted 3 lines

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

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

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

#15 lore raw

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

In reply to Peter Maydell
Quoted 7 lines · Peter Maydell
Quoted 4 lines · Christoffer Dall

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

Quoted 6 lines

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

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

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

Quoted 6 lines

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

Thanks,
-Christoffer

author
#16 lore raw

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

In reply to Christoffer Dall

Hello!

Quoted 2 lines

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

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

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

#17 lore raw

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

Quoted 10 lines · Pavel Fedin

Hello!

Quoted 2 lines

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

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

I think we make some assumptions throughout the vgic code that only the vcpu itself touches the state of the registers. Maybe there is no need for additional locking, but I'd sleep better at night if I knew that whoever implemented save/restore logic had thought about concurrency.

-Christoffer

#18 lore raw
In reply to Pavel Fedin
Quoted 4 lines · Pavel Fedin

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

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

Quoted 17 lines

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

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