|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v8 16/20] xen/riscv: implement IRQ routing for device passthrough
On 9/3/26 12:00 PM, Jan Beulich wrote: On 27.08.2026 17:19, Oleksii Kurochko wrote:dom0less device passthrough requires granting guest domains access to device interrupts. Introduce map_device_irqs_to_domain() to enumerate a DT node's interrupt properties, skipping those not owned by the primary interrupt controller (as at the moment I haven't seen usages of it), and map_irq_to_domain() to grant domain access and configure Xen's interrupt descriptor accordingly. Sharing IRQ between domains is rejected. Both map_irq_to_domain() and map_device_irqs_to_domain() are marked __overlay_init, mirroring Arm: without CONFIG_OVERLAY_DTB this expands to __init, so the functions are init-only and need no XSM check; with CONFIG_OVERLAY_DTB they become runtime-callable, but the only runtime entry point is dt_overlay_domctl(), which performs the XSM checks at the domctl layer. RISC-V does not wire up DT overlay yet, so today these are strictly __init; if/when overlay support is added, the domctl-level XSM gating must be added together with it, as on Arm. route_irq_to_guest() and release_irq() manage irq_desc ownership for guest-assigned interrupts. Each assignment carries a small irq_guest structure as irqaction::dev_id, recording the owning domain and virtual IRQ number which is 1:1 mapped to physical IRQ number. A per-domain vIRQ allocation bitmap (used_irqs in struct vintc), managed by vintc_reserve_virq(), prevents the same vIRQ being claimed twice. Host and guest interrupts may differ in some operations (EOI timing in particular, possibly others): a host IRQ is completed once Xen's handler runs, whereas a passthrough IRQ must defer the physical completion until the guest issues its own EOI, otherwise a still-asserted level line would immediately retrigger and storm. This affects only the .end callback; the rest of hw_interrupt_type is shared, hence the separate host and guest hw_interrupt_type instances.Irrespective of there not being any .end() hook yet, I think the two would better be split properly right away. aplic_guest_irq_type's .name could then also properly point to e.g. "aplic-guest". I agree with renaming and I will do that now.Regarding splitting I can split now but they will look the same for AIA case as we supports only a vAPLIC which works in MSI mode (vAPLIC+vIMSIC) and it will be true until we will introduce only vAPLIC support w/o vIMSIC. And in case of absence of vIMSIC we will really need a separate .end() for a guest.
And .end() for guest will be definitely also needed in the case of vPLIC.
So for now I can suggest to do the following:
diff --git a/xen/arch/riscv/aplic.c b/xen/arch/riscv/aplic.c
index 3681f0669efb..422c65ece4f7 100644
--- a/xen/arch/riscv/aplic.c
+++ b/xen/arch/riscv/aplic.c
@@ -325,13 +325,40 @@ static const hw_irq_controller aplic_xen_irq_type = {
.set_affinity = aplic_set_irq_affinity,
};
-/* At the moment there is no difference between guest and Xen ops */
-#define aplic_guest_irq_type aplic_xen_irq_type
+static unsigned int cf_check aplic_guest_irq_startup(struct irq_desc *desc)
+{
+ BUG_ON("unimplemented");
+}
+
+/*
+ * Shared by ->shutdown(), ->enable(), ->disable() and ->end(), which have
+ * no state.
+ */
+static void cf_check aplic_guest_irq_stub(struct irq_desc *desc)
+{
+ BUG_ON("unimplemented");
+}
+
+static void cf_check aplic_guest_set_irq_affinity(struct irq_desc *desc,
+ const cpumask_t *mask)
+{
+ BUG_ON("unimplemented");
+}
+
+static const hw_irq_controller aplic_guest = {
+ .typename = "aplic",
+ .startup = aplic_guest_irq_startup,
+ .shutdown = aplic_guest_irq_stub,
+ .enable = aplic_guest_irq_stub,
+ .disable = aplic_guest_irq_stub,
+ .end = aplic_guest_irq_stub,
+ .set_affinity = aplic_guest_set_irq_affinity,
+};
static const struct intc_hw_operations aplic_ops = {
.info = &aplic_info,
.host_irq_type = &aplic_xen_irq_type,
- .guest_irq_type = &aplic_guest_irq_type,
+ .guest_irq_type = &aplic_guest,
.handle_interrupt = aplic_handle_interrupt,
.set_irq_type = aplic_set_irq_type,
};
--- a/xen/arch/riscv/irq.c +++ b/xen/arch/riscv/irq.c @@ -12,11 +12,26 @@ #include <xen/errno.h> #include <xen/init.h> #include <xen/irq.h> +#include <xen/sched.h> #include <xen/spinlock.h> +#include <xen/xvmalloc.h>#include <asm/hardirq.h>#include <asm/intc.h>+/* Describe an IRQ assigned to a guest */ Right, the reference is wrong. release_guest_irq() detaches the action itself and then calls irq_release_action(), which is what actually honours free_on_release. It should be: - * it must not be freed by release_irq() (see free_on_release below).+ * it must not be freed on its own, which is why free_on_release is left + * false for it (see irq_release_action()).
It could. I will apply that.
I will apply that. It could be really done in this way as we are under lock here.
I will reword:
* The read barrier pairs with the spin_unlock() in do_IRQ(): once we
- * observe _IRQ_INPROGRESS cleared, we are guaranteed to also see the
- * writes do_IRQ() made to desc (e.g. desc->action) before
releasing the
- * lock, so it is safe to free the action below.+ * observe _IRQ_INPROGRESS cleared in desc->status, we are guaranteed to + * also see whatever the handler did on that CPU before the bit was + * cleared, so it is safe to free the action below.
I am thinking if a barrier is in correct place or needed at all.Considering that desc->status is updated under spinlock() which uses full barrier the result here should be already observable without smp_rmb() inside do {} while (). Probably we want to have load->load between test_bit() and a read of action->free_on_release in if () below but I don't see what could go wrong if this read will happen before do {} while (). xvfree() (stores inside it) can't be executed ealier because of control dependency [Rule 11: b (xfree) is a (action->free_on_release) store, and b has a syntactic control dependency on a] so again it looks like a barrier isn't needed here. So considering what kind of barrier is used inside spinlock + Rule 11 we can just move smp_rmb() after the cycle (just in case) and it looks like smp_rmb() is only here just to force compiler not to order the things considering how action->free_on_release is used:
static void irq_release_action(const struct irq_desc *desc,
struct irqaction *action)
{
/*
* Wait to make sure it's not being used on another CPU.
*
* desc->status is cleared in do_IRQ() under desc->lock, whose
* acquire/release barriers are a full smp_mb() on this arch, so the
* handler's writes are already ordered before the clear is visible.
* On this side, xvfree() is control-dependent on the final test_bit()
* load, so Rule 11 (RVWMO) already orders it after the wait with no
* barrier. smp_rmb() below adds real read->read ordering, but nothing
* after the loop depends on it (action->free_on_release isn't racy);
* it's kept as a guard against the compiler breaking the control
* dependency the ordering actually relies on.
*/
while ( test_bit(_IRQ_INPROGRESS, &desc->status) )
cpu_relax();
smp_rmb();
if ( action->free_on_release )
xvfree(action);
Am I missing something?
Please split this across three lines, to conform to style. (Also same nit as above.)+ if ( action->free_on_release ) + xvfree(action);How does this being done here fit with the last paragraph of the comment ahead of the function? The comment is inaccurate. I will drop "... by the caller."
Missed that. It looks like it could be really const. + goto unlock_err; + + /* + * Detaching the action happens with desc->lock still held, so that a + * concurrent release_guest_irq() for the same IRQ sees _IRQ_GUEST alreadyI think "sees" is misleading here, as it suggests that a racing check can occur. With the lock held, that's impossible. Hence imo better "will see" (i.e. only after having got hold of the lock). I will correct the comment in suggested way.
I will drop init. with false here.
Sure, I will aligh the comments. Thanks. ~ Oleksii
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |