Re: [XEN PATCH v3 13/16] x86/vpmu: guard calls to vmx/svm functions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.06.2024 13:32, Sergiy Kibrik wrote:
> If VMX/SVM disabled in the build, we may still want to have vPMU drivers for
> PV guests. Yet in such case before using VMX/SVM features and functions we 
> have
> to explicitly check if they're available in the build. For this puspose

Nit: It's not the first time that I see this apparent typo - I assume here
and elsewhere "purpose" is meant?

> --- a/xen/arch/x86/cpu/vpmu_amd.c
> +++ b/xen/arch/x86/cpu/vpmu_amd.c
> @@ -27,6 +27,7 @@
>  #define is_pmu_enabled(msr) ((msr) & (1ULL << MSR_F10H_EVNTSEL_EN_SHIFT))
>  #define set_guest_mode(msr) ((msr) |= (1ULL << MSR_F10H_EVNTSEL_GO_SHIFT))
>  #define is_overflowed(msr) (!((msr) & (1ULL << (MSR_F10H_COUNTER_LENGTH - 
> 1))))
> +#define is_svm_vcpu(v) (using_svm && is_hvm_vcpu(v))

Like for the earlier patch the implicit cpu_has_svm check you add here is
redundant with us knowing we're dealing with AMD hardware here. Please
consider switching to IS_ENABLED().

> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
> @@ -54,6 +54,8 @@
>  #define MSR_PMC_ALIAS_MASK       (~(MSR_IA32_PERFCTR0 ^ MSR_IA32_A_PERFCTR0))
>  static bool __read_mostly full_width_write;
>  
> +#define is_vmx_vcpu(v) ( using_vmx && is_hvm_vcpu(v) )

Nit (style): Unlike above, but like iirc you had it elsewhere, you have
stray blanks here immediately inside the parentheses. Beyond that the
comment above applies here, too (with s/AMD/Intel).

Jan