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.