diff mbox series

[XEN,v3,2/6] x86/intel: move vmce_has_lmce() routine to header

Message ID 77bc29d74cdc43539a060bca26495a4115171f6e.1715673586.git.Sergiy_Kibrik@epam.com (mailing list archive)
State Superseded
Headers show
Series x86: make Intel/AMD vPMU & MCE support configurable | expand

Commit Message

Sergiy Kibrik May 14, 2024, 8:20 a.m. UTC 
Moving this function out of mce_intel.c would make it possible to disable
build of Intel MCE code later on, because the function gets called from
common x86 code.

Also replace boilerplate code that checks for MCG_LMCE_P flag with
vmce_has_lmce(), which might contribute to readability a bit.

Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@epam.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
CC: Jan Beulich <jbeulich@suse.com>
---
changes in v3:
 - do not check for CONFIG_INTEL
 - remove CONFIG_INTEL from patch description
changes in v2:
 - move vmce_has_lmce() to cpu/mcheck/mce.h
 - move IS_ENABLED(CONFIG_INTEL) check inside vmce_has_lmce()
 - changed description
---
 xen/arch/x86/cpu/mcheck/mce.h       | 5 +++++
 xen/arch/x86/cpu/mcheck/mce_intel.c | 4 ----
 xen/arch/x86/cpu/mcheck/vmce.c      | 5 ++---
 xen/arch/x86/include/asm/mce.h      | 1 -
 xen/arch/x86/msr.c                  | 2 ++
 5 files changed, 9 insertions(+), 8 deletions(-)

Comments

Jan Beulich May 16, 2024, 9:39 a.m. UTC | #1 
On 14.05.2024 10:20, Sergiy Kibrik wrote:
> Moving this function out of mce_intel.c would make it possible to disable
> build of Intel MCE code later on, because the function gets called from
> common x86 code.

Why "would"? "Will" or "is going to" would seem more to the point to me.
But anyway.

> --- a/xen/arch/x86/cpu/mcheck/mce.h
> +++ b/xen/arch/x86/cpu/mcheck/mce.h
> @@ -170,6 +170,11 @@ static inline int mce_bank_msr(const struct vcpu *v, uint32_t msr)
>      return 0;
>  }
>  
> +static inline bool vmce_has_lmce(const struct vcpu *v)
> +{
> +    return v->arch.vmce.mcg_cap & MCG_LMCE_P;
> +}

Is there a particular reason this is placed here, rather than ...

> --- a/xen/arch/x86/include/asm/mce.h
> +++ b/xen/arch/x86/include/asm/mce.h
> @@ -41,7 +41,6 @@ extern void vmce_init_vcpu(struct vcpu *v);
>  extern int vmce_restore_vcpu(struct vcpu *v, const struct hvm_vmce_vcpu *ctxt);
>  extern int vmce_wrmsr(uint32_t msr, uint64_t val);
>  extern int vmce_rdmsr(uint32_t msr, uint64_t *val);
> -extern bool vmce_has_lmce(const struct vcpu *v);
>  extern int vmce_enable_mca_cap(struct domain *d, uint64_t cap);

... in the file the declaration was in, thus avoiding ...

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -24,6 +24,8 @@
>  
>  #include <public/hvm/params.h>
>  
> +#include "cpu/mcheck/mce.h"

... the need for such a non-standard, cross-directory #include?

Jan
Sergiy Kibrik May 20, 2024, 9:32 a.m. UTC | #2 
16.05.24 12:39, Jan Beulich:
> On 14.05.2024 10:20, Sergiy Kibrik wrote:
>> Moving this function out of mce_intel.c would make it possible to disable
>> build of Intel MCE code later on, because the function gets called from
>> common x86 code.
> 
> Why "would"? "Will" or "is going to" would seem more to the point to me.

yes, sure

> But anyway.
> 
>> --- a/xen/arch/x86/cpu/mcheck/mce.h
>> +++ b/xen/arch/x86/cpu/mcheck/mce.h
>> @@ -170,6 +170,11 @@ static inline int mce_bank_msr(const struct vcpu *v, uint32_t msr)
>>       return 0;
>>   }
>>   
>> +static inline bool vmce_has_lmce(const struct vcpu *v)
>> +{
>> +    return v->arch.vmce.mcg_cap & MCG_LMCE_P;
>> +}
> 
> Is there a particular reason this is placed here, rather than ...
> 
>> --- a/xen/arch/x86/include/asm/mce.h
>> +++ b/xen/arch/x86/include/asm/mce.h
>> @@ -41,7 +41,6 @@ extern void vmce_init_vcpu(struct vcpu *v);
>>   extern int vmce_restore_vcpu(struct vcpu *v, const struct hvm_vmce_vcpu *ctxt);
>>   extern int vmce_wrmsr(uint32_t msr, uint64_t val);
>>   extern int vmce_rdmsr(uint32_t msr, uint64_t *val);
>> -extern bool vmce_has_lmce(const struct vcpu *v);
>>   extern int vmce_enable_mca_cap(struct domain *d, uint64_t cap);
> 
> ... in the file the declaration was in, thus avoiding ...
> 
>> --- a/xen/arch/x86/msr.c
>> +++ b/xen/arch/x86/msr.c
>> @@ -24,6 +24,8 @@
>>   
>>   #include <public/hvm/params.h>
>>   
>> +#include "cpu/mcheck/mce.h"
> 
> ... the need for such a non-standard, cross-directory #include?
> 


This is because MCG_LMCE_P is defined in arch/x86/cpu/mcheck/x86_mca.h 
-- so either MCG_LMCE_P (+ a bunch of MCG_* declarations) has to be 
moved to common header to be accessible, or local x86_mca.h got to be 
included from common arch/x86/include/asm/mce.h.

As for the MCG_* declarations movement I didn't think there's a good 
enough reason to do it; as for the inclusion of x86_mca.h it didn't look 
nice at all.

Are there any more options?

  -Sergiy
Jan Beulich May 21, 2024, 6:05 a.m. UTC | #3 
On 20.05.2024 11:32, Sergiy Kibrik wrote:
> 16.05.24 12:39, Jan Beulich:
>> On 14.05.2024 10:20, Sergiy Kibrik wrote:
>>> Moving this function out of mce_intel.c would make it possible to disable
>>> build of Intel MCE code later on, because the function gets called from
>>> common x86 code.
>>
>> Why "would"? "Will" or "is going to" would seem more to the point to me.
> 
> yes, sure
> 
>> But anyway.
>>
>>> --- a/xen/arch/x86/cpu/mcheck/mce.h
>>> +++ b/xen/arch/x86/cpu/mcheck/mce.h
>>> @@ -170,6 +170,11 @@ static inline int mce_bank_msr(const struct vcpu *v, uint32_t msr)
>>>       return 0;
>>>   }
>>>   
>>> +static inline bool vmce_has_lmce(const struct vcpu *v)
>>> +{
>>> +    return v->arch.vmce.mcg_cap & MCG_LMCE_P;
>>> +}
>>
>> Is there a particular reason this is placed here, rather than ...
>>
>>> --- a/xen/arch/x86/include/asm/mce.h
>>> +++ b/xen/arch/x86/include/asm/mce.h
>>> @@ -41,7 +41,6 @@ extern void vmce_init_vcpu(struct vcpu *v);
>>>   extern int vmce_restore_vcpu(struct vcpu *v, const struct hvm_vmce_vcpu *ctxt);
>>>   extern int vmce_wrmsr(uint32_t msr, uint64_t val);
>>>   extern int vmce_rdmsr(uint32_t msr, uint64_t *val);
>>> -extern bool vmce_has_lmce(const struct vcpu *v);
>>>   extern int vmce_enable_mca_cap(struct domain *d, uint64_t cap);
>>
>> ... in the file the declaration was in, thus avoiding ...
>>
>>> --- a/xen/arch/x86/msr.c
>>> +++ b/xen/arch/x86/msr.c
>>> @@ -24,6 +24,8 @@
>>>   
>>>   #include <public/hvm/params.h>
>>>   
>>> +#include "cpu/mcheck/mce.h"
>>
>> ... the need for such a non-standard, cross-directory #include?
>>
> 
> 
> This is because MCG_LMCE_P is defined in arch/x86/cpu/mcheck/x86_mca.h 
> -- so either MCG_LMCE_P (+ a bunch of MCG_* declarations) has to be 
> moved to common header to be accessible, or local x86_mca.h got to be 
> included from common arch/x86/include/asm/mce.h.
> 
> As for the MCG_* declarations movement I didn't think there's a good 
> enough reason to do it; as for the inclusion of x86_mca.h it didn't look 
> nice at all.

I'm afraid I don't follow the latter: Why's including x86_mca.h any worse
than what you do right now?

Jan
Sergiy Kibrik May 21, 2024, 10 a.m. UTC | #4 
21.05.24 09:05, Jan Beulich:
>> This is because MCG_LMCE_P is defined in arch/x86/cpu/mcheck/x86_mca.h
>> -- so either MCG_LMCE_P (+ a bunch of MCG_* declarations) has to be
>> moved to common header to be accessible, or local x86_mca.h got to be
>> included from common arch/x86/include/asm/mce.h.
>>
>> As for the MCG_* declarations movement I didn't think there's a good
>> enough reason to do it; as for the inclusion of x86_mca.h it didn't look
>> nice at all.
> I'm afraid I don't follow the latter: Why's including x86_mca.h any worse
> than what you do right now?

To include x86_mca.h from asm/mce.h something like this line would be 
needed:

#include "../../cpu/mcheck/x86_mca.h"

I've found only two include-s of such kind, so I presume they're not common.
Besides xen/sched.h includes asm/mce.h before declaration of struct 
vcpu, so body of vmce_has_lmce(const struct vcpu *v) can't really be 
compiled in asm/mce.h

   -Sergiy
Jan Beulich May 21, 2024, 10:19 a.m. UTC | #5 
On 21.05.2024 12:00, Sergiy Kibrik wrote:
> 21.05.24 09:05, Jan Beulich:
>>> This is because MCG_LMCE_P is defined in arch/x86/cpu/mcheck/x86_mca.h
>>> -- so either MCG_LMCE_P (+ a bunch of MCG_* declarations) has to be
>>> moved to common header to be accessible, or local x86_mca.h got to be
>>> included from common arch/x86/include/asm/mce.h.
>>>
>>> As for the MCG_* declarations movement I didn't think there's a good
>>> enough reason to do it; as for the inclusion of x86_mca.h it didn't look
>>> nice at all.
>> I'm afraid I don't follow the latter: Why's including x86_mca.h any worse
>> than what you do right now?
> 
> To include x86_mca.h from asm/mce.h something like this line would be 
> needed:
> 
> #include "../../cpu/mcheck/x86_mca.h"
> 
> I've found only two include-s of such kind, so I presume they're not common.

Indeed, and I have to apologize for not reading your earlier reply quite
right.

Jan

> Besides xen/sched.h includes asm/mce.h before declaration of struct 
> vcpu, so body of vmce_has_lmce(const struct vcpu *v) can't really be 
> compiled in asm/mce.h
> 
>    -Sergiy
diff mbox series

Patch

diff --git a/xen/arch/x86/cpu/mcheck/mce.h b/xen/arch/x86/cpu/mcheck/mce.h
index 4806405f96..eba4b536c7 100644
--- a/xen/arch/x86/cpu/mcheck/mce.h
+++ b/xen/arch/x86/cpu/mcheck/mce.h
@@ -170,6 +170,11 @@  static inline int mce_bank_msr(const struct vcpu *v, uint32_t msr)
     return 0;
 }
 
+static inline bool vmce_has_lmce(const struct vcpu *v)
+{
+    return v->arch.vmce.mcg_cap & MCG_LMCE_P;
+}
+
 struct mce_callbacks {
     void (*handler)(const struct cpu_user_regs *regs);
     bool (*check_addr)(uint64_t status, uint64_t misc, int addr_type);
diff --git a/xen/arch/x86/cpu/mcheck/mce_intel.c b/xen/arch/x86/cpu/mcheck/mce_intel.c
index 3f5199b531..af43281cc6 100644
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -1050,7 +1050,3 @@  int vmce_intel_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
     return 1;
 }
 
-bool vmce_has_lmce(const struct vcpu *v)
-{
-    return v->arch.vmce.mcg_cap & MCG_LMCE_P;
-}
diff --git a/xen/arch/x86/cpu/mcheck/vmce.c b/xen/arch/x86/cpu/mcheck/vmce.c
index 353d4f19b2..94d1f021e1 100644
--- a/xen/arch/x86/cpu/mcheck/vmce.c
+++ b/xen/arch/x86/cpu/mcheck/vmce.c
@@ -199,7 +199,7 @@  int vmce_rdmsr(uint32_t msr, uint64_t *val)
          * bits are always set in guest MSR_IA32_FEATURE_CONTROL by Xen, so it
          * does not need to check them here.
          */
-        if ( cur->arch.vmce.mcg_cap & MCG_LMCE_P )
+        if ( vmce_has_lmce(cur) )
         {
             *val = cur->arch.vmce.mcg_ext_ctl;
             mce_printk(MCE_VERBOSE, "MCE: %pv: rd MCG_EXT_CTL %#"PRIx64"\n",
@@ -324,8 +324,7 @@  int vmce_wrmsr(uint32_t msr, uint64_t val)
         break;
 
     case MSR_IA32_MCG_EXT_CTL:
-        if ( (cur->arch.vmce.mcg_cap & MCG_LMCE_P) &&
-             !(val & ~MCG_EXT_CTL_LMCE_EN) )
+        if ( vmce_has_lmce(cur) && !(val & ~MCG_EXT_CTL_LMCE_EN) )
             cur->arch.vmce.mcg_ext_ctl = val;
         else
             ret = -1;
diff --git a/xen/arch/x86/include/asm/mce.h b/xen/arch/x86/include/asm/mce.h
index 6ce56b5b85..2ec47a71ae 100644
--- a/xen/arch/x86/include/asm/mce.h
+++ b/xen/arch/x86/include/asm/mce.h
@@ -41,7 +41,6 @@  extern void vmce_init_vcpu(struct vcpu *v);
 extern int vmce_restore_vcpu(struct vcpu *v, const struct hvm_vmce_vcpu *ctxt);
 extern int vmce_wrmsr(uint32_t msr, uint64_t val);
 extern int vmce_rdmsr(uint32_t msr, uint64_t *val);
-extern bool vmce_has_lmce(const struct vcpu *v);
 extern int vmce_enable_mca_cap(struct domain *d, uint64_t cap);
 
 DECLARE_PER_CPU(unsigned int, nr_mce_banks);
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 9babd441f9..b0ec96f021 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -24,6 +24,8 @@ 
 
 #include <public/hvm/params.h>
 
+#include "cpu/mcheck/mce.h"
+
 DEFINE_PER_CPU(uint32_t, tsc_aux);
 
 int init_vcpu_msr_policy(struct vcpu *v)