Re: [PATCH 52/59] KVM: arm64: nv: vgic: Allow userland to set VGIC maintenance IRQ

Julien Thierry
2019-07-04T07:38:28Z

On 21/06/2019 10:38, Marc Zyngier wrote:
> From: Andre Przywara <andre.przywara@arm.com>
>
> The VGIC maintenance IRQ signals various conditions about the LRs, when
> the GIC's virtualization extension is used.
> So far we didn't need it, but nested virtualization needs to know about
> this interrupt, so add a userland interface to setup the IRQ number.
> The architecture mandates that it must be a PPI, on top of that this code
> only exports a per-device option, so the PPI is the same on all VCPUs.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> [added some bits of documentation]
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> .../virtual/kvm/devices/arm-vgic-v3.txt | 9 ++++++++
> arch/arm/include/uapi/asm/kvm.h | 1 +
> arch/arm64/include/uapi/asm/kvm.h | 1 +
> include/kvm/arm_vgic.h | 3 +++
> virt/kvm/arm/vgic/vgic-kvm-device.c | 22 +++++++++++++++++++
> 5 files changed, 36 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> index ff290b43c8e5..c70e8f2e0c9c 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> @@ -249,3 +249,12 @@ Groups:
> Errors:
> -EINVAL: vINTID is not multiple of 32 or
> info field is not VGIC_LEVEL_INFO_LINE_LEVEL
> +
> + KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ
> + The attr field of kvm_device_attr encodes the following values:
> + bits: | 31 .... 5 | 4 .... 0 |
> + values: | RES0 | vINTID |
> +
> + The vINTID specifies which interrupt is generated when the vGIC
> + must generate a maintenance interrupt. This must be a PPI.
> +

Something seems off. The documentation suggests that the value of the
attribute will be between 0-15 (and other values will be masked down to
a value between 0 and 15). However, in the code the value should be
between 16 and 32 (PPI INTID) and other values are rejected.

Am I reading this wrong?

Cheers,

Julien

> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> index 4602464ebdfb..a6af45645a6d 100644
> --- a/arch/arm/include/uapi/asm/kvm.h
> +++ b/arch/arm/include/uapi/asm/kvm.h
> @@ -233,6 +233,7 @@ struct kvm_vcpu_events {
> #define KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS 6
> #define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7
> #define KVM_DEV_ARM_VGIC_GRP_ITS_REGS 8
> +#define KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ 9
> #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT 10
> #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK \
> (0x3fffffULL << KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT)
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index 563e2a8bae93..8e1c6ffe1b59 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -295,6 +295,7 @@ struct kvm_vcpu_events {
> #define KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS 6
> #define KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO 7
> #define KVM_DEV_ARM_VGIC_GRP_ITS_REGS 8
> +#define KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ 9
> #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT 10
> #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK \
> (0x3fffffULL << KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT)
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 17d32c2797c0..95c6232663c3 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -229,6 +229,9 @@ struct vgic_dist {
>
> int nr_spis;
>
> + /* The GIC maintenance IRQ for nested hypervisors. */
> + u32 maint_irq;
> +
> /* base addresses in guest physical address space: */
> gpa_t vgic_dist_base; /* distributor */
> union {
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index 44419679f91a..dfb1d7cc66b3 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -241,6 +241,12 @@ static int vgic_get_common_attr(struct kvm_device *dev,
> VGIC_NR_PRIVATE_IRQS, uaddr);
> break;
> }
> + case KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ: {
> + u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> +
> + r = put_user(dev->kvm->arch.vgic.maint_irq, uaddr);
> + break;
> + }
> }
>
> return r;
> @@ -627,6 +633,21 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
> reg = tmp32;
> return vgic_v3_attr_regs_access(dev, attr, &reg, true);
> }
> + case KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ: {
> + u32 __user *uaddr = (u32 __user *)(long)attr->addr;
> + u32 val;
> +
> + if (get_user(val, uaddr))
> + return -EFAULT;
> +
> + /* Must be a PPI. */
> + if ((val >= VGIC_NR_PRIVATE_IRQS) || (val < VGIC_NR_SGIS))
> + return -EINVAL;
> +
> + dev->kvm->arch.vgic.maint_irq = val;
> +
> + return 0;
> + }
> case KVM_DEV_ARM_VGIC_GRP_CTRL: {
> int ret;
>
> @@ -712,6 +733,7 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
> case KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS:
> return vgic_v3_has_attr_regs(dev, attr);
> case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
> + case KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ:
> return 0;
> case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
> if (((attr->attr & KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK) >>
>

--
Julien Thierry