Re: [RFC PATCH 00/12] Support vector and more extended registers in perf

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 6/18/2025 6:47 PM, Liang, Kan wrote:
>
> On 2025-06-17 8:57 p.m., Mi, Dapeng wrote:
>> On 6/17/2025 11:23 PM, Liang, Kan wrote:
>>> On 2025-06-17 10:29 a.m., Peter Zijlstra wrote:
>>>> On Tue, Jun 17, 2025 at 09:52:12AM -0400, Liang, Kan wrote:
>>>>
>>>>> OK. So the sample_simd_reg_words actually has another meaning now.
>>>> Well, any simd field being non-zero means userspace knows about it. Sort
>>>> of an implicit flag.
>>> Yes, but the tool probably wouldn't to touch any simd fields if user
>>> doesn't ask for simd registers
>>>
>>>>> It's used as a flag to tell whether utilizing the old format.
>>>>>
>>>>> If so, I think it may be better to have a dedicate sample_simd_reg_flag
>>>>> field.
>>>>>
>>>>> For example,
>>>>>
>>>>> #define SAMPLE_SIMD_FLAGS_FORMAT_LEGACY		0x0
>>>>> #define SAMPLE_SIMD_FLAGS_FORMAT_WORDS		0x1
>>>>>
>>>>> 	__u8 sample_simd_reg_flags;
>>>>> 	__u8 sample_simd_reg_words;
>>>>> 	__u64 sample_simd_reg_intr;
>>>>> 	__u64 sample_simd_reg_user;
>>>>>
>>>>> If (sample_simd_reg_flags != 0) reclaims the XMM space for APX and SPP.
>>>>>
>>>>> Does it make sense?
>> Not sure if I missed some discussion, but are these fields only intended
>> for SIMD regs? What about the APX extended GPRs? Suppose APX eGPRs can
>> reuse the legacy XMM bitmaps in sample_regs_user/intr[47:32], but we need
>> an extra flag to distinguish it's XMM regs or APX eGPRs, maybe add an extra
>> bit sample_egpr_reg : 1 in sample_simd_reg_words, but the *simd* word in
>> the name would become ambiguous.
> It can be used to explicitly tell the kernel that a new format is
> expected. The new format means
> - Put APX and SPP into sample_regs_user/intr[47:32]
> - Use the sample_simd_reg_*
>
> Alternatively, as Peter suggested, we can use the sample_simd_reg_words
> to imply the new format.
> If so, I will make it an union, for example.
> 	union {
> 		__u16 sample_reg_flags;
> 		__u16 sample_simd_reg_words;
> 	};
>
> The first thing the tool does should be to set sample_reg_flags = 1,
> regardless of whether simd is requested.

So just double check, as long as the sample_reg_flags
(sample_simd_reg_words) > 0, the below new format would be used.

    sample_regs_user/intr[31:0] bits unchanged, still represent the
original GPRs.

    sample_regs_user/intr[47:32] bits represents APX eGPRs R31 - R16.

    As for the SIMD regs including XMM regs, they are represented by the
dedicated SIMD regs structure ( or regs bitmap and regs word length) .

If sample_reg_flags (sample_simd_reg_words) == 0, then it falls back to
current format.

    sample_regs_user/intr[31:0] bits represent the original GPRs.

    sample_regs_user/intr[63:32] bits represent XMM regs.

If so, I think it's fine. The new format looks more reasonable than current
one.


>
>>
>>>> Not sure, it eats up a whole byte. Dapeng seemed to favour separate
>>>> intr/user vector width (although I'm not quite sure what the use would
>>>> be).
>> The reason that I prefer to add 2 separate "words" item is that user could
>> sample interrupt and user space SIMD regs (but with different bit-width)
>> simultaneously in theory, like "--intr-regs=YMM0, --user-regs=XMM0".
> I'm not sure why the user wants a different bit-width. The
> --user-regs=XMM0" doesn't seem to provide more useful information.
>
> Anyway, I believe the tool can handle this case. The tool can always ask
> YMM0 for both --intr-regs and --user-regs, but only output the XMM0 for
> --user-regs. The only drawback is that the kernel may dump extra
> information for the --user-regs. I don't think it's a big problem.
If we intent to handle it in user space tools, I'm not sure if user space
tool can easily know which records are from user space and filter out the
SIMD regs from kernel space and how complicated would the change be. IMO,
adding an extra u16 "words" would be much easier and won't consume too much
memory.


>
> Thanks,
> Kan 
>>
>>>> If you want an explicit bit, we might as well use one from __reserved_1,
>>>> we still have some left.
>>> OK. I may add a sample_simd_reg : 1 to explicitly tell kernel to utilize
>>> the sample_simd_reg_XXX.
>>>
>>> Thanks,
>>> Kan