Re: [PATCH v7 04/10] x86/tdx: Handle early IO operations

Sathyanarayanan Kuppuswamy
2021-11-05T23:09:05Z

On 11/5/21 2:12 PM, Sean Christopherson wrote:
> On Tue, Oct 05, 2021, Kuppuswamy Sathyanarayanan wrote:
>> Signed-off-by: Andi Kleen <ak@linux.intel.com>
>> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
>> Reviewed-by: Andi Kleen <ak@linux.intel.com>
>
> Heh, is Andi double-dipping to pad his stats? :-D

Sorry, it was my mistake. I will remove it.

>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>
> ...
>
>> diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
>> index 11e367228e96..4cbffcb737d9 100644
>> --- a/arch/x86/kernel/tdx.c
>> +++ b/arch/x86/kernel/tdx.c
>> @@ -10,6 +10,11 @@
>> /* TDX Module call Leaf IDs */
>> #define TDGETVEINFO 3
>>
>> +#define VE_IS_IO_OUT(exit_qual) (((exit_qual) & 8) ? 0 : 1)
>> +#define VE_GET_IO_SIZE(exit_qual) (((exit_qual) & 7) + 1)
>> +#define VE_GET_PORT_NUM(exit_qual) ((exit_qual) >> 16)
>> +#define VE_IS_IO_STRING(exit_qual) ((exit_qual) & 16 ? 1 : 0)
>> +
>> /*
>> * Allocate it in the data region to avoid zeroing it during
>> * BSS initialization. It is mainly used in cc_platform_has()
>> @@ -228,6 +233,61 @@ int tdx_handle_virtualization_exception(struct pt_regs *regs,
>> return ret;
>> }
>>
>> +/*
>> + * Handle early IO, mainly for early printks serial output.
>> + * This avoids anything that doesn't work early on, like tracing
>> + * or printks, by calling the low level functions directly. Any
>> + * problems are handled by falling back to a standard early exception.
>> + *
>> + * Assumes the IO instruction was using ax, which is enforced
>> + * by the standard io.h macros.
>> + */
>> +static __init bool tdx_early_io(struct pt_regs *regs, u32 exit_qual)
>> +{
>> + struct tdx_hypercall_output outh;
>
> "outh" looks like a typo. Maybe "result" or something alongs those lines?

I have fixed it (will be part of next submission). We are going to use
out here and change

out = VE_IS_IO_OUT(exit_qual);

to

in = VE_IS_IO_IN(exit_qual);

>
>> + int out, size, port, ret;
>> + bool string;
>> + u64 mask;
>> +
>> + string = VE_IS_IO_STRING(exit_qual);
>> +
>> + /* I/O strings ops are unrolled at build time. */
>> + if (string)
>
> Why bother with "string"?
>
> if (VE_IS_IO_STRING(exit_qual))
> return false;
>
>> + return 0;
>
> Ugh. This needs to be "return false". "return 0" in the kernel usually means
> success, but this horror returns a bool where "false" is failure.

It will be fixed in next version.

>
>> +
>> + out = VE_IS_IO_OUT(exit_qual);
>> + size = VE_GET_IO_SIZE(exit_qual);
>> + port = VE_GET_PORT_NUM(exit_qual);
>> + mask = GENMASK(8 * size, 0);
>
> size * BITS_PER_BYTE

Ok. I will fix this in next version.

>
>> +
>> + ret = _tdx_hypercall(EXIT_REASON_IO_INSTRUCTION, size, out, port,
>> + regs->ax, &outh);
>
> This unnecessarily exposes RAX to the untrusted VMM for IN.

Yes. I will remove it.

>
>> + if (!out && !ret) {
>> + regs->ax &= ~mask;
>> + regs->ax |= outh.r11 & mask;
>> + }
>> +
>> + return !ret;
>> +}

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer