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

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

On Tue, Jul 02, 2024 at 10:30:03AM +0200, Jan Beulich wrote:
> On 01.07.2024 17:17, Roger Pau Monné wrote:
> > On Mon, Jul 01, 2024 at 05:07:19PM +0200, Jan Beulich wrote:
> >> On 01.07.2024 15:29, Roger Pau Monné wrote:
> >>> 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/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.
> >>
> >> Which then would require making extra_{hwdom,domU}_irqs available to
> >> x86/io_apic.c, which also wouldn't be very nice. To be honest, I'd prefer
> >> to keep the logic where it is, until such time where perhaps we move pIRQ
> >> stuff wholesale to x86-only files.
> > 
> > Fine by me.
> > 
> > I think we are in agreement about what needs doing.
> 
> Hmm, after further thinking I'm not sure splitting would be the way to go.
> We should rather properly remove the concept of pIRQ from PVH, i.e. not
> only not have them visible to the kernel, but also not use e.g. ->nr_pirqs
> and ->pirq_tree internally. Then we could presumably drop the constraining
> via this command line option (documenting it as inapplicable to PVH).

Removing it from PVH would also imply removing from HVM AFAICT (unless
it's HVM with explicitly pIRQ support).  I think the main issue there
would be dealing with the change in all the interfaces that currently
take pIRQ parameters, albeit I could be wrong.  Seems like a
worthwhile improvement.

Thanks, Roger.