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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

Jan