Re: [Qemu-devel] [PATCH v6 4/6] Introduce irqchip type specification for KVMis

Re: [Qemu-devel] [PATCH v6 4/6] Introduce irqchip type specification for KVMis

[Shlomo Pongratz]

[-- Attachment #1: Type: text/plain, Size: 4973 bytes --]

Hi,

This Doesn't compile, a problem with KVM_DEV_TYPE_ARM_VGIC_V2.
I assume this is include file issue as it exists in
linux-headers/linux/kvm.h
Note that everything should compile also for TCG only.

Best regards,

S.P.


On Wednesday, July 22, 2015, Pavel Fedin <p.fedin@samsung.com> wrote:

> This patch introduces kernel_irqchip_type member in Machine class, which
> it passed to kvm_arch_irqchip_create. It allows machine models to specify
> correct GIC type during KVM capability verification. The variable is
> defined as int in order to be architecture-agnostic for potential future
> uses by other architectures.
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com <javascript:;>>
> ---
>  hw/arm/vexpress.c    |  1 +
>  hw/arm/virt.c        |  3 +++
>  include/hw/boards.h  |  1 +
>  include/sysemu/kvm.h |  3 ++-
>  kvm-all.c            |  2 +-
>  stubs/kvm.c          |  2 +-
>  target-arm/kvm.c     | 13 +++++++++++--
>  7 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index da21788..0675a00 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -556,6 +556,7 @@ static void vexpress_common_init(MachineState *machine)
>      const hwaddr *map = daughterboard->motherboard_map;
>      int i;
>
> +    machine->kernel_irqchip_type = KVM_DEV_TYPE_ARM_VGIC_V2;
>      daughterboard->init(vms, machine->ram_size, machine->cpu_model, pic);
>
>      /*
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 4846892..2e7d858 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -945,6 +945,9 @@ static void virt_instance_init(Object *obj)
>                                      "Set on/off to enable/disable the ARM
> "
>                                      "Security Extensions (TrustZone)",
>                                      NULL);
> +
> +    /* Default GIC type is v2 */
> +    vms->parent.kernel_irqchip_type = KVM_DEV_TYPE_ARM_VGIC_V2;
>  }
>
>  static void virt_class_init(ObjectClass *oc, void *data)
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 2aec9cb..37eb767 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -127,6 +127,7 @@ struct MachineState {
>      char *accel;
>      bool kernel_irqchip_allowed;
>      bool kernel_irqchip_required;
> +    int kernel_irqchip_type;
>      int kvm_shadow_mem;
>      char *dtb;
>      char *dumpdtb;
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 983e99e..8f4d485 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -434,6 +434,7 @@ void kvm_init_irq_routing(KVMState *s);
>  /**
>   * kvm_arch_irqchip_create:
>   * @KVMState: The KVMState pointer
> + * @type: irqchip type, architecture-specific
>   *
>   * Allow architectures to create an in-kernel irq chip themselves.
>   *
> @@ -441,7 +442,7 @@ void kvm_init_irq_routing(KVMState *s);
>   *            0: irq chip was not created
>   *          > 0: irq chip was created
>   */
> -int kvm_arch_irqchip_create(KVMState *s);
> +int kvm_arch_irqchip_create(KVMState *s, int type);
>
>  /**
>   * kvm_set_one_reg - set a register value in KVM via KVM_SET_ONE_REG ioctl
> diff --git a/kvm-all.c b/kvm-all.c
> index 06e06f2..8df938d 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1395,7 +1395,7 @@ static void kvm_irqchip_create(MachineState
> *machine, KVMState *s)
>
>      /* First probe and see if there's a arch-specific hook to create the
>       * in-kernel irqchip for us */
> -    ret = kvm_arch_irqchip_create(s);
> +    ret = kvm_arch_irqchip_create(s, machine->kernel_irqchip_type);
>      if (ret == 0) {
>          ret = kvm_vm_ioctl(s, KVM_CREATE_IRQCHIP);
>      }
> diff --git a/stubs/kvm.c b/stubs/kvm.c
> index e7c60b6..a8505ff 100644
> --- a/stubs/kvm.c
> +++ b/stubs/kvm.c
> @@ -1,7 +1,7 @@
>  #include "qemu-common.h"
>  #include "sysemu/kvm.h"
>
> -int kvm_arch_irqchip_create(KVMState *s)
> +int kvm_arch_irqchip_create(KVMState *s, int type)
>  {
>      return 0;
>  }
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index b278542..180f75f 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -583,19 +583,28 @@ void kvm_arch_init_irq_routing(KVMState *s)
>  {
>  }
>
> -int kvm_arch_irqchip_create(KVMState *s)
> +int kvm_arch_irqchip_create(KVMState *s, int type)
>  {
>      int ret;
>
> +    /* Failure here means forgotten initialization of
> +     * machine->kernel_irqchip_type in model code
> +     */
> +    assert(type != 0);
> +
>      /* If we can create the VGIC using the newer device control API, we
>       * let the device do this when it initializes itself, otherwise we
>       * fall back to the old API */
>
> -    ret = kvm_create_device(s, KVM_DEV_TYPE_ARM_VGIC_V2, true);
> +    ret = kvm_create_device(s, type, true);
>      if (ret == 0) {
>          return 1;
>      }
>
> +    /* Fallback will create VGIC v2 */
> +    if (type != KVM_DEV_TYPE_ARM_VGIC_V2) {
> +        return ret;
> +    }
>      return 0;
>  }
>
> --
> 1.9.5.msysgit.0
>
>

[-- Attachment #2: Type: text/html, Size: 6093 bytes --]

Re: [Qemu-devel] [PATCH v6 4/6] Introduce irqchip type specification for KVMis

[Pavel Fedin]

 Hello!

> This Doesn't compile, a problem with KVM_DEV_TYPE_ARM_VGIC_V2.
> I assume this is include file issue as it exists in linux-headers/linux/kvm.h
> Note that everything should compile also for TCG only.

 Damn! I think i know what the problem is... Your host arch != target arch and CONFIG_KVM gets disabled.
 Can you try adding explicit #include "linux/kvm.h" in vexpress.c and virt.c for a quick test?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH v6 4/6] Introduce irqchip type specification for KVMis

[Shlomo Pongratz]

[-- Attachment #1: Type: text/plain, Size: 661 bytes --]

Sorry, weekend.
But I'll test it as soon as possible.
S.P.

On Fri, Jul 24, 2015, 10:13 Pavel Fedin <p.fedin@samsung.com> wrote:

>  Hello!
>
> > This Doesn't compile, a problem with KVM_DEV_TYPE_ARM_VGIC_V2.
> > I assume this is include file issue as it exists in
> linux-headers/linux/kvm.h
> > Note that everything should compile also for TCG only.
>
>  Damn! I think i know what the problem is... Your host arch != target arch
> and CONFIG_KVM gets disabled.
>  Can you try adding explicit #include "linux/kvm.h" in vexpress.c and
> virt.c for a quick test?
>
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
>
>

[-- Attachment #2: Type: text/html, Size: 985 bytes --]

Re: [Qemu-devel] [PATCH v6 4/6] Introduce irqchip type specification for KVMis

[Pavel Fedin]

 Hello!

> Sorry, weekend.
> But I'll test it as soon as possible.

 Thanks for pointing at it, my guess was correct, i have verified it by myself. Looks like i cannot use KVM definitions outside of KVM-only code. And simple #include <linux/kvm.h> will not help because this will not compile on non-Linux hosts. I will look for another solution.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH v6 4/6] Introduce irqchip type specification for KVMis

[Peter Maydell]

On 24 July 2015 at 08:44, Pavel Fedin <p.fedin@samsung.com> wrote:
>  Thanks for pointing at it, my guess was correct, i have verified
> it by myself. Looks like i cannot use KVM definitions outside of
> KVM-only code. And simple #include <linux/kvm.h> will not help
> because this will not compile on non-Linux hosts. I will look for
> another solution.

Yep, that's right. You have a couple of choices:
(1) make sure the KVM definitions are only used in KVM-specific
source files. This is how we handle KVM_DEV_TYPE_ARM_VGIC_V2,
for instance.
(2) provide QEMU versions of the constants in target-arm/kvm-consts.h.
This is how we handle KVM_ARM_TARGET_* (which in non-KVM-specific
code are always referred to using the QEMU_KVM_ARM_TARGET_*
names which exist on all hosts.)

Following how the existing GICv2 code handles this is probably
a good approach.

thanks
-- PMM