Re: [Xen-devel] [PATCH v3 11/24] xen/arm: Let the toolstack configure the number of SPIs

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 29/01/15 12:01, Stefano Stabellini wrote:
> On Wed, 28 Jan 2015, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 28/01/15 18:26, Stefano Stabellini wrote:
>>> On Tue, 13 Jan 2015, Julien Grall wrote:
>>>> Each domain may have a different number of IRQs depending on the devices
>>>> assigned to it.
>>>>
>>>> Rather re-using the number of IRQs used by the hardwared GIC, let the
>>>> toolstack specify the number of SPIs when the domain is created. This
>>>> will avoid to waste memory.
>>>>
>>>> To calculate the number of SPIs, we assume that any IRQ given via the 
>>>> option
>>>> "irqs=" in xl is mapped 1:1 to the guest.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
>>>> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>>>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
>>>>
>>>> ---
>>>>     Changes in v3:
>>>>         - Fix typoes
>>>>         - A separate has been created to extend the DOMCTL create domain
>>>>
>>>>     Changes in v2:
>>>>         - Patch added
>>>> ---
>>>>  tools/libxc/xc_domain.c       |  1 +
>>>>  tools/libxl/libxl_arm.c       | 19 +++++++++++++++++++
>>>>  xen/arch/arm/domain.c         |  7 ++++++-
>>>>  xen/arch/arm/setup.c          |  1 +
>>>>  xen/arch/arm/vgic.c           | 10 +++++-----
>>>>  xen/include/asm-arm/domain.h  |  2 ++
>>>>  xen/include/asm-arm/setup.h   |  1 +
>>>>  xen/include/asm-arm/vgic.h    |  2 +-
>>>>  xen/include/public/arch-arm.h |  2 ++
>>>>  9 files changed, 38 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
>>>> index eebc121..eb066cf 100644
>>>> --- a/tools/libxc/xc_domain.c
>>>> +++ b/tools/libxc/xc_domain.c
>>>> @@ -67,6 +67,7 @@ int xc_domain_create(xc_interface *xch,
>>>>      /* No arch-specific configuration for now */
>>>>  #elif defined (__arm__) || defined(__aarch64__)
>>>>      config.gic_version = XEN_DOMCTL_CONFIG_GIC_DEFAULT;
>>>> +    config.nr_spis = 0;
>>>>  #else
>>>>      errno = ENOSYS;
>>>>      return -1;
>>>> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
>>>> index cddce6e..53177eb 100644
>>>> --- a/tools/libxl/libxl_arm.c
>>>> +++ b/tools/libxl/libxl_arm.c
>>>> @@ -39,6 +39,25 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>>>                                        libxl_domain_config *d_config,
>>>>                                        xc_domain_configuration_t 
>>>> *xc_config)
>>>>  {
>>>> +    uint32_t nr_spis = 0;
>>>> +    unsigned int i;
>>>> +
>>>> +    for (i = 0; i < d_config->b_info.num_irqs; i++) {
>>>> +        int irq = d_config->b_info.irqs[i];
>>>
>>> unsigned int
>>
>> I will use uint32_t.
>>
>>>> +        int spi = irq - 32;
>>
>> Same here.
>>
>>>
>>>> +        if (irq < 32)
>>>> +            continue;
>>>> +
>>>> +        if (nr_spis <= spi)
>>>> +            nr_spis = spi + 1;
>>>
>>> overflow check?
>>
>> If I use unsigned int, the overflow will go back to 0. While it won't
>> affect the code, the domain creation will fail later (unable to assign
>> the SPI).
>>
>> So is it useful to add a check here?
> 
> The maximum number of allowed spis has to be lower than UINT_MAX, right?

UINT_MAX + 1 would give 0. So it's still < UINT_MAX. When the toolstack
will try to assign the IRQ. The hypervisor will see the SPI is too high
for the vGIC. So it will reject.

Anyway, technically it's even 1024. I forgot to add this check in the
vgic code. I will do it in the next version.

Regards,

-- 
Julien Grall

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