Re: [PATCHv4 11/30] x86/tdx: Handle in-kernel MMIO

[Kirill A. Shutemov]

On Thu, Feb 24, 2022 at 12:11:54PM -0800, Dave Hansen wrote:
> 
> I found a few things lacking in that description.  How's that for a rewrite?

Looks great, thanks.
> > == Patching TDX drivers ==
> > 
> > Rather than touching the entire kernel, it might also be possible to
> > just go after drivers that use MMIO in TDX guests.  Right now, that's
> > limited only to virtio and some x86-specific drivers.
> > 
> > All virtio MMIO appears to be done through a single function, which
> > makes virtio eminently easy to patch. This will be implemented in the
> > future, removing the bulk of MMIO #VEs.
> 
> Given what is written here, this sounds like a great solution especially
> compared to all the instruction decoding nasiness.  What's wrong with it?

This will not cover non-virtio users. So #VE-based MMIO will remain as
fallback mechanism.

> > +	switch (mmio) {
> > +	case MMIO_WRITE:
> > +		memcpy(&val, reg, size);
> > +		return mmio_write(size, ve->gpa, val);
> > +	case MMIO_WRITE_IMM:
> > +		val = insn.immediate.value;
> > +		return mmio_write(size, ve->gpa, val);
> > +	case MMIO_READ:
> > +	case MMIO_READ_ZERO_EXTEND:
> > +	case MMIO_READ_SIGN_EXTEND:
> > +		break;
> > +	case MMIO_MOVS:
> > +	case MMIO_DECODE_FAILED:
> > +		return false;
> > +	default:
> > +		BUG();
> > +	}
> 
> Given the huge description above, it's borderline criminal to not
> discuss what could led to this BUG().

This bug actually covers "Unknown insn_decode_mmio() decode value" case.
I will add the comment.


> It could literally be some minor tweak in the compiler that changed a
> non-io.h-using MMIO access to get converted over to a instruction that
> can't be decoded.
> 
> Could we spend a few lines of comments to help out the future poor sod
> that sees "kernel bug at foo.c:1234"?  Maybe:
> 
> 	/*
> 	 * MMIO was accessed with an instruction that could not
> 	 * be decoded.  It was likely not using io.h helpers or
> 	 * accessed MMIO accidentally.
> 	 */

Thanks, I will use the comment for MMIO_DECODE_FAILED handling.

-- 
 Kirill A. Shutemov