Re: [PATCH v19 109/130] KVM: TDX: Handle TDX PV port io hypercall

Binbin Wu
2024-07-09T06:26:42Z

On 4/18/2024 4:10 AM, Isaku Yamahata wrote:
> On Wed, Apr 17, 2024 at 08:51:39PM +0800,
> Binbin Wu <binbin.wu@linux.intel.com> wrote:
>
>>
>> On 2/26/2024 4:26 PM, isaku.yamahata@intel.com wrote:
>>> From: Isaku Yamahata <isaku.yamahata@intel.com>
>>>
>>> Wire up TDX PV port IO hypercall to the KVM backend function.
>>>
>>> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>> v18:
>>> - Fix out case to set R10 and R11 correctly when user space handled port
>>> out.
>>> ---
>>> arch/x86/kvm/vmx/tdx.c | 67 ++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 67 insertions(+)
>>>
>>> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
>>> index a2caf2ae838c..55fc6cc6c816 100644
>>> --- a/arch/x86/kvm/vmx/tdx.c
>>> +++ b/arch/x86/kvm/vmx/tdx.c
>>> @@ -1152,6 +1152,71 @@ static int tdx_emulate_hlt(struct kvm_vcpu *vcpu)
>>> return kvm_emulate_halt_noskip(vcpu);
>>> }
>>> +static int tdx_complete_pio_out(struct kvm_vcpu *vcpu)
>>> +{
>>> + tdvmcall_set_return_code(vcpu, TDVMCALL_SUCCESS);
>>> + tdvmcall_set_return_val(vcpu, 0);
>>> + return 1;
>>> +}
>>> +
>>> +static int tdx_complete_pio_in(struct kvm_vcpu *vcpu)
>>> +{
>>> + struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
>>> + unsigned long val = 0;
>>> + int ret;
>>> +
>>> + WARN_ON_ONCE(vcpu->arch.pio.count != 1);
>>> +
>>> + ret = ctxt->ops->pio_in_emulated(ctxt, vcpu->arch.pio.size,
>>> + vcpu->arch.pio.port, &val, 1);
>>> + WARN_ON_ONCE(!ret);
>>> +
>>> + tdvmcall_set_return_code(vcpu, TDVMCALL_SUCCESS);
>>> + tdvmcall_set_return_val(vcpu, val);
>>> +
>>> + return 1;
>>> +}
>>> +
>>> +static int tdx_emulate_io(struct kvm_vcpu *vcpu)
>>> +{
>>> + struct x86_emulate_ctxt *ctxt = vcpu->arch.emulate_ctxt;
>>> + unsigned long val = 0;
>>> + unsigned int port;
>>> + int size, ret;
>>> + bool write;
>>> +
>>> + ++vcpu->stat.io_exits;
>>> +
>>> + size = tdvmcall_a0_read(vcpu);
>>> + write = tdvmcall_a1_read(vcpu);
>>> + port = tdvmcall_a2_read(vcpu);
>>> +
>>> + if (size != 1 && size != 2 && size != 4) {
>>> + tdvmcall_set_return_code(vcpu, TDVMCALL_INVALID_OPERAND);
>>> + return 1;
>>> + }
>>> +
>>> + if (write) {
>>> + val = tdvmcall_a3_read(vcpu);
>>> + ret = ctxt->ops->pio_out_emulated(ctxt, size, port, &val, 1);
>>> +
>>> + /* No need for a complete_userspace_io callback. */
>> I am confused about the comment.
>>
>> The code below sets the complete_userspace_io callback for write case,
>> i.e. tdx_complete_pio_out().
> You're correct. This comment is stale and should be removed it.
Also, since the tdx_complete_pio_out() is installed as
complete_userspace_io callback for write, it's more reasonable to move
the reset of pio.count into tdx_complete_pio_out().
How about the following fixup:

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 9ead46cb75ab..b43bb8ccddb9 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -1115,6 +1115,7 @@ static int tdx_emulate_hlt(struct kvm_vcpu *vcpu)

static int tdx_complete_pio_out(struct kvm_vcpu *vcpu)
{
+       vcpu->arch.pio.count = 0;
tdvmcall_set_return_code(vcpu, TDVMCALL_SUCCESS);
tdvmcall_set_return_val(vcpu, 0);
return 1;
@@ -1159,15 +1160,13 @@ static int tdx_emulate_io(struct kvm_vcpu *vcpu)
if (write) {
val = tdvmcall_a3_read(vcpu);
ret = ctxt->ops->pio_out_emulated(ctxt, size, port,
&val, 1);
-
-               /* No need for a complete_userspace_io callback. */
-               vcpu->arch.pio.count = 0;
-       } else
+       } else {
ret = ctxt->ops->pio_in_emulated(ctxt, size, port,
&val, 1);
+       }

-       if (ret)
+       if (ret) {
tdvmcall_set_return_val(vcpu, val);
-       else {
+       } else {
if (write)
vcpu->arch.complete_userspace_io =
tdx_complete_pio_out;
else