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

[Kuppuswamy, Sathyanarayanan]

On 5/10/21 4:34 PM, Dan Williams wrote:
> On Mon, May 10, 2021 at 4:08 PM Andi Kleen <ak@xxxxxxxxxxxxxxx> wrote:
> > On 5/10/2021 2:57 PM, Dan Williams wrote:
> > > There is a mix of direct-TDVMCALL usage and handling #VE when and why
> > > is either approached used?
> >
> > For the really early code in the decompressor or the main kernel we
> > can't use #VE because the IDT needed for handling the exception is not
> > set up, and some other infrastructure needed by the handler is missing.
> > The early code needs to do port IO to be able to write the early serial
> > console. To keep it all common it ended up that all port IO is paravirt.
> > Actually for most the main kernel port IO calls we could just use #VE
> > and it would result in smaller binaries, but then we would need to
> > annotate all early portio with some special name. That's why port IO is
> > all TDCALL.
>
> Thanks Andi. Sathya, please include the above in the next posting.

Will include it.

> > For some others the only thing that really has to be #VE is MMIO because
> > we don't want to annotate every MMIO read*/write* with an alternative
> > (which would result in incredible binary bloat) For the others they have
> > mostly become now direct calls.
> >
> >
> > > > Decompression code uses port IO for earlyprintk. We must use
> > > > paravirt calls there too if we want to allow earlyprintk.
> > > What is the tradeoff between teaching the decompression code to handle
> > > #VE (the implied assumption) vs teaching it to avoid #VE with direct
> > > TDVMCALLs (the chosen direction)?
> >
> > The decompression code only really needs it to output something. But you
> > couldn't debug anything until #VE is set up. Also the decompression code
> > has a very basic environment that doesn't supply most kernel services,
> > and the #VE handler is relatively complicated. It would probably need to
> > be duplicated and the instruction decoder be ported to work in this
> > environment. It would be all a lot of work, just to make the debug
> > output work.
> >
> > > > Co-developed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> > > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> > > > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> > > > Reviewed-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> > > > ---
> > > >    arch/x86/boot/compressed/Makefile |   1 +
> > > >    arch/x86/boot/compressed/tdcall.S |   9 ++
> > > >    arch/x86/include/asm/io.h         |   5 +-
> > > >    arch/x86/include/asm/tdx.h        |  46 ++++++++-
> > > >    arch/x86/kernel/tdcall.S          | 154 ++++++++++++++++++++++++++++++
> > > Why is this named "tdcall" when it is implementing tdvmcalls? I must
> > > say those names don't really help me understand what they do. Can we
> > > have Linux names that don't mandate keeping the spec terminology in my
> > > brain's translation cache?
> >
> > The instruction is called TDCALL. It's always the same instruction
> >
> > TDVMCALL is the variant when the host processes it (as opposed to the
> > TDX module), but it's just a different name space in the call number.
>
> Ok.
>
> >               \
> >
> > > Is there a unified Linux name these can be given to stop the
> > > proliferation of poor vendor names for similar concepts?
> >
> > We could use protected_guest()
>
> Looks good.
>
> > > Does it also not know how to handle #VE to keep it aligned with the
> > > runtime code?
> >
> >
> > Not sure I understand the question, but the decompression code supports
> > neither alternatives nor #VE. It's a very limited environment.
>
> Yes, that addresses the question.
>
> > > Outside the boot decompression code isn't this branch of the "ifdef
> > > BOOT_COMPRESSED_MISC_H"  handled by #VE? I also don't see any usage of
> > > __{in,out}() in this patch.
> >
> > I thought it was all alternative after decompression, so the #VE code
> > shouldn't be called. We still have it for some reason though.
>
> Right, I'm struggling to understand where these spurious in/out
> instructions are coming from that are not replaced by the
> alternative's code? Shouldn't those be dropped on the floor and warned
> about rather than handled? I.e. shouldn't port-io instruction escapes
> that would cause #VE be precluded at build-time?
>
> > > Perhaps "PAYLOAD_SIZE" since it is used for both input and output?
> > >
> > > If the ABI does not include the size of the payload then how would
> > > code detect if even 80 bytes was violated in the future?
> >
> >
> > The payload in memory is just a Linux concept. At the TDCALL level it's
> > only registers.
>
> If it's only a Linux concept why does this code need to "prepare for
> the future"?

It is the software only structure. It is created to group all the output
registers used by VMM. You can find more details about it in patch titled
# "[RFC v2 05/32] x86/tdx: Add __tdcall() and __tdvmcall() helper functions"

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


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

But currently we don't include the asm/tdx.h (which defines
struct tdx_hypercall_output) in tdcall.S. So I have defined the size as
constant value.

> > > 5
> > > Surely there's an existing macro for this pattern? Would
> > > PUSH_AND_CLEAR_REGS + POP_REGS be suitable? Besides code sharing it
> > > would eliminate clearing of %r8.
> >
> >
> > There used to be SAVE_ALL/SAVE_REGS, but they have been all removed in
> > some past refactorings.
>
> Not a huge deal, but at a minimum it seems a generic construct that
> deserves to be declared centrally rather than tdx-guest-port-io local.

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer