| Message ID | 1421159133-31526-12-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
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? [..] >> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h >> index d302fc9..101b4e9 100644 >> --- a/xen/include/asm-arm/domain.h >> +++ b/xen/include/asm-arm/domain.h >> @@ -121,6 +121,8 @@ struct arch_domain >> unsigned int evtchn_irq; >> } __cacheline_aligned; >> >> +#define domain_is_configured(d) ((d)->arch.is_configured) > > This is unused. Moreover it cannot find "is_configured" anywhere. Left-over of a previous version. Sorry. Regards,
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@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? 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,
On 20/02/15 16:08, Ian Campbell wrote: > On Wed, 2015-01-28 at 18:26 +0000, Stefano Stabellini wrote: > >>> + int spi = irq - 32; >> >> unsigned int > > and underflow? No because there is a check (irq < 32) before using the variable spi. It was more convenient to initialize it directly. Regards,
Hi Ian, On 20/02/15 16:23, Ian Campbell wrote: > On Tue, 2015-01-13 at 14:25 +0000, 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 > ^than and "hardware" (although "physical" might be better) > >> toolstack specify the number of SPIs when the domain is created. This >> will avoid to waste memory. > > "will avoid wasting 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. > > I don't see any xl changes here, and I think all that really matters is > the semantics of the libxl_domain_build_info.irqs[] array, which > currently I think can only express 1:1 mappings anyway. > So: > > "To calculate the number of SPIs, we take advantage of the fact that the > libxl interface can only expose a 1:1 mapping and look for the largest > SPI in the list" or something. I will use this. Regards,
Hi Ian, On 23/02/15 15:22, Ian Campbell wrote: > On Fri, 2015-02-20 at 17:29 +0000, Julien Grall wrote: >> On 20/02/15 16:08, Ian Campbell wrote: >>> On Wed, 2015-01-28 at 18:26 +0000, Stefano Stabellini wrote: >>> >>>>> + int spi = irq - 32; >>>> >>>> unsigned int >>> >>> and underflow? >> >> No because there is a check (irq < 32) before using the variable spi. >> It was more convenient to initialize it directly. > > You'd have to be sure that "irq - 32" wasn't undefined behaviour then, > or the compiler can decide to omit the rest of the function, or perhaps > just that second check, because things are undefined from the > initialiser onwards. > > I think with unsigned you are probably ok. I will move the (irq - 32) after the check for safety. Regards,
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]; + int spi = irq - 32; + + if (irq < 32) + continue; + + if (nr_spis <= spi) + nr_spis = spi + 1; + } + + LOG(DEBUG, "Configure the domain"); + + xc_config->nr_spis = nr_spis; + LOG(DEBUG, " - Allocate %u SPIs", nr_spis); + xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_DEFAULT; return 0; diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 2473b10..6e56665 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -560,10 +560,15 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, } config->gic_version = gic_version; + /* Sanity check on the number of SPIs */ + rc = -EINVAL; + if ( config->nr_spis > (gic_number_lines() - 32) ) + goto fail; + if ( (rc = gicv_setup(d)) != 0 ) goto fail; - if ( (rc = domain_vgic_init(d)) != 0 ) + if ( (rc = domain_vgic_init(d, config->nr_spis)) != 0 ) goto fail; if ( (rc = domain_vtimer_init(d)) != 0 ) diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 18227f6..b28a708 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -815,6 +815,7 @@ void __init start_xen(unsigned long boot_phys_offset, /* Create initial domain 0. */ /* The vGIC for DOM0 is exactly emulated the hardware GIC */ config.gic_version = XEN_DOMCTL_CONFIG_GIC_DEFAULT; + config.nr_spis = gic_number_lines() - 32; dom0 = domain_create(0, 0, 0, &config); if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) ) diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index c915670..fc8a270 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -67,16 +67,16 @@ static void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq) p->irq = virq; } -int domain_vgic_init(struct domain *d) +int domain_vgic_init(struct domain *d, unsigned int nr_spis) { int i; d->arch.vgic.ctlr = 0; - if ( is_hardware_domain(d) ) - d->arch.vgic.nr_spis = gic_number_lines() - 32; - else - d->arch.vgic.nr_spis = 0; /* We don't need SPIs for the guest */ + /* The number of SPIs has to be aligned to 32 see + * GICD_TYPER.ITLinesNumber definition + */ + d->arch.vgic.nr_spis = ROUNDUP(nr_spis, 32); switch ( gic_hw_version() ) { diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index d302fc9..101b4e9 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -121,6 +121,8 @@ struct arch_domain unsigned int evtchn_irq; } __cacheline_aligned; +#define domain_is_configured(d) ((d)->arch.is_configured) + struct arch_vcpu { struct { diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h index ba5a67d..254cc17 100644 --- a/xen/include/asm-arm/setup.h +++ b/xen/include/asm-arm/setup.h @@ -54,6 +54,7 @@ void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len); void arch_get_xen_caps(xen_capabilities_info_t *info); int construct_dom0(struct domain *d); +int configure_dom0(struct domain *d); void discard_initial_modules(void); diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index 1cd7808..e97a5eb 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -177,7 +177,7 @@ enum gic_sgi_mode; #define vgic_num_irqs(d) ((d)->arch.vgic.nr_spis + 32) -extern int domain_vgic_init(struct domain *d); +extern int domain_vgic_init(struct domain *d, unsigned int nr_spis); extern void domain_vgic_free(struct domain *d); extern int vcpu_vgic_init(struct vcpu *v); extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq); diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h index 4c1b9f9..45d3b1f 100644 --- a/xen/include/public/arch-arm.h +++ b/xen/include/public/arch-arm.h @@ -320,6 +320,8 @@ typedef uint64_t xen_callback_t; struct xen_arch_domainconfig { /* IN/OUT */ uint8_t gic_version; + /* IN */ + uint32_t nr_spis; }; #endif
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(-)