[PATCH 0/2] x86: fix build with gcc9

[PATCH 0/2] x86: fix build with gcc9

[Jan Beulich]

Several build issues (new warnings, causing the build to fail due to
-Werror) have been found. The two changes here address the ones
I can actually repro; there are a few more related to
-Waddress-of-packed-member which I haven't been able to see
myself, and hence for now I'm unable to sort out a proper fix /
workaround for them.

Despite this coming late, I think this is a worthwhile change for 4.12
with pretty low risk.

1: e820: fix build with gcc9
2: mtrr: fix build with gcc9

Jan



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

[PATCH 1/2] x86/e820: fix build with gcc9

[Jan Beulich]

e820.c: In function ‘clip_to_limit’:
.../xen/include/asm/string.h:10:26: error: ‘__builtin_memmove’ offset [-16, -36] is out of the bounds [0, 20484] of
object ‘e820’ with type ‘struct e820map’ [-Werror=array-bounds]
   10 | #define memmove(d, s, n) __builtin_memmove(d, s, n)
      |                          ^~~~~~~~~~~~~~~~~~~~~~~~~~
e820.c:404:13: note: in expansion of macro ‘memmove’
  404 |             memmove(&e820.map[i], &e820.map[i+1],
      |             ^~~~~~~
e820.c:36:16: note: ‘e820’ declared here
   36 | struct e820map e820;
      |                ^~~~

While I can't see where the negative offsets would come from, converting
the loop index to unsigned type helps. Take the opportunity and also
convert several other local variables and copy_e820_map()'s second
parameter to unsigned int (and bool in one case).

Reported-by: Charles Arnold <carnold@suse.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/e820.c
+++ b/xen/arch/x86/e820.c
@@ -44,7 +44,7 @@ struct e820map __initdata e820_raw;
  */
 int __init e820_all_mapped(u64 start, u64 end, unsigned type)
 {
-	int i;
+	unsigned int i;
 
 	for (i = 0; i < e820.nr_map; i++) {
 		struct e820entry *ei = &e820.map[i];
@@ -73,9 +73,7 @@ int __init e820_all_mapped(u64 start, u6
 static void __init add_memory_region(unsigned long long start,
                                      unsigned long long size, int type)
 {
-    int x;
-
-    x = e820.nr_map;
+    unsigned int x = e820.nr_map;
 
     if (x == ARRAY_SIZE(e820.map)) {
         printk(KERN_ERR "Ooops! Too many entries in the memory map!\n");
@@ -140,11 +138,9 @@ int __init sanitize_e820_map(struct e820
     struct change_member *change_tmp;
     unsigned long current_type, last_type;
     unsigned long long last_addr;
-    int chgidx, still_changing;
-    int overlap_entries;
-    int new_bios_entry;
-    int old_nr, new_nr, chg_nr;
-    int i;
+    bool still_changing;
+    unsigned int i, chgidx, overlap_entries, new_bios_entry;
+    unsigned int old_nr, new_nr, chg_nr;
 
     /*
       Visually we're performing the following (1,2,3,4 = memory types)...
@@ -211,9 +207,9 @@ int __init sanitize_e820_map(struct e820
     chg_nr = chgidx;    	/* true number of change-points */
 
     /* sort change-point list by memory addresses (low -> high) */
-    still_changing = 1;
+    still_changing = true;
     while (still_changing)	{
-        still_changing = 0;
+        still_changing = false;
         for (i=1; i < chg_nr; i++)  {
             /* if <current_addr> > <last_addr>, swap */
             /* or, if current=<start_addr> & last=<end_addr>, swap */
@@ -226,7 +222,7 @@ int __init sanitize_e820_map(struct e820
                 change_tmp = change_point[i];
                 change_point[i] = change_point[i-1];
                 change_point[i-1] = change_tmp;
-                still_changing=1;
+                still_changing = true;
             }
         }
     }
@@ -304,7 +300,7 @@ int __init sanitize_e820_map(struct e820
  * thinkpad 560x, for example, does not cooperate with the memory
  * detection code.)
  */
-static int __init copy_e820_map(struct e820entry * biosmap, int nr_map)
+static int __init copy_e820_map(struct e820entry * biosmap, unsigned int nr_map)
 {
     /* Only one memory region (or negative)? Ignore it */
     if (nr_map < 2)
@@ -345,7 +341,7 @@ static int __init copy_e820_map(struct e
  */
 static unsigned long __init find_max_pfn(void)
 {
-    int i;
+    unsigned int i;
     unsigned long max_pfn = 0;
 
     for (i = 0; i < e820.nr_map; i++) {
@@ -366,7 +362,7 @@ static unsigned long __init find_max_pfn
 
 static void __init clip_to_limit(uint64_t limit, char *warnmsg)
 {
-    int i;
+    unsigned int i;
     char _warnmsg[160];
     uint64_t old_limit = 0;
 
@@ -514,7 +510,7 @@ static void __init machine_specific_memo
 {
     unsigned long mpt_limit, ro_mpt_limit;
     uint64_t top_of_ram, size;
-    int i;
+    unsigned int i;
 
     sanitize_e820_map(raw->map, &raw->nr_map);
     copy_e820_map(raw->map, raw->nr_map);
@@ -604,7 +600,7 @@ int __init e820_change_range_type(
     uint32_t orig_type, uint32_t new_type)
 {
     uint64_t rs = 0, re = 0;
-    int i;
+    unsigned int i;
 
     for ( i = 0; i < e820->nr_map; i++ )
     {




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

Re: [PATCH 1/2] x86/e820: fix build with gcc9

[Roger Pau Monné]

On Thu, Mar 07, 2019 at 03:31:43AM -0700, Jan Beulich wrote:
> e820.c: In function ‘clip_to_limit’:
> .../xen/include/asm/string.h:10:26: error: ‘__builtin_memmove’ offset [-16, -36] is out of the bounds [0, 20484] of
> object ‘e820’ with type ‘struct e820map’ [-Werror=array-bounds]
>    10 | #define memmove(d, s, n) __builtin_memmove(d, s, n)
>       |                          ^~~~~~~~~~~~~~~~~~~~~~~~~~
> e820.c:404:13: note: in expansion of macro ‘memmove’
>   404 |             memmove(&e820.map[i], &e820.map[i+1],
>       |             ^~~~~~~
> e820.c:36:16: note: ‘e820’ declared here
>    36 | struct e820map e820;
>       |                ^~~~
> 
> While I can't see where the negative offsets would come from, converting
> the loop index to unsigned type helps. Take the opportunity and also
> convert several other local variables and copy_e820_map()'s second
> parameter to unsigned int (and bool in one case).

I also cannot see how you can end up with negative offsets, but
changing to unsigned int is definitely and improvement IMO.

> Reported-by: Charles Arnold <carnold@suse.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

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

Re: [PATCH 1/2] x86/e820: fix build with gcc9

[Wei Liu]

On Thu, Mar 07, 2019 at 11:46:08AM +0100, Roger Pau Monné wrote:
> On Thu, Mar 07, 2019 at 03:31:43AM -0700, Jan Beulich wrote:
> > e820.c: In function ‘clip_to_limit’:
> > .../xen/include/asm/string.h:10:26: error: ‘__builtin_memmove’ offset [-16, -36] is out of the bounds [0, 20484] of
> > object ‘e820’ with type ‘struct e820map’ [-Werror=array-bounds]
> >    10 | #define memmove(d, s, n) __builtin_memmove(d, s, n)
> >       |                          ^~~~~~~~~~~~~~~~~~~~~~~~~~
> > e820.c:404:13: note: in expansion of macro ‘memmove’
> >   404 |             memmove(&e820.map[i], &e820.map[i+1],
> >       |             ^~~~~~~
> > e820.c:36:16: note: ‘e820’ declared here
> >    36 | struct e820map e820;
> >       |                ^~~~
> > 
> > While I can't see where the negative offsets would come from, converting
> > the loop index to unsigned type helps. Take the opportunity and also
> > convert several other local variables and copy_e820_map()'s second
> > parameter to unsigned int (and bool in one case).
> 
> I also cannot see how you can end up with negative offsets, but
> changing to unsigned int is definitely and improvement IMO.

Gcc is becoming more and more aggressive in inferring what you mean with
your code. There are definitely bugs in that area.

This one comes to mind:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86827

Although I'm not sure if -Warray-bounds is the culprit in this case.

> 
> > Reported-by: Charles Arnold <carnold@suse.com>
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.

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

Re: [PATCH 1/2] x86/e820: fix build with gcc9

[Andrew Cooper]

On 07/03/2019 10:31, Jan Beulich wrote:
> e820.c: In function ‘clip_to_limit’:
> .../xen/include/asm/string.h:10:26: error: ‘__builtin_memmove’ offset [-16, -36] is out of the bounds [0, 20484] of
> object ‘e820’ with type ‘struct e820map’ [-Werror=array-bounds]
>    10 | #define memmove(d, s, n) __builtin_memmove(d, s, n)
>       |                          ^~~~~~~~~~~~~~~~~~~~~~~~~~
> e820.c:404:13: note: in expansion of macro ‘memmove’
>   404 |             memmove(&e820.map[i], &e820.map[i+1],
>       |             ^~~~~~~
> e820.c:36:16: note: ‘e820’ declared here
>    36 | struct e820map e820;
>       |                ^~~~
>
> While I can't see where the negative offsets would come from, converting
> the loop index to unsigned type helps. Take the opportunity and also
> convert several other local variables and copy_e820_map()'s second
> parameter to unsigned int (and bool in one case).
>
> Reported-by: Charles Arnold <carnold@suse.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, with one request.

>
> --- a/xen/arch/x86/e820.c
> +++ b/xen/arch/x86/e820.c
> @@ -304,7 +300,7 @@ int __init sanitize_e820_map(struct e820
>   * thinkpad 560x, for example, does not cooperate with the memory
>   * detection code.)
>   */
> -static int __init copy_e820_map(struct e820entry * biosmap, int nr_map)
> +static int __init copy_e820_map(struct e820entry * biosmap, unsigned int nr_map)
>  {
>      /* Only one memory region (or negative)? Ignore it */

This comment is now stale.  I'd just drop the bit in brackets.

~Andrew

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

Re: [PATCH 1/2] x86/e820: fix build with gcc9

[Jan Beulich]

>>> On 15.03.19 at 17:07, <andrew.cooper3@citrix.com> wrote:
> On 07/03/2019 10:31, Jan Beulich wrote:
>> e820.c: In function ‘clip_to_limit’:
>> .../xen/include/asm/string.h:10:26: error: ‘__builtin_memmove’ offset [-16, 
> -36] is out of the bounds [0, 20484] of
>> object ‘e820’ with type ‘struct e820map’ [-Werror=array-bounds]
>>    10 | #define memmove(d, s, n) __builtin_memmove(d, s, n)
>>       |                          ^~~~~~~~~~~~~~~~~~~~~~~~~~
>> e820.c:404:13: note: in expansion of macro ‘memmove’
>>   404 |             memmove(&e820.map[i], &e820.map[i+1],
>>       |             ^~~~~~~
>> e820.c:36:16: note: ‘e820’ declared here
>>    36 | struct e820map e820;
>>       |                ^~~~
>>
>> While I can't see where the negative offsets would come from, converting
>> the loop index to unsigned type helps. Take the opportunity and also
>> convert several other local variables and copy_e820_map()'s second
>> parameter to unsigned int (and bool in one case).
>>
>> Reported-by: Charles Arnold <carnold@suse.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>, with one request.

Thanks.

>> --- a/xen/arch/x86/e820.c
>> +++ b/xen/arch/x86/e820.c
>> @@ -304,7 +300,7 @@ int __init sanitize_e820_map(struct e820
>>   * thinkpad 560x, for example, does not cooperate with the memory
>>   * detection code.)
>>   */
>> -static int __init copy_e820_map(struct e820entry * biosmap, int nr_map)
>> +static int __init copy_e820_map(struct e820entry * biosmap, unsigned int nr_map)
>>  {
>>      /* Only one memory region (or negative)? Ignore it */
> 
> This comment is now stale.  I'd just drop the bit in brackets.

Sure, will do.

Jan


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

[PATCH 2/2] x86/mtrr: fix build with gcc9

[Jan Beulich]

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@suse.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/cpu/mtrr/generic.c
+++ b/xen/arch/x86/cpu/mtrr/generic.c
@@ -182,7 +182,7 @@ static void __init print_fixed(unsigned
 static void __init print_mtrr_state(const char *level)
 {
 	unsigned int i;
-	int width;
+	unsigned char width; /* gcc9 doesn't like plain "int" here */
 
 	printk("%sMTRR default type: %s\n", level,
 	       mtrr_attrib_to_str(mtrr_state.def_type));


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

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

[Roger Pau Monné]

On Thu, Mar 07, 2019 at 03:32:12AM -0700, 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.

I have to admit I'm not sure why gcc complains here, and why switching
to unsigned char fixes it. unsigned char max value would be 255, which
when used as a width to print an unsigned long it's also too high?

Does checking that width <= 16 also placate gcc?

Or maybe I'm just not understanding what gcc complains about.

Thanks.

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

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

[Jan Beulich]

>>> On 07.03.19 at 11:55, <roger.pau@citrix.com> wrote:
> On Thu, Mar 07, 2019 at 03:32:12AM -0700, 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.
> 
> I have to admit I'm not sure why gcc complains here, and why switching
> to unsigned char fixes it. unsigned char max value would be 255, which
> when used as a width to print an unsigned long it's also too high?

With

	width = (paddr_bits - PAGE_SHIFT + 3) / 4;

gcc simply can't know that the valid value range is rather small. Hence
it assumes millions of leading zeros _could_ be printed. Their doc
explicitly suggests using a more narrow type, which is why ...

> Does checking that width <= 16 also placate gcc?

... I didn't even try this option (despite having considered it).

Jan


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

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

[Roger Pau Monné]

On Thu, Mar 07, 2019 at 04:22:13AM -0700, Jan Beulich wrote:
> >>> On 07.03.19 at 11:55, <roger.pau@citrix.com> wrote:
> > On Thu, Mar 07, 2019 at 03:32:12AM -0700, 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.
> > 
> > I have to admit I'm not sure why gcc complains here, and why switching
> > to unsigned char fixes it. unsigned char max value would be 255, which
> > when used as a width to print an unsigned long it's also too high?
> 
> With
> 
> 	width = (paddr_bits - PAGE_SHIFT + 3) / 4;
> 
> gcc simply can't know that the valid value range is rather small. Hence
> it assumes millions of leading zeros _could_ be printed. Their doc
> explicitly suggests using a more narrow type, which is why ...
> 
> > Does checking that width <= 16 also placate gcc?
> 
> ... I didn't even try this option (despite having considered it).

Ack, if that's the recommended way. I would prefer using unsigned int
and adding a BUG_ON if > 16. Anyway:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

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

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

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 2808 bytes --]

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@suse.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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,
                               mtrr_attrib_to_str(mtrr_state.var_ranges[i].base &
                                                  MTRR_PHYSBASE_TYPE_MASK));
                else


[-- Attachment #1.2: Type: text/html, Size: 3439 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

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

[Jan Beulich]

>>> On 15.03.19 at 17:21, <andrew.cooper3@citrix.com> 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@suse.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Andrew Cooper]

On 18/03/2019 10:11, Jan Beulich wrote:
>>>> On 15.03.19 at 17:21, <andrew.cooper3@citrix.com> 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@suse.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Jan Beulich]

>>> On 18.03.19 at 11:30, <andrew.cooper3@citrix.com> wrote:
> On 18/03/2019 10:11, Jan Beulich wrote:
>>>>> On 15.03.19 at 17:21, <andrew.cooper3@citrix.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Jan Beulich]

>>> On 07.03.19 at 11:32,  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@suse.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

This one's still pending for us to build cleanly with gcc 9. I know
you'd like it be done differently, but I'm not happy with the
implications of your suggestion, and I've explained why. I would
(hesitantly, i.e. just to get the build issue out of the way) ack
your variant if you submitted it, but I'd appreciate if you would
re-consider whether you could live with going with the one here.

Jan

> --- a/xen/arch/x86/cpu/mtrr/generic.c
> +++ b/xen/arch/x86/cpu/mtrr/generic.c
> @@ -182,7 +182,7 @@ static void __init print_fixed(unsigned
>  static void __init print_mtrr_state(const char *level)
>  {
>  	unsigned int i;
> -	int width;
> +	unsigned char width; /* gcc9 doesn't like plain "int" here */
>  
>  	printk("%sMTRR default type: %s\n", level,
>  	       mtrr_attrib_to_str(mtrr_state.def_type));
> 
> 




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

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

[Andrew Cooper]

On 14/06/2019 16:56, Jan Beulich wrote:
>>>> On 07.03.19 at 11:32,  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@suse.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> This one's still pending for us to build cleanly with gcc 9.

I can't reproduce it with any build of GCC 9 (all of which are straight
from the upstream tree).  Is this a locally patched version?

> I know you'd like it be done differently, but I'm not happy with the
> implications of your suggestion, and I've explained why.

But you haven't adequately (IMO) addressed any of the shortcomings. 
Notably that the argument falls down on all common Intel platforms, and
its still a piece of magic which only you know how to interpret.

> I would
> (hesitantly, i.e. just to get the build issue out of the way) ack
> your variant if you submitted it, but I'd appreciate if you would
> re-consider whether you could live with going with the one here.

I'm happy to put a SoB and real commit message on my patch, but I'd like
to actually get to the bottom of the build failure, given that it hasn't
been reproduced by anyone else using GCC 9.

I don't view limiting the type as a viable fix, because all does is try
to game whichever piece of logic GCC is using to object to the
construct, and is therefore likely to break again in the future.

~Andrew

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

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

[Jan Beulich]

>>> On 17.06.19 at 17:47, <andrew.cooper3@citrix.com> wrote:
> On 14/06/2019 16:56, Jan Beulich wrote:
>>>>> On 07.03.19 at 11:32,  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@suse.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> This one's still pending for us to build cleanly with gcc 9.
> 
> I can't reproduce it with any build of GCC 9 (all of which are straight
> from the upstream tree).  Is this a locally patched version?

This was with a pre-RC version of gcc9; I wouldn't be surprised if they
changed their code before the release. I admit it didn't occur to me to
retry building without the patch.

>> I know you'd like it be done differently, but I'm not happy with the
>> implications of your suggestion, and I've explained why.
> 
> But you haven't adequately (IMO) addressed any of the shortcomings. 
> Notably that the argument falls down on all common Intel platforms, and
> its still a piece of magic which only you know how to interpret.

I'm unaware of shortcomings, and the "magics" of the number of digits
logged is simply not relevant to people now knowing of this. It is
relevant to people like me who do know. And I can't do anything about
the number of address bits on common Intel platforms not being evenly
divisible by 4.

>> I would
>> (hesitantly, i.e. just to get the build issue out of the way) ack
>> your variant if you submitted it, but I'd appreciate if you would
>> re-consider whether you could live with going with the one here.
> 
> I'm happy to put a SoB and real commit message on my patch, but I'd like
> to actually get to the bottom of the build failure, given that it hasn't
> been reproduced by anyone else using GCC 9.
> 
> I don't view limiting the type as a viable fix, because all does is try
> to game whichever piece of logic GCC is using to object to the
> construct, and is therefore likely to break again in the future.

Well, I can't exclude this happening, but the mention of INT_MAX in
the diagnostic doesn't seem to make this very likely with an 8-bit wide
width specifier.

Jan


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

Re: [PATCH 0/2] x86: fix build with gcc9

[Wei Liu]

On Thu, Mar 07, 2019 at 03:23:44AM -0700, Jan Beulich wrote:
> Several build issues (new warnings, causing the build to fail due to
> -Werror) have been found. The two changes here address the ones
> I can actually repro; there are a few more related to
> -Waddress-of-packed-member which I haven't been able to see
> myself, and hence for now I'm unable to sort out a proper fix /
> workaround for them.
> 
> Despite this coming late, I think this is a worthwhile change for 4.12
> with pretty low risk.
> 
> 1: e820: fix build with gcc9
> 2: mtrr: fix build with gcc9
> 

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH 0/2] x86: fix build with gcc9

[M A Young]

On Thu, 7 Mar 2019, Jan Beulich wrote:

> Several build issues (new warnings, causing the build to fail due to
> -Werror) have been found. The two changes here address the ones
> I can actually repro; there are a few more related to
> -Waddress-of-packed-member which I haven't been able to see
> myself, and hence for now I'm unable to sort out a proper fix /
> workaround for them.

I found issues with -Waddress-of-packed-member in 3 places in Fedora. My 
(possibly flawed) attempt to fix the gcc9 issues (on 4.11) is at 
https://src.fedoraproject.org/rpms/xen/blob/master/f/xen.gcc9.fixes.patch 

I am running an install of xen with the patch applied so it seems 
functional.

	Michael Young

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

Re: [PATCH 0/2] x86: fix build with gcc9

[Jan Beulich]

>>> On 07.03.19 at 12:37, <m.a.young@durham.ac.uk> wrote:
> On Thu, 7 Mar 2019, Jan Beulich wrote:
> 
>> Several build issues (new warnings, causing the build to fail due to
>> -Werror) have been found. The two changes here address the ones
>> I can actually repro; there are a few more related to
>> -Waddress-of-packed-member which I haven't been able to see
>> myself, and hence for now I'm unable to sort out a proper fix /
>> workaround for them.
> 
> I found issues with -Waddress-of-packed-member in 3 places in Fedora. My 
> (possibly flawed) attempt to fix the gcc9 issues (on 4.11) is at 
> https://src.fedoraproject.org/rpms/xen/blob/master/f/xen.gcc9.fixes.patch 

So here we see how effort gets duplicated: What was found here
internally was never posted. What you've found was also never
posted. Who knows how many more distros carry some for of fix
(or perhaps better workaround) for this. May I ask everyone that
such fixes be posted on xen-devel, even if the quick-and-dirty
solutions may not be what we want to commit? It'll at least allow
everyone to know where the issues are and to think of suitable
code changes.

Also sadly your reply doesn't really help me understand why I'm
not myself seeing those -Waddress-of-packed-member issues.

Jan



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

Ping: [PATCH 0/2] x86: fix build with gcc9

[Jan Beulich]

>>> On 07.03.19 at 11:23, <JBeulich@suse.com> wrote:
> Several build issues (new warnings, causing the build to fail due to
> -Werror) have been found. The two changes here address the ones
> I can actually repro; there are a few more related to
> -Waddress-of-packed-member which I haven't been able to see
> myself, and hence for now I'm unable to sort out a proper fix /
> workaround for them.
> 
> Despite this coming late, I think this is a worthwhile change for 4.12
> with pretty low risk.
> 
> 1: e820: fix build with gcc9
> 2: mtrr: fix build with gcc9
> 
> Jan



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