Message-ID: <80718d6c-9e53-d549-bfe2-6c10d618acb1@linux.intel.com>
Date:   Tue, 25 Apr 2023 23:07:17 -0700
From:   Sathyanarayanan Kuppuswamy 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
To:     "Huang, Kai" <kai.huang@...el.com>,
        "corbet@....net" <corbet@....net>, "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "shuah@...nel.org" <shuah@...nel.org>,
        "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>,
        "chongc@...gle.com" <chongc@...gle.com>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "qinkun@...che.org" <qinkun@...che.org>,
        "Luck, Tony" <tony.luck@...el.com>,
        "Aktas, Erdem" <erdemaktas@...gle.com>,
        "dionnaglaze@...gle.com" <dionnaglaze@...gle.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Du, Fan" <fan.du@...el.com>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>
Subject: Re: [PATCH v2 1/3] x86/tdx: Add TDX Guest event notify interrupt
 support



On 4/25/23 6:59 PM, Huang, Kai wrote:
> On Tue, 2023-04-25 at 16:47 -0700, Sathyanarayanan Kuppuswamy wrote:
>> Hi Kai,
>>
>> On 4/14/23 6:34 AM, Huang, Kai wrote:
>>> On Wed, 2023-04-12 at 20:41 -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 from the VMM. Details about the SetupEventNotifyInterrupt
>>>> hypercall can be found in TDX Guest-Host Communication Interface
>>>> (GHCI) Specification, section "VP.VMCALL<SetupEventNotifyInterrupt>".
>>>>
>>>> As per design, VMM will post the event completion IRQ using the same
>>>> CPU on 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 CPU on which
>>>> SetupEventNotifyInterrupt hypercall is made.
>>>>
>>>> Add tdx_register_event_irq_cb()/tdx_unregister_event_irq_cb()
>>>> interfaces to allow drivers register/unregister event noficiation
>>> 			      ^
>>> 			      to register/unregister
>>>> handlers.
>>>>
>>>>
>>>
>>> [...]
>>>
>>
>> With suggested changes, the final version looks like below.
>>
>> +/**
>> + * tdx_event_irq_init() - Register IRQ for event notification from the VMM to
>> + *                       the TDX Guest.
>> + *
>> + * Use SetupEventNotifyInterrupt TDVMCALL to register the event notification
>> + * IRQ with the VMM, which is used by the VMM to notify the TDX guest when
>> + * needed, for instance, when VMM finishes the GetQuote request from the TDX
>> + * guest. The VMM always notifies the TDX guest via the same CPU on which the
>> + * SetupEventNotifyInterrupt TDVMCALL is called. For simplicity, just allocate
>> + * an IRQ (and a vector) directly from x86_vector_domain for such notification
>> + * and pin the IRQ to the same CPU on which TDVMCALL is called.
> 
> I think "for simplicity" applies to allocate IRQ/vector "from BSP using
> early_initcall()" (so TDVMCALL is easily guaranteed to be called on the same cpu
> where vector is allocated), but doesn't apply to allocate IRQ/vector from
> x86_vector_domain and "pin the IRQ to the same CPU on which TDVMCALAL is
> called".  The latter is something you must do (otherwise you need to allocate
> the same vector on all cpus), but not something that you do "for simplicity".
> 
>> + *
>> + * Since tdx_event_irq_init() is triggered via early_initcall(), it will be
>> + * called before secondary CPUs bring up, so no special logic is required to
>> + * ensure that the same CPU is used for SetupEventNotifyInterrupt TDVMCALL and
>> + * IRQ allocation.
> 
> IMHO the second paragraph is obvious and no need to mention.
> 
> As explained above, I guess you just need to at somewhere simply mention
> something like: "for simplicity use early_initcall() to allocate and pin the
> IRQ/vector on BSP and also call the TDVMCALL on BSP".  Or probably "also call
> the TDVMCALL on BSP" can also be omitted as it's kinda already explained in the
> nature of the TDVMCALL.

How about the following?

Use SetupEventNotifyInterrupt TDVMCALL to register the event notification
IRQ with the VMM, which is used by the VMM to notify the TDX guest when
needed, for instance, when VMM finishes the GetQuote request from the TDX
guest. The VMM always notifies the TDX guest via the same CPU that calls the
SetupEventNotifyInterrupt TDVMCALL. Allocate an IRQ/vector from the
x86_vector_domain and pin it on the same CPU on which TDVMCALL is called.
For simplicity, use early_initcall() to allow both IRQ allocation and
TDVMCALL to use BSP.

> 
>> + */
>> +static int __init tdx_event_irq_init(void)
>> +{
>> +       struct irq_affinity_desc desc;
>> +       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);
>> +
>> +       cpumask_set_cpu(smp_processor_id(), &desc.mask);
>> +
>> +       irq = __irq_domain_alloc_irqs(x86_vector_domain, -1, 1, cpu_to_node(0),
> 
> cpu_to_node(smp_processor_id())?
> 
>> +                                     &info, false, &desc);
>> +       if (irq <= 0) {
>> +               pr_err("Event notification IRQ allocation failed %d\n", irq);
>> +               return -EIO;
>> +       }
>> +
>> +       irq_set_handler(irq, handle_edge_irq);
>> +
>> +       /*
>> +        * The IRQ cannot be migrated because VMM always notifies the TDX
>> +        * guest on the same CPU on which the SetupEventNotifyInterrupt
>> +        * TDVMCALL is called. Set the IRQ with IRQF_NOBALANCING to prevent
>> +        * its affinity from being changed.
>> +        */
>> +       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);
>> +
>> +       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)
>>
>>
>>
>>
>> -- 
>> Sathyanarayanan Kuppuswamy
>> Linux Kernel Developer
> 
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Powered by blists - more mailing lists