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

[Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com>]

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