Re: [PATCH] xen/printk: Avoid the use of L as a length modifier

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/07/2024 8:34 am, Jan Beulich wrote:
> On 23.07.2024 19:41, Andrew Cooper wrote:
>> Coverity complains about it being invalid.  It turns out that it is a GCC-ism
>> to treat ll and L equivalently.  C99 only permits L to mean long double.
>>
>> Convert all uses of L in to alternatives, either l, ll or PRI.64 depending on
>> the operand type.  This in turn removes some unnecessary casts which look to
>> predate us having correct PRI* constants.
> I'm certainly okay with switching to PRI.64 where appropriate. Switching to 
> ll,
> however, means longer string literals for no really good reason. We use all
> sorts of GCC / GNU extensions; I don't see why we shouldn't be permitted to
> also use this one. It's Coverity that wants to cope, imo.

I'm about as unfussed with ll as I am over size_t.  The differences
presented here are not interesting.

> Or, if we really meant to no longer use L, support for it should then imo also
> be purged from vsnprintf().
>
>> I'm disappointed at having to use %ll for __fix_to_virt() in apic.c and
>> io_apic.c.  The expression ends up ULL because of the GB(64) in 
>> VMAP_VIRT_END,
>> but can't really be changed without breaking 32bit builds of Xen.
>>
>> One option might be to turn __fix_to_virt() into a proper function, but
>> there's a lot of that infrastructure which should be dedup'd and not left to
>> each arch to copy.
> Maybe it doesn't need us going that far, as ...
>
>> --- a/xen/arch/x86/apic.c
>> +++ b/xen/arch/x86/apic.c
>> @@ -938,7 +938,7 @@ void __init init_apic_mappings(void)
>>          apic_phys = mp_lapic_addr;
>>  
>>      set_fixmap_nocache(FIX_APIC_BASE, apic_phys);
>> -    apic_printk(APIC_VERBOSE, "mapped APIC to %08Lx (%08lx)\n", APIC_BASE,
>> +    apic_printk(APIC_VERBOSE, "mapped APIC to %08llx (%08lx)\n", APIC_BASE,
>>                  apic_phys);
> ... I wonder why we use __fix_to_virt() for APIC_BASE in the first place.
> Using fix_to_virt() would look to be more logical, as all users cast to
> a pointer anyway. Then it could simply be %p here.

That could work.  Lets see how it ends up looking.


>
>> --- a/xen/arch/x86/cpu/intel.c
>> +++ b/xen/arch/x86/cpu/intel.c
>> @@ -441,7 +441,7 @@ static void intel_log_freq(const struct cpuinfo_x86 *c)
>>              unsigned long long val = ecx;
>>  
>>              val *= ebx;
>> -            printk("CPU%u: TSC: %u Hz * %u / %u = %Lu Hz\n",
>> +            printk("CPU%u: TSC: %u Hz * %u / %u = %llu Hz\n",
>>                     smp_processor_id(), ecx, ebx, eax, val / eax);
>>          }
> Maybe change val to be uint64_t instead? That's against what ./CODING_STYLE
> calls for, but would be for a reason (to be able to use PRIu64) here.

Can do.

>
>> --- 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.

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.)

~Andrew