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

[Sathyanarayanan Kuppuswamy]

On 11/5/21 2:12 PM, Sean Christopherson wrote:
> On Tue, Oct 05, 2021, Kuppuswamy Sathyanarayanan wrote:
> > Signed-off-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> > Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> > Reviewed-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> > ---
>
> ...
>
> > 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