Re: [PATCH 0/2] arm: Review irq pipeline

Florian Bezdeka <[email protected]> writes:

> Hi Philippe,
>
> This is the result of a review of the arm implementation of the IRQ
> pipeline. Triggered by some risc-v reviews.
>
> I'm sure about the two easy fixes, targeting different Dovetail
> versions, so split into different patches.
>
> In addition there is one more, I need your input here:
>
> We have two calls to interrupts_enabled() in arch/arm/mm/fault.c,
> both in the do_page_fault() path.
>
> Assuming that we are handling a page fault over kernel space,
> inband stage stalled, we would leave the kernel with the inband stage
> unstalled, as interrupts_enabled() is checking the hardware state (as
> pushed to the stack on entry) instead of the inband stage stall bit.
>
> Is that correct?
>

Nope.

> I'm quite sure we have more real problems that in this case, but
> maybe we should fix that up?
>
> We would have to call something that does the HW check in case
> CONFIG_IRQ_PIPELINE is off, but checks the stall bit in case pipelining
> is enabled.
>

Instead of checking for interrupts_enabled(regs) in do_page_fault() and
do_kernel_address_page_fault(), we should rather check for
!(arch_kentry_get_irqstate(regs) & KENTRY_STALL_BIT) when pipelining,
which would give us the virtual interrupt state on entry, provided we
run in-band.

Inner issue: since do_kernel_address_page_fault() skipped fault_entry()
on trap from supervisor mode, we might still be running oob, calling
local_irq_enable() in such a case would be wrong.

-- 
Philippe.