[Qemu-devel] Re: [PATCH v2 3/9] provide in-kernel ioapic

[Jan Kiszka <jan.kiszka@siemens.com>]

Glauber Costa wrote:
> This patch provides kvm with an in-kernel ioapic. We are currently not enabling it.
> The code is heavily based on what's in qemu-kvm.git.
> 
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
>  hw/ioapic.c |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  kvm-all.c   |   20 ++++++++++++++
>  kvm.h       |    4 +++
>  3 files changed, 107 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/ioapic.c b/hw/ioapic.c
> index d475654..d6a9dac 100644
> --- a/hw/ioapic.c
> +++ b/hw/ioapic.c
> @@ -24,6 +24,7 @@
>  #include "pc.h"
>  #include "qemu-timer.h"
>  #include "host-utils.h"
> +#include "kvm.h"
>  
>  //#define DEBUG_IOAPIC
>  
> @@ -193,6 +194,79 @@ static void ioapic_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t va
>      }
>  }
>  
> +static int kvm_kernel_ioapic_load_from_user(IOAPICState *s)
> +{
> +    int r = 0;
> +#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)

Hmm, here we additionally depend on TARGET_I386...

> +    struct kvm_irqchip chip;
> +    struct kvm_ioapic_state *kioapic;
> +    int i;
> +
> +    if (!(kvm_enabled() && kvm_irqchip_in_kernel()))
> +        return 0;
> +
> +    chip.chip_id = KVM_IRQCHIP_IOAPIC;
> +    kioapic = &chip.chip.ioapic;
> +
> +    kioapic->id = s->id;
> +    kioapic->ioregsel = s->ioregsel;
> +    kioapic->base_address = IOAPIC_DEFAULT_BASE_ADDRESS;
> +    kioapic->irr = s->irr;
> +    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
> +        kioapic->redirtbl[i].bits = s->ioredtbl[i];
> +    }
> +
> +    r = kvm_set_irqchip(&chip);
> +#endif
> +    return r;
> +}
> +
> +static void kvm_kernel_ioapic_save_to_user(IOAPICState *s)
> +{
> +#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386)
> +    struct kvm_irqchip chip;
> +    struct kvm_ioapic_state *kioapic;
> +    int i;
> +
> +    if (!(kvm_enabled() && kvm_irqchip_in_kernel()))
> +        return;
> +    chip.chip_id = KVM_IRQCHIP_IOAPIC;
> +    kvm_get_irqchip(&chip);
> +    kioapic = &chip.chip.ioapic;
> +
> +    s->id = kioapic->id;
> +    s->ioregsel = kioapic->ioregsel;
> +    s->irr = kioapic->irr;
> +    for (i = 0; i < IOAPIC_NUM_PINS; i++) {
> +        s->ioredtbl[i] = kioapic->redirtbl[i].bits;
> +    }
> +#endif
> +}
> +
> +static void ioapic_pre_save(void *opaque)
> +{
> +    IOAPICState *s = (void *)opaque;
> +
> +    kvm_kernel_ioapic_save_to_user(s);
> +}
> +
> +static int ioapic_pre_load(void *opaque)
> +{
> +    IOAPICState *s = opaque;
> +
> +    /* in case we are doing version 1, we just set these to sane values */
> +    s->irr = 0;
> +    return 0;
> +}
> +
> +static int ioapic_post_load(void *opaque, int version_id)
> +{
> +    IOAPICState *s = opaque;
> +
> +    return kvm_kernel_ioapic_load_from_user(s);
> +}
> +
> +
>  static const VMStateDescription vmstate_ioapic = {
>      .name = "ioapic",
>      .version_id = 2,
> @@ -205,7 +279,10 @@ static const VMStateDescription vmstate_ioapic = {
>          VMSTATE_UINT32_V(irr, IOAPICState, 2),
>          VMSTATE_UINT64_ARRAY(ioredtbl, IOAPICState, IOAPIC_NUM_PINS),
>          VMSTATE_END_OF_LIST()
> -    }
> +    },
> +    .pre_load = ioapic_pre_load,
> +    .post_load = ioapic_post_load,
> +    .pre_save = ioapic_pre_save,
>  };
>  
>  static void ioapic_reset(void *opaque)
> @@ -217,6 +294,11 @@ static void ioapic_reset(void *opaque)
>      s->base_address = IOAPIC_DEFAULT_BASE_ADDRESS;
>      for(i = 0; i < IOAPIC_NUM_PINS; i++)
>          s->ioredtbl[i] = 1 << 16; /* mask LVT */
> +#ifdef KVM_CAP_IRQCHIP

... but here only KVM_CAP_IRQCHIP suffices?

> +    if (kvm_enabled() && kvm_irqchip_in_kernel()) {
> +        kvm_kernel_ioapic_load_from_user(s);
> +    }
> +#endif

Independent of this, both kvm_irqchip_in_kernel() and
kvm_kernel_ioapic_load_from_user() should also be defined for the
!KVM_CAP_IRQCHIP case to avoid #ifdefs here and in many other places
this series touches.

>  }
>  
>  static CPUReadMemoryFunc * const ioapic_mem_read[3] = {
> diff --git a/kvm-all.c b/kvm-all.c
> index 48ae26c..d795285 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -411,6 +411,26 @@ int kvm_check_extension(KVMState *s, unsigned int extension)
>      return ret;
>  }
>  
> +#ifdef KVM_CAP_IRQCHIP

Again, only KVM_CAP_IRQCHIP - what is the actually required dependency?

> +int kvm_set_irqchip(struct kvm_irqchip *chip)
> +{
> +    if (!kvm_state->irqchip_in_kernel) {
> +        return 0;
> +    }
> +
> +    return kvm_vm_ioctl(kvm_state, KVM_SET_IRQCHIP, chip);
> +}
> +
> +int kvm_get_irqchip(struct kvm_irqchip *chip)
> +{
> +    if (!kvm_state->irqchip_in_kernel) {
> +        return 0;
> +    }
> +
> +    return kvm_vm_ioctl(kvm_state, KVM_GET_IRQCHIP, chip);
> +}
> +#endif
> +
>  int kvm_init(int smp_cpus)
>  {
>      static const char upgrade_note[] =
> diff --git a/kvm.h b/kvm.h
> index e7d5beb..8d4afa0 100644
> --- a/kvm.h
> +++ b/kvm.h
> @@ -16,6 +16,7 @@
>  
>  #include "config.h"
>  #include "qemu-queue.h"
> +#include <linux/kvm.h>
>  
>  #ifdef CONFIG_KVM
>  extern int kvm_allowed;
> @@ -63,6 +64,9 @@ int kvm_update_guest_debug(CPUState *env, unsigned long reinject_trap);
>  int kvm_pit_in_kernel(void);
>  int kvm_irqchip_in_kernel(void);
>  
> +int kvm_set_irqchip(struct kvm_irqchip *chip);
> +int kvm_get_irqchip(struct kvm_irqchip *chip);
> +
>  /* internal API */
>  
>  struct KVMState;

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux