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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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, &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) >>
> >   
> 




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux