Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 29 Mar 2023 09:47:15 -0700
In-Reply-To: <38601264-1957-579f-f156-c782bb9826cc@amd.com>
Mime-Version: 1.0
References: <20230206060545.628502-1-manali.shukla@amd.com>
 <20230206060545.628502-3-manali.shukla@amd.com> <ZB4AOaLRwSB0ClIH@google.com> <38601264-1957-579f-f156-c782bb9826cc@amd.com>
Message-ID: <ZCRrk0qsdv7rYqFq@google.com>
Subject: Re: [RFC PATCH kernel 2/2] KVM: SEV: PreventHostIBS enablement for
 SEV-ES and SNP guest
From:   Sean Christopherson <seanjc@google.com>
To:     Manali Shukla <manali.shukla@amd.com>
Cc:     linux-kernel@vger.kernel.org, peterz@infradead.org,
        mingo@redhat.com, acme@kernel.org, jolsa@kernel.org,
        namhyung@kernel.org, tglx@linutronix.de, bp@alien8.de,
        dave.hansen@linux.intel.com, pbonzini@redhat.com,
        jpoimboe@kernel.org, pawan.kumar.gupta@linux.intel.com,
        babu.moger@amd.com, sandipan.das@amd.com, jmattson@google.com,
        thomas.lendacky@amd.com, nikunj@amd.com, ravi.bangoria@amd.com,
        eranian@google.com, irogers@google.com, kvm@vger.kernel.org,
        x86@kernel.org, linux-perf-users@vger.kernel.org
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Wed, Mar 29, 2023, Manali Shukla wrote:
> On 3/25/2023 1:25 AM, Sean Christopherson wrote:
> > On Mon, Feb 06, 2023, Manali Shukla wrote:
> >> -	if (sev_es_guest(vcpu->kvm))
> >> +	if (sev_es_guest(vcpu->kvm)) {
> >> +		bool ibs_fetch_active, ibs_op_active;
> >> +		u64 ibs_fetch_ctl, ibs_op_ctl;
> >> +
> >> +		if (svm->prevent_hostibs_enabled) {
> >> +			/*
> >> +			 * With PreventHostIBS enabled, IBS profiling cannot
> >> +			 * be active when VMRUN is executed. Disable IBS before
> >> +			 * executing VMRUN and, because of a race condition,
> >> +			 * enable the PreventHostIBS window if IBS profiling was
> >> +			 * active.
> > 
> > And the race can't be fixed because...?
> 
> Race can not be fixed because VALID and ENABLE bit for IBS_FETCH_CTL and IBS_OP_CTL
> are contained in their same resepective MSRs. Due to this reason following scenario can
> be generated:
> Read IBS_FETCH_CTL (IbsFetchEn bit is 1 and IBSFetchVal bit is 0)
> Write IBS_FETCH_CTL (IbsFetchEn is 0 now)
> Imagine in between Read and Write, IBSFetchVal changes to 1. Write to IBS_FETCH_CTL will
> clear the IBSFetchVal bit. When STGI is executed after VMEXIT, the NMI is taken and check for
> valid mask will fail and generate Dazed and Confused NMI messages.
> Please refer to cover letter for more details.

I understand the race, I'm asking why this series doesn't fix the race.  Effectively
suppressing potentially unexpected NMIs because PreventHostIBS was enable is ugly.

> >> +			 */
> >> +			ibs_fetch_active =
> >> +				amd_disable_ibs_fetch(&ibs_fetch_ctl);
> >> +			ibs_op_active =
> >> +				amd_disable_ibs_op(&ibs_op_ctl);
> >> +
> >> +			amd_prevent_hostibs_window(ibs_fetch_active ||
> >> +						   ibs_op_active);
> >> +		}
> >> +
> >>  		__svm_sev_es_vcpu_run(svm, spec_ctrl_intercepted);
> >> -	else
> >> +
> >> +		if (svm->prevent_hostibs_enabled) {
> >> +			if (ibs_fetch_active)
> >> +				amd_restore_ibs_fetch(ibs_fetch_ctl);
> >> +
> >> +			if (ibs_op_active)
> >> +				amd_restore_ibs_op(ibs_op_ctl);
> > 
> > IIUC, this adds up to 2 RDMSRs and 4 WRMSRs to the VMRUN path.  Blech.  There's
> > gotta be a better way to implement this.  
> 
> I will try to find a better way to implement this.
> 
> > Like PeterZ said, this is basically
> > exclude_guest.
> 
> As I mentioned before, exclude_guest lets the profiler decide whether it wants to trace the guest
> data or not, whereas PreventHostIBS lets the owner of the guest decide whether host can trace guest's
> data or not.

PreventHostIBS is purely an enforcement, it does not actually do anything to
disable tracing of the guest.  What PeterZ and I are complaining about is that
instead of integrating this feature with exclude_guest, e.g. finding a way to
make guest tracing mutually exclusive with KVM_RUN so that PreventHostIBS can be
contexted switched according, this series instead backdoors into perf to forcefully
disable tracing.

In other words, please try to create a sane contract between userspace, perf, and
KVM, e.g. disallow tracing a guest with PreventHostIBS at some level instead of
silently toggling tracing around VMRUN.