On 24.07.2024 12:30, Andrew Cooper wrote:
> On 24/07/2024 8:34 am, Jan Beulich wrote:
>> On 23.07.2024 19:41, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/cpu/mcheck/vmce.c
>>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
>>> @@ -71,7 +71,7 @@ int vmce_restore_vcpu(struct vcpu *v, const struct
>>> hvm_vmce_vcpu *ctxt)
>>> if ( ctxt->caps & ~guest_mcg_cap & ~MCG_CAP_COUNT & ~MCG_CTL_P )
>>> {
>>> printk(XENLOG_G_ERR
>>> - "%s restore: unsupported MCA capabilities %#"PRIx64" for
>>> %pv (supported: %#Lx)\n",
>>> + "%s restore: unsupported MCA capabilities %#"PRIx64" for
>>> %pv (supported: %#llx)\n",
>>> is_hvm_vcpu(v) ? "HVM" : "PV", ctxt->caps,
>>> v, guest_mcg_cap & ~MCG_CAP_COUNT);
>> guest_mcg_cap is unsigned long and MCG_CAP_COUNT could as well use UL instead
>> of ULL, couldn't it?
>
> Well, like ...
>
>>
>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>>> @@ -517,7 +517,7 @@ static int vmx_init_vmcs_config(bool bsp)
>>> if ( (vmx_basic_msr_high & (VMX_BASIC_VMCS_SIZE_MASK >> 32)) >
>>> PAGE_SIZE )
>>> {
>>> - printk("VMX: CPU%d VMCS size is too big (%Lu bytes)\n",
>>> + printk("VMX: CPU%d VMCS size is too big (%llu bytes)\n",
>>> smp_processor_id(),
>>> vmx_basic_msr_high & (VMX_BASIC_VMCS_SIZE_MASK >> 32));
>>> return -EINVAL;
>>> @@ -564,7 +564,7 @@ static int vmx_init_vmcs_config(bool bsp)
>>> if ( (vmx_basic_msr_high & (VMX_BASIC_VMCS_SIZE_MASK >> 32)) !=
>>> ((vmx_basic_msr & VMX_BASIC_VMCS_SIZE_MASK) >> 32) )
>>> {
>>> - printk("VMX: CPU%d unexpected VMCS size %Lu\n",
>>> + printk("VMX: CPU%d unexpected VMCS size %llu\n",
>>> smp_processor_id(),
>>> vmx_basic_msr_high & (VMX_BASIC_VMCS_SIZE_MASK >> 32));
>>> mismatch = 1;
>> Same here for VMX_BASIC_VMCS_SIZE_MASK. We leverage not doing 32-bit builds
>> anymore in exactly this way elsewhere.
>
> ... this, it is about 32bit builds.
I don't think this is relevant for the VMCS constants?
> For better or worse, the msr-index cleanup says to use ULL, and this was
> so it could be shared into 32bit codebases. (In this case, I was
> thinking HVMLoader and misc bits of userspace.)
Hmm, yes. Let me mention though that right when starting that cleanup, you
introduced two outliers - MSR_CTC_{THREAD,CORE}_MASK. The latter was later
changed to have a U suffix, in part due to both Stefano and me not paying
enough attention when reviewing. So while I can see the goal, I'm wondering
whether we shouldn't take the abstraction there yet a step further and
arrange for UL suffixes in 64-bit builds, but for ULL in 32-bit ones:
#ifdef __i386__
# define _MC(x) _AC(x, ULL)
#else
# define _MC(x) _AC(x, UL)
#endif
Thoughts?
Jan
