lkml.org 
[lkml]   [2022]   [Feb]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCHv4 09/30] x86/tdx: Add MSR support for TDX guests
    On 2/24/22 11:04, Sean Christopherson wrote:
    > On Thu, Feb 24, 2022, Dave Hansen wrote:
    >> On 2/24/22 07:56, Kirill A. Shutemov wrote:
    >>> diff --git a/arch/x86/coco/tdx.c b/arch/x86/coco/tdx.c
    >>> index 0a2e6be0cdae..89992593a209 100644
    >>> --- a/arch/x86/coco/tdx.c
    >>> +++ b/arch/x86/coco/tdx.c
    >>> @@ -116,6 +116,44 @@ void __cpuidle tdx_safe_halt(void)
    >>> WARN_ONCE(1, "HLT instruction emulation failed\n");
    >>> }
    >>>
    >>> +static bool read_msr(struct pt_regs *regs)
    >>> +{
    >>> + struct tdx_hypercall_args args = {
    >>> + .r10 = TDX_HYPERCALL_STANDARD,
    >>> + .r11 = EXIT_REASON_MSR_READ,
    >> Just a minor note: these "EXIT_REASON_FOO"'s in r11 are effectively
    >> *the* hypercall being made, right?
    >>
    >> The hypercall is being made in response to what would have otherwise
    >> been a MSR read VMEXIT. But, it's a *bit* goofy to see them here when
    >> the TDX guest isn't doing any kind of VMEXIT.
    > But the TDX guest is doing a VM-Exit, that's all TDCALL is, an exit to the host.
    > r10 states that this is a GHCI-standard hypercall, r11 holds the reason why the
    > guest is exiting to the host. The guest could pretty it up by redefining all the
    > VM-Exit reasons as TDX_REQUEST_MSR_READ or whatever, but IMO diverging from
    > directly using EXIT_REASON_* will be annoying in the long run, e.g. will make it
    > more difficult to grep KVM + kernel to understand the end-to-end flow.

    I understand that it looks like an "exit" if you know how it's
    implemented, know the history and squint at it funny. But, r11 is not
    an exit reason. It's a hypercall number that just sometimes happens to
    also take an exit reason as a convention. Don't confuse that with "r11
    *is* an exit reason".

    Heck, look at the GHCI spec. Does it simply cede some of the
    sub-function space and map them directly to VMEXIT reasons? Nope. It
    goes to the trouble of individually defining them:

    12 Instruction.HLT
    30 Instruction.IO
    31 Instruction.RDMSR
    32 Instruction.WRMSR
    48 #VE.RequestMMIO
    65 Instruction.PCONFIG

    I'm not saying we need 15 new #defines. It would be really nice like
    you say to be able to connect the host and guest sides with a grep. I
    wouldn't hate if we did something like:

    .r11 = hcall_func(EXIT_REASON_MSR_READ),

    That retains greppability, but also tells you that r11 is a function
    number. It even gives a nice place to stick a comment to say what the
    heck is going on:

    /*
    * The TDG.VP.VMCALL-Instruction-execution sub-functions are defined
    * independently from but are currently matched 1:1 with VMX
    * EXIT_REASONs. Reusing the KVM EXIT_REASON macros makes it easier to
    * connect the host and guest sides of these calls.
    */
    static u64 hcall_func(u64 exit_reason)
    {
    return exit_reason;
    }

    Like I said, this is all a minor note. But, things like this really go
    a long way for folks like me who don't spend our days looking at the KVM
    code or thinking deeply about how the hypercall is implemented.

    \
     
     \ /
      Last update: 2022-02-24 20:39    [W:3.309 / U:0.176 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site