Re: [RFC v2 14/32] x86/tdx: Handle port I/O

[Kuppuswamy, Sathyanarayanan]

On 5/10/21 6:07 PM, Dan Williams wrote:
> On Mon, May 10, 2021 at 5:30 PM Kuppuswamy, Sathyanarayanan
> <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> wrote:
> [..]
> > It is mainly used by functions like __tdx_hypercall(),__tdx_hypercall_vendor_kvm()
> > and tdx_in{b,w,l}.
> >
> > u64 __tdx_hypercall(u64 fn, u64 r12, u64 r13, u64 r14, u64 r15,
> >                       struct tdx_hypercall_output *out);
> > u64 __tdx_hypercall_vendor_kvm(u64 fn, u64 r12, u64 r13, u64 r14,
> >                                  u64 r15, struct tdx_hypercall_output *out);
> >
> > struct tdx_hypercall_output {
> >           u64 r11;
> >           u64 r12;
> >           u64 r13;
> >           u64 r14;
> >           u64 r15;
> > };
>
> Why is this by register name and not something like:
>
> struct tdx_hypercall_payload {
>    u64 data[5];
> };
>
> ...because the code in this patch is reading the payload out of a
> stack relative offset, not r11.

Since this patch allocates this memory in ASM code, we read it via
offset. If you see other use cases in tdx.c, you will notice the use
of register names.

static void tdg_handle_cpuid(struct pt_regs *regs)
{
        u64 ret;
        struct tdx_hypercall_output out = {0};

        ret = __tdx_hypercall(EXIT_REASON_CPUID, regs->ax,
                              regs->cx, 0, 0, &out);

        WARN_ON(ret);

        regs->ax = out.r12;
        regs->bx = out.r13;
        regs->cx = out.r14;
        regs->dx = out.r15;
}

static u64 tdg_read_msr_safe(unsigned int msr, int *err)
{
        u64 ret;
        struct tdx_hypercall_output out = {0};

        WARN_ON_ONCE(tdg_is_context_switched_msr(msr));

        /*
         * Since CSTAR MSR is not used by Intel CPUs as SYSCALL
         * instruction, just ignore it. Even raising TDVMCALL
         * will lead to same result.
         */
        if (msr == MSR_CSTAR)
                return 0;

        ret = __tdx_hypercall(EXIT_REASON_MSR_READ, msr, 0, 0, 0, &out);

        *err = ret ? -EIO : 0;

        return out.r11;
}


> > Functions like __tdx_hypercall() and __tdx_hypercall_vendor_kvm() are used
> > by TDX guest to request services (like RDMSR, WRMSR,GetQuote, etc) from VMM
> > using TDCALL instruction. do_tdx_hypercall() is the helper function (in
> > tdcall.S) which actually implements this ABI.
> >
> > As per current ABI, VMM will use registers R11-R15 to share the output
> > values with the guest.
>
> Which ABI,

TDCALL ABI (see sections 3.1 to 3.12 and look for Output Operands in each TDVMCALL variant).

https://software.intel.com/content/dam/develop/external/us/en/documents/intel-tdx-guest-hypervisor-communication-interface.pdf

 __tdx_hypercall_vendor_kvm()? The code is putting the
> payload on the stack, so I'm not sure what ABI you are referring to?
>
>
> > So we have defined the structure
> > struct tdx_hypercall_output to group all output registers and make it easier
> > to share it with users of the TDCALLs. This is Linux defined structure.
> >
> > If there are any changes in TDCALL ABI for VMM, we might have to extend
> > this structure to accommodate new output register changes.  So if we
> > define TDVMCALL_OUTPUT_SIZE as 40, we will have modify this value for
> > any future struct tdx_hypercall_output changes. So to avoid it, we have
> > allocated double the size.
> >
> > May be I should define it as,
> >
> > #define TDVMCALL_OUTPUT_SIZE            sizeof(struct tdx_hypercall_output)
>
> An arrangement like that seems more reasonable than a seemingly
> arbitrary number and an ominous warning about things that may happen
> in the future.

I will use the above format.

>

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer