Re: [Xen-devel] [RFC PATCH] xen/arm: Add support for GICv3 for domU

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-09-24 at 18:35 +0530, vijay.kilari@xxxxxxxxx wrote:

This generally looks to be the right shape, thanks.

> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index 9605953..901032d 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -29,6 +29,7 @@ int libxl__arch_domain_create_pre(libxl__gc *gc, 
> libxl_domain_config *d_config,
>  {
>      int dev_index;
>      uint32_t nr_spis = 0;
> +    uint32_t gic_version;
>  
>      for (dev_index = 0; dev_index < d_config->num_dtdevs; dev_index++)
>          nr_spis += state->dtdevs_info[dev_index].num_irqs;
> @@ -42,6 +43,16 @@ int libxl__arch_domain_create_pre(libxl__gc *gc, 
> libxl_domain_config *d_config,
>          return ERROR_FAIL;
>      }
>  
> +    if (d_config->b_info.gic_version == 0) {
> +        /* GIC version is not set. Query host */
> +        if (xc_domain_get_gicversion(CTX->xch, domid, &gic_version) != 0) {
> +            LOG(ERROR, "Couldn't get GIC version from xen");
> +            return ERROR_FAIL;        
> +        }
> +        d_config->b_info.gic_version = gic_version;
> +    }
> +    LOG(DEBUG, "GIC version %d\n", d_config->b_info.gic_version);

I'm in two minds about whether this belongs here or in
libxl__domain_build_info_setdefaults inside an ifdef. Both approaches
have arguments for them (here==it's ARM specific, setdefaults == keeps
all defaults handling in one place).

Anyone got any opinions?

It should probably check that the gic version is either 2 or 3, since it
won't correctly handle anything else later on.

I'm also not seeing the bit which would push the selection down to Xen
(i.e. if someone asks for v2 on a v3 based system). I think the
intention was to add that to the xen_domctl_configuredomain? I'm not
sure but plumbing that in might allow us to avoid the get_gicversion
domctl e.g by declaring that passing 0 means "xen chooses and updates
the field on return to the selection" (i.e. make it an IN/OUT
parameter). That would also help handle the case of GICv3 H/W without
GIC v2 compat support present, you could either return an error or the
toolstack could check if the answer wasn't equal to the requested value
and bomb out.

> @@ -662,9 +688,19 @@ 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) );
> +
> +        if (info->gic_version == 3) {
> +            FDT( make_intc_node(gc, fdt,
> +                                GUEST_GICV3_GICD_BASE, GUEST_GICV3_GICD_SIZE,
> +                                GUEST_GICV3_GICR_BASE, GUEST_GICV3_GICR_SIZE,
> +                                info->gic_version) );
> +        }
> +        else {
> +            FDT( make_intc_node(gc, fdt,
> +                                GUEST_GICD_BASE, GUEST_GICD_SIZE,
> +                                GUEST_GICC_BASE, GUEST_GICD_SIZE,

I think I'd be inclined to drop all the addr arguments from this
function and select the set of bases within make_intc_node based on the
version parameter.

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index e93dbfa..fde5361 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -340,6 +340,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      ("disable_migrate", libxl_defbool),
>      ("cpuid",           libxl_cpuid_policy_list),
>      ("blkdev_start",    string),
> +    ("gic_version",     uint32),

A new field requires a LIBXL_HAVE #define in libxl.h. There's a comment
in there and a bunch of existing examples to follow.

We've so far not been very good at this but since this is ARM specific I
think there should be a comment above it nothing that this is ARM only.

Eventually we might want to retroactively tag the x86 only stuff as such
too for clarity.

> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 9bdda32..059b60e 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -907,9 +907,16 @@ static int gicv_v3_init(struct domain *d)
>          d->arch.vgic.rdist_count = gicv3.rdist_count;
>      }
>      else
> -        d->arch.vgic.dbase = GUEST_GICD_BASE;
> +    {
> +        d->arch.vgic.dbase = GUEST_GICV3_GICD_BASE;
> +        d->arch.vgic.dbase_size = GUEST_GICV3_GICD_SIZE;
> +
> +        /* XXX: Only one Re-distributor region mapped for guest */

That's ok thought, I think? Or does something need fixing here?

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel