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

[Jan Beulich <jbeulich@xxxxxxxx>]

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