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 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/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.
> 
> Can do, sure.
> 
> > 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.
> 
> Oh, so entirely the other way around than mentioned when we talked, once
> again due to the filtering in hvm/hypercall.h that I keep forgetting. So
> in principle we could avoid the bounding for HVM. Just that right now
> extra_domU_irqs covers both PV and HVM, and would hence need splitting
> first.

Yes, we would need to split, that's why I'm OK with what you propose
here.  We can do the split as a later change.

> >>>> --- 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.  I can provide:

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

With the changes we have agreed to arch_hwdom_irqs().

Thanks, Roger.