On 04/04/2023 10:20 am, 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.
>
> Signed-off-by: Jan Beulich <jbeul...@suse.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.
>
> On Arm the upper limit right now effectively is zero, albeit with -
> afaict - no impact if a higher value was used (and hence permitting up
> to the default of 32 is okay albeit useless). The question though is
> whether the command line option as a whole shouldn't be x86-only.
>
> Passing the domain pointer instead of the domain ID would also allow
> to return a possibly different value if sensible for PVH Dom0 (which
> presently has no access to PHYSDEVOP_pirq_eoi_gmfn_v<N> in the first
> place).
> ---
> v2: Also enforce these bounds. Adjust doc to constrain the bound to x86
> only.
>
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1130,7 +1130,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/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.
>
> struct irq_desc;
> struct irqaction;
> --- 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().
> @@ -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);
> + 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?
> + printk(XENLOG_WARNING "domU IRQs bounded to %u\n", n);
> + }
> +
> /*
> * 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
> @@ -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.
~Andrew
> #ifndef arch_hwdom_irqs
> -unsigned int arch_hwdom_irqs(domid_t);
> +unsigned int arch_hwdom_irqs(const struct domain *);
> #endif
>
> #ifndef arch_evtchn_bind_pirq
>
