When adding new featureset words, it is convenient to split the work into
several patches.  However, GCC 12 spotted that the way we prefer to split the
work results in a real (transient) breakage whereby the policy <-> featureset
helpers perform out-of-bounds accesses on the featureset array.

Fix this by having gen-cpuid.py calculate FEATURESET_NR_ENTRIES from the
comments describing the word blocks, rather than from the XEN_CPUFEATURE()
with the greatest value.

For simplicty, require that the word blocks appear in order.  This can be
revisted if we find a good reason to have blocks out of order.

No functional change.

Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

This supercedes the entire "x86: Fix transient build breakage with featureset
additions" series, but doesn't really feel as if it ought to be labelled v2
---
 xen/tools/gen-cpuid.py | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index 0edb7d4a19f8..e0664e0defe1 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -50,13 +50,37 @@ def parse_definitions(state):
         "\s+([\s\d]+\*[\s\d]+\+[\s\d]+)\)"
         "\s+/\*([\w!]*) .*$")
 
+    word_regex = re.compile(
+        r"^/\* .* word (\d*) \*/$")
+    last_word = -1
+
     this = sys.modules[__name__]
 
     for l in state.input.readlines():
-        # Short circuit the regex...
-        if not l.startswith("XEN_CPUFEATURE("):
+
+        # Short circuit the regexes...
+        if not (l.startswith("XEN_CPUFEATURE(") or
+                l.startswith("/* ")):
             continue
 
+        # Handle /* ... word $N */ lines
+        if l.startswith("/* "):
+
+            res = word_regex.match(l)
+            if res is None:
+                continue # Some other comment
+
+            word = int(res.groups()[0])
+
+            if word != last_word + 1:
+                raise Fail("Featureset word %u out of order (last word %u)"
+                           % (word, last_word))
+
+            last_word = word
+            state.nr_entries = word + 1
+            continue
+
+        # Handle XEN_CPUFEATURE( lines
         res = feat_regex.match(l)
 
         if res is None:
@@ -94,6 +118,15 @@ def parse_definitions(state):
     if len(state.names) == 0:
         raise Fail("No features found")
 
+    if state.nr_entries == 0:
+        raise Fail("No featureset word info found")
+
+    max_val = max(state.names.keys())
+    if (max_val >> 5) + 1 > state.nr_entries:
+        max_name = state.names[max_val]
+        raise Fail("Feature %s (%d*32+%d) exceeds FEATURESET_NR_ENTRIES (%d)"
+                   % (max_name, max_val >> 5, max_val & 31, state.nr_entries))
+
 def featureset_to_uint32s(fs, nr):
     """ Represent a featureset as a list of C-compatible uint32_t's """
 
@@ -122,9 +155,6 @@ def format_uint32s(state, featureset, indent):
 
 def crunch_numbers(state):
 
-    # Size of bitmaps
-    state.nr_entries = nr_entries = (max(state.names.keys()) >> 5) + 1
-
     # Features common between 1d and e1d.
     common_1d = (FPU, VME, DE, PSE, TSC, MSR, PAE, MCE, CX8, APIC,
                  MTRR, PGE, MCA, CMOV, PAT, PSE36, MMX, FXSR)
@@ -328,7 +358,7 @@ def crunch_numbers(state):
     state.nr_deep_deps = len(state.deep_deps.keys())
 
     # Calculate the bitfield name declarations
-    for word in range(nr_entries):
+    for word in range(state.nr_entries):
 
         names = []
         for bit in range(32):
-- 
2.30.2

On 12.05.2023 14:45, Andrew Cooper wrote:
> When adding new featureset words, it is convenient to split the work into
> several patches.  However, GCC 12 spotted that the way we prefer to split the
> work results in a real (transient) breakage whereby the policy <-> featureset
> helpers perform out-of-bounds accesses on the featureset array.
> 
> Fix this by having gen-cpuid.py calculate FEATURESET_NR_ENTRIES from the
> comments describing the word blocks, rather than from the XEN_CPUFEATURE()
> with the greatest value.
> 
> For simplicty, require that the word blocks appear in order.  This can be
> revisted if we find a good reason to have blocks out of order.
> 
> No functional change.
> 
> Reported-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

As far as my Python goes:
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Just one remark further down.

> This supercedes the entire "x86: Fix transient build breakage with featureset
> additions" series, but doesn't really feel as if it ought to be labelled v2

Thank you for re-doing this altogether. I think it's more safe this way,
and it now being less intrusive is imo also beneficial.

> --- a/xen/tools/gen-cpuid.py
> +++ b/xen/tools/gen-cpuid.py
> @@ -50,13 +50,37 @@ def parse_definitions(state):
>          "\s+([\s\d]+\*[\s\d]+\+[\s\d]+)\)"
>          "\s+/\*([\w!]*) .*$")
>  
> +    word_regex = re.compile(
> +        r"^/\* .* word (\d*) \*/$")
> +    last_word = -1
> +
>      this = sys.modules[__name__]
>  
>      for l in state.input.readlines():
> -        # Short circuit the regex...
> -        if not l.startswith("XEN_CPUFEATURE("):
> +
> +        # Short circuit the regexes...
> +        if not (l.startswith("XEN_CPUFEATURE(") or
> +                l.startswith("/* ")):
>              continue
>  
> +        # Handle /* ... word $N */ lines
> +        if l.startswith("/* "):
> +
> +            res = word_regex.match(l)
> +            if res is None:
> +                continue # Some other comment
> +
> +            word = int(res.groups()[0])
> +
> +            if word != last_word + 1:
> +                raise Fail("Featureset word %u out of order (last word %u)"
> +                           % (word, last_word))
> +
> +            last_word = word
> +            state.nr_entries = word + 1
> +            continue
> +
> +        # Handle XEN_CPUFEATURE( lines
>          res = feat_regex.match(l)
>  
>          if res is None:
> @@ -94,6 +118,15 @@ def parse_definitions(state):
>      if len(state.names) == 0:
>          raise Fail("No features found")
>  
> +    if state.nr_entries == 0:
> +        raise Fail("No featureset word info found")
> +
> +    max_val = max(state.names.keys())
> +    if (max_val >> 5) + 1 > state.nr_entries:

Maybe

    if (max_val >> 5) >= state.nr_entries:

?

Jan

On 15/05/2023 8:46 am, Jan Beulich wrote:
> On 12.05.2023 14:45, Andrew Cooper wrote:
>> When adding new featureset words, it is convenient to split the work into
>> several patches.  However, GCC 12 spotted that the way we prefer to split the
>> work results in a real (transient) breakage whereby the policy <-> featureset
>> helpers perform out-of-bounds accesses on the featureset array.
>>
>> Fix this by having gen-cpuid.py calculate FEATURESET_NR_ENTRIES from the
>> comments describing the word blocks, rather than from the XEN_CPUFEATURE()
>> with the greatest value.
>>
>> For simplicty, require that the word blocks appear in order.  This can be
>> revisted if we find a good reason to have blocks out of order.
>>
>> No functional change.
>>
>> Reported-by: Jan Beulich <jbeulich@suse.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> As far as my Python goes:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> Just one remark further down.
>
>> This supercedes the entire "x86: Fix transient build breakage with featureset
>> additions" series, but doesn't really feel as if it ought to be labelled v2
> Thank you for re-doing this altogether. I think it's more safe this way,
> and it now being less intrusive is imo also beneficial.

Yeah, now I've done both I do prefer this version.

>> --- a/xen/tools/gen-cpuid.py
>> +++ b/xen/tools/gen-cpuid.py
>> @@ -94,6 +118,15 @@ def parse_definitions(state):
>>      if len(state.names) == 0:
>>          raise Fail("No features found")
>>  
>> +    if state.nr_entries == 0:
>> +        raise Fail("No featureset word info found")
>> +
>> +    max_val = max(state.names.keys())
>> +    if (max_val >> 5) + 1 > state.nr_entries:
> Maybe
>
>     if (max_val >> 5) >= state.nr_entries:

Done.

~Andrew