Re: [PATCH v11 15/29] x86/arch_prctl: Create ARCH_SET_STATE_ENABLE/ARCH_GET_STATE_ENABLE

[Thomas Gleixner]

On Tue, Oct 05 2021 at 02:30, Thomas Gleixner wrote:
> On Fri, Oct 01 2021 at 15:37, Chang S. Bae wrote:
>> +		if (state_perm & ~features_mask)
>> +			return -EINVAL;
>> +
>> +		state_perm &= xfeatures_mask_user_perm();
>> +		if (!state_perm)
>> +			return 0;
>
> I really do not get the semantics of this prctl at all.
>
> GET stores _all_ permitted bits in the user space variable which makes
> sense.
>
> SET is just accepting everything except not supported bits, but as it
> takes a feature bitmask it suggests that this sets the xfeature bits
> which are available for the task or the process.
>
> How does prctl(..., SET, 0) make sense?
>
> It does not make any sense at all. There is no support for downgrading
> the permitted features:
>
>     1) Default features up to AVX512 cannot be disabled
>     
>     2) Once AMX (or any upcoming state) is enabled there is not way back.
>
> So no. This really want's to be
>
>    prctl(SET, xfeature_number)
>
> and not something which is semanticaly ill defined.

So of course this is odd at all ends because AMX requires two feature
bits to be enabled (17 and 18).

Now with the above bitmap based thing prctl(SET, 1 << 17) is valid
because it's supported and of course there is no sanity check at all.

With a feature number based interface it's even worse. Duh, should have
thought about that.

So this gives us two options:

   1) Bitmap with proper sanity checks

      reject (1 << 17) and (1 << 18)
      grant (1 << 17 | 1 << 18)

      but for sanity sake and also for ease of filtering, we want to
      restrict a permission request to one functional block at a time.

      #define X86_XCOMP_AMX	(1 << 17 | 1 << 18)
      #define X86_XCOMP_XYZ1    (1 << 19)

      But that gets a bit odd when there is a component which depends on
      others:

      #define X86_XCOMP_XYZ2    (1 << 19 | 1 << 20)

   2) Facility based numerical interface, i.e.

      #define X86_XCOMP_AMX	1
      #define X86_XCOMP_XYZ1    2
      #define X86_XCOMP_XYZ2    3

      is way simpler to understand IMO.

Thanks,

        tglx