Re: [PATCH 4/4] x86/hvm: create hvm_funcs for {svm,vmx}_{set,clear}_msr_intercept()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.02.2023 21:14, Xenia Ragiadakou wrote:
> 
> On 2/28/23 16:58, Jan Beulich wrote:
>> 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?
> 
> IMO it is more readable and they looked very good candidates for being 
> abstracted because they are doing the same thing under both technologies.
> Are you suggesting that their usage in common code should be discouraged 
> and should not be exported via the hvm_funcs interface? Or just that the 
> amount of changes cannot be justified.

The amount of changes is okay if the route taken is deemed useful going
forward. For now I view vPMU as too special to provide sufficient
justification for yet another pair of hook functions. The more that - as
indicated before - every single call site will only ever call one of the
two possible callees.

> IIUC Andrew also suggested to use hvm_funcs for msr intercept handling 
> but I 'm not sure whether he had this or sth else in mind.

Andrew, any chance you could outline your thinking / further plans here?
In particular, do you have other future use sites in mind that would
live outside of vendor-specific code?

Jan