Host-guest event notification via configured interrupt vector is useful
in cases where a guest makes an asynchronous request and needs a
callback from the host to indicate the completion or to let the host
notify the guest about events like device removal. One usage example is,
callback requirement of GetQuote asynchronous hypercall.
In TDX guest, SetupEventNotifyInterrupt hypercall can be used by the
guest to specify which interrupt vector to use as an event-notify
vector to the VMM. Details about the SetupEventNotifyInterrupt
hypercall can be found in TDX Guest-Host Communication Interface
(GHCI) Specification, sec 3.5 "VP.VMCALL<SetupEventNotifyInterrupt>".
As per design, VMM will post the event completion IRQ using the same
CPU in which SetupEventNotifyInterrupt hypercall request is received.
So allocate an IRQ vector from "x86_vector_domain", and set the CPU
affinity of the IRQ vector to the current CPU.
Add tdx_register_event_irq_cb()/tdx_unregister_event_irq_cb()
interfaces to allow drivers register/unregister event noficiation
handlers.
Reviewed-by: Tony Luck <[email protected]>
Reviewed-by: Andi Kleen <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
Acked-by: Kirill A. Shutemov <[email protected]>
Acked-by: Wander Lairson Costa <[email protected]>
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
---
arch/x86/coco/tdx/tdx.c | 163 +++++++++++++++++++++++++++++++++++++
arch/x86/include/asm/tdx.h | 6 ++
2 files changed, 169 insertions(+)
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 055300e08fb3..d03985952d45 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -7,12 +7,18 @@
#include <linux/cpufeature.h>
#include <linux/export.h>
#include <linux/io.h>
+#include <linux/string.h>
+#include <linux/uaccess.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/numa.h>
#include <asm/coco.h>
#include <asm/tdx.h>
#include <asm/vmx.h>
#include <asm/insn.h>
#include <asm/insn-eval.h>
#include <asm/pgtable.h>
+#include <asm/irqdomain.h>
/* TDX module Call Leaf IDs */
#define TDX_GET_INFO 1
@@ -27,6 +33,7 @@
/* TDX hypercall Leaf IDs */
#define TDVMCALL_MAP_GPA 0x10001
#define TDVMCALL_REPORT_FATAL_ERROR 0x10003
+#define TDVMCALL_SETUP_NOTIFY_INTR 0x10004
/* MMIO direction */
#define EPT_READ 0
@@ -51,6 +58,16 @@
#define TDREPORT_SUBTYPE_0 0
+struct event_irq_entry {
+ tdx_event_irq_cb_t handler;
+ void *data;
+ struct list_head head;
+};
+
+static int tdx_event_irq;
+static LIST_HEAD(event_irq_cb_list);
+static DEFINE_SPINLOCK(event_irq_cb_lock);
+
/*
* Wrapper for standard use of __tdx_hypercall with no output aside from
* return code.
@@ -873,3 +890,149 @@ void __init tdx_early_init(void)
pr_info("Guest detected\n");
}
+
+static irqreturn_t tdx_event_irq_handler(int irq, void *dev_id)
+{
+ struct event_irq_entry *entry;
+
+ spin_lock(&event_irq_cb_lock);
+ list_for_each_entry(entry, &event_irq_cb_list, head) {
+ if (entry->handler)
+ entry->handler(entry->data);
+ }
+ spin_unlock(&event_irq_cb_lock);
+
+ return IRQ_HANDLED;
+}
+
+/* Reserve an IRQ from x86_vector_domain for TD event notification */
+static int __init tdx_event_irq_init(void)
+{
+ struct irq_alloc_info info;
+ cpumask_t saved_cpumask;
+ struct irq_cfg *cfg;
+ int cpu, irq;
+
+ if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return 0;
+
+ init_irq_alloc_info(&info, NULL);
+
+ /*
+ * Event notification vector will be delivered to the CPU
+ * in which TDVMCALL_SETUP_NOTIFY_INTR hypercall is requested.
+ * So set the IRQ affinity to the current CPU.
+ */
+ cpu = get_cpu();
+ cpumask_copy(&saved_cpumask, current->cpus_ptr);
+ info.mask = cpumask_of(cpu);
+ put_cpu();
+
+ irq = irq_domain_alloc_irqs(x86_vector_domain, 1, NUMA_NO_NODE, &info);
+ if (irq <= 0) {
+ pr_err("Event notification IRQ allocation failed %d\n", irq);
+ return -EIO;
+ }
+
+ irq_set_handler(irq, handle_edge_irq);
+
+ cfg = irq_cfg(irq);
+ if (!cfg) {
+ pr_err("Event notification IRQ config not found\n");
+ goto err_free_irqs;
+ }
+
+ if (request_irq(irq, tdx_event_irq_handler, IRQF_NOBALANCING,
+ "tdx_event_irq", NULL)) {
+ pr_err("Event notification IRQ request failed\n");
+ goto err_free_irqs;
+ }
+
+ set_cpus_allowed_ptr(current, cpumask_of(cpu));
+
+ /*
+ * Register callback vector address with VMM. More details
+ * about the ABI can be found in TDX Guest-Host-Communication
+ * Interface (GHCI), sec titled
+ * "TDG.VP.VMCALL<SetupEventNotifyInterrupt>".
+ */
+ if (_tdx_hypercall(TDVMCALL_SETUP_NOTIFY_INTR, cfg->vector, 0, 0, 0)) {
+ pr_err("Event notification hypercall failed\n");
+ goto err_restore_cpus;
+ }
+
+ set_cpus_allowed_ptr(current, &saved_cpumask);
+
+ tdx_event_irq = irq;
+
+ return 0;
+
+err_restore_cpus:
+ set_cpus_allowed_ptr(current, &saved_cpumask);
+ free_irq(irq, NULL);
+err_free_irqs:
+ irq_domain_free_irqs(irq, 1);
+
+ return -EIO;
+}
+arch_initcall(tdx_event_irq_init)
+
+/**
+ * tdx_register_event_irq_cb() - Register TDX event IRQ callback handler.
+ * @handler: Address of driver specific event IRQ callback handler. Handler
+ * will be called in IRQ context and hence cannot sleep.
+ * @data: Context data to be passed to the callback handler.
+ *
+ * Return: 0 on success or standard error code on other failures.
+ */
+int tdx_register_event_irq_cb(tdx_event_irq_cb_t handler, void *data)
+{
+ struct event_irq_entry *entry;
+ unsigned long flags;
+
+ if (tdx_event_irq <= 0)
+ return -EIO;
+
+ entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+ if (!entry)
+ return -ENOMEM;
+
+ entry->data = data;
+ entry->handler = handler;
+
+ spin_lock_irqsave(&event_irq_cb_lock, flags);
+ list_add_tail(&entry->head, &event_irq_cb_list);
+ spin_unlock_irqrestore(&event_irq_cb_lock, flags);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tdx_register_event_irq_cb);
+
+/**
+ * tdx_unregister_event_irq_cb() - Unregister TDX event IRQ callback handler.
+ * @handler: Address of driver specific event IRQ callback handler.
+ * @data: Context data to be passed to the callback handler.
+ *
+ * Return: 0 on success or -EIO if event IRQ is not allocated.
+ */
+int tdx_unregister_event_irq_cb(tdx_event_irq_cb_t handler, void *data)
+{
+ struct event_irq_entry *entry;
+ unsigned long flags;
+
+ if (tdx_event_irq <= 0)
+ return -EIO;
+
+ spin_lock_irqsave(&event_irq_cb_lock, flags);
+ list_for_each_entry(entry, &event_irq_cb_list, head) {
+ if (entry->handler == handler && entry->data == data) {
+ list_del(&entry->head);
+ kfree(entry);
+ break;
+ }
+ }
+ spin_unlock_irqrestore(&event_irq_cb_lock, flags);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tdx_unregister_event_irq_cb);
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 28d889c9aa16..8807fe1b1f3f 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -53,6 +53,8 @@ struct ve_info {
#ifdef CONFIG_INTEL_TDX_GUEST
+typedef int (*tdx_event_irq_cb_t)(void *);
+
void __init tdx_early_init(void);
/* Used to communicate with the TDX module */
@@ -69,6 +71,10 @@ bool tdx_early_handle_ve(struct pt_regs *regs);
int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport);
+int tdx_register_event_irq_cb(tdx_event_irq_cb_t handler, void *data);
+
+int tdx_unregister_event_irq_cb(tdx_event_irq_cb_t handler, void *data);
+
#else
static inline void tdx_early_init(void) { };
--
2.34.1
On Sat, 2023-03-25 at 23:20 -0700, Kuppuswamy Sathyanarayanan wrote:
> Host-guest event notification via configured interrupt vector is useful
> in cases where a guest makes an asynchronous request and needs a
> callback from the host to indicate the completion or to let the host
> notify the guest about events like device removal. One usage example is,
> callback requirement of GetQuote asynchronous hypercall.
>
> In TDX guest, SetupEventNotifyInterrupt hypercall can be used by the
> guest to specify which interrupt vector to use as an event-notify
> vector to the VMM.
>
"to the VMM" -> "from the VMM"?
> Details about the SetupEventNotifyInterrupt
> hypercall can be found in TDX Guest-Host Communication Interface
> (GHCI) Specification, sec 3.5 "VP.VMCALL<SetupEventNotifyInterrupt>".
It seems we shouldn't mention the exact section number.
>
> As per design, VMM will post the event completion IRQ using the same
> CPU in which SetupEventNotifyInterrupt hypercall request is received.
"in which" -> "on which"
> So allocate an IRQ vector from "x86_vector_domain", and set the CPU
> affinity of the IRQ vector to the current CPU.
IMHO "current CPU" is a little bit vague. Perhaps just "to the CPU on which
SetupEventNotifyInterrupt hypercall is made".
Also, perhaps it's better to mention to use IRQF_NOBALANCING to prevent the IRQ
from being migrated to another cpu.
>
> Add tdx_register_event_irq_cb()/tdx_unregister_event_irq_cb()
> interfaces to allow drivers register/unregister event noficiation
> handlers.
>
> Reviewed-by: Tony Luck <[email protected]>
> Reviewed-by: Andi Kleen <[email protected]>
> Reviewed-by: Mika Westerberg <[email protected]>
> Acked-by: Kirill A. Shutemov <[email protected]>
> Acked-by: Wander Lairson Costa <[email protected]>
> Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
> ---
> arch/x86/coco/tdx/tdx.c | 163 +++++++++++++++++++++++++++++++++++++
> arch/x86/include/asm/tdx.h | 6 ++
> 2 files changed, 169 insertions(+)
>
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 055300e08fb3..d03985952d45 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -7,12 +7,18 @@
> #include <linux/cpufeature.h>
> #include <linux/export.h>
> #include <linux/io.h>
> +#include <linux/string.h>
> +#include <linux/uaccess.h>
Do you need above two headers?
Also, perhaps you should explicitly include <.../list.h> and <.../spinlock.h>.
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/numa.h>
> #include <asm/coco.h>
> #include <asm/tdx.h>
> #include <asm/vmx.h>
> #include <asm/insn.h>
> #include <asm/insn-eval.h>
> #include <asm/pgtable.h>
> +#include <asm/irqdomain.h>
>
> /* TDX module Call Leaf IDs */
> #define TDX_GET_INFO 1
> @@ -27,6 +33,7 @@
> /* TDX hypercall Leaf IDs */
> #define TDVMCALL_MAP_GPA 0x10001
> #define TDVMCALL_REPORT_FATAL_ERROR 0x10003
> +#define TDVMCALL_SETUP_NOTIFY_INTR 0x10004
>
> /* MMIO direction */
> #define EPT_READ 0
> @@ -51,6 +58,16 @@
>
> #define TDREPORT_SUBTYPE_0 0
>
> +struct event_irq_entry {
> + tdx_event_irq_cb_t handler;
> + void *data;
> + struct list_head head;
> +};
> +
> +static int tdx_event_irq;
__ro_after_init?
> +static LIST_HEAD(event_irq_cb_list);
> +static DEFINE_SPINLOCK(event_irq_cb_lock);
> +
> /*
> * Wrapper for standard use of __tdx_hypercall with no output aside from
> * return code.
> @@ -873,3 +890,149 @@ void __init tdx_early_init(void)
>
> pr_info("Guest detected\n");
> }
> +
> +static irqreturn_t tdx_event_irq_handler(int irq, void *dev_id)
> +{
> + struct event_irq_entry *entry;
> +
> + spin_lock(&event_irq_cb_lock);
> + list_for_each_entry(entry, &event_irq_cb_list, head) {
> + if (entry->handler)
> + entry->handler(entry->data);
> + }
> + spin_unlock(&event_irq_cb_lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> +/* Reserve an IRQ from x86_vector_domain for TD event notification */
> +static int __init tdx_event_irq_init(void)
> +{
> + struct irq_alloc_info info;
> + cpumask_t saved_cpumask;
> + struct irq_cfg *cfg;
> + int cpu, irq;
> +
> + if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> + return 0;
> +
> + init_irq_alloc_info(&info, NULL);
> +
> + /*
> + * Event notification vector will be delivered to the CPU
> + * in which TDVMCALL_SETUP_NOTIFY_INTR hypercall is requested.
> + * So set the IRQ affinity to the current CPU.
> + */
> + cpu = get_cpu();
> + cpumask_copy(&saved_cpumask, current->cpus_ptr);
> + info.mask = cpumask_of(cpu);
> + put_cpu();
The 'saved_cpumask' related code is ugly. If you move put_cpu() to the end of
this function, I think you can remove all related code:
cpu = get_cpu();
/*
* Set @info->mask to local cpu to make sure a valid vector is
* pre-allocated when TDX event notification IRQ is allocated
* from x86_vector_domain.
*/
init_irq_alloc_info(&info, cpumask_of(cpu));
// rest staff: request_irq(), hypercall ...
put_cpu();
> +
> + irq = irq_domain_alloc_irqs(x86_vector_domain, 1, NUMA_NO_NODE, &info);
Should you use cpu_to_node(cpu) instead of NUMA_NO_NODE?
> + if (irq <= 0) {
> + pr_err("Event notification IRQ allocation failed %d\n", irq);
> + return -EIO;
> + }
> +
> + irq_set_handler(irq, handle_edge_irq);
> +
> + cfg = irq_cfg(irq);
> + if (!cfg) {
> + pr_err("Event notification IRQ config not found\n");
> + goto err_free_irqs;
> + }
You are depending on irq_domain_alloc_irqs() to have already allocated a vector
on the local cpu. Then if !cfg, your code of calling irq_domain_alloc_irqs() to
allocate vector is broken.
So, perhaps you should just WARN() if vector hasn't been allocated to catch bug.
WARN(!(irq_cfg(irq)->vector));
> +
> + if (request_irq(irq, tdx_event_irq_handler, IRQF_NOBALANCING,
It's better to add a comment to explain why using IRQF_NOBALANCING.
> + "tdx_event_irq", NULL)) {
> + pr_err("Event notification IRQ request failed\n");
> + goto err_free_irqs;
> + }
> +
> + set_cpus_allowed_ptr(current, cpumask_of(cpu));
> +
> + /*
> + * Register callback vector address with VMM. More details
> + * about the ABI can be found in TDX Guest-Host-Communication
> + * Interface (GHCI), sec titled
> + * "TDG.VP.VMCALL<SetupEventNotifyInterrupt>".
> + */
> + if (_tdx_hypercall(TDVMCALL_SETUP_NOTIFY_INTR, cfg->vector, 0, 0, 0)) {
> + pr_err("Event notification hypercall failed\n");
> + goto err_restore_cpus;
> + }
> +
> + set_cpus_allowed_ptr(current, &saved_cpumask);
> +
> + tdx_event_irq = irq;
> +
> + return 0;
> +
> +err_restore_cpus:
> + set_cpus_allowed_ptr(current, &saved_cpumask);
> + free_irq(irq, NULL);
> +err_free_irqs:
> + irq_domain_free_irqs(irq, 1);
> +
> + return -EIO;
> +}
> +arch_initcall(tdx_event_irq_init)
> +
Hi Kai,
On 3/27/23 7:38 PM, Huang, Kai wrote:
>> +/* Reserve an IRQ from x86_vector_domain for TD event notification */
>> +static int __init tdx_event_irq_init(void)
>> +{
>> + struct irq_alloc_info info;
>> + cpumask_t saved_cpumask;
>> + struct irq_cfg *cfg;
>> + int cpu, irq;
>> +
>> + if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>> + return 0;
>> +
>> + init_irq_alloc_info(&info, NULL);
>> +
>> + /*
>> + * Event notification vector will be delivered to the CPU
>> + * in which TDVMCALL_SETUP_NOTIFY_INTR hypercall is requested.
>> + * So set the IRQ affinity to the current CPU.
>> + */
>> + cpu = get_cpu();
>> + cpumask_copy(&saved_cpumask, current->cpus_ptr);
>> + info.mask = cpumask_of(cpu);
>> + put_cpu();
> The 'saved_cpumask' related code is ugly. If you move put_cpu() to the end of
> this function, I think you can remove all related code:
>
> cpu = get_cpu();
>
> /*
> * Set @info->mask to local cpu to make sure a valid vector is
> * pre-allocated when TDX event notification IRQ is allocated
> * from x86_vector_domain.
> */
> init_irq_alloc_info(&info, cpumask_of(cpu));
>
> // rest staff: request_irq(), hypercall ...
>
> put_cpu();
>
init_irq_alloc_info() is a sleeping function. Since get_cpu() disables
preemption, we cannot call sleeping function after it. Initially, I
have implemented it like you have mentioned. However, I discovered the
following error.
[ 2.400755] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:580
[ 2.404664] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 1, name: swapper/0
[ 2.408671] preempt_count: 1, expected: 0
[ 2.412650] RCU nest depth: 0, expected: 0
[ 2.412666] no locks held by swapper/0/1.
[ 2.416650] Preemption disabled at:
[ 2.416650] [<ffffffff83b8089f>] tdx_arch_init+0x38/0x117
[ 2.420670] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.1.0-rc4-00117-g672ca073d9f9-dirty #2527
[ 2.424650] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
[ 2.424650] Call Trace:
[ 2.424650] <TASK>
[ 2.424650] dump_stack_lvl+0x6a/0x86
[ 2.424650] __might_resched.cold+0xf4/0x12f
[ 2.424650] __mutex_lock+0x50/0x810
[ 2.424650] ? lock_is_held_type+0xd8/0x130
[ 2.424650] ? __irq_alloc_descs+0xcf/0x310
[ 2.424650] ? find_held_lock+0x2b/0x80
[ 2.424650] ? __irq_alloc_descs+0xcf/0x310
[ 2.424650] __irq_alloc_descs+0xcf/0x310
[ 2.424650] irq_domain_alloc_descs.part.0+0x49/0xa0
[ 2.424650] __irq_domain_alloc_irqs+0x2a0/0x4f0
[ 2.424650] ? next_arg+0x129/0x1f0
[ 2.424650] ? tdx_guest_init+0x5b/0x5b
[ 2.424650] tdx_arch_init+0x8e/0x117
[ 2.424650] do_one_initcall+0x137/0x2ec
[ 2.424650] ? rcu_read_lock_sched_held+0x36/0x60
[ 2.424650] kernel_init_freeable+0x1e3/0x241
[ 2.424650] ? rest_init+0x1a0/0x1a0
[ 2.424650] kernel_init+0x17/0x170
[ 2.424650] ? rest_init+0x1a0/0x1a0
[ 2.424650] ret_from_fork+0x1f/0x30
[ 2.424650] </TASK>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On Mon, 2023-03-27 at 19:50 -0700, Sathyanarayanan Kuppuswamy wrote:
> Hi Kai,
>
> On 3/27/23 7:38 PM, Huang, Kai wrote:
> > > +/* Reserve an IRQ from x86_vector_domain for TD event notification */
> > > +static int __init tdx_event_irq_init(void)
> > > +{
> > > + struct irq_alloc_info info;
> > > + cpumask_t saved_cpumask;
> > > + struct irq_cfg *cfg;
> > > + int cpu, irq;
> > > +
> > > + if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> > > + return 0;
> > > +
> > > + init_irq_alloc_info(&info, NULL);
> > > +
> > > + /*
> > > + * Event notification vector will be delivered to the CPU
> > > + * in which TDVMCALL_SETUP_NOTIFY_INTR hypercall is requested.
> > > + * So set the IRQ affinity to the current CPU.
> > > + */
> > > + cpu = get_cpu();
> > > + cpumask_copy(&saved_cpumask, current->cpus_ptr);
> > > + info.mask = cpumask_of(cpu);
> > > + put_cpu();
> > The 'saved_cpumask' related code is ugly. If you move put_cpu() to the end of
> > this function, I think you can remove all related code:
> >
> > cpu = get_cpu();
> >
> > /*
> > * Set @info->mask to local cpu to make sure a valid vector is
> > * pre-allocated when TDX event notification IRQ is allocated
> > * from x86_vector_domain.
> > */
> > init_irq_alloc_info(&info, cpumask_of(cpu));
> >
> > // rest staff: request_irq(), hypercall ...
> >
> > put_cpu();
> >
>
> init_irq_alloc_info() is a sleeping function. Since get_cpu() disables
> preemption, we cannot call sleeping function after it. Initially, I
> have implemented it like you have mentioned. However, I discovered the
> following error.
Oh sorry I forgot this. So I think we should use migrate_disable() instead:
migrate_disable();
init_irq_alloc_info(&info, cpumask_of(smp_processor_id()));
...
migrate_enable();
Or, should we just use early_initcall() so that only BSP is running? IMHO it's
OK to always allocate the vector from BSP.
Anyway, either way is fine to me.
>
> [ 2.400755] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:580
> [ 2.404664] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 1, name: swapper/0
> [ 2.408671] preempt_count: 1, expected: 0
> [ 2.412650] RCU nest depth: 0, expected: 0
> [ 2.412666] no locks held by swapper/0/1.
> [ 2.416650] Preemption disabled at:
> [ 2.416650] [<ffffffff83b8089f>] tdx_arch_init+0x38/0x117
> [ 2.420670] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.1.0-rc4-00117-g672ca073d9f9-dirty #2527
> [ 2.424650] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
> [ 2.424650] Call Trace:
> [ 2.424650] <TASK>
> [ 2.424650] dump_stack_lvl+0x6a/0x86
> [ 2.424650] __might_resched.cold+0xf4/0x12f
> [ 2.424650] __mutex_lock+0x50/0x810
> [ 2.424650] ? lock_is_held_type+0xd8/0x130
> [ 2.424650] ? __irq_alloc_descs+0xcf/0x310
> [ 2.424650] ? find_held_lock+0x2b/0x80
> [ 2.424650] ? __irq_alloc_descs+0xcf/0x310
> [ 2.424650] __irq_alloc_descs+0xcf/0x310
> [ 2.424650] irq_domain_alloc_descs.part.0+0x49/0xa0
> [ 2.424650] __irq_domain_alloc_irqs+0x2a0/0x4f0
> [ 2.424650] ? next_arg+0x129/0x1f0
> [ 2.424650] ? tdx_guest_init+0x5b/0x5b
> [ 2.424650] tdx_arch_init+0x8e/0x117
> [ 2.424650] do_one_initcall+0x137/0x2ec
> [ 2.424650] ? rcu_read_lock_sched_held+0x36/0x60
> [ 2.424650] kernel_init_freeable+0x1e3/0x241
> [ 2.424650] ? rest_init+0x1a0/0x1a0
> [ 2.424650] kernel_init+0x17/0x170
> [ 2.424650] ? rest_init+0x1a0/0x1a0
> [ 2.424650] ret_from_fork+0x1f/0x30
> [ 2.424650] </TASK>
On 3/27/23 9:02 PM, Huang, Kai wrote:
> On Mon, 2023-03-27 at 19:50 -0700, Sathyanarayanan Kuppuswamy wrote:
>> Hi Kai,
>>
>> On 3/27/23 7:38 PM, Huang, Kai wrote:
>>>> +/* Reserve an IRQ from x86_vector_domain for TD event notification */
>>>> +static int __init tdx_event_irq_init(void)
>>>> +{
>>>> + struct irq_alloc_info info;
>>>> + cpumask_t saved_cpumask;
>>>> + struct irq_cfg *cfg;
>>>> + int cpu, irq;
>>>> +
>>>> + if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>>>> + return 0;
>>>> +
>>>> + init_irq_alloc_info(&info, NULL);
>>>> +
>>>> + /*
>>>> + * Event notification vector will be delivered to the CPU
>>>> + * in which TDVMCALL_SETUP_NOTIFY_INTR hypercall is requested.
>>>> + * So set the IRQ affinity to the current CPU.
>>>> + */
>>>> + cpu = get_cpu();
>>>> + cpumask_copy(&saved_cpumask, current->cpus_ptr);
>>>> + info.mask = cpumask_of(cpu);
>>>> + put_cpu();
>>> The 'saved_cpumask' related code is ugly. If you move put_cpu() to the end of
>>> this function, I think you can remove all related code:
>>>
>>> cpu = get_cpu();
>>>
>>> /*
>>> * Set @info->mask to local cpu to make sure a valid vector is
>>> * pre-allocated when TDX event notification IRQ is allocated
>>> * from x86_vector_domain.
>>> */
>>> init_irq_alloc_info(&info, cpumask_of(cpu));
>>>
>>> // rest staff: request_irq(), hypercall ...
>>>
>>> put_cpu();
>>>
>>
>> init_irq_alloc_info() is a sleeping function. Since get_cpu() disables
>> preemption, we cannot call sleeping function after it. Initially, I
>> have implemented it like you have mentioned. However, I discovered the
>> following error.
>
> Oh sorry I forgot this. So I think we should use migrate_disable() instead:
>
> migrate_disable();
>
> init_irq_alloc_info(&info, cpumask_of(smp_processor_id()));
>
> ...
>
> migrate_enable();
>
> Or, should we just use early_initcall() so that only BSP is running? IMHO it's
> OK to always allocate the vector from BSP.
>
> Anyway, either way is fine to me.
Final version looks like below.
static int __init tdx_event_irq_init(void)
{
struct irq_alloc_info info;
struct irq_cfg *cfg;
int irq;
if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
return 0;
init_irq_alloc_info(&info, NULL);
/*
* Event notification vector will be delivered to the CPU
* in which TDVMCALL_SETUP_NOTIFY_INTR hypercall is requested.
* So set the IRQ affinity to the current CPU.
*/
info.mask = cpumask_of(0);
irq = irq_domain_alloc_irqs(x86_vector_domain, 1, cpu_to_node(0), &info);
if (irq <= 0) {
pr_err("Event notification IRQ allocation failed %d\n", irq);
return -EIO;
}
irq_set_handler(irq, handle_edge_irq);
/* Since the IRQ affinity is set, it cannot be balanced */
if (request_irq(irq, tdx_event_irq_handler, IRQF_NOBALANCING,
"tdx_event_irq", NULL)) {
pr_err("Event notification IRQ request failed\n");
goto err_free_domain_irqs;
}
cfg = irq_cfg(irq);
/*
* Since tdx_event_irq_init() is triggered via early_initcall(),
* it will called before secondary CPUs bringup. Since there is
* only one CPU, it complies with the requirement of executing
* the TDVMCALL_SETUP_NOTIFY_INTR hypercall on the same CPU where
* the IRQ vector is allocated.
*
* Register callback vector address with VMM. More details
* about the ABI can be found in TDX Guest-Host-Communication
* Interface (GHCI), sec titled
* "TDG.VP.VMCALL<SetupEventNotifyInterrupt>".
*/
if (_tdx_hypercall(TDVMCALL_SETUP_NOTIFY_INTR, cfg->vector, 0, 0, 0)) {
pr_err("Event notification hypercall failed\n");
goto err_free_irqs;
}
tdx_event_irq = irq;
return 0;
err_free_irqs:
free_irq(irq, NULL);
err_free_domain_irqs:
irq_domain_free_irqs(irq, 1);
return -EIO;
}
early_initcall(tdx_event_irq_init)
>
>>
>> [ 2.400755] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:580
>> [ 2.404664] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 1, name: swapper/0
>> [ 2.408671] preempt_count: 1, expected: 0
>> [ 2.412650] RCU nest depth: 0, expected: 0
>> [ 2.412666] no locks held by swapper/0/1.
>> [ 2.416650] Preemption disabled at:
>> [ 2.416650] [<ffffffff83b8089f>] tdx_arch_init+0x38/0x117
>> [ 2.420670] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.1.0-rc4-00117-g672ca073d9f9-dirty #2527
>> [ 2.424650] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
>> [ 2.424650] Call Trace:
>> [ 2.424650] <TASK>
>> [ 2.424650] dump_stack_lvl+0x6a/0x86
>> [ 2.424650] __might_resched.cold+0xf4/0x12f
>> [ 2.424650] __mutex_lock+0x50/0x810
>> [ 2.424650] ? lock_is_held_type+0xd8/0x130
>> [ 2.424650] ? __irq_alloc_descs+0xcf/0x310
>> [ 2.424650] ? find_held_lock+0x2b/0x80
>> [ 2.424650] ? __irq_alloc_descs+0xcf/0x310
>> [ 2.424650] __irq_alloc_descs+0xcf/0x310
>> [ 2.424650] irq_domain_alloc_descs.part.0+0x49/0xa0
>> [ 2.424650] __irq_domain_alloc_irqs+0x2a0/0x4f0
>> [ 2.424650] ? next_arg+0x129/0x1f0
>> [ 2.424650] ? tdx_guest_init+0x5b/0x5b
>> [ 2.424650] tdx_arch_init+0x8e/0x117
>> [ 2.424650] do_one_initcall+0x137/0x2ec
>> [ 2.424650] ? rcu_read_lock_sched_held+0x36/0x60
>> [ 2.424650] kernel_init_freeable+0x1e3/0x241
>> [ 2.424650] ? rest_init+0x1a0/0x1a0
>> [ 2.424650] kernel_init+0x17/0x170
>> [ 2.424650] ? rest_init+0x1a0/0x1a0
>> [ 2.424650] ret_from_fork+0x1f/0x30
>> [ 2.424650] </TASK>
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
On Tue, 2023-04-04 at 18:02 -0700, Sathyanarayanan Kuppuswamy wrote:
>
> On 3/27/23 9:02 PM, Huang, Kai wrote:
> > On Mon, 2023-03-27 at 19:50 -0700, Sathyanarayanan Kuppuswamy wrote:
> > > Hi Kai,
> > >
> > > On 3/27/23 7:38 PM, Huang, Kai wrote:
> > > > > +/* Reserve an IRQ from x86_vector_domain for TD event notification */
> > > > > +static int __init tdx_event_irq_init(void)
> > > > > +{
> > > > > + struct irq_alloc_info info;
> > > > > + cpumask_t saved_cpumask;
> > > > > + struct irq_cfg *cfg;
> > > > > + int cpu, irq;
> > > > > +
> > > > > + if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> > > > > + return 0;
> > > > > +
> > > > > + init_irq_alloc_info(&info, NULL);
> > > > > +
> > > > > + /*
> > > > > + * Event notification vector will be delivered to the CPU
> > > > > + * in which TDVMCALL_SETUP_NOTIFY_INTR hypercall is requested.
> > > > > + * So set the IRQ affinity to the current CPU.
> > > > > + */
> > > > > + cpu = get_cpu();
> > > > > + cpumask_copy(&saved_cpumask, current->cpus_ptr);
> > > > > + info.mask = cpumask_of(cpu);
> > > > > + put_cpu();
> > > > The 'saved_cpumask' related code is ugly. If you move put_cpu() to the end of
> > > > this function, I think you can remove all related code:
> > > >
> > > > cpu = get_cpu();
> > > >
> > > > /*
> > > > * Set @info->mask to local cpu to make sure a valid vector is
> > > > * pre-allocated when TDX event notification IRQ is allocated
> > > > * from x86_vector_domain.
> > > > */
> > > > init_irq_alloc_info(&info, cpumask_of(cpu));
> > > >
> > > > // rest staff: request_irq(), hypercall ...
> > > >
> > > > put_cpu();
> > > >
> > >
> > > init_irq_alloc_info() is a sleeping function. Since get_cpu() disables
> > > preemption, we cannot call sleeping function after it. Initially, I
> > > have implemented it like you have mentioned. However, I discovered the
> > > following error.
> >
> > Oh sorry I forgot this. So I think we should use migrate_disable() instead:
> >
> > migrate_disable();
> >
> > init_irq_alloc_info(&info, cpumask_of(smp_processor_id()));
> >
> > ...
> >
> > migrate_enable();
> >
> > Or, should we just use early_initcall() so that only BSP is running? IMHO it's
> > OK to always allocate the vector from BSP.
> >
> > Anyway, either way is fine to me.
>
> Final version looks like below.
>
> static int __init tdx_event_irq_init(void)
> {
> struct irq_alloc_info info;
> struct irq_cfg *cfg;
> int irq;
>
> if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> return 0;
>
> init_irq_alloc_info(&info, NULL);
>
> /*
> * Event notification vector will be delivered to the CPU
> * in which TDVMCALL_SETUP_NOTIFY_INTR hypercall is requested.
> * So set the IRQ affinity to the current CPU.
> */
> info.mask = cpumask_of(0);
>
> irq = irq_domain_alloc_irqs(x86_vector_domain, 1, cpu_to_node(0), &info);
> if (irq <= 0) {
> pr_err("Event notification IRQ allocation failed %d\n", irq);
> return -EIO;
> }
>
> irq_set_handler(irq, handle_edge_irq);
>
> /* Since the IRQ affinity is set, it cannot be balanced */
> if (request_irq(irq, tdx_event_irq_handler, IRQF_NOBALANCING,
> "tdx_event_irq", NULL)) {
> pr_err("Event notification IRQ request failed\n");
> goto err_free_domain_irqs;
> }
>
> cfg = irq_cfg(irq);
>
> /*
> * Since tdx_event_irq_init() is triggered via early_initcall(),
> * it will called before secondary CPUs bringup. Since there is
> * only one CPU, it complies with the requirement of executing
> * the TDVMCALL_SETUP_NOTIFY_INTR hypercall on the same CPU where
> * the IRQ vector is allocated.
> *
> * Register callback vector address with VMM. More details
> * about the ABI can be found in TDX Guest-Host-Communication
> * Interface (GHCI), sec titled
> * "TDG.VP.VMCALL<SetupEventNotifyInterrupt>".
> */
> if (_tdx_hypercall(TDVMCALL_SETUP_NOTIFY_INTR, cfg->vector, 0, 0, 0)) {
> pr_err("Event notification hypercall failed\n");
> goto err_free_irqs;
> }
>
> tdx_event_irq = irq;
>
> return 0;
>
> err_free_irqs:
> free_irq(irq, NULL);
> err_free_domain_irqs:
> irq_domain_free_irqs(irq, 1);
>
> return -EIO;
> }
> early_initcall(tdx_event_irq_init)
I found there's another series also doing similar thing, and it seems Thomas
wasn't happy about using x86_vector_domain directly:
https://lore.kernel.org/lkml/877cv99k0y.ffs@tglx/
An alternative was also posted (creating IRQ domain on top of
x86_vector_domain):
https://lore.kernel.org/lkml/[email protected]/
I think we should monitor that and hear from others more.