[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 4 Apr 2023 12:40:24 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=xBCXfjvZiFQaKwerBMfqcds06QVNrK3ird1Ze6TJ1/4=; b=UwvzoLctm8iTZuLOqpTQgD4RH2FxNbqHK90nA9mVZWP5Z09/D3q/a3Exuj85Z0ZklgOy6EyVQ6e82LuihipArU1ifguVCG7sI0hQGDg8Ef0lfBTxZOBFLs0CTkt5TeH+8l7nNxjWeeozo0P31oqf2ayFPsVD8IO894xqVZ2Dao7677feYzgEEJQEARB3mdmiYa7CRZQR9DzdhNgv7quYLJKlxnc3AiqOTzW2Y+1fde2nWpGetX372PL1VcABlYsCZ8tLGO80tNQGNxl5mznRWXnFwoPCftv9VFGnQuHfASlfEp2GEnO7cR5G76oDcTsLPrWWgG75diVRINzWBC4rBQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=fI7x1J62ojqi97gy4+fTaJUsgb2OhfGDGOWxGAdMiXhREY4SgsbRMbgY01Ea+i5xTwPdJJvdzlw+X66rcEZK8rShpa0/S8P4R4jJf+knpEvkmGtzyqZKnG5n8NSNNabTO35FSthhaIM7/DtZSLRU0DN1fK/oIqskBXQRKQcLPaFhNM39zH3t27WC1v78V3ET402J5kxlCsRUnu7Cz8AWwuTJ43uoPnDxKsIuZg70ayKV0xkEyCKIWAHfXR345WpBeoPBCYDQuWfeIkDFKup8QTmcIJ5Zq/VtmO4PggPNZYKd/2GXArdDjcJua8XPwFwy3jDVP3SGM5faF3JFn3oPSA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Tue, 04 Apr 2023 10:40:37 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.