linux-kernel - Re: [PATCH 4/7] KVM: TDX: Handle TDG.VP.VMCALL<MapGPA>

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <692aacc1-809f-449d-8f67-8e8e7ede8c8d@linux.intel.com>
Date: Mon, 16 Dec 2024 09:08:29 +0800
From: Binbin Wu <binbin.wu@...ux.intel.com>
To: Xiaoyao Li <xiaoyao.li@...el.com>
Cc: pbonzini@...hat.com, seanjc@...gle.com, kvm@...r.kernel.org,
 rick.p.edgecombe@...el.com, kai.huang@...el.com, adrian.hunter@...el.com,
 reinette.chatre@...el.com, tony.lindgren@...ux.intel.com,
 isaku.yamahata@...el.com, yan.y.zhao@...el.com, chao.gao@...el.com,
 michael.roth@....com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/7] KVM: TDX: Handle TDG.VP.VMCALL<MapGPA>




On 12/13/2024 5:32 PM, Xiaoyao Li wrote:
> On 12/1/2024 11:53 AM, Binbin Wu wrote:
>
[...]
>> +
>> +static int tdx_map_gpa(struct kvm_vcpu *vcpu)
>> +{
>> +    struct vcpu_tdx * tdx = to_tdx(vcpu);
>> +    u64 gpa = tdvmcall_a0_read(vcpu);
>
> We can use kvm_r12_read() directly, which is more intuitive. And we can drop the MACRO for a0/a1/a2/a3 accessors in patch 022.
I am neutral about it.


>
>> +    u64 size = tdvmcall_a1_read(vcpu);
>> +    u64 ret;
>> +
>> +    /*
>> +     * Converting TDVMCALL_MAP_GPA to KVM_HC_MAP_GPA_RANGE requires
>> +     * userspace to enable KVM_CAP_EXIT_HYPERCALL with KVM_HC_MAP_GPA_RANGE
>> +     * bit set.  If not, the error code is not defined in GHCI for TDX, use
>> +     * TDVMCALL_STATUS_INVALID_OPERAND for this case.
>> +     */
>> +    if (!user_exit_on_hypercall(vcpu->kvm, KVM_HC_MAP_GPA_RANGE)) {
>> +        ret = TDVMCALL_STATUS_INVALID_OPERAND;
>> +        goto error;
>> +    }
>> +
>> +    if (gpa + size <= gpa || !kvm_vcpu_is_legal_gpa(vcpu, gpa) ||
>> +        !kvm_vcpu_is_legal_gpa(vcpu, gpa + size -1) ||
>> +        (vt_is_tdx_private_gpa(vcpu->kvm, gpa) !=
>> +         vt_is_tdx_private_gpa(vcpu->kvm, gpa + size -1))) {
>> +        ret = TDVMCALL_STATUS_INVALID_OPERAND;
>> +        goto error;
>> +    }
>> +
>> +    if (!PAGE_ALIGNED(gpa) || !PAGE_ALIGNED(size)) {
>> +        ret = TDVMCALL_STATUS_ALIGN_ERROR;
>> +        goto error;
>> +    }
>> +
>> +    tdx->map_gpa_end = gpa + size;
>> +    tdx->map_gpa_next = gpa;
>> +
>> +    __tdx_map_gpa(tdx);
>> +    /* Forward request to userspace. */
>> +    return 0;
>
> Maybe let __tdx_map_gpa() return a int to decide whether it needs to return to userspace and
>
>     return __tdx_map_gpa(tdx);
>
> ?

To save one line of code and the comment?
Because MapGPA always goes to userspace, I don't want to make a function return
a int that is a fixed value.
And if the multiple comments bother you, I think the comments can be removed.

>
>
>> +
>> +error:
>> +    tdvmcall_set_return_code(vcpu, ret);
>> +    kvm_r11_write(vcpu, gpa);
>> +    return 1;
>> +}
>> +
>>   static int handle_tdvmcall(struct kvm_vcpu *vcpu)
>>   {
>>       if (tdvmcall_exit_type(vcpu))
>>           return tdx_emulate_vmcall(vcpu);
>>         switch (tdvmcall_leaf(vcpu)) {
>> +    case TDVMCALL_MAP_GPA:
>> +        return tdx_map_gpa(vcpu);
>>       default:
>>           break;
>>       }
>> diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
>> index 1abc94b046a0..bfae70887695 100644
>> --- a/arch/x86/kvm/vmx/tdx.h
>> +++ b/arch/x86/kvm/vmx/tdx.h
>> @@ -71,6 +71,9 @@ struct vcpu_tdx {
>>         enum tdx_prepare_switch_state prep_switch_state;
>>       u64 msr_host_kernel_gs_base;
>> +
>> +    u64 map_gpa_next;
>> +    u64 map_gpa_end;
>>   };
>>     void tdh_vp_rd_failed(struct vcpu_tdx *tdx, char *uclass, u32 field, u64 err);
>