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

[Ian Campbell <Ian.Campbell@citrix.com>]

On Sat, 2014-11-01 at 20:10 +0000, Julien Grall wrote:
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index a9bcd4a..1aa1f45 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -48,6 +48,26 @@ int xc_domain_create(xc_interface *xch,
>      return 0;
>  }
>  
> +#if defined(__arm__) || defined(__aarch64__)
> +int xc_domain_configure(xc_interface *xch, uint32_t domid,
> +                        xc_domain_configuration_t *config)
> +{
> +    int rc;
> +    DECLARE_DOMCTL;
> +
> +    domctl.cmd = XEN_DOMCTL_arm_configure_domain;
> +    domctl.domain = (domid_t)domid;
> +    /* xc_domain_configure_t is an alias to xen_domctl_arm_configuredomain */

"is an alias of".

> +
> +    res = fdt_property_compat(gc, fdt, 1,
> +                              "arm,gic-v3");

Doesn't need wrapping I expect.

>  static int make_timer_node(libxl__gc *gc, void *fdt, const struct arch_info *ainfo)
>  {
>      int res;
> @@ -456,6 +519,7 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc,
>                                             libxl_domain_build_info *info,
>                                             struct xc_dom_image *dom)
>  {
> +    xc_domain_configuration_t config;
>      void *fdt = NULL;
>      int rc, res;
>      size_t fdt_size = 0;
> @@ -471,8 +535,16 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc,
>      ainfo = get_arch_info(gc, dom);
>      if (ainfo == NULL) return ERROR_FAIL;
>  
> +    LOG(DEBUG, "configure the domain");
> +    config.gic_version = XEN_DOMCTL_CONFIG_GIC_DEFAULT;
> +    if (xc_domain_configure(CTX->xch, dom->guest_domid, &config) != 0) {
> +        LOG(ERROR, "counldn't configure the domain");

"couldn't"

> @@ -520,9 +592,22 @@ next_resize:
>          FDT( make_psci_node(gc, fdt) );
>  
>          FDT( make_memory_nodes(gc, fdt, dom) );
> -        FDT( make_intc_node(gc, fdt,
> -                            GUEST_GICD_BASE, GUEST_GICD_SIZE,
> -                            GUEST_GICC_BASE, GUEST_GICD_SIZE) );
> +
> +        switch (config.gic_version) {
> +        case XEN_DOMCTL_CONFIG_GIC_V2:
> +            FDT( make_gicv2_node(gc, fdt,
> +                                 GUEST_GICD_BASE, GUEST_GICD_SIZE,
> +                                 GUEST_GICC_BASE, GUEST_GICC_SIZE) );
> +            break;
> +        case XEN_DOMCTL_CONFIG_GIC_V3:
> +            FDT( make_gicv3_node(gc, fdt) );
> +            break;
> +        default:
> +            LOG(ERROR, "Unknown how to create the DT node for gic_version = %d",
> +                config.gic_version);

"Don't know how..." or even just "Unknown GIC version %d".

> @@ -30,6 +32,39 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>  
>          return p2m_cache_flush(d, s, e);
>      }
> +    case XEN_DOMCTL_arm_configure_domain:
> +    {
> +        uint8_t gic_version;
> +
> +        /*
> +         * Xen 4.5: The vGIC is emulating the same version of the

No need to say "Xen 4.5" here, this comment is true until it is removed.
You could say "Currently the vGIC is..." or something if you wanted to
indicate that this is temporary.

> +         * hardware GIC. Only the value XEN_DOMCTL_CONFIG_GIC_DEFAULT
> +         * is allowed. The DOMCTL will return the actual version of the
> +         * GIC.
> +         */
> +        if ( domctl->u.configuredomain.gic_version != XEN_DOMCTL_CONFIG_GIC_DEFAULT )
> +            return -EOPNOTSUPP;

EOPNOTSUPP doesn't seem quite right. EPARM  or EINVAL perhaps?

I'm also tempted to suggest that we should accept gic_version == the hw
value, i.e. by moping the switch below up and including
        && domctl->u.configuredomain.gic_version != gic_version
in the condition.

> +#define GUEST_GICV3_GICR0_BASE     0x03020000ULL    /* vCPU0 - vCPU15 */

Don't we only support 8 cpus today? I don't mind reserving space for
more, and I'd be pleasantly surprised if 16 actually worked ;-)

Ian.