[PATCH v4 for-4.19? 0/2] new extra_guest_irqs adjustment

[PATCH v4 for-4.19? 0/2] new extra_guest_irqs adjustment

[Jan Beulich]

New patch 2 addresses a concern raised by Roger when reviewing what is
now patch 1.

1: cmdline: document and enforce "extra_guest_irqs" upper bounds
2: cmdline: "extra_guest_irqs" is inapplicable to PVH

Jan

[PATCH v4 for-4.19? 1/2] cmdline: document and enforce "extra_guest_irqs" upper bounds

[Jan Beulich]

PHYSDEVOP_pirq_eoi_gmfn_v<N> accepting just a single GFN implies that no
more than 32k pIRQ-s can be used by a domain on x86. Document this upper
bound.

To also enforce the limit, (ab)use both arch_hwdom_irqs() (changing its
parameter type) and setup_system_domains(). This is primarily to avoid
exposing the two static variables or introducing yet further arch hooks.

While touching arch_hwdom_irqs() also mark it hwdom-init.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
---
Instead of passing dom_xen into arch_hwdom_irqs(), NULL could also be
used. That would make the connection to setup_system_domains() yet more
weak, though.
---
v4: arch_hwdom_irqs() -> __hwdom_init. Local constant for upper bound in
    arch_hwdom_irqs(). Re-base.
v2: Also enforce these bounds. Adjust doc to constrain the bound to x86
    only. Re-base over new earlier patch.

--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1175,7 +1175,8 @@ common for all domUs, while the optional
 is for dom0.  Changing the setting for domU has no impact on dom0 and vice
 versa.  For example to change dom0 without changing domU, use
 `extra_guest_irqs=,512`.  The default value for Dom0 and an eventual separate
-hardware domain is architecture dependent.
+hardware domain is architecture dependent.  The upper limit for both values on
+x86 is such that the resulting total number of IRQs can't be higher than 32768.
 Note that specifying zero as domU value means zero, while for dom0 it means
 to use the default.
 
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -2660,18 +2660,20 @@ void __init ioapic_init(void)
            nr_irqs_gsi, nr_irqs - nr_irqs_gsi);
 }
 
-unsigned int arch_hwdom_irqs(domid_t domid)
+unsigned int __hwdom_init arch_hwdom_irqs(const struct domain *d)
 {
     unsigned int n = fls(num_present_cpus());
+    /* Bounded by the domain pirq EOI bitmap gfn. */
+    const unsigned int max_irqs = PAGE_SIZE * BITS_PER_BYTE;
 
-    if ( !domid )
-        n = min(n, dom0_max_vcpus());
-    n = min(nr_irqs_gsi + n * NR_DYNAMIC_VECTORS, nr_irqs);
+    if ( is_system_domain(d) )
+        return max_irqs;
 
-    /* Bounded by the domain pirq eoi bitmap gfn. */
-    n = min_t(unsigned int, n, PAGE_SIZE * BITS_PER_BYTE);
+    if ( !d->domain_id )
+        n = min(n, dom0_max_vcpus());
+    n = min(nr_irqs_gsi + n * NR_DYNAMIC_VECTORS, min(nr_irqs, max_irqs));
 
-    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
@@ -695,7 +695,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);
@@ -829,6 +829,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
+
     /*
      * Initialise our DOMID_IO domain.
      * This domain owns I/O pages that are within the range of the page_info
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -202,8 +202,9 @@ extern struct irq_desc *pirq_spin_lock_i
 
 unsigned int set_desc_affinity(struct irq_desc *desc, const cpumask_t *mask);
 
+/* When passed a system domain, this returns the maximum permissible value. */
 #ifndef arch_hwdom_irqs
-unsigned int arch_hwdom_irqs(domid_t domid);
+unsigned int arch_hwdom_irqs(const struct domain *d);
 #endif
 
 #ifndef arch_evtchn_bind_pirq

[PATCH v4 for-4.19? 2/2] cmdline: "extra_guest_irqs" is inapplicable to PVH

[Jan Beulich]

PVH in particular has no (externally visible) notion of pIRQ-s. Mention
that in the description of the respective command line option and have
arch_hwdom_irqs() also reflect this (thus suppressing the log message
there as well, as being pretty meaningless in this case anyway).

Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Since the EOI map physdevop-s aren't available to HVM no matter whether
the PVH sub-flavor is meant, the condition could in principle be without
the has_pirq() part. Just that there really isn't any "pure HVM" Dom0.
---
v4: New.

--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1178,7 +1178,8 @@ versa.  For example to change dom0 witho
 hardware domain is architecture dependent.  The upper limit for both values on
 x86 is such that the resulting total number of IRQs can't be higher than 32768.
 Note that specifying zero as domU value means zero, while for dom0 it means
-to use the default.
+to use the default.  Note further that the Dom0 setting has no useful meaning
+for the PVH case; use of the option may have an adverse effect there, though.
 
 ### ext_regions (Arm)
 > `= <boolean>`
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -2669,6 +2669,10 @@ unsigned int __hwdom_init arch_hwdom_irq
     if ( is_system_domain(d) )
         return max_irqs;
 
+    /* PVH isn't constrained by the EOI map. */
+    if ( is_hvm_domain(d) && !has_pirq(d) )
+        return nr_irqs;
+
     if ( !d->domain_id )
         n = min(n, dom0_max_vcpus());
     n = min(nr_irqs_gsi + n * NR_DYNAMIC_VECTORS, min(nr_irqs, max_irqs));

Re: [PATCH v4 for-4.19? 1/2] cmdline: document and enforce "extra_guest_irqs" upper bounds

[Roger Pau Monné]

On Tue, Jul 02, 2024 at 11:52:04AM +0200, Jan Beulich wrote:
> PHYSDEVOP_pirq_eoi_gmfn_v<N> accepting just a single GFN implies that no
> more than 32k pIRQ-s can be used by a domain on x86. Document this upper
> bound.
> 
> To also enforce the limit, (ab)use both arch_hwdom_irqs() (changing its
> parameter type) and setup_system_domains(). This is primarily to avoid
> exposing the two static variables or introducing yet further arch hooks.
> 
> While touching arch_hwdom_irqs() also mark it hwdom-init.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Instead of passing dom_xen into arch_hwdom_irqs(), NULL could also be
> used. That would make the connection to setup_system_domains() yet more
> weak, though.
> ---
> v4: arch_hwdom_irqs() -> __hwdom_init. Local constant for upper bound in
>     arch_hwdom_irqs(). Re-base.
> v2: Also enforce these bounds. Adjust doc to constrain the bound to x86
>     only. Re-base over new earlier patch.
> 
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1175,7 +1175,8 @@ common for all domUs, while the optional
>  is for dom0.  Changing the setting for domU has no impact on dom0 and vice
>  versa.  For example to change dom0 without changing domU, use
>  `extra_guest_irqs=,512`.  The default value for Dom0 and an eventual separate
> -hardware domain is architecture dependent.
> +hardware domain is architecture dependent.  The upper limit for both values on
> +x86 is such that the resulting total number of IRQs can't be higher than 32768.
>  Note that specifying zero as domU value means zero, while for dom0 it means
>  to use the default.
>  
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -2660,18 +2660,20 @@ void __init ioapic_init(void)
>             nr_irqs_gsi, nr_irqs - nr_irqs_gsi);
>  }
>  
> -unsigned int arch_hwdom_irqs(domid_t domid)
> +unsigned int __hwdom_init arch_hwdom_irqs(const struct domain *d)
>  {
>      unsigned int n = fls(num_present_cpus());
> +    /* Bounded by the domain pirq EOI bitmap gfn. */
> +    const unsigned int max_irqs = PAGE_SIZE * BITS_PER_BYTE;

Seeing next patch where we return nr_irqs for PVH, should max_irqs be
min(nr_irqs, PAGE_SIZE * BITS_PER_BYTE)?

As that has the bonus of avoiding the nested min() in the expression
below.

>  
> -    if ( !domid )
> -        n = min(n, dom0_max_vcpus());
> -    n = min(nr_irqs_gsi + n * NR_DYNAMIC_VECTORS, nr_irqs);
> +    if ( is_system_domain(d) )
> +        return max_irqs;
>  
> -    /* Bounded by the domain pirq eoi bitmap gfn. */
> -    n = min_t(unsigned int, n, PAGE_SIZE * BITS_PER_BYTE);
> +    if ( !d->domain_id )
> +        n = min(n, dom0_max_vcpus());
> +    n = min(nr_irqs_gsi + n * NR_DYNAMIC_VECTORS, min(nr_irqs, max_irqs));
>  
> -    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
> @@ -695,7 +695,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);
> @@ -829,6 +829,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) )

FWIW; I fear the open-coded 32 here and the one in the
extra_domU_irqs initialization can go out of sync.  It might be
helpful to define this as a constant close to the extra_domU_irqs
definition.

Thanks, Roger.

Re: [PATCH v4 for-4.19? 2/2] cmdline: "extra_guest_irqs" is inapplicable to PVH

[Roger Pau Monné]

On Tue, Jul 02, 2024 at 11:52:38AM +0200, Jan Beulich wrote:
> PVH in particular has no (externally visible) notion of pIRQ-s. Mention
> that in the description of the respective command line option and have
> arch_hwdom_irqs() also reflect this (thus suppressing the log message
> there as well, as being pretty meaningless in this case anyway).
> 
> Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Since the EOI map physdevop-s aren't available to HVM no matter whether
> the PVH sub-flavor is meant, the condition could in principle be without
> the has_pirq() part. Just that there really isn't any "pure HVM" Dom0.
> ---
> v4: New.
> 
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1178,7 +1178,8 @@ versa.  For example to change dom0 witho
>  hardware domain is architecture dependent.  The upper limit for both values on
>  x86 is such that the resulting total number of IRQs can't be higher than 32768.
>  Note that specifying zero as domU value means zero, while for dom0 it means
> -to use the default.
> +to use the default.  Note further that the Dom0 setting has no useful meaning
> +for the PVH case; use of the option may have an adverse effect there, though.

I would maybe remove the has_pirq() check and just mention in the
comment added ahead of the is_hvm_domain() check that PVH/HVM guests
never have access to the PHYSDEVOP_pirq_eoi_gmfn_v{1,2} hypercall,
regardless of whether XENFEAT_hvm_pirqs is exposed.

Would that be OK with you?

Thanks, Roger.

Re: [PATCH v4 for-4.19? 2/2] cmdline: "extra_guest_irqs" is inapplicable to PVH

[Jan Beulich]

On 03.07.2024 09:51, Roger Pau Monné wrote:
> On Tue, Jul 02, 2024 at 11:52:38AM +0200, Jan Beulich wrote:
>> PVH in particular has no (externally visible) notion of pIRQ-s. Mention
>> that in the description of the respective command line option and have
>> arch_hwdom_irqs() also reflect this (thus suppressing the log message
>> there as well, as being pretty meaningless in this case anyway).
>>
>> Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Since the EOI map physdevop-s aren't available to HVM no matter whether
>> the PVH sub-flavor is meant, the condition could in principle be without
>> the has_pirq() part. Just that there really isn't any "pure HVM" Dom0.
>> ---
>> v4: New.
>>
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -1178,7 +1178,8 @@ versa.  For example to change dom0 witho
>>  hardware domain is architecture dependent.  The upper limit for both values on
>>  x86 is such that the resulting total number of IRQs can't be higher than 32768.
>>  Note that specifying zero as domU value means zero, while for dom0 it means
>> -to use the default.
>> +to use the default.  Note further that the Dom0 setting has no useful meaning
>> +for the PVH case; use of the option may have an adverse effect there, though.
> 
> I would maybe remove the has_pirq() check and just mention in the
> comment added ahead of the is_hvm_domain() check that PVH/HVM guests
> never have access to the PHYSDEVOP_pirq_eoi_gmfn_v{1,2} hypercall,
> regardless of whether XENFEAT_hvm_pirqs is exposed.
> 
> Would that be OK with you?

Okay-ish. That's why I had the post-commit-message remark on this very aspect.

Jan

Re: [PATCH v4 for-4.19? 2/2] cmdline: "extra_guest_irqs" is inapplicable to PVH

[Roger Pau Monné]

On Wed, Jul 03, 2024 at 10:00:51AM +0200, Jan Beulich wrote:
> On 03.07.2024 09:51, Roger Pau Monné wrote:
> > On Tue, Jul 02, 2024 at 11:52:38AM +0200, Jan Beulich wrote:
> >> PVH in particular has no (externally visible) notion of pIRQ-s. Mention
> >> that in the description of the respective command line option and have
> >> arch_hwdom_irqs() also reflect this (thus suppressing the log message
> >> there as well, as being pretty meaningless in this case anyway).
> >>
> >> Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> Since the EOI map physdevop-s aren't available to HVM no matter whether
> >> the PVH sub-flavor is meant, the condition could in principle be without
> >> the has_pirq() part. Just that there really isn't any "pure HVM" Dom0.
> >> ---
> >> v4: New.
> >>
> >> --- a/docs/misc/xen-command-line.pandoc
> >> +++ b/docs/misc/xen-command-line.pandoc
> >> @@ -1178,7 +1178,8 @@ versa.  For example to change dom0 witho
> >>  hardware domain is architecture dependent.  The upper limit for both values on
> >>  x86 is such that the resulting total number of IRQs can't be higher than 32768.
> >>  Note that specifying zero as domU value means zero, while for dom0 it means
> >> -to use the default.
> >> +to use the default.  Note further that the Dom0 setting has no useful meaning
> >> +for the PVH case; use of the option may have an adverse effect there, though.
> > 
> > I would maybe remove the has_pirq() check and just mention in the
> > comment added ahead of the is_hvm_domain() check that PVH/HVM guests
> > never have access to the PHYSDEVOP_pirq_eoi_gmfn_v{1,2} hypercall,
> > regardless of whether XENFEAT_hvm_pirqs is exposed.
> > 
> > Would that be OK with you?
> 
> Okay-ish. That's why I had the post-commit-message remark on this very aspect.

I think adding the has_pirq() check is confusing, as the option is not
available to PVH.  Even if it was available it won't change the fact
that PHYSDEVOP_pirq_eoi_gmfn_v{1,2} hypercall is not reachable.

Thanks, Roger.