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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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.

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

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

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

> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -2597,7 +2597,7 @@ static void __init ioapic_init_mappings(void)
>          }
>  
>          set_fixmap_nocache(idx, ioapic_phys);
> -        apic_printk(APIC_VERBOSE, "mapped IOAPIC to %08Lx (%08lx)\n",
> +        apic_printk(APIC_VERBOSE, "mapped IOAPIC to %08llx (%08lx)\n",
>                      __fix_to_virt(idx), ioapic_phys);

Like above, switch to using fix_to_virt() and %p?

Jan