Re: [Xen-devel] [PATCH 2/2] x86/mtrr: fix build with gcc9

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.03.19 at 17:21, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 07/03/2019 10:32, Jan Beulich wrote:
>> generic.c: In function ‘print_mtrr_state’:
>> generic.c:210:11: error: ‘%0*lx’ directive output between 1 and 1073741823 
> bytes may cause result to exceed
>> ‘INT_MAX’ [-Werror=format-overflow=]
>>   210 |    printk("%s  %u base %0*"PRIx64"000 mask %0*"PRIx64"000 %s\n",
>>       |           ^~~~~~~~~~~~~~~~~
>> generic.c:210:44: note: format string is defined here
>>   210 |    printk("%s  %u base %0*"PRIx64"000 mask %0*"PRIx64"000 %s\n",
>> generic.c:210:11: note: directive argument in the range [0, 
> 4503599627370495]
>>   210 |    printk("%s  %u base %0*"PRIx64"000 mask %0*"PRIx64"000 %s\n",
>>       |           ^~~~~~~~~~~~~~~~~
>> generic.c:210:11: note: assuming directive output of 1 byte
>>
>> Restrict the width of the variable "width" controlling the number of
>> address digits output.
>>
>> Reported-by: Charles Arnold <carnold@xxxxxxxx>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> This is because GCC doesn't know the value of paddr_bits, and has
> concluded that
> 
> width = (paddr_bits - PAGE_SHIFT + 3) / 4;
> 
> can result in some insane values, which is true.
> 
> However, this logic to unnecessarily complicated for something which is
> only printed in a verbose or error case.
> 
> I'd prefer this as an alternative:
> 
> diff --git a/xen/arch/x86/cpu/mtrr/generic.c 
> b/xen/arch/x86/cpu/mtrr/generic.c
> index 8f9cf1b..566396f 100644
> --- a/xen/arch/x86/cpu/mtrr/generic.c
> +++ b/xen/arch/x86/cpu/mtrr/generic.c
> @@ -182,7 +182,6 @@ static void __init print_fixed(unsigned int base, 
> unsigned int step,
>  static void __init print_mtrr_state(const char *level)
>  {
>         unsigned int i;
> -       int width;
>  
>         printk("%sMTRR default type: %s\n", level,
>                mtrr_attrib_to_str(mtrr_state.def_type));
> @@ -203,14 +202,13 @@ static void __init print_mtrr_state(const char *level)
>         }
>         printk("%sMTRR variable ranges %sabled:\n", level,
>                mtrr_state.enabled ? "en" : "dis");
> -       width = (paddr_bits - PAGE_SHIFT + 3) / 4;
>  
>         for (i = 0; i < num_var_ranges; ++i) {
>                 if (mtrr_state.var_ranges[i].mask & MTRR_PHYSMASK_VALID)
> -                       printk("%s  %u base %0*"PRIx64"000 mask 
> %0*"PRIx64"000 %s\n",
> +                       printk("%s  %u base %013"PRIx64"000 mask 
> %013"PRIx64"000 %s\n",
>                                level, i,
> -                              width, mtrr_state.var_ranges[i].base >> 12,
> -                              width, mtrr_state.var_ranges[i].mask >> 12,
> +                              mtrr_state.var_ranges[i].base >> 12,
> +                              mtrr_state.var_ranges[i].mask >> 12,

I don't prefer this, and it was done the way it is for a very simple
reason: By omitting unnecessary leading zeros we convey an
extra bit of information - this way it is easier to spot if the mask
values in particular indeed go up all the way to the paddr limit. I
have at least one system where the BIOS screws up, and there
end up being leading zeros.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel