* [PATCH v3 1/5] KVM: arm/arm64: Refactor vGIC attributes handling code
2015-09-04 12:40 [PATCH v3 0/5] KVM: arm64: Implement API for vGICv3 live migration Pavel Fedin
@ 2015-09-04 12:40 ` Pavel Fedin
2015-09-04 12:40 ` [PATCH v3 2/5] KVM: arm64: Implement vGICv3 distributor and redistributor access from userspace Pavel Fedin
` (3 subsequent siblings)
4 siblings, 0 replies; 21+ messages in thread
From: Pavel Fedin @ 2015-09-04 12:40 UTC (permalink / raw)
To: kvmarm, kvm; +Cc: Marc Zyngier, Andre Przywara, Peter Maydell
Separate all implementation-independent code in vgic_attr_regs_access()
and move it to vgic.c. This will allow to reuse this code for vGICv3
implementation.
Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
virt/kvm/arm/vgic-v2-emul.c | 126 +++++++++-----------------------------------
virt/kvm/arm/vgic.c | 77 +++++++++++++++++++++++++++
virt/kvm/arm/vgic.h | 4 ++
3 files changed, 107 insertions(+), 100 deletions(-)
diff --git a/virt/kvm/arm/vgic-v2-emul.c b/virt/kvm/arm/vgic-v2-emul.c
index 1390797..557c5a6 100644
--- a/virt/kvm/arm/vgic-v2-emul.c
+++ b/virt/kvm/arm/vgic-v2-emul.c
@@ -661,97 +661,38 @@ static const struct vgic_io_range vgic_cpu_ranges[] = {
},
};
-static int vgic_attr_regs_access(struct kvm_device *dev,
- struct kvm_device_attr *attr,
- u32 *reg, bool is_write)
+static int vgic_v2_attr_regs_access(struct kvm_device *dev,
+ struct kvm_device_attr *attr,
+ __le32 *data, bool is_write)
{
- const struct vgic_io_range *r = NULL, *ranges;
+ const struct vgic_io_range *ranges;
phys_addr_t offset;
- int ret, cpuid, c;
- struct kvm_vcpu *vcpu, *tmp_vcpu;
- struct vgic_dist *vgic;
+ int cpuid;
+ struct vgic_dist *vgic = &dev->kvm->arch.vgic;
struct kvm_exit_mmio mmio;
- u32 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 = 4;
- mmio.is_write = is_write;
- mmio.data = &data;
- if (is_write)
- mmio_data_write(&mmio, ~0, *reg);
switch (attr->group) {
case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
- mmio.phys_addr = vgic->vgic_dist_base + offset;
+ mmio.phys_addr = vgic->vgic_dist_base;
ranges = vgic_dist_ranges;
break;
case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
- mmio.phys_addr = vgic->vgic_cpu_base + offset;
+ mmio.phys_addr = vgic->vgic_cpu_base;
ranges = vgic_cpu_ranges;
break;
default:
- BUG();
+ return -ENXIO;
}
- 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)
- *reg = mmio_data_read(&mmio, ~0);
+ mmio.is_write = is_write;
+ mmio.data = data;
- ret = 0;
-out_vgic_unlock:
- spin_unlock(&vgic->lock);
-out:
- mutex_unlock(&dev->kvm->lock);
- return ret;
+ return vgic_attr_regs_access(dev, ranges, &mmio, offset, sizeof(data),
+ cpuid);
}
static int vgic_v2_create(struct kvm_device *dev, u32 type)
@@ -767,53 +708,38 @@ static void vgic_v2_destroy(struct kvm_device *dev)
static int vgic_v2_set_attr(struct kvm_device *dev,
struct kvm_device_attr *attr)
{
+ u32 __user *uaddr = (u32 __user *)(long)attr->addr;
+ u32 reg;
+ __le32 data;
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_CPU_REGS: {
- u32 __user *uaddr = (u32 __user *)(long)attr->addr;
- u32 reg;
-
- if (get_user(reg, uaddr))
- return -EFAULT;
-
- return vgic_attr_regs_access(dev, attr, ®, true);
- }
-
- }
+ if (get_user(reg, uaddr))
+ return -EFAULT;
- return -ENXIO;
+ data = cpu_to_le32(reg);
+ return vgic_v2_attr_regs_access(dev, attr, &data, true);
}
static int vgic_v2_get_attr(struct kvm_device *dev,
struct kvm_device_attr *attr)
{
+ u32 __user *uaddr = (u32 __user *)(long)attr->addr;
+ __le32 data = 0;
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_CPU_REGS: {
- u32 __user *uaddr = (u32 __user *)(long)attr->addr;
- u32 reg = 0;
-
- ret = vgic_attr_regs_access(dev, attr, ®, false);
- if (ret)
- return ret;
- return put_user(reg, uaddr);
- }
-
- }
+ ret = vgic_v2_attr_regs_access(dev, attr, &data, false);
+ if (ret)
+ return ret;
- return -ENXIO;
+ return put_user(le32_to_cpu(data), uaddr);
}
static int vgic_v2_has_attr(struct kvm_device *dev,
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 9eb489a..33b00e5 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -2410,6 +2410,83 @@ int vgic_has_attr_regs(const struct vgic_io_range *ranges, phys_addr_t offset)
return -ENXIO;
}
+int vgic_attr_regs_access(struct kvm_device *dev,
+ const struct vgic_io_range *ranges,
+ struct kvm_exit_mmio *mmio, phys_addr_t offset,
+ int len, int cpuid)
+{
+ const struct vgic_io_range *r;
+ int ret, c;
+ struct kvm_vcpu *vcpu, *tmp_vcpu;
+ struct vgic_dist *vgic;
+
+ r = vgic_find_range(ranges, len, offset);
+
+ if (unlikely(!r || !r->handle_mmio))
+ return -ENXIO;
+
+ 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;
+
+ 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);
+
+ /*
+ * Unfortunately our MMIO handlers can process only up to 32 bits per
+ * access. For 64-bit registers we have to split up the operation.
+ */
+ mmio->len = sizeof(u32);
+ mmio->phys_addr += offset;
+ offset -= r->base;
+ r->handle_mmio(vcpu, mmio, offset);
+
+ if (len == sizeof(u64)) {
+ mmio->data += sizeof(u32);
+ mmio->phys_addr += sizeof(u32);
+ offset += sizeof(u32);
+ r->handle_mmio(vcpu, mmio, offset);
+ }
+
+ ret = 0;
+out_vgic_unlock:
+ spin_unlock(&vgic->lock);
+out:
+ mutex_unlock(&dev->kvm->lock);
+ return ret;
+}
+
static void vgic_init_maintenance_interrupt(void *info)
{
enable_percpu_irq(vgic->maint_irq, 0);
diff --git a/virt/kvm/arm/vgic.h b/virt/kvm/arm/vgic.h
index 0df74cb..a08348d 100644
--- a/virt/kvm/arm/vgic.h
+++ b/virt/kvm/arm/vgic.h
@@ -132,6 +132,10 @@ void vgic_kick_vcpus(struct kvm *kvm);
int vgic_has_attr_regs(const struct vgic_io_range *ranges, phys_addr_t offset);
int vgic_set_common_attr(struct kvm_device *dev, struct kvm_device_attr *attr);
int vgic_get_common_attr(struct kvm_device *dev, struct kvm_device_attr *attr);
+int vgic_attr_regs_access(struct kvm_device *dev,
+ const struct vgic_io_range *ranges,
+ struct kvm_exit_mmio *mmio, phys_addr_t offset,
+ int len, int cpuid);
int vgic_init(struct kvm *kvm);
void vgic_v2_init_emulation(struct kvm *kvm);
--
2.4.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v3 2/5] KVM: arm64: Implement vGICv3 distributor and redistributor access from userspace
2015-09-04 12:40 [PATCH v3 0/5] KVM: arm64: Implement API for vGICv3 live migration Pavel Fedin
2015-09-04 12:40 ` [PATCH v3 1/5] KVM: arm/arm64: Refactor vGIC attributes handling code Pavel Fedin
@ 2015-09-04 12:40 ` Pavel Fedin
2015-09-04 13:53 ` Andre Przywara
2015-09-04 12:40 ` [PATCH v3 3/5] KVM: arm64: Refactor system register handlers Pavel Fedin
` (2 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Pavel Fedin @ 2015-09-04 12:40 UTC (permalink / raw)
To: kvmarm, kvm; +Cc: Marc Zyngier, Andre Przywara, Peter Maydell
The access is done similar to vGICv2, 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. Since GICv3 can handle large number of CPUs,
KVM_DEV_ARM_VGIC_CPUID_MASK has been extended to 20 bits. This is enough
for 1048576 CPUs.
Some registers are 64-bit wide according to the specification.
KVM_DEV_ARM_VGIC_64BIT flag is introduced, allowing to perform full 64-bit
accesses.
Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
Documentation/virtual/kvm/devices/arm-vgic.txt | 35 ++++++++--
arch/arm64/include/uapi/asm/kvm.h | 4 +-
virt/kvm/arm/vgic-v3-emul.c | 95 ++++++++++++++++++++++----
virt/kvm/arm/vgic.c | 1 +
4 files changed, 116 insertions(+), 19 deletions(-)
diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
index 3fb9054..03f640f 100644
--- a/Documentation/virtual/kvm/devices/arm-vgic.txt
+++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
@@ -43,10 +43,13 @@ Groups:
KVM_DEV_ARM_VGIC_GRP_DIST_REGS
Attributes:
The attr field of kvm_device_attr encodes two values:
- bits: | 63 .... 40 | 39 .. 32 | 31 .... 0 |
- values: | reserved | cpu id | offset |
+ bits: | 63 | 62 .. 52 | 51 .. 32 | 31 .... 0 |
+ values: | size | reserved | cpu id | offset |
All distributor regs are (rw, 32-bit)
+ For GICv3 some regs are also (rw, 64-bit) according to the specification.
+ In order to perform full 64-bit access 'size' bit should be set to 1.
+ KVM_DEV_ARM_VGIC_64BIT flag value is provided for this purpose.
The offset is relative to the "Distributor base address" as defined in the
GICv2 specs. Getting or setting such a register has the same effect as
@@ -54,9 +57,33 @@ Groups:
specified with cpu id field. Note that most distributor fields are not
banked, but return the same value regardless of the cpu id used to access
the register.
+
+ Limitations:
+ - Priorities are not implemented, and registers are RAZ/WI
+ Errors:
+ -ENODEV: Getting or setting this register is not yet supported
+ -EBUSY: One or more VCPUs are running
+
+ KVM_DEV_ARM_VGIC_GRP_REDIST_REGS
+ Attributes:
+ The attr field of kvm_device_attr encodes two values:
+ bits: | 63 | 62 .. 52 | 51 .. 32 | 31 .... 0 |
+ values: | size | reserved | cpu id | offset |
+
+ All redistributor regs are (rw, 32-bit)
+ For GICv3 some regs are also (rw, 64-bit) according to the specification.
+ In order to perform full 64-bit access 'size' bit should be set to 1.
+ KVM_DEV_ARM_VGIC_64BIT flag value is provided for this purpose.
+
+ The offset is relative to the "Redistributor base address" as defined in
+ the GICv3 specs. Getting or setting such a register has the same effect as
+ reading or writing the register on the actual hardware from the cpu
+ specified with cpu id field. Note that most distributor fields are not
+ banked, but return the same value regardless of the cpu id used to access
+ the register.
+
Limitations:
- Priorities are not implemented, and registers are RAZ/WI
- - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
Errors:
-ENODEV: Getting or setting this register is not yet supported
-EBUSY: One or more VCPUs are running
@@ -64,7 +91,7 @@ Groups:
KVM_DEV_ARM_VGIC_GRP_CPU_REGS
Attributes:
The attr field of kvm_device_attr encodes two values:
- bits: | 63 .... 40 | 39 .. 32 | 31 .... 0 |
+ bits: | 63 .... 52 | 51 .. 32 | 31 .... 0 |
values: | reserved | cpu id | offset |
All CPU interface regs are (rw, 32-bit)
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 0cd7b59..249954f 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -196,13 +196,15 @@ struct kvm_arch_memory_slot {
#define KVM_DEV_ARM_VGIC_GRP_ADDR 0
#define KVM_DEV_ARM_VGIC_GRP_DIST_REGS 1
#define KVM_DEV_ARM_VGIC_GRP_CPU_REGS 2
+#define KVM_DEV_ARM_VGIC_64BIT (1ULL << 63)
#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_CPUID_MASK (0xfffffULL << 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_CTRL_INIT 0
+#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
/* KVM_IRQ_LINE irq field index values */
#define KVM_ARM_IRQ_TYPE_SHIFT 24
diff --git a/virt/kvm/arm/vgic-v3-emul.c b/virt/kvm/arm/vgic-v3-emul.c
index e661e7f..8bda714 100644
--- a/virt/kvm/arm/vgic-v3-emul.c
+++ b/virt/kvm/arm/vgic-v3-emul.c
@@ -39,6 +39,7 @@
#include <linux/kvm.h>
#include <linux/kvm_host.h>
#include <linux/interrupt.h>
+#include <linux/uaccess.h>
#include <linux/irqchip/arm-gic-v3.h>
#include <kvm/arm_vgic.h>
@@ -990,6 +991,77 @@ 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,
+ bool is_write)
+{
+ const struct vgic_io_range *ranges;
+ phys_addr_t offset;
+ int cpuid, ret;
+ struct vgic_dist *vgic = &dev->kvm->arch.vgic;
+ struct kvm_exit_mmio mmio;
+
+ cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
+ KVM_DEV_ARM_VGIC_CPUID_SHIFT;
+
+ switch (attr->group) {
+ case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
+ offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
+ mmio.phys_addr = vgic->vgic_dist_base;
+ ranges = vgic_v3_dist_ranges;
+ break;
+ case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
+ offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
+ mmio.phys_addr = vgic->vgic_redist_base;
+ ranges = vgic_redist_ranges;
+ break;
+ default:
+ return -ENXIO;
+ }
+
+ mmio.is_write = is_write;
+
+ if (attr->attr & KVM_DEV_ARM_VGIC_64BIT) {
+ u64 __user *uaddr = (u64 __user *)(long)attr->addr;
+ __le64 data;
+
+ if (is_write) {
+ u64 reg;
+
+ if (get_user(reg, uaddr))
+ return -EFAULT;
+ data = cpu_to_le64(reg);
+ }
+
+ mmio.data = &data;
+ ret = vgic_attr_regs_access(dev, ranges, &mmio, offset,
+ sizeof(data), cpuid);
+
+ if (!ret && !is_write)
+ ret = put_user(le64_to_cpu(data), uaddr);
+ } else {
+ u32 __user *uaddr = (u32 __user *)(long)attr->addr;
+ __le32 data;
+
+ if (is_write) {
+ u32 reg;
+
+ if (get_user(reg, uaddr))
+ return -EFAULT;
+ data = cpu_to_le32(reg);
+ }
+
+ mmio.data = &data;
+ ret = vgic_attr_regs_access(dev, ranges, &mmio, offset,
+ sizeof(data), cpuid);
+
+ if (!ret && !is_write)
+ ret = put_user(le32_to_cpu(data), uaddr);
+ }
+
+ return ret;
+}
+
static int vgic_v3_create(struct kvm_device *dev, u32 type)
{
return kvm_vgic_create(dev->kvm, type);
@@ -1009,13 +1081,7 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
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;
- }
-
- return -ENXIO;
+ return vgic_v3_attr_regs_access(dev, attr, true);
}
static int vgic_v3_get_attr(struct kvm_device *dev,
@@ -1027,18 +1093,14 @@ static int vgic_v3_get_attr(struct kvm_device *dev,
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;
- }
-
- return -ENXIO;
+ return vgic_v3_attr_regs_access(dev, attr, false);
}
static int vgic_v3_has_attr(struct kvm_device *dev,
struct kvm_device_attr *attr)
{
+ phys_addr_t offset;
+
switch (attr->group) {
case KVM_DEV_ARM_VGIC_GRP_ADDR:
switch (attr->attr) {
@@ -1051,6 +1113,11 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
}
break;
case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
+ offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
+ return vgic_has_attr_regs(vgic_v3_dist_ranges, offset);
+ case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
+ offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
+ return vgic_has_attr_regs(vgic_redist_ranges, offset);
case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
return -ENXIO;
case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 33b00e5..9aa802b 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -2468,6 +2468,7 @@ int vgic_attr_regs_access(struct kvm_device *dev,
* access. For 64-bit registers we have to split up the operation.
*/
mmio->len = sizeof(u32);
+ mmio->private = vcpu; /* For redistributor handlers */
mmio->phys_addr += offset;
offset -= r->base;
r->handle_mmio(vcpu, mmio, offset);
--
2.4.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3 2/5] KVM: arm64: Implement vGICv3 distributor and redistributor access from userspace
2015-09-04 12:40 ` [PATCH v3 2/5] KVM: arm64: Implement vGICv3 distributor and redistributor access from userspace Pavel Fedin
@ 2015-09-04 13:53 ` Andre Przywara
2015-09-04 15:22 ` Pavel Fedin
` (2 more replies)
0 siblings, 3 replies; 21+ messages in thread
From: Andre Przywara @ 2015-09-04 13:53 UTC (permalink / raw)
To: Pavel Fedin
Cc: Marc Zyngier, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org
Hi Pavel,
On 04/09/15 13:40, Pavel Fedin wrote:
> The access is done similar to vGICv2, 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. Since GICv3 can handle large number of CPUs,
> KVM_DEV_ARM_VGIC_CPUID_MASK has been extended to 20 bits. This is enough
> for 1048576 CPUs.
I guess the 20 bits come from 8 bits for Aff2 and Aff1 and 4-bits for
Aff0? If so, please mention this. But I am not sure we should limit the
cpu index in this public API to something as low 20 bits. Since this
mask is GIC specific, we could push the size bit into offset and use the
full upper 32 bits for cpuid, or at least 28 bits plus 4 reserved.
>
> Some registers are 64-bit wide according to the specification.
> KVM_DEV_ARM_VGIC_64BIT flag is introduced, allowing to perform full 64-bit
> accesses.
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
> Documentation/virtual/kvm/devices/arm-vgic.txt | 35 ++++++++--
> arch/arm64/include/uapi/asm/kvm.h | 4 +-
> virt/kvm/arm/vgic-v3-emul.c | 95 ++++++++++++++++++++++----
> virt/kvm/arm/vgic.c | 1 +
> 4 files changed, 116 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
> index 3fb9054..03f640f 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> @@ -43,10 +43,13 @@ Groups:
> KVM_DEV_ARM_VGIC_GRP_DIST_REGS
> Attributes:
> The attr field of kvm_device_attr encodes two values:
> - bits: | 63 .... 40 | 39 .. 32 | 31 .... 0 |
> - values: | reserved | cpu id | offset |
> + bits: | 63 | 62 .. 52 | 51 .. 32 | 31 .... 0 |
> + values: | size | reserved | cpu id | offset |
>
> All distributor regs are (rw, 32-bit)
> + For GICv3 some regs are also (rw, 64-bit) according to the specification.
That sounds contradictory to me. What about:
All registers can be accessed by using 32-bit accesses, some registers
also by 64-bit reads/writes according to the specification.
> + In order to perform full 64-bit access 'size' bit should be set to 1.
> + KVM_DEV_ARM_VGIC_64BIT flag value is provided for this purpose.
>
> The offset is relative to the "Distributor base address" as defined in the
> GICv2 specs. Getting or setting such a register has the same effect as
> @@ -54,9 +57,33 @@ Groups:
> specified with cpu id field. Note that most distributor fields are not
> banked, but return the same value regardless of the cpu id used to access
> the register.
> +
> + Limitations:
> + - Priorities are not implemented, and registers are RAZ/WI
> + Errors:
> + -ENODEV: Getting or setting this register is not yet supported
Isn't that actually -ENXIO in the code? I see that this is just copy &
paste, but it should be fixed in either case.
> + -EBUSY: One or more VCPUs are running
> +
> + KVM_DEV_ARM_VGIC_GRP_REDIST_REGS
> + Attributes:
> + The attr field of kvm_device_attr encodes two values:
> + bits: | 63 | 62 .. 52 | 51 .. 32 | 31 .... 0 |
> + values: | size | reserved | cpu id | offset |
> +
> + All redistributor regs are (rw, 32-bit)
> + For GICv3 some regs are also (rw, 64-bit) according to the specification.
> + In order to perform full 64-bit access 'size' bit should be set to 1.
> + KVM_DEV_ARM_VGIC_64BIT flag value is provided for this purpose.
> +
> + The offset is relative to the "Redistributor base address" as defined in
> + the GICv3 specs. Getting or setting such a register has the same effect as
> + reading or writing the register on the actual hardware from the cpu
> + specified with cpu id field. Note that most distributor fields are not
> + banked, but return the same value regardless of the cpu id used to access
> + the register.
> +
> Limitations:
> - Priorities are not implemented, and registers are RAZ/WI
> - - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
> Errors:
> -ENODEV: Getting or setting this register is not yet supported
> -EBUSY: One or more VCPUs are running
> @@ -64,7 +91,7 @@ Groups:
> KVM_DEV_ARM_VGIC_GRP_CPU_REGS
> Attributes:
> The attr field of kvm_device_attr encodes two values:
> - bits: | 63 .... 40 | 39 .. 32 | 31 .... 0 |
> + bits: | 63 .... 52 | 51 .. 32 | 31 .... 0 |
> values: | reserved | cpu id | offset |
>
> All CPU interface regs are (rw, 32-bit)
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 0cd7b59..249954f 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -196,13 +196,15 @@ struct kvm_arch_memory_slot {
> #define KVM_DEV_ARM_VGIC_GRP_ADDR 0
> #define KVM_DEV_ARM_VGIC_GRP_DIST_REGS 1
> #define KVM_DEV_ARM_VGIC_GRP_CPU_REGS 2
> +#define KVM_DEV_ARM_VGIC_64BIT (1ULL << 63)
> #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_CPUID_MASK (0xfffffULL << 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_CTRL_INIT 0
> +#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
>
> /* KVM_IRQ_LINE irq field index values */
> #define KVM_ARM_IRQ_TYPE_SHIFT 24
> diff --git a/virt/kvm/arm/vgic-v3-emul.c b/virt/kvm/arm/vgic-v3-emul.c
> index e661e7f..8bda714 100644
> --- a/virt/kvm/arm/vgic-v3-emul.c
> +++ b/virt/kvm/arm/vgic-v3-emul.c
> @@ -39,6 +39,7 @@
> #include <linux/kvm.h>
> #include <linux/kvm_host.h>
> #include <linux/interrupt.h>
> +#include <linux/uaccess.h>
>
> #include <linux/irqchip/arm-gic-v3.h>
> #include <kvm/arm_vgic.h>
> @@ -990,6 +991,77 @@ 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,
> + bool is_write)
> +{
> + const struct vgic_io_range *ranges;
> + phys_addr_t offset;
> + int cpuid, ret;
> + struct vgic_dist *vgic = &dev->kvm->arch.vgic;
> + struct kvm_exit_mmio mmio;
> +
> + cpuid = (attr->attr & KVM_DEV_ARM_VGIC_CPUID_MASK) >>
> + KVM_DEV_ARM_VGIC_CPUID_SHIFT;
> +
> + switch (attr->group) {
> + case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> + offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
Can't this be just after the cpuid= assignment outside of the switch
statement?
> + mmio.phys_addr = vgic->vgic_dist_base;
> + ranges = vgic_v3_dist_ranges;
> + break;
> + case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
> + offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
> + mmio.phys_addr = vgic->vgic_redist_base;
> + ranges = vgic_redist_ranges;
> + break;
> + default:
> + return -ENXIO;
> + }
> +
> + mmio.is_write = is_write;
> +
> + if (attr->attr & KVM_DEV_ARM_VGIC_64BIT) {
> + u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> + __le64 data;
> +
> + if (is_write) {
> + u64 reg;
> +
> + if (get_user(reg, uaddr))
Wouldn't the use of copy_from_user/copy_to_user here make this whole
switch redundant? Even if not, you could think about moving this logic
into into vgic_attr_regs_access() and just hardcode "len=4" into GICv2
accesses.
> + return -EFAULT;
> + data = cpu_to_le64(reg);
> + }
> +
> + mmio.data = &data;
> + ret = vgic_attr_regs_access(dev, ranges, &mmio, offset,
> + sizeof(data), cpuid);
> +
> + if (!ret && !is_write)
> + ret = put_user(le64_to_cpu(data), uaddr);
> + } else {
> + u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> + __le32 data;
> +
> + if (is_write) {
> + u32 reg;
> +
> + if (get_user(reg, uaddr))
> + return -EFAULT;
> + data = cpu_to_le32(reg);
> + }
> +
> + mmio.data = &data;
> + ret = vgic_attr_regs_access(dev, ranges, &mmio, offset,
> + sizeof(data), cpuid);
> +
> + if (!ret && !is_write)
> + ret = put_user(le32_to_cpu(data), uaddr);
> + }
> +
> + return ret;
> +}
> +
> static int vgic_v3_create(struct kvm_device *dev, u32 type)
> {
> return kvm_vgic_create(dev->kvm, type);
> @@ -1009,13 +1081,7 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
> 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;
> - }
> -
> - return -ENXIO;
> + return vgic_v3_attr_regs_access(dev, attr, true);
> }
>
> static int vgic_v3_get_attr(struct kvm_device *dev,
> @@ -1027,18 +1093,14 @@ static int vgic_v3_get_attr(struct kvm_device *dev,
> 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;
> - }
> -
> - return -ENXIO;
> + return vgic_v3_attr_regs_access(dev, attr, false);
> }
>
> static int vgic_v3_has_attr(struct kvm_device *dev,
> struct kvm_device_attr *attr)
> {
> + phys_addr_t offset;
> +
> switch (attr->group) {
> case KVM_DEV_ARM_VGIC_GRP_ADDR:
> switch (attr->attr) {
> @@ -1051,6 +1113,11 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
> }
> break;
> case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> + offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
> + return vgic_has_attr_regs(vgic_v3_dist_ranges, offset);
> + case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
> + offset = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
> + return vgic_has_attr_regs(vgic_redist_ranges, offset);
> case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
> return -ENXIO;
> case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 33b00e5..9aa802b 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -2468,6 +2468,7 @@ int vgic_attr_regs_access(struct kvm_device *dev,
> * access. For 64-bit registers we have to split up the operation.
> */
> mmio->len = sizeof(u32);
> + mmio->private = vcpu; /* For redistributor handlers */
I guess this can be moved into the caller and then you can drop the vcpu
parameter and use private here instead, no?
Cheers,
Andre.
> mmio->phys_addr += offset;
> offset -= r->base;
> r->handle_mmio(vcpu, mmio, offset);
>
^ permalink raw reply [flat|nested] 21+ messages in thread* RE: [PATCH v3 2/5] KVM: arm64: Implement vGICv3 distributor and redistributor access from userspace
2015-09-04 13:53 ` Andre Przywara
@ 2015-09-04 15:22 ` Pavel Fedin
2015-09-07 7:56 ` Pavel Fedin
2015-09-07 8:50 ` Pavel Fedin
2 siblings, 0 replies; 21+ messages in thread
From: Pavel Fedin @ 2015-09-04 15:22 UTC (permalink / raw)
To: 'Andre Przywara'
Cc: kvmarm, kvm, 'Marc Zyngier', 'Peter Maydell'
Hello!
> > The access is done similar to vGICv2, 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. Since GICv3 can handle large number of CPUs,
> > KVM_DEV_ARM_VGIC_CPUID_MASK has been extended to 20 bits. This is enough
> > for 1048576 CPUs.
>
> I guess the 20 bits come from 8 bits for Aff2 and Aff1 and 4-bits for
> Aff0? If so, please mention this. But I am not sure we should limit the
> cpu index in this public API
The documentation is confusing. It is actually not the affinity ID, but just number from 0 to N.
See http://lxr.free-electrons.com/source/include/linux/kvm_host.h#L427 - this is the function which
gets this "id" and returns vcpu structure.
So, we can have up to 1 << 20 = 1048576 CPUs. Enough?
If you are OK with this, i can post a separate patch to fix the documentation, or include it in
respin.
> we could push the size bit into offset and use the
> full upper 32 bits for cpuid, or at least 28 bits plus 4 reserved.
Actually 20 bits comes from ARM64_SYS_REG() macro, which i reuse for KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
--- cut ---
bits: | 63 .. 56 | 55 .. 48 | 47 ... 40 | 39 .. 32 | 31 .. 0 |
values: | arch | size | reserved | cpu id | reg id |
--- cut ---
arch = KVM_REG_ARM64 and size = KVM_REG_SIZE_U64. I decided not to invent own macro.
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
^ permalink raw reply [flat|nested] 21+ messages in thread* RE: [PATCH v3 2/5] KVM: arm64: Implement vGICv3 distributor and redistributor access from userspace
2015-09-04 13:53 ` Andre Przywara
2015-09-04 15:22 ` Pavel Fedin
@ 2015-09-07 7:56 ` Pavel Fedin
2015-09-09 11:28 ` Pavel Fedin
2015-09-07 8:50 ` Pavel Fedin
2 siblings, 1 reply; 21+ messages in thread
From: Pavel Fedin @ 2015-09-07 7:56 UTC (permalink / raw)
To: 'Andre Przywara'; +Cc: 'Marc Zyngier', kvmarm, kvm
Hello!
> > --- a/virt/kvm/arm/vgic.c
> > +++ b/virt/kvm/arm/vgic.c
> > @@ -2468,6 +2468,7 @@ int vgic_attr_regs_access(struct kvm_device *dev,
> > * access. For 64-bit registers we have to split up the operation.
> > */
> > mmio->len = sizeof(u32);
> > + mmio->private = vcpu; /* For redistributor handlers */
>
> I guess this can be moved into the caller and then you can drop the vcpu
> parameter and use private here instead, no?
No because 'vcpu' is not a parameter. It is figured out in the middle of the function, under
dev->kvm->lock mutex, out of 'cpuid' index.
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH v3 2/5] KVM: arm64: Implement vGICv3 distributor and redistributor access from userspace
2015-09-07 7:56 ` Pavel Fedin
@ 2015-09-09 11:28 ` Pavel Fedin
0 siblings, 0 replies; 21+ messages in thread
From: Pavel Fedin @ 2015-09-09 11:28 UTC (permalink / raw)
To: 'Andre Przywara'; +Cc: 'Marc Zyngier', kvmarm, kvm
Hello Andre! I haven't heard from you on any of my questions. But, nevertheless, i would like to
inform you (and other interested people) that i'm leaving on vacation, and will be back 2 weeks
later. Just in case if someone writes me something and i don't reply.
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of Pavel Fedin
> Sent: Monday, September 07, 2015 10:57 AM
> To: 'Andre Przywara'
> Cc: kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; 'Marc Zyngier'; 'Peter Maydell'
> Subject: RE: [PATCH v3 2/5] KVM: arm64: Implement vGICv3 distributor and redistributor access
> from userspace
>
> Hello!
>
> > > --- a/virt/kvm/arm/vgic.c
> > > +++ b/virt/kvm/arm/vgic.c
> > > @@ -2468,6 +2468,7 @@ int vgic_attr_regs_access(struct kvm_device *dev,
> > > * access. For 64-bit registers we have to split up the operation.
> > > */
> > > mmio->len = sizeof(u32);
> > > + mmio->private = vcpu; /* For redistributor handlers */
> >
> > I guess this can be moved into the caller and then you can drop the vcpu
> > parameter and use private here instead, no?
>
> No because 'vcpu' is not a parameter. It is figured out in the middle of the function, under
> dev->kvm->lock mutex, out of 'cpuid' index.
>
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH v3 2/5] KVM: arm64: Implement vGICv3 distributor and redistributor access from userspace
2015-09-04 13:53 ` Andre Przywara
2015-09-04 15:22 ` Pavel Fedin
2015-09-07 7:56 ` Pavel Fedin
@ 2015-09-07 8:50 ` Pavel Fedin
2 siblings, 0 replies; 21+ messages in thread
From: Pavel Fedin @ 2015-09-07 8:50 UTC (permalink / raw)
To: 'Andre Przywara'
Cc: kvmarm, kvm, 'Marc Zyngier', 'Peter Maydell'
Hello!
> > + mmio.is_write = is_write;
> > +
> > + if (attr->attr & KVM_DEV_ARM_VGIC_64BIT) {
> > + u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> > + __le64 data;
> > +
> > + if (is_write) {
> > + u64 reg;
> > +
> > + if (get_user(reg, uaddr))
>
> Wouldn't the use of copy_from_user/copy_to_user here make this whole
> switch redundant? Even if not, you could think about moving this logic
> into into vgic_attr_regs_access() and just hardcode "len=4" into GICv2
> accesses.
No, because endianess conversion is still strongly typed. And this ends up in:
if (is_write) {
u64 reg;
__le64 data;
copy_from_user(®, uaddr, mmio.len);
if (mmio.len == 8)
data = cpu_to_le64(reg);
else
*((__le32 *)&data) = cpu_to_le32(*((__le32 *)®);
isn't this even more ugly? Regardless of where it is placed.
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 3/5] KVM: arm64: Refactor system register handlers
2015-09-04 12:40 [PATCH v3 0/5] KVM: arm64: Implement API for vGICv3 live migration Pavel Fedin
2015-09-04 12:40 ` [PATCH v3 1/5] KVM: arm/arm64: Refactor vGIC attributes handling code Pavel Fedin
2015-09-04 12:40 ` [PATCH v3 2/5] KVM: arm64: Implement vGICv3 distributor and redistributor access from userspace Pavel Fedin
@ 2015-09-04 12:40 ` Pavel Fedin
2015-09-04 15:11 ` Andre Przywara
2015-09-04 12:40 ` [PATCH v3 4/5] KVM: arm64: Introduce find_reg_by_id() Pavel Fedin
2015-09-04 12:40 ` [PATCH v3 5/5] KVM: arm64: Implement vGICv3 CPU interface access Pavel Fedin
4 siblings, 1 reply; 21+ messages in thread
From: Pavel Fedin @ 2015-09-04 12:40 UTC (permalink / raw)
To: kvmarm, kvm; +Cc: Marc Zyngier, Andre Przywara
Replace Rt with data pointer in struct sys_reg_params. This will allow to
reuse system register handling code in implementation of vGICv3 CPU
interface access API. Additionally, got rid of "massive hack"
in kvm_handle_cp_64().
Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
arch/arm64/kvm/sys_regs.c | 61 +++++++++++++++++-------------------
arch/arm64/kvm/sys_regs.h | 4 +--
arch/arm64/kvm/sys_regs_generic_v8.c | 2 +-
3 files changed, 32 insertions(+), 35 deletions(-)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index b41607d..fe6b517 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -102,7 +102,7 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
BUG_ON(!p->is_write);
- val = *vcpu_reg(vcpu, p->Rt);
+ val = *p->val;
if (!p->is_aarch32) {
vcpu_sys_reg(vcpu, r->reg) = val;
} else {
@@ -125,13 +125,10 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
const struct sys_reg_params *p,
const struct sys_reg_desc *r)
{
- u64 val;
-
if (!p->is_write)
return read_from_write_only(vcpu, p);
- val = *vcpu_reg(vcpu, p->Rt);
- vgic_v3_dispatch_sgi(vcpu, val);
+ vgic_v3_dispatch_sgi(vcpu, *p->val);
return true;
}
@@ -153,7 +150,7 @@ static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
if (p->is_write) {
return ignore_write(vcpu, p);
} else {
- *vcpu_reg(vcpu, p->Rt) = (1 << 3);
+ *p->val = (1 << 3);
return true;
}
}
@@ -167,7 +164,7 @@ static bool trap_dbgauthstatus_el1(struct kvm_vcpu *vcpu,
} else {
u32 val;
asm volatile("mrs %0, dbgauthstatus_el1" : "=r" (val));
- *vcpu_reg(vcpu, p->Rt) = val;
+ *p->val = val;
return true;
}
}
@@ -204,13 +201,13 @@ static bool trap_debug_regs(struct kvm_vcpu *vcpu,
const struct sys_reg_desc *r)
{
if (p->is_write) {
- vcpu_sys_reg(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt);
+ vcpu_sys_reg(vcpu, r->reg) = *p->val;
vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
} else {
- *vcpu_reg(vcpu, p->Rt) = vcpu_sys_reg(vcpu, r->reg);
+ *p->val = vcpu_sys_reg(vcpu, r->reg);
}
- trace_trap_reg(__func__, r->reg, p->is_write, *vcpu_reg(vcpu, p->Rt));
+ trace_trap_reg(__func__, r->reg, p->is_write, *p->val);
return true;
}
@@ -228,7 +225,7 @@ static inline void reg_to_dbg(struct kvm_vcpu *vcpu,
const struct sys_reg_params *p,
u64 *dbg_reg)
{
- u64 val = *vcpu_reg(vcpu, p->Rt);
+ u64 val = *p->val;
if (p->is_32bit) {
val &= 0xffffffffUL;
@@ -248,7 +245,7 @@ static inline void dbg_to_reg(struct kvm_vcpu *vcpu,
if (p->is_32bit)
val &= 0xffffffffUL;
- *vcpu_reg(vcpu, p->Rt) = val;
+ *p->val = val;
}
static inline bool trap_bvr(struct kvm_vcpu *vcpu,
@@ -704,10 +701,10 @@ static bool trap_dbgidr(struct kvm_vcpu *vcpu,
u64 pfr = read_cpuid(ID_AA64PFR0_EL1);
u32 el3 = !!((pfr >> 12) & 0xf);
- *vcpu_reg(vcpu, p->Rt) = ((((dfr >> 20) & 0xf) << 28) |
- (((dfr >> 12) & 0xf) << 24) |
- (((dfr >> 28) & 0xf) << 20) |
- (6 << 16) | (el3 << 14) | (el3 << 12));
+ *p->val = ((((dfr >> 20) & 0xf) << 28) |
+ (((dfr >> 12) & 0xf) << 24) |
+ (((dfr >> 28) & 0xf) << 20) |
+ (6 << 16) | (el3 << 14) | (el3 << 12));
return true;
}
}
@@ -717,10 +714,10 @@ static bool trap_debug32(struct kvm_vcpu *vcpu,
const struct sys_reg_desc *r)
{
if (p->is_write) {
- vcpu_cp14(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt);
+ vcpu_cp14(vcpu, r->reg) = *p->val;
vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
} else {
- *vcpu_reg(vcpu, p->Rt) = vcpu_cp14(vcpu, r->reg);
+ *p->val = vcpu_cp14(vcpu, r->reg);
}
return true;
@@ -747,12 +744,12 @@ static inline bool trap_xvr(struct kvm_vcpu *vcpu,
u64 val = *dbg_reg;
val &= 0xffffffffUL;
- val |= *vcpu_reg(vcpu, p->Rt) << 32;
+ val |= *p->val << 32;
*dbg_reg = val;
vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
} else {
- *vcpu_reg(vcpu, p->Rt) = *dbg_reg >> 32;
+ *p->val = *dbg_reg >> 32;
}
trace_trap_reg(__func__, rd->reg, p->is_write, *dbg_reg);
@@ -1069,12 +1066,14 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
{
struct sys_reg_params params;
u32 hsr = kvm_vcpu_get_hsr(vcpu);
+ int Rt = (hsr >> 5) & 0xf;
int Rt2 = (hsr >> 10) & 0xf;
+ unsigned long val;
params.is_aarch32 = true;
params.is_32bit = false;
params.CRm = (hsr >> 1) & 0xf;
- params.Rt = (hsr >> 5) & 0xf;
+ params.val = &val;
params.is_write = ((hsr & 1) == 0);
params.Op0 = 0;
@@ -1083,15 +1082,12 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
params.CRn = 0;
/*
- * Massive hack here. Store Rt2 in the top 32bits so we only
- * have one register to deal with. As we use the same trap
+ * Make a 64-bit value out of Rt and Rt2. As we use the same trap
* backends between AArch32 and AArch64, we get away with it.
*/
if (params.is_write) {
- u64 val = *vcpu_reg(vcpu, params.Rt);
- val &= 0xffffffff;
+ val = *vcpu_reg(vcpu, Rt) & 0xffffffff;
val |= *vcpu_reg(vcpu, Rt2) << 32;
- *vcpu_reg(vcpu, params.Rt) = val;
}
if (!emulate_cp(vcpu, ¶ms, target_specific, nr_specific))
@@ -1102,11 +1098,10 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
unhandled_cp_access(vcpu, ¶ms);
out:
- /* Do the opposite hack for the read side */
+ /* Split up the value between registers for the read side */
if (!params.is_write) {
- u64 val = *vcpu_reg(vcpu, params.Rt);
- val >>= 32;
- *vcpu_reg(vcpu, Rt2) = val;
+ *vcpu_reg(vcpu, Rt) = val & 0xffffffff;
+ *vcpu_reg(vcpu, Rt2) = val >> 32;
}
return 1;
@@ -1125,11 +1120,12 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
{
struct sys_reg_params params;
u32 hsr = kvm_vcpu_get_hsr(vcpu);
+ int Rt = (hsr >> 5) & 0xf;
params.is_aarch32 = true;
params.is_32bit = true;
params.CRm = (hsr >> 1) & 0xf;
- params.Rt = (hsr >> 5) & 0xf;
+ params.val = vcpu_reg(vcpu, Rt);
params.is_write = ((hsr & 1) == 0);
params.CRn = (hsr >> 10) & 0xf;
params.Op0 = 0;
@@ -1237,6 +1233,7 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu, struct kvm_run *run)
{
struct sys_reg_params params;
unsigned long esr = kvm_vcpu_get_hsr(vcpu);
+ int Rt = (esr >> 5) & 0x1f;
trace_kvm_handle_sys_reg(esr);
@@ -1247,7 +1244,7 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu, struct kvm_run *run)
params.CRn = (esr >> 10) & 0xf;
params.CRm = (esr >> 1) & 0xf;
params.Op2 = (esr >> 17) & 0x7;
- params.Rt = (esr >> 5) & 0x1f;
+ params.val = vcpu_reg(vcpu, Rt);
params.is_write = !(esr & 1);
return emulate_sys_reg(vcpu, ¶ms);
diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
index eaa324e..3267518 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -28,7 +28,7 @@ struct sys_reg_params {
u8 CRn;
u8 CRm;
u8 Op2;
- u8 Rt;
+ u_long *val;
bool is_write;
bool is_aarch32;
bool is_32bit; /* Only valid if is_aarch32 is true */
@@ -79,7 +79,7 @@ static inline bool ignore_write(struct kvm_vcpu *vcpu,
static inline bool read_zero(struct kvm_vcpu *vcpu,
const struct sys_reg_params *p)
{
- *vcpu_reg(vcpu, p->Rt) = 0;
+ *p->val = 0;
return true;
}
diff --git a/arch/arm64/kvm/sys_regs_generic_v8.c b/arch/arm64/kvm/sys_regs_generic_v8.c
index 1e45768..c805576 100644
--- a/arch/arm64/kvm/sys_regs_generic_v8.c
+++ b/arch/arm64/kvm/sys_regs_generic_v8.c
@@ -37,7 +37,7 @@ static bool access_actlr(struct kvm_vcpu *vcpu,
if (p->is_write)
return ignore_write(vcpu, p);
- *vcpu_reg(vcpu, p->Rt) = vcpu_sys_reg(vcpu, ACTLR_EL1);
+ *p->val = vcpu_sys_reg(vcpu, ACTLR_EL1);
return true;
}
--
2.4.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3 3/5] KVM: arm64: Refactor system register handlers
2015-09-04 12:40 ` [PATCH v3 3/5] KVM: arm64: Refactor system register handlers Pavel Fedin
@ 2015-09-04 15:11 ` Andre Przywara
2015-09-04 15:32 ` Pavel Fedin
0 siblings, 1 reply; 21+ messages in thread
From: Andre Przywara @ 2015-09-04 15:11 UTC (permalink / raw)
To: Pavel Fedin
Cc: Marc Zyngier, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org
Hi Pavel,
On 04/09/15 13:40, Pavel Fedin wrote:
> Replace Rt with data pointer in struct sys_reg_params. This will allow to
> reuse system register handling code in implementation of vGICv3 CPU
> interface access API. Additionally, got rid of "massive hack"
> in kvm_handle_cp_64().
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
> arch/arm64/kvm/sys_regs.c | 61 +++++++++++++++++-------------------
> arch/arm64/kvm/sys_regs.h | 4 +--
> arch/arm64/kvm/sys_regs_generic_v8.c | 2 +-
> 3 files changed, 32 insertions(+), 35 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index b41607d..fe6b517 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -102,7 +102,7 @@ static bool access_vm_reg(struct kvm_vcpu *vcpu,
>
> BUG_ON(!p->is_write);
>
> - val = *vcpu_reg(vcpu, p->Rt);
> + val = *p->val;
> if (!p->is_aarch32) {
> vcpu_sys_reg(vcpu, r->reg) = val;
> } else {
> @@ -125,13 +125,10 @@ static bool access_gic_sgi(struct kvm_vcpu *vcpu,
> const struct sys_reg_params *p,
> const struct sys_reg_desc *r)
> {
> - u64 val;
> -
> if (!p->is_write)
> return read_from_write_only(vcpu, p);
>
> - val = *vcpu_reg(vcpu, p->Rt);
> - vgic_v3_dispatch_sgi(vcpu, val);
> + vgic_v3_dispatch_sgi(vcpu, *p->val);
>
> return true;
> }
> @@ -153,7 +150,7 @@ static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
> if (p->is_write) {
> return ignore_write(vcpu, p);
> } else {
> - *vcpu_reg(vcpu, p->Rt) = (1 << 3);
> + *p->val = (1 << 3);
> return true;
> }
> }
> @@ -167,7 +164,7 @@ static bool trap_dbgauthstatus_el1(struct kvm_vcpu *vcpu,
> } else {
> u32 val;
> asm volatile("mrs %0, dbgauthstatus_el1" : "=r" (val));
> - *vcpu_reg(vcpu, p->Rt) = val;
> + *p->val = val;
> return true;
> }
> }
> @@ -204,13 +201,13 @@ static bool trap_debug_regs(struct kvm_vcpu *vcpu,
> const struct sys_reg_desc *r)
> {
> if (p->is_write) {
> - vcpu_sys_reg(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt);
> + vcpu_sys_reg(vcpu, r->reg) = *p->val;
> vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
> } else {
> - *vcpu_reg(vcpu, p->Rt) = vcpu_sys_reg(vcpu, r->reg);
> + *p->val = vcpu_sys_reg(vcpu, r->reg);
> }
>
> - trace_trap_reg(__func__, r->reg, p->is_write, *vcpu_reg(vcpu, p->Rt));
> + trace_trap_reg(__func__, r->reg, p->is_write, *p->val);
>
> return true;
> }
> @@ -228,7 +225,7 @@ static inline void reg_to_dbg(struct kvm_vcpu *vcpu,
> const struct sys_reg_params *p,
> u64 *dbg_reg)
> {
> - u64 val = *vcpu_reg(vcpu, p->Rt);
> + u64 val = *p->val;
>
> if (p->is_32bit) {
> val &= 0xffffffffUL;
> @@ -248,7 +245,7 @@ static inline void dbg_to_reg(struct kvm_vcpu *vcpu,
> if (p->is_32bit)
> val &= 0xffffffffUL;
>
> - *vcpu_reg(vcpu, p->Rt) = val;
> + *p->val = val;
> }
>
> static inline bool trap_bvr(struct kvm_vcpu *vcpu,
> @@ -704,10 +701,10 @@ static bool trap_dbgidr(struct kvm_vcpu *vcpu,
> u64 pfr = read_cpuid(ID_AA64PFR0_EL1);
> u32 el3 = !!((pfr >> 12) & 0xf);
>
> - *vcpu_reg(vcpu, p->Rt) = ((((dfr >> 20) & 0xf) << 28) |
> - (((dfr >> 12) & 0xf) << 24) |
> - (((dfr >> 28) & 0xf) << 20) |
> - (6 << 16) | (el3 << 14) | (el3 << 12));
> + *p->val = ((((dfr >> 20) & 0xf) << 28) |
> + (((dfr >> 12) & 0xf) << 24) |
> + (((dfr >> 28) & 0xf) << 20) |
> + (6 << 16) | (el3 << 14) | (el3 << 12));
> return true;
> }
> }
> @@ -717,10 +714,10 @@ static bool trap_debug32(struct kvm_vcpu *vcpu,
> const struct sys_reg_desc *r)
> {
> if (p->is_write) {
> - vcpu_cp14(vcpu, r->reg) = *vcpu_reg(vcpu, p->Rt);
> + vcpu_cp14(vcpu, r->reg) = *p->val;
> vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
> } else {
> - *vcpu_reg(vcpu, p->Rt) = vcpu_cp14(vcpu, r->reg);
> + *p->val = vcpu_cp14(vcpu, r->reg);
> }
>
> return true;
> @@ -747,12 +744,12 @@ static inline bool trap_xvr(struct kvm_vcpu *vcpu,
> u64 val = *dbg_reg;
>
> val &= 0xffffffffUL;
> - val |= *vcpu_reg(vcpu, p->Rt) << 32;
> + val |= *p->val << 32;
> *dbg_reg = val;
>
> vcpu->arch.debug_flags |= KVM_ARM64_DEBUG_DIRTY;
> } else {
> - *vcpu_reg(vcpu, p->Rt) = *dbg_reg >> 32;
> + *p->val = *dbg_reg >> 32;
> }
>
> trace_trap_reg(__func__, rd->reg, p->is_write, *dbg_reg);
> @@ -1069,12 +1066,14 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
> {
> struct sys_reg_params params;
> u32 hsr = kvm_vcpu_get_hsr(vcpu);
> + int Rt = (hsr >> 5) & 0xf;
> int Rt2 = (hsr >> 10) & 0xf;
> + unsigned long val;
Mmmh, not sure about this, but I guess u64 would be more precise here?
With the Rt2 handling below we rely on this being 64 bits anyway. But I
guess that breaks with assigning vcpu_reg later and since this is arm64
only, long is always 64 bits?
Otherwise looks good to me and nice to see this hack go away.
Cheers,
Andre.
>
> params.is_aarch32 = true;
> params.is_32bit = false;
> params.CRm = (hsr >> 1) & 0xf;
> - params.Rt = (hsr >> 5) & 0xf;
> + params.val = &val;
> params.is_write = ((hsr & 1) == 0);
>
> params.Op0 = 0;
> @@ -1083,15 +1082,12 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
> params.CRn = 0;
>
> /*
> - * Massive hack here. Store Rt2 in the top 32bits so we only
> - * have one register to deal with. As we use the same trap
> + * Make a 64-bit value out of Rt and Rt2. As we use the same trap
> * backends between AArch32 and AArch64, we get away with it.
> */
> if (params.is_write) {
> - u64 val = *vcpu_reg(vcpu, params.Rt);
> - val &= 0xffffffff;
> + val = *vcpu_reg(vcpu, Rt) & 0xffffffff;
> val |= *vcpu_reg(vcpu, Rt2) << 32;
> - *vcpu_reg(vcpu, params.Rt) = val;
> }
>
> if (!emulate_cp(vcpu, ¶ms, target_specific, nr_specific))
> @@ -1102,11 +1098,10 @@ static int kvm_handle_cp_64(struct kvm_vcpu *vcpu,
> unhandled_cp_access(vcpu, ¶ms);
>
> out:
> - /* Do the opposite hack for the read side */
> + /* Split up the value between registers for the read side */
> if (!params.is_write) {
> - u64 val = *vcpu_reg(vcpu, params.Rt);
> - val >>= 32;
> - *vcpu_reg(vcpu, Rt2) = val;
> + *vcpu_reg(vcpu, Rt) = val & 0xffffffff;
> + *vcpu_reg(vcpu, Rt2) = val >> 32;
> }
>
> return 1;
> @@ -1125,11 +1120,12 @@ static int kvm_handle_cp_32(struct kvm_vcpu *vcpu,
> {
> struct sys_reg_params params;
> u32 hsr = kvm_vcpu_get_hsr(vcpu);
> + int Rt = (hsr >> 5) & 0xf;
>
> params.is_aarch32 = true;
> params.is_32bit = true;
> params.CRm = (hsr >> 1) & 0xf;
> - params.Rt = (hsr >> 5) & 0xf;
> + params.val = vcpu_reg(vcpu, Rt);
> params.is_write = ((hsr & 1) == 0);
> params.CRn = (hsr >> 10) & 0xf;
> params.Op0 = 0;
> @@ -1237,6 +1233,7 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu, struct kvm_run *run)
> {
> struct sys_reg_params params;
> unsigned long esr = kvm_vcpu_get_hsr(vcpu);
> + int Rt = (esr >> 5) & 0x1f;
>
> trace_kvm_handle_sys_reg(esr);
>
> @@ -1247,7 +1244,7 @@ int kvm_handle_sys_reg(struct kvm_vcpu *vcpu, struct kvm_run *run)
> params.CRn = (esr >> 10) & 0xf;
> params.CRm = (esr >> 1) & 0xf;
> params.Op2 = (esr >> 17) & 0x7;
> - params.Rt = (esr >> 5) & 0x1f;
> + params.val = vcpu_reg(vcpu, Rt);
> params.is_write = !(esr & 1);
>
> return emulate_sys_reg(vcpu, ¶ms);
> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
> index eaa324e..3267518 100644
> --- a/arch/arm64/kvm/sys_regs.h
> +++ b/arch/arm64/kvm/sys_regs.h
> @@ -28,7 +28,7 @@ struct sys_reg_params {
> u8 CRn;
> u8 CRm;
> u8 Op2;
> - u8 Rt;
> + u_long *val;
> bool is_write;
> bool is_aarch32;
> bool is_32bit; /* Only valid if is_aarch32 is true */
> @@ -79,7 +79,7 @@ static inline bool ignore_write(struct kvm_vcpu *vcpu,
> static inline bool read_zero(struct kvm_vcpu *vcpu,
> const struct sys_reg_params *p)
> {
> - *vcpu_reg(vcpu, p->Rt) = 0;
> + *p->val = 0;
> return true;
> }
>
> diff --git a/arch/arm64/kvm/sys_regs_generic_v8.c b/arch/arm64/kvm/sys_regs_generic_v8.c
> index 1e45768..c805576 100644
> --- a/arch/arm64/kvm/sys_regs_generic_v8.c
> +++ b/arch/arm64/kvm/sys_regs_generic_v8.c
> @@ -37,7 +37,7 @@ static bool access_actlr(struct kvm_vcpu *vcpu,
> if (p->is_write)
> return ignore_write(vcpu, p);
>
> - *vcpu_reg(vcpu, p->Rt) = vcpu_sys_reg(vcpu, ACTLR_EL1);
> + *p->val = vcpu_sys_reg(vcpu, ACTLR_EL1);
> return true;
> }
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread* RE: [PATCH v3 3/5] KVM: arm64: Refactor system register handlers
2015-09-04 15:11 ` Andre Przywara
@ 2015-09-04 15:32 ` Pavel Fedin
0 siblings, 0 replies; 21+ messages in thread
From: Pavel Fedin @ 2015-09-04 15:32 UTC (permalink / raw)
To: 'Andre Przywara'; +Cc: 'Marc Zyngier', kvmarm, kvm
Hello!
> Mmmh, not sure about this, but I guess u64 would be more precise here?
> With the Rt2 handling below we rely on this being 64 bits anyway. But I
> guess that breaks with assigning vcpu_reg later and since this is arm64
> only, long is always 64 bits?
It is because ' params.val = &val'. And params.val is u_long * because of: 'params.val =
vcpu_reg(vcpu, Rt)'. vcpu_reg() is declared as unsigned long * :
http://lxr.free-electrons.com/source/arch/arm64/include/asm/kvm_emulate.h#L102
Initially i declared params.val as u64 *, but got warning in 'params.val = vcpu_reg(vcpu, Rt)'. So
i decided to change it to u_long * instead of having casts here and there. Do you like casts?
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 4/5] KVM: arm64: Introduce find_reg_by_id()
2015-09-04 12:40 [PATCH v3 0/5] KVM: arm64: Implement API for vGICv3 live migration Pavel Fedin
` (2 preceding siblings ...)
2015-09-04 12:40 ` [PATCH v3 3/5] KVM: arm64: Refactor system register handlers Pavel Fedin
@ 2015-09-04 12:40 ` Pavel Fedin
2015-09-04 15:12 ` Andre Przywara
2015-09-04 12:40 ` [PATCH v3 5/5] KVM: arm64: Implement vGICv3 CPU interface access Pavel Fedin
4 siblings, 1 reply; 21+ messages in thread
From: Pavel Fedin @ 2015-09-04 12:40 UTC (permalink / raw)
To: kvmarm, kvm; +Cc: Marc Zyngier, Andre Przywara
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>
---
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 fe6b517..21403fa 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1283,6 +1283,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)
@@ -1410,10 +1421,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, ¶ms))
- return -ENOENT;
-
- r = find_reg(¶ms, invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs));
+ r = find_reg_by_id(id, ¶ms, invariant_sys_regs,
+ ARRAY_SIZE(invariant_sys_regs));
if (!r)
return -ENOENT;
@@ -1427,9 +1436,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, ¶ms))
- return -ENOENT;
- r = find_reg(¶ms, invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs));
+ r = find_reg_by_id(id, ¶ms, 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 3267518..0646108 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
--
2.4.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3 4/5] KVM: arm64: Introduce find_reg_by_id()
2015-09-04 12:40 ` [PATCH v3 4/5] KVM: arm64: Introduce find_reg_by_id() Pavel Fedin
@ 2015-09-04 15:12 ` Andre Przywara
0 siblings, 0 replies; 21+ messages in thread
From: Andre Przywara @ 2015-09-04 15:12 UTC (permalink / raw)
To: Pavel Fedin
Cc: Marc Zyngier, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org
On 04/09/15 13:40, Pavel Fedin wrote:
> 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>
> ---
> 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 fe6b517..21403fa 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1283,6 +1283,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)
> @@ -1410,10 +1421,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, ¶ms))
> - return -ENOENT;
> -
> - r = find_reg(¶ms, invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs));
> + r = find_reg_by_id(id, ¶ms, invariant_sys_regs,
> + ARRAY_SIZE(invariant_sys_regs));
> if (!r)
> return -ENOENT;
>
> @@ -1427,9 +1436,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, ¶ms))
> - return -ENOENT;
> - r = find_reg(¶ms, invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs));
> + r = find_reg_by_id(id, ¶ms, 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 3267518..0646108 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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 5/5] KVM: arm64: Implement vGICv3 CPU interface access
2015-09-04 12:40 [PATCH v3 0/5] KVM: arm64: Implement API for vGICv3 live migration Pavel Fedin
` (3 preceding siblings ...)
2015-09-04 12:40 ` [PATCH v3 4/5] KVM: arm64: Introduce find_reg_by_id() Pavel Fedin
@ 2015-09-04 12:40 ` Pavel Fedin
2015-09-04 15:13 ` Andre Przywara
4 siblings, 1 reply; 21+ messages in thread
From: Pavel Fedin @ 2015-09-04 12:40 UTC (permalink / raw)
To: kvmarm, kvm; +Cc: Marc Zyngier, Andre Przywara, Peter Maydell
The access is done similar to GICv2, using KVM_DEV_ARM_VGIC_GRP_CPU_REGS
group, however attribute ID encodes corresponding system register. Access
size is always 64 bits.
Since CPU interface state actually affects only a single vCPU, no vGIC
locking is done. Just made sure that the vCPU is not running.
Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
Documentation/virtual/kvm/devices/arm-vgic.txt | 38 +++-
arch/arm64/include/uapi/asm/kvm.h | 7 +
include/linux/irqchip/arm-gic-v3.h | 18 +-
virt/kvm/arm/vgic-v3-emul.c | 244 +++++++++++++++++++++++++
4 files changed, 303 insertions(+), 4 deletions(-)
diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
index 03f640f..518b634 100644
--- a/Documentation/virtual/kvm/devices/arm-vgic.txt
+++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
@@ -88,7 +88,7 @@ Groups:
-ENODEV: Getting or setting this register is not yet supported
-EBUSY: One or more VCPUs are running
- KVM_DEV_ARM_VGIC_GRP_CPU_REGS
+ KVM_DEV_ARM_VGIC_GRP_CPU_REGS (vGICv2)
Attributes:
The attr field of kvm_device_attr encodes two values:
bits: | 63 .... 52 | 51 .. 32 | 31 .... 0 |
@@ -116,11 +116,45 @@ Groups:
Limitations:
- Priorities are not implemented, and registers are RAZ/WI
- - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
Errors:
-ENODEV: Getting or setting this register is not yet supported
-EBUSY: One or more VCPUs are running
+ KVM_DEV_ARM_VGIC_GRP_CPU_REGS (vGICv3)
+ Attributes:
+ The attr field of kvm_device_attr encodes the following values:
+ bits: | 63 .. 56 | 55 .. 48 | 47 ... 40 | 39 .. 32 | 31 .. 0 |
+ values: | arch | size | reserved | cpu id | reg id |
+
+ All CPU interface regs are (rw, 64-bit). The only supported size value is
+ KVM_REG_SIZE_U64.
+
+ Arch, size and reg id fields actually encode system register to be
+ accessed. Normally these values are obtained using ARM64_SYS_REG() macro.
+ Getting or setting such a register has the same effect as reading or
+ writing the register on the actual hardware.
+
+ The Active Priorities Registers AP0Rn and AP1Rn are implementation defined,
+ so we set a fixed format for our implementation that fits with the model of
+ a "GICv3 implementation without the security extensions" which we present
+ to the guest. This interface always exposes four register APR[0-3]
+ describing the maximum possible 128 preemption levels. The semantics of the
+ register indicates if any interrupts in a given preemption level are in the
+ active state by setting the corresponding bit.
+
+ Thus, preemption level X has one or more active interrupts if and only if:
+
+ APRn[X mod 32] == 0b1, where n = X / 32
+
+ Bits for undefined preemption levels are RAZ/WI.
+
+ Limitations:
+ - Priorities are not implemented, and registers are RAZ/WI
+ Errors:
+ -ENODEV: Getting or setting this register is not yet supported
+ -EBUSY: One or more VCPUs are running
+
+
KVM_DEV_ARM_VGIC_GRP_NR_IRQS
Attributes:
A value describing the number of interrupts (SGI, PPI and SPI) for
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 249954f..7d37ccd 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -201,6 +201,13 @@ struct kvm_arch_memory_slot {
#define KVM_DEV_ARM_VGIC_CPUID_MASK (0xfffffULL << 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_REG_MASK (KVM_REG_SIZE_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_GRP_NR_IRQS 3
#define KVM_DEV_ARM_VGIC_GRP_CTRL 4
#define KVM_DEV_ARM_VGIC_CTRL_INIT 0
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 9eeeb95..dbc5c49 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -259,8 +259,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)
/*
@@ -285,6 +291,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
diff --git a/virt/kvm/arm/vgic-v3-emul.c b/virt/kvm/arm/vgic-v3-emul.c
index 8bda714..2f1c27b 100644
--- a/virt/kvm/arm/vgic-v3-emul.c
+++ b/virt/kvm/arm/vgic-v3-emul.c
@@ -48,6 +48,7 @@
#include <asm/kvm_arm.h>
#include <asm/kvm_mmu.h>
+#include "sys_regs.h"
#include "vgic.h"
static bool handle_mmio_rao_wi(struct kvm_vcpu *vcpu,
@@ -991,6 +992,247 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
vgic_kick_vcpus(vcpu->kvm);
}
+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 (p->is_write) {
+ val = *p->val;
+
+ 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);
+
+ *p->val = val;
+ }
+
+ return true;
+}
+
+static bool access_gic_pmr(struct kvm_vcpu *vcpu,
+ const 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_PMR_MASK;
+ vgicv3->vgic_vmcr |= (*p->val << ICH_VMCR_PMR_SHIFT) &
+ ICH_VMCR_PMR_MASK;
+ } else {
+ *p->val = (vgicv3->vgic_vmcr & ICH_VMCR_PMR_MASK) >>
+ ICH_VMCR_PMR_SHIFT;
+ }
+
+ return true;
+}
+
+static bool access_gic_bpr0(struct kvm_vcpu *vcpu,
+ const 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_BPR0_MASK;
+ vgicv3->vgic_vmcr |= (*p->val << ICH_VMCR_BPR0_SHIFT) &
+ ICH_VMCR_BPR0_MASK;
+ } else {
+ *p->val = (vgicv3->vgic_vmcr & ICH_VMCR_BPR0_MASK) >>
+ ICH_VMCR_BPR0_SHIFT;
+ }
+
+ return true;
+}
+
+static bool access_gic_bpr1(struct kvm_vcpu *vcpu,
+ const 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_BPR1_MASK;
+ vgicv3->vgic_vmcr |= (*p->val << ICH_VMCR_BPR1_SHIFT) &
+ ICH_VMCR_BPR1_MASK;
+ } else {
+ *p->val = (vgicv3->vgic_vmcr & ICH_VMCR_BPR1_MASK) >>
+ ICH_VMCR_BPR1_SHIFT;
+ }
+
+ return true;
+}
+
+static bool access_gic_grpen0(struct kvm_vcpu *vcpu,
+ const 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->val << ICH_VMCR_ENG0_SHIFT) &
+ ICH_VMCR_ENG0;
+ } else {
+ *p->val = (vgicv3->vgic_vmcr & ICH_VMCR_ENG0) >>
+ ICH_VMCR_ENG0_SHIFT;
+ }
+
+ return true;
+}
+
+static bool access_gic_grpen1(struct kvm_vcpu *vcpu,
+ const 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->val << ICH_VMCR_ENG1_SHIFT) &
+ ICH_VMCR_ENG1;
+ } else {
+ *p->val = (vgicv3->vgic_vmcr & ICH_VMCR_ENG1) >>
+ ICH_VMCR_ENG1_SHIFT;
+ }
+
+ return true;
+}
+
+static bool access_gic_ap0r(struct kvm_vcpu *vcpu,
+ const 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->val;
+ else
+ *p->val = vgicv3->vgic_ap0r[idx];
+
+ return true;
+}
+
+static bool access_gic_ap1r(struct kvm_vcpu *vcpu,
+ const 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->val;
+ else
+ *p->val = 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 },
+};
+
+static int vgic_v3_cpu_regs_access(struct kvm_device *dev,
+ struct kvm_device_attr *attr,
+ bool is_write, int cpuid)
+{
+ u64 __user *uaddr = (u64 __user *)(long)attr->addr;
+ u64 id = attr->attr & KVM_DEV_ARM_VGIC_REG_MASK;
+ struct kvm_vcpu *vcpu;
+ struct sys_reg_params params;
+ u_long reg;
+ const struct sys_reg_desc *r;
+
+ params.val = ®
+ params.is_write = is_write;
+ params.is_aarch32 = false;
+ params.is_32bit = false;
+
+ r = find_reg_by_id(id, ¶ms, gic_v3_icc_reg_descs,
+ ARRAY_SIZE(gic_v3_icc_reg_descs));
+ if (!r)
+ return -ENXIO;
+
+ if (is_write) {
+ if (get_user(reg, uaddr))
+ return -EFAULT;
+ }
+
+ if (cpuid >= atomic_read(&dev->kvm->online_vcpus))
+ return -EINVAL;
+
+ vcpu = kvm_get_vcpu(dev->kvm, cpuid);
+ /* Ensure that VCPU is not running */
+ if (unlikely(vcpu->cpu != -1))
+ return -EBUSY;
+
+ if (!r->access(vcpu, ¶ms, r))
+ return -EINVAL;
+
+ if (!is_write)
+ return put_user(reg, uaddr);
+
+ return 0;
+}
+
static int vgic_v3_attr_regs_access(struct kvm_device *dev,
struct kvm_device_attr *attr,
bool is_write)
@@ -1015,6 +1257,8 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
mmio.phys_addr = vgic->vgic_redist_base;
ranges = vgic_redist_ranges;
break;
+ case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
+ return vgic_v3_cpu_regs_access(dev, attr, is_write, cpuid);
default:
return -ENXIO;
}
--
2.4.4
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3 5/5] KVM: arm64: Implement vGICv3 CPU interface access
2015-09-04 12:40 ` [PATCH v3 5/5] KVM: arm64: Implement vGICv3 CPU interface access Pavel Fedin
@ 2015-09-04 15:13 ` Andre Przywara
2015-09-04 15:40 ` Pavel Fedin
2015-09-24 12:08 ` Pavel Fedin
0 siblings, 2 replies; 21+ messages in thread
From: Andre Przywara @ 2015-09-04 15:13 UTC (permalink / raw)
To: Pavel Fedin
Cc: Marc Zyngier, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org
Hi Pavel,
On 04/09/15 13:40, Pavel Fedin wrote:
> The access is done similar to GICv2, using KVM_DEV_ARM_VGIC_GRP_CPU_REGS
> group, however attribute ID encodes corresponding system register. Access
> size is always 64 bits.
Why is this? Actually all registers in the CPU interface (except the w/o
SGI registers) are 32 bits and in the pending 32-bit GICv3 support
series[1] this is exploited by using MRC/MCR accesses.
The only thing that is pure 64-bit is the MRS/MSR _instruction_ in
Aarch64, which always takes a x<nn> register.
So can you model the register size according to the spec and allow
32-bit accesses from userland?
> Since CPU interface state actually affects only a single vCPU, no vGIC
> locking is done. Just made sure that the vCPU is not running.
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
> Documentation/virtual/kvm/devices/arm-vgic.txt | 38 +++-
> arch/arm64/include/uapi/asm/kvm.h | 7 +
> include/linux/irqchip/arm-gic-v3.h | 18 +-
> virt/kvm/arm/vgic-v3-emul.c | 244 +++++++++++++++++++++++++
> 4 files changed, 303 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic.txt b/Documentation/virtual/kvm/devices/arm-vgic.txt
> index 03f640f..518b634 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic.txt
> @@ -88,7 +88,7 @@ Groups:
> -ENODEV: Getting or setting this register is not yet supported
> -EBUSY: One or more VCPUs are running
>
> - KVM_DEV_ARM_VGIC_GRP_CPU_REGS
> + KVM_DEV_ARM_VGIC_GRP_CPU_REGS (vGICv2)
> Attributes:
> The attr field of kvm_device_attr encodes two values:
> bits: | 63 .... 52 | 51 .. 32 | 31 .... 0 |
> @@ -116,11 +116,45 @@ Groups:
>
> Limitations:
> - Priorities are not implemented, and registers are RAZ/WI
> - - Currently only implemented for KVM_DEV_TYPE_ARM_VGIC_V2.
> Errors:
> -ENODEV: Getting or setting this register is not yet supported
> -EBUSY: One or more VCPUs are running
>
> + KVM_DEV_ARM_VGIC_GRP_CPU_REGS (vGICv3)
> + Attributes:
> + The attr field of kvm_device_attr encodes the following values:
> + bits: | 63 .. 56 | 55 .. 48 | 47 ... 40 | 39 .. 32 | 31 .. 0 |
> + values: | arch | size | reserved | cpu id | reg id |
> +
> + All CPU interface regs are (rw, 64-bit). The only supported size value is
> + KVM_REG_SIZE_U64.
> +
> + Arch, size and reg id fields actually encode system register to be
> + accessed. Normally these values are obtained using ARM64_SYS_REG() macro.
> + Getting or setting such a register has the same effect as reading or
> + writing the register on the actual hardware.
> +
> + The Active Priorities Registers AP0Rn and AP1Rn are implementation defined,
> + so we set a fixed format for our implementation that fits with the model of
> + a "GICv3 implementation without the security extensions" which we present
> + to the guest. This interface always exposes four register APR[0-3]
> + describing the maximum possible 128 preemption levels. The semantics of the
> + register indicates if any interrupts in a given preemption level are in the
> + active state by setting the corresponding bit.
> +
> + Thus, preemption level X has one or more active interrupts if and only if:
> +
> + APRn[X mod 32] == 0b1, where n = X / 32
> +
> + Bits for undefined preemption levels are RAZ/WI.
> +
> + Limitations:
> + - Priorities are not implemented, and registers are RAZ/WI
> + Errors:
> + -ENODEV: Getting or setting this register is not yet supported
The code uses -ENXIO.
> + -EBUSY: One or more VCPUs are running
> +
> +
> KVM_DEV_ARM_VGIC_GRP_NR_IRQS
> Attributes:
> A value describing the number of interrupts (SGI, PPI and SPI) for
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 249954f..7d37ccd 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -201,6 +201,13 @@ struct kvm_arch_memory_slot {
> #define KVM_DEV_ARM_VGIC_CPUID_MASK (0xfffffULL << 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_REG_MASK (KVM_REG_SIZE_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_GRP_NR_IRQS 3
> #define KVM_DEV_ARM_VGIC_GRP_CTRL 4
> #define KVM_DEV_ARM_VGIC_CTRL_INIT 0
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index 9eeeb95..dbc5c49 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -259,8 +259,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)
>
> /*
> @@ -285,6 +291,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
> diff --git a/virt/kvm/arm/vgic-v3-emul.c b/virt/kvm/arm/vgic-v3-emul.c
> index 8bda714..2f1c27b 100644
> --- a/virt/kvm/arm/vgic-v3-emul.c
> +++ b/virt/kvm/arm/vgic-v3-emul.c
> @@ -48,6 +48,7 @@
> #include <asm/kvm_arm.h>
> #include <asm/kvm_mmu.h>
>
> +#include "sys_regs.h"
> #include "vgic.h"
>
> static bool handle_mmio_rao_wi(struct kvm_vcpu *vcpu,
> @@ -991,6 +992,247 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
> vgic_kick_vcpus(vcpu->kvm);
> }
>
> +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 (p->is_write) {
> + val = *p->val;
> +
> + 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);
> +
> + *p->val = val;
> + }
> +
> + return true;
> +}
> +
> +static bool access_gic_pmr(struct kvm_vcpu *vcpu,
> + const 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_PMR_MASK;
> + vgicv3->vgic_vmcr |= (*p->val << ICH_VMCR_PMR_SHIFT) &
> + ICH_VMCR_PMR_MASK;
> + } else {
> + *p->val = (vgicv3->vgic_vmcr & ICH_VMCR_PMR_MASK) >>
> + ICH_VMCR_PMR_SHIFT;
> + }
> +
> + return true;
> +}
> +
> +static bool access_gic_bpr0(struct kvm_vcpu *vcpu,
> + const 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_BPR0_MASK;
> + vgicv3->vgic_vmcr |= (*p->val << ICH_VMCR_BPR0_SHIFT) &
> + ICH_VMCR_BPR0_MASK;
> + } else {
> + *p->val = (vgicv3->vgic_vmcr & ICH_VMCR_BPR0_MASK) >>
> + ICH_VMCR_BPR0_SHIFT;
> + }
> +
> + return true;
> +}
> +
> +static bool access_gic_bpr1(struct kvm_vcpu *vcpu,
> + const 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_BPR1_MASK;
> + vgicv3->vgic_vmcr |= (*p->val << ICH_VMCR_BPR1_SHIFT) &
> + ICH_VMCR_BPR1_MASK;
> + } else {
> + *p->val = (vgicv3->vgic_vmcr & ICH_VMCR_BPR1_MASK) >>
> + ICH_VMCR_BPR1_SHIFT;
> + }
> +
> + return true;
> +}
> +
> +static bool access_gic_grpen0(struct kvm_vcpu *vcpu,
> + const 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->val << ICH_VMCR_ENG0_SHIFT) &
> + ICH_VMCR_ENG0;
> + } else {
> + *p->val = (vgicv3->vgic_vmcr & ICH_VMCR_ENG0) >>
> + ICH_VMCR_ENG0_SHIFT;
> + }
> +
> + return true;
> +}
> +
> +static bool access_gic_grpen1(struct kvm_vcpu *vcpu,
> + const 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->val << ICH_VMCR_ENG1_SHIFT) &
> + ICH_VMCR_ENG1;
> + } else {
> + *p->val = (vgicv3->vgic_vmcr & ICH_VMCR_ENG1) >>
> + ICH_VMCR_ENG1_SHIFT;
> + }
> +
> + return true;
> +}
I wonder if the 5 functions above could be merged to have only one
implementation and 5 wrappers, since they are so similar.
Cheers,
Andre.
[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/327034.html
> +
> +static bool access_gic_ap0r(struct kvm_vcpu *vcpu,
> + const 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->val;
> + else
> + *p->val = vgicv3->vgic_ap0r[idx];
> +
> + return true;
> +}
> +
> +static bool access_gic_ap1r(struct kvm_vcpu *vcpu,
> + const 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->val;
> + else
> + *p->val = 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 },
> +};
> +
> +static int vgic_v3_cpu_regs_access(struct kvm_device *dev,
> + struct kvm_device_attr *attr,
> + bool is_write, int cpuid)
> +{
> + u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> + u64 id = attr->attr & KVM_DEV_ARM_VGIC_REG_MASK;
> + struct kvm_vcpu *vcpu;
> + struct sys_reg_params params;
> + u_long reg;
> + const struct sys_reg_desc *r;
> +
> + params.val = ®
> + params.is_write = is_write;
> + params.is_aarch32 = false;
> + params.is_32bit = false;
> +
> + r = find_reg_by_id(id, ¶ms, gic_v3_icc_reg_descs,
> + ARRAY_SIZE(gic_v3_icc_reg_descs));
> + if (!r)
> + return -ENXIO;
> +
> + if (is_write) {
> + if (get_user(reg, uaddr))
> + return -EFAULT;
> + }
> +
> + if (cpuid >= atomic_read(&dev->kvm->online_vcpus))
> + return -EINVAL;
> +
> + vcpu = kvm_get_vcpu(dev->kvm, cpuid);
> + /* Ensure that VCPU is not running */
> + if (unlikely(vcpu->cpu != -1))
> + return -EBUSY;
> +
> + if (!r->access(vcpu, ¶ms, r))
> + return -EINVAL;
> +
> + if (!is_write)
> + return put_user(reg, uaddr);
> +
> + return 0;
> +}
> +
> static int vgic_v3_attr_regs_access(struct kvm_device *dev,
> struct kvm_device_attr *attr,
> bool is_write)
> @@ -1015,6 +1257,8 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
> mmio.phys_addr = vgic->vgic_redist_base;
> ranges = vgic_redist_ranges;
> break;
> + case KVM_DEV_ARM_VGIC_GRP_CPU_REGS:
> + return vgic_v3_cpu_regs_access(dev, attr, is_write, cpuid);
> default:
> return -ENXIO;
> }
> --
> 2.4.4
>
^ permalink raw reply [flat|nested] 21+ messages in thread* RE: [PATCH v3 5/5] KVM: arm64: Implement vGICv3 CPU interface access
2015-09-04 15:13 ` Andre Przywara
@ 2015-09-04 15:40 ` Pavel Fedin
2015-09-24 12:08 ` Pavel Fedin
1 sibling, 0 replies; 21+ messages in thread
From: Pavel Fedin @ 2015-09-04 15:40 UTC (permalink / raw)
To: 'Andre Przywara'; +Cc: 'Marc Zyngier', kvmarm, kvm
Hello!
> So can you model the register size according to the spec and allow
> 32-bit accesses from userland?
I can. But i'll have to invent my own macro for encoding register IDs into the attribute, as well
as drop reusing index_to_params(). Will it be OK?
Upside: i can further extend "cpu id" (actually CPU index) field to 31 bit. :)
It's friday evening here, work week is almost over, i'll read your replies 2 days later on monday.
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH v3 5/5] KVM: arm64: Implement vGICv3 CPU interface access
2015-09-04 15:13 ` Andre Przywara
2015-09-04 15:40 ` Pavel Fedin
@ 2015-09-24 12:08 ` Pavel Fedin
2015-09-25 22:27 ` Andre Przywara
1 sibling, 1 reply; 21+ messages in thread
From: Pavel Fedin @ 2015-09-24 12:08 UTC (permalink / raw)
To: 'Andre Przywara'
Cc: kvmarm, kvm, 'Marc Zyngier', 'Peter Maydell'
Hello!
> The only thing that is pure 64-bit is the MRS/MSR _instruction_ in
> Aarch64, which always takes a x<nn> register.
> So can you model the register size according to the spec and allow
> 32-bit accesses from userland?
I would like to complete the rework and respin v4, but this is, i guess, the only major issue left.
Additionally, it impacts the API. So...
In order to allow 32-bit accesses we would have to drop using ARM64_SYS_REG() for building
attribute ID and introduce something own, like KVM_DEV_ARM_VGIC_REG(). It will have different bits
layout (actually it will be missing 'arch' and 'size' field, and instead i will use
KVM_DEV_ARM_VGIC_64BIT flag for length specification, the same as for redistributor.
Will this be OK ?
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 5/5] KVM: arm64: Implement vGICv3 CPU interface access
2015-09-24 12:08 ` Pavel Fedin
@ 2015-09-25 22:27 ` Andre Przywara
2015-09-25 22:33 ` Peter Maydell
2015-09-28 15:29 ` Pavel Fedin
0 siblings, 2 replies; 21+ messages in thread
From: Andre Przywara @ 2015-09-25 22:27 UTC (permalink / raw)
To: Pavel Fedin
Cc: kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, Marc Zyngier,
'Peter Maydell'
On 24/09/15 13:08, Pavel Fedin wrote:
> Hello!
>
>> The only thing that is pure 64-bit is the MRS/MSR _instruction_ in
>> Aarch64, which always takes a x<nn> register.
>> So can you model the register size according to the spec and allow
>> 32-bit accesses from userland?
>
> I would like to complete the rework and respin v4, but this is, i guess, the only major issue left.
> Additionally, it impacts the API. So...
> In order to allow 32-bit accesses we would have to drop using ARM64_SYS_REG() for building
> attribute ID and introduce something own, like KVM_DEV_ARM_VGIC_REG(). It will have different bits
> layout (actually it will be missing 'arch' and 'size' field, and instead i will use
> KVM_DEV_ARM_VGIC_64BIT flag for length specification, the same as for redistributor.
> Will this be OK ?
No, instead you should go with your original approach ;-)
Thinking about that again I see that this interface is of course modeled
after the architectured GICv3 system registers, where AArch32 has its
own, separate encoding. So it's perfectly fine to use that 64-bit
interface between userland and KVM now. If we later get Aarch32 support
for the GICv3, we can add the appropriate Aarch32 sysregs to that
interface and have a natural match.
So: sorry for the noise, you can just go ahead with that native 64-bit
sysregs encoding for [SG]ET_ONE_REG as you had before.
Cheers,
Andre.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 5/5] KVM: arm64: Implement vGICv3 CPU interface access
2015-09-25 22:27 ` Andre Przywara
@ 2015-09-25 22:33 ` Peter Maydell
2015-09-25 23:00 ` Marc Zyngier
2015-09-28 15:29 ` Pavel Fedin
1 sibling, 1 reply; 21+ messages in thread
From: Peter Maydell @ 2015-09-25 22:33 UTC (permalink / raw)
To: Andre Przywara
Cc: Pavel Fedin, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
Marc Zyngier, Christoffer Dall
On 25 September 2015 at 15:27, Andre Przywara <andre.przywara@arm.com> wrote:
> On 24/09/15 13:08, Pavel Fedin wrote:
>> Hello!
>>
>>> The only thing that is pure 64-bit is the MRS/MSR _instruction_ in
>>> Aarch64, which always takes a x<nn> register.
>>> So can you model the register size according to the spec and allow
>>> 32-bit accesses from userland?
>>
>> I would like to complete the rework and respin v4, but this is, i guess, the only major issue left.
>> Additionally, it impacts the API. So...
>> In order to allow 32-bit accesses we would have to drop using ARM64_SYS_REG() for building
>> attribute ID and introduce something own, like KVM_DEV_ARM_VGIC_REG(). It will have different bits
>> layout (actually it will be missing 'arch' and 'size' field, and instead i will use
>> KVM_DEV_ARM_VGIC_64BIT flag for length specification, the same as for redistributor.
>> Will this be OK ?
>
> No, instead you should go with your original approach ;-)
> Thinking about that again I see that this interface is of course modeled
> after the architectured GICv3 system registers, where AArch32 has its
> own, separate encoding. So it's perfectly fine to use that 64-bit
> interface between userland and KVM now. If we later get Aarch32 support
> for the GICv3, we can add the appropriate Aarch32 sysregs to that
> interface and have a natural match.
Christoffer, Marc and I had a discussion about how best to handle GICv3
state save and restore here at Linaro Connect this week, and one of the
conclusions there was that all of it should be done via the device
fd's save/restore interface, not via the SET/GET_ONE_REG CPU register
interface. (Because it's much less confusing to have all of the GICv3's
state be dealt with via the same interface, rather than splitting it;
it's also more convenient for QEMU.)
More details to follow from one of us next week when we aren't all
on aeroplanes, I expect.
thanks
-- PMM
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 5/5] KVM: arm64: Implement vGICv3 CPU interface access
2015-09-25 22:33 ` Peter Maydell
@ 2015-09-25 23:00 ` Marc Zyngier
0 siblings, 0 replies; 21+ messages in thread
From: Marc Zyngier @ 2015-09-25 23:00 UTC (permalink / raw)
To: Peter Maydell
Cc: Andre Przywara, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org
On Fri, 25 Sep 2015 15:33:44 -0700
Peter Maydell <peter.maydell@linaro.org> wrote:
> On 25 September 2015 at 15:27, Andre Przywara <andre.przywara@arm.com> wrote:
> > On 24/09/15 13:08, Pavel Fedin wrote:
> >> Hello!
> >>
> >>> The only thing that is pure 64-bit is the MRS/MSR _instruction_ in
> >>> Aarch64, which always takes a x<nn> register.
> >>> So can you model the register size according to the spec and allow
> >>> 32-bit accesses from userland?
> >>
> >> I would like to complete the rework and respin v4, but this is, i guess, the only major issue left.
> >> Additionally, it impacts the API. So...
> >> In order to allow 32-bit accesses we would have to drop using ARM64_SYS_REG() for building
> >> attribute ID and introduce something own, like KVM_DEV_ARM_VGIC_REG(). It will have different bits
> >> layout (actually it will be missing 'arch' and 'size' field, and instead i will use
> >> KVM_DEV_ARM_VGIC_64BIT flag for length specification, the same as for redistributor.
> >> Will this be OK ?
> >
> > No, instead you should go with your original approach ;-)
> > Thinking about that again I see that this interface is of course modeled
> > after the architectured GICv3 system registers, where AArch32 has its
> > own, separate encoding. So it's perfectly fine to use that 64-bit
> > interface between userland and KVM now. If we later get Aarch32 support
> > for the GICv3, we can add the appropriate Aarch32 sysregs to that
> > interface and have a natural match.
>
> Christoffer, Marc and I had a discussion about how best to handle GICv3
> state save and restore here at Linaro Connect this week, and one of the
> conclusions there was that all of it should be done via the device
> fd's save/restore interface, not via the SET/GET_ONE_REG CPU register
> interface. (Because it's much less confusing to have all of the GICv3's
> state be dealt with via the same interface, rather than splitting it;
> it's also more convenient for QEMU.)
>
> More details to follow from one of us next week when we aren't all
> on aeroplanes, I expect.
Indeed. Another thing to consider now is (at the ITS level) the
necessity to be able to flush the ITS "caches" (basically our internal
data structures) out to guest memory so that it can be saved/restored.
That's pretty easy for the pending and property tables as well as
the command queue (where the format is fixed by the architecture), but
slightly more complicated with the device table, the collection table
and the various ITTs (we need to come up with a formal representation
that effectively becomes an ABI).
>From a flow point of view, I expect the flush to at least occur when
the VM gets suspended, ensuring that the full interrupt state can be
read out via the userspace mapping of the guest memory.
That being said, I'm off to catch that plane.
Thanks,
M.
--
Jazz is not dead. It just smells funny.
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH v3 5/5] KVM: arm64: Implement vGICv3 CPU interface access
2015-09-25 22:27 ` Andre Przywara
2015-09-25 22:33 ` Peter Maydell
@ 2015-09-28 15:29 ` Pavel Fedin
1 sibling, 0 replies; 21+ messages in thread
From: Pavel Fedin @ 2015-09-28 15:29 UTC (permalink / raw)
To: 'Andre Przywara'
Cc: kvmarm, kvm, 'Marc Zyngier', 'Peter Maydell'
Hello!
> So: sorry for the noise, you can just go ahead with that native 64-bit
> sysregs encoding for [SG]ET_ONE_REG as you had before.
Ok, good. Take v4 then. Some issues you've commented on were fixed, some other things were left as
they are (i replied to your comments, why). Let's move on. :)
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
^ permalink raw reply [flat|nested] 21+ messages in thread