On Thu, 4 Jul 2019 08:38:20 +0100
Julien Thierry <julien.thierry@xxxxxxx> wrote:
> On 21/06/2019 10:38, Marc Zyngier wrote:
> > From: Andre Przywara <andre.przywara@xxxxxxx>
> >
> > 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@xxxxxxx>
> > [added some bits of documentation]
> > Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> > ---
> > .../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).
Where does that happen? The mask is [4:0], so 5 bits, that should be enough for PPIs as well.
We could add a line to the documentation to stress that this is an interrupt ID as seen by the virtual GIC, if that helps.
Cheers,
Andre.
> 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, ®, 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) >>
> >
>
