[PATCH] x86: Simplify AMX XSAVEC calculation logic.

[H.J. Lu]

On Wed, Apr 2, 2025 at 4:46 PM Sunil Pandey <skpgkp2@gmail.com> wrote:
>
>
>
> On Wed, Apr 2, 2025 at 3:24 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>> On Tue, Apr 1, 2025 at 7:07 AM Sunil Pandey <skpgkp2@gmail.com> wrote:
>> >
>> >
>> >
>> > On Mon, Mar 31, 2025 at 9:25 PM Sunil Pandey <skpgkp2@gmail.com> wrote:
>> >>
>> >>
>> >>
>> >> On Mon, Mar 31, 2025 at 6:13 AM Florian Weimer <fweimer@redhat.com> wrote:
>> >>>
>> >>> * Sunil K. Pandey:
>> >>>
>> >>> > +#ifdef __x86_64__
>> >>> > +                   if (amx_flag)
>> >>> > +                     {
>> >>> > +                       unsigned int amx_size
>> >>> > +                         = xstate_comp_offsets[18] + xstate_comp_sizes[18];
>> >>> > +                       _dl_x86_features_tlsdesc_state_size
>> >>> > +                         = ALIGN_UP (amx_size + TLSDESC_CALL_REGISTER_SAVE_AREA,
>> >>> > +                                     64);
>> >>> > +                     }
>> >>> > +#endif
>> >>>
>> >>> I diskike the magic 18 (as the AMX bit number) appearing here.
>> >>>
>> >>> The logic for including and excluding states seems reversed to me.
>> >>> Shouldn't we save and restore everything that we don't know anything
>> >>> about, especially for the TLSDESC case?
>> >>>
>> >>> Thanks,
>> >>> Florian
>> >>
>> >>
>> >> Thank you so much. Will submit v2.
>> >
>> >
>> > v2 patch attached.
>>
>> Please add more descriptions in the commit log.
>>
>>  #ifdef __x86_64__
>> -       unsigned int amx_size
>> - = (xstate_amx_comp_offsets[31]
>> -    + xstate_amx_comp_sizes[31]);
>> -       amx_size
>> - = ALIGN_UP ((amx_size
>> -      + TLSDESC_CALL_REGISTER_SAVE_AREA),
>> -     64);
>> -       /* Set TLSDESC state size to the compact AMX
>> - state size for XSAVEC.  */
>> -       _dl_x86_features_tlsdesc_state_size = amx_size;
>> +       _dl_x86_features_tlsdesc_state_size
>> + = ALIGN_UP (size + TLSDESC_CALL_REGISTER_SAVE_AREA, 64);
>> +       /* Subtract AMX size from total size.  */
>>
>> Subtract size from the start of X86_XSTATE_TILECFG_ID space to the end of
>> X86_XSTATE_TILEDATA_ID space.
>>
>> +       size -= ((xstate_comp_offsets[18]
>> + + xstate_comp_sizes[18])
>>
>> xstate_comp_offsets[X86_XSTATE_TILEDATA_ID + 1]?
>>
>> +        - (xstate_comp_offsets[16]
>> +   + xstate_comp_sizes[16]));
>>
>> xstate_comp_offsets[X86_XSTATE_TILECFG_ID]?
>
>
>
>  X86_XSTATE_TILEDATA_ID and X86_XSTATE_TILECFG_ID must start from cache aligned location.
> Depending on which previous state gets enabled, size calculation may not be correct.
>
> How about
>
>  size -= (xstate_comp_offsets[X86_XSTATE_TILEDATA_ID + 1]
>               - (xstate_comp_offsets[X86_XSTATE_TILECFG_ID - 1]
>                  + xstate_comp_sizes[X86_XSTATE_TILECFG_ID - 1]));
>

How about

size  = ALIGN_UP (size + TLSDESC_CALL_REGISTER_SAVE_AREA,  64);
size -= xstate_comp_offsets[X86_XSTATE_TILEDATA_ID + 1] -
xstate_comp_offsets[X86_XSTATE_TILECFG_ID];

xstate_comp_offsets[X86_XSTATE_TILECFG_ID] ==
xstate_comp_offsets[X86_XSTATE_TILEDATA_ID + 1]
if there is no AMX.  Otherwise, they are multiples of 64.


-- 
H.J.