lkml.org 
[lkml]   [2022]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 08/26] x86/tdx: Handle in-kernel MMIO
    On Tue, Dec 14, 2021 at 06:02:46PM +0300, Kirill A. Shutemov wrote:
    > In non-TDX VMs, MMIO is implemented by providing the guest a mapping
    > which will cause a VMEXIT on access and then the VMM emulating the
    > instruction that caused the VMEXIT. That's not possible in TDX guests
    > because it requires exposing guest register and memory state to
    > potentially malicious VMM.

    What does that mean exactly? Aren't TDX registers encrypted just like
    SEV-ES ones? If so, they can't really be exposed...

    > In TDX the MMIO regions are instead configured to trigger a #VE
    > exception in the guest. The guest #VE handler then emulates the MMIO
    > instruction inside the guest and converts them into a controlled

    s/them/it/

    > hypercall to the host.
    >
    > MMIO addresses can be used with any CPU instruction that accesses the

    s/the //

    > memory. This patch, however, covers only MMIO accesses done via io.h

    "Here are covered only the MMIO accesses ... "

    > helpers, such as 'readl()' or 'writeq()'.
    >
    > MMIO access via other means (like structure overlays) may result in
    > MMIO_DECODE_FAILED and an oops.

    Why? They won't cause a EXIT_REASON_EPT_VIOLATION #VE or?

    > AMD SEV has the same limitations to MMIO handling.

    See, the other guy is no better here. :-P

    > === Potential alternative approaches ===
    >
    > == Paravirtualizing all MMIO ==
    >
    > An alternative to letting MMIO induce a #VE exception is to avoid
    > the #VE in the first place. Similar to the port I/O case, it is
    > theoretically possible to paravirtualize MMIO accesses.
    >
    > Like the exception-based approach offered by this patch, a fully

    "... offered here, a fully ..."

    > paravirtualized approach would be limited to MMIO users that leverage
    > common infrastructure like the io.h macros.
    >
    > However, any paravirtual approach would be patching approximately
    > 120k call sites. With a conservative overhead estimation of 5 bytes per
    > call site (CALL instruction), it leads to bloating code by 600k.
    >
    > Many drivers will never be used in the TDX environment and the bloat
    > cannot be justified.

    I like the conservative approach here.

    > == 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. Future patches will implement this
    > idea,

    "This will be implemented in the future, ... "

    > +static int tdx_handle_mmio(struct pt_regs *regs, struct ve_info *ve)
    > +{
    > + char buffer[MAX_INSN_SIZE];
    > + unsigned long *reg, val = 0;
    > + struct insn insn = {};
    > + enum mmio_type mmio;
    > + int size;
    > + u8 sign_byte;
    > + bool err;
    > +
    > + if (copy_from_kernel_nofault(buffer, (void *)regs->ip, MAX_INSN_SIZE))
    > + return -EFAULT;
    > +
    > + insn_init(&insn, buffer, MAX_INSN_SIZE, 1);
    > + insn_get_length(&insn);

    There is insn_decode() - see how it is used and use it here pls.

    > + case MMIO_READ_SIGN_EXTEND:
    > + err = tdx_mmio_read(size, ve->gpa, &val);
    > + if (err)
    > + break;
    > +
    > + if (size == 1)
    > + sign_byte = (val & 0x80) ? 0xff : 0x00;
    > + else
    > + sign_byte = (val & 0x8000) ? 0xff : 0x00;
    > +
    > + /* Sign extend based on operand size */
    > + memset(reg, sign_byte, insn.opnd_bytes);
    > + memcpy(reg, &val, size);
    > + break;

    You can simplify this a bit:

    case MMIO_READ_SIGN_EXTEND: {
    u8 sign_byte = 0, msb = 7;

    err = tdx_mmio_read(size, ve->gpa, &val);
    if (err)
    break;

    if (size > 1)
    msb = 15;

    if (val & BIT(msb))
    sign_byte = -1;

    /* Sign extend based on operand size */
    memset(reg, sign_byte, insn.opnd_bytes);
    memcpy(reg, &val, size);
    break;
    }



    > + case MMIO_MOVS:
    > + case MMIO_DECODE_FAILED:
    > + return -EFAULT;
    > + }
    > +
    > + if (err)
    > + return -EFAULT;


    <---- newline here.

    > + return insn.length;
    > +}
    > +

    --
    Regards/Gruss,
    Boris.

    https://people.kernel.org/tglx/notes-about-netiquette

    \
     
     \ /
      Last update: 2022-01-05 11:39    [W:4.310 / U:0.492 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site