On Mon, 02 Aug 2021 11:49:42 +0100, Viresh Kumar viresh.kumar@linaro.org wrote:
Hi Marc,
On 02-08-21, 10:45, Marc Zyngier wrote:
On Sun, 01 Aug 2021 14:43:37 +0100, Arnd Bergmann arnd@kernel.org wrote:
What are the requirements for a ->irq_mask() operation? Can this be called in atomic context? If it can, is it allowed to be posted (e.g. by queuing a new command without waiting for the reply)?
irq_mask() can definitely be called in atomic context. But in this case the driver must implement the bus_lock madness and commit changes to the backend on unlock. See how most of the I2C GPIO implementations work.
Right, I already have that in place but I have a few doubts on that.
I can see irq_bus_lock/unlock() getting called around irq_mask/unmask/set_type() during request_irq(), and so I can issue all the virtio calls from irq_bus_unlock() there.
But I don't see the same sequence being followed by the irq core when an interrupt actually happens, i.e. when I call generic_handle_irq() from virtqueue callback for eventq vq. In this case, the irq core can directly call mask/unmask() around irq_eoi(), without calling the irq_bus_lock/unlock() guards.
You can't take that lock from the core irq code, for obvious reasons: you're in IRQ context, and you cannot sleep. You definitely need to use threaded interrupts for this.
How should the driver take care of these mask/unmask calls, or should it take care of it at all, as they should negate each other eventually ?
Obviously, this has an effect on performance and complexity. It would be a lot better if there was a way to perform the mask/unmask operations synchronously, without the need of a buffer. For example, virtio-console has the so called 'emergency write' feature, which allows for write operations without any buffer.
We discussed this sometime back over IRC, and Jean Philippe said this in context of virtio-iommu:
"From some profiling the overhead of context switching is much greater than that of adding buffers, so adding a new virtio interface to bypass the virtqueue might not show any improvement."
If you need something to be synchronous, there is not exactly a choice. Masking an interrupt is something that is much better as a synchronous interface, because it leads to tons of further simplifications. And in my book, simplicity is a much easier path to correctness.
We were looking to explore the config space for normal GPIO operations as well then.
The host would be required to perform some action here on a config update and the guest would be waiting over an atomic operation for it to complete. So eventually it should also be slow.
The later would allow the implementation of a "fire and forget" mask/unmask that would still be synchronous.
Alternatively, would it be possible to simply emulate the 'irq_mask()' operation in software, by not delivering the next interrupt to the handler until the driver calls irq_unmask()? Once it's been delivered, it would just not get requeued in this case.
That'd be a poor-man's solution, as you would still get host notifications, which could have an impact in the case of a screaming interrupt.
Right, irq_bus_unlock() works fine for this though.
To some extent (insert a comment about thrust and flying pigs here). It imposes a lot of restrictions, adds latency, and makes it harder to reason about when what happens when. It also means that you are probably designing for Linux instead of building something more generic.
Having contributed to a PV interrupt architecture in the recent past, I quickly realised that keeping things as simple as possible was a good way to avoid baked-in assumptions. Yes, a synchronous interface results in a few extra traps. But you can also now reason about it and *prove* things. YMMV.
Thanks,
M.