Re: [PATCH 11/16] KVM: TDX: Always block INIT/SIPI

[Binbin Wu <binbin.wu@linux.intel.com>]

On 1/8/2025 3:21 PM, Xiaoyao Li wrote:
> On 12/9/2024 9:07 AM, Binbin Wu wrote:
>> From: Isaku Yamahata <isaku.yamahata@intel.com>
>>
>> Always block INIT and SIPI events for the TDX guest because the TDX module
>> doesn't provide API for VMM to inject INIT IPI or SIPI.
>>
>> TDX defines its own vCPU creation and initialization sequence including
>> multiple seamcalls.  Also, it's only allowed during TD build time.
>>
>> Given that TDX guest is para-virtualized to boot BSP/APs, normally there
>> shouldn't be any INIT/SIPI event for TDX guest.  If any, three options to
>> handle them:
>> 1. Always block INIT/SIPI request.
>> 2. (Silently) ignore INIT/SIPI request during delivery.
>> 3. Return error to guest TDs somehow.
>>
>> Choose option 1 for simplicity. Since INIT and SIPI are always blocked,
>> INIT handling and the OP vcpu_deliver_sipi_vector() won't be called, no
>> need to add new interface or helper function for INIT/SIPI delivery.
>>
>> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
>> Co-developed-by: Binbin Wu <binbin.wu@linux.intel.com>
>> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
>> ---
>> TDX interrupts breakout:
>> - Renamed from "KVM: TDX: Silently ignore INIT/SIPI" to
>>    "KVM: TDX: Always block INIT/SIPI".
>> - Remove KVM_BUG_ON() in tdx_vcpu_reset(). (Rick)
>> - Drop tdx_vcpu_reset() and move the comment to vt_vcpu_reset().
>> - Remove unnecessary interface and helpers to delivery INIT/SIPI
>>    because INIT/SIPI events are always blocked for TDX. (Binbin)
>> - Update changelog.
>> ---
>>   arch/x86/kvm/lapic.c    |  2 +-
>>   arch/x86/kvm/vmx/main.c | 19 ++++++++++++++++++-
>>   2 files changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 474e0a7c1069..f93c382344ee 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -3365,7 +3365,7 @@ int kvm_apic_accept_events(struct kvm_vcpu *vcpu)
>>         if (test_and_clear_bit(KVM_APIC_INIT, &apic->pending_events)) {
>>           kvm_vcpu_reset(vcpu, true);
>> -        if (kvm_vcpu_is_bsp(apic->vcpu))
>> +        if (kvm_vcpu_is_bsp(vcpu))
>>               vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
>>           else
>>               vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
>> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
>> index 8ec96646faec..7f933f821188 100644
>> --- a/arch/x86/kvm/vmx/main.c
>> +++ b/arch/x86/kvm/vmx/main.c
>> @@ -115,6 +115,11 @@ static void vt_vcpu_free(struct kvm_vcpu *vcpu)
>>     static void vt_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>>   {
>> +    /*
>> +     * TDX has its own sequence to do init during TD build time (by
>> +     * KVM_TDX_INIT_VCPU) and it doesn't support INIT event during TD
>> +     * runtime.
>> +     */
>
> The first half is confusing. It seems to mix up init(ialization) with INIT event.
>
> And this callback is about *reset*, which can be due to INIT event or not. That's why it has a second parameter of init_event. The comment needs to clarify why reset is not needed for both cases.
>
> I think we can just say TDX doesn't support vcpu reset no matter due to INIT event or not.
>
>
Will update it.
Thanks!