linux-kernel - Re: [PATCH] irqchip/msi-lib: Honor the MSI_FLAG_PCI_MSI_MASK

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <86o6vjelw2.wl-maz@kernel.org>
Date: Fri, 23 May 2025 10:06:53 +0100
From: Marc Zyngier <maz@...nel.org>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] irqchip/msi-lib: Honor the MSI_FLAG_PCI_MSI_MASK_PARENT flag

On Sat, 17 May 2025 20:59:10 +0100,
Thomas Gleixner <tglx@...utronix.de> wrote:
> 
> On Sat, May 17 2025 at 11:30, Marc Zyngier wrote:
> > +	/*
> > +	 * If the parent domain insists on being in charge of masking, obey
> > +	 * blindly. The default mask/unmask become the shutdown/enable
> > +	 * callbacks, ensuring that we correctly start/stop the interrupt.
> > + 	 * We make a point in not using the irq_disable() in order to
> > +	 * preserve the "lazy disable" behaviour.
> > +	 */
> > +	if (info->flags & MSI_FLAG_PCI_MSI_MASK_PARENT) {
> > +		chip->irq_shutdown	= chip->irq_mask;
> > +		chip->irq_enable	= chip->irq_unmask;
> 
> This is only correct, when the chip does not have dedicated
> irq_shutdown/enable callbacks.

The chip structure provided by the PCI MSI code doesn't provide such
callback, meaning that they are unused for the whole hierarchy.

> And I really hate the asymmetry of this.

So do I, but that's how the lazy disable thing currently works. Drop
the bizarre asymmetry on irq_disable, and we can make this nicely
symmetric as well.

> 
> > +		chip->irq_mask		= irq_chip_mask_parent;
> > +		chip->irq_unmask	= irq_chip_unmask_parent;
> > +	}
> 
> I'm still trying to understand, what's the actual problem is you are
> trying to solve.

I'm trying to remove some overhead from machines that don't need to
suffer from this nonsense double masking. Specially in VMs when
masking/unmasking requires *two* extremely costly exits (write +
synchronising read-back). This change reduces the overhead
significantly by only masking where it actually matters.

> MSIs are edge type interrupts, so the interrupt handling hotpath usually
> does not mask at all. The only time masking happens is when it's lazy
> disabled or during affinity changes, which is not the end of the world.

And that's part of the problem. The lazy disable ends up being way
more costly than it should when the interrupt fires during the
"disabled but not quite" phase, and in turn makes the critical section
delineated by disable_irq()/enable_irq() more expensive.

So while, as you put it, it's "not the end of the world", this seems
to me like a valuable optimisation.

Another possible improvement would be to teach the PCI code it can
still rely on masking even when the endpoint is not capable of masking
individual MSIs.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.