Re: [XEN PATCH v1 2/7] x86/intel: guard vmce_has_lmce() with INTEL option

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.04.2024 10:50, Sergiy Kibrik wrote:
> Since MCG_LMCE_P bit is specific to Intel CPUs

That's the case now. It could change going forward, and an underlying hypervisor
might also have (obscure?) reasons to surface it elsewhere.

> the code to check it can
> possibly be excluded from build if !CONFIG_INTEL. With these guards
> calls to vmce_has_lmce() are eliminated and mce_intel.c can end up
> not being built.
> 
> Also replace boilerplate code that checks for MCG_LMCE_P flag with
> vmce_has_lmce(), which might contribute to readability a bit.

Alternatively, have you considered making that function an inline one in a
suitable header? Besides addressing your build issue (I think), ...

> --- 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 ( IS_ENABLED(CONFIG_INTEL) && vmce_has_lmce(cur) )

... doing so would alternatively also permit integrating the IS_ENABLED()
into the function, rather than repeating the same ...

> @@ -324,7 +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) &&
> +        if ( IS_ENABLED(CONFIG_INTEL) && vmce_has_lmce(cur) &&
>               !(val & ~MCG_EXT_CTL_LMCE_EN) )
>              cur->arch.vmce.mcg_ext_ctl = val;
>          else
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -86,7 +86,7 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>              goto gp_fault;
>  
>          *val = IA32_FEATURE_CONTROL_LOCK;
> -        if ( vmce_has_lmce(v) )
> +        if ( IS_ENABLED(CONFIG_INTEL) && vmce_has_lmce(v) )
>              *val |= IA32_FEATURE_CONTROL_LMCE_ON;
>          if ( cp->basic.vmx )
>              *val |= IA32_FEATURE_CONTROL_ENABLE_VMXON_OUTSIDE_SMX;

... three times.

Jan