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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.04.2023 12:34, Andrew Cooper wrote:
> On 04/04/2023 10:20 am, Jan Beulich wrote:
>> --- a/xen/arch/arm/include/asm/irq.h
>> +++ b/xen/arch/arm/include/asm/irq.h
>> @@ -52,7 +52,7 @@ struct arch_irq_desc {
>>  
>>  extern const unsigned int nr_irqs;
>>  #define nr_static_irqs NR_IRQS
>> -#define arch_hwdom_irqs(domid) NR_IRQS
>> +#define arch_hwdom_irqs(d) NR_IRQS
> 
> I know it's not your bug, but this ought to be (d, NR_IRQS) as you're
> changing it.

I can add this (with a cast to void), but I'll leave the final say to
Arm maintainers.

>> --- a/xen/arch/x86/io_apic.c
>> +++ b/xen/arch/x86/io_apic.c
>> @@ -2665,18 +2665,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)
>>  {
>>      unsigned int n = fls(num_present_cpus());
>>  
>> -    if ( !domid )
>> +    if ( is_system_domain(d) )
>> +        return PAGE_SIZE * BITS_PER_BYTE;
> 
> System domains never reach here, because ...
> 
>> +
>> +    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);
>>  
>> -    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
> 
> ... just out of context here is the system domain early exit from
> domain_create().

Of course. But that's not the path I care about; this ...

>> @@ -659,7 +659,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);
>> @@ -771,6 +771,8 @@ struct domain *domain_create(domid_t dom
>>  
>>  void __init setup_system_domains(void)
>>  {
>> +    unsigned int n;
>> +
>>      /*
>>       * Initialise our DOMID_XEN domain.
>>       * Any Xen-heap pages that we will allow to be mapped will have
>> @@ -782,6 +784,19 @@ void __init setup_system_domains(void)
>>      if ( IS_ERR(dom_xen) )
>>          panic("Failed to create d[XEN]: %ld\n", PTR_ERR(dom_xen));
>>  
>> +    /* Bound-check values passed via "extra_guest_irqs=". */
>> +    n = max(arch_hwdom_irqs(dom_xen), nr_static_irqs);

... is the one.

>> +    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;
> 
> Why the extra 32 here?

On Arm we would warn even if the command line option wasn't used. Plus
I view it as bogus to warn for any value up to the default.

>> --- a/xen/include/xen/irq.h
>> +++ b/xen/include/xen/irq.h
>> @@ -173,8 +173,9 @@ extern irq_desc_t *pirq_spin_lock_irq_de
>>  
>>  unsigned int set_desc_affinity(struct irq_desc *, const cpumask_t *);
>>  
>> +/* When passed a system domain, this returns the maximum permissible value. 
>> */
> 
> This comment is technically true, but it probably doesn't want to stay.

Why not? We (now) depend on this property.

Jan