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
