[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] cmdline: document and enforce "extra_guest_irqs" upper bounds
On 04.04.2023 12:34, Andrew Cooper wrote: > On 04/04/2023 10:20 am, Jan Beulich wrote: >> --- a/xen/arch/arm/include/asm/irq.h >> +++ b/xen/arch/arm/include/asm/irq.h >> @@ -52,7 +52,7 @@ struct arch_irq_desc { >> >> extern const unsigned int nr_irqs; >> #define nr_static_irqs NR_IRQS >> -#define arch_hwdom_irqs(domid) NR_IRQS >> +#define arch_hwdom_irqs(d) NR_IRQS > > I know it's not your bug, but this ought to be (d, NR_IRQS) as you're > changing it. I can add this (with a cast to void), but I'll leave the final say to Arm maintainers. >> --- a/xen/arch/x86/io_apic.c >> +++ b/xen/arch/x86/io_apic.c >> @@ -2665,18 +2665,21 @@ void __init ioapic_init(void) >> nr_irqs_gsi, nr_irqs - nr_irqs_gsi); >> } >> >> -unsigned int arch_hwdom_irqs(domid_t domid) >> +unsigned int arch_hwdom_irqs(const struct domain *d) >> { >> unsigned int n = fls(num_present_cpus()); >> >> - if ( !domid ) >> + if ( is_system_domain(d) ) >> + return PAGE_SIZE * BITS_PER_BYTE; > > System domains never reach here, because ... > >> + >> + if ( !d->domain_id ) >> n = min(n, dom0_max_vcpus()); >> n = min(nr_irqs_gsi + n * NR_DYNAMIC_VECTORS, nr_irqs); >> >> /* Bounded by the domain pirq eoi bitmap gfn. */ >> n = min_t(unsigned int, n, PAGE_SIZE * BITS_PER_BYTE); >> >> - printk("Dom%d has maximum %u PIRQs\n", domid, n); >> + printk("%pd has maximum %u PIRQs\n", d, n); >> >> return n; >> } >> --- a/xen/common/domain.c >> +++ b/xen/common/domain.c > > ... just out of context here is the system domain early exit from > domain_create(). Of course. But that's not the path I care about; this ... >> @@ -659,7 +659,7 @@ struct domain *domain_create(domid_t dom >> d->nr_pirqs = nr_static_irqs + extra_domU_irqs; >> else >> d->nr_pirqs = extra_hwdom_irqs ? nr_static_irqs + >> extra_hwdom_irqs >> - : arch_hwdom_irqs(domid); >> + : arch_hwdom_irqs(d); >> d->nr_pirqs = min(d->nr_pirqs, nr_irqs); >> >> radix_tree_init(&d->pirq_tree); >> @@ -771,6 +771,8 @@ struct domain *domain_create(domid_t dom >> >> void __init setup_system_domains(void) >> { >> + unsigned int n; >> + >> /* >> * Initialise our DOMID_XEN domain. >> * Any Xen-heap pages that we will allow to be mapped will have >> @@ -782,6 +784,19 @@ void __init setup_system_domains(void) >> if ( IS_ERR(dom_xen) ) >> panic("Failed to create d[XEN]: %ld\n", PTR_ERR(dom_xen)); >> >> + /* Bound-check values passed via "extra_guest_irqs=". */ >> + n = max(arch_hwdom_irqs(dom_xen), nr_static_irqs); ... is the one. >> + if ( extra_hwdom_irqs > n - nr_static_irqs ) >> + { >> + extra_hwdom_irqs = n - nr_static_irqs; >> + printk(XENLOG_WARNING "hwdom IRQs bounded to %u\n", n); >> + } >> + if ( extra_domU_irqs > max(32U, n - nr_static_irqs) ) >> + { >> + extra_domU_irqs = n - nr_static_irqs; > > Why the extra 32 here? On Arm we would warn even if the command line option wasn't used. Plus I view it as bogus to warn for any value up to the default. >> --- a/xen/include/xen/irq.h >> +++ b/xen/include/xen/irq.h >> @@ -173,8 +173,9 @@ extern irq_desc_t *pirq_spin_lock_irq_de >> >> unsigned int set_desc_affinity(struct irq_desc *, const cpumask_t *); >> >> +/* When passed a system domain, this returns the maximum permissible value. >> */ > > This comment is technically true, but it probably doesn't want to stay. Why not? We (now) depend on this property. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |