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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/03/2019 10:11, Jan Beulich wrote:
>>>> 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.

How is that expected to work in the way you describe for the
overwhelming majority of systems with don't have MAXPHYSADDR aligned on
a nibble?

39, 42 and 46 are the widths used in practice by Intel processors, none
of which are divisible by 4.

If you want to print this information out in a useful way, print 1ul <<
paddr_bits so it is obvious in the logs.   Your logic of omitting
leading zeros is of no use to the Xen community when you are the only
person who knows what it means.

~Andrew

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