| Message ID | c76a60c9-c491-44bd-a6be-0199f0022e3d@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | new extra_guest_irqs adjustment | expand |
On Tue, Jul 02, 2024 at 11:52:04AM +0200, Jan Beulich wrote: > PHYSDEVOP_pirq_eoi_gmfn_v<N> accepting just a single GFN implies that no > more than 32k pIRQ-s can be used by a domain on x86. Document this upper > bound. > > To also enforce the limit, (ab)use both arch_hwdom_irqs() (changing its > parameter type) and setup_system_domains(). This is primarily to avoid > exposing the two static variables or introducing yet further arch hooks. > > While touching arch_hwdom_irqs() also mark it hwdom-init. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Acked-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Instead of passing dom_xen into arch_hwdom_irqs(), NULL could also be > used. That would make the connection to setup_system_domains() yet more > weak, though. > --- > v4: arch_hwdom_irqs() -> __hwdom_init. Local constant for upper bound in > arch_hwdom_irqs(). Re-base. > v2: Also enforce these bounds. Adjust doc to constrain the bound to x86 > only. Re-base over new earlier patch. > > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -1175,7 +1175,8 @@ common for all domUs, while the optional > is for dom0. Changing the setting for domU has no impact on dom0 and vice > versa. For example to change dom0 without changing domU, use > `extra_guest_irqs=,512`. The default value for Dom0 and an eventual separate > -hardware domain is architecture dependent. > +hardware domain is architecture dependent. The upper limit for both values on > +x86 is such that the resulting total number of IRQs can't be higher than 32768. > Note that specifying zero as domU value means zero, while for dom0 it means > to use the default. > > --- a/xen/arch/x86/io_apic.c > +++ b/xen/arch/x86/io_apic.c > @@ -2660,18 +2660,20 @@ void __init ioapic_init(void) > nr_irqs_gsi, nr_irqs - nr_irqs_gsi); > } > > -unsigned int arch_hwdom_irqs(domid_t domid) > +unsigned int __hwdom_init arch_hwdom_irqs(const struct domain *d) > { > unsigned int n = fls(num_present_cpus()); > + /* Bounded by the domain pirq EOI bitmap gfn. */ > + const unsigned int max_irqs = PAGE_SIZE * BITS_PER_BYTE; Seeing next patch where we return nr_irqs for PVH, should max_irqs be min(nr_irqs, PAGE_SIZE * BITS_PER_BYTE)? As that has the bonus of avoiding the nested min() in the expression below. > > - if ( !domid ) > - n = min(n, dom0_max_vcpus()); > - n = min(nr_irqs_gsi + n * NR_DYNAMIC_VECTORS, nr_irqs); > + if ( is_system_domain(d) ) > + return max_irqs; > > - /* Bounded by the domain pirq eoi bitmap gfn. */ > - n = min_t(unsigned int, n, PAGE_SIZE * BITS_PER_BYTE); > + if ( !d->domain_id ) > + n = min(n, dom0_max_vcpus()); > + n = min(nr_irqs_gsi + n * NR_DYNAMIC_VECTORS, min(nr_irqs, max_irqs)); > > - 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 > @@ -695,7 +695,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); > @@ -829,6 +829,24 @@ void __init setup_system_domains(void) > if ( IS_ERR(dom_xen) ) > panic("Failed to create d[XEN]: %ld\n", PTR_ERR(dom_xen)); > > +#ifdef CONFIG_HAS_PIRQ > + /* Bound-check values passed via "extra_guest_irqs=". */ > + { > + unsigned int n = max(arch_hwdom_irqs(dom_xen), nr_static_irqs); > + > + 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) ) FWIW; I fear the open-coded 32 here and the one in the extra_domU_irqs initialization can go out of sync. It might be helpful to define this as a constant close to the extra_domU_irqs definition. Thanks, Roger.
--- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -1175,7 +1175,8 @@ common for all domUs, while the optional is for dom0. Changing the setting for domU has no impact on dom0 and vice versa. For example to change dom0 without changing domU, use `extra_guest_irqs=,512`. The default value for Dom0 and an eventual separate -hardware domain is architecture dependent. +hardware domain is architecture dependent. The upper limit for both values on +x86 is such that the resulting total number of IRQs can't be higher than 32768. Note that specifying zero as domU value means zero, while for dom0 it means to use the default. --- a/xen/arch/x86/io_apic.c +++ b/xen/arch/x86/io_apic.c @@ -2660,18 +2660,20 @@ void __init ioapic_init(void) nr_irqs_gsi, nr_irqs - nr_irqs_gsi); } -unsigned int arch_hwdom_irqs(domid_t domid) +unsigned int __hwdom_init arch_hwdom_irqs(const struct domain *d) { unsigned int n = fls(num_present_cpus()); + /* Bounded by the domain pirq EOI bitmap gfn. */ + const unsigned int max_irqs = PAGE_SIZE * BITS_PER_BYTE; - if ( !domid ) - n = min(n, dom0_max_vcpus()); - n = min(nr_irqs_gsi + n * NR_DYNAMIC_VECTORS, nr_irqs); + if ( is_system_domain(d) ) + return max_irqs; - /* Bounded by the domain pirq eoi bitmap gfn. */ - n = min_t(unsigned int, n, PAGE_SIZE * BITS_PER_BYTE); + if ( !d->domain_id ) + n = min(n, dom0_max_vcpus()); + n = min(nr_irqs_gsi + n * NR_DYNAMIC_VECTORS, min(nr_irqs, max_irqs)); - 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 @@ -695,7 +695,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); @@ -829,6 +829,24 @@ void __init setup_system_domains(void) if ( IS_ERR(dom_xen) ) panic("Failed to create d[XEN]: %ld\n", PTR_ERR(dom_xen)); +#ifdef CONFIG_HAS_PIRQ + /* Bound-check values passed via "extra_guest_irqs=". */ + { + unsigned int n = max(arch_hwdom_irqs(dom_xen), nr_static_irqs); + + 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; + printk(XENLOG_WARNING "domU IRQs bounded to %u\n", n); + } + } +#endif + /* * Initialise our DOMID_IO domain. * This domain owns I/O pages that are within the range of the page_info --- a/xen/include/xen/irq.h +++ b/xen/include/xen/irq.h @@ -202,8 +202,9 @@ extern struct irq_desc *pirq_spin_lock_i unsigned int set_desc_affinity(struct irq_desc *desc, const cpumask_t *mask); +/* When passed a system domain, this returns the maximum permissible value. */ #ifndef arch_hwdom_irqs -unsigned int arch_hwdom_irqs(domid_t domid); +unsigned int arch_hwdom_irqs(const struct domain *d); #endif #ifndef arch_evtchn_bind_pirq