linux-kernel - Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)

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]

Message-ID: <54584681.2010103@amd.com>
Date:	Mon, 3 Nov 2014 21:22:41 -0600
From:	Suravee Suthikulanit <suravee.suthikulpanit@....com>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	Marc Zyngier <marc.zyngier@....com>,
	Mark Rutland <mark.rutland@....com>, <jason@...edaemon.net>,
	<Catalin.Marinas@....com>, <Will.Deacon@....com>,
	<liviu.dudau@....com>, <Harish.Kasiviswanathan@....com>,
	<linux-arm-kernel@...ts.infradead.org>,
	<linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-doc@...r.kernel.org>, <devicetree@...r.kernel.org>
Subject: Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m
 MSI(-X)

On 11/3/2014 4:51 PM, Thomas Gleixner wrote:
> On Mon, 3 Nov 2014, suravee.suthikulpanit@....com wrote:
>> +static void gicv2m_teardown_msi_irq(struct msi_chip *chip, unsigned int irq)
>> +{
>> +	int pos;
>> +	struct v2m_data *v2m = container_of(chip, struct v2m_data, msi_chip);
>> +
>> +	spin_lock(&v2m->msi_cnt_lock);
>
> Why do you need an extra lock here? Is that stuff not serialized from
> the msi_chip layer already?
>
> If not, why don't we have the serialization there instead of forcing
> every callback to implement its own?

 From the following call paths:
   |--> pci_enable_msi_range
    |--> msi_capability_init
     |--> arch_setup_msi_irqs
      |--> arch_setup_msi_irq
and
   |--> pci_enable_msix
    |--> msix_capability_init
     |--> arch_setup_msi_irqs
      |--> arch_setup_msi_irq

It serialize when a PCI device driver tries to allocate multiple 
interrupts. However, AFAICT, it would not serialize the allocation when 
multiple drivers trying to setup MSI irqs at the same time. I needed 
that to protect the bitmap structure. I also noticed the same in other 
drivers as well.

I can look into this more to see where would be a good point.

>> +	pos = irq - v2m->spi_start;
>
> So this assumes that @irq is the hwirq number, right? How does the
> calling function know about that? It should only have knowledge about
> the virq number if I'm not missing something.
>
> And if I'm missing something, then that msi_chip stuff is seriously
> broken.

It works this way because of the direct mapping (as you noticed). But I 
am planning to change that. See below.

>
>> +	if (pos >= 0 && pos < v2m->nr_spis)
>
> So you simply avoid the clear bitmap instead of yelling loudly about
> being called with completely wrong data?

I'll provide appropriate warnings.

> I would not be surprised if that is related to my question above.

Not quite sure which of the above questions.

>> +	spin_lock(&v2m->msi_cnt_lock);
>> +	offset = bitmap_find_free_region(v2m->bm, v2m->nr_spis, 0);
>> +	spin_unlock(&v2m->msi_cnt_lock);
>> +	if (offset < 0)
>> +		return offset;
>> +
>> +	hwirq = v2m->spi_start + offset;
>> +	virq = __irq_domain_alloc_irqs(v2m->domain, hwirq,
>> +				       1, NUMA_NO_NODE, v2m, true);
>> +	if (virq < 0) {
>> +		gicv2m_teardown_msi_irq(chip, hwirq);
>> +		return virq;
>> +	}
>> +
>> +	irq_domain_set_hwirq_and_chip(v2m->domain, virq, hwirq,
>> +				&v2m_chip, v2m);
>> +
>> +	irq_set_msi_desc(hwirq, desc);
>> +	irq_set_irq_type(hwirq, IRQ_TYPE_EDGE_RISING);
>
> Sure both calls work perfectly fine as long as virq == hwirq, right?

I was running into an issue when calling the 
irq_domain_alloc_irq_parent(), it requires of_phandle_args pointer to be 
passed in. However, this does not work for GICv2m since it does not have 
interrupt information in the device tree. So, I decided at first to use 
direct (virq == hwirq) mapping, which simplifies the code a bit, but 
might not be ideal solution, as you pointed out.

An alternative would be to create a temporary struct of_phandle_args, 
and populate it with the interrupt information for the requested MSI. 
Then pass it to:
   --> irq_domain_alloc_irq_parent
    |--> gic_irq_domain_alloc
      |--> gic_irq_domain_xlate
      |--> gic_irq_domain_map

However, this would still not be ideal if we want to support ACPI. 
Another alternative would be coming up with a dedicate structure to be 
used here. I noticed on X86, it uses struct irq_alloc_info. May be 
that's what we also need here.

> [...]
> I do not care at all how YOU waste your time. But I care very much
> about the fact that YOU are wasting MY precious time by exposing me to
> your patch trainwrecks.

I don't intend to waste yours or anybody's precious time. Sorry if it 
takes a couple iterations to work out the issues. Also, I will try to 
put more comment in my code to make it more clear. Let me know what 
works best for you to work out the issues.

Thanks,

Suravee

>
> Thanks,
>
> 	tglx
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/