Re: [PATCH v3 11/18] KVM: x86/pmu: Emulate IA32_PERF_GLOBAL_STATUS_SET MSR

["Chen, Zide" <zide.chen@intel.com>]

On 11/6/2026 2:35 PM, Yosry Ahmed wrote:
> On Thu, Nov 05, 2026 at 06:39:20PM -0700, Zide Chen wrote:
>> Intel PerfMon v4 introduces IA32_PERF_GLOBAL_STATUS_SET (0x391) to
>> allow software to set individual bits in the global status MSR. Reads
>> of IA32_PERF_GLOBAL_STATUS_SET always return zero.
>>
>> IA32_PERF_GLOBAL_STATUS_SET is not part of the VM state, so it doesn't
>> need to be advertised to userspace, nor saved and restored during live
>> migration.
>>
>> Always intercept IA32_PERF_GLOBAL_STATUS_SET to prevent the guest from
>> modifying status bits that are not virtualized by KVM.
>>
>> Bit 63 (CondChgd) stays unreserved in pmu->global_status_rsvd to
>> architecturally allow the guest to write this bit on STATUS_RESET (0x390)
>> to clear it, and allow userspace to set it on GLOBAL_STATUS (0x38E).
>>
>> However, bit 63 is architecturally reserved in STATUS_SET per the SDM.
>> Reject writes to bit 63, injecting #GP for guest writes. While older
>> processors had silicon errata that accepted writes without #GP (e.g.
>> SKX95), strictly enforce the architectural specification for
>> deterministic behavior across migrations.
>>
>> Platform-specific EDS clarifies that CondChgd indicates a change to
>> IA32_MISC_ENABLE[7], which KVM intercepts and keeps strictly read-only
>> for guests. This guarantees hardware will never assert CondChgd during
>> guest execution.
>>
>> Thus, the architectural asymmetry among these three MSRs is safe; being
>> unable to restore bit 63 to hardware via STATUS_SET is benign because
>> the guest will never legitimately observe CondChgd. Filter it out
>> defensively before writing to hardware in intel_mediated_pmu_load().
>>
>> Originally-by: Yang Weijiang <weijiang.yang@intel.com>
>> Signed-off-by: Zide Chen <zide.chen@intel.com>
>> ---
>> v3:
>> - Split from combined v2 patch into standalone GLOBAL_STATUS_SET patch.
>> - Always intercept IA32_PERF_GLOBAL_STATUS_SET to prevent passthrough of
>>   unvirtualized status bits. (Jim)
>> - Reject writing bit 63 (CondChgd) to IA32_PERF_GLOBAL_STATUS_SET. (Jim)
>> - Filter out bit 63 before writing to hardware in intel_mediated_pmu_load().
>> - Move validation for MSR_CORE_PERF_GLOBAL_STATUS_SET to pmu_intel.c rather
>>   than common pmu.c to avoid exposing the Intel MSR on AMD.
>>
>> v2:
>> - Add the missing vmcs02 updates for these two MSRs.
>> - Change "> 3" to ">= 4" to make the "v4-gated" more obvious and match
>>   the existing code style.
>> ---
>>  arch/x86/kvm/pmu.c           | 13 +++++++++++++
>>  arch/x86/kvm/vmx/pmu_intel.c | 14 ++++++++++++--
>>  2 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
>> index 27d6a9169821..2cb5bccc7044 100644
>> --- a/arch/x86/kvm/pmu.c
>> +++ b/arch/x86/kvm/pmu.c
>> @@ -855,6 +855,7 @@ int kvm_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>  	case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR:
>>  	case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_SET:
>>  	case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
>> +	case MSR_CORE_PERF_GLOBAL_STATUS_SET:
>>  		msr_info->data = 0;
>>  		break;
>>  	default:
>> @@ -921,6 +922,18 @@ int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>  		if (!msr_info->host_initiated)
>>  			pmu->global_status &= ~data;
>>  		break;
>> +	case MSR_CORE_PERF_GLOBAL_STATUS_SET:
>> +		/*
>> +		 * Bit 63 (CondChgd) is architecturally reserved in STATUS_SET.
>> +		 * Older CPUs errantly accepted writes without #GP (e.g. SKX95),
>> +		 * but KVM strictly enforces the architectural spec to ensure
>> +		 * consistent behavior across migrations.
>> +		 */
>> +		if (data & (pmu->global_status_rsvd | GLOBAL_STATUS_COND_CHG))
>> +			return 1;
> 
> Is it intentional to disallow userspace from writing
> GLOBAL_STATUS_COND_CHG through MSR_CORE_PERF_GLOBAL_STATUS_SET, even tho
> it's allowed through MSR_CORE_PERF_GLOBAL_STATUS?

Yes, this is intentionally, since bit 63 is architecturally reserved in
MSR 0x391.

> 
>> +		if (!msr_info->host_initiated)
>> +			pmu->global_status |= data;
>> +		break;
>>  	case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_SET:
>>  		if (!msr_info->host_initiated)
>>  			pmu->global_status |= data & ~pmu->global_status_rsvd;
>> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
>> index ce668ea08da5..e81a3f8b9b02 100644
>> --- a/arch/x86/kvm/vmx/pmu_intel.c
>> +++ b/arch/x86/kvm/vmx/pmu_intel.c
>> @@ -197,6 +197,8 @@ static bool intel_is_valid_msr(struct kvm_vcpu *vcpu, u32 msr)
>>  	case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
>>  	case MSR_CORE_PERF_FIXED_CTR_CTRL:
>>  		return kvm_pmu_has_perf_global_ctrl(pmu);
>> +	case MSR_CORE_PERF_GLOBAL_STATUS_SET:
>> +		return pmu->version >= 4;
>>  	case MSR_IA32_PEBS_ENABLE:
>>  		ret = vcpu_get_perf_capabilities(vcpu) & PERF_CAP_PEBS_FORMAT;
>>  		break;
>> @@ -827,8 +829,16 @@ static void intel_mediated_pmu_load(struct kvm_vcpu *vcpu)
>>  	toggle = pmu->global_status ^ global_status;
>>  	if (global_status & toggle)
>>  		wrmsrq(MSR_CORE_PERF_GLOBAL_OVF_CTRL, global_status & toggle);
>> -	if (pmu->global_status & toggle)
>> -		wrmsrq(MSR_CORE_PERF_GLOBAL_STATUS_SET, pmu->global_status & toggle);
>> +
>> +	/*
>> +	 * CondChgd is architecturally reserved in GLOBAL_STATUS_SET. Unless
>> +	 * set by malicious userspace, this bit should never be set in
>> +	 * pmu->global_status; filter it out defensively to protect against
>> +	 * a host #GP.
> 
> IIUC, this will ignore bit 63 completely when written by userspace,
> should KVM be doing this?

Yes, if userspace writes GLOBAL_STATUS (0x38E) with bit 63 set, it would
be silently dropped here before writing to hardware.

Because KVM always intercepts IA32_MISC_ENABLE, bit 63 is not supposed
to be set for guest GLOBAL_STATUS.

The case you mention should not happen in reality, but theoretically it
exists and this sounds not clean enough.

I'm thinking of doing it differently: similar to the CTR_Frz bit, KVM
can keep bit 63 CondChgd reserved, and provide a backdoor to allow
writing this bit to 0x390:

diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index 393068093757..1429da7148bd 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -925,9 +925,11 @@ int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct
msr_data *msr_info)
        case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
-               if (data & pmu->global_status_rsvd)
+               if (data & (pmu->global_status_rsvd &
~GLOBAL_STATUS_COND_CHG))
                        return 1;
                fallthrough;
        case MSR_AMD64_PERF_CNTR_GLOBAL_STATUS_CLR:
@@ -935,13 +937,7 @@ int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct
msr_data *msr_info)
                        pmu->global_status &= ~data;
                break;
        case MSR_CORE_PERF_GLOBAL_STATUS_SET:
-               if (data & (pmu->global_status_rsvd |
GLOBAL_STATUS_COND_CHG))
+               if (data & pmu->global_status_rsvd)
                        return 1;
                if (!msr_info->host_initiated)
                        pmu->global_status |= data;
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 117ff54d4aea..0f426634373e 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -652,7 +652,7 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
        pmu->global_ctrl_rsvd = counter_rsvd;

        pmu->global_status_rsvd = pmu->global_ctrl_rsvd
-                       & ~(GLOBAL_STATUS_BUFFER_OVF |
GLOBAL_STATUS_COND_CHG);
+                       & ~GLOBAL_STATUS_BUFFER_OVF;
        if (vmx_pt_mode_is_host_guest())
                pmu->global_status_rsvd &= ~GLOBAL_STATUS_TRACE_TOPAPMI;

@@ -892,13 +892,7 @@ static void intel_mediated_pmu_load(struct kvm_vcpu
*vcpu)
        if (global_status & toggle)
                wrmsrq(MSR_CORE_PERF_GLOBAL_OVF_CTRL, global_status &
toggle);

-       /*
-        * CondChgd is architecturally reserved in GLOBAL_STATUS_SET. Unless
-        * set by malicious userspace, this bit should never be set in
-        * pmu->global_status; filter it out defensively to protect against
-        * a host #GP.
-        */
-       set_bits = (pmu->global_status & toggle) & ~GLOBAL_STATUS_COND_CHG;
+       set_bits = pmu->global_status & toggle;
        if (set_bits)
                wrmsrq(MSR_CORE_PERF_GLOBAL_STATUS_SET, set_bits);

>> +	 */
>> +	if ((pmu->global_status & toggle) & ~GLOBAL_STATUS_COND_CHG)
> 
> Maybe store this in a variable as it's used in wrmsrq() as well?

Yes, thought about it. One more variable vs cleaner code. Will do.

> 
>> +		wrmsrq(MSR_CORE_PERF_GLOBAL_STATUS_SET,
>> +		       (pmu->global_status & toggle) & ~GLOBAL_STATUS_COND_CHG);
>>  
>>  	wrmsrq(MSR_CORE_PERF_FIXED_CTR_CTRL, pmu->fixed_ctr_ctrl_hw);
>>  }
>> -- 
>> 2.56.0
>>