[XEN PATCH v1 08/15] x86/vpmu: separate amd/intel vPMU code

[XEN PATCH v1 08/15] x86/vpmu: separate amd/intel vPMU code

[Sergiy Kibrik]

Build AMD vPMU when CONFIG_SVM is on, and Intel vPMU when CONFIG_VMX is on
respectively, allowing for a plaftorm-specific build. Also separate
arch_vpmu_ops initializers using these options and static inline stubs.

No functional change intended.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
---
 xen/arch/x86/cpu/Makefile       |  4 +++-
 xen/arch/x86/include/asm/vpmu.h | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
index 35561fe51d..d3d7b8fb2e 100644
--- a/xen/arch/x86/cpu/Makefile
+++ b/xen/arch/x86/cpu/Makefile
@@ -10,4 +10,6 @@ obj-y += intel.o
 obj-y += intel_cacheinfo.o
 obj-y += mwait-idle.o
 obj-y += shanghai.o
-obj-y += vpmu.o vpmu_amd.o vpmu_intel.o
+obj-y += vpmu.o
+obj-$(CONFIG_SVM) += vpmu_amd.o
+obj-$(CONFIG_VMX) += vpmu_intel.o
diff --git a/xen/arch/x86/include/asm/vpmu.h b/xen/arch/x86/include/asm/vpmu.h
index dae9b43dac..da86f2e420 100644
--- a/xen/arch/x86/include/asm/vpmu.h
+++ b/xen/arch/x86/include/asm/vpmu.h
@@ -11,6 +11,7 @@
 #define __ASM_X86_HVM_VPMU_H_
 
 #include <public/pmu.h>
+#include <xen/err.h>
 
 #define vcpu_vpmu(vcpu)   (&(vcpu)->arch.vpmu)
 #define vpmu_vcpu(vpmu)   container_of((vpmu), struct vcpu, arch.vpmu)
@@ -42,9 +43,27 @@ struct arch_vpmu_ops {
 #endif
 };
 
+#ifdef CONFIG_VMX
 const struct arch_vpmu_ops *core2_vpmu_init(void);
+#else
+static inline const struct arch_vpmu_ops* core2_vpmu_init(void)
+{
+    return ERR_PTR(-ENODEV);
+}
+#endif
+#ifdef CONFIG_SVM
 const struct arch_vpmu_ops *amd_vpmu_init(void);
 const struct arch_vpmu_ops *hygon_vpmu_init(void);
+#else
+static inline const struct arch_vpmu_ops* amd_vpmu_init(void)
+{
+    return ERR_PTR(-ENODEV);
+}
+static inline const struct arch_vpmu_ops* hygon_vpmu_init(void)
+{
+    return ERR_PTR(-ENODEV);
+}
+#endif
 
 struct vpmu_struct {
     u32 flags;
-- 
2.25.1

Re: [XEN PATCH v1 08/15] x86/vpmu: separate amd/intel vPMU code

[Andrew Cooper]

On 16/04/2024 7:35 am, Sergiy Kibrik wrote:
> diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
> index 35561fe51d..d3d7b8fb2e 100644
> --- a/xen/arch/x86/cpu/Makefile
> +++ b/xen/arch/x86/cpu/Makefile
> @@ -10,4 +10,6 @@ obj-y += intel.o
>  obj-y += intel_cacheinfo.o
>  obj-y += mwait-idle.o
>  obj-y += shanghai.o
> -obj-y += vpmu.o vpmu_amd.o vpmu_intel.o
> +obj-y += vpmu.o
> +obj-$(CONFIG_SVM) += vpmu_amd.o
> +obj-$(CONFIG_VMX) += vpmu_intel.o

I'm afraid this breaks perf counters on PV guests.  These files are
joint guest-type implementations.

Seeing as you leave vpmu.o alone, I guess that all you're actually
wanting to do is compile out vpmu_intel.o?  In which case, use
CONFIG_{AMD,INTEL} rather than CONFIG_{SVM,VMX} please.

~Andrew

Re: [XEN PATCH v1 08/15] x86/vpmu: separate amd/intel vPMU code

[Sergiy Kibrik]

16.04.24 14:05, Andrew Cooper:
> On 16/04/2024 7:35 am, Sergiy Kibrik wrote:
>> diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
>> index 35561fe51d..d3d7b8fb2e 100644
>> --- a/xen/arch/x86/cpu/Makefile
>> +++ b/xen/arch/x86/cpu/Makefile
>> @@ -10,4 +10,6 @@ obj-y += intel.o
>>   obj-y += intel_cacheinfo.o
>>   obj-y += mwait-idle.o
>>   obj-y += shanghai.o
>> -obj-y += vpmu.o vpmu_amd.o vpmu_intel.o
>> +obj-y += vpmu.o
>> +obj-$(CONFIG_SVM) += vpmu_amd.o
>> +obj-$(CONFIG_VMX) += vpmu_intel.o
> 
> I'm afraid this breaks perf counters on PV guests.  These files are
> joint guest-type implementations.
> 
> Seeing as you leave vpmu.o alone, I guess that all you're actually
> wanting to do is compile out vpmu_intel.o?  In which case, use
> CONFIG_{AMD,INTEL} rather than CONFIG_{SVM,VMX} please.
> 

Thanks for pointing that out.
I think I'll just exclude this patch from the series, and make a 
separate series with CONFIG_{AMD,INTEL} option and code separation that 
unrelated to VMX/SVM & HVM/PV, only to CPUs themselves.

BTW, how would you suggest CONFIG_{AMD,INTEL} shall relate to 
CONFIG_{SVM,VMX}? Should CONFIG_VMX just plainly depend on CONFIG_AMD, 
or more complex relations needed?

   -Sergiy

Re: [XEN PATCH v1 08/15] x86/vpmu: separate amd/intel vPMU code

[Jan Beulich]

On 18.04.2024 15:25, Sergiy Kibrik wrote:
> 16.04.24 14:05, Andrew Cooper:
>> On 16/04/2024 7:35 am, Sergiy Kibrik wrote:
>>> diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
>>> index 35561fe51d..d3d7b8fb2e 100644
>>> --- a/xen/arch/x86/cpu/Makefile
>>> +++ b/xen/arch/x86/cpu/Makefile
>>> @@ -10,4 +10,6 @@ obj-y += intel.o
>>>   obj-y += intel_cacheinfo.o
>>>   obj-y += mwait-idle.o
>>>   obj-y += shanghai.o
>>> -obj-y += vpmu.o vpmu_amd.o vpmu_intel.o
>>> +obj-y += vpmu.o
>>> +obj-$(CONFIG_SVM) += vpmu_amd.o
>>> +obj-$(CONFIG_VMX) += vpmu_intel.o
>>
>> I'm afraid this breaks perf counters on PV guests.  These files are
>> joint guest-type implementations.
>>
>> Seeing as you leave vpmu.o alone, I guess that all you're actually
>> wanting to do is compile out vpmu_intel.o?  In which case, use
>> CONFIG_{AMD,INTEL} rather than CONFIG_{SVM,VMX} please.
>>
> 
> Thanks for pointing that out.
> I think I'll just exclude this patch from the series, and make a 
> separate series with CONFIG_{AMD,INTEL} option and code separation that 
> unrelated to VMX/SVM & HVM/PV, only to CPUs themselves.
> 
> BTW, how would you suggest CONFIG_{AMD,INTEL} shall relate to 
> CONFIG_{SVM,VMX}? Should CONFIG_VMX just plainly depend on CONFIG_AMD, 
> or more complex relations needed?

The answer to this, from my perspective, is actually one of the reasons
why I would have wished we'd have separate CONFIG_INTEL and
CONFIG_INTEL_CPU. I'd then suggest to have VMX simply default to INTEL,
permitting at least experts to select it also when INTEL=n. Perhaps that
would still be the best approach even with this INTEL / INTEL_CPU
distinction.

Jan

Re: [XEN PATCH v1 08/15] x86/vpmu: separate amd/intel vPMU code

[Andrew Cooper]

On 18/04/2024 2:25 pm, Sergiy Kibrik wrote:
> 16.04.24 14:05, Andrew Cooper:
>> On 16/04/2024 7:35 am, Sergiy Kibrik wrote:
>>> diff --git a/xen/arch/x86/cpu/Makefile b/xen/arch/x86/cpu/Makefile
>>> index 35561fe51d..d3d7b8fb2e 100644
>>> --- a/xen/arch/x86/cpu/Makefile
>>> +++ b/xen/arch/x86/cpu/Makefile
>>> @@ -10,4 +10,6 @@ obj-y += intel.o
>>>   obj-y += intel_cacheinfo.o
>>>   obj-y += mwait-idle.o
>>>   obj-y += shanghai.o
>>> -obj-y += vpmu.o vpmu_amd.o vpmu_intel.o
>>> +obj-y += vpmu.o
>>> +obj-$(CONFIG_SVM) += vpmu_amd.o
>>> +obj-$(CONFIG_VMX) += vpmu_intel.o
>>
>> I'm afraid this breaks perf counters on PV guests.  These files are
>> joint guest-type implementations.
>>
>> Seeing as you leave vpmu.o alone, I guess that all you're actually
>> wanting to do is compile out vpmu_intel.o?  In which case, use
>> CONFIG_{AMD,INTEL} rather than CONFIG_{SVM,VMX} please.
>>
>
> Thanks for pointing that out.
> I think I'll just exclude this patch from the series, and make a
> separate series with CONFIG_{AMD,INTEL} option and code separation
> that unrelated to VMX/SVM & HVM/PV, only to CPUs themselves.
>
> BTW, how would you suggest CONFIG_{AMD,INTEL} shall relate to
> CONFIG_{SVM,VMX}? Should CONFIG_VMX just plainly depend on CONFIG_AMD,
> or more complex relations needed?

To a first approximation, no linkage.

Centaur have an implementation of VMX on the market, and Hygon have an
implementation of SVM.

~Andrew