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

[Binbin Wu]

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