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

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 12/16/2024 9:08 AM, Binbin Wu wrote:
> 
> 
> 
> 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.
> 

a0, a1, a2, a3, are the name convention for KVM's hypercall. It makes 
sense when serving as the parameters to  __kvm_emulate_hypercall().

However, now __kvm_emulate_hypercall() is made to a MACRO and we don't 
need the temp variable like a0 = kvm_xx_read().

For TDVMCALL leafs other than normal KVM hypercalls, they are all TDX 
specific and defined in TDX GHCI spec, a0/a1/a2/a3 makes no sense for them.

> 
>>
>>> +    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?

No. Just I found most of the cases that need to exit to usespace, comes 
with "return 0" after setting up the run->exit_reason and run->(fields).

> 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);
>>
>