Re: [PATCH 1/3] x86/vPMU: convert vendor hook invocations to altcall

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.11.2021 21:56, Andrew Cooper wrote:
> On 29/11/2021 09:10, Jan Beulich wrote:
>> @@ -133,14 +133,13 @@ int vpmu_do_msr(unsigned int msr, uint64
>>           goto nop;
>>  
>>      vpmu = vcpu_vpmu(curr);
>> -    ops = vpmu->arch_vpmu_ops;
>> -    if ( !ops )
>> +    if ( !vpmu_is_set(vpmu, VPMU_INITIALIZED) )
>>          goto nop;
>>  
>> -    if ( is_write && ops->do_wrmsr )
>> -        ret = ops->do_wrmsr(msr, *msr_content, supported);
>> -    else if ( !is_write && ops->do_rdmsr )
>> -        ret = ops->do_rdmsr(msr, msr_content);
>> +    if ( is_write && vpmu_ops.do_wrmsr )
>> +        ret = alternative_call(vpmu_ops.do_wrmsr, msr, *msr_content, 
>> supported);
>> +    else if ( !is_write && vpmu_ops.do_rdmsr )
>> +        ret = alternative_call(vpmu_ops.do_rdmsr, msr, msr_content);
> 
> Elsewhere, you've dropped the function pointer NULL checks.  Why not here?

No, I'm not dropping any function pointer checks here; all I drop is
checks of the ops pointer being NULL. These checks all get dropped in
patch 3.

>> --- a/xen/include/asm-x86/vpmu.h
>> +++ b/xen/include/asm-x86/vpmu.h
>> @@ -61,25 +61,25 @@ struct vpmu_struct {
>>      u32 hw_lapic_lvtpc;
>>      void *context;      /* May be shared with PV guest */
>>      void *priv_context; /* hypervisor-only */
>> -    const struct arch_vpmu_ops *arch_vpmu_ops;
>>      struct xen_pmu_data *xenpmu_data;
>>      spinlock_t vpmu_lock;
>>  };
>>  
>>  /* VPMU states */
>> -#define VPMU_CONTEXT_ALLOCATED              0x1
>> -#define VPMU_CONTEXT_LOADED                 0x2
>> -#define VPMU_RUNNING                        0x4
>> -#define VPMU_CONTEXT_SAVE                   0x8   /* Force context save */
>> -#define VPMU_FROZEN                         0x10  /* Stop counters while 
>> VCPU is not running */
>> -#define VPMU_PASSIVE_DOMAIN_ALLOCATED       0x20
>> +#define VPMU_INITIALIZED                    0x1
>> +#define VPMU_CONTEXT_ALLOCATED              0x2
>> +#define VPMU_CONTEXT_LOADED                 0x4
>> +#define VPMU_RUNNING                        0x8
>> +#define VPMU_CONTEXT_SAVE                   0x10  /* Force context save */
>> +#define VPMU_FROZEN                         0x20  /* Stop counters while 
>> VCPU is not running */
>> +#define VPMU_PASSIVE_DOMAIN_ALLOCATED       0x40
>>  /* PV(H) guests: VPMU registers are accessed by guest from shared page */
>> -#define VPMU_CACHED                         0x40
>> -#define VPMU_AVAILABLE                      0x80
>> +#define VPMU_CACHED                         0x80
>> +#define VPMU_AVAILABLE                      0x100
>>  
>>  /* Intel-specific VPMU features */
>> -#define VPMU_CPU_HAS_DS                     0x100 /* Has Debug Store */
>> -#define VPMU_CPU_HAS_BTS                    0x200 /* Has Branch Trace Store 
>> */
>> +#define VPMU_CPU_HAS_DS                     0x1000 /* Has Debug Store */
>> +#define VPMU_CPU_HAS_BTS                    0x2000 /* Has Branch Trace 
>> Store */
> 
> Seeing as you're shuffling each of these, how about adding some leading
> 0's for alignment?

Fine with me; I did consider it at the time of writing the patch,
but decided that such a change of non-mandatory style may not be
justified here (or even in general), as there are also downsides
to such padding: Once adding a constant with more significant
digits, all pre-existing ones need touching to insert yet another
zero.

Jan