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.
Signed-off-by: Jan Beulich <jbeulich@suse.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.
On Arm the upper limit right now effectively is zero, albeit with -
afaict - no impact if a higher value was used (and hence permitting up
to the default of 32 is okay albeit useless). The question though is
whether the command line option as a whole shouldn't be x86-only.
Passing the domain pointer instead of the domain ID would also allow
to return a possibly different value if sensible for PVH Dom0 (which
presently has no access to PHYSDEVOP_pirq_eoi_gmfn_v<N> in the first
place).
---
v2: Also enforce these bounds. Adjust doc to constrain the bound to x86
only.
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1130,7 +1130,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/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
struct irq_desc;
struct irqaction;
--- 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;
+
+ 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
@@ -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);
+ 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);
+ }
+
/*
* 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
@@ -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. */
#ifndef arch_hwdom_irqs
-unsigned int arch_hwdom_irqs(domid_t);
+unsigned int arch_hwdom_irqs(const struct domain *);
#endif
#ifndef arch_evtchn_bind_pirqHi Jan, On 04/04/2023 10:20, 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. > > Signed-off-by: Jan Beulich <jbeulich@suse.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. > > On Arm the upper limit right now effectively is zero, albeit with - > afaict - no impact if a higher value was used (and hence permitting up > to the default of 32 is okay albeit useless). The question though is > whether the command line option as a whole shouldn't be x86-only. AFAIK, ->nr_pirq is not used at all on Arm because we don't have any concept of PIRQs. So I think it would be fine to move the command line option to x86-only (assuming this will not be needed on RISC-V). Cheers, -- Julien Grall
On 04/04/2023 10:20 am, 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.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.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.
>
> On Arm the upper limit right now effectively is zero, albeit with -
> afaict - no impact if a higher value was used (and hence permitting up
> to the default of 32 is okay albeit useless). The question though is
> whether the command line option as a whole shouldn't be x86-only.
>
> Passing the domain pointer instead of the domain ID would also allow
> to return a possibly different value if sensible for PVH Dom0 (which
> presently has no access to PHYSDEVOP_pirq_eoi_gmfn_v<N> in the first
> place).
> ---
> v2: Also enforce these bounds. Adjust doc to constrain the bound to x86
> only.
>
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1130,7 +1130,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/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.
>
> struct irq_desc;
> struct irqaction;
> --- 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().
> @@ -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);
> + 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?
> + printk(XENLOG_WARNING "domU IRQs bounded to %u\n", n);
> + }
> +
> /*
> * 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
> @@ -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.
~Andrew
> #ifndef arch_hwdom_irqs
> -unsigned int arch_hwdom_irqs(domid_t);
> +unsigned int arch_hwdom_irqs(const struct domain *);
> #endif
>
> #ifndef arch_evtchn_bind_pirq
>
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
On 04.04.2023 12:40, Jan Beulich wrote:
> 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.
Arm maintainers,
may I ask for some kind of statement here? I'd be happy to follow
Andrew's request, but I don't want to then end up with an "unrelated
change" objection.
Thanks, Jan
Hi Jan,
On 03/05/2023 11:43, Jan Beulich wrote:
> On 04.04.2023 12:40, Jan Beulich wrote:
>> 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.
>
> Arm maintainers,
>
> may I ask for some kind of statement here? I'd be happy to follow
> Andrew's request, but I don't want to then end up with an "unrelated
> change" objection.
I am OK so long it is mentioned in the commit message.
Cheers,
--
Julien Grall
© 2016 - 2024 Red Hat, Inc.