Re: [PATCH v2 08/21] xen/arm: Initialize the virtual GIC later

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

Hi Stefano,

Sorry, I forgot to answer to this mail.

On 06/08/14 08:35, Stefano Stabellini wrote:
> On Thu, 31 Jul 2014, Julien Grall wrote:
>> The virtual GIC may differ between each guest (number of SPIs, emulate GIC
>> version...). Those informations may not be know when the domain is created
>> (for instance in case of migration). Therefore, move the VGIC initialization
>> in a separate function.
>>
>> Introduce a new DOMCTL for ARM to configure the domain. This has to be called
>> before setting the maximum number of VCPUs for the guest.
>>
>> For the moment, only configure the number SPIs. New informations could be
>> added later.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>> Cc: Jan Beulich <jbeulich@suse.com>
>>
>> ---
>>      Changes in v2:
>>          - Patch added
>> ---
>>   tools/libxc/xc_domain.c             |   12 ++++++++++++
>>   tools/libxc/xenctrl.h               |    4 ++++
>>   tools/libxl/libxl_arch.h            |    3 +++
>>   tools/libxl/libxl_arm.c             |   19 +++++++++++++++++++
>>   tools/libxl/libxl_dom.c             |    4 ++++
>>   tools/libxl/libxl_x86.c             |    7 +++++++
>>   xen/arch/arm/domain.c               |   28 ++++++++++++++++++++++------
>>   xen/arch/arm/domctl.c               |   11 +++++++++++
>>   xen/arch/arm/setup.c                |   10 ++++++++--
>>   xen/arch/arm/vgic.c                 |   10 +++++-----
>>   xen/include/asm-arm/domain.h        |    6 ++++++
>>   xen/include/asm-arm/vgic.h          |    4 +++-
>>   xen/include/public/domctl.h         |   14 ++++++++++++++
>>   xen/xsm/flask/hooks.c               |    3 +++
>>   xen/xsm/flask/policy/access_vectors |    2 ++
>>   15 files changed, 123 insertions(+), 14 deletions(-)
>>
>> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
>> index 27fe3b6..1348905 100644
>> --- a/tools/libxc/xc_domain.c
>> +++ b/tools/libxc/xc_domain.c
>> @@ -48,6 +48,18 @@ int xc_domain_create(xc_interface *xch,
>>       return 0;
>>   }
>>
>> +#if defined(__arm__) || defined(__arch64__)
>> +int xc_domain_configure(xc_interface *xch, uint32_t domid,
>> +                        uint32_t nr_spis)
>
> Given that we'll likely add new fields to xen_domctl_configuredomain, I
> think it is best if we pass a struct domain_configure to
> xc_domain_configure instead of nr_spis. The struct we pass to
> xc_domain_configure doesn't have to be identical to struct
> xen_domctl_configuredomain, but at the moment it would be.

Ok. I will do.

[..]

>> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
>> index 45974e7..bab92b2 100644
>> --- a/xen/arch/arm/domctl.c
>> +++ b/xen/arch/arm/domctl.c
>> @@ -30,6 +30,17 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>>
>>           return p2m_cache_flush(d, s, e);
>>       }
>> +    case XEN_DOMCTL_configure_domain:
>> +    {
>> +        if ( domain_vgic_is_initialized(d) )
>> +            return -EBUSY;
>
> Given that XEN_DOMCTL_configure_domain should be called exactly once at
> domain creation, instead of introducing domain_vgic_is_initialized, I
> would make sure that XEN_DOMCTL_configure_domain hasn't been called for
> this domain before. In other words, I would make this check more
> generic, rather than vgic specific.

The VGIC initialization may have fail because there is not enough 
memory. So it would be valid, even if it's stupid, to call this DOMCTL 
twice.

domain_vgic_is_initialized is also used in vcpu_initialise to check that 
the VGIC has effectively been initialized

I would keep this check for now.

-- 
Julien Grall