[4/6] kvm: Don't assume irqchip-in-kernel implies irqfds

Instead of assuming that we can use irqfds if and only if
kvm_irqchip_in_kernel(), add a bool to the KVMState which
indicates this, and is set only on x86 and only if the
irqchip is in the kernel.

The kernel documentation implies that the only thing
you need to use KVM_IRQFD is that KVM_CAP_IRQFD is
advertised, but this seems to be untrue. In particular
the kernel does not (alas) return a sensible error if you
try to set up an irqfd when you haven't created an irqchip.
If it did we could remove all this nonsense and let the
kernel return the error code.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 kvm-all.c         |    3 ++-
 kvm-stub.c        |    1 +
 kvm.h             |   10 ++++++++++
 target-i386/kvm.c |    4 ++++
 4 files changed, 17 insertions(+), 1 deletions(-)

On 2012-07-25 15:24, Peter Maydell wrote:
> Instead of assuming that we can use irqfds if and only if
> kvm_irqchip_in_kernel(), add a bool to the KVMState which
> indicates this, and is set only on x86 and only if the
> irqchip is in the kernel.
> 
> The kernel documentation implies that the only thing
> you need to use KVM_IRQFD is that KVM_CAP_IRQFD is
> advertised, but this seems to be untrue. In particular
> the kernel does not (alas) return a sensible error if you
> try to set up an irqfd when you haven't created an irqchip.
> If it did we could remove all this nonsense and let the
> kernel return the error code.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  kvm-all.c         |    3 ++-
>  kvm-stub.c        |    1 +
>  kvm.h             |   10 ++++++++++
>  target-i386/kvm.c |    4 ++++
>  4 files changed, 17 insertions(+), 1 deletions(-)
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index 8e21d81..a88b8ad 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -101,6 +101,7 @@ struct KVMState
>  KVMState *kvm_state;
>  bool kvm_kernel_irqchip;
>  bool kvm_async_interrupt_injection;
> +bool kvm_irqfds_allowed;

Why allowed vs enabled? You only have kvm_async_interrupt_injection as well.

>  
>  static const KVMCapabilityInfo kvm_required_capabilites[] = {
>      KVM_CAP_INFO(USER_MEMORY),
> @@ -1126,7 +1127,7 @@ static int kvm_irqchip_assign_irqfd(KVMState *s, int fd, int virq, bool assign)
>          .flags = assign ? 0 : KVM_IRQFD_FLAG_DEASSIGN,
>      };
>  
> -    if (!kvm_irqchip_in_kernel()) {
> +    if (!kvm_irqfds_enabled()) {
>          return -ENOSYS;
>      }
>  
> diff --git a/kvm-stub.c b/kvm-stub.c
> index af1cb5e..179e5de 100644
> --- a/kvm-stub.c
> +++ b/kvm-stub.c
> @@ -20,6 +20,7 @@
>  KVMState *kvm_state;
>  bool kvm_kernel_irqchip;
>  bool kvm_async_interrupt_injection;
> +bool kvm_irqfds_allowed;
>  
>  int kvm_init_vcpu(CPUArchState *env)
>  {
> diff --git a/kvm.h b/kvm.h
> index e6cbf12..2337eb0 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -25,6 +25,7 @@
>  extern int kvm_allowed;
>  extern bool kvm_kernel_irqchip;
>  extern bool kvm_async_interrupt_injection;
> +extern bool kvm_irqfds_allowed;
>  
>  #if defined CONFIG_KVM || !defined NEED_CPU_H
>  #define kvm_enabled()           (kvm_allowed)
> @@ -38,10 +39,19 @@ extern bool kvm_async_interrupt_injection;
>   * (where the vcpu must be stopped at a suitable point first).
>   */
>  #define kvm_async_interrupt_injection() (kvm_async_interrupt_injection)
> +/**
> + * kvm_irqfds_enabled:
> + *
> + * Returns: true if we can use irqfds to inject interrupts into
> + * a KVM CPU (ie the kernel supports irqfds and we are running
> + * with a configuration where it is meaningful to use them).
> + */
> +#define kvm_irqfds_enabled() (kvm_irqfds_allowed)
>  #else
>  #define kvm_enabled()           (0)
>  #define kvm_irqchip_in_kernel() (false)
>  #define kvm_async_interrupt_injection() (false)
> +#define kvm_irqfds_enabled() (false)
>  #endif
>  
>  struct kvm_run;
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 503abeb..8e19a4d 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -2045,4 +2045,8 @@ void kvm_arch_init_irq_routing(KVMState *s)
>           */
>          no_hpet = 1;
>      }
> +    /* We know at this point that we're using the in-kernel
> +     * irqchip, so we can use irqfds.
> +     */
> +    kvm_irqfds_allowed = true;
>  }
> 

Otherwise:

Acked-by: Jan Kiszka <jan.kiszka@siemens.com>

On 25 July 2012 16:47, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2012-07-25 15:24, Peter Maydell wrote:
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -101,6 +101,7 @@ struct KVMState
>>  KVMState *kvm_state;
>>  bool kvm_kernel_irqchip;
>>  bool kvm_async_interrupt_injection;
>> +bool kvm_irqfds_allowed;
>
> Why allowed vs enabled? You only have kvm_async_interrupt_injection as well.

I was trying to follow the existing pattern where the macro kvm_enabled()
tests the variable kvm_allowed (though as you noticed I got it wrong for
kvm_async_interrupt_injection: will fix that in v2.)

Having the two the same is valid C, it's just a style question whether
having a variable foo and a macro foo() is considered confusing I guess.

-- PMM

On 2012-07-25 17:52, Peter Maydell wrote:
> On 25 July 2012 16:47, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2012-07-25 15:24, Peter Maydell wrote:
>>> --- a/kvm-all.c
>>> +++ b/kvm-all.c
>>> @@ -101,6 +101,7 @@ struct KVMState
>>>  KVMState *kvm_state;
>>>  bool kvm_kernel_irqchip;
>>>  bool kvm_async_interrupt_injection;
>>> +bool kvm_irqfds_allowed;
>>
>> Why allowed vs enabled? You only have kvm_async_interrupt_injection as well.
> 
> I was trying to follow the existing pattern where the macro kvm_enabled()
> tests the variable kvm_allowed (though as you noticed I got it wrong for
> kvm_async_interrupt_injection: will fix that in v2.)
> 
> Having the two the same is valid C, it's just a style question whether
> having a variable foo and a macro foo() is considered confusing I guess.

I don't mind which way if they are consistent.

Jan