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

[Andrew Cooper <amc96@xxxxxxxx>]

On 01/12/2021 07:32, Jan Beulich wrote:
> 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.

Oh ok.

Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

>
>>> --- 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.

I don't mind specifically if it gets left as-is, but having a pile of
constants like this tabulated correct makes a massive improvement to
code clarity.


That said, this whole flags infrastructure is almost exclusively
obfuscation, and I've got a good mind to replace it all with a
bitfield.  I'll save taking some shears to this code for another time.

~Andrew