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

Kirill A. Shutemov
2022-03-02T13:42:39Z

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