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

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

Hi Stefano,

On 31/10/2014 17:45, Stefano Stabellini wrote:
>>   static int make_timer_node(libxl__gc *gc, void *fdt, const struct arch_info *ainfo)
>>   {
>>       int res;
>> @@ -454,6 +539,7 @@ out:
>>
>>   int libxl__arch_domain_init_hw_description(libxl__gc *gc,
>>                                              libxl_domain_build_info *info,
>> +                                           libxl__domain_build_state *state,
>>                                              struct xc_dom_image *dom)
>>   {
>>       void *fdt = NULL;
>
> Can't you just call xc_configure_domain from here? Why do you need to
> introduce libxl__arch_domain_create_pre?

The DOMCTL was initially created to defer the VGIC initialization which 
has to be done before the toolstack set the number of VCPUs.

Even though, the current implementation of the DOMCTL only returns the 
version of the VGIC, I think it's still better to call it in the right 
place for now.

I don't really want to justify on the latter patch why we have to move 
the call earlier.

>>   long arch_do_domctl(struct xen_domctl *domctl, struct domain *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_configure_domain:
>> +    {
>> +        uint8_t gic_version;
>> +
>> +        /*
>> +         * Xen 4.5: The vGIC is emulating the same version of the
>> +         * 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 -EINVAL;
>
> -EOPNOTSUPP is a better choice I think

I will use it in the next version.

>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index 91161a2..076aa62 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -906,7 +906,21 @@ 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 the guest */
>> +        BUILD_BUG_ON(GUEST_GICV3_RDIST_REGIONS != 1);
>
> I think that the comment is enough: GUEST_GICV3_RDIST_REGIONS is already
> #define to be 1.

I would prefer to keep this compilation-time check.

If someone decides to bump the number of re-distributor without 
implementing the hyp side, it will be able to know quickly.

>> +        d->arch.vgic.rdist_count = GUEST_GICV3_RDIST_REGIONS;
>> +        d->arch.vgic.rdist_stride = GUEST_GICV3_RDIST_STRIDE;
>> +
>> +        /* The first redistributor should contain enough space for all CPUs */
>> +        BUILD_BUG_ON((GUEST_GICV3_GICR0_SIZE / GUEST_GICV3_RDIST_STRIDE) < MAX_VIRT_CPUS);
>> +        d->arch.vgic.rbase[0] = GUEST_GICV3_GICR0_BASE;
>> +        d->arch.vgic.rbase_size[0] = GUEST_GICV3_GICR0_SIZE;
>> +    }
>>
>>       d->arch.vgic.nr_lines = 0;
>>
>> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
>> index 787e93c..4b81e70 100644
>> --- a/xen/include/asm-arm/domain.h
>> +++ b/xen/include/asm-arm/domain.h
>> @@ -47,7 +47,6 @@ struct arch_domain
>>   #ifdef CONFIG_ARM_64
>>       enum domain_type type;
>>   #endif
>> -
>>       /* Virtual MMU */
>>       struct p2m_domain p2m;
>>       uint64_t vttbr;
>
> spurious change

Oops, it was a leftover after melting the 2 patches. I will drop it in 
the next version.

Regards,

-- 
Julien Grall