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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.03.19 at 11:30, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 18/03/2019 10:11, Jan Beulich wrote:
>>>>> On 15.03.19 at 17:21, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> @@ -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.

I didn't say it's ideal, but as said - on one of my systems it makes
very obvious a flaw in the system's BIOS. I think that's good
enough as a reason.

> If you want to print this information out in a useful way, print 1ul <<
> paddr_bits so it is obvious in the logs.

Which will require everyone trying to consume this to count zeros
and f-s.

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

No-one is prevented from knowing this. In fact, for the purpose of
reducing pressure on both serial line bandwidth and log buffer size,
I think we'd be better off logging _all_ physical addresses and MFNs
with just as many leading zeros as are meaningful on the platform.
Granted in several cases even just going from %016lx to %013lx
would already be a reduction, but we clearly can do better.

Jan



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