On 9/16/2025 6:12 AM, Sean Christopherson wrote:
> On Mon, Sep 15, 2025, Xiaoyao Li wrote:
>> On 9/13/2025 7:22 AM, Sean Christopherson wrote:
>>> @@ -6097,11 +6105,22 @@ static int kvm_get_set_one_reg(struct kvm_vcpu *vcpu, unsigned int ioctl,
>>> static int kvm_get_reg_list(struct kvm_vcpu *vcpu,
>>> struct kvm_reg_list __user *user_list)
>>> {
>>> - u64 nr_regs = 0;
>>> + u64 nr_regs = guest_cpu_cap_has(vcpu, X86_FEATURE_SHSTK) ? 1 : 0;
>>
>> I wonder what's the semantic of KVM returning KVM_REG_GUEST_SSP on
>> KVM_GET_REG_LIST. Does it ensure KVM_{G,S}ET_ONE_REG returns -EINVAL on
>> KVM_REG_GUEST_SSP when it's not enumerated by KVM_GET_REG_LIST?
>>
>> If so, but KVM_{G,S}ET_ONE_REG can succeed on GUEST_SSP even if
>> !guest_cpu_cap_has() when @ignore_msrs is true.
>
> Ugh, great catch. Too many knobs. The best idea I've got it to to exempt KVM-
> internal MSRs from ignore_msrs and report_ignored_msrs on host-initiated writes.
> That's unfortunately still a userspace visible change, and would continue to be
> userspace-visible, e.g. if we wanted to change the magic value for
> MSR_KVM_INTERNAL_GUEST_SSP.
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c78acab2ff3f..6a50261d1c5c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -511,6 +511,11 @@ static bool kvm_is_advertised_msr(u32 msr_index)
> return false;
> }
>
> +static bool kvm_is_internal_msr(u32 msr)
> +{
> + return msr == MSR_KVM_INTERNAL_GUEST_SSP;
> +}
> +
> typedef int (*msr_access_t)(struct kvm_vcpu *vcpu, u32 index, u64 *data,
> bool host_initiated);
>
> @@ -544,6 +549,9 @@ static __always_inline int kvm_do_msr_access(struct kvm_vcpu *vcpu, u32 msr,
> if (host_initiated && !*data && kvm_is_advertised_msr(msr))
> return 0;
>
> + if (host_initiated && kvm_is_internal_msr(msr))
> + return ret;
> +
> if (!ignore_msrs) {
> kvm_debug_ratelimited("unhandled %s: 0x%x data 0x%llx\n",
> op, msr, *data);
>
> Alternatively, simply exempt host writes from ignore_msrs. Aha! And KVM even
> documents that as the behavior:
>
> kvm.ignore_msrs=[KVM] Ignore guest accesses to unhandled MSRs.
> Default is 0 (don't ignore, but inject #GP)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c78acab2ff3f..177253e75b41 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -544,7 +544,7 @@ static __always_inline int kvm_do_msr_access(struct kvm_vcpu *vcpu, u32 msr,
> if (host_initiated && !*data && kvm_is_advertised_msr(msr))
> return 0;
>
> - if (!ignore_msrs) {
> + if (host_initiated || !ignore_msrs) {
> kvm_debug_ratelimited("unhandled %s: 0x%x data 0x%llx\n",
> op, msr, *data);
> return ret;
>
> So while it's technically an ABI change (arguable since it's guarded by an
> off-by-default param), I suspect we can get away with it. Hmm, commit 6abe9c1386e5
> ("KVM: X86: Move ignore_msrs handling upper the stack") exempted KVM-internal
> MSR accesses from ignore_msrs, but doesn't provide much in the way of justification
> for _why_ that's desirable.
>
> Argh, and that same mini-series extended the behavior to feature MSRs, again
> without seeming to consider whether or not it's actually desirable to suppress
> bad VMM accesses. Even worse, that decision likely generated an absurd amount
> of churn and noise due to splattering helpers and variants all over the place. :-(
>
> commit 12bc2132b15e0a969b3f455d90a5f215ef239eff
> Author: Peter Xu <peterx@redhat.com>
> AuthorDate: Mon Jun 22 18:04:42 2020 -0400
> Commit: Paolo Bonzini <pbonzini@redhat.com>
> CommitDate: Wed Jul 8 16:21:40 2020 -0400
>
> KVM: X86: Do the same ignore_msrs check for feature msrs
>
> Logically the ignore_msrs and report_ignored_msrs should also apply to feature
> MSRs. Add them in.
>
> For 6.18, I think the safe play is to go with the first path (exempt KVM-internal
> MSRs), and then try to go for the second approach (exempt all host accesses) for
> 6.19. KVM's ABI for ignore_msrs=true is already all kinds of messed up, so I'm
> not terribly concerned about temporarily making it marginally worse.
Looks OK to me.