Re: [PATCH v3 3/3] cmdline: document and enforce "extra_guest_irqs" upper bounds

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

On Mon, Jul 01, 2024 at 12:40:35PM +0200, Jan Beulich wrote:
> On 01.07.2024 11:55, Roger Pau Monné wrote:
> > On Thu, Jul 27, 2023 at 09:38:55AM +0200, Jan Beulich wrote:
> >> --- a/xen/arch/x86/io_apic.c
> >> +++ b/xen/arch/x86/io_apic.c
> >> @@ -2663,18 +2663,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)
> > 
> > While at it, should this be __hwdom_init?
> 
> It indeed can be, so I've done this for v4.
> 
> > I'm fine with changing the function to take a domain parameter...
> > 
> >>  {
> >>      unsigned int n = fls(num_present_cpus());
> >>  
> >> -    if ( !domid )
> >> +    if ( is_system_domain(d) )
> >> +        return PAGE_SIZE * BITS_PER_BYTE;
> > 
> > ... but why do we need a function call just to get a constant value?
> > Wouldn't this better be a define in a header?
> 
> Would be an option, but would result in parts of the logic living is
> distinct places.
> 
> >> +
> >> +    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);
> > 
> > So that could also use the same constant here?

I would have a slight preference for PAGE_SIZE * BITS_PER_BYTE being
defined inside of this function as:

/* Bounded by the domain pirq eoi bitmap gfn. */
const unsigned int max_irqs = PAGE_SIZE * BITS_PER_BYTE;

Or similar for clarity purposes.

While at it, I've noticed that PHYSDEVOP_pirq_eoi_gmfn_v{1,2} is not
available to HVM guests (not even when exposing PIRQ support) and
hence I wonder if we should special case PVH dom0, but maybe it's best
to deal with this properly rather than hacking something special
just for PVH dom0.  At the end of the day the current limit is high
enough to not cause issues on current systems I would expect.

> >> -    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
> >> @@ -693,7 +693,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);
> >> @@ -819,6 +819,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
> > 
> > IMO this is kind of a weird placement. Wouldn't this be more naturally
> > handled in parse_extra_guest_irqs()?
> 
> Indeed it is and yes it would, but no, it can't. We shouldn't rely on
> the particular behavior of arch_hwdom_irqs(), and in the general case
> we can't call it as early as when command line arguments are parsed. I
> couldn't think of a neater way of doing this, and it not being pretty
> is why I'm saying "(ab)use" in the description.

I see, nr_static_irqs is an alias of nr_irqs_gsi, which is not properly
set by the time we evaluate command line arguments.

My only possible suggestion would be to do it as a presmp initcall,
and define/register such initcall for x86 only, the only benefit would
be that such inicall could be defined in the same translation unit as
arch_hwdom_irqs() then.

Thanks, Roger.