* [RFC PATCH v3 1/5] arm/arm64: vgic-new: Implement support for userspace access
2016-08-24 11:20 [RFC PATCH v3 0/5] arm/arm64: vgic-new: Implement API for vGICv3 live migration vijay.kilari at gmail.com
@ 2016-08-24 11:20 ` vijay.kilari at gmail.com
2016-08-30 10:31 ` Christoffer Dall
2016-08-30 10:50 ` Christoffer Dall
2016-08-24 11:20 ` [RFC PATCH v3 2/5] arm/arm64: vgic-new: Add distributor and redistributor access vijay.kilari at gmail.com
` (3 subsequent siblings)
4 siblings, 2 replies; 23+ messages in thread
From: vijay.kilari at gmail.com @ 2016-08-24 11:20 UTC (permalink / raw)
To: linux-arm-kernel
From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
Read and write of some registers like ISPENDR and ICPENDR
from userspace requires special handling when compared to
guest access for these registers.
Refer to Documentation/virtual/kvm/devices/arm-vgic-its.txt
for handling of ISPENDR, ICPENDR registers handling.
Add infrastructure to support guest and userspace read
and write for the required registers
Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
---
virt/kvm/arm/vgic/vgic-mmio-v2.c | 5 ++-
virt/kvm/arm/vgic/vgic-mmio-v3.c | 40 ++++++++++++++----
virt/kvm/arm/vgic/vgic-mmio.c | 87 ++++++++++++++++++++++++++++++++++++----
virt/kvm/arm/vgic/vgic-mmio.h | 25 ++++++++++++
4 files changed, 139 insertions(+), 18 deletions(-)
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index b44b359..cd37159 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -421,9 +421,10 @@ static int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
if (is_write) {
vgic_data_host_to_mmio_bus(buf, len, *val);
- ret = kvm_io_gic_ops.write(vcpu, &dev->dev, offset, len, buf);
+ ret = vgic_mmio_uaccess_write(vcpu, &dev->dev, offset,
+ len, buf);
} else {
- ret = kvm_io_gic_ops.read(vcpu, &dev->dev, offset, len, buf);
+ ret = vgic_mmio_uaccess_read(vcpu, &dev->dev, offset, len, buf);
if (!ret)
*val = vgic_data_mmio_bus_to_host(buf, len);
}
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index ff668e0..dd0d602 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -367,6 +367,26 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
.write = wr, \
}
+#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED_UACCESS(off, rd, wr, ur, \
+ uw, bpi, acc) \
+ { \
+ .reg_offset = off, \
+ .bits_per_irq = bpi, \
+ .len = (bpi * VGIC_NR_PRIVATE_IRQS) / 8, \
+ .access_flags = acc, \
+ .read = vgic_mmio_read_raz, \
+ .write = vgic_mmio_write_wi, \
+ }, { \
+ .reg_offset = off + (bpi * VGIC_NR_PRIVATE_IRQS) / 8, \
+ .bits_per_irq = bpi, \
+ .len = (bpi * (1024 - VGIC_NR_PRIVATE_IRQS)) / 8, \
+ .access_flags = acc, \
+ .read = rd, \
+ .write = wr, \
+ .uaccess_read = ur, \
+ .uaccess_write = uw, \
+ }
+
static const struct vgic_register_region vgic_v3_dist_registers[] = {
REGISTER_DESC_WITH_LENGTH(GICD_CTLR,
vgic_mmio_read_v3_misc, vgic_mmio_write_v3_misc, 16,
@@ -380,11 +400,13 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICENABLER,
vgic_mmio_read_enable, vgic_mmio_write_cenable, 1,
VGIC_ACCESS_32bit),
- REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISPENDR,
- vgic_mmio_read_pending, vgic_mmio_write_spending, 1,
+ REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED_UACCESS(GICD_ISPENDR,
+ vgic_mmio_read_pending, vgic_mmio_write_spending,
+ vgic_mmio_read_soft_pending, vgic_mmio_write_spending, 1,
VGIC_ACCESS_32bit),
- REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICPENDR,
- vgic_mmio_read_pending, vgic_mmio_write_cpending, 1,
+ REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED_UACCESS(GICD_ICPENDR,
+ vgic_mmio_read_pending, vgic_mmio_write_cpending,
+ vgic_mmio_read_soft_pending, vgic_mmio_write_cpending, 1,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
vgic_mmio_read_active, vgic_mmio_write_sactive, 1,
@@ -443,11 +465,13 @@ static const struct vgic_register_region vgic_v3_sgibase_registers[] = {
REGISTER_DESC_WITH_LENGTH(GICR_ICENABLER0,
vgic_mmio_read_enable, vgic_mmio_write_cenable, 4,
VGIC_ACCESS_32bit),
- REGISTER_DESC_WITH_LENGTH(GICR_ISPENDR0,
- vgic_mmio_read_pending, vgic_mmio_write_spending, 4,
+ REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_ISPENDR0,
+ vgic_mmio_read_pending, vgic_mmio_write_spending,
+ vgic_mmio_read_soft_pending, vgic_mmio_write_spending, 4,
VGIC_ACCESS_32bit),
- REGISTER_DESC_WITH_LENGTH(GICR_ICPENDR0,
- vgic_mmio_read_pending, vgic_mmio_write_cpending, 4,
+ REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_ICPENDR0,
+ vgic_mmio_read_pending, vgic_mmio_write_cpending,
+ vgic_mmio_read_soft_pending, vgic_mmio_write_cpending, 4,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_LENGTH(GICR_ISACTIVER0,
vgic_mmio_read_active, vgic_mmio_write_sactive, 4,
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index 3bad3c5..dcf5d25 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -100,6 +100,26 @@ void vgic_mmio_write_cenable(struct kvm_vcpu *vcpu,
}
}
+unsigned long vgic_mmio_read_soft_pending(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len)
+{
+ u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
+ u32 value = 0;
+ int i;
+
+ /* Loop over all IRQs affected by this read */
+ for (i = 0; i < len * 8; i++) {
+ struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
+
+ if (irq->config == VGIC_CONFIG_LEVEL && irq->soft_pending)
+ value |= (1U << i);
+ if (irq->config != VGIC_CONFIG_LEVEL && irq->pending)
+ value |= (1U << i);
+ }
+
+ return value;
+}
+
unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len)
{
@@ -468,6 +488,62 @@ static bool check_region(const struct vgic_register_region *region,
return false;
}
+static const struct vgic_register_region *
+ vgic_get_mmio_region(struct vgic_io_device *iodev, gpa_t addr, int len)
+{
+ const struct vgic_register_region *region;
+
+ region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
+ addr - iodev->base_addr);
+ if (!region || !check_region(region, addr, len))
+ return NULL;
+
+ return region;
+}
+
+int vgic_mmio_uaccess_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
+ gpa_t addr, int len, void *val)
+{
+ struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev);
+ const struct vgic_register_region *region;
+ struct kvm_vcpu *r_vcpu;
+ unsigned long data;
+
+ region = vgic_get_mmio_region(iodev, addr, len);
+ if (!region) {
+ memset(val, 0, len);
+ return 0;
+ }
+
+ r_vcpu = iodev->redist_vcpu ? iodev->redist_vcpu : vcpu;
+ if (region->uaccess_read != NULL)
+ data = region->uaccess_read(r_vcpu, addr, len);
+ else
+ data = region->read(r_vcpu, addr, len);
+ vgic_data_host_to_mmio_bus(val, len, data);
+ return 0;
+}
+
+int vgic_mmio_uaccess_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
+ gpa_t addr, int len, const void *val)
+{
+ struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev);
+ const struct vgic_register_region *region;
+ struct kvm_vcpu *r_vcpu;
+ unsigned long data = vgic_data_mmio_bus_to_host(val, len);
+
+ region = vgic_get_mmio_region(iodev, addr, len);
+ if (!region)
+ return 0;
+
+ r_vcpu = iodev->redist_vcpu ? iodev->redist_vcpu : vcpu;
+ if (region->uaccess_write != NULL)
+ region->uaccess_write(r_vcpu, addr, len, data);
+ else
+ region->write(r_vcpu, addr, len, data);
+ return 0;
+}
+
static int dispatch_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
gpa_t addr, int len, void *val)
{
@@ -475,9 +551,8 @@ static int dispatch_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
const struct vgic_register_region *region;
unsigned long data = 0;
- region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
- addr - iodev->base_addr);
- if (!region || !check_region(region, addr, len)) {
+ region = vgic_get_mmio_region(iodev, addr, len);
+ if (!region) {
memset(val, 0, len);
return 0;
}
@@ -508,14 +583,10 @@ static int dispatch_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
const struct vgic_register_region *region;
unsigned long data = vgic_data_mmio_bus_to_host(val, len);
- region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
- addr - iodev->base_addr);
+ region = vgic_get_mmio_region(iodev, addr, len);
if (!region)
return 0;
- if (!check_region(region, addr, len))
- return 0;
-
switch (iodev->iodev_type) {
case IODEV_CPUIF:
region->write(vcpu, addr, len, data);
diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
index 0b3ecf9..b97a97b 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.h
+++ b/virt/kvm/arm/vgic/vgic-mmio.h
@@ -34,6 +34,10 @@ struct vgic_register_region {
gpa_t addr, unsigned int len,
unsigned long val);
};
+ unsigned long (*uaccess_read)(struct kvm_vcpu *vcpu, gpa_t addr,
+ unsigned int len);
+ void (*uaccess_write)(struct kvm_vcpu *vcpu, gpa_t addr,
+ unsigned int len, unsigned long val);
};
extern struct kvm_io_device_ops kvm_io_gic_ops;
@@ -86,6 +90,18 @@ extern struct kvm_io_device_ops kvm_io_gic_ops;
.write = wr, \
}
+#define REGISTER_DESC_WITH_LENGTH_UACCESS(off, rd, wr, urd, uwr, length, acc) \
+ { \
+ .reg_offset = off, \
+ .bits_per_irq = 0, \
+ .len = length, \
+ .access_flags = acc, \
+ .read = rd, \
+ .write = wr, \
+ .uaccess_read = urd, \
+ .uaccess_write = uwr, \
+ }
+
int kvm_vgic_register_mmio_region(struct kvm *kvm, struct kvm_vcpu *vcpu,
struct vgic_register_region *reg_desc,
struct vgic_io_device *region,
@@ -122,6 +138,9 @@ void vgic_mmio_write_cenable(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len,
unsigned long val);
+unsigned long vgic_mmio_read_soft_pending(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len);
+
unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len);
@@ -158,6 +177,12 @@ void vgic_mmio_write_config(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len,
unsigned long val);
+int vgic_mmio_uaccess_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
+ gpa_t addr, int len, void *val);
+
+int vgic_mmio_uaccess_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
+ gpa_t addr, int len, const void *val);
+
unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev);
unsigned int vgic_v3_init_dist_iodev(struct vgic_io_device *dev);
--
1.9.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [RFC PATCH v3 1/5] arm/arm64: vgic-new: Implement support for userspace access
2016-08-24 11:20 ` [RFC PATCH v3 1/5] arm/arm64: vgic-new: Implement support for userspace access vijay.kilari at gmail.com
@ 2016-08-30 10:31 ` Christoffer Dall
2016-08-30 10:49 ` Christoffer Dall
2016-08-30 10:50 ` Christoffer Dall
1 sibling, 1 reply; 23+ messages in thread
From: Christoffer Dall @ 2016-08-30 10:31 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 24, 2016 at 04:50:05PM +0530, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> Read and write of some registers like ISPENDR and ICPENDR
> from userspace requires special handling when compared to
> guest access for these registers.
>
> Refer to Documentation/virtual/kvm/devices/arm-vgic-its.txt
you mean arm-vgic-v3.txt right?
> for handling of ISPENDR, ICPENDR registers handling.
>
> Add infrastructure to support guest and userspace read
> and write for the required registers
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
> virt/kvm/arm/vgic/vgic-mmio-v2.c | 5 ++-
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 40 ++++++++++++++----
> virt/kvm/arm/vgic/vgic-mmio.c | 87 ++++++++++++++++++++++++++++++++++++----
> virt/kvm/arm/vgic/vgic-mmio.h | 25 ++++++++++++
> 4 files changed, 139 insertions(+), 18 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index b44b359..cd37159 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -421,9 +421,10 @@ static int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
>
> if (is_write) {
> vgic_data_host_to_mmio_bus(buf, len, *val);
> - ret = kvm_io_gic_ops.write(vcpu, &dev->dev, offset, len, buf);
> + ret = vgic_mmio_uaccess_write(vcpu, &dev->dev, offset,
> + len, buf);
> } else {
> - ret = kvm_io_gic_ops.read(vcpu, &dev->dev, offset, len, buf);
> + ret = vgic_mmio_uaccess_read(vcpu, &dev->dev, offset, len, buf);
> if (!ret)
> *val = vgic_data_mmio_bus_to_host(buf, len);
> }
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index ff668e0..dd0d602 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -367,6 +367,26 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
> .write = wr, \
> }
>
> +#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED_UACCESS(off, rd, wr, ur, \
> + uw, bpi, acc) \
> + { \
> + .reg_offset = off, \
> + .bits_per_irq = bpi, \
> + .len = (bpi * VGIC_NR_PRIVATE_IRQS) / 8, \
> + .access_flags = acc, \
> + .read = vgic_mmio_read_raz, \
> + .write = vgic_mmio_write_wi, \
> + }, { \
> + .reg_offset = off + (bpi * VGIC_NR_PRIVATE_IRQS) / 8, \
> + .bits_per_irq = bpi, \
> + .len = (bpi * (1024 - VGIC_NR_PRIVATE_IRQS)) / 8, \
> + .access_flags = acc, \
> + .read = rd, \
> + .write = wr, \
> + .uaccess_read = ur, \
> + .uaccess_write = uw, \
> + }
> +
that macro name is getting really long and complicated.
Could we consider simply modifying the existing macro to take two
additional parameters, and change the list of users of the macro to pass
NULL where these functions are not used?
> static const struct vgic_register_region vgic_v3_dist_registers[] = {
> REGISTER_DESC_WITH_LENGTH(GICD_CTLR,
> vgic_mmio_read_v3_misc, vgic_mmio_write_v3_misc, 16,
> @@ -380,11 +400,13 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICENABLER,
> vgic_mmio_read_enable, vgic_mmio_write_cenable, 1,
> VGIC_ACCESS_32bit),
> - REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISPENDR,
> - vgic_mmio_read_pending, vgic_mmio_write_spending, 1,
> + REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED_UACCESS(GICD_ISPENDR,
> + vgic_mmio_read_pending, vgic_mmio_write_spending,
> + vgic_mmio_read_soft_pending, vgic_mmio_write_spending, 1,
You need a uaccess for the write part as well to provide raw access to
the latch state, without imposing any ordering requirements for the
restore part of userspace. For example, you cannot rely on userspace
having restored the configuration state of the IRQs before the pending
state, unless we modify the API to require this.
I think you need a function that looks very similar tot he
write_spending function, but which always sets the soft_pending state,
regardless of the configuration of the IRQ.
We need to tweak the vgic_mmio_write_config function to queue interrupts
that become pending when changed to LEVEL to go along with this. I can
send a patch with this separately.
> VGIC_ACCESS_32bit),
> - REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICPENDR,
> - vgic_mmio_read_pending, vgic_mmio_write_cpending, 1,
> + REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED_UACCESS(GICD_ICPENDR,
> + vgic_mmio_read_pending, vgic_mmio_write_cpending,
> + vgic_mmio_read_soft_pending, vgic_mmio_write_cpending, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
> vgic_mmio_read_active, vgic_mmio_write_sactive, 1,
> @@ -443,11 +465,13 @@ static const struct vgic_register_region vgic_v3_sgibase_registers[] = {
> REGISTER_DESC_WITH_LENGTH(GICR_ICENABLER0,
> vgic_mmio_read_enable, vgic_mmio_write_cenable, 4,
> VGIC_ACCESS_32bit),
> - REGISTER_DESC_WITH_LENGTH(GICR_ISPENDR0,
> - vgic_mmio_read_pending, vgic_mmio_write_spending, 4,
> + REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_ISPENDR0,
> + vgic_mmio_read_pending, vgic_mmio_write_spending,
> + vgic_mmio_read_soft_pending, vgic_mmio_write_spending, 4,
> VGIC_ACCESS_32bit),
> - REGISTER_DESC_WITH_LENGTH(GICR_ICPENDR0,
> - vgic_mmio_read_pending, vgic_mmio_write_cpending, 4,
> + REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_ICPENDR0,
> + vgic_mmio_read_pending, vgic_mmio_write_cpending,
> + vgic_mmio_read_soft_pending, vgic_mmio_write_cpending, 4,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_LENGTH(GICR_ISACTIVER0,
> vgic_mmio_read_active, vgic_mmio_write_sactive, 4,
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index 3bad3c5..dcf5d25 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -100,6 +100,26 @@ void vgic_mmio_write_cenable(struct kvm_vcpu *vcpu,
> }
> }
>
> +unsigned long vgic_mmio_read_soft_pending(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len)
since this is only reading the soft_pending state for level triggered
interrupts, I don't appreciate this name.
How about vgic_uaccess_read_pending ?
> +{
> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> + u32 value = 0;
> + int i;
> +
> + /* Loop over all IRQs affected by this read */
> + for (i = 0; i < len * 8; i++) {
> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> +
Add a comment with a reference to the logic defined in the
arm-vgic-v3.txt for this implementation here.
> + if (irq->config == VGIC_CONFIG_LEVEL && irq->soft_pending)
> + value |= (1U << i);
> + if (irq->config != VGIC_CONFIG_LEVEL && irq->pending)
please change the latter to irq->config == VGIC_CONFIG_EDGE.
> + value |= (1U << i);
> + }
> +
> + return value;
> +}
> +
> unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len)
> {
> @@ -468,6 +488,62 @@ static bool check_region(const struct vgic_register_region *region,
> return false;
> }
>
> +static const struct vgic_register_region *
> + vgic_get_mmio_region(struct vgic_io_device *iodev, gpa_t addr, int len)
> +{
> + const struct vgic_register_region *region;
> +
> + region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
> + addr - iodev->base_addr);
> + if (!region || !check_region(region, addr, len))
> + return NULL;
> +
> + return region;
> +}
> +
> +int vgic_mmio_uaccess_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> + gpa_t addr, int len, void *val)
I would probably rename these two functions to be
vgic_uaccess_read/write, that is get rid of the 'mmio' part.
> +{
> + struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev);
> + const struct vgic_register_region *region;
> + struct kvm_vcpu *r_vcpu;
> + unsigned long data;
> +
> + region = vgic_get_mmio_region(iodev, addr, len);
> + if (!region) {
> + memset(val, 0, len);
> + return 0;
> + }
> +
> + r_vcpu = iodev->redist_vcpu ? iodev->redist_vcpu : vcpu;
> + if (region->uaccess_read != NULL)
> + data = region->uaccess_read(r_vcpu, addr, len);
> + else
> + data = region->read(r_vcpu, addr, len);
> + vgic_data_host_to_mmio_bus(val, len, data);
> + return 0;
> +}
> +
> +int vgic_mmio_uaccess_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> + gpa_t addr, int len, const void *val)
> +{
> + struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev);
> + const struct vgic_register_region *region;
> + struct kvm_vcpu *r_vcpu;
> + unsigned long data = vgic_data_mmio_bus_to_host(val, len);
> +
> + region = vgic_get_mmio_region(iodev, addr, len);
> + if (!region)
> + return 0;
> +
> + r_vcpu = iodev->redist_vcpu ? iodev->redist_vcpu : vcpu;
> + if (region->uaccess_write != NULL)
> + region->uaccess_write(r_vcpu, addr, len, data);
> + else
> + region->write(r_vcpu, addr, len, data);
> + return 0;
> +}
> +
> static int dispatch_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> gpa_t addr, int len, void *val)
> {
> @@ -475,9 +551,8 @@ static int dispatch_mmio_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> const struct vgic_register_region *region;
> unsigned long data = 0;
>
> - region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
> - addr - iodev->base_addr);
> - if (!region || !check_region(region, addr, len)) {
> + region = vgic_get_mmio_region(iodev, addr, len);
> + if (!region) {
> memset(val, 0, len);
> return 0;
> }
> @@ -508,14 +583,10 @@ static int dispatch_mmio_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> const struct vgic_register_region *region;
> unsigned long data = vgic_data_mmio_bus_to_host(val, len);
>
> - region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
> - addr - iodev->base_addr);
> + region = vgic_get_mmio_region(iodev, addr, len);
> if (!region)
> return 0;
>
> - if (!check_region(region, addr, len))
> - return 0;
> -
> switch (iodev->iodev_type) {
> case IODEV_CPUIF:
> region->write(vcpu, addr, len, data);
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
> index 0b3ecf9..b97a97b 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.h
> +++ b/virt/kvm/arm/vgic/vgic-mmio.h
> @@ -34,6 +34,10 @@ struct vgic_register_region {
> gpa_t addr, unsigned int len,
> unsigned long val);
> };
> + unsigned long (*uaccess_read)(struct kvm_vcpu *vcpu, gpa_t addr,
> + unsigned int len);
> + void (*uaccess_write)(struct kvm_vcpu *vcpu, gpa_t addr,
> + unsigned int len, unsigned long val);
> };
>
> extern struct kvm_io_device_ops kvm_io_gic_ops;
> @@ -86,6 +90,18 @@ extern struct kvm_io_device_ops kvm_io_gic_ops;
> .write = wr, \
> }
>
> +#define REGISTER_DESC_WITH_LENGTH_UACCESS(off, rd, wr, urd, uwr, length, acc) \
> + { \
> + .reg_offset = off, \
> + .bits_per_irq = 0, \
> + .len = length, \
> + .access_flags = acc, \
> + .read = rd, \
> + .write = wr, \
> + .uaccess_read = urd, \
> + .uaccess_write = uwr, \
> + }
> +
> int kvm_vgic_register_mmio_region(struct kvm *kvm, struct kvm_vcpu *vcpu,
> struct vgic_register_region *reg_desc,
> struct vgic_io_device *region,
> @@ -122,6 +138,9 @@ void vgic_mmio_write_cenable(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val);
>
> +unsigned long vgic_mmio_read_soft_pending(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len);
> +
> unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len);
>
> @@ -158,6 +177,12 @@ void vgic_mmio_write_config(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val);
>
> +int vgic_mmio_uaccess_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> + gpa_t addr, int len, void *val);
> +
> +int vgic_mmio_uaccess_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> + gpa_t addr, int len, const void *val);
> +
> unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev);
>
> unsigned int vgic_v3_init_dist_iodev(struct vgic_io_device *dev);
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 23+ messages in thread* [RFC PATCH v3 1/5] arm/arm64: vgic-new: Implement support for userspace access
2016-08-30 10:31 ` Christoffer Dall
@ 2016-08-30 10:49 ` Christoffer Dall
0 siblings, 0 replies; 23+ messages in thread
From: Christoffer Dall @ 2016-08-30 10:49 UTC (permalink / raw)
To: linux-arm-kernel
[replying to myself...]
On Tue, Aug 30, 2016 at 12:31:50PM +0200, Christoffer Dall wrote:
> On Wed, Aug 24, 2016 at 04:50:05PM +0530, vijay.kilari at gmail.com wrote:
> > From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
[...]
>
> > static const struct vgic_register_region vgic_v3_dist_registers[] = {
> > REGISTER_DESC_WITH_LENGTH(GICD_CTLR,
> > vgic_mmio_read_v3_misc, vgic_mmio_write_v3_misc, 16,
> > @@ -380,11 +400,13 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> > REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICENABLER,
> > vgic_mmio_read_enable, vgic_mmio_write_cenable, 1,
> > VGIC_ACCESS_32bit),
> > - REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISPENDR,
> > - vgic_mmio_read_pending, vgic_mmio_write_spending, 1,
> > + REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED_UACCESS(GICD_ISPENDR,
> > + vgic_mmio_read_pending, vgic_mmio_write_spending,
> > + vgic_mmio_read_soft_pending, vgic_mmio_write_spending, 1,
>
> You need a uaccess for the write part as well to provide raw access to
> the latch state, without imposing any ordering requirements for the
> restore part of userspace. For example, you cannot rely on userspace
> having restored the configuration state of the IRQs before the pending
> state, unless we modify the API to require this.
>
> I think you need a function that looks very similar tot he
> write_spending function, but which always sets the soft_pending state,
> regardless of the configuration of the IRQ.
>
> We need to tweak the vgic_mmio_write_config function to queue interrupts
> that become pending when changed to LEVEL to go along with this. I can
> send a patch with this separately.
>
>
Thinking about this some more, this last part of my comment, about
vgic_mmio_write_config, is not necessary.
If we implement the uaccess_write_pending such that we call
vgic_queue_irq_unlock when setting the pending state, then we obviously
don't need to do it again later, just because we're changing the
configuration.
Another thing I forgot to say was that the API also specifies that
writes to the CPENDR registers are ignored and writes to the SPENDR
register directly set the latch state, so I you need to make sure the
uaccess writes to CPENDR are ignored and that the writes to SPENDR can
both set/clear the values.
I think the uaccess_write_pending function needs to look something like
this (completely untested):
void vgic_uaccess_write_pending(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len,
unsigned long val)
{
u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
int i;
for (i = 0; i < len * 8; i++) {
struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
spin_lock(&irq->irq_lock);
if (test_bit(i, &val)) {
irq->pending = true;
irq->soft_pending = true;
vgic_queue_irq_unlock(vcpu->kvm, irq);
} else {
irq->soft_pending = false;
if (irq->config == VGIC_CONFIG_EDGE ||
(irq->config == VGIC_CONFIG_LEVEL && !irq->line_level))
irq->pending = false;
spin_unlock(&irq->irq_lock);
}
vgic_put_irq(vcpu->kvm, irq);
}
}
Thanks,
-Christoffer
^ permalink raw reply [flat|nested] 23+ messages in thread
* [RFC PATCH v3 1/5] arm/arm64: vgic-new: Implement support for userspace access
2016-08-24 11:20 ` [RFC PATCH v3 1/5] arm/arm64: vgic-new: Implement support for userspace access vijay.kilari at gmail.com
2016-08-30 10:31 ` Christoffer Dall
@ 2016-08-30 10:50 ` Christoffer Dall
1 sibling, 0 replies; 23+ messages in thread
From: Christoffer Dall @ 2016-08-30 10:50 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 24, 2016 at 04:50:05PM +0530, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> Read and write of some registers like ISPENDR and ICPENDR
> from userspace requires special handling when compared to
> guest access for these registers.
>
> Refer to Documentation/virtual/kvm/devices/arm-vgic-its.txt
> for handling of ISPENDR, ICPENDR registers handling.
>
> Add infrastructure to support guest and userspace read
> and write for the required registers
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
> virt/kvm/arm/vgic/vgic-mmio-v2.c | 5 ++-
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 40 ++++++++++++++----
> virt/kvm/arm/vgic/vgic-mmio.c | 87 ++++++++++++++++++++++++++++++++++++----
> virt/kvm/arm/vgic/vgic-mmio.h | 25 ++++++++++++
> 4 files changed, 139 insertions(+), 18 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index b44b359..cd37159 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -421,9 +421,10 @@ static int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
>
> if (is_write) {
> vgic_data_host_to_mmio_bus(buf, len, *val);
> - ret = kvm_io_gic_ops.write(vcpu, &dev->dev, offset, len, buf);
> + ret = vgic_mmio_uaccess_write(vcpu, &dev->dev, offset,
> + len, buf);
> } else {
> - ret = kvm_io_gic_ops.read(vcpu, &dev->dev, offset, len, buf);
> + ret = vgic_mmio_uaccess_read(vcpu, &dev->dev, offset, len, buf);
> if (!ret)
> *val = vgic_data_mmio_bus_to_host(buf, len);
> }
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index ff668e0..dd0d602 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -367,6 +367,26 @@ static void vgic_mmio_write_pendbase(struct kvm_vcpu *vcpu,
> .write = wr, \
> }
>
> +#define REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED_UACCESS(off, rd, wr, ur, \
> + uw, bpi, acc) \
> + { \
> + .reg_offset = off, \
> + .bits_per_irq = bpi, \
> + .len = (bpi * VGIC_NR_PRIVATE_IRQS) / 8, \
> + .access_flags = acc, \
> + .read = vgic_mmio_read_raz, \
> + .write = vgic_mmio_write_wi, \
> + }, { \
> + .reg_offset = off + (bpi * VGIC_NR_PRIVATE_IRQS) / 8, \
> + .bits_per_irq = bpi, \
> + .len = (bpi * (1024 - VGIC_NR_PRIVATE_IRQS)) / 8, \
> + .access_flags = acc, \
> + .read = rd, \
> + .write = wr, \
> + .uaccess_read = ur, \
> + .uaccess_write = uw, \
> + }
> +
> static const struct vgic_register_region vgic_v3_dist_registers[] = {
> REGISTER_DESC_WITH_LENGTH(GICD_CTLR,
> vgic_mmio_read_v3_misc, vgic_mmio_write_v3_misc, 16,
> @@ -380,11 +400,13 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICENABLER,
> vgic_mmio_read_enable, vgic_mmio_write_cenable, 1,
> VGIC_ACCESS_32bit),
> - REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISPENDR,
> - vgic_mmio_read_pending, vgic_mmio_write_spending, 1,
> + REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED_UACCESS(GICD_ISPENDR,
> + vgic_mmio_read_pending, vgic_mmio_write_spending,
> + vgic_mmio_read_soft_pending, vgic_mmio_write_spending, 1,
> VGIC_ACCESS_32bit),
> - REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICPENDR,
> - vgic_mmio_read_pending, vgic_mmio_write_cpending, 1,
> + REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED_UACCESS(GICD_ICPENDR,
> + vgic_mmio_read_pending, vgic_mmio_write_cpending,
> + vgic_mmio_read_soft_pending, vgic_mmio_write_cpending, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISACTIVER,
> vgic_mmio_read_active, vgic_mmio_write_sactive, 1,
> @@ -443,11 +465,13 @@ static const struct vgic_register_region vgic_v3_sgibase_registers[] = {
> REGISTER_DESC_WITH_LENGTH(GICR_ICENABLER0,
> vgic_mmio_read_enable, vgic_mmio_write_cenable, 4,
> VGIC_ACCESS_32bit),
> - REGISTER_DESC_WITH_LENGTH(GICR_ISPENDR0,
> - vgic_mmio_read_pending, vgic_mmio_write_spending, 4,
> + REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_ISPENDR0,
> + vgic_mmio_read_pending, vgic_mmio_write_spending,
> + vgic_mmio_read_soft_pending, vgic_mmio_write_spending, 4,
> VGIC_ACCESS_32bit),
> - REGISTER_DESC_WITH_LENGTH(GICR_ICPENDR0,
> - vgic_mmio_read_pending, vgic_mmio_write_cpending, 4,
> + REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_ICPENDR0,
> + vgic_mmio_read_pending, vgic_mmio_write_cpending,
> + vgic_mmio_read_soft_pending, vgic_mmio_write_cpending, 4,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_LENGTH(GICR_ISACTIVER0,
> vgic_mmio_read_active, vgic_mmio_write_sactive, 4,
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index 3bad3c5..dcf5d25 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -100,6 +100,26 @@ void vgic_mmio_write_cenable(struct kvm_vcpu *vcpu,
> }
> }
>
> +unsigned long vgic_mmio_read_soft_pending(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len)
> +{
> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> + u32 value = 0;
> + int i;
> +
> + /* Loop over all IRQs affected by this read */
> + for (i = 0; i < len * 8; i++) {
> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> +
> + if (irq->config == VGIC_CONFIG_LEVEL && irq->soft_pending)
> + value |= (1U << i);
> + if (irq->config != VGIC_CONFIG_LEVEL && irq->pending)
> + value |= (1U << i);
> + }
> +
> + return value;
> +}
> +
> unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len)
> {
> @@ -468,6 +488,62 @@ static bool check_region(const struct vgic_register_region *region,
> return false;
> }
>
> +static const struct vgic_register_region *
> + vgic_get_mmio_region(struct vgic_io_device *iodev, gpa_t addr, int len)
> +{
> + const struct vgic_register_region *region;
> +
> + region = vgic_find_mmio_region(iodev->regions, iodev->nr_regions,
> + addr - iodev->base_addr);
> + if (!region || !check_region(region, addr, len))
> + return NULL;
> +
> + return region;
> +}
> +
> +int vgic_mmio_uaccess_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> + gpa_t addr, int len, void *val)
> +{
> + struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev);
> + const struct vgic_register_region *region;
> + struct kvm_vcpu *r_vcpu;
> + unsigned long data;
> +
> + region = vgic_get_mmio_region(iodev, addr, len);
> + if (!region) {
> + memset(val, 0, len);
> + return 0;
> + }
> +
> + r_vcpu = iodev->redist_vcpu ? iodev->redist_vcpu : vcpu;
> + if (region->uaccess_read != NULL)
> + data = region->uaccess_read(r_vcpu, addr, len);
> + else
> + data = region->read(r_vcpu, addr, len);
> + vgic_data_host_to_mmio_bus(val, len, data);
> + return 0;
> +}
> +
> +int vgic_mmio_uaccess_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> + gpa_t addr, int len, const void *val)
> +{
> + struct vgic_io_device *iodev = kvm_to_vgic_iodev(dev);
> + const struct vgic_register_region *region;
> + struct kvm_vcpu *r_vcpu;
> + unsigned long data = vgic_data_mmio_bus_to_host(val, len);
> +
> + region = vgic_get_mmio_region(iodev, addr, len);
> + if (!region)
> + return 0;
> +
> + r_vcpu = iodev->redist_vcpu ? iodev->redist_vcpu : vcpu;
> + if (region->uaccess_write != NULL)
nit: we use the form if (!region->uaccess_write) most other places,
please use this instead.
Thanks,
-Christoffer
^ permalink raw reply [flat|nested] 23+ messages in thread
* [RFC PATCH v3 2/5] arm/arm64: vgic-new: Add distributor and redistributor access
2016-08-24 11:20 [RFC PATCH v3 0/5] arm/arm64: vgic-new: Implement API for vGICv3 live migration vijay.kilari at gmail.com
2016-08-24 11:20 ` [RFC PATCH v3 1/5] arm/arm64: vgic-new: Implement support for userspace access vijay.kilari at gmail.com
@ 2016-08-24 11:20 ` vijay.kilari at gmail.com
2016-08-30 12:31 ` Christoffer Dall
2016-08-24 11:20 ` [RFC PATCH v3 3/5] arm/arm64: vgic-new: Introduce find_reg_by_id() vijay.kilari at gmail.com
` (2 subsequent siblings)
4 siblings, 1 reply; 23+ messages in thread
From: vijay.kilari at gmail.com @ 2016-08-24 11:20 UTC (permalink / raw)
To: linux-arm-kernel
From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
VGICv3 Distributor and Redistributor registers are accessed using
KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_DIST_REGS
with KVM_SET_DEVICE_ATTR and KVM_GET_DEVICE_ATTR ioctls.
These registers are accessed as 32-bit and cpu mpidr
value passed along with register offset is used to identify the
cpu for redistributor registers access.
The version of VGIC v3 specification is define here
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
---
arch/arm64/include/uapi/asm/kvm.h | 3 +
virt/kvm/arm/vgic/vgic-kvm-device.c | 127 +++++++++++++++++++++++++++++++++++-
virt/kvm/arm/vgic/vgic-mmio-v3.c | 113 ++++++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic-mmio.c | 2 +-
virt/kvm/arm/vgic/vgic.h | 8 +++
5 files changed, 250 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 3051f86..94ea676 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -201,10 +201,13 @@ struct kvm_arch_memory_slot {
#define KVM_DEV_ARM_VGIC_GRP_CPU_REGS 2
#define KVM_DEV_ARM_VGIC_CPUID_SHIFT 32
#define KVM_DEV_ARM_VGIC_CPUID_MASK (0xffULL << KVM_DEV_ARM_VGIC_CPUID_SHIFT)
+#define KVM_DEV_ARM_VGIC_V3_CPUID_MASK \
+ (0xffffffffULL << KVM_DEV_ARM_VGIC_CPUID_SHIFT)
#define KVM_DEV_ARM_VGIC_OFFSET_SHIFT 0
#define KVM_DEV_ARM_VGIC_OFFSET_MASK (0xffffffffULL << KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
#define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
#define KVM_DEV_ARM_VGIC_GRP_CTRL 4
+#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
#define KVM_DEV_ARM_VGIC_CTRL_INIT 0
/* Device Control API on vcpu fd */
diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
index 163b057..06f0158 100644
--- a/virt/kvm/arm/vgic/vgic-kvm-device.c
+++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
@@ -433,16 +433,136 @@ struct kvm_device_ops kvm_arm_vgic_v2_ops = {
#ifdef CONFIG_KVM_ARM_VGIC_V3
+static int parse_vgic_v3_attr(struct kvm_device *dev,
+ struct kvm_device_attr *attr,
+ struct vgic_reg_attr *reg_attr)
+{
+ int cpuid;
+
+ cpuid = (attr->attr & KVM_DEV_ARM_VGIC_V3_CPUID_MASK) >>
+ KVM_DEV_ARM_VGIC_CPUID_SHIFT;
+
+ if (cpuid >= atomic_read(&dev->kvm->online_vcpus))
+ return -EINVAL;
+
+ reg_attr->vcpu = kvm_mpidr_to_vcpu(dev->kvm, cpuid);
+ reg_attr->addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
+
+ return 0;
+}
+
+/**
+ * vgic_attr_regs_access_v3 - allows user space to access VGIC v3 state
+ *
+ * @dev: kvm device handle
+ * @attr: kvm device attribute
+ * @reg: address the value is read or written
+ * @is_write: true if userspace is writing a register
+ */
+static int vgic_attr_regs_access_v3(struct kvm_device *dev,
+ struct kvm_device_attr *attr,
+ u64 *reg, bool is_write)
+{
+ struct vgic_reg_attr reg_attr;
+ gpa_t addr;
+ struct kvm_vcpu *vcpu;
+ int ret;
+ u32 tmp32;
+
+ ret = parse_vgic_v3_attr(dev, attr, ®_attr);
+ if (ret)
+ return ret;
+
+ vcpu = reg_attr.vcpu;
+ addr = reg_attr.addr;
+
+ mutex_lock(&dev->kvm->lock);
+
+ ret = vgic_init(dev->kvm);
+ if (ret)
+ goto out;
+
+ if (!lock_all_vcpus(dev->kvm)) {
+ ret = -EBUSY;
+ goto out;
+ }
+
+ switch (attr->group) {
+ case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
+ tmp32 = *reg;
+ ret = vgic_v3_dist_uaccess(vcpu, is_write, addr, &tmp32);
+ if (!is_write)
+ *reg = tmp32;
+ break;
+ case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
+ tmp32 = *reg;
+ ret = vgic_v3_redist_uaccess(vcpu, is_write, addr, &tmp32);
+ if (!is_write)
+ *reg = tmp32;
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ unlock_all_vcpus(dev->kvm);
+out:
+ mutex_unlock(&dev->kvm->lock);
+ return ret;
+}
+
static int vgic_v3_set_attr(struct kvm_device *dev,
struct kvm_device_attr *attr)
{
- return vgic_set_common_attr(dev, attr);
+ int ret;
+
+ ret = vgic_set_common_attr(dev, attr);
+ if (ret != -ENXIO)
+ return ret;
+
+ switch (attr->group) {
+ case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
+ case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS: {
+ u32 __user *uaddr = (u32 __user *)(long)attr->addr;
+ u32 tmp32;
+ u64 reg;
+
+ if (get_user(tmp32, uaddr))
+ return -EFAULT;
+
+ reg = tmp32;
+ return vgic_attr_regs_access_v3(dev, attr, ®, true);
+ }
+ }
+ return -ENXIO;
}
static int vgic_v3_get_attr(struct kvm_device *dev,
struct kvm_device_attr *attr)
{
- return vgic_get_common_attr(dev, attr);
+ int ret;
+
+ ret = vgic_get_common_attr(dev, attr);
+ if (ret != -ENXIO)
+ return ret;
+
+ switch (attr->group) {
+ case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
+ case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS: {
+ u32 __user *uaddr = (u32 __user *)(long)attr->addr;
+ u64 reg;
+ u32 tmp32;
+
+ ret = vgic_attr_regs_access_v3(dev, attr, ®, false);
+ if (ret)
+ return ret;
+ tmp32 = reg;
+ ret = put_user(tmp32, uaddr);
+ return ret;
+ }
+ }
+
+ return -ENXIO;
}
static int vgic_v3_has_attr(struct kvm_device *dev,
@@ -456,6 +576,9 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
return 0;
}
break;
+ case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
+ case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
+ return vgic_v3_has_attr_regs(dev, attr);
case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
return 0;
case KVM_DEV_ARM_VGIC_GRP_CTRL:
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index dd0d602..c2df103 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -18,6 +18,8 @@
#include <kvm/arm_vgic.h>
#include <asm/kvm_emulate.h>
+#include <asm/kvm_arm.h>
+#include <asm/kvm_mmu.h>
#include "vgic.h"
#include "vgic-mmio.h"
@@ -391,6 +393,9 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
REGISTER_DESC_WITH_LENGTH(GICD_CTLR,
vgic_mmio_read_v3_misc, vgic_mmio_write_v3_misc, 16,
VGIC_ACCESS_32bit),
+ REGISTER_DESC_WITH_LENGTH(GICD_STATUSR,
+ vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
+ VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGROUPR,
vgic_mmio_read_rao, vgic_mmio_write_wi, 1,
VGIC_ACCESS_32bit),
@@ -438,12 +443,18 @@ static const struct vgic_register_region vgic_v3_rdbase_registers[] = {
REGISTER_DESC_WITH_LENGTH(GICR_CTLR,
vgic_mmio_read_v3r_ctlr, vgic_mmio_write_v3r_ctlr, 4,
VGIC_ACCESS_32bit),
+ REGISTER_DESC_WITH_LENGTH(GICR_STATUSR,
+ vgic_mmio_read_raz, vgic_mmio_write_wi, 4,
+ VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_LENGTH(GICR_IIDR,
vgic_mmio_read_v3r_iidr, vgic_mmio_write_wi, 4,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_LENGTH(GICR_TYPER,
vgic_mmio_read_v3r_typer, vgic_mmio_write_wi, 8,
VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
+ REGISTER_DESC_WITH_LENGTH(GICR_WAKER,
+ vgic_mmio_read_raz, vgic_mmio_write_wi, 8,
+ VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_LENGTH(GICR_PROPBASER,
vgic_mmio_read_propbase, vgic_mmio_write_propbase, 8,
VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
@@ -564,6 +575,46 @@ int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t redist_base_address)
return ret;
}
+int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
+{
+ struct kvm_vcpu *vcpu;
+ int nr_irqs = dev->kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS;
+ const struct vgic_register_region *regions;
+ gpa_t addr;
+ int nr_regions, i, len, cpuid;
+
+ addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
+ cpuid = (attr->attr & KVM_DEV_ARM_VGIC_V3_CPUID_MASK) >>
+ KVM_DEV_ARM_VGIC_CPUID_SHIFT;
+ vcpu = kvm_mpidr_to_vcpu(dev->kvm, cpuid);
+
+ switch (attr->group) {
+ case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
+ regions = vgic_v3_dist_registers;
+ nr_regions = ARRAY_SIZE(vgic_v3_dist_registers);
+ break;
+ case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:{
+ regions = vgic_v3_rdbase_registers;
+ nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers);
+ break;
+ }
+ default:
+ return -ENXIO;
+ }
+
+ for (i = 0; i < nr_regions; i++) {
+ if (regions[i].bits_per_irq)
+ len = (regions[i].bits_per_irq * nr_irqs) / 8;
+ else
+ len = regions[i].len;
+
+ if (regions[i].reg_offset <= addr &&
+ regions[i].reg_offset + len > addr)
+ return 0;
+ }
+
+ return -ENXIO;
+}
/*
* Compare a given affinity (level 1-3 and a level 0 mask, from the SGI
* generation register ICC_SGI1R_EL1) with a given VCPU.
@@ -670,3 +721,65 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
vgic_put_irq(vcpu->kvm, irq);
}
}
+
+/*
+ * When userland tries to access the VGIC register handlers, we need to
+ * create a usable struct vgic_io_device to be passed to the handlers and we
+ * have to set up a buffer similar to what would have happened if a guest MMIO
+ * access occurred, including doing endian conversions on BE systems.
+ */
+static int vgic_v3_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
+ bool is_write, int offset, u32 *val)
+{
+ unsigned int len = 4;
+ u8 buf[4];
+ int ret;
+
+ if (is_write) {
+ vgic_data_host_to_mmio_bus(buf, len, *val);
+ ret = vgic_mmio_uaccess_write(vcpu, &dev->dev, offset,
+ len, buf);
+ } else {
+ ret = vgic_mmio_uaccess_read(vcpu, &dev->dev, offset, len, buf);
+ if (!ret)
+ *val = vgic_data_mmio_bus_to_host(buf, len);
+ }
+
+ return ret;
+}
+
+int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
+ int offset, u32 *val)
+{
+ struct vgic_io_device dev = {
+ .regions = vgic_v3_dist_registers,
+ .nr_regions = ARRAY_SIZE(vgic_v3_dist_registers),
+ };
+
+ return vgic_v3_uaccess(vcpu, &dev, is_write, offset, val);
+}
+
+int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
+ int offset, u32 *val)
+{
+ struct vgic_io_device *dev;
+ const struct vgic_register_region *region;
+
+ struct vgic_io_device rd_dev = {
+ .regions = vgic_v3_rdbase_registers,
+ .nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers),
+ };
+
+ struct vgic_io_device sgi_dev = {
+ .regions = vgic_v3_sgibase_registers,
+ .nr_regions = ARRAY_SIZE(vgic_v3_sgibase_registers),
+ };
+
+ dev = &sgi_dev;
+ region = vgic_find_mmio_region(dev->regions, dev->nr_regions,
+ offset - dev->base_addr);
+ if (region == NULL)
+ dev = &rd_dev;
+
+ return vgic_v3_uaccess(vcpu, dev, is_write, offset, val);
+}
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index dcf5d25..38f2c75 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -406,7 +406,7 @@ static int match_region(const void *key, const void *elt)
}
/* Find the proper register handler entry given a certain address offset. */
-static const struct vgic_register_region *
+const struct vgic_register_region *
vgic_find_mmio_region(const struct vgic_register_region *region, int nr_regions,
unsigned int offset)
{
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 1d8e21d..14e4ce5 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -72,6 +72,9 @@ static inline void vgic_get_irq_kref(struct vgic_irq *irq)
kref_get(&irq->refcount);
}
+const struct vgic_register_region *
+ vgic_find_mmio_region(const struct vgic_register_region *region,
+ int nr_regions, unsigned int offset);
#ifdef CONFIG_KVM_ARM_VGIC_V3
void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu);
void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu);
@@ -88,6 +91,11 @@ bool vgic_has_its(struct kvm *kvm);
int kvm_vgic_register_its_device(void);
void vgic_enable_lpis(struct kvm_vcpu *vcpu);
int vgic_its_inject_msi(struct kvm *kvm, struct kvm_msi *msi);
+int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr);
+int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
+ int offset, u32 *val);
+int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
+ int offset, u32 *val);
#else
static inline void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu)
{
--
1.9.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [RFC PATCH v3 2/5] arm/arm64: vgic-new: Add distributor and redistributor access
2016-08-24 11:20 ` [RFC PATCH v3 2/5] arm/arm64: vgic-new: Add distributor and redistributor access vijay.kilari at gmail.com
@ 2016-08-30 12:31 ` Christoffer Dall
[not found] ` <CALicx6tbUDCUe6SBr=HA1MnNdZa6L1+U67C3V_pT-Nw2RGjR6g@mail.gmail.com>
0 siblings, 1 reply; 23+ messages in thread
From: Christoffer Dall @ 2016-08-30 12:31 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 24, 2016 at 04:50:06PM +0530, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> VGICv3 Distributor and Redistributor registers are accessed using
> KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_DIST_REGS
> with KVM_SET_DEVICE_ATTR and KVM_GET_DEVICE_ATTR ioctls.
> These registers are accessed as 32-bit and cpu mpidr
> value passed along with register offset is used to identify the
> cpu for redistributor registers access.
>
> The version of VGIC v3 specification is define here
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
> arch/arm64/include/uapi/asm/kvm.h | 3 +
> virt/kvm/arm/vgic/vgic-kvm-device.c | 127 +++++++++++++++++++++++++++++++++++-
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 113 ++++++++++++++++++++++++++++++++
> virt/kvm/arm/vgic/vgic-mmio.c | 2 +-
> virt/kvm/arm/vgic/vgic.h | 8 +++
> 5 files changed, 250 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 3051f86..94ea676 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -201,10 +201,13 @@ struct kvm_arch_memory_slot {
> #define KVM_DEV_ARM_VGIC_GRP_CPU_REGS 2
> #define KVM_DEV_ARM_VGIC_CPUID_SHIFT 32
> #define KVM_DEV_ARM_VGIC_CPUID_MASK (0xffULL << KVM_DEV_ARM_VGIC_CPUID_SHIFT)
> +#define KVM_DEV_ARM_VGIC_V3_CPUID_MASK \
> + (0xffffffffULL << KVM_DEV_ARM_VGIC_CPUID_SHIFT)
I think this should be KVM_DEV_ARM_VGIC_V3_MPIDR_MASK, and I would
probably define a V3 of the shift as well, since we're really talking
about two distinct APIs here.
> #define KVM_DEV_ARM_VGIC_OFFSET_SHIFT 0
> #define KVM_DEV_ARM_VGIC_OFFSET_MASK (0xffffffffULL << KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
> #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
> #define KVM_DEV_ARM_VGIC_GRP_CTRL 4
> +#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
> #define KVM_DEV_ARM_VGIC_CTRL_INIT 0
>
> /* Device Control API on vcpu fd */
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index 163b057..06f0158 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -433,16 +433,136 @@ struct kvm_device_ops kvm_arm_vgic_v2_ops = {
>
> #ifdef CONFIG_KVM_ARM_VGIC_V3
>
> +static int parse_vgic_v3_attr(struct kvm_device *dev,
> + struct kvm_device_attr *attr,
> + struct vgic_reg_attr *reg_attr)
> +{
> + int cpuid;
> +
> + cpuid = (attr->attr & KVM_DEV_ARM_VGIC_V3_CPUID_MASK) >>
> + KVM_DEV_ARM_VGIC_CPUID_SHIFT;
> +
> + if (cpuid >= atomic_read(&dev->kvm->online_vcpus))
> + return -EINVAL;
huh? it's an MPIDR, not a cpu id.
just resolve it with kvm_mpidr_to_vcpu and check its return value.
> +
> + reg_attr->vcpu = kvm_mpidr_to_vcpu(dev->kvm, cpuid);
please check the return value of this function in any case...
> + reg_attr->addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
> +
> + return 0;
> +}
> +
> +/**
> + * vgic_attr_regs_access_v3 - allows user space to access VGIC v3 state
> + *
> + * @dev: kvm device handle
> + * @attr: kvm device attribute
> + * @reg: address the value is read or written
> + * @is_write: true if userspace is writing a register
> + */
> +static int vgic_attr_regs_access_v3(struct kvm_device *dev,
> + struct kvm_device_attr *attr,
> + u64 *reg, bool is_write)
> +{
> + struct vgic_reg_attr reg_attr;
> + gpa_t addr;
> + struct kvm_vcpu *vcpu;
> + int ret;
> + u32 tmp32;
> +
> + ret = parse_vgic_v3_attr(dev, attr, ®_attr);
> + if (ret)
> + return ret;
> +
> + vcpu = reg_attr.vcpu;
> + addr = reg_attr.addr;
> +
> + mutex_lock(&dev->kvm->lock);
> +
> + ret = vgic_init(dev->kvm);
> + if (ret)
> + goto out;
no, no, please no more auto-init at userspace access time. This code
should instead check vgic_initialized() and return an error to userspace
if not initialized.
> +
> + if (!lock_all_vcpus(dev->kvm)) {
> + ret = -EBUSY;
> + goto out;
> + }
> +
> + switch (attr->group) {
> + case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> + tmp32 = *reg;
why is this assignment not predicated on if (is_write) ?
also, all this type conversion nonsense is probably unnecessary, see my
comment below.
> + ret = vgic_v3_dist_uaccess(vcpu, is_write, addr, &tmp32);
> + if (!is_write)
> + *reg = tmp32;
> + break;
> + case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
> + tmp32 = *reg;
> + ret = vgic_v3_redist_uaccess(vcpu, is_write, addr, &tmp32);
> + if (!is_write)
> + *reg = tmp32;
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + unlock_all_vcpus(dev->kvm);
> +out:
> + mutex_unlock(&dev->kvm->lock);
> + return ret;
> +}
> +
> static int vgic_v3_set_attr(struct kvm_device *dev,
> struct kvm_device_attr *attr)
> {
> - return vgic_set_common_attr(dev, attr);
> + int ret;
> +
> + ret = vgic_set_common_attr(dev, attr);
> + if (ret != -ENXIO)
> + return ret;
> +
> + switch (attr->group) {
> + case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> + case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS: {
> + u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> + u32 tmp32;
> + u64 reg;
> +
> + if (get_user(tmp32, uaddr))
> + return -EFAULT;
> +
> + reg = tmp32;
> + return vgic_attr_regs_access_v3(dev, attr, ®, true);
oh, but wait, you already had a 32-bit value, which you convert into a
64-bit value, just to convert it back again, to do some extra data
copies?
I'm really confused as to why this has to be so complicated.
Why not simply use u32 all the way?
> + }
> + }
> + return -ENXIO;
> }
>
> static int vgic_v3_get_attr(struct kvm_device *dev,
> struct kvm_device_attr *attr)
> {
> - return vgic_get_common_attr(dev, attr);
> + int ret;
> +
> + ret = vgic_get_common_attr(dev, attr);
> + if (ret != -ENXIO)
> + return ret;
> +
> + switch (attr->group) {
> + case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> + case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS: {
> + u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> + u64 reg;
> + u32 tmp32;
> +
> + ret = vgic_attr_regs_access_v3(dev, attr, ®, false);
> + if (ret)
> + return ret;
> + tmp32 = reg;
> + ret = put_user(tmp32, uaddr);
same, type stuff.
> + return ret;
> + }
> + }
> +
> + return -ENXIO;
> }
>
> static int vgic_v3_has_attr(struct kvm_device *dev,
> @@ -456,6 +576,9 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
> return 0;
> }
> break;
> + case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> + case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
> + return vgic_v3_has_attr_regs(dev, attr);
> case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
> return 0;
> case KVM_DEV_ARM_VGIC_GRP_CTRL:
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index dd0d602..c2df103 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -18,6 +18,8 @@
> #include <kvm/arm_vgic.h>
>
> #include <asm/kvm_emulate.h>
> +#include <asm/kvm_arm.h>
> +#include <asm/kvm_mmu.h>
>
> #include "vgic.h"
> #include "vgic-mmio.h"
> @@ -391,6 +393,9 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> REGISTER_DESC_WITH_LENGTH(GICD_CTLR,
> vgic_mmio_read_v3_misc, vgic_mmio_write_v3_misc, 16,
> VGIC_ACCESS_32bit),
> + REGISTER_DESC_WITH_LENGTH(GICD_STATUSR,
> + vgic_mmio_read_rao, vgic_mmio_write_wi, 4,
> + VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_IGROUPR,
> vgic_mmio_read_rao, vgic_mmio_write_wi, 1,
> VGIC_ACCESS_32bit),
> @@ -438,12 +443,18 @@ static const struct vgic_register_region vgic_v3_rdbase_registers[] = {
> REGISTER_DESC_WITH_LENGTH(GICR_CTLR,
> vgic_mmio_read_v3r_ctlr, vgic_mmio_write_v3r_ctlr, 4,
> VGIC_ACCESS_32bit),
> + REGISTER_DESC_WITH_LENGTH(GICR_STATUSR,
> + vgic_mmio_read_raz, vgic_mmio_write_wi, 4,
> + VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_LENGTH(GICR_IIDR,
> vgic_mmio_read_v3r_iidr, vgic_mmio_write_wi, 4,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_LENGTH(GICR_TYPER,
> vgic_mmio_read_v3r_typer, vgic_mmio_write_wi, 8,
> VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
> + REGISTER_DESC_WITH_LENGTH(GICR_WAKER,
> + vgic_mmio_read_raz, vgic_mmio_write_wi, 8,
> + VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_LENGTH(GICR_PROPBASER,
> vgic_mmio_read_propbase, vgic_mmio_write_propbase, 8,
> VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
> @@ -564,6 +575,46 @@ int vgic_register_redist_iodevs(struct kvm *kvm, gpa_t redist_base_address)
> return ret;
> }
>
> +int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
> +{
> + struct kvm_vcpu *vcpu;
> + int nr_irqs = dev->kvm->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS;
> + const struct vgic_register_region *regions;
> + gpa_t addr;
> + int nr_regions, i, len, cpuid;
> +
> + addr = attr->attr & KVM_DEV_ARM_VGIC_OFFSET_MASK;
> + cpuid = (attr->attr & KVM_DEV_ARM_VGIC_V3_CPUID_MASK) >>
> + KVM_DEV_ARM_VGIC_CPUID_SHIFT;
> + vcpu = kvm_mpidr_to_vcpu(dev->kvm, cpuid);
why do you need the vcpu variable?
(also, you don't check the return value)
> +
> + switch (attr->group) {
> + case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> + regions = vgic_v3_dist_registers;
> + nr_regions = ARRAY_SIZE(vgic_v3_dist_registers);
> + break;
> + case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:{
> + regions = vgic_v3_rdbase_registers;
> + nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers);
> + break;
> + }
> + default:
> + return -ENXIO;
> + }
why don't you need an address alignment check here?
> +
> + for (i = 0; i < nr_regions; i++) {
> + if (regions[i].bits_per_irq)
> + len = (regions[i].bits_per_irq * nr_irqs) / 8;
> + else
> + len = regions[i].len;
> +
> + if (regions[i].reg_offset <= addr &&
> + regions[i].reg_offset + len > addr)
> + return 0;
> + }
this is directly lifted from v2, definitely this loop or the whole
function can be shared between v2/v3 somehow.
> +
> + return -ENXIO;
> +}
> /*
> * Compare a given affinity (level 1-3 and a level 0 mask, from the SGI
> * generation register ICC_SGI1R_EL1) with a given VCPU.
> @@ -670,3 +721,65 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg)
> vgic_put_irq(vcpu->kvm, irq);
> }
> }
> +
> +/*
> + * When userland tries to access the VGIC register handlers, we need to
> + * create a usable struct vgic_io_device to be passed to the handlers and we
> + * have to set up a buffer similar to what would have happened if a guest MMIO
> + * access occurred, including doing endian conversions on BE systems.
> + */
> +static int vgic_v3_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
> + bool is_write, int offset, u32 *val)
> +{
> + unsigned int len = 4;
> + u8 buf[4];
> + int ret;
> +
> + if (is_write) {
> + vgic_data_host_to_mmio_bus(buf, len, *val);
> + ret = vgic_mmio_uaccess_write(vcpu, &dev->dev, offset,
> + len, buf);
> + } else {
> + ret = vgic_mmio_uaccess_read(vcpu, &dev->dev, offset, len, buf);
> + if (!ret)
> + *val = vgic_data_mmio_bus_to_host(buf, len);
> + }
> +
> + return ret;
> +}
this as well seems to be completely identical to the v2 version, or am I
missing something?
> +
> +int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> + int offset, u32 *val)
> +{
> + struct vgic_io_device dev = {
> + .regions = vgic_v3_dist_registers,
> + .nr_regions = ARRAY_SIZE(vgic_v3_dist_registers),
> + };
> +
> + return vgic_v3_uaccess(vcpu, &dev, is_write, offset, val);
> +}
> +
> +int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> + int offset, u32 *val)
> +{
> + struct vgic_io_device *dev;
> + const struct vgic_register_region *region;
> +
> + struct vgic_io_device rd_dev = {
> + .regions = vgic_v3_rdbase_registers,
> + .nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers),
> + };
> +
> + struct vgic_io_device sgi_dev = {
> + .regions = vgic_v3_sgibase_registers,
> + .nr_regions = ARRAY_SIZE(vgic_v3_sgibase_registers),
> + };
> +
The whole bit following here deserves a comment.
I'm guessing the idea is to check if the offset falls into the sgi_dev
or rd_dev region?
Why not simply do:
/* SGI_base is the next 64K frame after RD_base */
if (offset >= SZ_64K)
return vgic_v3_uaccess(vcpu, &sgi_dev, is_write,
offset - SZ_64K, val);
else
return vgic_v3_uaccess(vcpu, &rd_dev, is_write, offset, val);
> + dev = &sgi_dev;
> + region = vgic_find_mmio_region(dev->regions, dev->nr_regions,
> + offset - dev->base_addr);
In any case, this doesn't look right: dev->base_addr is not set?
> + if (region == NULL)
If you must, then do: if (!region)
> + dev = &rd_dev;
> +
> + return vgic_v3_uaccess(vcpu, dev, is_write, offset, val);
> +}
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index dcf5d25..38f2c75 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -406,7 +406,7 @@ static int match_region(const void *key, const void *elt)
> }
>
> /* Find the proper register handler entry given a certain address offset. */
> -static const struct vgic_register_region *
> +const struct vgic_register_region *
> vgic_find_mmio_region(const struct vgic_register_region *region, int nr_regions,
> unsigned int offset)
> {
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index 1d8e21d..14e4ce5 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -72,6 +72,9 @@ static inline void vgic_get_irq_kref(struct vgic_irq *irq)
> kref_get(&irq->refcount);
> }
>
> +const struct vgic_register_region *
> + vgic_find_mmio_region(const struct vgic_register_region *region,
> + int nr_regions, unsigned int offset);
> #ifdef CONFIG_KVM_ARM_VGIC_V3
> void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu);
> void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu);
> @@ -88,6 +91,11 @@ bool vgic_has_its(struct kvm *kvm);
> int kvm_vgic_register_its_device(void);
> void vgic_enable_lpis(struct kvm_vcpu *vcpu);
> int vgic_its_inject_msi(struct kvm *kvm, struct kvm_msi *msi);
> +int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr);
> +int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> + int offset, u32 *val);
> +int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> + int offset, u32 *val);
> #else
> static inline void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu)
> {
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [RFC PATCH v3 3/5] arm/arm64: vgic-new: Introduce find_reg_by_id()
2016-08-24 11:20 [RFC PATCH v3 0/5] arm/arm64: vgic-new: Implement API for vGICv3 live migration vijay.kilari at gmail.com
2016-08-24 11:20 ` [RFC PATCH v3 1/5] arm/arm64: vgic-new: Implement support for userspace access vijay.kilari at gmail.com
2016-08-24 11:20 ` [RFC PATCH v3 2/5] arm/arm64: vgic-new: Add distributor and redistributor access vijay.kilari at gmail.com
@ 2016-08-24 11:20 ` vijay.kilari at gmail.com
2016-08-30 12:41 ` Christoffer Dall
2016-08-24 11:20 ` [RFC PATCH v3 4/5] arm/arm64: vgic-new: Implement VGICv3 CPU interface access vijay.kilari at gmail.com
2016-08-24 11:20 ` [RFC PATCH v3 5/5] arm/arm64: vgic-new: Implement KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO ioctl vijay.kilari at gmail.com
4 siblings, 1 reply; 23+ messages in thread
From: vijay.kilari at gmail.com @ 2016-08-24 11:20 UTC (permalink / raw)
To: linux-arm-kernel
From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
In order to implement vGICv3 CPU interface access, we will need to perform
table lookup of system registers. We would need both index_to_params() and
find_reg() exported for that purpose, but instead we export a single
function which combines them both.
Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
---
arch/arm64/kvm/sys_regs.c | 22 +++++++++++++++-------
arch/arm64/kvm/sys_regs.h | 4 ++++
2 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index b0b225c..2c9bd52 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1802,6 +1802,17 @@ static bool index_to_params(u64 id, struct sys_reg_params *params)
}
}
+const struct sys_reg_desc *find_reg_by_id(u64 id,
+ struct sys_reg_params *params,
+ const struct sys_reg_desc table[],
+ unsigned int num)
+{
+ if (!index_to_params(id, params))
+ return NULL;
+
+ return find_reg(params, table, num);
+}
+
/* Decode an index value, and find the sys_reg_desc entry. */
static const struct sys_reg_desc *index_to_sys_reg_desc(struct kvm_vcpu *vcpu,
u64 id)
@@ -1929,10 +1940,8 @@ static int get_invariant_sys_reg(u64 id, void __user *uaddr)
struct sys_reg_params params;
const struct sys_reg_desc *r;
- if (!index_to_params(id, ¶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;
@@ -1946,9 +1955,8 @@ static int set_invariant_sys_reg(u64 id, void __user *uaddr)
int err;
u64 val = 0; /* Make sure high bits are 0 for 32-bit regs */
- if (!index_to_params(id, ¶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 dbbb01c..9c6ffd0 100644
--- a/arch/arm64/kvm/sys_regs.h
+++ b/arch/arm64/kvm/sys_regs.h
@@ -136,6 +136,10 @@ static inline int cmp_sys_reg(const struct sys_reg_desc *i1,
return i1->Op2 - i2->Op2;
}
+const struct sys_reg_desc *find_reg_by_id(u64 id,
+ struct sys_reg_params *params,
+ const struct sys_reg_desc table[],
+ unsigned int num);
#define Op0(_x) .Op0 = _x
#define Op1(_x) .Op1 = _x
--
1.9.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [RFC PATCH v3 3/5] arm/arm64: vgic-new: Introduce find_reg_by_id()
2016-08-24 11:20 ` [RFC PATCH v3 3/5] arm/arm64: vgic-new: Introduce find_reg_by_id() vijay.kilari at gmail.com
@ 2016-08-30 12:41 ` Christoffer Dall
0 siblings, 0 replies; 23+ messages in thread
From: Christoffer Dall @ 2016-08-30 12:41 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 24, 2016 at 04:50:07PM +0530, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> In order to implement vGICv3 CPU interface access, we will need to perform
> table lookup of system registers. We would need both index_to_params() and
> find_reg() exported for that purpose, but instead we export a single
> function which combines them both.
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [RFC PATCH v3 4/5] arm/arm64: vgic-new: Implement VGICv3 CPU interface access
2016-08-24 11:20 [RFC PATCH v3 0/5] arm/arm64: vgic-new: Implement API for vGICv3 live migration vijay.kilari at gmail.com
` (2 preceding siblings ...)
2016-08-24 11:20 ` [RFC PATCH v3 3/5] arm/arm64: vgic-new: Introduce find_reg_by_id() vijay.kilari at gmail.com
@ 2016-08-24 11:20 ` vijay.kilari at gmail.com
2016-08-30 13:45 ` Christoffer Dall
2016-09-06 17:10 ` Christoffer Dall
2016-08-24 11:20 ` [RFC PATCH v3 5/5] arm/arm64: vgic-new: Implement KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO ioctl vijay.kilari at gmail.com
4 siblings, 2 replies; 23+ messages in thread
From: vijay.kilari at gmail.com @ 2016-08-24 11:20 UTC (permalink / raw)
To: linux-arm-kernel
From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
VGICv3 CPU interface registers are accessed using
KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
as 64-bit. The cpu MPIDR value is passed along with register id.
is used to identify the cpu for registers access.
The version of VGIC v3 specification is define here
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
---
arch/arm64/include/uapi/asm/kvm.h | 15 ++-
arch/arm64/kvm/Makefile | 1 +
include/linux/irqchip/arm-gic-v3.h | 4 +
virt/kvm/arm/vgic/vgic-kvm-device.c | 29 +++++
virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-
virt/kvm/arm/vgic/vgic-mmio-v3.c | 6 +
virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 211 ++++++++++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic.h | 6 +
8 files changed, 271 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 94ea676..b13c944 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -182,14 +182,14 @@ struct kvm_arch_memory_slot {
KVM_REG_ARM64_SYSREG_ ## n ## _MASK)
#define __ARM64_SYS_REG(op0,op1,crn,crm,op2) \
- (KVM_REG_ARM64 | KVM_REG_ARM64_SYSREG | \
- ARM64_SYS_REG_SHIFT_MASK(op0, OP0) | \
+ (ARM64_SYS_REG_SHIFT_MASK(op0, OP0) | \
ARM64_SYS_REG_SHIFT_MASK(op1, OP1) | \
ARM64_SYS_REG_SHIFT_MASK(crn, CRN) | \
ARM64_SYS_REG_SHIFT_MASK(crm, CRM) | \
ARM64_SYS_REG_SHIFT_MASK(op2, OP2))
-#define ARM64_SYS_REG(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U64)
+#define ARM64_SYS_REG(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U64 | \
+ KVM_REG_ARM64 | KVM_REG_ARM64_SYSREG)
#define KVM_REG_ARM_TIMER_CTL ARM64_SYS_REG(3, 3, 14, 3, 1)
#define KVM_REG_ARM_TIMER_CNT ARM64_SYS_REG(3, 3, 14, 3, 2)
@@ -208,7 +208,16 @@ struct kvm_arch_memory_slot {
#define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
#define KVM_DEV_ARM_VGIC_GRP_CTRL 4
#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
+#define KVM_DEV_ARM_VGIC_CPU_SYSREGS 6
+
#define KVM_DEV_ARM_VGIC_CTRL_INIT 0
+#define KVM_DEV_ARM_VGIC_SYSREG_MASK (KVM_REG_ARM64_SYSREG_OP0_MASK | \
+ KVM_REG_ARM64_SYSREG_OP1_MASK | \
+ KVM_REG_ARM64_SYSREG_CRN_MASK | \
+ KVM_REG_ARM64_SYSREG_CRM_MASK | \
+ KVM_REG_ARM64_SYSREG_OP2_MASK)
+#define KVM_DEV_ARM_VGIC_SYSREG(op0, op1, crn, crm, op2) \
+ __ARM64_SYS_REG(op0, op1, crn, crm, op2)
/* Device Control API on vcpu fd */
#define KVM_ARM_VCPU_PMU_V3_CTRL 0
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index 695eb3c..dafbd0e 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -31,5 +31,6 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v3.o
kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/irqchip.o
+kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-sys-reg-v3.o
kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 56b0b7e..164463b 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -383,6 +383,10 @@
#define ICH_VMCR_CTLR_SHIFT 0
#define ICH_VMCR_CTLR_MASK (0x21f << ICH_VMCR_CTLR_SHIFT)
+#define ICH_VMCR_ENG0_SHIFT 0
+#define ICH_VMCR_ENG0 (1 << ICH_VMCR_ENG0_SHIFT)
+#define ICH_VMCR_ENG1_SHIFT 1
+#define ICH_VMCR_ENG1 (1 << ICH_VMCR_ENG1_SHIFT)
#define ICH_VMCR_BPR1_SHIFT 18
#define ICH_VMCR_BPR1_MASK (7 << ICH_VMCR_BPR1_SHIFT)
#define ICH_VMCR_BPR0_SHIFT 21
diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
index 06f0158..74e5c38 100644
--- a/virt/kvm/arm/vgic/vgic-kvm-device.c
+++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
@@ -500,6 +500,15 @@ static int vgic_attr_regs_access_v3(struct kvm_device *dev,
if (!is_write)
*reg = tmp32;
break;
+ case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
+ u64 regid;
+
+ regid = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_MASK) |
+ KVM_REG_SIZE_U64;
+ ret = vgic_v3_cpu_sysregs_uaccess(vcpu, is_write,
+ regid, reg);
+ break;
+ }
default:
ret = -EINVAL;
break;
@@ -533,6 +542,15 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
reg = tmp32;
return vgic_attr_regs_access_v3(dev, attr, ®, true);
}
+ case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
+ u64 __user *uaddr = (u64 __user *)(long)attr->addr;
+ u64 reg;
+
+ if (get_user(reg, uaddr))
+ return -EFAULT;
+
+ return vgic_attr_regs_access_v3(dev, attr, ®, true);
+ }
}
return -ENXIO;
}
@@ -560,6 +578,16 @@ static int vgic_v3_get_attr(struct kvm_device *dev,
ret = put_user(tmp32, uaddr);
return ret;
}
+ case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
+ u64 __user *uaddr = (u64 __user *)(long)attr->addr;
+ u64 reg;
+
+ ret = vgic_attr_regs_access_v3(dev, attr, ®, false);
+ if (ret)
+ return ret;
+ ret = put_user(reg, uaddr);
+ return ret;
+ }
}
return -ENXIO;
@@ -578,6 +606,7 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
break;
case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
+ case KVM_DEV_ARM_VGIC_CPU_SYSREGS:
return vgic_v3_has_attr_regs(dev, attr);
case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
return 0;
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index cd37159..4a35eb8 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -212,7 +212,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu,
}
}
-static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
+void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
{
if (kvm_vgic_global_state.type == VGIC_V2)
vgic_v2_set_vmcr(vcpu, vmcr);
@@ -220,7 +220,7 @@ static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
vgic_v3_set_vmcr(vcpu, vmcr);
}
-static void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
+void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
{
if (kvm_vgic_global_state.type == VGIC_V2)
vgic_v2_get_vmcr(vcpu, vmcr);
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index c2df103..61abea0 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -23,6 +23,7 @@
#include "vgic.h"
#include "vgic-mmio.h"
+#include "sys_regs.h"
/* extract @num bytes at @offset bytes offset in data */
unsigned long extract_bytes(unsigned long data, unsigned int offset,
@@ -598,6 +599,11 @@ int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers);
break;
}
+ case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
+ u64 reg;
+
+ return vgic_v3_has_cpu_sysregs_attr(vcpu, 0, cpuid, ®);
+ }
default:
return -ENXIO;
}
diff --git a/virt/kvm/arm/vgic/vgic-sys-reg-v3.c b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
new file mode 100644
index 0000000..581d053
--- /dev/null
+++ b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
@@ -0,0 +1,211 @@
+#include <linux/irqchip/arm-gic-v3.h>
+#include <linux/kvm.h>
+#include <linux/kvm_host.h>
+#include <kvm/iodev.h>
+#include <kvm/arm_vgic.h>
+#include <asm/kvm_emulate.h>
+#include <asm/kvm_arm.h>
+#include <asm/kvm_mmu.h>
+
+#include "vgic.h"
+#include "vgic-mmio.h"
+#include "sys_regs.h"
+
+static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+ const struct sys_reg_desc *r)
+{
+ struct vgic_vmcr vmcr;
+
+ vgic_get_vmcr(vcpu, &vmcr);
+ if (p->is_write) {
+ vmcr.ctlr = (u32)p->regval;
+ vgic_set_vmcr(vcpu, &vmcr);
+ } else {
+ p->regval = vmcr.ctlr;
+ }
+
+ return true;
+}
+
+static bool access_gic_pmr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+ const struct sys_reg_desc *r)
+{
+ struct vgic_vmcr vmcr;
+
+ vgic_get_vmcr(vcpu, &vmcr);
+ if (p->is_write) {
+ vmcr.pmr = (u32)p->regval;
+ vgic_set_vmcr(vcpu, &vmcr);
+ } else {
+ p->regval = vmcr.pmr;
+ }
+
+ return true;
+}
+
+static bool access_gic_bpr0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+ const struct sys_reg_desc *r)
+{
+ struct vgic_vmcr vmcr;
+
+ vgic_get_vmcr(vcpu, &vmcr);
+ if (p->is_write) {
+ vmcr.bpr = (u32)p->regval;
+ vgic_set_vmcr(vcpu, &vmcr);
+ } else {
+ p->regval = vmcr.bpr;
+ }
+
+ return true;
+}
+
+static bool access_gic_bpr1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+ const struct sys_reg_desc *r)
+{
+ struct vgic_vmcr vmcr;
+
+ vgic_get_vmcr(vcpu, &vmcr);
+ if (p->is_write) {
+ vmcr.abpr = (u32)p->regval;
+ vgic_set_vmcr(vcpu, &vmcr);
+ } else {
+ p->regval = vmcr.abpr;
+ }
+
+ return true;
+}
+
+static bool access_gic_grpen0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+ const struct sys_reg_desc *r)
+{
+ struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+ if (p->is_write) {
+ vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG0;
+ vgicv3->vgic_vmcr |= (p->regval << ICH_VMCR_ENG0_SHIFT) &
+ ICH_VMCR_ENG0;
+ } else {
+ p->regval = (vgicv3->vgic_vmcr & ICH_VMCR_ENG0) >>
+ ICH_VMCR_ENG0_SHIFT;
+ }
+
+ return true;
+}
+
+static bool access_gic_grpen1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+ const struct sys_reg_desc *r)
+{
+ struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+
+ if (p->is_write) {
+ vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG1;
+ vgicv3->vgic_vmcr |= (p->regval << ICH_VMCR_ENG1_SHIFT) &
+ ICH_VMCR_ENG1;
+ } else {
+ p->regval = (vgicv3->vgic_vmcr & ICH_VMCR_ENG1) >>
+ ICH_VMCR_ENG1_SHIFT;
+ }
+
+ return true;
+}
+
+static bool access_gic_ap0r(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+ const struct sys_reg_desc *r)
+{
+ struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+ u8 idx = r->Op2 & 3;
+
+ if (p->is_write)
+ vgicv3->vgic_ap0r[idx] = p->regval;
+ else
+ p->regval = vgicv3->vgic_ap0r[idx];
+
+ return true;
+}
+
+static bool access_gic_ap1r(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
+ const struct sys_reg_desc *r)
+{
+ struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
+ u8 idx = r->Op2 & 3;
+
+ if (p->is_write)
+ vgicv3->vgic_ap1r[idx] = p->regval;
+ else
+ p->regval = vgicv3->vgic_ap1r[idx];
+
+ return true;
+}
+
+static const struct sys_reg_desc gic_v3_icc_reg_descs[] = {
+ /* ICC_PMR_EL1 */
+ { Op0(3), Op1(0), CRn(4), CRm(6), Op2(0), access_gic_pmr },
+ /* ICC_BPR0_EL1 */
+ { Op0(3), Op1(0), CRn(12), CRm(8), Op2(3), access_gic_bpr0 },
+ /* ICC_AP0R0_EL1 */
+ { Op0(3), Op1(0), CRn(12), CRm(8), Op2(4), access_gic_ap0r },
+ /* ICC_AP0R1_EL1 */
+ { Op0(3), Op1(0), CRn(12), CRm(8), Op2(5), access_gic_ap0r },
+ /* ICC_AP0R2_EL1 */
+ { Op0(3), Op1(0), CRn(12), CRm(8), Op2(6), access_gic_ap0r },
+ /* ICC_AP0R3_EL1 */
+ { Op0(3), Op1(0), CRn(12), CRm(8), Op2(7), access_gic_ap0r },
+ /* ICC_AP1R0_EL1 */
+ { Op0(3), Op1(0), CRn(12), CRm(9), Op2(0), access_gic_ap1r },
+ /* ICC_AP1R1_EL1 */
+ { Op0(3), Op1(0), CRn(12), CRm(9), Op2(1), access_gic_ap1r },
+ /* ICC_AP1R2_EL1 */
+ { Op0(3), Op1(0), CRn(12), CRm(9), Op2(2), access_gic_ap1r },
+ /* ICC_AP1R3_EL1 */
+ { Op0(3), Op1(0), CRn(12), CRm(9), Op2(3), access_gic_ap1r },
+ /* ICC_BPR1_EL1 */
+ { Op0(3), Op1(0), CRn(12), CRm(12), Op2(3), access_gic_bpr1 },
+ /* ICC_CTLR_EL1 */
+ { Op0(3), Op1(0), CRn(12), CRm(12), Op2(4), access_gic_ctlr },
+ /* ICC_IGRPEN0_EL1 */
+ { Op0(3), Op1(0), CRn(12), CRm(12), Op2(6), access_gic_grpen0 },
+ /* ICC_GRPEN1_EL1 */
+ { Op0(3), Op1(0), CRn(12), CRm(12), Op2(7), access_gic_grpen1 },
+};
+
+int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
+ u64 *reg)
+{
+ struct sys_reg_params params;
+
+ params.regval = le64_to_cpu(*reg);
+ params.is_write = is_write;
+ params.is_aarch32 = false;
+ params.is_32bit = false;
+
+ return find_reg_by_id(id, ¶ms, gic_v3_icc_reg_descs,
+ ARRAY_SIZE(gic_v3_icc_reg_descs)) ?
+ 0 : -ENXIO;
+}
+
+int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, u64 id,
+ u64 *reg)
+{
+ struct sys_reg_params params;
+ const struct sys_reg_desc *r;
+
+ if (is_write)
+ params.regval = le64_to_cpu(*reg);
+ params.is_write = is_write;
+ params.is_aarch32 = false;
+ params.is_32bit = false;
+
+ r = find_reg_by_id(id, ¶ms, gic_v3_icc_reg_descs,
+ ARRAY_SIZE(gic_v3_icc_reg_descs));
+ if (!r)
+ return -ENXIO;
+
+ if (!r->access(vcpu, ¶ms, r))
+ return -EINVAL;
+
+ if (!is_write)
+ *reg = cpu_to_le64(params.regval);
+
+ return 0;
+}
+
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 14e4ce5..20eab36c 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -96,6 +96,10 @@ int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
int offset, u32 *val);
int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
int offset, u32 *val);
+int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write,
+ u64 id, u64 *val);
+int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
+ u64 *reg);
#else
static inline void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu)
{
@@ -169,6 +173,8 @@ static inline int vgic_its_inject_msi(struct kvm *kvm, struct kvm_msi *msi)
#endif
int kvm_register_vgic_device(unsigned long type);
+void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
+void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
int vgic_lazy_init(struct kvm *kvm);
int vgic_init(struct kvm *kvm);
--
1.9.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [RFC PATCH v3 4/5] arm/arm64: vgic-new: Implement VGICv3 CPU interface access
2016-08-24 11:20 ` [RFC PATCH v3 4/5] arm/arm64: vgic-new: Implement VGICv3 CPU interface access vijay.kilari at gmail.com
@ 2016-08-30 13:45 ` Christoffer Dall
[not found] ` <CALicx6vW9Lu7nNu86d-+a985iSH2vZ7ekb_5AgCxct-z90M7Wg@mail.gmail.com>
2016-09-06 17:10 ` Christoffer Dall
1 sibling, 1 reply; 23+ messages in thread
From: Christoffer Dall @ 2016-08-30 13:45 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 24, 2016 at 04:50:08PM +0530, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> VGICv3 CPU interface registers are accessed using
> KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
> as 64-bit. The cpu MPIDR value is passed along with register id.
> is used to identify the cpu for registers access.
>
> The version of VGIC v3 specification is define here
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
> arch/arm64/include/uapi/asm/kvm.h | 15 ++-
> arch/arm64/kvm/Makefile | 1 +
> include/linux/irqchip/arm-gic-v3.h | 4 +
> virt/kvm/arm/vgic/vgic-kvm-device.c | 29 +++++
> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 6 +
> virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 211 ++++++++++++++++++++++++++++++++++++
> virt/kvm/arm/vgic/vgic.h | 6 +
> 8 files changed, 271 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 94ea676..b13c944 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -182,14 +182,14 @@ struct kvm_arch_memory_slot {
> KVM_REG_ARM64_SYSREG_ ## n ## _MASK)
>
> #define __ARM64_SYS_REG(op0,op1,crn,crm,op2) \
> - (KVM_REG_ARM64 | KVM_REG_ARM64_SYSREG | \
> - ARM64_SYS_REG_SHIFT_MASK(op0, OP0) | \
> + (ARM64_SYS_REG_SHIFT_MASK(op0, OP0) | \
> ARM64_SYS_REG_SHIFT_MASK(op1, OP1) | \
> ARM64_SYS_REG_SHIFT_MASK(crn, CRN) | \
> ARM64_SYS_REG_SHIFT_MASK(crm, CRM) | \
> ARM64_SYS_REG_SHIFT_MASK(op2, OP2))
eh, no, please don't modify an exported userspace API header file in
this way.
If you want to reuse this, then add a new define for the sysreg field
generators and call that from __ARM64_SYS_REG() and from
KVM_DEV_ARM_VGIC_SYSREG.
>
> -#define ARM64_SYS_REG(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U64)
> +#define ARM64_SYS_REG(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U64 | \
> + KVM_REG_ARM64 | KVM_REG_ARM64_SYSREG)
>
> #define KVM_REG_ARM_TIMER_CTL ARM64_SYS_REG(3, 3, 14, 3, 1)
> #define KVM_REG_ARM_TIMER_CNT ARM64_SYS_REG(3, 3, 14, 3, 2)
> @@ -208,7 +208,16 @@ struct kvm_arch_memory_slot {
> #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
> #define KVM_DEV_ARM_VGIC_GRP_CTRL 4
> #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
> +#define KVM_DEV_ARM_VGIC_CPU_SYSREGS 6
> +
> #define KVM_DEV_ARM_VGIC_CTRL_INIT 0
> +#define KVM_DEV_ARM_VGIC_SYSREG_MASK (KVM_REG_ARM64_SYSREG_OP0_MASK | \
> + KVM_REG_ARM64_SYSREG_OP1_MASK | \
> + KVM_REG_ARM64_SYSREG_CRN_MASK | \
> + KVM_REG_ARM64_SYSREG_CRM_MASK | \
> + KVM_REG_ARM64_SYSREG_OP2_MASK)
we didn't need this for the existing userspace to kernel sysreg
interface, why do we need this now, and even need to export it to
userspace?
> +#define KVM_DEV_ARM_VGIC_SYSREG(op0, op1, crn, crm, op2) \
> + __ARM64_SYS_REG(op0, op1, crn, crm, op2)
>
> /* Device Control API on vcpu fd */
> #define KVM_ARM_VCPU_PMU_V3_CTRL 0
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index 695eb3c..dafbd0e 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -31,5 +31,6 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v3.o
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/irqchip.o
> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-sys-reg-v3.o
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
> kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index 56b0b7e..164463b 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -383,6 +383,10 @@
>
> #define ICH_VMCR_CTLR_SHIFT 0
> #define ICH_VMCR_CTLR_MASK (0x21f << ICH_VMCR_CTLR_SHIFT)
> +#define ICH_VMCR_ENG0_SHIFT 0
> +#define ICH_VMCR_ENG0 (1 << ICH_VMCR_ENG0_SHIFT)
> +#define ICH_VMCR_ENG1_SHIFT 1
> +#define ICH_VMCR_ENG1 (1 << ICH_VMCR_ENG1_SHIFT)
> #define ICH_VMCR_BPR1_SHIFT 18
> #define ICH_VMCR_BPR1_MASK (7 << ICH_VMCR_BPR1_SHIFT)
> #define ICH_VMCR_BPR0_SHIFT 21
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index 06f0158..74e5c38 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -500,6 +500,15 @@ static int vgic_attr_regs_access_v3(struct kvm_device *dev,
> if (!is_write)
> *reg = tmp32;
> break;
> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
> + u64 regid;
> +
> + regid = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_MASK) |
> + KVM_REG_SIZE_U64;
I think the simpler way, which is analogous to what the other sysreg
access functions do, it so simply define a mask fort the 'instr' field
(0xffff) and let the lookup function worry about the individual fields.
> + ret = vgic_v3_cpu_sysregs_uaccess(vcpu, is_write,
> + regid, reg);
> + break;
> + }
> default:
> ret = -EINVAL;
> break;
> @@ -533,6 +542,15 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
> reg = tmp32;
> return vgic_attr_regs_access_v3(dev, attr, ®, true);
> }
> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
> + u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> + u64 reg;
> +
> + if (get_user(reg, uaddr))
> + return -EFAULT;
> +
> + return vgic_attr_regs_access_v3(dev, attr, ®, true);
> + }
> }
> return -ENXIO;
> }
> @@ -560,6 +578,16 @@ static int vgic_v3_get_attr(struct kvm_device *dev,
> ret = put_user(tmp32, uaddr);
> return ret;
> }
> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
> + u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> + u64 reg;
> +
> + ret = vgic_attr_regs_access_v3(dev, attr, ®, false);
> + if (ret)
> + return ret;
> + ret = put_user(reg, uaddr);
> + return ret;
nit: you can just do 'return put_user(reg, uaddr);'
> + }
> }
>
> return -ENXIO;
> @@ -578,6 +606,7 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
> break;
> case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS:
> return vgic_v3_has_attr_regs(dev, attr);
> case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
> return 0;
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index cd37159..4a35eb8 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -212,7 +212,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu,
> }
> }
>
> -static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
> +void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
> {
> if (kvm_vgic_global_state.type == VGIC_V2)
> vgic_v2_set_vmcr(vcpu, vmcr);
> @@ -220,7 +220,7 @@ static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
> vgic_v3_set_vmcr(vcpu, vmcr);
> }
>
> -static void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
> +void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
> {
> if (kvm_vgic_global_state.type == VGIC_V2)
> vgic_v2_get_vmcr(vcpu, vmcr);
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index c2df103..61abea0 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -23,6 +23,7 @@
>
> #include "vgic.h"
> #include "vgic-mmio.h"
> +#include "sys_regs.h"
>
> /* extract @num bytes at @offset bytes offset in data */
> unsigned long extract_bytes(unsigned long data, unsigned int offset,
> @@ -598,6 +599,11 @@ int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
> nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers);
> break;
> }
> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
> + u64 reg;
> +
> + return vgic_v3_has_cpu_sysregs_attr(vcpu, 0, cpuid, ®);
> + }
> default:
> return -ENXIO;
> }
> diff --git a/virt/kvm/arm/vgic/vgic-sys-reg-v3.c b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
> new file mode 100644
> index 0000000..581d053
> --- /dev/null
> +++ b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
> @@ -0,0 +1,211 @@
> +#include <linux/irqchip/arm-gic-v3.h>
> +#include <linux/kvm.h>
> +#include <linux/kvm_host.h>
> +#include <kvm/iodev.h>
> +#include <kvm/arm_vgic.h>
> +#include <asm/kvm_emulate.h>
> +#include <asm/kvm_arm.h>
> +#include <asm/kvm_mmu.h>
> +
> +#include "vgic.h"
> +#include "vgic-mmio.h"
> +#include "sys_regs.h"
> +
> +static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_vmcr vmcr;
> +
> + vgic_get_vmcr(vcpu, &vmcr);
> + if (p->is_write) {
> + vmcr.ctlr = (u32)p->regval;
> + vgic_set_vmcr(vcpu, &vmcr);
> + } else {
> + p->regval = vmcr.ctlr;
> + }
> +
really? Have you looked at the spec and implementation of this or did
you just copy the v2 code?
The ICH_VMCR_EL2 register field mappings are not identical to the ctlr
mappings. I think this causes some rework for much of this patch, so
I'll have a look at the next revision.
> + return true;
> +}
> +
> +static bool access_gic_pmr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_vmcr vmcr;
> +
> + vgic_get_vmcr(vcpu, &vmcr);
> + if (p->is_write) {
> + vmcr.pmr = (u32)p->regval;
> + vgic_set_vmcr(vcpu, &vmcr);
> + } else {
> + p->regval = vmcr.pmr;
> + }
> +
> + return true;
> +}
> +
> +static bool access_gic_bpr0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_vmcr vmcr;
> +
> + vgic_get_vmcr(vcpu, &vmcr);
> + if (p->is_write) {
> + vmcr.bpr = (u32)p->regval;
> + vgic_set_vmcr(vcpu, &vmcr);
> + } else {
> + p->regval = vmcr.bpr;
> + }
> +
> + return true;
> +}
> +
> +static bool access_gic_bpr1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_vmcr vmcr;
> +
> + vgic_get_vmcr(vcpu, &vmcr);
> + if (p->is_write) {
> + vmcr.abpr = (u32)p->regval;
> + vgic_set_vmcr(vcpu, &vmcr);
> + } else {
> + p->regval = vmcr.abpr;
> + }
> +
> + return true;
> +}
> +
> +static bool access_gic_grpen0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> + if (p->is_write) {
> + vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG0;
> + vgicv3->vgic_vmcr |= (p->regval << ICH_VMCR_ENG0_SHIFT) &
> + ICH_VMCR_ENG0;
> + } else {
> + p->regval = (vgicv3->vgic_vmcr & ICH_VMCR_ENG0) >>
> + ICH_VMCR_ENG0_SHIFT;
> + }
so for example, why shouldn't these go through the vgic_set/get_vmcr
wrappers?
> +
> + return true;
> +}
> +
> +static bool access_gic_grpen1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> + if (p->is_write) {
> + vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG1;
> + vgicv3->vgic_vmcr |= (p->regval << ICH_VMCR_ENG1_SHIFT) &
> + ICH_VMCR_ENG1;
> + } else {
> + p->regval = (vgicv3->vgic_vmcr & ICH_VMCR_ENG1) >>
> + ICH_VMCR_ENG1_SHIFT;
> + }
> +
> + return true;
> +}
> +
> +static bool access_gic_ap0r(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> + u8 idx = r->Op2 & 3;
> +
> + if (p->is_write)
> + vgicv3->vgic_ap0r[idx] = p->regval;
> + else
> + p->regval = vgicv3->vgic_ap0r[idx];
> +
> + return true;
> +}
> +
> +static bool access_gic_ap1r(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> + u8 idx = r->Op2 & 3;
> +
> + if (p->is_write)
> + vgicv3->vgic_ap1r[idx] = p->regval;
> + else
> + p->regval = vgicv3->vgic_ap1r[idx];
> +
> + return true;
> +}
> +
> +static const struct sys_reg_desc gic_v3_icc_reg_descs[] = {
> + /* ICC_PMR_EL1 */
> + { Op0(3), Op1(0), CRn(4), CRm(6), Op2(0), access_gic_pmr },
> + /* ICC_BPR0_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(8), Op2(3), access_gic_bpr0 },
> + /* ICC_AP0R0_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(8), Op2(4), access_gic_ap0r },
> + /* ICC_AP0R1_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(8), Op2(5), access_gic_ap0r },
> + /* ICC_AP0R2_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(8), Op2(6), access_gic_ap0r },
> + /* ICC_AP0R3_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(8), Op2(7), access_gic_ap0r },
> + /* ICC_AP1R0_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(9), Op2(0), access_gic_ap1r },
> + /* ICC_AP1R1_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(9), Op2(1), access_gic_ap1r },
> + /* ICC_AP1R2_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(9), Op2(2), access_gic_ap1r },
> + /* ICC_AP1R3_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(9), Op2(3), access_gic_ap1r },
> + /* ICC_BPR1_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(12), Op2(3), access_gic_bpr1 },
> + /* ICC_CTLR_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(12), Op2(4), access_gic_ctlr },
> + /* ICC_IGRPEN0_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(12), Op2(6), access_gic_grpen0 },
> + /* ICC_GRPEN1_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(12), Op2(7), access_gic_grpen1 },
Do we need to allow userspace to at least read ICC_SRE_EL1?
Should we verify that the DIB and FDB fields of that register are
written as the system understands them (clear, WI)?
> +};
> +
> +int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
> + u64 *reg)
> +{
> + struct sys_reg_params params;
> +
> + params.regval = le64_to_cpu(*reg);
> + params.is_write = is_write;
> + params.is_aarch32 = false;
> + params.is_32bit = false;
> +
> + return find_reg_by_id(id, ¶ms, gic_v3_icc_reg_descs,
> + ARRAY_SIZE(gic_v3_icc_reg_descs)) ?
> + 0 : -ENXIO;
this looks terrible, please rewrite without the ternary operator.
> +}
> +
> +int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, u64 id,
> + u64 *reg)
> +{
> + struct sys_reg_params params;
> + const struct sys_reg_desc *r;
> +
> + if (is_write)
> + params.regval = le64_to_cpu(*reg);
why do we need this conversion here?
> + 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 (!r->access(vcpu, ¶ms, r))
> + return -EINVAL;
> +
> + if (!is_write)
> + *reg = cpu_to_le64(params.regval);
same question as above
> +
> + return 0;
> +}
> +
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index 14e4ce5..20eab36c 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -96,6 +96,10 @@ int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> int offset, u32 *val);
> int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> int offset, u32 *val);
> +int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> + u64 id, u64 *val);
> +int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
> + u64 *reg);
> #else
> static inline void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu)
> {
> @@ -169,6 +173,8 @@ static inline int vgic_its_inject_msi(struct kvm *kvm, struct kvm_msi *msi)
> #endif
>
> int kvm_register_vgic_device(unsigned long type);
> +void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
> +void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
> int vgic_lazy_init(struct kvm *kvm);
> int vgic_init(struct kvm *kvm);
>
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 23+ messages in thread* [RFC PATCH v3 4/5] arm/arm64: vgic-new: Implement VGICv3 CPU interface access
2016-08-24 11:20 ` [RFC PATCH v3 4/5] arm/arm64: vgic-new: Implement VGICv3 CPU interface access vijay.kilari at gmail.com
2016-08-30 13:45 ` Christoffer Dall
@ 2016-09-06 17:10 ` Christoffer Dall
1 sibling, 0 replies; 23+ messages in thread
From: Christoffer Dall @ 2016-09-06 17:10 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 24, 2016 at 04:50:08PM +0530, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> VGICv3 CPU interface registers are accessed using
> KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
> as 64-bit. The cpu MPIDR value is passed along with register id.
> is used to identify the cpu for registers access.
>
> The version of VGIC v3 specification is define here
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
> arch/arm64/include/uapi/asm/kvm.h | 15 ++-
> arch/arm64/kvm/Makefile | 1 +
> include/linux/irqchip/arm-gic-v3.h | 4 +
> virt/kvm/arm/vgic/vgic-kvm-device.c | 29 +++++
> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 6 +
> virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 211 ++++++++++++++++++++++++++++++++++++
> virt/kvm/arm/vgic/vgic.h | 6 +
> 8 files changed, 271 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 94ea676..b13c944 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -182,14 +182,14 @@ struct kvm_arch_memory_slot {
> KVM_REG_ARM64_SYSREG_ ## n ## _MASK)
>
> #define __ARM64_SYS_REG(op0,op1,crn,crm,op2) \
> - (KVM_REG_ARM64 | KVM_REG_ARM64_SYSREG | \
> - ARM64_SYS_REG_SHIFT_MASK(op0, OP0) | \
> + (ARM64_SYS_REG_SHIFT_MASK(op0, OP0) | \
> ARM64_SYS_REG_SHIFT_MASK(op1, OP1) | \
> ARM64_SYS_REG_SHIFT_MASK(crn, CRN) | \
> ARM64_SYS_REG_SHIFT_MASK(crm, CRM) | \
> ARM64_SYS_REG_SHIFT_MASK(op2, OP2))
>
> -#define ARM64_SYS_REG(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U64)
> +#define ARM64_SYS_REG(...) (__ARM64_SYS_REG(__VA_ARGS__) | KVM_REG_SIZE_U64 | \
> + KVM_REG_ARM64 | KVM_REG_ARM64_SYSREG)
>
> #define KVM_REG_ARM_TIMER_CTL ARM64_SYS_REG(3, 3, 14, 3, 1)
> #define KVM_REG_ARM_TIMER_CNT ARM64_SYS_REG(3, 3, 14, 3, 2)
> @@ -208,7 +208,16 @@ struct kvm_arch_memory_slot {
> #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
> #define KVM_DEV_ARM_VGIC_GRP_CTRL 4
> #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
> +#define KVM_DEV_ARM_VGIC_CPU_SYSREGS 6
> +
> #define KVM_DEV_ARM_VGIC_CTRL_INIT 0
> +#define KVM_DEV_ARM_VGIC_SYSREG_MASK (KVM_REG_ARM64_SYSREG_OP0_MASK | \
> + KVM_REG_ARM64_SYSREG_OP1_MASK | \
> + KVM_REG_ARM64_SYSREG_CRN_MASK | \
> + KVM_REG_ARM64_SYSREG_CRM_MASK | \
> + KVM_REG_ARM64_SYSREG_OP2_MASK)
> +#define KVM_DEV_ARM_VGIC_SYSREG(op0, op1, crn, crm, op2) \
> + __ARM64_SYS_REG(op0, op1, crn, crm, op2)
>
> /* Device Control API on vcpu fd */
> #define KVM_ARM_VCPU_PMU_V3_CTRL 0
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index 695eb3c..dafbd0e 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -31,5 +31,6 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v3.o
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/irqchip.o
> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-sys-reg-v3.o
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
> kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
> index 56b0b7e..164463b 100644
> --- a/include/linux/irqchip/arm-gic-v3.h
> +++ b/include/linux/irqchip/arm-gic-v3.h
> @@ -383,6 +383,10 @@
>
> #define ICH_VMCR_CTLR_SHIFT 0
> #define ICH_VMCR_CTLR_MASK (0x21f << ICH_VMCR_CTLR_SHIFT)
> +#define ICH_VMCR_ENG0_SHIFT 0
> +#define ICH_VMCR_ENG0 (1 << ICH_VMCR_ENG0_SHIFT)
> +#define ICH_VMCR_ENG1_SHIFT 1
> +#define ICH_VMCR_ENG1 (1 << ICH_VMCR_ENG1_SHIFT)
> #define ICH_VMCR_BPR1_SHIFT 18
> #define ICH_VMCR_BPR1_MASK (7 << ICH_VMCR_BPR1_SHIFT)
> #define ICH_VMCR_BPR0_SHIFT 21
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index 06f0158..74e5c38 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -500,6 +500,15 @@ static int vgic_attr_regs_access_v3(struct kvm_device *dev,
> if (!is_write)
> *reg = tmp32;
> break;
> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
> + u64 regid;
> +
> + regid = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_MASK) |
> + KVM_REG_SIZE_U64;
> + ret = vgic_v3_cpu_sysregs_uaccess(vcpu, is_write,
> + regid, reg);
> + break;
> + }
> default:
> ret = -EINVAL;
> break;
> @@ -533,6 +542,15 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
> reg = tmp32;
> return vgic_attr_regs_access_v3(dev, attr, ®, true);
> }
> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
> + u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> + u64 reg;
> +
> + if (get_user(reg, uaddr))
> + return -EFAULT;
> +
> + return vgic_attr_regs_access_v3(dev, attr, ®, true);
> + }
> }
> return -ENXIO;
> }
> @@ -560,6 +578,16 @@ static int vgic_v3_get_attr(struct kvm_device *dev,
> ret = put_user(tmp32, uaddr);
> return ret;
> }
> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
> + u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> + u64 reg;
> +
> + ret = vgic_attr_regs_access_v3(dev, attr, ®, false);
> + if (ret)
> + return ret;
> + ret = put_user(reg, uaddr);
> + return ret;
> + }
> }
>
> return -ENXIO;
> @@ -578,6 +606,7 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
> break;
> case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS:
> return vgic_v3_has_attr_regs(dev, attr);
> case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
> return 0;
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index cd37159..4a35eb8 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -212,7 +212,7 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu,
> }
> }
>
> -static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
> +void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
> {
> if (kvm_vgic_global_state.type == VGIC_V2)
> vgic_v2_set_vmcr(vcpu, vmcr);
> @@ -220,7 +220,7 @@ static void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
> vgic_v3_set_vmcr(vcpu, vmcr);
> }
>
> -static void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
> +void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr)
> {
> if (kvm_vgic_global_state.type == VGIC_V2)
> vgic_v2_get_vmcr(vcpu, vmcr);
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index c2df103..61abea0 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -23,6 +23,7 @@
>
> #include "vgic.h"
> #include "vgic-mmio.h"
> +#include "sys_regs.h"
>
> /* extract @num bytes at @offset bytes offset in data */
> unsigned long extract_bytes(unsigned long data, unsigned int offset,
> @@ -598,6 +599,11 @@ int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
> nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers);
> break;
> }
> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
> + u64 reg;
> +
> + return vgic_v3_has_cpu_sysregs_attr(vcpu, 0, cpuid, ®);
> + }
> default:
> return -ENXIO;
> }
> diff --git a/virt/kvm/arm/vgic/vgic-sys-reg-v3.c b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
> new file mode 100644
> index 0000000..581d053
> --- /dev/null
> +++ b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
> @@ -0,0 +1,211 @@
> +#include <linux/irqchip/arm-gic-v3.h>
> +#include <linux/kvm.h>
> +#include <linux/kvm_host.h>
> +#include <kvm/iodev.h>
> +#include <kvm/arm_vgic.h>
> +#include <asm/kvm_emulate.h>
> +#include <asm/kvm_arm.h>
> +#include <asm/kvm_mmu.h>
> +
> +#include "vgic.h"
> +#include "vgic-mmio.h"
> +#include "sys_regs.h"
> +
> +static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_vmcr vmcr;
> +
> + vgic_get_vmcr(vcpu, &vmcr);
> + if (p->is_write) {
> + vmcr.ctlr = (u32)p->regval;
> + vgic_set_vmcr(vcpu, &vmcr);
> + } else {
> + p->regval = vmcr.ctlr;
> + }
> +
> + return true;
> +}
> +
> +static bool access_gic_pmr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_vmcr vmcr;
> +
> + vgic_get_vmcr(vcpu, &vmcr);
> + if (p->is_write) {
> + vmcr.pmr = (u32)p->regval;
> + vgic_set_vmcr(vcpu, &vmcr);
> + } else {
> + p->regval = vmcr.pmr;
> + }
> +
> + return true;
> +}
> +
> +static bool access_gic_bpr0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_vmcr vmcr;
> +
> + vgic_get_vmcr(vcpu, &vmcr);
> + if (p->is_write) {
> + vmcr.bpr = (u32)p->regval;
> + vgic_set_vmcr(vcpu, &vmcr);
> + } else {
> + p->regval = vmcr.bpr;
> + }
> +
> + return true;
> +}
> +
> +static bool access_gic_bpr1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_vmcr vmcr;
> +
> + vgic_get_vmcr(vcpu, &vmcr);
> + if (p->is_write) {
> + vmcr.abpr = (u32)p->regval;
> + vgic_set_vmcr(vcpu, &vmcr);
> + } else {
> + p->regval = vmcr.abpr;
> + }
> +
> + return true;
> +}
> +
> +static bool access_gic_grpen0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> + if (p->is_write) {
> + vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG0;
> + vgicv3->vgic_vmcr |= (p->regval << ICH_VMCR_ENG0_SHIFT) &
> + ICH_VMCR_ENG0;
> + } else {
> + p->regval = (vgicv3->vgic_vmcr & ICH_VMCR_ENG0) >>
> + ICH_VMCR_ENG0_SHIFT;
> + }
> +
> + return true;
> +}
> +
> +static bool access_gic_grpen1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> + if (p->is_write) {
> + vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG1;
> + vgicv3->vgic_vmcr |= (p->regval << ICH_VMCR_ENG1_SHIFT) &
> + ICH_VMCR_ENG1;
> + } else {
> + p->regval = (vgicv3->vgic_vmcr & ICH_VMCR_ENG1) >>
> + ICH_VMCR_ENG1_SHIFT;
> + }
> +
> + return true;
> +}
> +
> +static bool access_gic_ap0r(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> + u8 idx = r->Op2 & 3;
> +
> + if (p->is_write)
> + vgicv3->vgic_ap0r[idx] = p->regval;
> + else
> + p->regval = vgicv3->vgic_ap0r[idx];
> +
> + return true;
> +}
> +
> +static bool access_gic_ap1r(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> + const struct sys_reg_desc *r)
> +{
> + struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> + u8 idx = r->Op2 & 3;
> +
> + if (p->is_write)
> + vgicv3->vgic_ap1r[idx] = p->regval;
> + else
> + p->regval = vgicv3->vgic_ap1r[idx];
> +
> + return true;
> +}
> +
> +static const struct sys_reg_desc gic_v3_icc_reg_descs[] = {
> + /* ICC_PMR_EL1 */
> + { Op0(3), Op1(0), CRn(4), CRm(6), Op2(0), access_gic_pmr },
> + /* ICC_BPR0_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(8), Op2(3), access_gic_bpr0 },
> + /* ICC_AP0R0_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(8), Op2(4), access_gic_ap0r },
> + /* ICC_AP0R1_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(8), Op2(5), access_gic_ap0r },
> + /* ICC_AP0R2_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(8), Op2(6), access_gic_ap0r },
> + /* ICC_AP0R3_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(8), Op2(7), access_gic_ap0r },
> + /* ICC_AP1R0_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(9), Op2(0), access_gic_ap1r },
> + /* ICC_AP1R1_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(9), Op2(1), access_gic_ap1r },
> + /* ICC_AP1R2_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(9), Op2(2), access_gic_ap1r },
> + /* ICC_AP1R3_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(9), Op2(3), access_gic_ap1r },
> + /* ICC_BPR1_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(12), Op2(3), access_gic_bpr1 },
> + /* ICC_CTLR_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(12), Op2(4), access_gic_ctlr },
> + /* ICC_IGRPEN0_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(12), Op2(6), access_gic_grpen0 },
> + /* ICC_GRPEN1_EL1 */
> + { Op0(3), Op1(0), CRn(12), CRm(12), Op2(7), access_gic_grpen1 },
> +};
> +
> +int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
> + u64 *reg)
> +{
> + struct sys_reg_params params;
> +
> + params.regval = le64_to_cpu(*reg);
> + params.is_write = is_write;
> + params.is_aarch32 = false;
> + params.is_32bit = false;
> +
> + return find_reg_by_id(id, ¶ms, gic_v3_icc_reg_descs,
> + ARRAY_SIZE(gic_v3_icc_reg_descs)) ?
> + 0 : -ENXIO;
> +}
> +
> +int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, u64 id,
> + u64 *reg)
> +{
> + struct sys_reg_params params;
> + const struct sys_reg_desc *r;
> +
> + if (is_write)
> + params.regval = le64_to_cpu(*reg);
> + params.is_write = is_write;
> + params.is_aarch32 = false;
> + params.is_32bit = false;
> +
> + r = find_reg_by_id(id, ¶ms, gic_v3_icc_reg_descs,
> + ARRAY_SIZE(gic_v3_icc_reg_descs));
> + if (!r)
> + return -ENXIO;
> +
> + if (!r->access(vcpu, ¶ms, r))
> + return -EINVAL;
> +
> + if (!is_write)
> + *reg = cpu_to_le64(params.regval);
> +
> + return 0;
> +}
> +
Btw. I get a warning about this stray white space line at the end of the
file here when applying these patches.
Thanks,
-Christoffer
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index 14e4ce5..20eab36c 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -96,6 +96,10 @@ int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> int offset, u32 *val);
> int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> int offset, u32 *val);
> +int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> + u64 id, u64 *val);
> +int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
> + u64 *reg);
> #else
> static inline void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu)
> {
> @@ -169,6 +173,8 @@ static inline int vgic_its_inject_msi(struct kvm *kvm, struct kvm_msi *msi)
> #endif
>
> int kvm_register_vgic_device(unsigned long type);
> +void vgic_set_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
> +void vgic_get_vmcr(struct kvm_vcpu *vcpu, struct vgic_vmcr *vmcr);
> int vgic_lazy_init(struct kvm *kvm);
> int vgic_init(struct kvm *kvm);
>
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [RFC PATCH v3 5/5] arm/arm64: vgic-new: Implement KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO ioctl
2016-08-24 11:20 [RFC PATCH v3 0/5] arm/arm64: vgic-new: Implement API for vGICv3 live migration vijay.kilari at gmail.com
` (3 preceding siblings ...)
2016-08-24 11:20 ` [RFC PATCH v3 4/5] arm/arm64: vgic-new: Implement VGICv3 CPU interface access vijay.kilari at gmail.com
@ 2016-08-24 11:20 ` vijay.kilari at gmail.com
2016-08-30 14:00 ` Christoffer Dall
4 siblings, 1 reply; 23+ messages in thread
From: vijay.kilari at gmail.com @ 2016-08-24 11:20 UTC (permalink / raw)
To: linux-arm-kernel
From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
Userspace requires to store and restore of line_level for
level triggered interrupts. For this ioctl KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO
is defined.
Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
---
arch/arm64/include/uapi/asm/kvm.h | 6 +++++
virt/kvm/arm/vgic/vgic-kvm-device.c | 44 ++++++++++++++++++++++++++++++++++++-
virt/kvm/arm/vgic/vgic-mmio-v3.c | 19 ++++++++++++++++
virt/kvm/arm/vgic/vgic-mmio.c | 34 ++++++++++++++++++++++++++++
virt/kvm/arm/vgic/vgic-mmio.h | 6 +++++
virt/kvm/arm/vgic/vgic.h | 3 +++
6 files changed, 111 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index b13c944..45c56d7 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -209,6 +209,12 @@ struct kvm_arch_memory_slot {
#define KVM_DEV_ARM_VGIC_GRP_CTRL 4
#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
#define KVM_DEV_ARM_VGIC_CPU_SYSREGS 6
+#define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7
+#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT 9
+#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK \
+ (0x7fffffULL << KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT)
+#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x1ff
+#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_VAL 1
#define KVM_DEV_ARM_VGIC_CTRL_INIT 0
#define KVM_DEV_ARM_VGIC_SYSREG_MASK (KVM_REG_ARM64_SYSREG_OP0_MASK | \
diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
index 74e5c38..7e3bc49 100644
--- a/virt/kvm/arm/vgic/vgic-kvm-device.c
+++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
@@ -509,6 +509,23 @@ static int vgic_attr_regs_access_v3(struct kvm_device *dev,
regid, reg);
break;
}
+ case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
+ unsigned int info, intid;
+
+ info = (attr->attr & KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK) >>
+ KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT;
+ if (info == KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_VAL) {
+ intid = attr->attr &
+ KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK;
+ ret = vgic_v3_line_level_info_uaccess(vcpu, is_write,
+ intid, &tmp32);
+ if (!is_write)
+ *reg = tmp32;
+ } else {
+ ret = -EINVAL;
+ }
+ break;
+ }
default:
ret = -EINVAL;
break;
@@ -551,6 +568,17 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
return vgic_attr_regs_access_v3(dev, attr, ®, true);
}
+ case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
+ u32 __user *uaddr = (u32 __user *)(long)attr->addr;
+ u64 reg;
+ u32 tmp32;
+
+ if (get_user(tmp32, uaddr))
+ return -EFAULT;
+
+ reg = tmp32;
+ return vgic_attr_regs_access_v3(dev, attr, ®, true);
+ }
}
return -ENXIO;
}
@@ -588,8 +616,19 @@ static int vgic_v3_get_attr(struct kvm_device *dev,
ret = put_user(reg, uaddr);
return ret;
}
- }
+ case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
+ u32 __user *uaddr = (u32 __user *)(long)attr->addr;
+ u64 reg;
+ u32 tmp32;
+ ret = vgic_attr_regs_access_v3(dev, attr, ®, false);
+ if (ret)
+ return ret;
+ tmp32 = reg;
+ ret = put_user(tmp32, uaddr);
+ return ret;
+ }
+ }
return -ENXIO;
}
@@ -610,11 +649,14 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
return vgic_v3_has_attr_regs(dev, attr);
case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
return 0;
+ case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO:
+ return 0;
case KVM_DEV_ARM_VGIC_GRP_CTRL:
switch (attr->attr) {
case KVM_DEV_ARM_VGIC_CTRL_INIT:
return 0;
}
+ break;
}
return -ENXIO;
}
diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
index 61abea0..fde1472 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
@@ -789,3 +789,22 @@ int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
return vgic_v3_uaccess(vcpu, dev, is_write, offset, val);
}
+
+int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, bool is_write,
+ u32 intid, u32 *val)
+{
+ unsigned int len = 4;
+ u8 buf[4];
+ int ret;
+
+ if (is_write) {
+ vgic_data_host_to_mmio_bus(buf, len, *val);
+ ret = vgic_write_irq_line_level_info(vcpu, intid, len, buf);
+ } else {
+ ret = vgic_read_irq_line_level_info(vcpu, intid, len, buf);
+ if (!ret)
+ *val = vgic_data_mmio_bus_to_host(buf, len);
+ }
+
+ return ret;
+}
diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
index 38f2c75..74d0449 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.c
+++ b/virt/kvm/arm/vgic/vgic-mmio.c
@@ -391,6 +391,40 @@ void vgic_mmio_write_config(struct kvm_vcpu *vcpu,
}
}
+int vgic_read_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid,
+ unsigned int len, void *val)
+{
+ unsigned long data = 0;
+ int i;
+
+ for (i = 0; i < len * 8; i++) {
+ struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
+
+ if (irq->line_level)
+ data |= (1U << i);
+ }
+ vgic_data_host_to_mmio_bus(val, len, data);
+
+ return 0;
+}
+
+int vgic_write_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid,
+ unsigned int len, const void *val)
+{
+ int i;
+ unsigned long data = vgic_data_mmio_bus_to_host(val, len);
+
+ for_each_set_bit(i, &data, len * 8) {
+ struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
+
+ spin_lock(&irq->irq_lock);
+ irq->line_level = true;
+ spin_unlock(&irq->irq_lock);
+ }
+
+ return 0;
+}
+
static int match_region(const void *key, const void *elt)
{
const unsigned int offset = (unsigned long)key;
diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
index b97a97b..b03c4e7 100644
--- a/virt/kvm/arm/vgic/vgic-mmio.h
+++ b/virt/kvm/arm/vgic/vgic-mmio.h
@@ -183,6 +183,12 @@ int vgic_mmio_uaccess_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
int vgic_mmio_uaccess_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
gpa_t addr, int len, const void *val);
+int vgic_read_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid,
+ unsigned int len, void *val);
+
+int vgic_write_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid,
+ unsigned int len, const void *val);
+
unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev);
unsigned int vgic_v3_init_dist_iodev(struct vgic_io_device *dev);
diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
index 20eab36c..b8ee5b9 100644
--- a/virt/kvm/arm/vgic/vgic.h
+++ b/virt/kvm/arm/vgic/vgic.h
@@ -100,6 +100,9 @@ int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write,
u64 id, u64 *val);
int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
u64 *reg);
+int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, bool is_write,
+ u32 intid, u32 *val);
+
#else
static inline void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu)
{
--
1.9.1
^ permalink raw reply related [flat|nested] 23+ messages in thread* [RFC PATCH v3 5/5] arm/arm64: vgic-new: Implement KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO ioctl
2016-08-24 11:20 ` [RFC PATCH v3 5/5] arm/arm64: vgic-new: Implement KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO ioctl vijay.kilari at gmail.com
@ 2016-08-30 14:00 ` Christoffer Dall
2016-08-30 14:07 ` Christoffer Dall
2016-09-06 14:12 ` Vijay Kilari
0 siblings, 2 replies; 23+ messages in thread
From: Christoffer Dall @ 2016-08-30 14:00 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 24, 2016 at 04:50:09PM +0530, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> Userspace requires to store and restore of line_level for
> level triggered interrupts. For this ioctl KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO
> is defined.
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
> arch/arm64/include/uapi/asm/kvm.h | 6 +++++
> virt/kvm/arm/vgic/vgic-kvm-device.c | 44 ++++++++++++++++++++++++++++++++++++-
> virt/kvm/arm/vgic/vgic-mmio-v3.c | 19 ++++++++++++++++
> virt/kvm/arm/vgic/vgic-mmio.c | 34 ++++++++++++++++++++++++++++
> virt/kvm/arm/vgic/vgic-mmio.h | 6 +++++
> virt/kvm/arm/vgic/vgic.h | 3 +++
> 6 files changed, 111 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index b13c944..45c56d7 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -209,6 +209,12 @@ struct kvm_arch_memory_slot {
> #define KVM_DEV_ARM_VGIC_GRP_CTRL 4
> #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
> #define KVM_DEV_ARM_VGIC_CPU_SYSREGS 6
> +#define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7
> +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT 9
This should be 10, bits 0 through 9 gives you 10 to work with.
> +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK \
> + (0x7fffffULL << KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT)
this mask is also wrong, 32 - 10 == 22, not 23.
> +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x1ff
this is also wrong, you have 10 bits, not 9 bits.
Hint: the max SPI number is around 1024.
> +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_VAL 1
This should really be KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO_LEVEL. Why is 0
not a valid value?
>
> #define KVM_DEV_ARM_VGIC_CTRL_INIT 0
> #define KVM_DEV_ARM_VGIC_SYSREG_MASK (KVM_REG_ARM64_SYSREG_OP0_MASK | \
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index 74e5c38..7e3bc49 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -509,6 +509,23 @@ static int vgic_attr_regs_access_v3(struct kvm_device *dev,
> regid, reg);
> break;
> }
> + case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
> + unsigned int info, intid;
> +
> + info = (attr->attr & KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK) >>
> + KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT;
> + if (info == KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_VAL) {
> + intid = attr->attr &
> + KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK;
> + ret = vgic_v3_line_level_info_uaccess(vcpu, is_write,
> + intid, &tmp32);
> + if (!is_write)
> + *reg = tmp32;
> + } else {
> + ret = -EINVAL;
> + }
> + break;
> + }
> default:
> ret = -EINVAL;
> break;
> @@ -551,6 +568,17 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
>
> return vgic_attr_regs_access_v3(dev, attr, ®, true);
> }
> + case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
> + u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> + u64 reg;
> + u32 tmp32;
here we have more type fun, which I assume you fix based on my comments
on a previous patch.
> +
> + if (get_user(tmp32, uaddr))
> + return -EFAULT;
> +
> + reg = tmp32;
> + return vgic_attr_regs_access_v3(dev, attr, ®, true);
> + }
> }
> return -ENXIO;
> }
> @@ -588,8 +616,19 @@ static int vgic_v3_get_attr(struct kvm_device *dev,
> ret = put_user(reg, uaddr);
> return ret;
> }
> - }
> + case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
> + u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> + u64 reg;
> + u32 tmp32;
>
> + ret = vgic_attr_regs_access_v3(dev, attr, ®, false);
> + if (ret)
> + return ret;
> + tmp32 = reg;
> + ret = put_user(tmp32, uaddr);
> + return ret;
> + }
> + }
> return -ENXIO;
> }
>
> @@ -610,11 +649,14 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
> return vgic_v3_has_attr_regs(dev, attr);
> case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
> return 0;
> + case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO:
> + return 0;
probably you should check the info field here as well
> case KVM_DEV_ARM_VGIC_GRP_CTRL:
> switch (attr->attr) {
> case KVM_DEV_ARM_VGIC_CTRL_INIT:
> return 0;
> }
> + break;
> }
> return -ENXIO;
> }
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 61abea0..fde1472 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -789,3 +789,22 @@ int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
>
> return vgic_v3_uaccess(vcpu, dev, is_write, offset, val);
> }
> +
> +int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> + u32 intid, u32 *val)
> +{
> + unsigned int len = 4;
> + u8 buf[4];
> + int ret;
> +
> + if (is_write) {
> + vgic_data_host_to_mmio_bus(buf, len, *val);
why do you involve the mmio bus in this?
> + ret = vgic_write_irq_line_level_info(vcpu, intid, len, buf);
> + } else {
> + ret = vgic_read_irq_line_level_info(vcpu, intid, len, buf);
> + if (!ret)
> + *val = vgic_data_mmio_bus_to_host(buf, len);
> + }
> +
> + return ret;
> +}
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index 38f2c75..74d0449 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -391,6 +391,40 @@ void vgic_mmio_write_config(struct kvm_vcpu *vcpu,
> }
> }
>
> +int vgic_read_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid,
> + unsigned int len, void *val)
> +{
> + unsigned long data = 0;
> + int i;
> +
> + for (i = 0; i < len * 8; i++) {
> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> +
> + if (irq->line_level)
> + data |= (1U << i);
> + }
> + vgic_data_host_to_mmio_bus(val, len, data);
why???
> +
> + return 0;
> +}
> +
> +int vgic_write_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid,
> + unsigned int len, const void *val)
> +{
> + int i;
> + unsigned long data = vgic_data_mmio_bus_to_host(val, len);
why???
> +
> + for_each_set_bit(i, &data, len * 8) {
> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> +
> + spin_lock(&irq->irq_lock);
> + irq->line_level = true;
> + spin_unlock(&irq->irq_lock);
> + }
> +
> + return 0;
> +}
> +
> static int match_region(const void *key, const void *elt)
> {
> const unsigned int offset = (unsigned long)key;
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
> index b97a97b..b03c4e7 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.h
> +++ b/virt/kvm/arm/vgic/vgic-mmio.h
> @@ -183,6 +183,12 @@ int vgic_mmio_uaccess_read(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> int vgic_mmio_uaccess_write(struct kvm_vcpu *vcpu, struct kvm_io_device *dev,
> gpa_t addr, int len, const void *val);
>
> +int vgic_read_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid,
> + unsigned int len, void *val);
> +
> +int vgic_write_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid,
> + unsigned int len, const void *val);
> +
> unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev);
>
> unsigned int vgic_v3_init_dist_iodev(struct vgic_io_device *dev);
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index 20eab36c..b8ee5b9 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -100,6 +100,9 @@ int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> u64 id, u64 *val);
> int vgic_v3_has_cpu_sysregs_attr(struct kvm_vcpu *vcpu, bool is_write, u64 id,
> u64 *reg);
> +int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> + u32 intid, u32 *val);
> +
> #else
> static inline void vgic_v3_process_maintenance(struct kvm_vcpu *vcpu)
> {
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 23+ messages in thread* [RFC PATCH v3 5/5] arm/arm64: vgic-new: Implement KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO ioctl
2016-08-30 14:00 ` Christoffer Dall
@ 2016-08-30 14:07 ` Christoffer Dall
2016-09-06 14:12 ` Vijay Kilari
1 sibling, 0 replies; 23+ messages in thread
From: Christoffer Dall @ 2016-08-30 14:07 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Aug 30, 2016 at 04:00:34PM +0200, Christoffer Dall wrote:
> On Wed, Aug 24, 2016 at 04:50:09PM +0530, vijay.kilari at gmail.com wrote:
> > From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> >
> > Userspace requires to store and restore of line_level for
> > level triggered interrupts. For this ioctl KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO
> > is defined.
> >
> > Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> > ---
> > arch/arm64/include/uapi/asm/kvm.h | 6 +++++
> > virt/kvm/arm/vgic/vgic-kvm-device.c | 44 ++++++++++++++++++++++++++++++++++++-
> > virt/kvm/arm/vgic/vgic-mmio-v3.c | 19 ++++++++++++++++
> > virt/kvm/arm/vgic/vgic-mmio.c | 34 ++++++++++++++++++++++++++++
> > virt/kvm/arm/vgic/vgic-mmio.h | 6 +++++
> > virt/kvm/arm/vgic/vgic.h | 3 +++
> > 6 files changed, 111 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> > index b13c944..45c56d7 100644
> > --- a/arch/arm64/include/uapi/asm/kvm.h
> > +++ b/arch/arm64/include/uapi/asm/kvm.h
> > @@ -209,6 +209,12 @@ struct kvm_arch_memory_slot {
> > #define KVM_DEV_ARM_VGIC_GRP_CTRL 4
> > #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
> > #define KVM_DEV_ARM_VGIC_CPU_SYSREGS 6
> > +#define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7
> > +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT 9
>
> This should be 10, bits 0 through 9 gives you 10 to work with.
>
> > +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK \
> > + (0x7fffffULL << KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT)
>
> this mask is also wrong, 32 - 10 == 22, not 23.
>
> > +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x1ff
>
> this is also wrong, you have 10 bits, not 9 bits.
>
> Hint: the max SPI number is around 1024.
>
> > +#define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_VAL 1
>
> This should really be KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO_LEVEL. Why is 0
> not a valid value?
>
actually, the API documentation specifies that this should simply be
defined as VGIC_LEVEL_INFO_LINE_LEVEL.
-Christoffer
^ permalink raw reply [flat|nested] 23+ messages in thread* [RFC PATCH v3 5/5] arm/arm64: vgic-new: Implement KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO ioctl
2016-08-30 14:00 ` Christoffer Dall
2016-08-30 14:07 ` Christoffer Dall
@ 2016-09-06 14:12 ` Vijay Kilari
2016-09-06 19:20 ` Christoffer Dall
1 sibling, 1 reply; 23+ messages in thread
From: Vijay Kilari @ 2016-09-06 14:12 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Aug 30, 2016 at 7:30 PM, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
>
> On Wed, Aug 24, 2016 at 04:50:09PM +0530, vijay.kilari at gmail.com wrote:
> > From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> > }
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > index 61abea0..fde1472 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > @@ -789,3 +789,22 @@ int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> >
> > return vgic_v3_uaccess(vcpu, dev, is_write, offset, val);
> > }
> > +
> > +int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> > + u32 intid, u32 *val)
> > +{
> > + unsigned int len = 4;
> > + u8 buf[4];
> > + int ret;
> > +
> > + if (is_write) {
> > + vgic_data_host_to_mmio_bus(buf, len, *val);
>
> why do you involve the mmio bus in this?
This is doing LE conversion. This is being done by vgic_uaccess function.
IRC, This sys register access in not following vgic_uaccess path. Hence
added it here.
>
>
> > + ret = vgic_write_irq_line_level_info(vcpu, intid, len, buf);
> > + } else {
> > + ret = vgic_read_irq_line_level_info(vcpu, intid, len, buf);
> > + if (!ret)
> > + *val = vgic_data_mmio_bus_to_host(buf, len);
> > + }
> > +
> > + return ret;
> > +}
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> > index 38f2c75..74d0449 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> > @@ -391,6 +391,40 @@ void vgic_mmio_write_config(struct kvm_vcpu *vcpu,
> > }
> > }
> >
> > +int vgic_read_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid,
> > + unsigned int len, void *val)
> > +{
> > + unsigned long data = 0;
> > + int i;
> > +
> > + for (i = 0; i < len * 8; i++) {
> > + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> > +
> > + if (irq->line_level)
> > + data |= (1U << i);
> > + }
> > + vgic_data_host_to_mmio_bus(val, len, data);
>
> why???
Same as above reason
>
>
> > +
> > + return 0;
> > +}
> > +
> > +int vgic_write_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid,
> > + unsigned int len, const void *val)
> > +{
> > + int i;
> > + unsigned long data = vgic_data_mmio_bus_to_host(val, len);
>
> why???
Same as above reason
^ permalink raw reply [flat|nested] 23+ messages in thread* [RFC PATCH v3 5/5] arm/arm64: vgic-new: Implement KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO ioctl
2016-09-06 14:12 ` Vijay Kilari
@ 2016-09-06 19:20 ` Christoffer Dall
0 siblings, 0 replies; 23+ messages in thread
From: Christoffer Dall @ 2016-09-06 19:20 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Sep 06, 2016 at 07:42:19PM +0530, Vijay Kilari wrote:
> On Tue, Aug 30, 2016 at 7:30 PM, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> >
> > On Wed, Aug 24, 2016 at 04:50:09PM +0530, vijay.kilari at gmail.com wrote:
> > > From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> > > }
> > > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > > index 61abea0..fde1472 100644
> > > --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > > +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> > > @@ -789,3 +789,22 @@ int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> > >
> > > return vgic_v3_uaccess(vcpu, dev, is_write, offset, val);
> > > }
> > > +
> > > +int vgic_v3_line_level_info_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> > > + u32 intid, u32 *val)
> > > +{
> > > + unsigned int len = 4;
> > > + u8 buf[4];
> > > + int ret;
> > > +
> > > + if (is_write) {
> > > + vgic_data_host_to_mmio_bus(buf, len, *val);
> >
> > why do you involve the mmio bus in this?
>
> This is doing LE conversion. This is being done by vgic_uaccess function.
> IRC, This sys register access in not following vgic_uaccess path. Hence
> added it here.
See my e-mail on the other patch. I think this is all just legacy from
the time where we didn't distinguish between uaccess and mmio
dispatches.
> >
> >
> > > + ret = vgic_write_irq_line_level_info(vcpu, intid, len, buf);
> > > + } else {
> > > + ret = vgic_read_irq_line_level_info(vcpu, intid, len, buf);
> > > + if (!ret)
> > > + *val = vgic_data_mmio_bus_to_host(buf, len);
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> > > index 38f2c75..74d0449 100644
> > > --- a/virt/kvm/arm/vgic/vgic-mmio.c
> > > +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> > > @@ -391,6 +391,40 @@ void vgic_mmio_write_config(struct kvm_vcpu *vcpu,
> > > }
> > > }
> > >
> > > +int vgic_read_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid,
> > > + unsigned int len, void *val)
> > > +{
> > > + unsigned long data = 0;
> > > + int i;
> > > +
> > > + for (i = 0; i < len * 8; i++) {
> > > + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> > > +
> > > + if (irq->line_level)
> > > + data |= (1U << i);
> > > + }
> > > + vgic_data_host_to_mmio_bus(val, len, data);
> >
> > why???
>
> Same as above reason
> >
> >
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +int vgic_write_irq_line_level_info(struct kvm_vcpu *vcpu, u32 intid,
> > > + unsigned int len, const void *val)
> > > +{
> > > + int i;
> > > + unsigned long data = vgic_data_mmio_bus_to_host(val, len);
> >
> > why???
>
> Same as above reason
Thanks,
-Christoffer
^ permalink raw reply [flat|nested] 23+ messages in thread