From:   Thomas Gleixner <tglx@linutronix.de>
To:     isaku.yamahata@intel.com, Ingo Molnar <mingo@redhat.com>,
        Borislav Petkov <bp@alien8.de>,
        "H . Peter Anvin" <hpa@zytor.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>, erdemaktas@google.com,
        Connor Kuehl <ckuehl@redhat.com>,
        Sean Christopherson <seanjc@google.com>,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Cc:     isaku.yamahata@intel.com, isaku.yamahata@gmail.com,
        Chao Gao <chao.gao@intel.com>
Subject: Re: [RFC PATCH v3 53/59] KVM: x86: Add a helper function to restore
 4 host MSRs on exit to user space
In-Reply-To: <4ede5c987a4ae938a37ab7fe70d5e1d561ee97d4.1637799475.git.isaku.yamahata@intel.com>
References: <cover.1637799475.git.isaku.yamahata@intel.com>
 <4ede5c987a4ae938a37ab7fe70d5e1d561ee97d4.1637799475.git.isaku.yamahata@intel.com>
Date:   Thu, 25 Nov 2021 21:34:59 +0100
Message-ID: <878rxcht3g.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Wed, Nov 24 2021 at 16:20, isaku yamahata wrote:
> From: Chao Gao <chao.gao@intel.com>

> $Subject: KVM: x86: Add a helper function to restore 4 host MSRs on exit to user space

Which user space are you talking about? This subject line is misleading
at best. The unconditional reset is happening when a TDX VM exits
because the SEAM firmware enforces this to prevent unformation leaks.

It also does not matter whether this are four or ten MSR. Fact is that
the SEAM firmware is buggy because it does not save/restore those MSRs.

So the proper subject line is:

   KVM: x86: Add infrastructure to handle MSR corruption by broken TDX firmware 

> The TDX module unconditionally reset 4 host MSRs (MSR_SYSCALL_MASK,
> MSR_START, MSR_LSTAR, MSR_TSC_AUX) to architectural INIT state on exit from
> TDX VM to KVM.  KVM needs to save their values before TD enter and restore
> them on exit to userspace.
>
> Reuse current kvm_user_return mechanism and introduce a function to update
> cached values and register the user return notifier in this new function.
>
> The later patch will use the helper function to save/restore 4 host
> MSRs.

'The later patch ...' is useless information. Of course there will be a
later patch to make use of this which is implied by 'Add infrastructure
...'. Can we please get rid of these useless phrases which have no value
at patch submission time and are even more confusing once the pile is
merged?

Thanks,

        tglx