Re: [RFC PATCH v5 5/7] iommu/vt-d: Fixup delivery mode of the HPET hardlockup interrupt
On Tue, May 04 2021 at 12:10, Ricardo Neri wrote:
Resending as the original one did not make it on the list because of
fatfingers. Sorry for the noise.
> In x86 there is not an IRQF_NMI flag that can be used to indicate the
There exists no IRQF_NMI flag at all. No architecture provides that.
> delivery mode when requesting an interrupt (via request_irq()). Thus,
> there is no way for the interrupt remapping driver to know and set
> the delivery mode.
There is no support for this today. So what?
> Hence, when allocating an interrupt, check if such interrupt belongs to
> the HPET hardlockup detector and fixup the delivery mode accordingly.
What?
> + /*
> + * If we find the HPET hardlockup detector irq, fixup the
> + * delivery mode.
> + */
> + if (is_hpet_irq_hardlockup_detector(info))
> + irq_cfg->delivery_mode = APIC_DELIVERY_MODE_NMI;
Again. We are not sticking some random device checks into that
code. It's wrong and I explained it to you before.
https://lore.kernel.org/lkml/alpine.deb.2.21.1906161042080.1...@nanos.tec.linutronix.de/
But I'm happy to repeat it again:
"No. This is horrible hackery violating all the layering which we carefully
put into place to avoid exactly this kind of sprinkling conditionals into
all code pathes.
With some thought the existing irqdomain hierarchy can be used to achieve
the same thing without tons of extra functions and conditionals."
So the outcome of thought and using the irqdomain hierarchy is:
Replacing an hpet specific conditional in one place with an hpet
specific conditional in a different place.
Impressive.
hpet_assign_irq(, bool nmi)
init_info(info)
...
if (nmi)
info.flags |= X86_IRQ_ALLOC_AS_NMI;
irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, )
intel_irq_remapping_alloc(..., info)
irq_domain_alloc_irq_parents(..., info)
x86_vector_alloc_irqs(..., info)
{
if (info->flags & X86_IRQ_ALLOC_AS_NMI && nr_irqs != 1)
return -EINVAL;
for (i = 0; i < nr_irqs; i++) {
if (info->flags & X86_IRQ_ALLOC_AS_NMI) {
irq_cfg_setup_nmi(apicd);
continue;
}
...
}
irq_cfg_setup_nmi() sets irq_cfg->delivery_mode and whatever is required
and everything else just works. Of course this needs a few other minor
tweaks but none of those introduces random hpet quirks all over the
place. Not convoluted enough, right?
But that solves none of other problems. Let me summarize again which
options or non-options we have:
1) Selective IPIs from NMI context cannot work
As explained in the other thread.
2) Shorthand IPI allbutself from NMI
This should work, but that obviously does not take the watchdog
cpumask into account.
Also this only works when IPI shorthand mode is enabled. See
apic_smt_update() for details.
3) Sending the IPIs from irq_work
This would solve the problem, but if the CPU which is the NMI
target is really stuck in an interrupt disabled region then the
IPIs won't be sent.
OTOH, if that's the case then the CPU which was processing the
NMI will continue to be stuck until the next NMI hits which
will detect that the CPU is stuck which is a good enough
reason to send a shorthand IPI to all CPUs ignoring the
watchdog cpumask.
Same limitation vs. shorthand mode as #2
4) Changing affinity of the HPET NMI from NMI
As we established two years ago that cannot work with interrupt
remapping
5) Changing affinity of the HPET NMI from irq_work
Same issues as #3
Anything else than #2 is just causing more problems than it solves, but
surely the NOHZ_FULL/isolation people might have opinions on this.
OTOH, as this is opt-in, anything which wants a watchdog mask which is
not the full online set, has to accept that HPET has these restrictions.
And that's exactly what I suggested two years ago:
https://lore.kernel.org/lkml/alpine.deb.2.21.1906172343120.1...@nanos.tec.linutronix.de/
"It definitely would be worthwhile to experiment with that. if we
could use shorthands (also for regular IPIs) that would be a great
improvement in general and would nicely solve that NMI issue. Beware
of the dragons though."
As a consequence of this conversation I implemented shorthand IPIs...
But I haven't seen any mentioning that this has been tried, why the
approach was not chosen or any discussion about that matter.
Not that I'm surprised.
Thanks,
tglx
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH v5 5/7] iommu/vt-d: Fixup delivery mode of the HPET hardlockup interrupt
On Wed, May 05, 2021 at 01:03:18AM +0200, Thomas Gleixner wrote:
> On Tue, May 04 2021 at 12:10, Ricardo Neri wrote:
Thank you very much for your feedback, Thomas. I am sorry it took me a
while to reply to your email. I needed to digest and research your
comments.
> > In x86 there is not an IRQF_NMI flag that can be used to indicate the
>
> There exists no IRQF_NMI flag at all. No architecture provides that.
Thank you for the clarification. I think I meant to say that there is a
request_nmi() function but AFAIK it is only used in the ARM PMU and
would not work on x86.
>
> > delivery mode when requesting an interrupt (via request_irq()). Thus,
> > there is no way for the interrupt remapping driver to know and set
> > the delivery mode.
>
> There is no support for this today. So what?
Using request_irq() plus a HPET quirk looked to me a reasonable
way to use the irqdomain hierarchy to allocate an interrupt with NMI as
the delivery mode.
>
> > Hence, when allocating an interrupt, check if such interrupt belongs to
> > the HPET hardlockup detector and fixup the delivery mode accordingly.
>
> What?
>
> > + /*
> > +* If we find the HPET hardlockup detector irq, fixup the
> > +* delivery mode.
> > +*/
> > + if (is_hpet_irq_hardlockup_detector(info))
> > + irq_cfg->delivery_mode = APIC_DELIVERY_MODE_NMI;
>
> Again. We are not sticking some random device checks into that
> code. It's wrong and I explained it to you before.
>
>
> https://lore.kernel.org/lkml/alpine.deb.2.21.1906161042080.1...@nanos.tec.linutronix.de/
>
> But I'm happy to repeat it again:
>
> "No. This is horrible hackery violating all the layering which we carefully
>put into place to avoid exactly this kind of sprinkling conditionals into
>all code pathes.
>
>With some thought the existing irqdomain hierarchy can be used to achieve
>the same thing without tons of extra functions and conditionals."
>
> So the outcome of thought and using the irqdomain hierarchy is:
>
>Replacing an hpet specific conditional in one place with an hpet
>specific conditional in a different place.
>
> Impressive.
I am sorry Thomas, I did try to make the quirk less hacky but I did not
think of the solution you provide below.
>
> hpet_assign_irq(, bool nmi)
> init_info(info)
> ...
> if (nmi)
> info.flags |= X86_IRQ_ALLOC_AS_NMI;
>
>irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, )
> intel_irq_remapping_alloc(..., info)
>irq_domain_alloc_irq_parents(..., info)
> x86_vector_alloc_irqs(..., info)
> {
>if (info->flags & X86_IRQ_ALLOC_AS_NMI && nr_irqs != 1)
> return -EINVAL;
>
>for (i = 0; i < nr_irqs; i++) {
>
> if (info->flags & X86_IRQ_ALLOC_AS_NMI) {
> irq_cfg_setup_nmi(apicd);
> continue;
> }
> ...
> }
>
> irq_cfg_setup_nmi() sets irq_cfg->delivery_mode and whatever is required
> and everything else just works. Of course this needs a few other minor
> tweaks but none of those introduces random hpet quirks all over the
> place. Not convoluted enough, right?
Thanks for the detailed demonstration! It does seem cleaner than what I
implemented.
>
> But that solves none of other problems. Let me summarize again which
> options or non-options we have:
>
> 1) Selective IPIs from NMI context cannot work
>
>As explained in the other thread.
>
> 2) Shorthand IPI allbutself from NMI
>
>This should work, but that obviously does not take the watchdog
>cpumask into account.
>
>Also this only works when IPI shorthand mode is enabled. See
>apic_smt_update() for details.
>
> 3) Sending the IPIs from irq_work
>
>This would solve the problem, but if the CPU which is the NMI
>target is really stuck in an interrupt disabled region then the
>IPIs won't be sent.
>
>OTOH, if that's the case then the CPU which was processing the
>NMI will continue to be stuck until the next NMI hits which
>will detect that the CPU is stuck which is a good enough
>reason to send a shorthand IPI to all CPUs ignoring the
>watchdog cpumask.
>
>Same limitation vs. shorthand mode as #2
>
> 4) Changing affinity of the HPET NMI from NMI
>
>As we established two years ago that cannot work with interrupt
>remapping
>
> 5) Changing affinity of the HPET NMI from irq_work
>
>Same issues as #3
>
> Anything else than #2 is just causing more problems than it solves, but
> surely the NOHZ_FULL/isolation people might have opinions on this.
>
> OTOH, as this is opt-in, anything which wants a watchdog mask which is
> not the full online set, has to accept that HPET has these restrictions.
>
> And that's exactly what I suggested two
Re: [RFC PATCH v5 5/7] iommu/vt-d: Fixup delivery mode of the HPET hardlockup interrupt
On Tue, May 04 2021 at 12:10, Ricardo Neri wrote:
> In x86 there is not an IRQF_NMI flag that can be used to indicate the
There exists no IRQF_NMI flag at all. No architecture provides that.
> delivery mode when requesting an interrupt (via request_irq()). Thus,
> there is no way for the interrupt remapping driver to know and set
> the delivery mode.
There is no support for this today. So what?
> Hence, when allocating an interrupt, check if such interrupt belongs to
> the HPET hardlockup detector and fixup the delivery mode accordingly.
What?
> + /*
> + * If we find the HPET hardlockup detector irq, fixup the
> + * delivery mode.
> + */
> + if (is_hpet_irq_hardlockup_detector(info))
> + irq_cfg->delivery_mode = APIC_DELIVERY_MODE_NMI;
Again. We are not sticking some random device checks into that
code. It's wrong and I explained it to you before.
https://lore.kernel.org/lkml/alpine.deb.2.21.1906161042080.1...@nanos.tec.linutronix.de/
But I'm happy to repeat it again:
"No. This is horrible hackery violating all the layering which we carefully
put into place to avoid exactly this kind of sprinkling conditionals into
all code pathes.
With some thought the existing irqdomain hierarchy can be used to achieve
the same thing without tons of extra functions and conditionals."
So the outcome of thought and using the irqdomain hierarchy is:
Replacing an hpet specific conditional in one place with an hpet
specific conditional in a different place.
Impressive.
hpet_assign_irq(, bool nmi)
init_info(info)
...
if (nmi)
info.flags |= X86_IRQ_ALLOC_AS_NMI;
irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, )
intel_irq_remapping_alloc(..., info)
irq_domain_alloc_irq_parents(..., info)
x86_vector_alloc_irqs(..., info)
{
if (info->flags & X86_IRQ_ALLOC_AS_NMI && nr_irqs != 1)
return -EINVAL;
for (i = 0; i < nr_irqs; i++) {
if (info->flags & X86_IRQ_ALLOC_AS_NMI) {
irq_cfg_setup_nmi(apicd);
continue;
}
...
}
irq_cfg_setup_nmi() sets irq_cfg->delivery_mode and whatever is required
and everything else just works. Of course this needs a few other minor
tweaks but none of those introduces random hpet quirks all over the
place. Not convoluted enough, right?
But that solves none of other problems. Let me summarize again which
options or non-options we have:
1) Selective IPIs from NMI context cannot work
As explained in the other thread.
2) Shorthand IPI allbutself from NMI
This should work, but that obviously does not take the watchdog
cpumask into account.
Also this only works when IPI shorthand mode is enabled. See
apic_smt_update() for details.
3) Sending the IPIs from irq_work
This would solve the problem, but if the CPU which is the NMI
target is really stuck in an interrupt disabled region then the
IPIs won't be sent.
OTOH, if that's the case then the CPU which was processing the
NMI will continue to be stuck until the next NMI hits which
will detect that the CPU is stuck which is a good enough
reason to send a shorthand IPI to all CPUs ignoring the
watchdog cpumask.
Same limitation vs. shorthand mode as #2
4) Changing affinity of the HPET NMI from NMI
As we established two years ago that cannot work with interrupt
remapping
5) Changing affinity of the HPET NMI from irq_work
Same issues as #3
Anything else than #2 is just causing more problems than it solves, but
surely the NOHZ_FULL/isolation people might have opinions on this.
OTOH, as this is opt-in, anything which wants a watchdog mask which is
not the full online set, has to accept that HPET has these restrictions.
And that's exactly what I suggested two years ago:
https://lore.kernel.org/lkml/alpine.deb.2.21.1906172343120.1...@nanos.tec.linutronix.de/
"It definitely would be worthwhile to experiment with that. if we
could use shorthands (also for regular IPIs) that would be a great
improvement in general and would nicely solve that NMI issue. Beware
of the dragons though."
As a consequence of this conversation I implemented shorthand IPIs...
But I haven't seen any mentioning that this has been tried, why the
approach was not chosen or any discussion about that matter.
Not that I'm surprised.
Thanks,
tglx
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[RFC PATCH v5 5/7] iommu/vt-d: Fixup delivery mode of the HPET hardlockup interrupt
The HPET hardlockup detector requires that the HPET timer delivers the interrupt as NMI. When interrupt remapping is disabled, this can be done by programming the HPET MSI registers directly. With interrupt remapping, it is necessary to populate an entry in the interrupt remapping table. In x86 there is not an IRQF_NMI flag that can be used to indicate the delivery mode when requesting an interrupt (via request_irq()). Thus, there is no way for the interrupt remapping driver to know and set the delivery mode. Hence, when allocating an interrupt, check if such interrupt belongs to the HPET hardlockup detector and fixup the delivery mode accordingly. Cc: Andi Kleen Cc: Borislav Petkov Cc: David Woodhouse (supporter:INTEL IOMMU (VT-d)) Cc: "Ravi V. Shankar" Cc: Ingo Molnar Cc: Jacob Pan Cc: Lu Baolu (supporter:INTEL IOMMU (VT-d)) Cc: Stephane Eranian Cc: Thomas Gleixner Cc: iommu@lists.linux-foundation.org (open list:INTEL IOMMU (VT-d)) Cc: x...@kernel.org Reviewed-by: Ashok Raj Signed-off-by: Ricardo Neri --- Changes since v4: * Introduced this patch. Changes since v3: * N/A Changes since v2: * N/A Changes since v1: * N/A --- drivers/iommu/intel/irq_remapping.c | 9 + 1 file changed, 9 insertions(+) diff --git a/drivers/iommu/intel/irq_remapping.c b/drivers/iommu/intel/irq_remapping.c index daa5df53db59..b07c68ecac01 100644 --- a/drivers/iommu/intel/irq_remapping.c +++ b/drivers/iommu/intel/irq_remapping.c @@ -18,6 +18,7 @@ #include #include #include +#include #include #include @@ -1376,6 +1377,14 @@ static int intel_irq_remapping_alloc(struct irq_domain *domain, irq_data->hwirq = (index << 16) + i; irq_data->chip_data = ird; irq_data->chip = _ir_chip; + + /* +* If we find the HPET hardlockup detector irq, fixup the +* delivery mode. +*/ + if (is_hpet_irq_hardlockup_detector(info)) + irq_cfg->delivery_mode = APIC_DELIVERY_MODE_NMI; + intel_irq_remapping_prepare_irte(ird, irq_cfg, info, index, i); irq_set_status_flags(virq + i, IRQ_MOVE_PCNTXT); } -- 2.17.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
