Re: [Qemu-devel] [PATCH v3 3/5] Introduce irqchip type specification for KVM

[Eric Auger <eric.auger@linaro.org>]

Hi Pavel,
On 07/07/2015 03:54 PM, Pavel Fedin 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
s/it/is
> 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.
> 
> Just in case, the default value is set for absolutely all board models
> which include GIC in some form. I am not sure whether all these models can
> use KVM acceleration, but it definitely would not hurt.
To me this is misleading to set the interrupt control type to a wrong
value, if it is confirmed. A reader can get the impression this GIC
model is used by the virtual machine whereas it is not.

Besides we discussed this together before and you know I think we can
manage without this patch file by using kvm_create_device in trial mode
for v2 and v3 - but we did not agree on this -. Let's see what do the
other reviewers say ;-)
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  hw/arm/exynos4_boards.c | 1 +
>  hw/arm/realview.c       | 1 +
>  hw/arm/vexpress.c       | 1 +
>  hw/arm/virt.c           | 3 +++
>  include/hw/boards.h     | 1 +
>  include/sysemu/kvm.h    | 3 ++-
>  kvm-all.c               | 6 ++++--
>  stubs/kvm.c             | 2 +-
>  target-arm/kvm.c        | 8 ++++++--
>  9 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c
> index d644db1..d4136bc 100644
> --- a/hw/arm/exynos4_boards.c
> +++ b/hw/arm/exynos4_boards.c
> @@ -104,6 +104,7 @@ static Exynos4210State *exynos4_boards_init_common(MachineState *machine,
>                  exynos4_machines[board_type].max_cpus);
>      }
>  
> +    machine->kernel_irqchip_type = KVM_DEV_TYPE_ARM_VGIC_V2;
>      exynos4_board_binfo.ram_size = exynos4_board_ram_size[board_type];
>      exynos4_board_binfo.board_id = exynos4_board_id[board_type];
>      exynos4_board_binfo.smp_bootreg_addr =
> diff --git a/hw/arm/realview.c b/hw/arm/realview.c
> index ef2788d..f670d9f 100644
> --- a/hw/arm/realview.c
> +++ b/hw/arm/realview.c
> @@ -74,6 +74,7 @@ static void realview_init(MachineState *machine,
>      ram_addr_t ram_size = machine->ram_size;
>      hwaddr periphbase = 0;
>  
> +    machine->kernel_irqchip_type = KVM_DEV_TYPE_ARM_VGIC_V2;
>      switch (board_type) {
>      case BOARD_EB:
>          break;
> 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 6379901..6e42cf2 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -126,6 +126,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..c103aad 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1395,8 +1395,10 @@ 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);
> -    if (ret == 0) {
> +    ret = kvm_arch_irqchip_create(s, machine->kernel_irqchip_type);
> +    if (ret < 0) {
> +        return ret;
this fails to compile, kvm_irqchip_create being a function returning a void

Best Regards

Eric
> +    } else if (ret == 0) {
>          ret = kvm_vm_ioctl(s, KVM_CREATE_IRQCHIP);
>      }
>      if (ret < 0) {
> 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 548bfd7..fa1073f 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -579,7 +579,7 @@ 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;
>  
> @@ -587,11 +587,15 @@ int kvm_arch_irqchip_create(KVMState *s)
>       * 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;
>  }
>  
>