Received-SPF: pass (google.com: domain of linux-kernel+bounces-101707-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 13 Mar 2024 09:00:12 -0700
In-Reply-To: <9f820b96-0e4b-4cdc-93ff-f63aed829f0d@intel.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240219074733.122080-1-weijiang.yang@intel.com>
 <20240219074733.122080-21-weijiang.yang@intel.com> <ZfDdS8rtVtyEr0UR@google.com>
 <9f820b96-0e4b-4cdc-93ff-f63aed829f0d@intel.com>
Message-ID: <ZfHNjJWWsI0wpDRd@google.com>
Subject: Re: [PATCH v10 20/27] KVM: VMX: Emulate read and write to CET MSRs
From: Sean Christopherson <seanjc@google.com>
To: Weijiang Yang <weijiang.yang@intel.com>
Cc: pbonzini@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, 
	chao.gao@intel.com, rick.p.edgecombe@intel.com, mlevitsk@redhat.com, 
	john.allen@amd.com, Aaron Lewis <aaronlewis@google.com>, 
	Jim Mattson <jmattson@google.com>, Oliver Upton <oupton@google.com>, 
	Mingwei Zhang <mizhang@google.com>
Content-Type: text/plain; charset="us-ascii"

On Wed, Mar 13, 2024, Weijiang Yang wrote:
> On 3/13/2024 6:55 AM, Sean Christopherson wrote:
> > PERF_CAPABILITIES has a similar, but opposite, problem where KVM returns a non-zero
> > value on reads, but rejects that same non-zero value on write.  PERF_CAPABILITIES
> > is even more complicated because KVM stuff a non-zero value at vCPU creation, but
> > that's not really relevant to this discussion, just another data point for how
> > messed up this all is.
> > 
> > Also relevant to this discussion are KVM's PV MSRs, e.g. MSR_KVM_ASYNC_PF_ACK,
> > as KVM rejects attempts to write '0' if the guest doesn't support the MSR, but
> > if and only userspace has enabled KVM_CAP_ENFORCE_PV_FEATURE_CPUID.
> > 
> > Coming to the point, this mess is getting too hard to maintain, both from a code
> > perspective and "what is KVM's ABI?" perspective.
> > 
> > Rather than play whack-a-mole and inevitably end up with bugs and/or inconsistencies,
> > what if we (a) return KVM_MSR_RET_INVALID when an MSR access is denied based on
> > guest CPUID,
> 
> Can we define a new return value KVM_MSR_RET_REJECTED for this case in order
> to tell it from KVM_MSR_RET_INVALID which means the msr index doesn't exit?

No.  Well, I mean, we could, but I don't see any reason to define another return
value, because the semantics further up the stack need to be identical.  And
unfortunately, correctly differentiating between the two scenario would require
quite a bit of surgery to play nice with PMU MSRs.

Hmm, I suppose we could WARN if a _completely_ unhandled MSR ends up in the
msrs_to_save or emulated_msrs lists.  But because of the PMU MSRs complications,
this is definitely not worth doing right away, if ever.

> > static bool kvm_is_msr_to_save(u32 msr_index)
> > {
> > 	unsigned int i;
> > 
> > 	for (i = 0; i < num_msrs_to_save; i++) {
> > 		if (msrs_to_save[i] == msr_index)
> > 			return true;
> > 	}
> 
> Should we also check emulated_msrs list here since KVM_GET_MSR_INDEX_LIST
> exposes it too?

Ah, yes.  I was thinking msrs_to_save was a superset, but KVM_GET_MSR_INDEX_LIST
is where the lists get smushed together.