[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 3/3] cmdline: document and enforce "extra_guest_irqs" upper bounds
On Tue, Jul 02, 2024 at 10:30:03AM +0200, Jan Beulich wrote: > On 01.07.2024 17:17, Roger Pau Monné wrote: > > On Mon, Jul 01, 2024 at 05:07:19PM +0200, Jan Beulich wrote: > >> On 01.07.2024 15:29, Roger Pau Monné wrote: > >>> On Mon, Jul 01, 2024 at 12:40:35PM +0200, Jan Beulich wrote: > >>>> 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/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. > >>> > >>> I see, nr_static_irqs is an alias of nr_irqs_gsi, which is not properly > >>> set by the time we evaluate command line arguments. > >>> > >>> My only possible suggestion would be to do it as a presmp initcall, > >>> and define/register such initcall for x86 only, the only benefit would > >>> be that such inicall could be defined in the same translation unit as > >>> arch_hwdom_irqs() then. > >> > >> Which then would require making extra_{hwdom,domU}_irqs available to > >> x86/io_apic.c, which also wouldn't be very nice. To be honest, I'd prefer > >> to keep the logic where it is, until such time where perhaps we move pIRQ > >> stuff wholesale to x86-only files. > > > > Fine by me. > > > > I think we are in agreement about what needs doing. > > Hmm, after further thinking I'm not sure splitting would be the way to go. > We should rather properly remove the concept of pIRQ from PVH, i.e. not > only not have them visible to the kernel, but also not use e.g. ->nr_pirqs > and ->pirq_tree internally. Then we could presumably drop the constraining > via this command line option (documenting it as inapplicable to PVH). Removing it from PVH would also imply removing from HVM AFAICT (unless it's HVM with explicitly pIRQ support). I think the main issue there would be dealing with the change in all the interfaces that currently take pIRQ parameters, albeit I could be wrong. Seems like a worthwhile improvement. Thanks, Roger.
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |