Re: [RFC PATCH v2] xen/arm: Add support for GICv3 for domU

[Julien Grall <julien.grall@linaro.org>]

Hello Vijay,

On 10/06/2014 01:46 PM, vijay.kilari@gmail.com wrote:
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 7d9eec2..8f3f074 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -349,6 +349,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      ("disable_migrate", libxl_defbool),
>      ("cpuid",           libxl_cpuid_policy_list),
>      ("blkdev_start",    string),
> +    ("gic_version",     uint32),

How would you differentiate GICv2 from GICv2m with an integer? I think
an enum would be better to describe the GIC version.

> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 2ec17ca..5fcb396 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1523,6 +1523,9 @@ skip_vfb:
>      if (!xlu_cfg_get_long (config, "pci_seize", &l, 0))
>          pci_seize = l;
>  
> +    if (!xlu_cfg_get_long (config, "gic_version", &l, 0))
> +        b_info->gic_version = l;
> +

You have to document this new option in docs/man/xl.cfg.pod.5

[..]

> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index 370dd99..1bea026 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -10,6 +10,8 @@
>  #include <xen/errno.h>
>  #include <xen/sched.h>
>  #include <xen/hypercall.h>
> +#include <asm/gic.h>
> +#include <xen/guest_access.h>
>  #include <public/domctl.h>
>  
>  long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
> @@ -39,9 +41,36 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>          if ( domctl->u.configuredomain.nr_spis > (gic_number_lines() - 32) )
>              return -EINVAL;
>  
> -        return domain_configure_vgic(d, domctl->u.configuredomain.nr_spis);
> -    }
> +        if ( domain_configure_vgic(d, domctl->u.configuredomain.nr_spis) )
> +            return -EINVAL;

domain_configure_vgic should be called after we check that current
version of GIC match. The user may want to chose to emulate a GICv2 on
GICv3 hardware.

> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index cebb349..6f80c99 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -363,6 +363,14 @@ typedef uint64_t xen_callback_t;
>   * should instead use the FDT.
>   */
>
> +/* GICv3 address space */
> +#define GUEST_GICV3_GICD_BASE      0x03001000ULL
> +#define GUEST_GICV3_GICD_SIZE      0x10000ULL
> +#define GUEST_GICV3_GICR_BASE      0x03020000ULL
> +#define GUEST_GICV3_GICR_SIZE      0x200000ULL
> +#define GUEST_GICV3_RDIST_STRIDE   0x20000ULL
> +#define GUEST_GICV3_RDIST_REGIONS  0x1ULL
> +

This should go after "/* Physical Address Space */

>  /* Physical Address Space */
>  #define GUEST_GICD_BASE   0x03001000ULL
>  #define GUEST_GICD_SIZE   0x00001000ULL

Please modify those defines, along *GICC* to add GICV2 in the name.

> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 8adb8e2..502cfb6 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -73,6 +73,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_createdomain_t);
>  struct xen_domctl_configuredomain {
>      /* IN parameters */
>      uint32_t nr_spis;
> +    /* IN/OUT parameter */
> +    uint32_t gic_version;

uint32_t sounds a bit too much for the gic_version. Maybe a uint8_t?
Also a better name would be vgic_version.

Futhermore, people reading the structure don't know what value should be
expected in this field. I would introduce define to specify the
different value.

Regards,

-- 
Julien Grall