On 27.02.2023 08:56, Xenia Ragiadakou wrote:
> Add hvm_funcs hooks for {set,clear}_msr_intercept() for controlling the msr
> intercept in common vpmu code.
What is this going to buy us? All calls ...
> --- a/xen/arch/x86/cpu/vpmu_amd.c
> +++ b/xen/arch/x86/cpu/vpmu_amd.c
> @@ -165,9 +165,9 @@ static void amd_vpmu_set_msr_bitmap(struct vcpu *v)
>
> for ( i = 0; i < num_counters; i++ )
> {
> - svm_clear_msr_intercept(v, counters[i], MSR_RW);
> - svm_set_msr_intercept(v, ctrls[i], MSR_W);
> - svm_clear_msr_intercept(v, ctrls[i], MSR_R);
> + hvm_clear_msr_intercept(v, counters[i], MSR_RW);
> + hvm_set_msr_intercept(v, ctrls[i], MSR_W);
> + hvm_clear_msr_intercept(v, ctrls[i], MSR_R);
> }
>
> msr_bitmap_on(vpmu);
> @@ -180,8 +180,8 @@ static void amd_vpmu_unset_msr_bitmap(struct vcpu *v)
>
> for ( i = 0; i < num_counters; i++ )
> {
> - svm_set_msr_intercept(v, counters[i], MSR_RW);
> - svm_set_msr_intercept(v, ctrls[i], MSR_RW);
> + hvm_set_msr_intercept(v, counters[i], MSR_RW);
> + hvm_set_msr_intercept(v, ctrls[i], MSR_RW);
> }
>
> msr_bitmap_off(vpmu);
... here will got to the SVM functions anyway, while ...
> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
> @@ -230,22 +230,22 @@ static void core2_vpmu_set_msr_bitmap(struct vcpu *v)
>
> /* Allow Read/Write PMU Counters MSR Directly. */
> for ( i = 0; i < fixed_pmc_cnt; i++ )
> - vmx_clear_msr_intercept(v, MSR_CORE_PERF_FIXED_CTR0 + i, MSR_RW);
> + hvm_clear_msr_intercept(v, MSR_CORE_PERF_FIXED_CTR0 + i, MSR_RW);
>
> for ( i = 0; i < arch_pmc_cnt; i++ )
> {
> - vmx_clear_msr_intercept(v, MSR_IA32_PERFCTR0 + i, MSR_RW);
> + hvm_clear_msr_intercept(v, MSR_IA32_PERFCTR0 + i, MSR_RW);
>
> if ( full_width_write )
> - vmx_clear_msr_intercept(v, MSR_IA32_A_PERFCTR0 + i, MSR_RW);
> + hvm_clear_msr_intercept(v, MSR_IA32_A_PERFCTR0 + i, MSR_RW);
> }
>
> /* Allow Read PMU Non-global Controls Directly. */
> for ( i = 0; i < arch_pmc_cnt; i++ )
> - vmx_clear_msr_intercept(v, MSR_P6_EVNTSEL(i), MSR_R);
> + hvm_clear_msr_intercept(v, MSR_P6_EVNTSEL(i), MSR_R);
>
> - vmx_clear_msr_intercept(v, MSR_CORE_PERF_FIXED_CTR_CTRL, MSR_R);
> - vmx_clear_msr_intercept(v, MSR_IA32_DS_AREA, MSR_R);
> + hvm_clear_msr_intercept(v, MSR_CORE_PERF_FIXED_CTR_CTRL, MSR_R);
> + hvm_clear_msr_intercept(v, MSR_IA32_DS_AREA, MSR_R);
> }
>
> static void core2_vpmu_unset_msr_bitmap(struct vcpu *v)
> @@ -253,21 +253,21 @@ static void core2_vpmu_unset_msr_bitmap(struct vcpu *v)
> unsigned int i;
>
> for ( i = 0; i < fixed_pmc_cnt; i++ )
> - vmx_set_msr_intercept(v, MSR_CORE_PERF_FIXED_CTR0 + i, MSR_RW);
> + hvm_set_msr_intercept(v, MSR_CORE_PERF_FIXED_CTR0 + i, MSR_RW);
>
> for ( i = 0; i < arch_pmc_cnt; i++ )
> {
> - vmx_set_msr_intercept(v, MSR_IA32_PERFCTR0 + i, MSR_RW);
> + hvm_set_msr_intercept(v, MSR_IA32_PERFCTR0 + i, MSR_RW);
>
> if ( full_width_write )
> - vmx_set_msr_intercept(v, MSR_IA32_A_PERFCTR0 + i, MSR_RW);
> + hvm_set_msr_intercept(v, MSR_IA32_A_PERFCTR0 + i, MSR_RW);
> }
>
> for ( i = 0; i < arch_pmc_cnt; i++ )
> - vmx_set_msr_intercept(v, MSR_P6_EVNTSEL(i), MSR_R);
> + hvm_set_msr_intercept(v, MSR_P6_EVNTSEL(i), MSR_R);
>
> - vmx_set_msr_intercept(v, MSR_CORE_PERF_FIXED_CTR_CTRL, MSR_R);
> - vmx_set_msr_intercept(v, MSR_IA32_DS_AREA, MSR_R);
> + hvm_set_msr_intercept(v, MSR_CORE_PERF_FIXED_CTR_CTRL, MSR_R);
> + hvm_set_msr_intercept(v, MSR_IA32_DS_AREA, MSR_R);
> }
>
> static inline void __core2_vpmu_save(struct vcpu *v)
... all calls here will go to VMX'es. For making either vpmu_<vendor>.c
build without that vendor's virtualization enabled, isn't it all it
takes to have ...
> @@ -916,6 +932,18 @@ static inline void hvm_set_reg(struct vcpu *v, unsigned
> int reg, uint64_t val)
> ASSERT_UNREACHABLE();
> }
>
> +static inline void hvm_set_msr_intercept(struct vcpu *v, uint32_t msr,
> + int flags)
> +{
> + ASSERT_UNREACHABLE();
> +}
> +
> +static inline void hvm_clear_msr_intercept(struct vcpu *v, uint32_t msr,
> + int flags)
> +{
> + ASSERT_UNREACHABLE();
> +}
... respective SVM and VMX stubs in place instead?
Jan
