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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

>  I can provide:
> 
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> With the changes we have agreed to arch_hwdom_irqs().

Thanks; changes done locally.

Jan