[PATCH 0/5] Userspace timer IRQ number control and PMU with userspace-gic

[PATCH 0/5] Userspace timer IRQ number control and PMU with userspace-gic

[Christoffer Dall]

So far we have been getting away with letting the kernel choosing
interrupt numbers for the timers in the kernel and we have crossed our
fingers in hoping that the DT/ACPI provided by userspace matches with
the interrupt number we use in the kernel for a given VCPU type.

But as we are generally moving towards letting userspace be in more fine
grained control of what is being emulated, let userspace decide the irq
numbers for the timers.

Slightly related, and therefore included in this series, we have
recently added support for a userspace GIC while still supporting the
generic timers and the PMU from inside the VM.  Unfortunately we forgot
to rework the code to actually let userspace create the PMU device
without creating an in-kernel GIC.

Tested on APM X-Gene, AMD Seattle, and TC2.

QEMU patches used for testing can be found here:

https://git.linaro.org/people/christoffer.dall/qemu-arm.git timer-pmu-irqs


Thanks,
-Christoffer

Christoffer Dall (5):
  KVM: arm64: Allow creating the PMU without the in-kernel GIC
  KVM: arm: Handle VCPU device attributes in guest.c
  KVM: arm/arm64: Move irq_is_ppi() to header file
  KVM: arm/arm64: Move timer IRQ default init to arch_timer.c
  KVM: arm/arm64: Allow setting the timer IRQ numbers from userspace

 Documentation/virtual/kvm/devices/vcpu.txt |  41 +++++++--
 arch/arm/include/asm/kvm_host.h            |  22 ++---
 arch/arm/include/uapi/asm/kvm.h            |   8 ++
 arch/arm/kvm/guest.c                       |  51 +++++++++++
 arch/arm/kvm/reset.c                       |  16 +---
 arch/arm64/include/uapi/asm/kvm.h          |   3 +
 arch/arm64/kvm/guest.c                     |   9 ++
 arch/arm64/kvm/reset.c                     |  16 +---
 include/kvm/arm_arch_timer.h               |   8 +-
 include/kvm/arm_vgic.h                     |   2 +
 virt/kvm/arm/arch_timer.c                  | 132 ++++++++++++++++++++++++++---
 virt/kvm/arm/pmu.c                         |  29 ++++---
 12 files changed, 258 insertions(+), 79 deletions(-)

-- 
2.9.0

[PATCH 1/5] KVM: arm64: Allow creating the PMU without the in-kernel GIC

[Christoffer Dall]

Since we got support for devices in userspace which allows reporting the
PMU overflow output status to userspace, we should actually allow
creating the PMU on systems without an in-kernel irqchip, which in turn
requires us to slightly clarify error codes for the ABI and move things
around for the initialization phase.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 Documentation/virtual/kvm/devices/vcpu.txt | 16 +++++++++-------
 virt/kvm/arm/pmu.c                         | 27 +++++++++++++++++----------
 2 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt
index 02f5068..352af6e 100644
--- a/Documentation/virtual/kvm/devices/vcpu.txt
+++ b/Documentation/virtual/kvm/devices/vcpu.txt
@@ -16,7 +16,9 @@ Parameters: in kvm_device_attr.addr the address for PMU overflow interrupt is a
 Returns: -EBUSY: The PMU overflow interrupt is already set
          -ENXIO: The overflow interrupt not set when attempting to get it
          -ENODEV: PMUv3 not supported
-         -EINVAL: Invalid PMU overflow interrupt number supplied
+         -EINVAL: Invalid PMU overflow interrupt number supplied or
+                  trying to set the IRQ number without using an in-kernel
+                  irqchip.
 
 A value describing the PMUv3 (Performance Monitor Unit v3) overflow interrupt
 number for this vcpu. This interrupt could be a PPI or SPI, but the interrupt
@@ -25,11 +27,11 @@ all vcpus, while as an SPI it must be a separate number per vcpu.
 
 1.2 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_INIT
 Parameters: no additional parameter in kvm_device_attr.addr
-Returns: -ENODEV: PMUv3 not supported
-         -ENXIO: PMUv3 not properly configured as required prior to calling this
-                 attribute
+Returns: -ENODEV: PMUv3 not supported or GIC not initialized
+         -ENXIO: PMUv3 not properly configured or in-kernel irqchip not
+                 conigured as required prior to calling this attribute
          -EBUSY: PMUv3 already initialized
 
-Request the initialization of the PMUv3.  This must be done after creating the
-in-kernel irqchip.  Creating a PMU with a userspace irqchip is currently not
-supported.
+Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
+virtual GIC implementation, this must be done after initializing the in-kernel
+irqchip.
diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
index 4b43e7f..f046b08 100644
--- a/virt/kvm/arm/pmu.c
+++ b/virt/kvm/arm/pmu.c
@@ -456,21 +456,25 @@ static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
 	if (!kvm_arm_support_pmu_v3())
 		return -ENODEV;
 
-	/*
-	 * We currently require an in-kernel VGIC to use the PMU emulation,
-	 * because we do not support forwarding PMU overflow interrupts to
-	 * userspace yet.
-	 */
-	if (!irqchip_in_kernel(vcpu->kvm) || !vgic_initialized(vcpu->kvm))
-		return -ENODEV;
-
-	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features) ||
-	    !kvm_arm_pmu_irq_initialized(vcpu))
+	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
 		return -ENXIO;
 
 	if (kvm_arm_pmu_v3_ready(vcpu))
 		return -EBUSY;
 
+	if (irqchip_in_kernel(vcpu->kvm)) {
+		/*
+		 * If using the PMU with an in-kernel virtual GIC
+		 * implementation, we require the GIC to be already
+		 * initialized when initializing the PMU.
+		 */
+		if (!vgic_initialized(vcpu->kvm))
+			return -ENODEV;
+
+		if (!kvm_arm_pmu_irq_initialized(vcpu))
+			return -ENXIO;
+	}
+
 	kvm_pmu_vcpu_reset(vcpu);
 	vcpu->arch.pmu.ready = true;
 
@@ -512,6 +516,9 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 		int __user *uaddr = (int __user *)(long)attr->addr;
 		int irq;
 
+		if (!irqchip_in_kernel(vcpu->kvm))
+			return -EINVAL;
+
 		if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
 			return -ENODEV;
 
-- 
2.9.0

[PATCH 1/5] KVM: arm64: Allow creating the PMU without the in-kernel GIC

[Marc Zyngier]

On 03/05/17 19:32, Christoffer Dall wrote:
> Since we got support for devices in userspace which allows reporting the
> PMU overflow output status to userspace, we should actually allow
> creating the PMU on systems without an in-kernel irqchip, which in turn
> requires us to slightly clarify error codes for the ABI and move things
> around for the initialization phase.
> 
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> ---
>  Documentation/virtual/kvm/devices/vcpu.txt | 16 +++++++++-------
>  virt/kvm/arm/pmu.c                         | 27 +++++++++++++++++----------
>  2 files changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt
> index 02f5068..352af6e 100644
> --- a/Documentation/virtual/kvm/devices/vcpu.txt
> +++ b/Documentation/virtual/kvm/devices/vcpu.txt
> @@ -16,7 +16,9 @@ Parameters: in kvm_device_attr.addr the address for PMU overflow interrupt is a
>  Returns: -EBUSY: The PMU overflow interrupt is already set
>           -ENXIO: The overflow interrupt not set when attempting to get it
>           -ENODEV: PMUv3 not supported
> -         -EINVAL: Invalid PMU overflow interrupt number supplied
> +         -EINVAL: Invalid PMU overflow interrupt number supplied or
> +                  trying to set the IRQ number without using an in-kernel
> +                  irqchip.
>  
>  A value describing the PMUv3 (Performance Monitor Unit v3) overflow interrupt
>  number for this vcpu. This interrupt could be a PPI or SPI, but the interrupt
> @@ -25,11 +27,11 @@ all vcpus, while as an SPI it must be a separate number per vcpu.
>  
>  1.2 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_INIT
>  Parameters: no additional parameter in kvm_device_attr.addr
> -Returns: -ENODEV: PMUv3 not supported
> -         -ENXIO: PMUv3 not properly configured as required prior to calling this
> -                 attribute
> +Returns: -ENODEV: PMUv3 not supported or GIC not initialized
> +         -ENXIO: PMUv3 not properly configured or in-kernel irqchip not
> +                 conigured as required prior to calling this attribute
>           -EBUSY: PMUv3 already initialized
>  
> -Request the initialization of the PMUv3.  This must be done after creating the
> -in-kernel irqchip.  Creating a PMU with a userspace irqchip is currently not
> -supported.
> +Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
> +virtual GIC implementation, this must be done after initializing the in-kernel
> +irqchip.
> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
> index 4b43e7f..f046b08 100644
> --- a/virt/kvm/arm/pmu.c
> +++ b/virt/kvm/arm/pmu.c
> @@ -456,21 +456,25 @@ static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>  	if (!kvm_arm_support_pmu_v3())
>  		return -ENODEV;
>  
> -	/*
> -	 * We currently require an in-kernel VGIC to use the PMU emulation,
> -	 * because we do not support forwarding PMU overflow interrupts to
> -	 * userspace yet.
> -	 */
> -	if (!irqchip_in_kernel(vcpu->kvm) || !vgic_initialized(vcpu->kvm))
> -		return -ENODEV;
> -
> -	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features) ||
> -	    !kvm_arm_pmu_irq_initialized(vcpu))
> +	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>  		return -ENXIO;
>  
>  	if (kvm_arm_pmu_v3_ready(vcpu))
>  		return -EBUSY;
>  
> +	if (irqchip_in_kernel(vcpu->kvm)) {
> +		/*
> +		 * If using the PMU with an in-kernel virtual GIC
> +		 * implementation, we require the GIC to be already
> +		 * initialized when initializing the PMU.
> +		 */
> +		if (!vgic_initialized(vcpu->kvm))
> +			return -ENODEV;
> +
> +		if (!kvm_arm_pmu_irq_initialized(vcpu))
> +			return -ENXIO;
> +	}
> +
>  	kvm_pmu_vcpu_reset(vcpu);
>  	vcpu->arch.pmu.ready = true;
>  
> @@ -512,6 +516,9 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  		int __user *uaddr = (int __user *)(long)attr->addr;
>  		int irq;
>  
> +		if (!irqchip_in_kernel(vcpu->kvm))
> +			return -EINVAL;
> +

Shouldn't we fail the same way for {get,has}_attr? get_attr is going to
generate a -ENXIO, and has_attr is going to lie about the availability
of KVM_ARM_VCPU_PMU_V3_IRQ...

>  		if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>  			return -ENODEV;
>  
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 1/5] KVM: arm64: Allow creating the PMU without the in-kernel GIC

[Christoffer Dall]

On Thu, May 04, 2017 at 09:09:47AM +0100, Marc Zyngier wrote:
> On 03/05/17 19:32, Christoffer Dall wrote:
> > Since we got support for devices in userspace which allows reporting the
> > PMU overflow output status to userspace, we should actually allow
> > creating the PMU on systems without an in-kernel irqchip, which in turn
> > requires us to slightly clarify error codes for the ABI and move things
> > around for the initialization phase.
> > 
> > Signed-off-by: Christoffer Dall <cdall@linaro.org>
> > ---
> >  Documentation/virtual/kvm/devices/vcpu.txt | 16 +++++++++-------
> >  virt/kvm/arm/pmu.c                         | 27 +++++++++++++++++----------
> >  2 files changed, 26 insertions(+), 17 deletions(-)
> > 
> > diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt
> > index 02f5068..352af6e 100644
> > --- a/Documentation/virtual/kvm/devices/vcpu.txt
> > +++ b/Documentation/virtual/kvm/devices/vcpu.txt
> > @@ -16,7 +16,9 @@ Parameters: in kvm_device_attr.addr the address for PMU overflow interrupt is a
> >  Returns: -EBUSY: The PMU overflow interrupt is already set
> >           -ENXIO: The overflow interrupt not set when attempting to get it
> >           -ENODEV: PMUv3 not supported
> > -         -EINVAL: Invalid PMU overflow interrupt number supplied
> > +         -EINVAL: Invalid PMU overflow interrupt number supplied or
> > +                  trying to set the IRQ number without using an in-kernel
> > +                  irqchip.
> >  
> >  A value describing the PMUv3 (Performance Monitor Unit v3) overflow interrupt
> >  number for this vcpu. This interrupt could be a PPI or SPI, but the interrupt
> > @@ -25,11 +27,11 @@ all vcpus, while as an SPI it must be a separate number per vcpu.
> >  
> >  1.2 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_INIT
> >  Parameters: no additional parameter in kvm_device_attr.addr
> > -Returns: -ENODEV: PMUv3 not supported
> > -         -ENXIO: PMUv3 not properly configured as required prior to calling this
> > -                 attribute
> > +Returns: -ENODEV: PMUv3 not supported or GIC not initialized
> > +         -ENXIO: PMUv3 not properly configured or in-kernel irqchip not
> > +                 conigured as required prior to calling this attribute
> >           -EBUSY: PMUv3 already initialized
> >  
> > -Request the initialization of the PMUv3.  This must be done after creating the
> > -in-kernel irqchip.  Creating a PMU with a userspace irqchip is currently not
> > -supported.
> > +Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
> > +virtual GIC implementation, this must be done after initializing the in-kernel
> > +irqchip.
> > diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
> > index 4b43e7f..f046b08 100644
> > --- a/virt/kvm/arm/pmu.c
> > +++ b/virt/kvm/arm/pmu.c
> > @@ -456,21 +456,25 @@ static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
> >  	if (!kvm_arm_support_pmu_v3())
> >  		return -ENODEV;
> >  
> > -	/*
> > -	 * We currently require an in-kernel VGIC to use the PMU emulation,
> > -	 * because we do not support forwarding PMU overflow interrupts to
> > -	 * userspace yet.
> > -	 */
> > -	if (!irqchip_in_kernel(vcpu->kvm) || !vgic_initialized(vcpu->kvm))
> > -		return -ENODEV;
> > -
> > -	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features) ||
> > -	    !kvm_arm_pmu_irq_initialized(vcpu))
> > +	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
> >  		return -ENXIO;
> >  
> >  	if (kvm_arm_pmu_v3_ready(vcpu))
> >  		return -EBUSY;
> >  
> > +	if (irqchip_in_kernel(vcpu->kvm)) {
> > +		/*
> > +		 * If using the PMU with an in-kernel virtual GIC
> > +		 * implementation, we require the GIC to be already
> > +		 * initialized when initializing the PMU.
> > +		 */
> > +		if (!vgic_initialized(vcpu->kvm))
> > +			return -ENODEV;
> > +
> > +		if (!kvm_arm_pmu_irq_initialized(vcpu))
> > +			return -ENXIO;
> > +	}
> > +
> >  	kvm_pmu_vcpu_reset(vcpu);
> >  	vcpu->arch.pmu.ready = true;
> >  
> > @@ -512,6 +516,9 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
> >  		int __user *uaddr = (int __user *)(long)attr->addr;
> >  		int irq;
> >  
> > +		if (!irqchip_in_kernel(vcpu->kvm))
> > +			return -EINVAL;
> > +
> 
> Shouldn't we fail the same way for {get,has}_attr? get_attr is going to
> generate a -ENXIO, and has_attr is going to lie about the availability
> of KVM_ARM_VCPU_PMU_V3_IRQ...
> 

Here's the text from api.txt:

  Tests whether a device supports a particular attribute.  A successful
  return indicates the attribute is implemented.  It does not necessarily
  indicate that the attribute can be read or written in the device's
  current state.  "addr" is ignored.

My interpretation therefore is that QEMU can use this ioctl to figure
out if the feature is supported (sort of like a capability), but that
doesn't mean that the configuration of the VM is such that the attribute
can be get or set at that moment.

For example, there will also alway be situations where you can get an
attr, but not set an attr, what should the has_attr return then?

Thanks,
-Christoffer

[PATCH 1/5] KVM: arm64: Allow creating the PMU without the in-kernel GIC

[Marc Zyngier]

On 04/05/17 09:13, Christoffer Dall wrote:
> On Thu, May 04, 2017 at 09:09:47AM +0100, Marc Zyngier wrote:
>> On 03/05/17 19:32, Christoffer Dall wrote:
>>> Since we got support for devices in userspace which allows reporting the
>>> PMU overflow output status to userspace, we should actually allow
>>> creating the PMU on systems without an in-kernel irqchip, which in turn
>>> requires us to slightly clarify error codes for the ABI and move things
>>> around for the initialization phase.
>>>
>>> Signed-off-by: Christoffer Dall <cdall@linaro.org>
>>> ---
>>>  Documentation/virtual/kvm/devices/vcpu.txt | 16 +++++++++-------
>>>  virt/kvm/arm/pmu.c                         | 27 +++++++++++++++++----------
>>>  2 files changed, 26 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt
>>> index 02f5068..352af6e 100644
>>> --- a/Documentation/virtual/kvm/devices/vcpu.txt
>>> +++ b/Documentation/virtual/kvm/devices/vcpu.txt
>>> @@ -16,7 +16,9 @@ Parameters: in kvm_device_attr.addr the address for PMU overflow interrupt is a
>>>  Returns: -EBUSY: The PMU overflow interrupt is already set
>>>           -ENXIO: The overflow interrupt not set when attempting to get it
>>>           -ENODEV: PMUv3 not supported
>>> -         -EINVAL: Invalid PMU overflow interrupt number supplied
>>> +         -EINVAL: Invalid PMU overflow interrupt number supplied or
>>> +                  trying to set the IRQ number without using an in-kernel
>>> +                  irqchip.
>>>  
>>>  A value describing the PMUv3 (Performance Monitor Unit v3) overflow interrupt
>>>  number for this vcpu. This interrupt could be a PPI or SPI, but the interrupt
>>> @@ -25,11 +27,11 @@ all vcpus, while as an SPI it must be a separate number per vcpu.
>>>  
>>>  1.2 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_INIT
>>>  Parameters: no additional parameter in kvm_device_attr.addr
>>> -Returns: -ENODEV: PMUv3 not supported
>>> -         -ENXIO: PMUv3 not properly configured as required prior to calling this
>>> -                 attribute
>>> +Returns: -ENODEV: PMUv3 not supported or GIC not initialized
>>> +         -ENXIO: PMUv3 not properly configured or in-kernel irqchip not
>>> +                 conigured as required prior to calling this attribute
>>>           -EBUSY: PMUv3 already initialized
>>>  
>>> -Request the initialization of the PMUv3.  This must be done after creating the
>>> -in-kernel irqchip.  Creating a PMU with a userspace irqchip is currently not
>>> -supported.
>>> +Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
>>> +virtual GIC implementation, this must be done after initializing the in-kernel
>>> +irqchip.
>>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>>> index 4b43e7f..f046b08 100644
>>> --- a/virt/kvm/arm/pmu.c
>>> +++ b/virt/kvm/arm/pmu.c
>>> @@ -456,21 +456,25 @@ static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>>>  	if (!kvm_arm_support_pmu_v3())
>>>  		return -ENODEV;
>>>  
>>> -	/*
>>> -	 * We currently require an in-kernel VGIC to use the PMU emulation,
>>> -	 * because we do not support forwarding PMU overflow interrupts to
>>> -	 * userspace yet.
>>> -	 */
>>> -	if (!irqchip_in_kernel(vcpu->kvm) || !vgic_initialized(vcpu->kvm))
>>> -		return -ENODEV;
>>> -
>>> -	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features) ||
>>> -	    !kvm_arm_pmu_irq_initialized(vcpu))
>>> +	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>  		return -ENXIO;
>>>  
>>>  	if (kvm_arm_pmu_v3_ready(vcpu))
>>>  		return -EBUSY;
>>>  
>>> +	if (irqchip_in_kernel(vcpu->kvm)) {
>>> +		/*
>>> +		 * If using the PMU with an in-kernel virtual GIC
>>> +		 * implementation, we require the GIC to be already
>>> +		 * initialized when initializing the PMU.
>>> +		 */
>>> +		if (!vgic_initialized(vcpu->kvm))
>>> +			return -ENODEV;
>>> +
>>> +		if (!kvm_arm_pmu_irq_initialized(vcpu))
>>> +			return -ENXIO;
>>> +	}
>>> +
>>>  	kvm_pmu_vcpu_reset(vcpu);
>>>  	vcpu->arch.pmu.ready = true;
>>>  
>>> @@ -512,6 +516,9 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>>>  		int __user *uaddr = (int __user *)(long)attr->addr;
>>>  		int irq;
>>>  
>>> +		if (!irqchip_in_kernel(vcpu->kvm))
>>> +			return -EINVAL;
>>> +
>>
>> Shouldn't we fail the same way for {get,has}_attr? get_attr is going to
>> generate a -ENXIO, and has_attr is going to lie about the availability
>> of KVM_ARM_VCPU_PMU_V3_IRQ...
>>
> 
> Here's the text from api.txt:
> 
>   Tests whether a device supports a particular attribute.  A successful
>   return indicates the attribute is implemented.  It does not necessarily
>   indicate that the attribute can be read or written in the device's
>   current state.  "addr" is ignored.
> 
> My interpretation therefore is that QEMU can use this ioctl to figure
> out if the feature is supported (sort of like a capability), but that
> doesn't mean that the configuration of the VM is such that the attribute
> can be get or set at that moment.
> 
> For example, there will also alway be situations where you can get an
> attr, but not set an attr, what should the has_attr return then?

My issue here is that whether we can get/set the interrupt or not is not
a function of the device itself, but of the way it is "wired". No matter
what "the device's current state" is, we'll never be able to get/set the
interrupt.

I'd tend to err on the side of caution and return something that is
unambiguous, be maybe I have too strict an interpretation of the API.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 1/5] KVM: arm64: Allow creating the PMU without the in-kernel GIC

[Christoffer Dall]

On Thu, May 04, 2017 at 09:28:50AM +0100, Marc Zyngier wrote:
> On 04/05/17 09:13, Christoffer Dall wrote:
> > On Thu, May 04, 2017 at 09:09:47AM +0100, Marc Zyngier wrote:
> >> On 03/05/17 19:32, Christoffer Dall wrote:
> >>> Since we got support for devices in userspace which allows reporting the
> >>> PMU overflow output status to userspace, we should actually allow
> >>> creating the PMU on systems without an in-kernel irqchip, which in turn
> >>> requires us to slightly clarify error codes for the ABI and move things
> >>> around for the initialization phase.
> >>>
> >>> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> >>> ---
> >>>  Documentation/virtual/kvm/devices/vcpu.txt | 16 +++++++++-------
> >>>  virt/kvm/arm/pmu.c                         | 27 +++++++++++++++++----------
> >>>  2 files changed, 26 insertions(+), 17 deletions(-)
> >>>
> >>> diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt
> >>> index 02f5068..352af6e 100644
> >>> --- a/Documentation/virtual/kvm/devices/vcpu.txt
> >>> +++ b/Documentation/virtual/kvm/devices/vcpu.txt
> >>> @@ -16,7 +16,9 @@ Parameters: in kvm_device_attr.addr the address for PMU overflow interrupt is a
> >>>  Returns: -EBUSY: The PMU overflow interrupt is already set
> >>>           -ENXIO: The overflow interrupt not set when attempting to get it
> >>>           -ENODEV: PMUv3 not supported
> >>> -         -EINVAL: Invalid PMU overflow interrupt number supplied
> >>> +         -EINVAL: Invalid PMU overflow interrupt number supplied or
> >>> +                  trying to set the IRQ number without using an in-kernel
> >>> +                  irqchip.
> >>>  
> >>>  A value describing the PMUv3 (Performance Monitor Unit v3) overflow interrupt
> >>>  number for this vcpu. This interrupt could be a PPI or SPI, but the interrupt
> >>> @@ -25,11 +27,11 @@ all vcpus, while as an SPI it must be a separate number per vcpu.
> >>>  
> >>>  1.2 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_INIT
> >>>  Parameters: no additional parameter in kvm_device_attr.addr
> >>> -Returns: -ENODEV: PMUv3 not supported
> >>> -         -ENXIO: PMUv3 not properly configured as required prior to calling this
> >>> -                 attribute
> >>> +Returns: -ENODEV: PMUv3 not supported or GIC not initialized
> >>> +         -ENXIO: PMUv3 not properly configured or in-kernel irqchip not
> >>> +                 conigured as required prior to calling this attribute
> >>>           -EBUSY: PMUv3 already initialized
> >>>  
> >>> -Request the initialization of the PMUv3.  This must be done after creating the
> >>> -in-kernel irqchip.  Creating a PMU with a userspace irqchip is currently not
> >>> -supported.
> >>> +Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
> >>> +virtual GIC implementation, this must be done after initializing the in-kernel
> >>> +irqchip.
> >>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
> >>> index 4b43e7f..f046b08 100644
> >>> --- a/virt/kvm/arm/pmu.c
> >>> +++ b/virt/kvm/arm/pmu.c
> >>> @@ -456,21 +456,25 @@ static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
> >>>  	if (!kvm_arm_support_pmu_v3())
> >>>  		return -ENODEV;
> >>>  
> >>> -	/*
> >>> -	 * We currently require an in-kernel VGIC to use the PMU emulation,
> >>> -	 * because we do not support forwarding PMU overflow interrupts to
> >>> -	 * userspace yet.
> >>> -	 */
> >>> -	if (!irqchip_in_kernel(vcpu->kvm) || !vgic_initialized(vcpu->kvm))
> >>> -		return -ENODEV;
> >>> -
> >>> -	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features) ||
> >>> -	    !kvm_arm_pmu_irq_initialized(vcpu))
> >>> +	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
> >>>  		return -ENXIO;
> >>>  
> >>>  	if (kvm_arm_pmu_v3_ready(vcpu))
> >>>  		return -EBUSY;
> >>>  
> >>> +	if (irqchip_in_kernel(vcpu->kvm)) {
> >>> +		/*
> >>> +		 * If using the PMU with an in-kernel virtual GIC
> >>> +		 * implementation, we require the GIC to be already
> >>> +		 * initialized when initializing the PMU.
> >>> +		 */
> >>> +		if (!vgic_initialized(vcpu->kvm))
> >>> +			return -ENODEV;
> >>> +
> >>> +		if (!kvm_arm_pmu_irq_initialized(vcpu))
> >>> +			return -ENXIO;
> >>> +	}
> >>> +
> >>>  	kvm_pmu_vcpu_reset(vcpu);
> >>>  	vcpu->arch.pmu.ready = true;
> >>>  
> >>> @@ -512,6 +516,9 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
> >>>  		int __user *uaddr = (int __user *)(long)attr->addr;
> >>>  		int irq;
> >>>  
> >>> +		if (!irqchip_in_kernel(vcpu->kvm))
> >>> +			return -EINVAL;
> >>> +
> >>
> >> Shouldn't we fail the same way for {get,has}_attr? get_attr is going to
> >> generate a -ENXIO, and has_attr is going to lie about the availability
> >> of KVM_ARM_VCPU_PMU_V3_IRQ...
> >>
> > 
> > Here's the text from api.txt:
> > 
> >   Tests whether a device supports a particular attribute.  A successful
> >   return indicates the attribute is implemented.  It does not necessarily
> >   indicate that the attribute can be read or written in the device's
> >   current state.  "addr" is ignored.
> > 
> > My interpretation therefore is that QEMU can use this ioctl to figure
> > out if the feature is supported (sort of like a capability), but that
> > doesn't mean that the configuration of the VM is such that the attribute
> > can be get or set at that moment.
> > 
> > For example, there will also alway be situations where you can get an
> > attr, but not set an attr, what should the has_attr return then?
> 
> My issue here is that whether we can get/set the interrupt or not is not
> a function of the device itself, but of the way it is "wired". No matter
> what "the device's current state" is, we'll never be able to get/set the
> interrupt.
> 
> I'd tend to err on the side of caution and return something that is
> unambiguous, be maybe I have too strict an interpretation of the API.
> 

Hmm, I see the has_attr as a method for userspace to discover "does this
kernel have this feature".  If we make has_attr return a value depending
on the VM having an in-kernel gic or not, we implicitly require
userspace to create a VM with an in-kernel GIC to discover if this
kernel has that feature, and therefore also impose an ordering
requirement of figuring out the capabilities of the kernel (i.e. create
the GIC before checking this API).

I think QEMU should be able to do:

  if (has_attr()) {
     kvm_supports_set_timer_irq = true;
     vtimer_irq = foo;
  } else {
     kvm_supports_set_timer_irq = false;
     vtimer_irq = 27; /* default, we're stuck with it */
  }

  create_board_definition();
  create_dt();
  create_acpi();

  /* do whatever */

  if (kvm_supports_set_timer_irq && kvm_irqchip_in_kernel()) {
  	kvm_arm_timer_set_irq(...);
  }

And all this should not be coupled to when we create the irqchip device.

But I may be failing to see the case where the current implementation
creates a problem for userspace, in which case we should document the
ordering requirement.

Note: that I don't think it's expected that has_attr implies get_attr
and set_attr succeed.  For example, has_attr is currently typically
called with a null pointer to the addr field.

Thanks,
-Christoffer

[PATCH 1/5] KVM: arm64: Allow creating the PMU without the in-kernel GIC

[Marc Zyngier]

On 04/05/17 09:38, Christoffer Dall wrote:
> On Thu, May 04, 2017 at 09:28:50AM +0100, Marc Zyngier wrote:
>> On 04/05/17 09:13, Christoffer Dall wrote:
>>> On Thu, May 04, 2017 at 09:09:47AM +0100, Marc Zyngier wrote:
>>>> On 03/05/17 19:32, Christoffer Dall wrote:
>>>>> Since we got support for devices in userspace which allows reporting the
>>>>> PMU overflow output status to userspace, we should actually allow
>>>>> creating the PMU on systems without an in-kernel irqchip, which in turn
>>>>> requires us to slightly clarify error codes for the ABI and move things
>>>>> around for the initialization phase.
>>>>>
>>>>> Signed-off-by: Christoffer Dall <cdall@linaro.org>
>>>>> ---
>>>>>  Documentation/virtual/kvm/devices/vcpu.txt | 16 +++++++++-------
>>>>>  virt/kvm/arm/pmu.c                         | 27 +++++++++++++++++----------
>>>>>  2 files changed, 26 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt
>>>>> index 02f5068..352af6e 100644
>>>>> --- a/Documentation/virtual/kvm/devices/vcpu.txt
>>>>> +++ b/Documentation/virtual/kvm/devices/vcpu.txt
>>>>> @@ -16,7 +16,9 @@ Parameters: in kvm_device_attr.addr the address for PMU overflow interrupt is a
>>>>>  Returns: -EBUSY: The PMU overflow interrupt is already set
>>>>>           -ENXIO: The overflow interrupt not set when attempting to get it
>>>>>           -ENODEV: PMUv3 not supported
>>>>> -         -EINVAL: Invalid PMU overflow interrupt number supplied
>>>>> +         -EINVAL: Invalid PMU overflow interrupt number supplied or
>>>>> +                  trying to set the IRQ number without using an in-kernel
>>>>> +                  irqchip.
>>>>>  
>>>>>  A value describing the PMUv3 (Performance Monitor Unit v3) overflow interrupt
>>>>>  number for this vcpu. This interrupt could be a PPI or SPI, but the interrupt
>>>>> @@ -25,11 +27,11 @@ all vcpus, while as an SPI it must be a separate number per vcpu.
>>>>>  
>>>>>  1.2 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_INIT
>>>>>  Parameters: no additional parameter in kvm_device_attr.addr
>>>>> -Returns: -ENODEV: PMUv3 not supported
>>>>> -         -ENXIO: PMUv3 not properly configured as required prior to calling this
>>>>> -                 attribute
>>>>> +Returns: -ENODEV: PMUv3 not supported or GIC not initialized
>>>>> +         -ENXIO: PMUv3 not properly configured or in-kernel irqchip not
>>>>> +                 conigured as required prior to calling this attribute
>>>>>           -EBUSY: PMUv3 already initialized
>>>>>  
>>>>> -Request the initialization of the PMUv3.  This must be done after creating the
>>>>> -in-kernel irqchip.  Creating a PMU with a userspace irqchip is currently not
>>>>> -supported.
>>>>> +Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
>>>>> +virtual GIC implementation, this must be done after initializing the in-kernel
>>>>> +irqchip.
>>>>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>>>>> index 4b43e7f..f046b08 100644
>>>>> --- a/virt/kvm/arm/pmu.c
>>>>> +++ b/virt/kvm/arm/pmu.c
>>>>> @@ -456,21 +456,25 @@ static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>>>>>  	if (!kvm_arm_support_pmu_v3())
>>>>>  		return -ENODEV;
>>>>>  
>>>>> -	/*
>>>>> -	 * We currently require an in-kernel VGIC to use the PMU emulation,
>>>>> -	 * because we do not support forwarding PMU overflow interrupts to
>>>>> -	 * userspace yet.
>>>>> -	 */
>>>>> -	if (!irqchip_in_kernel(vcpu->kvm) || !vgic_initialized(vcpu->kvm))
>>>>> -		return -ENODEV;
>>>>> -
>>>>> -	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features) ||
>>>>> -	    !kvm_arm_pmu_irq_initialized(vcpu))
>>>>> +	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>>>  		return -ENXIO;
>>>>>  
>>>>>  	if (kvm_arm_pmu_v3_ready(vcpu))
>>>>>  		return -EBUSY;
>>>>>  
>>>>> +	if (irqchip_in_kernel(vcpu->kvm)) {
>>>>> +		/*
>>>>> +		 * If using the PMU with an in-kernel virtual GIC
>>>>> +		 * implementation, we require the GIC to be already
>>>>> +		 * initialized when initializing the PMU.
>>>>> +		 */
>>>>> +		if (!vgic_initialized(vcpu->kvm))
>>>>> +			return -ENODEV;
>>>>> +
>>>>> +		if (!kvm_arm_pmu_irq_initialized(vcpu))
>>>>> +			return -ENXIO;
>>>>> +	}
>>>>> +
>>>>>  	kvm_pmu_vcpu_reset(vcpu);
>>>>>  	vcpu->arch.pmu.ready = true;
>>>>>  
>>>>> @@ -512,6 +516,9 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>>>>>  		int __user *uaddr = (int __user *)(long)attr->addr;
>>>>>  		int irq;
>>>>>  
>>>>> +		if (!irqchip_in_kernel(vcpu->kvm))
>>>>> +			return -EINVAL;
>>>>> +
>>>>
>>>> Shouldn't we fail the same way for {get,has}_attr? get_attr is going to
>>>> generate a -ENXIO, and has_attr is going to lie about the availability
>>>> of KVM_ARM_VCPU_PMU_V3_IRQ...
>>>>
>>>
>>> Here's the text from api.txt:
>>>
>>>   Tests whether a device supports a particular attribute.  A successful
>>>   return indicates the attribute is implemented.  It does not necessarily
>>>   indicate that the attribute can be read or written in the device's
>>>   current state.  "addr" is ignored.
>>>
>>> My interpretation therefore is that QEMU can use this ioctl to figure
>>> out if the feature is supported (sort of like a capability), but that
>>> doesn't mean that the configuration of the VM is such that the attribute
>>> can be get or set at that moment.
>>>
>>> For example, there will also alway be situations where you can get an
>>> attr, but not set an attr, what should the has_attr return then?
>>
>> My issue here is that whether we can get/set the interrupt or not is not
>> a function of the device itself, but of the way it is "wired". No matter
>> what "the device's current state" is, we'll never be able to get/set the
>> interrupt.
>>
>> I'd tend to err on the side of caution and return something that is
>> unambiguous, be maybe I have too strict an interpretation of the API.
>>
> 
> Hmm, I see the has_attr as a method for userspace to discover "does this
> kernel have this feature".  If we make has_attr return a value depending
> on the VM having an in-kernel gic or not, we implicitly require
> userspace to create a VM with an in-kernel GIC to discover if this
> kernel has that feature, and therefore also impose an ordering
> requirement of figuring out the capabilities of the kernel (i.e. create
> the GIC before checking this API).
> 
> I think QEMU should be able to do:
> 
>   if (has_attr()) {
>      kvm_supports_set_timer_irq = true;
>      vtimer_irq = foo;
>   } else {
>      kvm_supports_set_timer_irq = false;
>      vtimer_irq = 27; /* default, we're stuck with it */
>   }
> 
>   create_board_definition();
>   create_dt();
>   create_acpi();
> 
>   /* do whatever */
> 
>   if (kvm_supports_set_timer_irq && kvm_irqchip_in_kernel()) {
>   	kvm_arm_timer_set_irq(...);
>   }
> 
> And all this should not be coupled to when we create the irqchip device.
> 
> But I may be failing to see the case where the current implementation
> creates a problem for userspace, in which case we should document the
> ordering requirement.

I'm not sure it would create any problem, at least not for the PMU
(there is no working code that would have created a PMU without an irqchip).

It is just that we have a slightly diverging interpretation of what
has_attr means. You see it as "attribute that the device supports", and
I see it as "attribute that the device supports in this configuration".
I'm happy to use your semantics from now on.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 1/5] KVM: arm64: Allow creating the PMU without the in-kernel GIC

[Christoffer Dall]

On Thu, May 04, 2017 at 10:05:43AM +0100, Marc Zyngier wrote:
> On 04/05/17 09:38, Christoffer Dall wrote:
> > On Thu, May 04, 2017 at 09:28:50AM +0100, Marc Zyngier wrote:
> >> On 04/05/17 09:13, Christoffer Dall wrote:
> >>> On Thu, May 04, 2017 at 09:09:47AM +0100, Marc Zyngier wrote:
> >>>> On 03/05/17 19:32, Christoffer Dall wrote:
> >>>>> Since we got support for devices in userspace which allows reporting the
> >>>>> PMU overflow output status to userspace, we should actually allow
> >>>>> creating the PMU on systems without an in-kernel irqchip, which in turn
> >>>>> requires us to slightly clarify error codes for the ABI and move things
> >>>>> around for the initialization phase.
> >>>>>
> >>>>> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> >>>>> ---
> >>>>>  Documentation/virtual/kvm/devices/vcpu.txt | 16 +++++++++-------
> >>>>>  virt/kvm/arm/pmu.c                         | 27 +++++++++++++++++----------
> >>>>>  2 files changed, 26 insertions(+), 17 deletions(-)
> >>>>>
> >>>>> diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt
> >>>>> index 02f5068..352af6e 100644
> >>>>> --- a/Documentation/virtual/kvm/devices/vcpu.txt
> >>>>> +++ b/Documentation/virtual/kvm/devices/vcpu.txt
> >>>>> @@ -16,7 +16,9 @@ Parameters: in kvm_device_attr.addr the address for PMU overflow interrupt is a
> >>>>>  Returns: -EBUSY: The PMU overflow interrupt is already set
> >>>>>           -ENXIO: The overflow interrupt not set when attempting to get it
> >>>>>           -ENODEV: PMUv3 not supported
> >>>>> -         -EINVAL: Invalid PMU overflow interrupt number supplied
> >>>>> +         -EINVAL: Invalid PMU overflow interrupt number supplied or
> >>>>> +                  trying to set the IRQ number without using an in-kernel
> >>>>> +                  irqchip.
> >>>>>  
> >>>>>  A value describing the PMUv3 (Performance Monitor Unit v3) overflow interrupt
> >>>>>  number for this vcpu. This interrupt could be a PPI or SPI, but the interrupt
> >>>>> @@ -25,11 +27,11 @@ all vcpus, while as an SPI it must be a separate number per vcpu.
> >>>>>  
> >>>>>  1.2 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_INIT
> >>>>>  Parameters: no additional parameter in kvm_device_attr.addr
> >>>>> -Returns: -ENODEV: PMUv3 not supported
> >>>>> -         -ENXIO: PMUv3 not properly configured as required prior to calling this
> >>>>> -                 attribute
> >>>>> +Returns: -ENODEV: PMUv3 not supported or GIC not initialized
> >>>>> +         -ENXIO: PMUv3 not properly configured or in-kernel irqchip not
> >>>>> +                 conigured as required prior to calling this attribute
> >>>>>           -EBUSY: PMUv3 already initialized
> >>>>>  
> >>>>> -Request the initialization of the PMUv3.  This must be done after creating the
> >>>>> -in-kernel irqchip.  Creating a PMU with a userspace irqchip is currently not
> >>>>> -supported.
> >>>>> +Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
> >>>>> +virtual GIC implementation, this must be done after initializing the in-kernel
> >>>>> +irqchip.
> >>>>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
> >>>>> index 4b43e7f..f046b08 100644
> >>>>> --- a/virt/kvm/arm/pmu.c
> >>>>> +++ b/virt/kvm/arm/pmu.c
> >>>>> @@ -456,21 +456,25 @@ static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
> >>>>>  	if (!kvm_arm_support_pmu_v3())
> >>>>>  		return -ENODEV;
> >>>>>  
> >>>>> -	/*
> >>>>> -	 * We currently require an in-kernel VGIC to use the PMU emulation,
> >>>>> -	 * because we do not support forwarding PMU overflow interrupts to
> >>>>> -	 * userspace yet.
> >>>>> -	 */
> >>>>> -	if (!irqchip_in_kernel(vcpu->kvm) || !vgic_initialized(vcpu->kvm))
> >>>>> -		return -ENODEV;
> >>>>> -
> >>>>> -	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features) ||
> >>>>> -	    !kvm_arm_pmu_irq_initialized(vcpu))
> >>>>> +	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
> >>>>>  		return -ENXIO;
> >>>>>  
> >>>>>  	if (kvm_arm_pmu_v3_ready(vcpu))
> >>>>>  		return -EBUSY;
> >>>>>  
> >>>>> +	if (irqchip_in_kernel(vcpu->kvm)) {
> >>>>> +		/*
> >>>>> +		 * If using the PMU with an in-kernel virtual GIC
> >>>>> +		 * implementation, we require the GIC to be already
> >>>>> +		 * initialized when initializing the PMU.
> >>>>> +		 */
> >>>>> +		if (!vgic_initialized(vcpu->kvm))
> >>>>> +			return -ENODEV;
> >>>>> +
> >>>>> +		if (!kvm_arm_pmu_irq_initialized(vcpu))
> >>>>> +			return -ENXIO;
> >>>>> +	}
> >>>>> +
> >>>>>  	kvm_pmu_vcpu_reset(vcpu);
> >>>>>  	vcpu->arch.pmu.ready = true;
> >>>>>  
> >>>>> @@ -512,6 +516,9 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
> >>>>>  		int __user *uaddr = (int __user *)(long)attr->addr;
> >>>>>  		int irq;
> >>>>>  
> >>>>> +		if (!irqchip_in_kernel(vcpu->kvm))
> >>>>> +			return -EINVAL;
> >>>>> +
> >>>>
> >>>> Shouldn't we fail the same way for {get,has}_attr? get_attr is going to
> >>>> generate a -ENXIO, and has_attr is going to lie about the availability
> >>>> of KVM_ARM_VCPU_PMU_V3_IRQ...
> >>>>
> >>>
> >>> Here's the text from api.txt:
> >>>
> >>>   Tests whether a device supports a particular attribute.  A successful
> >>>   return indicates the attribute is implemented.  It does not necessarily
> >>>   indicate that the attribute can be read or written in the device's
> >>>   current state.  "addr" is ignored.
> >>>
> >>> My interpretation therefore is that QEMU can use this ioctl to figure
> >>> out if the feature is supported (sort of like a capability), but that
> >>> doesn't mean that the configuration of the VM is such that the attribute
> >>> can be get or set at that moment.
> >>>
> >>> For example, there will also alway be situations where you can get an
> >>> attr, but not set an attr, what should the has_attr return then?
> >>
> >> My issue here is that whether we can get/set the interrupt or not is not
> >> a function of the device itself, but of the way it is "wired". No matter
> >> what "the device's current state" is, we'll never be able to get/set the
> >> interrupt.
> >>
> >> I'd tend to err on the side of caution and return something that is
> >> unambiguous, be maybe I have too strict an interpretation of the API.
> >>
> > 
> > Hmm, I see the has_attr as a method for userspace to discover "does this
> > kernel have this feature".  If we make has_attr return a value depending
> > on the VM having an in-kernel gic or not, we implicitly require
> > userspace to create a VM with an in-kernel GIC to discover if this
> > kernel has that feature, and therefore also impose an ordering
> > requirement of figuring out the capabilities of the kernel (i.e. create
> > the GIC before checking this API).
> > 
> > I think QEMU should be able to do:
> > 
> >   if (has_attr()) {
> >      kvm_supports_set_timer_irq = true;
> >      vtimer_irq = foo;
> >   } else {
> >      kvm_supports_set_timer_irq = false;
> >      vtimer_irq = 27; /* default, we're stuck with it */
> >   }
> > 
> >   create_board_definition();
> >   create_dt();
> >   create_acpi();
> > 
> >   /* do whatever */
> > 
> >   if (kvm_supports_set_timer_irq && kvm_irqchip_in_kernel()) {
> >   	kvm_arm_timer_set_irq(...);
> >   }
> > 
> > And all this should not be coupled to when we create the irqchip device.
> > 
> > But I may be failing to see the case where the current implementation
> > creates a problem for userspace, in which case we should document the
> > ordering requirement.
> 
> I'm not sure it would create any problem, at least not for the PMU
> (there is no working code that would have created a PMU without an irqchip).
> 
> It is just that we have a slightly diverging interpretation of what
> has_attr means. You see it as "attribute that the device supports", and
> I see it as "attribute that the device supports in this configuration".
> I'm happy to use your semantics from now on.

In either case, we should make sure this is clear in the ABI.  I thought
that the "It does not necessarily indicate that the attribute can be
read or written in the device's current state." implied my
interpretation, but maybe I'm missing some subtlety there?

Do you think we should clarify the API?

By the way, I now realize that we are not maintining the same
understanding between get/set_attr, which I really think we should, so
I'll add the following:


diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
index 9b3e3ea..0cf62b7 100644
--- a/virt/kvm/arm/pmu.c
+++ b/virt/kvm/arm/pmu.c
@@ -551,6 +551,9 @@ int kvm_arm_pmu_v3_get_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 		int __user *uaddr = (int __user *)(long)attr->addr;
 		int irq;
 
+		if (!irqchip_in_kernel(vcpu->kvm))
+			return -EINVAL;
+
 		if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
 			return -ENODEV;
 

I was only talking about has_attr above.

Let me know.

Thanks,
-Christoffer

[PATCH 1/5] KVM: arm64: Allow creating the PMU without the in-kernel GIC

[Marc Zyngier]

On 04/05/17 10:44, Christoffer Dall wrote:
> On Thu, May 04, 2017 at 10:05:43AM +0100, Marc Zyngier wrote:
>> On 04/05/17 09:38, Christoffer Dall wrote:
>>> On Thu, May 04, 2017 at 09:28:50AM +0100, Marc Zyngier wrote:
>>>> On 04/05/17 09:13, Christoffer Dall wrote:
>>>>> On Thu, May 04, 2017 at 09:09:47AM +0100, Marc Zyngier wrote:
>>>>>> On 03/05/17 19:32, Christoffer Dall wrote:
>>>>>>> Since we got support for devices in userspace which allows reporting the
>>>>>>> PMU overflow output status to userspace, we should actually allow
>>>>>>> creating the PMU on systems without an in-kernel irqchip, which in turn
>>>>>>> requires us to slightly clarify error codes for the ABI and move things
>>>>>>> around for the initialization phase.
>>>>>>>
>>>>>>> Signed-off-by: Christoffer Dall <cdall@linaro.org>
>>>>>>> ---
>>>>>>>  Documentation/virtual/kvm/devices/vcpu.txt | 16 +++++++++-------
>>>>>>>  virt/kvm/arm/pmu.c                         | 27 +++++++++++++++++----------
>>>>>>>  2 files changed, 26 insertions(+), 17 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt
>>>>>>> index 02f5068..352af6e 100644
>>>>>>> --- a/Documentation/virtual/kvm/devices/vcpu.txt
>>>>>>> +++ b/Documentation/virtual/kvm/devices/vcpu.txt
>>>>>>> @@ -16,7 +16,9 @@ Parameters: in kvm_device_attr.addr the address for PMU overflow interrupt is a
>>>>>>>  Returns: -EBUSY: The PMU overflow interrupt is already set
>>>>>>>           -ENXIO: The overflow interrupt not set when attempting to get it
>>>>>>>           -ENODEV: PMUv3 not supported
>>>>>>> -         -EINVAL: Invalid PMU overflow interrupt number supplied
>>>>>>> +         -EINVAL: Invalid PMU overflow interrupt number supplied or
>>>>>>> +                  trying to set the IRQ number without using an in-kernel
>>>>>>> +                  irqchip.
>>>>>>>  
>>>>>>>  A value describing the PMUv3 (Performance Monitor Unit v3) overflow interrupt
>>>>>>>  number for this vcpu. This interrupt could be a PPI or SPI, but the interrupt
>>>>>>> @@ -25,11 +27,11 @@ all vcpus, while as an SPI it must be a separate number per vcpu.
>>>>>>>  
>>>>>>>  1.2 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_INIT
>>>>>>>  Parameters: no additional parameter in kvm_device_attr.addr
>>>>>>> -Returns: -ENODEV: PMUv3 not supported
>>>>>>> -         -ENXIO: PMUv3 not properly configured as required prior to calling this
>>>>>>> -                 attribute
>>>>>>> +Returns: -ENODEV: PMUv3 not supported or GIC not initialized
>>>>>>> +         -ENXIO: PMUv3 not properly configured or in-kernel irqchip not
>>>>>>> +                 conigured as required prior to calling this attribute
>>>>>>>           -EBUSY: PMUv3 already initialized
>>>>>>>  
>>>>>>> -Request the initialization of the PMUv3.  This must be done after creating the
>>>>>>> -in-kernel irqchip.  Creating a PMU with a userspace irqchip is currently not
>>>>>>> -supported.
>>>>>>> +Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
>>>>>>> +virtual GIC implementation, this must be done after initializing the in-kernel
>>>>>>> +irqchip.
>>>>>>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>>>>>>> index 4b43e7f..f046b08 100644
>>>>>>> --- a/virt/kvm/arm/pmu.c
>>>>>>> +++ b/virt/kvm/arm/pmu.c
>>>>>>> @@ -456,21 +456,25 @@ static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>>>>>>>  	if (!kvm_arm_support_pmu_v3())
>>>>>>>  		return -ENODEV;
>>>>>>>  
>>>>>>> -	/*
>>>>>>> -	 * We currently require an in-kernel VGIC to use the PMU emulation,
>>>>>>> -	 * because we do not support forwarding PMU overflow interrupts to
>>>>>>> -	 * userspace yet.
>>>>>>> -	 */
>>>>>>> -	if (!irqchip_in_kernel(vcpu->kvm) || !vgic_initialized(vcpu->kvm))
>>>>>>> -		return -ENODEV;
>>>>>>> -
>>>>>>> -	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features) ||
>>>>>>> -	    !kvm_arm_pmu_irq_initialized(vcpu))
>>>>>>> +	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>>>>>  		return -ENXIO;
>>>>>>>  
>>>>>>>  	if (kvm_arm_pmu_v3_ready(vcpu))
>>>>>>>  		return -EBUSY;
>>>>>>>  
>>>>>>> +	if (irqchip_in_kernel(vcpu->kvm)) {
>>>>>>> +		/*
>>>>>>> +		 * If using the PMU with an in-kernel virtual GIC
>>>>>>> +		 * implementation, we require the GIC to be already
>>>>>>> +		 * initialized when initializing the PMU.
>>>>>>> +		 */
>>>>>>> +		if (!vgic_initialized(vcpu->kvm))
>>>>>>> +			return -ENODEV;
>>>>>>> +
>>>>>>> +		if (!kvm_arm_pmu_irq_initialized(vcpu))
>>>>>>> +			return -ENXIO;
>>>>>>> +	}
>>>>>>> +
>>>>>>>  	kvm_pmu_vcpu_reset(vcpu);
>>>>>>>  	vcpu->arch.pmu.ready = true;
>>>>>>>  
>>>>>>> @@ -512,6 +516,9 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>>>>>>>  		int __user *uaddr = (int __user *)(long)attr->addr;
>>>>>>>  		int irq;
>>>>>>>  
>>>>>>> +		if (!irqchip_in_kernel(vcpu->kvm))
>>>>>>> +			return -EINVAL;
>>>>>>> +
>>>>>>
>>>>>> Shouldn't we fail the same way for {get,has}_attr? get_attr is going to
>>>>>> generate a -ENXIO, and has_attr is going to lie about the availability
>>>>>> of KVM_ARM_VCPU_PMU_V3_IRQ...
>>>>>>
>>>>>
>>>>> Here's the text from api.txt:
>>>>>
>>>>>   Tests whether a device supports a particular attribute.  A successful
>>>>>   return indicates the attribute is implemented.  It does not necessarily
>>>>>   indicate that the attribute can be read or written in the device's
>>>>>   current state.  "addr" is ignored.
>>>>>
>>>>> My interpretation therefore is that QEMU can use this ioctl to figure
>>>>> out if the feature is supported (sort of like a capability), but that
>>>>> doesn't mean that the configuration of the VM is such that the attribute
>>>>> can be get or set at that moment.
>>>>>
>>>>> For example, there will also alway be situations where you can get an
>>>>> attr, but not set an attr, what should the has_attr return then?
>>>>
>>>> My issue here is that whether we can get/set the interrupt or not is not
>>>> a function of the device itself, but of the way it is "wired". No matter
>>>> what "the device's current state" is, we'll never be able to get/set the
>>>> interrupt.
>>>>
>>>> I'd tend to err on the side of caution and return something that is
>>>> unambiguous, be maybe I have too strict an interpretation of the API.
>>>>
>>>
>>> Hmm, I see the has_attr as a method for userspace to discover "does this
>>> kernel have this feature".  If we make has_attr return a value depending
>>> on the VM having an in-kernel gic or not, we implicitly require
>>> userspace to create a VM with an in-kernel GIC to discover if this
>>> kernel has that feature, and therefore also impose an ordering
>>> requirement of figuring out the capabilities of the kernel (i.e. create
>>> the GIC before checking this API).
>>>
>>> I think QEMU should be able to do:
>>>
>>>   if (has_attr()) {
>>>      kvm_supports_set_timer_irq = true;
>>>      vtimer_irq = foo;
>>>   } else {
>>>      kvm_supports_set_timer_irq = false;
>>>      vtimer_irq = 27; /* default, we're stuck with it */
>>>   }
>>>
>>>   create_board_definition();
>>>   create_dt();
>>>   create_acpi();
>>>
>>>   /* do whatever */
>>>
>>>   if (kvm_supports_set_timer_irq && kvm_irqchip_in_kernel()) {
>>>   	kvm_arm_timer_set_irq(...);
>>>   }
>>>
>>> And all this should not be coupled to when we create the irqchip device.
>>>
>>> But I may be failing to see the case where the current implementation
>>> creates a problem for userspace, in which case we should document the
>>> ordering requirement.
>>
>> I'm not sure it would create any problem, at least not for the PMU
>> (there is no working code that would have created a PMU without an irqchip).
>>
>> It is just that we have a slightly diverging interpretation of what
>> has_attr means. You see it as "attribute that the device supports", and
>> I see it as "attribute that the device supports in this configuration".
>> I'm happy to use your semantics from now on.
> 
> In either case, we should make sure this is clear in the ABI.  I thought
> that the "It does not necessarily indicate that the attribute can be
> read or written in the device's current state." implied my
> interpretation, but maybe I'm missing some subtlety there?

Well, that's what I said above. The interrupt number is not a function
of the device state, but one of the integration of that device in the
system. The PMU itself is not concerned with an interrupt number, only
the GIC is.

> Do you think we should clarify the API?

Not sure. I admit my interpretation is a bit borderline. If I was brave,
I'd say that all the interrupt numbers should be a property of the GIC,
and not of the device. But that ship has sailed a while ago, and I'm
weak ;-).

More seriously, I don't think this is likely to cause any issue.

> 
> By the way, I now realize that we are not maintining the same
> understanding between get/set_attr, which I really think we should, so
> I'll add the following:
> 
> 
> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
> index 9b3e3ea..0cf62b7 100644
> --- a/virt/kvm/arm/pmu.c
> +++ b/virt/kvm/arm/pmu.c
> @@ -551,6 +551,9 @@ int kvm_arm_pmu_v3_get_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  		int __user *uaddr = (int __user *)(long)attr->addr;
>  		int irq;
>  
> +		if (!irqchip_in_kernel(vcpu->kvm))
> +			return -EINVAL;
> +
>  		if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>  			return -ENODEV;
>  

Looks good to me.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 1/5] KVM: arm64: Allow creating the PMU without the in-kernel GIC

[Christoffer Dall]

On Thu, May 04, 2017 at 11:16:57AM +0100, Marc Zyngier wrote:
> On 04/05/17 10:44, Christoffer Dall wrote:
> > On Thu, May 04, 2017 at 10:05:43AM +0100, Marc Zyngier wrote:
> >> On 04/05/17 09:38, Christoffer Dall wrote:
> >>> On Thu, May 04, 2017 at 09:28:50AM +0100, Marc Zyngier wrote:
> >>>> On 04/05/17 09:13, Christoffer Dall wrote:
> >>>>> On Thu, May 04, 2017 at 09:09:47AM +0100, Marc Zyngier wrote:
> >>>>>> On 03/05/17 19:32, Christoffer Dall wrote:
> >>>>>>> Since we got support for devices in userspace which allows reporting the
> >>>>>>> PMU overflow output status to userspace, we should actually allow
> >>>>>>> creating the PMU on systems without an in-kernel irqchip, which in turn
> >>>>>>> requires us to slightly clarify error codes for the ABI and move things
> >>>>>>> around for the initialization phase.
> >>>>>>>
> >>>>>>> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> >>>>>>> ---
> >>>>>>>  Documentation/virtual/kvm/devices/vcpu.txt | 16 +++++++++-------
> >>>>>>>  virt/kvm/arm/pmu.c                         | 27 +++++++++++++++++----------
> >>>>>>>  2 files changed, 26 insertions(+), 17 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt
> >>>>>>> index 02f5068..352af6e 100644
> >>>>>>> --- a/Documentation/virtual/kvm/devices/vcpu.txt
> >>>>>>> +++ b/Documentation/virtual/kvm/devices/vcpu.txt
> >>>>>>> @@ -16,7 +16,9 @@ Parameters: in kvm_device_attr.addr the address for PMU overflow interrupt is a
> >>>>>>>  Returns: -EBUSY: The PMU overflow interrupt is already set
> >>>>>>>           -ENXIO: The overflow interrupt not set when attempting to get it
> >>>>>>>           -ENODEV: PMUv3 not supported
> >>>>>>> -         -EINVAL: Invalid PMU overflow interrupt number supplied
> >>>>>>> +         -EINVAL: Invalid PMU overflow interrupt number supplied or
> >>>>>>> +                  trying to set the IRQ number without using an in-kernel
> >>>>>>> +                  irqchip.
> >>>>>>>  
> >>>>>>>  A value describing the PMUv3 (Performance Monitor Unit v3) overflow interrupt
> >>>>>>>  number for this vcpu. This interrupt could be a PPI or SPI, but the interrupt
> >>>>>>> @@ -25,11 +27,11 @@ all vcpus, while as an SPI it must be a separate number per vcpu.
> >>>>>>>  
> >>>>>>>  1.2 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_INIT
> >>>>>>>  Parameters: no additional parameter in kvm_device_attr.addr
> >>>>>>> -Returns: -ENODEV: PMUv3 not supported
> >>>>>>> -         -ENXIO: PMUv3 not properly configured as required prior to calling this
> >>>>>>> -                 attribute
> >>>>>>> +Returns: -ENODEV: PMUv3 not supported or GIC not initialized
> >>>>>>> +         -ENXIO: PMUv3 not properly configured or in-kernel irqchip not
> >>>>>>> +                 conigured as required prior to calling this attribute
> >>>>>>>           -EBUSY: PMUv3 already initialized
> >>>>>>>  
> >>>>>>> -Request the initialization of the PMUv3.  This must be done after creating the
> >>>>>>> -in-kernel irqchip.  Creating a PMU with a userspace irqchip is currently not
> >>>>>>> -supported.
> >>>>>>> +Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
> >>>>>>> +virtual GIC implementation, this must be done after initializing the in-kernel
> >>>>>>> +irqchip.
> >>>>>>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
> >>>>>>> index 4b43e7f..f046b08 100644
> >>>>>>> --- a/virt/kvm/arm/pmu.c
> >>>>>>> +++ b/virt/kvm/arm/pmu.c
> >>>>>>> @@ -456,21 +456,25 @@ static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
> >>>>>>>  	if (!kvm_arm_support_pmu_v3())
> >>>>>>>  		return -ENODEV;
> >>>>>>>  
> >>>>>>> -	/*
> >>>>>>> -	 * We currently require an in-kernel VGIC to use the PMU emulation,
> >>>>>>> -	 * because we do not support forwarding PMU overflow interrupts to
> >>>>>>> -	 * userspace yet.
> >>>>>>> -	 */
> >>>>>>> -	if (!irqchip_in_kernel(vcpu->kvm) || !vgic_initialized(vcpu->kvm))
> >>>>>>> -		return -ENODEV;
> >>>>>>> -
> >>>>>>> -	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features) ||
> >>>>>>> -	    !kvm_arm_pmu_irq_initialized(vcpu))
> >>>>>>> +	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
> >>>>>>>  		return -ENXIO;
> >>>>>>>  
> >>>>>>>  	if (kvm_arm_pmu_v3_ready(vcpu))
> >>>>>>>  		return -EBUSY;
> >>>>>>>  
> >>>>>>> +	if (irqchip_in_kernel(vcpu->kvm)) {
> >>>>>>> +		/*
> >>>>>>> +		 * If using the PMU with an in-kernel virtual GIC
> >>>>>>> +		 * implementation, we require the GIC to be already
> >>>>>>> +		 * initialized when initializing the PMU.
> >>>>>>> +		 */
> >>>>>>> +		if (!vgic_initialized(vcpu->kvm))
> >>>>>>> +			return -ENODEV;
> >>>>>>> +
> >>>>>>> +		if (!kvm_arm_pmu_irq_initialized(vcpu))
> >>>>>>> +			return -ENXIO;
> >>>>>>> +	}
> >>>>>>> +
> >>>>>>>  	kvm_pmu_vcpu_reset(vcpu);
> >>>>>>>  	vcpu->arch.pmu.ready = true;
> >>>>>>>  
> >>>>>>> @@ -512,6 +516,9 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
> >>>>>>>  		int __user *uaddr = (int __user *)(long)attr->addr;
> >>>>>>>  		int irq;
> >>>>>>>  
> >>>>>>> +		if (!irqchip_in_kernel(vcpu->kvm))
> >>>>>>> +			return -EINVAL;
> >>>>>>> +
> >>>>>>
> >>>>>> Shouldn't we fail the same way for {get,has}_attr? get_attr is going to
> >>>>>> generate a -ENXIO, and has_attr is going to lie about the availability
> >>>>>> of KVM_ARM_VCPU_PMU_V3_IRQ...
> >>>>>>
> >>>>>
> >>>>> Here's the text from api.txt:
> >>>>>
> >>>>>   Tests whether a device supports a particular attribute.  A successful
> >>>>>   return indicates the attribute is implemented.  It does not necessarily
> >>>>>   indicate that the attribute can be read or written in the device's
> >>>>>   current state.  "addr" is ignored.
> >>>>>
> >>>>> My interpretation therefore is that QEMU can use this ioctl to figure
> >>>>> out if the feature is supported (sort of like a capability), but that
> >>>>> doesn't mean that the configuration of the VM is such that the attribute
> >>>>> can be get or set at that moment.
> >>>>>
> >>>>> For example, there will also alway be situations where you can get an
> >>>>> attr, but not set an attr, what should the has_attr return then?
> >>>>
> >>>> My issue here is that whether we can get/set the interrupt or not is not
> >>>> a function of the device itself, but of the way it is "wired". No matter
> >>>> what "the device's current state" is, we'll never be able to get/set the
> >>>> interrupt.
> >>>>
> >>>> I'd tend to err on the side of caution and return something that is
> >>>> unambiguous, be maybe I have too strict an interpretation of the API.
> >>>>
> >>>
> >>> Hmm, I see the has_attr as a method for userspace to discover "does this
> >>> kernel have this feature".  If we make has_attr return a value depending
> >>> on the VM having an in-kernel gic or not, we implicitly require
> >>> userspace to create a VM with an in-kernel GIC to discover if this
> >>> kernel has that feature, and therefore also impose an ordering
> >>> requirement of figuring out the capabilities of the kernel (i.e. create
> >>> the GIC before checking this API).
> >>>
> >>> I think QEMU should be able to do:
> >>>
> >>>   if (has_attr()) {
> >>>      kvm_supports_set_timer_irq = true;
> >>>      vtimer_irq = foo;
> >>>   } else {
> >>>      kvm_supports_set_timer_irq = false;
> >>>      vtimer_irq = 27; /* default, we're stuck with it */
> >>>   }
> >>>
> >>>   create_board_definition();
> >>>   create_dt();
> >>>   create_acpi();
> >>>
> >>>   /* do whatever */
> >>>
> >>>   if (kvm_supports_set_timer_irq && kvm_irqchip_in_kernel()) {
> >>>   	kvm_arm_timer_set_irq(...);
> >>>   }
> >>>
> >>> And all this should not be coupled to when we create the irqchip device.
> >>>
> >>> But I may be failing to see the case where the current implementation
> >>> creates a problem for userspace, in which case we should document the
> >>> ordering requirement.
> >>
> >> I'm not sure it would create any problem, at least not for the PMU
> >> (there is no working code that would have created a PMU without an irqchip).
> >>
> >> It is just that we have a slightly diverging interpretation of what
> >> has_attr means. You see it as "attribute that the device supports", and
> >> I see it as "attribute that the device supports in this configuration".
> >> I'm happy to use your semantics from now on.
> > 
> > In either case, we should make sure this is clear in the ABI.  I thought
> > that the "It does not necessarily indicate that the attribute can be
> > read or written in the device's current state." implied my
> > interpretation, but maybe I'm missing some subtlety there?
> 
> Well, that's what I said above. The interrupt number is not a function
> of the device state, but one of the integration of that device in the
> system. The PMU itself is not concerned with an interrupt number, only
> the GIC is.
> 
> > Do you think we should clarify the API?
> 
> Not sure. I admit my interpretation is a bit borderline. If I was brave,
> I'd say that all the interrupt numbers should be a property of the GIC,
> and not of the device. But that ship has sailed a while ago, and I'm
> weak ;-).
> 
> More seriously, I don't think this is likely to cause any issue.
> 

ok, I'll leave it as is then.

> > 
> > By the way, I now realize that we are not maintining the same
> > understanding between get/set_attr, which I really think we should, so
> > I'll add the following:
> > 
> > 
> > diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
> > index 9b3e3ea..0cf62b7 100644
> > --- a/virt/kvm/arm/pmu.c
> > +++ b/virt/kvm/arm/pmu.c
> > @@ -551,6 +551,9 @@ int kvm_arm_pmu_v3_get_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
> >  		int __user *uaddr = (int __user *)(long)attr->addr;
> >  		int irq;
> >  
> > +		if (!irqchip_in_kernel(vcpu->kvm))
> > +			return -EINVAL;
> > +
> >  		if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
> >  			return -ENODEV;
> >  
> 
> Looks good to me.
> 

Thanks,
-Christoffer

[PATCH 2/5] KVM: arm: Handle VCPU device attributes in guest.c

[Christoffer Dall]

As we are about to support VCPU attributes to set the timer IRQ numbers
in guest.c, move the static inlines for the VCPU attributes handlers
from the header file to guest.c.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 arch/arm/include/asm/kvm_host.h | 22 +++++++--------------
 arch/arm/kvm/guest.c            | 42 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 15 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index d488b88..0e8c233 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -292,20 +292,12 @@ static inline void kvm_arm_init_debug(void) {}
 static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) {}
 static inline void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu) {}
-static inline int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
-					     struct kvm_device_attr *attr)
-{
-	return -ENXIO;
-}
-static inline int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
-					     struct kvm_device_attr *attr)
-{
-	return -ENXIO;
-}
-static inline int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
-					     struct kvm_device_attr *attr)
-{
-	return -ENXIO;
-}
+
+int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
+			       struct kvm_device_attr *attr);
+int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
+			       struct kvm_device_attr *attr);
+int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
+			       struct kvm_device_attr *attr);
 
 #endif /* __ARM_KVM_HOST_H__ */
diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
index fa6182a..acea05e 100644
--- a/arch/arm/kvm/guest.c
+++ b/arch/arm/kvm/guest.c
@@ -301,3 +301,45 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 {
 	return -EINVAL;
 }
+
+int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
+			       struct kvm_device_attr *attr)
+{
+	int ret;
+
+	switch (attr->group) {
+	default:
+		ret = -ENXIO;
+		break;
+	}
+
+	return ret;
+}
+
+int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
+			       struct kvm_device_attr *attr)
+{
+	int ret;
+
+	switch (attr->group) {
+	default:
+		ret = -ENXIO;
+		break;
+	}
+
+	return ret;
+}
+
+int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
+			       struct kvm_device_attr *attr)
+{
+	int ret;
+
+	switch (attr->group) {
+	default:
+		ret = -ENXIO;
+		break;
+	}
+
+	return ret;
+}
-- 
2.9.0

[PATCH 3/5] KVM: arm/arm64: Move irq_is_ppi() to header file

[Christoffer Dall]

We are about to need this define in the arch timer code as well so move
it to a common location.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 include/kvm/arm_vgic.h | 2 ++
 virt/kvm/arm/pmu.c     | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 26ed4fb..1541f5d 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -38,6 +38,8 @@
 #define VGIC_MIN_LPI		8192
 #define KVM_IRQCHIP_NUM_PINS	(1020 - 32)
 
+#define irq_is_ppi(irq) ((irq) >= VGIC_NR_SGIS && (irq) < VGIC_NR_PRIVATE_IRQS)
+
 enum vgic_type {
 	VGIC_V2,		/* Good ol' GICv2 */
 	VGIC_V3,		/* New fancy GICv3 */
diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
index f046b08..9b3e3ea 100644
--- a/virt/kvm/arm/pmu.c
+++ b/virt/kvm/arm/pmu.c
@@ -481,8 +481,6 @@ static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
-#define irq_is_ppi(irq) ((irq) >= VGIC_NR_SGIS && (irq) < VGIC_NR_PRIVATE_IRQS)
-
 /*
  * For one VM the interrupt type must be same for each vcpu.
  * As a PPI, the interrupt number is the same for all vcpus,
-- 
2.9.0

[PATCH 3/5] KVM: arm/arm64: Move irq_is_ppi() to header file

[Marc Zyngier]

On 03/05/17 19:32, Christoffer Dall wrote:
> We are about to need this define in the arch timer code as well so move
> it to a common location.
> 
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> ---
>  include/kvm/arm_vgic.h | 2 ++
>  virt/kvm/arm/pmu.c     | 2 --
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 26ed4fb..1541f5d 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -38,6 +38,8 @@
>  #define VGIC_MIN_LPI		8192
>  #define KVM_IRQCHIP_NUM_PINS	(1020 - 32)
>  
> +#define irq_is_ppi(irq) ((irq) >= VGIC_NR_SGIS && (irq) < VGIC_NR_PRIVATE_IRQS)
> +
>  enum vgic_type {
>  	VGIC_V2,		/* Good ol' GICv2 */
>  	VGIC_V3,		/* New fancy GICv3 */
> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
> index f046b08..9b3e3ea 100644
> --- a/virt/kvm/arm/pmu.c
> +++ b/virt/kvm/arm/pmu.c
> @@ -481,8 +481,6 @@ static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>  	return 0;
>  }
>  
> -#define irq_is_ppi(irq) ((irq) >= VGIC_NR_SGIS && (irq) < VGIC_NR_PRIVATE_IRQS)
> -
>  /*
>   * For one VM the interrupt type must be same for each vcpu.
>   * As a PPI, the interrupt number is the same for all vcpus,
> 

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 4/5] KVM: arm/arm64: Move timer IRQ default init to arch_timer.c

[Christoffer Dall]

We currently initialize the arch timer IRQ numbers from the reset code,
presumably because we once intended to model multiple CPU or SoC types
from within the kernel and have hard-coded reset values in the reset
code.

As we are moving towards userspace being in charge of more fine-grained
CPU emulation and stitching together the pieces needed to emulate a
particular type of CPU, we should no longer have a tight coupling
between resetting a VCPU and setting IRQ numbers.

Therefore, move the logic to define and use the default IRQ numbers to
the timer code and set the IRQ number immediately when creating the
VCPU.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 arch/arm/kvm/reset.c         | 16 +---------------
 arch/arm64/kvm/reset.c       | 16 +---------------
 include/kvm/arm_arch_timer.h |  4 +---
 virt/kvm/arm/arch_timer.c    | 28 ++++++++++++++++------------
 4 files changed, 19 insertions(+), 45 deletions(-)

diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
index 1da8b2d..5ed0c3e 100644
--- a/arch/arm/kvm/reset.c
+++ b/arch/arm/kvm/reset.c
@@ -37,16 +37,6 @@ static struct kvm_regs cortexa_regs_reset = {
 	.usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT,
 };
 
-static const struct kvm_irq_level cortexa_ptimer_irq = {
-	{ .irq = 30 },
-	.level = 1,
-};
-
-static const struct kvm_irq_level cortexa_vtimer_irq = {
-	{ .irq = 27 },
-	.level = 1,
-};
-
 
 /*******************************************************************************
  * Exported reset function
@@ -62,16 +52,12 @@ static const struct kvm_irq_level cortexa_vtimer_irq = {
 int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 {
 	struct kvm_regs *reset_regs;
-	const struct kvm_irq_level *cpu_vtimer_irq;
-	const struct kvm_irq_level *cpu_ptimer_irq;
 
 	switch (vcpu->arch.target) {
 	case KVM_ARM_TARGET_CORTEX_A7:
 	case KVM_ARM_TARGET_CORTEX_A15:
 		reset_regs = &cortexa_regs_reset;
 		vcpu->arch.midr = read_cpuid_id();
-		cpu_vtimer_irq = &cortexa_vtimer_irq;
-		cpu_ptimer_irq = &cortexa_ptimer_irq;
 		break;
 	default:
 		return -ENODEV;
@@ -84,5 +70,5 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 	kvm_reset_coprocs(vcpu);
 
 	/* Reset arch_timer context */
-	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq, cpu_ptimer_irq);
+	return kvm_timer_vcpu_reset(vcpu);
 }
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index d9e9697..4942de2 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -46,16 +46,6 @@ static const struct kvm_regs default_regs_reset32 = {
 			COMPAT_PSR_I_BIT | COMPAT_PSR_F_BIT),
 };
 
-static const struct kvm_irq_level default_ptimer_irq = {
-	.irq	= 30,
-	.level	= 1,
-};
-
-static const struct kvm_irq_level default_vtimer_irq = {
-	.irq	= 27,
-	.level	= 1,
-};
-
 static bool cpu_has_32bit_el1(void)
 {
 	u64 pfr0;
@@ -108,8 +98,6 @@ int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, long ext)
  */
 int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 {
-	const struct kvm_irq_level *cpu_vtimer_irq;
-	const struct kvm_irq_level *cpu_ptimer_irq;
 	const struct kvm_regs *cpu_reset;
 
 	switch (vcpu->arch.target) {
@@ -122,8 +110,6 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 			cpu_reset = &default_regs_reset;
 		}
 
-		cpu_vtimer_irq = &default_vtimer_irq;
-		cpu_ptimer_irq = &default_ptimer_irq;
 		break;
 	}
 
@@ -137,5 +123,5 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 	kvm_pmu_vcpu_reset(vcpu);
 
 	/* Reset timer */
-	return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq, cpu_ptimer_irq);
+	return kvm_timer_vcpu_reset(vcpu);
 }
diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index 295584f..f1c967a 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -57,9 +57,7 @@ struct arch_timer_cpu {
 
 int kvm_timer_hyp_init(void);
 int kvm_timer_enable(struct kvm_vcpu *vcpu);
-int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
-			 const struct kvm_irq_level *virt_irq,
-			 const struct kvm_irq_level *phys_irq);
+int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu);
 void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu);
 void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu);
 void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu);
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 5976609..6c5a064 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -35,6 +35,16 @@ static struct timecounter *timecounter;
 static unsigned int host_vtimer_irq;
 static u32 host_vtimer_irq_flags;
 
+static const struct kvm_irq_level default_ptimer_irq = {
+	.irq	= 30,
+	.level	= 1,
+};
+
+static const struct kvm_irq_level default_vtimer_irq = {
+	.irq	= 27,
+	.level	= 1,
+};
+
 void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu)
 {
 	vcpu_vtimer(vcpu)->active_cleared_last = false;
@@ -445,23 +455,12 @@ void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu)
 	kvm_timer_update_state(vcpu);
 }
 
-int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
-			 const struct kvm_irq_level *virt_irq,
-			 const struct kvm_irq_level *phys_irq)
+int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
 
 	/*
-	 * The vcpu timer irq number cannot be determined in
-	 * kvm_timer_vcpu_init() because it is called much before
-	 * kvm_vcpu_set_target(). To handle this, we determine
-	 * vcpu timer irq number when the vcpu is reset.
-	 */
-	vtimer->irq.irq = virt_irq->irq;
-	ptimer->irq.irq = phys_irq->irq;
-
-	/*
 	 * The bits in CNTV_CTL are architecturally reset to UNKNOWN for ARMv8
 	 * and to 0 for ARMv7.  We provide an implementation that always
 	 * resets the timer to be disabled and unmasked and is compliant with
@@ -496,6 +495,8 @@ static void update_vtimer_cntvoff(struct kvm_vcpu *vcpu, u64 cntvoff)
 void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
+	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
+	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
 
 	/* Synchronize cntvoff across all vtimers of a VM. */
 	update_vtimer_cntvoff(vcpu, kvm_phys_timer_read());
@@ -504,6 +505,9 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu)
 	INIT_WORK(&timer->expired, kvm_timer_inject_irq_work);
 	hrtimer_init(&timer->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
 	timer->timer.function = kvm_timer_expire;
+
+	vtimer->irq.irq = default_vtimer_irq.irq;
+	ptimer->irq.irq = default_ptimer_irq.irq;
 }
 
 static void kvm_timer_init_interrupt(void *info)
-- 
2.9.0

[PATCH 5/5] KVM: arm/arm64: Allow setting the timer IRQ numbers from userspace

[Christoffer Dall]

First we define an ABI using the vcpu devices that lets userspace set
the interrupt numbers for the various timers on both the 32-bit and
64-bit KVM/ARM implementations.

Second, we add the definitions for the groups and attributes introduced
by the above ABI.  (We add the PMU define on the 32-bit side as well for
symmetry and it may get used some day.)

Third, we set up the arch-specific vcpu device operation handlers to
call into the timer code for anything related to the
KVM_ARM_VCPU_TIMER_CTRL group.

Fourth, we implement support for getting and setting the timer interrupt
numbers using the above defined ABI in the arch timer code.

Fifth, we introduce error checking upon enabling the arch timer (which
is called when first running a VCPU) to check that all VCPUs are
configured to use the same PPI for the timer (as mandated by the
architecture) and that the virtual and physical timers are not
configured to use the same IRQ number.

Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 Documentation/virtual/kvm/devices/vcpu.txt |  25 +++++++
 arch/arm/include/uapi/asm/kvm.h            |   8 +++
 arch/arm/kvm/guest.c                       |   9 +++
 arch/arm64/include/uapi/asm/kvm.h          |   3 +
 arch/arm64/kvm/guest.c                     |   9 +++
 include/kvm/arm_arch_timer.h               |   4 ++
 virt/kvm/arm/arch_timer.c                  | 104 +++++++++++++++++++++++++++++
 7 files changed, 162 insertions(+)

diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt
index 352af6e..013e3f1 100644
--- a/Documentation/virtual/kvm/devices/vcpu.txt
+++ b/Documentation/virtual/kvm/devices/vcpu.txt
@@ -35,3 +35,28 @@ Returns: -ENODEV: PMUv3 not supported or GIC not initialized
 Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
 virtual GIC implementation, this must be done after initializing the in-kernel
 irqchip.
+
+
+2. GROUP: KVM_ARM_VCPU_TIMER_CTRL
+Architectures: ARM,ARM64
+
+2.1. ATTRIBUTE: KVM_ARM_VCPU_TIMER_IRQ_VTIMER
+2.2. ATTRIBUTE: KVM_ARM_VCPU_TIMER_IRQ_PTIMER
+Parameters: in kvm_device_attr.addr the address for the timer interrupt is a
+            pointer to an int
+Returns: -EINVAL: Invalid timer interrupt number
+         -EBUSY:  One or more VCPUs has already run
+
+A value describing the architected timer interrupt number when connected to an
+in-kernel virtual GIC.  These must be a PPI (16 <= intid < 32).  If an
+attribute is not set, a default value is applied (see below).
+
+KVM_ARM_VCPU_TIMER_IRQ_VTIMER: The EL1 virtual timer intid (default: 27)
+KVM_ARM_VCPU_TIMER_IRQ_PTIMER: The EL1 physical timer intid (default: 30)
+
+Setting the same PPI for different timers will prevent the VCPUs from running.
+Setting the interrupt number on a VCPU configures all VCPUs created at that
+time to use the number provided for a given timer, overwriting any previously
+configured values on other VCPUs.  Userspace should configure the interrupt
+numbers on@least one VCPU after creating all VCPUs and before running any
+VCPUs.
diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
index a5838d6..6a75ec4 100644
--- a/arch/arm/include/uapi/asm/kvm.h
+++ b/arch/arm/include/uapi/asm/kvm.h
@@ -200,6 +200,14 @@ struct kvm_arch_memory_slot {
 #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x3ff
 #define VGIC_LEVEL_INFO_LINE_LEVEL	0
 
+/* Device Control API on vcpu fd */
+#define KVM_ARM_VCPU_PMU_V3_CTRL	0
+#define   KVM_ARM_VCPU_PMU_V3_IRQ	0
+#define   KVM_ARM_VCPU_PMU_V3_INIT	1
+#define KVM_ARM_VCPU_TIMER_CTRL		1
+#define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER		0
+#define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER		1
+
 #define   KVM_DEV_ARM_VGIC_CTRL_INIT    0
 
 /* KVM_IRQ_LINE irq field index values */
diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
index acea05e..1e0784e 100644
--- a/arch/arm/kvm/guest.c
+++ b/arch/arm/kvm/guest.c
@@ -308,6 +308,9 @@ int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
 	int ret;
 
 	switch (attr->group) {
+	case KVM_ARM_VCPU_TIMER_CTRL:
+		ret = kvm_arm_timer_set_attr(vcpu, attr);
+		break;
 	default:
 		ret = -ENXIO;
 		break;
@@ -322,6 +325,9 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
 	int ret;
 
 	switch (attr->group) {
+	case KVM_ARM_VCPU_TIMER_CTRL:
+		ret = kvm_arm_timer_get_attr(vcpu, attr);
+		break;
 	default:
 		ret = -ENXIO;
 		break;
@@ -336,6 +342,9 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
 	int ret;
 
 	switch (attr->group) {
+	case KVM_ARM_VCPU_TIMER_CTRL:
+		ret = kvm_arm_timer_has_attr(vcpu, attr);
+		break;
 	default:
 		ret = -ENXIO;
 		break;
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index cd6bea4..fc64518 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -226,6 +226,9 @@ struct kvm_arch_memory_slot {
 #define KVM_ARM_VCPU_PMU_V3_CTRL	0
 #define   KVM_ARM_VCPU_PMU_V3_IRQ	0
 #define   KVM_ARM_VCPU_PMU_V3_INIT	1
+#define KVM_ARM_VCPU_TIMER_CTRL		1
+#define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER		0
+#define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER		1
 
 /* KVM_IRQ_LINE irq field index values */
 #define KVM_ARM_IRQ_TYPE_SHIFT		24
diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index b37446a..5c7f657 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -390,6 +390,9 @@ int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
 	case KVM_ARM_VCPU_PMU_V3_CTRL:
 		ret = kvm_arm_pmu_v3_set_attr(vcpu, attr);
 		break;
+	case KVM_ARM_VCPU_TIMER_CTRL:
+		ret = kvm_arm_timer_set_attr(vcpu, attr);
+		break;
 	default:
 		ret = -ENXIO;
 		break;
@@ -407,6 +410,9 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
 	case KVM_ARM_VCPU_PMU_V3_CTRL:
 		ret = kvm_arm_pmu_v3_get_attr(vcpu, attr);
 		break;
+	case KVM_ARM_VCPU_TIMER_CTRL:
+		ret = kvm_arm_timer_get_attr(vcpu, attr);
+		break;
 	default:
 		ret = -ENXIO;
 		break;
@@ -424,6 +430,9 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
 	case KVM_ARM_VCPU_PMU_V3_CTRL:
 		ret = kvm_arm_pmu_v3_has_attr(vcpu, attr);
 		break;
+	case KVM_ARM_VCPU_TIMER_CTRL:
+		ret = kvm_arm_timer_has_attr(vcpu, attr);
+		break;
 	default:
 		ret = -ENXIO;
 		break;
diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index f1c967a..f0053f8 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -68,6 +68,10 @@ void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu);
 u64 kvm_arm_timer_get_reg(struct kvm_vcpu *, u64 regid);
 int kvm_arm_timer_set_reg(struct kvm_vcpu *, u64 regid, u64 value);
 
+int kvm_arm_timer_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr);
+int kvm_arm_timer_get_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr);
+int kvm_arm_timer_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr);
+
 bool kvm_timer_should_fire(struct arch_timer_context *timer_ctx);
 void kvm_timer_schedule(struct kvm_vcpu *vcpu);
 void kvm_timer_unschedule(struct kvm_vcpu *vcpu);
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 6c5a064..d3d1dce 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -21,6 +21,7 @@
 #include <linux/kvm_host.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
+#include <linux/uaccess.h>
 
 #include <clocksource/arm_arch_timer.h>
 #include <asm/arch_timer.h>
@@ -617,6 +618,28 @@ void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu)
 	kvm_vgic_unmap_phys_irq(vcpu, vtimer->irq.irq);
 }
 
+static bool timer_irqs_are_valid(struct kvm *kvm)
+{
+	struct kvm_vcpu *vcpu;
+	int vtimer_irq, ptimer_irq;
+	int i;
+
+	vcpu = kvm_get_vcpu(kvm, 0);
+	vtimer_irq = vcpu_vtimer(vcpu)->irq.irq;
+	ptimer_irq = vcpu_ptimer(vcpu)->irq.irq;
+
+	if (vtimer_irq == ptimer_irq)
+		return false;
+
+	kvm_for_each_vcpu(i, vcpu, kvm) {
+		if (vcpu_vtimer(vcpu)->irq.irq != vtimer_irq ||
+		    vcpu_ptimer(vcpu)->irq.irq != ptimer_irq)
+			return false;
+	}
+
+	return true;
+}
+
 int kvm_timer_enable(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
@@ -636,6 +659,11 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
 	if (!vgic_initialized(vcpu->kvm))
 		return -ENODEV;
 
+	if (!timer_irqs_are_valid(vcpu->kvm)) {
+		kvm_debug("incorrectly configured timer irqs\n");
+		return -EINVAL;
+	}
+
 	/*
 	 * Find the physical IRQ number corresponding to the host_vtimer_irq
 	 */
@@ -685,3 +713,79 @@ void kvm_timer_init_vhe(void)
 	val |= (CNTHCTL_EL1PCTEN << cnthctl_shift);
 	write_sysreg(val, cnthctl_el2);
 }
+
+static void set_timer_irqs(struct kvm *kvm, int vtimer_irq, int ptimer_irq)
+{
+	struct kvm_vcpu *vcpu;
+	int i;
+
+	kvm_for_each_vcpu(i, vcpu, kvm) {
+		vcpu_vtimer(vcpu)->irq.irq = vtimer_irq;
+		vcpu_ptimer(vcpu)->irq.irq = ptimer_irq;
+	}
+}
+
+int kvm_arm_timer_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
+{
+	int __user *uaddr = (int __user *)(long)attr->addr;
+	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
+	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
+	int irq;
+
+	if (!irqchip_in_kernel(vcpu->kvm))
+		return -EINVAL;
+
+	if (get_user(irq, uaddr))
+		return -EFAULT;
+
+	if (!(irq_is_ppi(irq)))
+		return -EINVAL;
+
+	if (vcpu->arch.timer_cpu.enabled)
+		return -EBUSY;
+
+	switch (attr->attr) {
+	case KVM_ARM_VCPU_TIMER_IRQ_VTIMER:
+		set_timer_irqs(vcpu->kvm, irq, ptimer->irq.irq);
+		break;
+	case KVM_ARM_VCPU_TIMER_IRQ_PTIMER:
+		set_timer_irqs(vcpu->kvm, vtimer->irq.irq, irq);
+		break;
+	default:
+		return -ENXIO;
+	}
+
+	return 0;
+}
+
+int kvm_arm_timer_get_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
+{
+	int __user *uaddr = (int __user *)(long)attr->addr;
+	struct arch_timer_context *timer;
+	int irq;
+
+	switch (attr->attr) {
+	case KVM_ARM_VCPU_TIMER_IRQ_VTIMER:
+		timer = vcpu_vtimer(vcpu);
+		break;
+	case KVM_ARM_VCPU_TIMER_IRQ_PTIMER:
+		timer = vcpu_ptimer(vcpu);
+		break;
+	default:
+		return -ENXIO;
+	}
+
+	irq = timer->irq.irq;
+	return put_user(irq, uaddr);
+}
+
+int kvm_arm_timer_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
+{
+	switch (attr->attr) {
+	case KVM_ARM_VCPU_TIMER_IRQ_VTIMER:
+	case KVM_ARM_VCPU_TIMER_IRQ_PTIMER:
+		return 0;
+	}
+
+	return -ENXIO;
+}
-- 
2.9.0

[PATCH 5/5] KVM: arm/arm64: Allow setting the timer IRQ numbers from userspace

[Marc Zyngier]

On 03/05/17 19:33, Christoffer Dall wrote:
> First we define an ABI using the vcpu devices that lets userspace set
> the interrupt numbers for the various timers on both the 32-bit and
> 64-bit KVM/ARM implementations.
> 
> Second, we add the definitions for the groups and attributes introduced
> by the above ABI.  (We add the PMU define on the 32-bit side as well for
> symmetry and it may get used some day.)
> 
> Third, we set up the arch-specific vcpu device operation handlers to
> call into the timer code for anything related to the
> KVM_ARM_VCPU_TIMER_CTRL group.
> 
> Fourth, we implement support for getting and setting the timer interrupt
> numbers using the above defined ABI in the arch timer code.
> 
> Fifth, we introduce error checking upon enabling the arch timer (which
> is called when first running a VCPU) to check that all VCPUs are
> configured to use the same PPI for the timer (as mandated by the
> architecture) and that the virtual and physical timers are not
> configured to use the same IRQ number.
> 
> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> ---
>  Documentation/virtual/kvm/devices/vcpu.txt |  25 +++++++
>  arch/arm/include/uapi/asm/kvm.h            |   8 +++
>  arch/arm/kvm/guest.c                       |   9 +++
>  arch/arm64/include/uapi/asm/kvm.h          |   3 +
>  arch/arm64/kvm/guest.c                     |   9 +++
>  include/kvm/arm_arch_timer.h               |   4 ++
>  virt/kvm/arm/arch_timer.c                  | 104 +++++++++++++++++++++++++++++
>  7 files changed, 162 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt
> index 352af6e..013e3f1 100644
> --- a/Documentation/virtual/kvm/devices/vcpu.txt
> +++ b/Documentation/virtual/kvm/devices/vcpu.txt
> @@ -35,3 +35,28 @@ Returns: -ENODEV: PMUv3 not supported or GIC not initialized
>  Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
>  virtual GIC implementation, this must be done after initializing the in-kernel
>  irqchip.
> +
> +
> +2. GROUP: KVM_ARM_VCPU_TIMER_CTRL
> +Architectures: ARM,ARM64
> +
> +2.1. ATTRIBUTE: KVM_ARM_VCPU_TIMER_IRQ_VTIMER
> +2.2. ATTRIBUTE: KVM_ARM_VCPU_TIMER_IRQ_PTIMER
> +Parameters: in kvm_device_attr.addr the address for the timer interrupt is a
> +            pointer to an int
> +Returns: -EINVAL: Invalid timer interrupt number
> +         -EBUSY:  One or more VCPUs has already run
> +
> +A value describing the architected timer interrupt number when connected to an
> +in-kernel virtual GIC.  These must be a PPI (16 <= intid < 32).  If an
> +attribute is not set, a default value is applied (see below).
> +
> +KVM_ARM_VCPU_TIMER_IRQ_VTIMER: The EL1 virtual timer intid (default: 27)
> +KVM_ARM_VCPU_TIMER_IRQ_PTIMER: The EL1 physical timer intid (default: 30)

uber nit: my reading of the code is that the default is set at VCPU
creation time. So setting the attribute overrides the default, not that
the default is applied if no attribute is set (i.e. there is always a
valid value).

> +
> +Setting the same PPI for different timers will prevent the VCPUs from running.
> +Setting the interrupt number on a VCPU configures all VCPUs created at that
> +time to use the number provided for a given timer, overwriting any previously
> +configured values on other VCPUs.  Userspace should configure the interrupt
> +numbers on at least one VCPU after creating all VCPUs and before running any
> +VCPUs.
> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> index a5838d6..6a75ec4 100644
> --- a/arch/arm/include/uapi/asm/kvm.h
> +++ b/arch/arm/include/uapi/asm/kvm.h
> @@ -200,6 +200,14 @@ struct kvm_arch_memory_slot {
>  #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x3ff
>  #define VGIC_LEVEL_INFO_LINE_LEVEL	0
>  
> +/* Device Control API on vcpu fd */
> +#define KVM_ARM_VCPU_PMU_V3_CTRL	0
> +#define   KVM_ARM_VCPU_PMU_V3_IRQ	0
> +#define   KVM_ARM_VCPU_PMU_V3_INIT	1
> +#define KVM_ARM_VCPU_TIMER_CTRL		1
> +#define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER		0
> +#define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER		1
> +
>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT    0
>  
>  /* KVM_IRQ_LINE irq field index values */
> diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
> index acea05e..1e0784e 100644
> --- a/arch/arm/kvm/guest.c
> +++ b/arch/arm/kvm/guest.c
> @@ -308,6 +308,9 @@ int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
>  	int ret;
>  
>  	switch (attr->group) {
> +	case KVM_ARM_VCPU_TIMER_CTRL:
> +		ret = kvm_arm_timer_set_attr(vcpu, attr);
> +		break;
>  	default:
>  		ret = -ENXIO;
>  		break;
> @@ -322,6 +325,9 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
>  	int ret;
>  
>  	switch (attr->group) {
> +	case KVM_ARM_VCPU_TIMER_CTRL:
> +		ret = kvm_arm_timer_get_attr(vcpu, attr);
> +		break;
>  	default:
>  		ret = -ENXIO;
>  		break;
> @@ -336,6 +342,9 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
>  	int ret;
>  
>  	switch (attr->group) {
> +	case KVM_ARM_VCPU_TIMER_CTRL:
> +		ret = kvm_arm_timer_has_attr(vcpu, attr);
> +		break;
>  	default:
>  		ret = -ENXIO;
>  		break;
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index cd6bea4..fc64518 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -226,6 +226,9 @@ struct kvm_arch_memory_slot {
>  #define KVM_ARM_VCPU_PMU_V3_CTRL	0
>  #define   KVM_ARM_VCPU_PMU_V3_IRQ	0
>  #define   KVM_ARM_VCPU_PMU_V3_INIT	1
> +#define KVM_ARM_VCPU_TIMER_CTRL		1
> +#define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER		0
> +#define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER		1
>  
>  /* KVM_IRQ_LINE irq field index values */
>  #define KVM_ARM_IRQ_TYPE_SHIFT		24
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index b37446a..5c7f657 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -390,6 +390,9 @@ int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
>  	case KVM_ARM_VCPU_PMU_V3_CTRL:
>  		ret = kvm_arm_pmu_v3_set_attr(vcpu, attr);
>  		break;
> +	case KVM_ARM_VCPU_TIMER_CTRL:
> +		ret = kvm_arm_timer_set_attr(vcpu, attr);
> +		break;
>  	default:
>  		ret = -ENXIO;
>  		break;
> @@ -407,6 +410,9 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
>  	case KVM_ARM_VCPU_PMU_V3_CTRL:
>  		ret = kvm_arm_pmu_v3_get_attr(vcpu, attr);
>  		break;
> +	case KVM_ARM_VCPU_TIMER_CTRL:
> +		ret = kvm_arm_timer_get_attr(vcpu, attr);
> +		break;
>  	default:
>  		ret = -ENXIO;
>  		break;
> @@ -424,6 +430,9 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
>  	case KVM_ARM_VCPU_PMU_V3_CTRL:
>  		ret = kvm_arm_pmu_v3_has_attr(vcpu, attr);
>  		break;
> +	case KVM_ARM_VCPU_TIMER_CTRL:
> +		ret = kvm_arm_timer_has_attr(vcpu, attr);
> +		break;
>  	default:
>  		ret = -ENXIO;
>  		break;
> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> index f1c967a..f0053f8 100644
> --- a/include/kvm/arm_arch_timer.h
> +++ b/include/kvm/arm_arch_timer.h
> @@ -68,6 +68,10 @@ void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu);
>  u64 kvm_arm_timer_get_reg(struct kvm_vcpu *, u64 regid);
>  int kvm_arm_timer_set_reg(struct kvm_vcpu *, u64 regid, u64 value);
>  
> +int kvm_arm_timer_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr);
> +int kvm_arm_timer_get_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr);
> +int kvm_arm_timer_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr);
> +
>  bool kvm_timer_should_fire(struct arch_timer_context *timer_ctx);
>  void kvm_timer_schedule(struct kvm_vcpu *vcpu);
>  void kvm_timer_unschedule(struct kvm_vcpu *vcpu);
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 6c5a064..d3d1dce 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -21,6 +21,7 @@
>  #include <linux/kvm_host.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
> +#include <linux/uaccess.h>
>  
>  #include <clocksource/arm_arch_timer.h>
>  #include <asm/arch_timer.h>
> @@ -617,6 +618,28 @@ void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu)
>  	kvm_vgic_unmap_phys_irq(vcpu, vtimer->irq.irq);
>  }
>  
> +static bool timer_irqs_are_valid(struct kvm *kvm)
> +{
> +	struct kvm_vcpu *vcpu;
> +	int vtimer_irq, ptimer_irq;
> +	int i;
> +
> +	vcpu = kvm_get_vcpu(kvm, 0);
> +	vtimer_irq = vcpu_vtimer(vcpu)->irq.irq;
> +	ptimer_irq = vcpu_ptimer(vcpu)->irq.irq;
> +
> +	if (vtimer_irq == ptimer_irq)
> +		return false;
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		if (vcpu_vtimer(vcpu)->irq.irq != vtimer_irq ||
> +		    vcpu_ptimer(vcpu)->irq.irq != ptimer_irq)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  int kvm_timer_enable(struct kvm_vcpu *vcpu)
>  {
>  	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> @@ -636,6 +659,11 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>  	if (!vgic_initialized(vcpu->kvm))
>  		return -ENODEV;
>  
> +	if (!timer_irqs_are_valid(vcpu->kvm)) {
> +		kvm_debug("incorrectly configured timer irqs\n");
> +		return -EINVAL;
> +	}
> +
>  	/*
>  	 * Find the physical IRQ number corresponding to the host_vtimer_irq
>  	 */
> @@ -685,3 +713,79 @@ void kvm_timer_init_vhe(void)
>  	val |= (CNTHCTL_EL1PCTEN << cnthctl_shift);
>  	write_sysreg(val, cnthctl_el2);
>  }
> +
> +static void set_timer_irqs(struct kvm *kvm, int vtimer_irq, int ptimer_irq)
> +{
> +	struct kvm_vcpu *vcpu;
> +	int i;
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		vcpu_vtimer(vcpu)->irq.irq = vtimer_irq;
> +		vcpu_ptimer(vcpu)->irq.irq = ptimer_irq;
> +	}
> +}
> +
> +int kvm_arm_timer_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
> +{
> +	int __user *uaddr = (int __user *)(long)attr->addr;
> +	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
> +	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
> +	int irq;
> +
> +	if (!irqchip_in_kernel(vcpu->kvm))
> +		return -EINVAL;
> +
> +	if (get_user(irq, uaddr))
> +		return -EFAULT;
> +
> +	if (!(irq_is_ppi(irq)))
> +		return -EINVAL;
> +
> +	if (vcpu->arch.timer_cpu.enabled)
> +		return -EBUSY;
> +
> +	switch (attr->attr) {
> +	case KVM_ARM_VCPU_TIMER_IRQ_VTIMER:
> +		set_timer_irqs(vcpu->kvm, irq, ptimer->irq.irq);

Could we move the (vtimer_irq == ptimer_irq) check here? It is probably
better to fail early than to wait until the first run to discover that
something was wrong... Or does that clash horribly with the default values?

Another issue that just popped in my head: how do we ensure that we
don't clash between the PMU and the timers? Yes, that's a foolish thing
to do, but that's no different from avoiding the same interrupt on the
timers...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 5/5] KVM: arm/arm64: Allow setting the timer IRQ numbers from userspace

[Christoffer Dall]

On Thu, May 04, 2017 at 10:34:32AM +0100, Marc Zyngier wrote:
> On 03/05/17 19:33, Christoffer Dall wrote:
> > First we define an ABI using the vcpu devices that lets userspace set
> > the interrupt numbers for the various timers on both the 32-bit and
> > 64-bit KVM/ARM implementations.
> > 
> > Second, we add the definitions for the groups and attributes introduced
> > by the above ABI.  (We add the PMU define on the 32-bit side as well for
> > symmetry and it may get used some day.)
> > 
> > Third, we set up the arch-specific vcpu device operation handlers to
> > call into the timer code for anything related to the
> > KVM_ARM_VCPU_TIMER_CTRL group.
> > 
> > Fourth, we implement support for getting and setting the timer interrupt
> > numbers using the above defined ABI in the arch timer code.
> > 
> > Fifth, we introduce error checking upon enabling the arch timer (which
> > is called when first running a VCPU) to check that all VCPUs are
> > configured to use the same PPI for the timer (as mandated by the
> > architecture) and that the virtual and physical timers are not
> > configured to use the same IRQ number.
> > 
> > Signed-off-by: Christoffer Dall <cdall@linaro.org>
> > ---
> >  Documentation/virtual/kvm/devices/vcpu.txt |  25 +++++++
> >  arch/arm/include/uapi/asm/kvm.h            |   8 +++
> >  arch/arm/kvm/guest.c                       |   9 +++
> >  arch/arm64/include/uapi/asm/kvm.h          |   3 +
> >  arch/arm64/kvm/guest.c                     |   9 +++
> >  include/kvm/arm_arch_timer.h               |   4 ++
> >  virt/kvm/arm/arch_timer.c                  | 104 +++++++++++++++++++++++++++++
> >  7 files changed, 162 insertions(+)
> > 
> > diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt
> > index 352af6e..013e3f1 100644
> > --- a/Documentation/virtual/kvm/devices/vcpu.txt
> > +++ b/Documentation/virtual/kvm/devices/vcpu.txt
> > @@ -35,3 +35,28 @@ Returns: -ENODEV: PMUv3 not supported or GIC not initialized
> >  Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
> >  virtual GIC implementation, this must be done after initializing the in-kernel
> >  irqchip.
> > +
> > +
> > +2. GROUP: KVM_ARM_VCPU_TIMER_CTRL
> > +Architectures: ARM,ARM64
> > +
> > +2.1. ATTRIBUTE: KVM_ARM_VCPU_TIMER_IRQ_VTIMER
> > +2.2. ATTRIBUTE: KVM_ARM_VCPU_TIMER_IRQ_PTIMER
> > +Parameters: in kvm_device_attr.addr the address for the timer interrupt is a
> > +            pointer to an int
> > +Returns: -EINVAL: Invalid timer interrupt number
> > +         -EBUSY:  One or more VCPUs has already run
> > +
> > +A value describing the architected timer interrupt number when connected to an
> > +in-kernel virtual GIC.  These must be a PPI (16 <= intid < 32).  If an
> > +attribute is not set, a default value is applied (see below).
> > +
> > +KVM_ARM_VCPU_TIMER_IRQ_VTIMER: The EL1 virtual timer intid (default: 27)
> > +KVM_ARM_VCPU_TIMER_IRQ_PTIMER: The EL1 physical timer intid (default: 30)
> 
> uber nit: my reading of the code is that the default is set at VCPU
> creation time. So setting the attribute overrides the default, not that
> the default is applied if no attribute is set (i.e. there is always a
> valid value).
> 

uh, right, I don't see the distinction though, so not sure how to
correct the text.

Would "Setting the attribute overrides the default values (see below)."
work instead?

> > +
> > +Setting the same PPI for different timers will prevent the VCPUs from running.
> > +Setting the interrupt number on a VCPU configures all VCPUs created at that
> > +time to use the number provided for a given timer, overwriting any previously
> > +configured values on other VCPUs.  Userspace should configure the interrupt
> > +numbers on at least one VCPU after creating all VCPUs and before running any
> > +VCPUs.
> > diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> > index a5838d6..6a75ec4 100644
> > --- a/arch/arm/include/uapi/asm/kvm.h
> > +++ b/arch/arm/include/uapi/asm/kvm.h
> > @@ -200,6 +200,14 @@ struct kvm_arch_memory_slot {
> >  #define KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK 0x3ff
> >  #define VGIC_LEVEL_INFO_LINE_LEVEL	0
> >  
> > +/* Device Control API on vcpu fd */
> > +#define KVM_ARM_VCPU_PMU_V3_CTRL	0
> > +#define   KVM_ARM_VCPU_PMU_V3_IRQ	0
> > +#define   KVM_ARM_VCPU_PMU_V3_INIT	1
> > +#define KVM_ARM_VCPU_TIMER_CTRL		1
> > +#define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER		0
> > +#define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER		1
> > +
> >  #define   KVM_DEV_ARM_VGIC_CTRL_INIT    0
> >  
> >  /* KVM_IRQ_LINE irq field index values */
> > diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
> > index acea05e..1e0784e 100644
> > --- a/arch/arm/kvm/guest.c
> > +++ b/arch/arm/kvm/guest.c
> > @@ -308,6 +308,9 @@ int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
> >  	int ret;
> >  
> >  	switch (attr->group) {
> > +	case KVM_ARM_VCPU_TIMER_CTRL:
> > +		ret = kvm_arm_timer_set_attr(vcpu, attr);
> > +		break;
> >  	default:
> >  		ret = -ENXIO;
> >  		break;
> > @@ -322,6 +325,9 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
> >  	int ret;
> >  
> >  	switch (attr->group) {
> > +	case KVM_ARM_VCPU_TIMER_CTRL:
> > +		ret = kvm_arm_timer_get_attr(vcpu, attr);
> > +		break;
> >  	default:
> >  		ret = -ENXIO;
> >  		break;
> > @@ -336,6 +342,9 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
> >  	int ret;
> >  
> >  	switch (attr->group) {
> > +	case KVM_ARM_VCPU_TIMER_CTRL:
> > +		ret = kvm_arm_timer_has_attr(vcpu, attr);
> > +		break;
> >  	default:
> >  		ret = -ENXIO;
> >  		break;
> > diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> > index cd6bea4..fc64518 100644
> > --- a/arch/arm64/include/uapi/asm/kvm.h
> > +++ b/arch/arm64/include/uapi/asm/kvm.h
> > @@ -226,6 +226,9 @@ struct kvm_arch_memory_slot {
> >  #define KVM_ARM_VCPU_PMU_V3_CTRL	0
> >  #define   KVM_ARM_VCPU_PMU_V3_IRQ	0
> >  #define   KVM_ARM_VCPU_PMU_V3_INIT	1
> > +#define KVM_ARM_VCPU_TIMER_CTRL		1
> > +#define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER		0
> > +#define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER		1
> >  
> >  /* KVM_IRQ_LINE irq field index values */
> >  #define KVM_ARM_IRQ_TYPE_SHIFT		24
> > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> > index b37446a..5c7f657 100644
> > --- a/arch/arm64/kvm/guest.c
> > +++ b/arch/arm64/kvm/guest.c
> > @@ -390,6 +390,9 @@ int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
> >  	case KVM_ARM_VCPU_PMU_V3_CTRL:
> >  		ret = kvm_arm_pmu_v3_set_attr(vcpu, attr);
> >  		break;
> > +	case KVM_ARM_VCPU_TIMER_CTRL:
> > +		ret = kvm_arm_timer_set_attr(vcpu, attr);
> > +		break;
> >  	default:
> >  		ret = -ENXIO;
> >  		break;
> > @@ -407,6 +410,9 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
> >  	case KVM_ARM_VCPU_PMU_V3_CTRL:
> >  		ret = kvm_arm_pmu_v3_get_attr(vcpu, attr);
> >  		break;
> > +	case KVM_ARM_VCPU_TIMER_CTRL:
> > +		ret = kvm_arm_timer_get_attr(vcpu, attr);
> > +		break;
> >  	default:
> >  		ret = -ENXIO;
> >  		break;
> > @@ -424,6 +430,9 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
> >  	case KVM_ARM_VCPU_PMU_V3_CTRL:
> >  		ret = kvm_arm_pmu_v3_has_attr(vcpu, attr);
> >  		break;
> > +	case KVM_ARM_VCPU_TIMER_CTRL:
> > +		ret = kvm_arm_timer_has_attr(vcpu, attr);
> > +		break;
> >  	default:
> >  		ret = -ENXIO;
> >  		break;
> > diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> > index f1c967a..f0053f8 100644
> > --- a/include/kvm/arm_arch_timer.h
> > +++ b/include/kvm/arm_arch_timer.h
> > @@ -68,6 +68,10 @@ void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu);
> >  u64 kvm_arm_timer_get_reg(struct kvm_vcpu *, u64 regid);
> >  int kvm_arm_timer_set_reg(struct kvm_vcpu *, u64 regid, u64 value);
> >  
> > +int kvm_arm_timer_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr);
> > +int kvm_arm_timer_get_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr);
> > +int kvm_arm_timer_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr);
> > +
> >  bool kvm_timer_should_fire(struct arch_timer_context *timer_ctx);
> >  void kvm_timer_schedule(struct kvm_vcpu *vcpu);
> >  void kvm_timer_unschedule(struct kvm_vcpu *vcpu);
> > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> > index 6c5a064..d3d1dce 100644
> > --- a/virt/kvm/arm/arch_timer.c
> > +++ b/virt/kvm/arm/arch_timer.c
> > @@ -21,6 +21,7 @@
> >  #include <linux/kvm_host.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/irq.h>
> > +#include <linux/uaccess.h>
> >  
> >  #include <clocksource/arm_arch_timer.h>
> >  #include <asm/arch_timer.h>
> > @@ -617,6 +618,28 @@ void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu)
> >  	kvm_vgic_unmap_phys_irq(vcpu, vtimer->irq.irq);
> >  }
> >  
> > +static bool timer_irqs_are_valid(struct kvm *kvm)
> > +{
> > +	struct kvm_vcpu *vcpu;
> > +	int vtimer_irq, ptimer_irq;
> > +	int i;
> > +
> > +	vcpu = kvm_get_vcpu(kvm, 0);
> > +	vtimer_irq = vcpu_vtimer(vcpu)->irq.irq;
> > +	ptimer_irq = vcpu_ptimer(vcpu)->irq.irq;
> > +
> > +	if (vtimer_irq == ptimer_irq)
> > +		return false;
> > +
> > +	kvm_for_each_vcpu(i, vcpu, kvm) {
> > +		if (vcpu_vtimer(vcpu)->irq.irq != vtimer_irq ||
> > +		    vcpu_ptimer(vcpu)->irq.irq != ptimer_irq)
> > +			return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> >  int kvm_timer_enable(struct kvm_vcpu *vcpu)
> >  {
> >  	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
> > @@ -636,6 +659,11 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
> >  	if (!vgic_initialized(vcpu->kvm))
> >  		return -ENODEV;
> >  
> > +	if (!timer_irqs_are_valid(vcpu->kvm)) {
> > +		kvm_debug("incorrectly configured timer irqs\n");
> > +		return -EINVAL;
> > +	}
> > +
> >  	/*
> >  	 * Find the physical IRQ number corresponding to the host_vtimer_irq
> >  	 */
> > @@ -685,3 +713,79 @@ void kvm_timer_init_vhe(void)
> >  	val |= (CNTHCTL_EL1PCTEN << cnthctl_shift);
> >  	write_sysreg(val, cnthctl_el2);
> >  }
> > +
> > +static void set_timer_irqs(struct kvm *kvm, int vtimer_irq, int ptimer_irq)
> > +{
> > +	struct kvm_vcpu *vcpu;
> > +	int i;
> > +
> > +	kvm_for_each_vcpu(i, vcpu, kvm) {
> > +		vcpu_vtimer(vcpu)->irq.irq = vtimer_irq;
> > +		vcpu_ptimer(vcpu)->irq.irq = ptimer_irq;
> > +	}
> > +}
> > +
> > +int kvm_arm_timer_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
> > +{
> > +	int __user *uaddr = (int __user *)(long)attr->addr;
> > +	struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
> > +	struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
> > +	int irq;
> > +
> > +	if (!irqchip_in_kernel(vcpu->kvm))
> > +		return -EINVAL;
> > +
> > +	if (get_user(irq, uaddr))
> > +		return -EFAULT;
> > +
> > +	if (!(irq_is_ppi(irq)))
> > +		return -EINVAL;
> > +
> > +	if (vcpu->arch.timer_cpu.enabled)
> > +		return -EBUSY;
> > +
> > +	switch (attr->attr) {
> > +	case KVM_ARM_VCPU_TIMER_IRQ_VTIMER:
> > +		set_timer_irqs(vcpu->kvm, irq, ptimer->irq.irq);
> 
> Could we move the (vtimer_irq == ptimer_irq) check here? It is probably
> better to fail early than to wait until the first run to discover that
> something was wrong... Or does that clash horribly with the default values?

It clashes horribly with the default value.  I tried it and gave up.  I
don't think it's reasonable to require userspace to set stuff to a
non-default value first etc., and I experimented with initializing the
irq numbers to 0 and then check stuff later, but it became a huge mess
basically.

> 
> Another issue that just popped in my head: how do we ensure that we
> don't clash between the PMU and the timers? Yes, that's a foolish thing
> to do, but that's no different from avoiding the same interrupt on the
> timers...
> 
Argh, good point.

So actually, nothing *really bad* happens from using the same IRQ number
except that the VM gets confused.  However, it's living life dangerously
because we use the HW bit for the vtimer, so we if ever start using it
for other timers or the PMU, then you could end up in a situation where
you unmap the phys mapping on behalf of another device, and the physical
interrupt could be left in an active state.

On the other hand, the vtimer currently belongs only to VMs and we set
the required physical active state before entering the VM, so maybe it
doesn't matter.

Should we just forego these checks and let the user shoot itself in the
foot if he/she wants to?

Thanks,
-Christoffer

[PATCH 5/5] KVM: arm/arm64: Allow setting the timer IRQ numbers from userspace

[Marc Zyngier]

On 04/05/17 10:59, Christoffer Dall wrote:
> On Thu, May 04, 2017 at 10:34:32AM +0100, Marc Zyngier wrote:
>> On 03/05/17 19:33, Christoffer Dall wrote:
>>> First we define an ABI using the vcpu devices that lets userspace set
>>> the interrupt numbers for the various timers on both the 32-bit and
>>> 64-bit KVM/ARM implementations.
>>>
>>> Second, we add the definitions for the groups and attributes introduced
>>> by the above ABI.  (We add the PMU define on the 32-bit side as well for
>>> symmetry and it may get used some day.)
>>>
>>> Third, we set up the arch-specific vcpu device operation handlers to
>>> call into the timer code for anything related to the
>>> KVM_ARM_VCPU_TIMER_CTRL group.
>>>
>>> Fourth, we implement support for getting and setting the timer interrupt
>>> numbers using the above defined ABI in the arch timer code.
>>>
>>> Fifth, we introduce error checking upon enabling the arch timer (which
>>> is called when first running a VCPU) to check that all VCPUs are
>>> configured to use the same PPI for the timer (as mandated by the
>>> architecture) and that the virtual and physical timers are not
>>> configured to use the same IRQ number.
>>>
>>> Signed-off-by: Christoffer Dall <cdall@linaro.org>
>>> ---
>>>  Documentation/virtual/kvm/devices/vcpu.txt |  25 +++++++
>>>  arch/arm/include/uapi/asm/kvm.h            |   8 +++
>>>  arch/arm/kvm/guest.c                       |   9 +++
>>>  arch/arm64/include/uapi/asm/kvm.h          |   3 +
>>>  arch/arm64/kvm/guest.c                     |   9 +++
>>>  include/kvm/arm_arch_timer.h               |   4 ++
>>>  virt/kvm/arm/arch_timer.c                  | 104 +++++++++++++++++++++++++++++
>>>  7 files changed, 162 insertions(+)
>>>
>>> diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt
>>> index 352af6e..013e3f1 100644
>>> --- a/Documentation/virtual/kvm/devices/vcpu.txt
>>> +++ b/Documentation/virtual/kvm/devices/vcpu.txt
>>> @@ -35,3 +35,28 @@ Returns: -ENODEV: PMUv3 not supported or GIC not initialized
>>>  Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
>>>  virtual GIC implementation, this must be done after initializing the in-kernel
>>>  irqchip.
>>> +
>>> +
>>> +2. GROUP: KVM_ARM_VCPU_TIMER_CTRL
>>> +Architectures: ARM,ARM64
>>> +
>>> +2.1. ATTRIBUTE: KVM_ARM_VCPU_TIMER_IRQ_VTIMER
>>> +2.2. ATTRIBUTE: KVM_ARM_VCPU_TIMER_IRQ_PTIMER
>>> +Parameters: in kvm_device_attr.addr the address for the timer interrupt is a
>>> +            pointer to an int
>>> +Returns: -EINVAL: Invalid timer interrupt number
>>> +         -EBUSY:  One or more VCPUs has already run
>>> +
>>> +A value describing the architected timer interrupt number when connected to an
>>> +in-kernel virtual GIC.  These must be a PPI (16 <= intid < 32).  If an
>>> +attribute is not set, a default value is applied (see below).
>>> +
>>> +KVM_ARM_VCPU_TIMER_IRQ_VTIMER: The EL1 virtual timer intid (default: 27)
>>> +KVM_ARM_VCPU_TIMER_IRQ_PTIMER: The EL1 physical timer intid (default: 30)
>>
>> uber nit: my reading of the code is that the default is set at VCPU
>> creation time. So setting the attribute overrides the default, not that
>> the default is applied if no attribute is set (i.e. there is always a
>> valid value).
>>
> 
> uh, right, I don't see the distinction though, so not sure how to
> correct the text.
> 
> Would "Setting the attribute overrides the default values (see below)."
> work instead?

Looks good to me.

[...]

>>
>> Another issue that just popped in my head: how do we ensure that we
>> don't clash between the PMU and the timers? Yes, that's a foolish thing
>> to do, but that's no different from avoiding the same interrupt on the
>> timers...
>>
> Argh, good point.
> 
> So actually, nothing *really bad* happens from using the same IRQ number
> except that the VM gets confused.  However, it's living life dangerously
> because we use the HW bit for the vtimer, so we if ever start using it
> for other timers or the PMU, then you could end up in a situation where
> you unmap the phys mapping on behalf of another device, and the physical
> interrupt could be left in an active state.
> 
> On the other hand, the vtimer currently belongs only to VMs and we set
> the required physical active state before entering the VM, so maybe it
> doesn't matter.

So far, we always assume that there is never more than a single source
per interrupt. We'll end-up with weird behaviours because our line_level
field is not an OR of the various inputs, but a direct assignment
(device A and B raise the line, then A drops it -> B's interrupt is gone).

I think that only the guest will be confused by it, but accepting it may
be interpreted as "this should work correctly". Would documenting that
it is a bad idea be enough?

> Should we just forego these checks and let the user shoot itself in the
> foot if he/she wants to?

If the documentation is enough, why not. Otherwise, we need some form of
allocator. Boring. ;-)

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 5/5] KVM: arm/arm64: Allow setting the timer IRQ numbers from userspace

[Christoffer Dall]

On Thu, May 04, 2017 at 11:54:06AM +0100, Marc Zyngier wrote:
> On 04/05/17 10:59, Christoffer Dall wrote:
> > On Thu, May 04, 2017 at 10:34:32AM +0100, Marc Zyngier wrote:
> >> On 03/05/17 19:33, Christoffer Dall wrote:
> >>> First we define an ABI using the vcpu devices that lets userspace set
> >>> the interrupt numbers for the various timers on both the 32-bit and
> >>> 64-bit KVM/ARM implementations.
> >>>
> >>> Second, we add the definitions for the groups and attributes introduced
> >>> by the above ABI.  (We add the PMU define on the 32-bit side as well for
> >>> symmetry and it may get used some day.)
> >>>
> >>> Third, we set up the arch-specific vcpu device operation handlers to
> >>> call into the timer code for anything related to the
> >>> KVM_ARM_VCPU_TIMER_CTRL group.
> >>>
> >>> Fourth, we implement support for getting and setting the timer interrupt
> >>> numbers using the above defined ABI in the arch timer code.
> >>>
> >>> Fifth, we introduce error checking upon enabling the arch timer (which
> >>> is called when first running a VCPU) to check that all VCPUs are
> >>> configured to use the same PPI for the timer (as mandated by the
> >>> architecture) and that the virtual and physical timers are not
> >>> configured to use the same IRQ number.
> >>>
> >>> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> >>> ---
> >>>  Documentation/virtual/kvm/devices/vcpu.txt |  25 +++++++
> >>>  arch/arm/include/uapi/asm/kvm.h            |   8 +++
> >>>  arch/arm/kvm/guest.c                       |   9 +++
> >>>  arch/arm64/include/uapi/asm/kvm.h          |   3 +
> >>>  arch/arm64/kvm/guest.c                     |   9 +++
> >>>  include/kvm/arm_arch_timer.h               |   4 ++
> >>>  virt/kvm/arm/arch_timer.c                  | 104 +++++++++++++++++++++++++++++
> >>>  7 files changed, 162 insertions(+)
> >>>
> >>> diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt
> >>> index 352af6e..013e3f1 100644
> >>> --- a/Documentation/virtual/kvm/devices/vcpu.txt
> >>> +++ b/Documentation/virtual/kvm/devices/vcpu.txt
> >>> @@ -35,3 +35,28 @@ Returns: -ENODEV: PMUv3 not supported or GIC not initialized
> >>>  Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
> >>>  virtual GIC implementation, this must be done after initializing the in-kernel
> >>>  irqchip.
> >>> +
> >>> +
> >>> +2. GROUP: KVM_ARM_VCPU_TIMER_CTRL
> >>> +Architectures: ARM,ARM64
> >>> +
> >>> +2.1. ATTRIBUTE: KVM_ARM_VCPU_TIMER_IRQ_VTIMER
> >>> +2.2. ATTRIBUTE: KVM_ARM_VCPU_TIMER_IRQ_PTIMER
> >>> +Parameters: in kvm_device_attr.addr the address for the timer interrupt is a
> >>> +            pointer to an int
> >>> +Returns: -EINVAL: Invalid timer interrupt number
> >>> +         -EBUSY:  One or more VCPUs has already run
> >>> +
> >>> +A value describing the architected timer interrupt number when connected to an
> >>> +in-kernel virtual GIC.  These must be a PPI (16 <= intid < 32).  If an
> >>> +attribute is not set, a default value is applied (see below).
> >>> +
> >>> +KVM_ARM_VCPU_TIMER_IRQ_VTIMER: The EL1 virtual timer intid (default: 27)
> >>> +KVM_ARM_VCPU_TIMER_IRQ_PTIMER: The EL1 physical timer intid (default: 30)
> >>
> >> uber nit: my reading of the code is that the default is set at VCPU
> >> creation time. So setting the attribute overrides the default, not that
> >> the default is applied if no attribute is set (i.e. there is always a
> >> valid value).
> >>
> > 
> > uh, right, I don't see the distinction though, so not sure how to
> > correct the text.
> > 
> > Would "Setting the attribute overrides the default values (see below)."
> > work instead?
> 
> Looks good to me.
> 
> [...]
> 
> >>
> >> Another issue that just popped in my head: how do we ensure that we
> >> don't clash between the PMU and the timers? Yes, that's a foolish thing
> >> to do, but that's no different from avoiding the same interrupt on the
> >> timers...
> >>
> > Argh, good point.
> > 
> > So actually, nothing *really bad* happens from using the same IRQ number
> > except that the VM gets confused.  However, it's living life dangerously
> > because we use the HW bit for the vtimer, so we if ever start using it
> > for other timers or the PMU, then you could end up in a situation where
> > you unmap the phys mapping on behalf of another device, and the physical
> > interrupt could be left in an active state.
> > 
> > On the other hand, the vtimer currently belongs only to VMs and we set
> > the required physical active state before entering the VM, so maybe it
> > doesn't matter.
> 
> So far, we always assume that there is never more than a single source
> per interrupt. We'll end-up with weird behaviours because our line_level
> field is not an OR of the various inputs, but a direct assignment
> (device A and B raise the line, then A drops it -> B's interrupt is gone).
> 
> I think that only the guest will be confused by it, but accepting it may
> be interpreted as "this should work correctly". Would documenting that
> it is a bad idea be enough?
> 
> > Should we just forego these checks and let the user shoot itself in the
> > foot if he/she wants to?
> 
> If the documentation is enough, why not. Otherwise, we need some form of
> allocator. Boring. ;-)
> 

Well, it's not that bad really (untested):

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 1541f5d..122f9d3 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -121,6 +121,9 @@ struct vgic_irq {
 	u8 source;			/* GICv2 SGIs only */
 	u8 priority;
 	enum vgic_irq_config config;	/* Level or edge */
+
+	void *owner;			/* Opaque pointer to reserve an interrupt
+					   for in-kernel devices. */
 };
 
 struct vgic_register_region;
diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
index 3d0979c..7561d2d 100644
--- a/virt/kvm/arm/vgic/vgic.c
+++ b/virt/kvm/arm/vgic/vgic.c
@@ -429,6 +429,42 @@ int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq)
 }
 
 /**
+ * kvm_vgic_set_owner - Set the owner of an interrupt for a VM
+ *
+ * @kvm:    Pointer to the VM structure
+ * @intid:  The virtual INTID identifying the interrupt (PPI or SPI)
+ * @owner:  Opaque pointer to the owner
+ *
+ * Returns 0 if intid is not already used by another in-kernel device and the
+ * owner is set, otherwise returns an error code.
+ *
+ * We only set the owner for VCPU 0 for PPIs.
+ */
+int kvm_vgic_set_owner(struct kvm *kvm, unsigned int intid, void *owner)
+{
+	struct vgic_irq *irq;
+	struct kvm_vcpu *vcpu = kvm_get_vcpu(kvm, 0);
+	int ret = 0;
+
+	if (!vgic_initialized(kvm))
+		return -EAGAIN;
+
+	/* SGIs and LPIs cannot be wired up to any device */
+	if (!irq_is_ppi(intid) && !vgic_valid_spi(kvm, intid))
+		return -EINVAL;
+
+	irq = vgic_get_irq(kvm, vcpu, intid);
+	spin_lock(&irq->irq_lock);
+	if (irq->owner && irq->owner != owner)
+		ret = -EEXIST;
+	else
+		irq->owner = owner;
+	spin_unlock(&irq->irq_lock);
+
+	return ret;
+}
+
+/**
  * vgic_prune_ap_list - Remove non-relevant interrupts from the list
  *
  * @vcpu: The VCPU pointer

The problem is that it doesn't help that much, because userspace can
still change the level of an IRQ which is connected to an in-kernel
device, unless we also introduce checking the owner field in the
injection path.

I don't see it blowing up the host with/without an allocator, so I'm
fine with documentation, but I can also include the above.

Thoughts?

Thanks,
-Christoffer

[PATCH 5/5] KVM: arm/arm64: Allow setting the timer IRQ numbers from userspace

[Christoffer Dall]

Hi Marc,

On Thu, May 04, 2017 at 01:22:25PM +0200, Christoffer Dall wrote:
> On Thu, May 04, 2017 at 11:54:06AM +0100, Marc Zyngier wrote:
> > On 04/05/17 10:59, Christoffer Dall wrote:
> > > On Thu, May 04, 2017 at 10:34:32AM +0100, Marc Zyngier wrote:
> > >> On 03/05/17 19:33, Christoffer Dall wrote:
> > >>> First we define an ABI using the vcpu devices that lets userspace set
> > >>> the interrupt numbers for the various timers on both the 32-bit and
> > >>> 64-bit KVM/ARM implementations.
> > >>>
> > >>> Second, we add the definitions for the groups and attributes introduced
> > >>> by the above ABI.  (We add the PMU define on the 32-bit side as well for
> > >>> symmetry and it may get used some day.)
> > >>>
> > >>> Third, we set up the arch-specific vcpu device operation handlers to
> > >>> call into the timer code for anything related to the
> > >>> KVM_ARM_VCPU_TIMER_CTRL group.
> > >>>
> > >>> Fourth, we implement support for getting and setting the timer interrupt
> > >>> numbers using the above defined ABI in the arch timer code.
> > >>>
> > >>> Fifth, we introduce error checking upon enabling the arch timer (which
> > >>> is called when first running a VCPU) to check that all VCPUs are
> > >>> configured to use the same PPI for the timer (as mandated by the
> > >>> architecture) and that the virtual and physical timers are not
> > >>> configured to use the same IRQ number.
> > >>>
> > >>> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> > >>> ---
> > >>>  Documentation/virtual/kvm/devices/vcpu.txt |  25 +++++++
> > >>>  arch/arm/include/uapi/asm/kvm.h            |   8 +++
> > >>>  arch/arm/kvm/guest.c                       |   9 +++
> > >>>  arch/arm64/include/uapi/asm/kvm.h          |   3 +
> > >>>  arch/arm64/kvm/guest.c                     |   9 +++
> > >>>  include/kvm/arm_arch_timer.h               |   4 ++
> > >>>  virt/kvm/arm/arch_timer.c                  | 104 +++++++++++++++++++++++++++++
> > >>>  7 files changed, 162 insertions(+)
> > >>>
> > >>> diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt
> > >>> index 352af6e..013e3f1 100644
> > >>> --- a/Documentation/virtual/kvm/devices/vcpu.txt
> > >>> +++ b/Documentation/virtual/kvm/devices/vcpu.txt
> > >>> @@ -35,3 +35,28 @@ Returns: -ENODEV: PMUv3 not supported or GIC not initialized
> > >>>  Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
> > >>>  virtual GIC implementation, this must be done after initializing the in-kernel
> > >>>  irqchip.
> > >>> +
> > >>> +
> > >>> +2. GROUP: KVM_ARM_VCPU_TIMER_CTRL
> > >>> +Architectures: ARM,ARM64
> > >>> +
> > >>> +2.1. ATTRIBUTE: KVM_ARM_VCPU_TIMER_IRQ_VTIMER
> > >>> +2.2. ATTRIBUTE: KVM_ARM_VCPU_TIMER_IRQ_PTIMER
> > >>> +Parameters: in kvm_device_attr.addr the address for the timer interrupt is a
> > >>> +            pointer to an int
> > >>> +Returns: -EINVAL: Invalid timer interrupt number
> > >>> +         -EBUSY:  One or more VCPUs has already run
> > >>> +
> > >>> +A value describing the architected timer interrupt number when connected to an
> > >>> +in-kernel virtual GIC.  These must be a PPI (16 <= intid < 32).  If an
> > >>> +attribute is not set, a default value is applied (see below).
> > >>> +
> > >>> +KVM_ARM_VCPU_TIMER_IRQ_VTIMER: The EL1 virtual timer intid (default: 27)
> > >>> +KVM_ARM_VCPU_TIMER_IRQ_PTIMER: The EL1 physical timer intid (default: 30)
> > >>
> > >> uber nit: my reading of the code is that the default is set at VCPU
> > >> creation time. So setting the attribute overrides the default, not that
> > >> the default is applied if no attribute is set (i.e. there is always a
> > >> valid value).
> > >>
> > > 
> > > uh, right, I don't see the distinction though, so not sure how to
> > > correct the text.
> > > 
> > > Would "Setting the attribute overrides the default values (see below)."
> > > work instead?
> > 
> > Looks good to me.
> > 
> > [...]
> > 
> > >>
> > >> Another issue that just popped in my head: how do we ensure that we
> > >> don't clash between the PMU and the timers? Yes, that's a foolish thing
> > >> to do, but that's no different from avoiding the same interrupt on the
> > >> timers...
> > >>
> > > Argh, good point.
> > > 
> > > So actually, nothing *really bad* happens from using the same IRQ number
> > > except that the VM gets confused.  However, it's living life dangerously
> > > because we use the HW bit for the vtimer, so we if ever start using it
> > > for other timers or the PMU, then you could end up in a situation where
> > > you unmap the phys mapping on behalf of another device, and the physical
> > > interrupt could be left in an active state.
> > > 
> > > On the other hand, the vtimer currently belongs only to VMs and we set
> > > the required physical active state before entering the VM, so maybe it
> > > doesn't matter.
> > 
> > So far, we always assume that there is never more than a single source
> > per interrupt. We'll end-up with weird behaviours because our line_level
> > field is not an OR of the various inputs, but a direct assignment
> > (device A and B raise the line, then A drops it -> B's interrupt is gone).
> > 
> > I think that only the guest will be confused by it, but accepting it may
> > be interpreted as "this should work correctly". Would documenting that
> > it is a bad idea be enough?
> > 
> > > Should we just forego these checks and let the user shoot itself in the
> > > foot if he/she wants to?
> > 
> > If the documentation is enough, why not. Otherwise, we need some form of
> > allocator. Boring. ;-)
> > 
> 
> Well, it's not that bad really (untested):
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 1541f5d..122f9d3 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -121,6 +121,9 @@ struct vgic_irq {
>  	u8 source;			/* GICv2 SGIs only */
>  	u8 priority;
>  	enum vgic_irq_config config;	/* Level or edge */
> +
> +	void *owner;			/* Opaque pointer to reserve an interrupt
> +					   for in-kernel devices. */
>  };
>  
>  struct vgic_register_region;
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index 3d0979c..7561d2d 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -429,6 +429,42 @@ int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq)
>  }
>  
>  /**
> + * kvm_vgic_set_owner - Set the owner of an interrupt for a VM
> + *
> + * @kvm:    Pointer to the VM structure
> + * @intid:  The virtual INTID identifying the interrupt (PPI or SPI)
> + * @owner:  Opaque pointer to the owner
> + *
> + * Returns 0 if intid is not already used by another in-kernel device and the
> + * owner is set, otherwise returns an error code.
> + *
> + * We only set the owner for VCPU 0 for PPIs.
> + */
> +int kvm_vgic_set_owner(struct kvm *kvm, unsigned int intid, void *owner)
> +{
> +	struct vgic_irq *irq;
> +	struct kvm_vcpu *vcpu = kvm_get_vcpu(kvm, 0);
> +	int ret = 0;
> +
> +	if (!vgic_initialized(kvm))
> +		return -EAGAIN;
> +
> +	/* SGIs and LPIs cannot be wired up to any device */
> +	if (!irq_is_ppi(intid) && !vgic_valid_spi(kvm, intid))
> +		return -EINVAL;
> +
> +	irq = vgic_get_irq(kvm, vcpu, intid);
> +	spin_lock(&irq->irq_lock);
> +	if (irq->owner && irq->owner != owner)
> +		ret = -EEXIST;
> +	else
> +		irq->owner = owner;
> +	spin_unlock(&irq->irq_lock);
> +
> +	return ret;
> +}
> +
> +/**
>   * vgic_prune_ap_list - Remove non-relevant interrupts from the list
>   *
>   * @vcpu: The VCPU pointer
> 
> The problem is that it doesn't help that much, because userspace can
> still change the level of an IRQ which is connected to an in-kernel
> device, unless we also introduce checking the owner field in the
> injection path.
> 
> I don't see it blowing up the host with/without an allocator, so I'm
> fine with documentation, but I can also include the above.
> 
> Thoughts?
> 

I think we lost track of this series because it got stuck here.  I'm
tempted to just apply the series with the comment and without the owner
stuff posted above, and then we can fix it up later if it turns out
there's a problem.

Thanks,
-Christoffer

[PATCH 5/5] KVM: arm/arm64: Allow setting the timer IRQ numbers from userspace

[Christoffer Dall]

On Tue, May 16, 2017 at 08:54:21AM +0200, Christoffer Dall wrote:
> Hi Marc,
> 
> On Thu, May 04, 2017 at 01:22:25PM +0200, Christoffer Dall wrote:
> > On Thu, May 04, 2017 at 11:54:06AM +0100, Marc Zyngier wrote:
> > > On 04/05/17 10:59, Christoffer Dall wrote:
> > > > On Thu, May 04, 2017 at 10:34:32AM +0100, Marc Zyngier wrote:
> > > >> On 03/05/17 19:33, Christoffer Dall wrote:
> > > >>> First we define an ABI using the vcpu devices that lets userspace set
> > > >>> the interrupt numbers for the various timers on both the 32-bit and
> > > >>> 64-bit KVM/ARM implementations.
> > > >>>
> > > >>> Second, we add the definitions for the groups and attributes introduced
> > > >>> by the above ABI.  (We add the PMU define on the 32-bit side as well for
> > > >>> symmetry and it may get used some day.)
> > > >>>
> > > >>> Third, we set up the arch-specific vcpu device operation handlers to
> > > >>> call into the timer code for anything related to the
> > > >>> KVM_ARM_VCPU_TIMER_CTRL group.
> > > >>>
> > > >>> Fourth, we implement support for getting and setting the timer interrupt
> > > >>> numbers using the above defined ABI in the arch timer code.
> > > >>>
> > > >>> Fifth, we introduce error checking upon enabling the arch timer (which
> > > >>> is called when first running a VCPU) to check that all VCPUs are
> > > >>> configured to use the same PPI for the timer (as mandated by the
> > > >>> architecture) and that the virtual and physical timers are not
> > > >>> configured to use the same IRQ number.
> > > >>>
> > > >>> Signed-off-by: Christoffer Dall <cdall@linaro.org>
> > > >>> ---
> > > >>>  Documentation/virtual/kvm/devices/vcpu.txt |  25 +++++++
> > > >>>  arch/arm/include/uapi/asm/kvm.h            |   8 +++
> > > >>>  arch/arm/kvm/guest.c                       |   9 +++
> > > >>>  arch/arm64/include/uapi/asm/kvm.h          |   3 +
> > > >>>  arch/arm64/kvm/guest.c                     |   9 +++
> > > >>>  include/kvm/arm_arch_timer.h               |   4 ++
> > > >>>  virt/kvm/arm/arch_timer.c                  | 104 +++++++++++++++++++++++++++++
> > > >>>  7 files changed, 162 insertions(+)
> > > >>>
> > > >>> diff --git a/Documentation/virtual/kvm/devices/vcpu.txt b/Documentation/virtual/kvm/devices/vcpu.txt
> > > >>> index 352af6e..013e3f1 100644
> > > >>> --- a/Documentation/virtual/kvm/devices/vcpu.txt
> > > >>> +++ b/Documentation/virtual/kvm/devices/vcpu.txt
> > > >>> @@ -35,3 +35,28 @@ Returns: -ENODEV: PMUv3 not supported or GIC not initialized
> > > >>>  Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
> > > >>>  virtual GIC implementation, this must be done after initializing the in-kernel
> > > >>>  irqchip.
> > > >>> +
> > > >>> +
> > > >>> +2. GROUP: KVM_ARM_VCPU_TIMER_CTRL
> > > >>> +Architectures: ARM,ARM64
> > > >>> +
> > > >>> +2.1. ATTRIBUTE: KVM_ARM_VCPU_TIMER_IRQ_VTIMER
> > > >>> +2.2. ATTRIBUTE: KVM_ARM_VCPU_TIMER_IRQ_PTIMER
> > > >>> +Parameters: in kvm_device_attr.addr the address for the timer interrupt is a
> > > >>> +            pointer to an int
> > > >>> +Returns: -EINVAL: Invalid timer interrupt number
> > > >>> +         -EBUSY:  One or more VCPUs has already run
> > > >>> +
> > > >>> +A value describing the architected timer interrupt number when connected to an
> > > >>> +in-kernel virtual GIC.  These must be a PPI (16 <= intid < 32).  If an
> > > >>> +attribute is not set, a default value is applied (see below).
> > > >>> +
> > > >>> +KVM_ARM_VCPU_TIMER_IRQ_VTIMER: The EL1 virtual timer intid (default: 27)
> > > >>> +KVM_ARM_VCPU_TIMER_IRQ_PTIMER: The EL1 physical timer intid (default: 30)
> > > >>
> > > >> uber nit: my reading of the code is that the default is set at VCPU
> > > >> creation time. So setting the attribute overrides the default, not that
> > > >> the default is applied if no attribute is set (i.e. there is always a
> > > >> valid value).
> > > >>
> > > > 
> > > > uh, right, I don't see the distinction though, so not sure how to
> > > > correct the text.
> > > > 
> > > > Would "Setting the attribute overrides the default values (see below)."
> > > > work instead?
> > > 
> > > Looks good to me.
> > > 
> > > [...]
> > > 
> > > >>
> > > >> Another issue that just popped in my head: how do we ensure that we
> > > >> don't clash between the PMU and the timers? Yes, that's a foolish thing
> > > >> to do, but that's no different from avoiding the same interrupt on the
> > > >> timers...
> > > >>
> > > > Argh, good point.
> > > > 
> > > > So actually, nothing *really bad* happens from using the same IRQ number
> > > > except that the VM gets confused.  However, it's living life dangerously
> > > > because we use the HW bit for the vtimer, so we if ever start using it
> > > > for other timers or the PMU, then you could end up in a situation where
> > > > you unmap the phys mapping on behalf of another device, and the physical
> > > > interrupt could be left in an active state.
> > > > 
> > > > On the other hand, the vtimer currently belongs only to VMs and we set
> > > > the required physical active state before entering the VM, so maybe it
> > > > doesn't matter.
> > > 
> > > So far, we always assume that there is never more than a single source
> > > per interrupt. We'll end-up with weird behaviours because our line_level
> > > field is not an OR of the various inputs, but a direct assignment
> > > (device A and B raise the line, then A drops it -> B's interrupt is gone).
> > > 
> > > I think that only the guest will be confused by it, but accepting it may
> > > be interpreted as "this should work correctly". Would documenting that
> > > it is a bad idea be enough?
> > > 
> > > > Should we just forego these checks and let the user shoot itself in the
> > > > foot if he/she wants to?
> > > 
> > > If the documentation is enough, why not. Otherwise, we need some form of
> > > allocator. Boring. ;-)
> > > 
> > 
> > Well, it's not that bad really (untested):
> > 
> > diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> > index 1541f5d..122f9d3 100644
> > --- a/include/kvm/arm_vgic.h
> > +++ b/include/kvm/arm_vgic.h
> > @@ -121,6 +121,9 @@ struct vgic_irq {
> >  	u8 source;			/* GICv2 SGIs only */
> >  	u8 priority;
> >  	enum vgic_irq_config config;	/* Level or edge */
> > +
> > +	void *owner;			/* Opaque pointer to reserve an interrupt
> > +					   for in-kernel devices. */
> >  };
> >  
> >  struct vgic_register_region;
> > diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> > index 3d0979c..7561d2d 100644
> > --- a/virt/kvm/arm/vgic/vgic.c
> > +++ b/virt/kvm/arm/vgic/vgic.c
> > @@ -429,6 +429,42 @@ int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq)
> >  }
> >  
> >  /**
> > + * kvm_vgic_set_owner - Set the owner of an interrupt for a VM
> > + *
> > + * @kvm:    Pointer to the VM structure
> > + * @intid:  The virtual INTID identifying the interrupt (PPI or SPI)
> > + * @owner:  Opaque pointer to the owner
> > + *
> > + * Returns 0 if intid is not already used by another in-kernel device and the
> > + * owner is set, otherwise returns an error code.
> > + *
> > + * We only set the owner for VCPU 0 for PPIs.
> > + */
> > +int kvm_vgic_set_owner(struct kvm *kvm, unsigned int intid, void *owner)
> > +{
> > +	struct vgic_irq *irq;
> > +	struct kvm_vcpu *vcpu = kvm_get_vcpu(kvm, 0);
> > +	int ret = 0;
> > +
> > +	if (!vgic_initialized(kvm))
> > +		return -EAGAIN;
> > +
> > +	/* SGIs and LPIs cannot be wired up to any device */
> > +	if (!irq_is_ppi(intid) && !vgic_valid_spi(kvm, intid))
> > +		return -EINVAL;
> > +
> > +	irq = vgic_get_irq(kvm, vcpu, intid);
> > +	spin_lock(&irq->irq_lock);
> > +	if (irq->owner && irq->owner != owner)
> > +		ret = -EEXIST;
> > +	else
> > +		irq->owner = owner;
> > +	spin_unlock(&irq->irq_lock);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> >   * vgic_prune_ap_list - Remove non-relevant interrupts from the list
> >   *
> >   * @vcpu: The VCPU pointer
> > 
> > The problem is that it doesn't help that much, because userspace can
> > still change the level of an IRQ which is connected to an in-kernel
> > device, unless we also introduce checking the owner field in the
> > injection path.
> > 
> > I don't see it blowing up the host with/without an allocator, so I'm
> > fine with documentation, but I can also include the above.
> > 
> > Thoughts?
> > 
> 
> I think we lost track of this series because it got stuck here.  I'm
> tempted to just apply the series with the comment and without the owner
> stuff posted above, and then we can fix it up later if it turns out
> there's a problem.
> 

Actually, since people didn't formally ack these patches, I'm going to
simply send out a new series.

Thanks,
-Christoffer