linux-kernel - Re: [Patch Part2 v3 15/24] x86, MSI: Use hierarchy irqdomain to manage MSI interrupts

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] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 29 Oct 2014 16:48:24 +0800
From:	Jiang Liu <jiang.liu@...ux.intel.com>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Randy Dunlap <rdunlap@...radead.org>,
	Yinghai Lu <yinghai@...nel.org>,
	Borislav Petkov <bp@...en8.de>,
	Grant Likely <grant.likely@...aro.org>,
	Marc Zyngier <marc.zyngier@....com>,
	Yingjoe Chen <yingjoe.chen@...iatek.com>, x86@...nel.org,
	Joerg Roedel <joro@...tes.org>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Tony Luck <tony.luck@...el.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
	linux-acpi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	iommu@...ts.linux-foundation.org
Subject: Re: [Patch Part2 v3 15/24] x86, MSI: Use hierarchy irqdomain to manage
 MSI interrupts

On 2014/10/29 5:37, Thomas Gleixner wrote:
> On Tue, 28 Oct 2014, Jiang Liu wrote:
>> +static int msi_set_affinity(struct irq_data *data, const struct cpumask *mask,
>> +			    bool force)
>> +{
>> +	struct irq_data *parent = data->parent_data;
>> +	int ret;
>>  
>> -	msg.data &= ~MSI_DATA_VECTOR_MASK;
>> -	msg.data |= MSI_DATA_VECTOR(cfg->vector);
>> -	msg.address_lo &= ~MSI_ADDR_DEST_ID_MASK;
>> -	msg.address_lo |= MSI_ADDR_DEST_ID(dest);
>> +	ret = parent->chip->irq_set_affinity(parent, mask, force);
>> +	/* No need to reprogram MSI registers if interrupt is remapped */
>> +	if (ret >= 0 && !msi_irq_remapped(data)) {
>> +		struct msi_msg msg;
>>  
>> -	__write_msi_msg(data->msi_desc, &msg);
>> +		__get_cached_msi_msg(data->msi_desc, &msg);
>> +		msi_update_msg(&msg, data);
>> +		__write_msi_msg(data->msi_desc, &msg);
>> +	}
> 
> I'm not too happy about the msi_irq_remapped() conditional here. It
> violates the whole concept of domain stacking somewhat.
> 
> A better separation would be to add a callback to the irq chip:
> 
>   	void (*irq_write_msi_msg)(struct irq_data *data, struct msi_desc *msi_desc, bool cached);
> 
> and change this code to:
> 
>     	if (ret >= 0)
> 	   	parent->chip->irq_write_msi_msg(parent, data->msi-desc, true);
>   
Hi Thomas,
	Thanks for your great suggestion and I have worked out a draft
patch to achieve what you want:)
	I have made following changes to irq core to get rid of remapped
irq logic from msi.c:
1) Add IRQ_SET_MASK_OK_DONE in addition to IRQ_SET_MASK_OK and
IRQ_SET_MASK_OK_NOCOPY. IRQ_SET_MASK_OK_DONE is the same as
IRQ_SET_MASK_OK for irq core and indicates to stacked irqchip that
parent irqchips have done all work and skip any handling in descendant
irqchips.
2) Add int (*irq_compose_msi_msg)(struct irq_data *data, struct msi_msg
*msg) into struct irq_chip. I'm still hesitating to return void or int
here. By returning void, irq_chip_compose_msi_msg() will be simpler,
but it loses flexibility.

With above changes to core, we could remove all knowledge of irq
remapping from msi.c and the irq remapping interfaces get simpler too.
Please refer to following patch for details. The patch passes basic
booting tests with irq remapping enabled. If it's OK, I will fold
it into the patch set.

IOAPIC runs into the same situation, but I need some more time
to find a solution:)

Regards!
Gerry


View attachment "tmp2.patch" of type "text/plain" (15503 bytes)