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

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

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@linaro.org>
> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> >> Cc: Jan Beulich <jbeulich@suse.com>
> >> Cc: Wei Liu <wei.liu2@citrix.com>
> >>
> >> ---
> >>     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?