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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Thu, Sep 25, 2014 at 5:16 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> 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?

   One Re-distributor is enough because max vcpus supported
for now is 8.

Vijay

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