linux-kernel - Re: [RFC PATCH 4/9] irqchip: GIC: add support for forwarded 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:	Mon, 07 Jul 2014 11:43:30 +0100
From:	Marc Zyngier <marc.zyngier@....com>
To:	Will Deacon <will.deacon@....com>
Cc:	"kvmarm\@lists.cs.columbia.edu" <kvmarm@...ts.cs.columbia.edu>,
	"linux-arm-kernel\@lists.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
	Catalin Marinas <Catalin.Marinas@....com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"eric.auger\@linaro.org" <eric.auger@...aro.org>,
	Christoffer Dall <christoffer.dall@...aro.org>
Subject: Re: [RFC PATCH 4/9] irqchip: GIC: add support for forwarded interrupts

On Fri, Jun 27 2014 at  2:17:48 pm BST, Will Deacon <will.deacon@....com> wrote:
> Hi Marc,
>
> On Wed, Jun 25, 2014 at 10:28:45AM +0100, Marc Zyngier wrote:
>> Now that we've switched to EOImode == 1, prevent a forwarded interrupt
>> from being deactivated after its priority has been dropped.
>> 
>> Also add support for the interrupt state to be saved/restored.
>> 
>> Signed-off-by: Marc Zyngier <marc.zyngier@....com>
>> ---
>>  drivers/irqchip/irq-gic.c | 48
>> +++++++++++++++++++++++++++++++++++++++++------
>>  1 file changed, 42 insertions(+), 6 deletions(-)
>
> [...]
>
>> +static void gic_irq_set_fwd_state(struct irq_data *d, u32 val, u32 mask)
>> +{
>> +	if (mask & IRQ_FWD_STATE_PENDING)
>> + gic_poke_irq(d, (val & IRQ_FWD_STATE_PENDING) ?
>> GIC_DIST_ENABLE_SET : GIC_DIST_ENABLE_CLEAR);
>> +	if (mask & IRQ_FWD_STATE_ACTIVE)
>> + gic_poke_irq(d, (val & IRQ_FWD_STATE_ACTIVE) ? GIC_DIST_ACTIVE_SET
>> : GIC_DIST_ACTIVE_CLEAR);
>> +	if (mask & IRQ_FWD_STATE_MASKED)
>> + gic_poke_irq(d, (val & IRQ_FWD_STATE_MASKED) ?
>> GIC_DIST_ENABLE_CLEAR : GIC_DIST_ENABLE_SET);
>
> Given that this isn't atomic and KVM only cares about ACTIVE, why not change
> mask to be a single state only? Renumbering the states so that's not bits
> would help to enforce this (i.e. make IRQ_FWD_STATE_ACTIVE 0, PENDING 1,
> ...).
>
> That would also allow you to switch on the state and return early.

Well, this is atomic from the point of view of the interrupt (we hold
the descriptor spinlock). Now, given my answer to your remark about the
locking earlier in this thread and given the KVM use case, we could
indeed relax this and loose the atomicity. But we need to agree on what
is the model we want to expose.

>> +static u32 gic_irq_get_fwd_state(struct irq_data *d, u32 mask)
>> +{
>> +	u32 val = 0;
>> +
>> + if (mask & IRQ_FWD_STATE_PENDING && gic_peek_irq(d,
>> GIC_DIST_ENABLE_SET))
>> +		val |= IRQ_FWD_STATE_PENDING;
>> +	if (mask & IRQ_FWD_STATE_ACTIVE && gic_peek_irq(d, GIC_DIST_ACTIVE_SET))
>> +		val |= IRQ_FWD_STATE_ACTIVE;
>> + if (mask & IRQ_FWD_STATE_MASKED && !gic_peek_irq(d,
>> GIC_DIST_ENABLE_SET))
>> +		val |= IRQ_FWD_STATE_MASKED;
>
> *and* you could peek GIC_DIST_ENABLE_SET in one place here.

Ah yes, this is a GIC specificity (MASKED = !ENABLED). Good point.

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.
--
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/