Re: [PATCH v2] x86/gen-cpuid: correct cycle detection

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/09/2025 12:02 pm, Jan Beulich wrote:
> On 01.09.2025 12:31, Andrew Cooper wrote:
>> On 01/09/2025 9:56 am, Jan Beulich wrote:
>>> With the processing done linearly (rather than recursively), checking
>>> whether any of the features was previously seen is wrong: That would
>>> e.g. trigger for this simple set of dependencies
>>>
>>>     X: [A, B]
>>>     A: [C]
>>>     B: [C]
>>>
>>> (observed in reality when making AMX-AVX512 dependent upon both
>>> AMX-TILE and AVX512F, causing XSAVE to see AMX-AVX512 twice in its list
>>> of dependents). But checking the whole accumulated set also isn't
>>> necessary - just checking the feature we're processing dependents of is
>>> sufficient. We may detect a cycle later that way, but we still will
>>> detect it. What we need to avoid is adding a feature again when we've
>>> already seen it.
>>>
>>> As a result, seeding "seen[]" with "feat" isn't necessary anymore.
>>>
>>> Fixes: fe4408d180f4 ("xen/x86: Generate deep dependencies of features")
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, with one further
>> minor adjustment.
> Thanks.
>
>>> --- a/xen/tools/gen-cpuid.py
>>> +++ b/xen/tools/gen-cpuid.py
>>> @@ -379,14 +379,17 @@ def crunch_numbers(state):
>>>  
>>>              f = to_process.pop(0)
>>>  
>>> +            if f == feat:
>>> +                raise Fail("ERROR: Cycle found when processing %s" % \
>> No need for the \ here.
> Okay, but then why is there one in the commented out code you touch in the
> other patch?

Oh, that's wrong too.

That will have originally been a print statement (no brackets in py2,
thus needing the line continuation) which I refactored to
sys.stderr.write() (has brackets) and didn't clean up correctly.

I'll adjust it in my patch, as I'm dropping the trailing whitespace as well.

~Andrew