Re: [PATCH v4 for-4.19? 1/2] cmdline: document and enforce "extra_guest_irqs" upper bounds

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxx>
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> 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.