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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>>>> --- 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.

Jan