| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <be6482713dad31bba416d8f5f9c0901051b3c915.camel@intel.com>
Date: Wed, 5 Apr 2023 02:45:32 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "corbet@....net" <corbet@....net>,
"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
"bp@...en8.de" <bp@...en8.de>,
"shuah@...nel.org" <shuah@...nel.org>,
"sathyanarayanan.kuppuswamy@...ux.intel.com"
<sathyanarayanan.kuppuswamy@...ux.intel.com>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"x86@...nel.org" <x86@...nel.org>,
"mingo@...hat.com" <mingo@...hat.com>
CC: "Yu, Guorui" <guorui.yu@...ux.alibaba.com>,
"linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
"wander@...hat.com" <wander@...hat.com>,
"hpa@...or.com" <hpa@...or.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
"Luck, Tony" <tony.luck@...el.com>, "Du, Fan" <fan.du@...el.com>,
"Aktas, Erdem" <erdemaktas@...gle.com>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>
Subject: Re: [PATCH v1 1/3] x86/tdx: Add TDX Guest event notify interrupt
support
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/20230328182933.GA1403032@vm02.guest.corp.microsoft.com/
I think we should monitor that and hear from others more.
Powered by blists - more mailing lists