diff mbox series

[5/5] perf: arm_spe: Decode SVE events

Message ID 20200922101225.183554-6-andre.przywara@arm.com
State New, archived
Headers show
Series perf: arm64: Support ARMv8.3-SPE extensions | expand

Commit Message

Andre Przywara Sept. 22, 2020, 10:12 a.m. UTC
The Scalable Vector Extension (SVE) is an ARMv8 architecture extension
that introduces very long vector operations (up to 2048 bits).
The SPE profiling feature can tag SVE instructions with additional
properties like predication or the effective vector length.

Decode the new operation type bits in the SPE decoder to allow the perf
tool to correctly report about SVE instructions.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 48 ++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

Comments

Leo Yan Sept. 27, 2020, 3:30 a.m. UTC | #1
Hi Andre,

On Tue, Sep 22, 2020 at 11:12:25AM +0100, Andre Przywara wrote:
> The Scalable Vector Extension (SVE) is an ARMv8 architecture extension
> that introduces very long vector operations (up to 2048 bits).
> The SPE profiling feature can tag SVE instructions with additional
> properties like predication or the effective vector length.
> 
> Decode the new operation type bits in the SPE decoder to allow the perf
> tool to correctly report about SVE instructions.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 48 ++++++++++++++++++-
>  1 file changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> index a033f34846a6..f0c369259554 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> @@ -372,8 +372,35 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  	}
>  	case ARM_SPE_OP_TYPE:
>  		switch (idx) {
> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> +		case 0: {
> +			size_t blen = buf_len;
> +
> +			if ((payload & 0x89) == 0x08) {
> +				ret = snprintf(buf, buf_len, "SVE");
> +				buf += ret;
> +				blen -= ret;
> +				if (payload & 0x2)
> +					ret = snprintf(buf, buf_len, " FP");
> +				else
> +					ret = snprintf(buf, buf_len, " INT");
> +				buf += ret;
> +				blen -= ret;
> +				if (payload & 0x4) {
> +					ret = snprintf(buf, buf_len, " PRED");
> +					buf += ret;
> +					blen -= ret;
> +				}
> +				/* Bits [7..4] encode the vector length */
> +				ret = snprintf(buf, buf_len, " EVLEN%d",
> +					       32 << ((payload >> 4) & 0x7));
> +				buf += ret;
> +				blen -= ret;
> +				return buf_len - blen;
> +			}
> +
> +			return snprintf(buf, buf_len, "%s", payload & 0x1 ?
>  					"COND-SELECT" : "INSN-OTHER");
> +			}
>  		case 1:	{
>  			size_t blen = buf_len;
>  
> @@ -403,6 +430,25 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  				ret = snprintf(buf, buf_len, " NV-SYSREG");
>  				buf += ret;
>  				blen -= ret;
> +			} else if ((payload & 0x0a) == 0x08) {
> +				ret = snprintf(buf, buf_len, " SVE");
> +				buf += ret;
> +				blen -= ret;
> +				if (payload & 0x4) {
> +					ret = snprintf(buf, buf_len, " PRED");
> +					buf += ret;
> +					blen -= ret;
> +				}
> +				if (payload & 0x80) {
> +					ret = snprintf(buf, buf_len, " SG");
> +					buf += ret;
> +					blen -= ret;
> +				}
> +				/* Bits [7..4] encode the vector length */
> +				ret = snprintf(buf, buf_len, " EVLEN%d",
> +					       32 << ((payload >> 4) & 0x7));
> +				buf += ret;
> +				blen -= ret;

The changes in this patch has been included in the patch [1].

So my summary for patches 02 ~ 05, except patch 04, other changes has
been included in the patch set "perf arm-spe: Refactor decoding &
dumping flow".

I'd like to add your patch 04 into the patch set "perf arm-spe:
Refactor decoding & dumping flow" and I will respin the patch set v2 on
the latest perf/core branch and send out to review.

For patch 01, you could continue to try to land it in the kernel.
(Maybe consolidate a bit with Wei?).

Do you think this is okay for you?

Thanks,
Leo

[1] https://lore.kernel.org/patchwork/patch/1288413/

>  			} else if (payload & 0x4) {
>  				ret = snprintf(buf, buf_len, " SIMD-FP");
>  				buf += ret;
> -- 
> 2.17.1
>
Andre Przywara Sept. 28, 2020, 10:15 a.m. UTC | #2
On 27/09/2020 04:30, Leo Yan wrote:

Hi Leo,

> On Tue, Sep 22, 2020 at 11:12:25AM +0100, Andre Przywara wrote:
>> The Scalable Vector Extension (SVE) is an ARMv8 architecture extension
>> that introduces very long vector operations (up to 2048 bits).
>> The SPE profiling feature can tag SVE instructions with additional
>> properties like predication or the effective vector length.
>>
>> Decode the new operation type bits in the SPE decoder to allow the perf
>> tool to correctly report about SVE instructions.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 48 ++++++++++++++++++-
>>  1 file changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
>> index a033f34846a6..f0c369259554 100644
>> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
>> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
>> @@ -372,8 +372,35 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>>  	}
>>  	case ARM_SPE_OP_TYPE:
>>  		switch (idx) {
>> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
>> +		case 0: {
>> +			size_t blen = buf_len;
>> +
>> +			if ((payload & 0x89) == 0x08) {
>> +				ret = snprintf(buf, buf_len, "SVE");
>> +				buf += ret;
>> +				blen -= ret;
>> +				if (payload & 0x2)
>> +					ret = snprintf(buf, buf_len, " FP");
>> +				else
>> +					ret = snprintf(buf, buf_len, " INT");
>> +				buf += ret;
>> +				blen -= ret;
>> +				if (payload & 0x4) {
>> +					ret = snprintf(buf, buf_len, " PRED");
>> +					buf += ret;
>> +					blen -= ret;
>> +				}
>> +				/* Bits [7..4] encode the vector length */
>> +				ret = snprintf(buf, buf_len, " EVLEN%d",
>> +					       32 << ((payload >> 4) & 0x7));
>> +				buf += ret;
>> +				blen -= ret;
>> +				return buf_len - blen;
>> +			}
>> +
>> +			return snprintf(buf, buf_len, "%s", payload & 0x1 ?
>>  					"COND-SELECT" : "INSN-OTHER");
>> +			}
>>  		case 1:	{
>>  			size_t blen = buf_len;
>>  
>> @@ -403,6 +430,25 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>>  				ret = snprintf(buf, buf_len, " NV-SYSREG");
>>  				buf += ret;
>>  				blen -= ret;
>> +			} else if ((payload & 0x0a) == 0x08) {
>> +				ret = snprintf(buf, buf_len, " SVE");
>> +				buf += ret;
>> +				blen -= ret;
>> +				if (payload & 0x4) {
>> +					ret = snprintf(buf, buf_len, " PRED");
>> +					buf += ret;
>> +					blen -= ret;
>> +				}
>> +				if (payload & 0x80) {
>> +					ret = snprintf(buf, buf_len, " SG");
>> +					buf += ret;
>> +					blen -= ret;
>> +				}
>> +				/* Bits [7..4] encode the vector length */
>> +				ret = snprintf(buf, buf_len, " EVLEN%d",
>> +					       32 << ((payload >> 4) & 0x7));
>> +				buf += ret;
>> +				blen -= ret;
> 
> The changes in this patch has been included in the patch [1].
> 
> So my summary for patches 02 ~ 05, except patch 04, other changes has
> been included in the patch set "perf arm-spe: Refactor decoding &
> dumping flow".

Ah, my sincere apologies, I totally missed Wei's and your series on this
(although I did some research on "prior art").

> I'd like to add your patch 04 into the patch set "perf arm-spe:
> Refactor decoding & dumping flow" and I will respin the patch set v2 on
> the latest perf/core branch and send out to review.
> 
> For patch 01, you could continue to try to land it in the kernel.
> (Maybe consolidate a bit with Wei?).
> 
> Do you think this is okay for you?

Yes, sounds like a plan. So Wei's original series is now fully
integrated into your 13-patch rework, right?

Is "[RESEND,v1,xx/13] ..." the latest revision of your series?
Do you plan on sending a v2 anytime soon? Or shall I do review on the
existing one?

Cheers,
Andre

> 
> [1] https://lore.kernel.org/patchwork/patch/1288413/
> 
>>  			} else if (payload & 0x4) {
>>  				ret = snprintf(buf, buf_len, " SIMD-FP");
>>  				buf += ret;
>> -- 
>> 2.17.1
>>
Leo Yan Sept. 28, 2020, 11:08 a.m. UTC | #3
Hi Andre,

On Mon, Sep 28, 2020 at 11:15:53AM +0100, André Przywara wrote:

[...]

> > So my summary for patches 02 ~ 05, except patch 04, other changes has
> > been included in the patch set "perf arm-spe: Refactor decoding &
> > dumping flow".
> 
> Ah, my sincere apologies, I totally missed Wei's and your series on this
> (although I did some research on "prior art").

No worries!

> > I'd like to add your patch 04 into the patch set "perf arm-spe:
> > Refactor decoding & dumping flow" and I will respin the patch set v2 on
> > the latest perf/core branch and send out to review.
> > 
> > For patch 01, you could continue to try to land it in the kernel.
> > (Maybe consolidate a bit with Wei?).
> > 
> > Do you think this is okay for you?
> 
> Yes, sounds like a plan. So Wei's original series is now fully
> integrated into your 13-patch rework, right?

Thanks for confirmation.

You could see Wei's patch set has 4 patches [1].  I only picked the
patch 02 [2] from Wei's patch set into my refactoring patch set; the
patch 01 is for enabling driver for SVE events, the patches 03/04
introduced new synthesized events.

Patches 03 / 04 should be considered carefully and it's good to prove
these synthesized events will be useful for user cases before upstream
them.  The reason is AFAIK a good direction to generate SPE trace data
for memory events [3], and for SVE, I think we should firstly consider
if can reuse the memory event for profiling rather than adding new
synthesized events.

So I prefer to give priority for patches 01 / 02.

> Is "[RESEND,v1,xx/13] ..." the latest revision of your series?

Yes.

> Do you plan on sending a v2 anytime soon? Or shall I do review on the
> existing one?

For saving time, let me respin patch set v2 and send to LKML (hope in
next 1~2 days).  Then you could review patch set v2.

Thanks,
Leo

[1] https://lore.kernel.org/patchwork/cover/1278778/
[2] https://lore.kernel.org/patchwork/patch/1278780/
[3] https://lore.kernel.org/patchwork/cover/1298085/
Dave Martin Sept. 28, 2020, 1:21 p.m. UTC | #4
On Tue, Sep 22, 2020 at 11:12:25AM +0100, Andre Przywara wrote:
> The Scalable Vector Extension (SVE) is an ARMv8 architecture extension
> that introduces very long vector operations (up to 2048 bits).

(8192, in fact, though don't expect to see that on real hardware any
time soon...  qemu and the Arm fast model can do it, though.)

> The SPE profiling feature can tag SVE instructions with additional
> properties like predication or the effective vector length.
> 
> Decode the new operation type bits in the SPE decoder to allow the perf
> tool to correctly report about SVE instructions.


I don't know anything about SPE, so just commenting on a few minor
things that catch my eye here.

> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 48 ++++++++++++++++++-
>  1 file changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> index a033f34846a6..f0c369259554 100644
> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> @@ -372,8 +372,35 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  	}
>  	case ARM_SPE_OP_TYPE:
>  		switch (idx) {
> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> +		case 0: {
> +			size_t blen = buf_len;
> +
> +			if ((payload & 0x89) == 0x08) {
> +				ret = snprintf(buf, buf_len, "SVE");
> +				buf += ret;
> +				blen -= ret;

(Nit: can ret be < 0 ?  I've never been 100% clear on this myself for
the s*printf() family -- if this assumption is widespread in perf tool
a lready that I guess just go with the flow.)

I wonder if this snprintf+increment+decrement sequence could be wrapped
up as a helper, rather than having to be repeated all over the place.

> +				if (payload & 0x2)
> +					ret = snprintf(buf, buf_len, " FP");
> +				else
> +					ret = snprintf(buf, buf_len, " INT");
> +				buf += ret;
> +				blen -= ret;
> +				if (payload & 0x4) {
> +					ret = snprintf(buf, buf_len, " PRED");
> +					buf += ret;
> +					blen -= ret;
> +				}
> +				/* Bits [7..4] encode the vector length */
> +				ret = snprintf(buf, buf_len, " EVLEN%d",
> +					       32 << ((payload >> 4) & 0x7));

Isn't this just extracting 3 bits (0x7)?  And what unit are we aiming
for here: is it the number of bytes per vector, or something else?  I'm
confused by the fact that this will go up in steps of 32, which doesn't
seem to match up to the architecure.

I notice that bit 7 has to be zero to get into this if() though.

> +				buf += ret;
> +				blen -= ret;
> +				return buf_len - blen;
> +			}
> +
> +			return snprintf(buf, buf_len, "%s", payload & 0x1 ?
>  					"COND-SELECT" : "INSN-OTHER");
> +			}
>  		case 1:	{
>  			size_t blen = buf_len;
>  
> @@ -403,6 +430,25 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>  				ret = snprintf(buf, buf_len, " NV-SYSREG");
>  				buf += ret;
>  				blen -= ret;
> +			} else if ((payload & 0x0a) == 0x08) {
> +				ret = snprintf(buf, buf_len, " SVE");
> +				buf += ret;
> +				blen -= ret;
> +				if (payload & 0x4) {
> +					ret = snprintf(buf, buf_len, " PRED");
> +					buf += ret;
> +					blen -= ret;
> +				}
> +				if (payload & 0x80) {
> +					ret = snprintf(buf, buf_len, " SG");
> +					buf += ret;
> +					blen -= ret;
> +				}
> +				/* Bits [7..4] encode the vector length */
> +				ret = snprintf(buf, buf_len, " EVLEN%d",
> +					       32 << ((payload >> 4) & 0x7));

Same comment as above.  Maybe have a common helper for decoding the
vector length bits so it can be fixed in a single place?

> +				buf += ret;
> +				blen -= ret;
>  			} else if (payload & 0x4) {
>  				ret = snprintf(buf, buf_len, " SIMD-FP");
>  				buf += ret;

Cheers
---Dave
Andre Przywara Sept. 28, 2020, 1:59 p.m. UTC | #5
On 28/09/2020 14:21, Dave Martin wrote:

Hi Dave,

> On Tue, Sep 22, 2020 at 11:12:25AM +0100, Andre Przywara wrote:
>> The Scalable Vector Extension (SVE) is an ARMv8 architecture extension
>> that introduces very long vector operations (up to 2048 bits).
> 
> (8192, in fact, though don't expect to see that on real hardware any
> time soon...  qemu and the Arm fast model can do it, though.)
> 
>> The SPE profiling feature can tag SVE instructions with additional
>> properties like predication or the effective vector length.
>>
>> Decode the new operation type bits in the SPE decoder to allow the perf
>> tool to correctly report about SVE instructions.
> 
> 
> I don't know anything about SPE, so just commenting on a few minor
> things that catch my eye here.

Many thanks for taking a look!
Please note that I actually missed a prior submission by Wei, so the
code changes here will end up in:
https://lore.kernel.org/patchwork/patch/1288413/

But your two points below magically apply to his patch as well, so....

> 
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>> ---
>>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 48 ++++++++++++++++++-
>>  1 file changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
>> index a033f34846a6..f0c369259554 100644
>> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
>> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
>> @@ -372,8 +372,35 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>>  	}
>>  	case ARM_SPE_OP_TYPE:
>>  		switch (idx) {
>> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
>> +		case 0: {
>> +			size_t blen = buf_len;
>> +
>> +			if ((payload & 0x89) == 0x08) {
>> +				ret = snprintf(buf, buf_len, "SVE");
>> +				buf += ret;
>> +				blen -= ret;
> 
> (Nit: can ret be < 0 ?  I've never been 100% clear on this myself for
> the s*printf() family -- if this assumption is widespread in perf tool
> a lready that I guess just go with the flow.)

Yeah, some parts of the code in here check for -1, actually, but doing
this on every call to snprintf would push this current code over the
edge - and I cowardly avoided a refactoring ;-)

Please note that his is perf userland, and also we are printing constant
strings here.
Although admittedly this starts to sounds like an excuse now ...

> I wonder if this snprintf+increment+decrement sequence could be wrapped
> up as a helper, rather than having to be repeated all over the place.

Yes, I was hoping nobody would notice ;-)

>> +				if (payload & 0x2)
>> +					ret = snprintf(buf, buf_len, " FP");
>> +				else
>> +					ret = snprintf(buf, buf_len, " INT");
>> +				buf += ret;
>> +				blen -= ret;
>> +				if (payload & 0x4) {
>> +					ret = snprintf(buf, buf_len, " PRED");
>> +					buf += ret;
>> +					blen -= ret;
>> +				}
>> +				/* Bits [7..4] encode the vector length */
>> +				ret = snprintf(buf, buf_len, " EVLEN%d",
>> +					       32 << ((payload >> 4) & 0x7));
> 
> Isn't this just extracting 3 bits (0x7)? 

Ah, right, the comment is wrong. It's actually bits [6:4].

> And what unit are we aiming
> for here: is it the number of bytes per vector, or something else?  I'm
> confused by the fact that this will go up in steps of 32, which doesn't
> seem to match up to the architecure.

So this is how SPE encodes the effective vector length in its payload:
the format is described in section "D10.2.7 Operation Type packet" in a
(recent) ARMv8 ARM. I put the above statement in a C file and ran all
input values through it, it produced the exact *bit* length values as in
the spec.

Is there any particular pattern you are concerned about?
I admit this is somewhat hackish, I can do an extra function to put some
comments in there.

> 
> I notice that bit 7 has to be zero to get into this if() though.
> 
>> +				buf += ret;
>> +				blen -= ret;
>> +				return buf_len - blen;
>> +			}
>> +
>> +			return snprintf(buf, buf_len, "%s", payload & 0x1 ?
>>  					"COND-SELECT" : "INSN-OTHER");
>> +			}
>>  		case 1:	{
>>  			size_t blen = buf_len;
>>  
>> @@ -403,6 +430,25 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
>>  				ret = snprintf(buf, buf_len, " NV-SYSREG");
>>  				buf += ret;
>>  				blen -= ret;
>> +			} else if ((payload & 0x0a) == 0x08) {
>> +				ret = snprintf(buf, buf_len, " SVE");
>> +				buf += ret;
>> +				blen -= ret;
>> +				if (payload & 0x4) {
>> +					ret = snprintf(buf, buf_len, " PRED");
>> +					buf += ret;
>> +					blen -= ret;
>> +				}
>> +				if (payload & 0x80) {
>> +					ret = snprintf(buf, buf_len, " SG");
>> +					buf += ret;
>> +					blen -= ret;
>> +				}
>> +				/* Bits [7..4] encode the vector length */
>> +				ret = snprintf(buf, buf_len, " EVLEN%d",
>> +					       32 << ((payload >> 4) & 0x7));
> 
> Same comment as above.  Maybe have a common helper for decoding the
> vector length bits so it can be fixed in a single place?

Yup. Although I wonder if this is the smallest of the problems with this
function going forward.

Cheers,
Andre

> 
>> +				buf += ret;
>> +				blen -= ret;
>>  			} else if (payload & 0x4) {
>>  				ret = snprintf(buf, buf_len, " SIMD-FP");
>>  				buf += ret;
> 
> Cheers
> ---Dave
>
Dave Martin Sept. 28, 2020, 2:47 p.m. UTC | #6
On Mon, Sep 28, 2020 at 02:59:34PM +0100, André Przywara wrote:
> On 28/09/2020 14:21, Dave Martin wrote:
> 
> Hi Dave,
> 
> > On Tue, Sep 22, 2020 at 11:12:25AM +0100, Andre Przywara wrote:
> >> The Scalable Vector Extension (SVE) is an ARMv8 architecture extension
> >> that introduces very long vector operations (up to 2048 bits).
> > 
> > (8192, in fact, though don't expect to see that on real hardware any
> > time soon...  qemu and the Arm fast model can do it, though.)
> > 
> >> The SPE profiling feature can tag SVE instructions with additional
> >> properties like predication or the effective vector length.
> >>
> >> Decode the new operation type bits in the SPE decoder to allow the perf
> >> tool to correctly report about SVE instructions.
> > 
> > 
> > I don't know anything about SPE, so just commenting on a few minor
> > things that catch my eye here.
> 
> Many thanks for taking a look!
> Please note that I actually missed a prior submission by Wei, so the
> code changes here will end up in:
> https://lore.kernel.org/patchwork/patch/1288413/
> 
> But your two points below magically apply to his patch as well, so....
> 
> > 
> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >> ---
> >>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 48 ++++++++++++++++++-
> >>  1 file changed, 47 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> >> index a033f34846a6..f0c369259554 100644
> >> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> >> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> >> @@ -372,8 +372,35 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> >>  	}
> >>  	case ARM_SPE_OP_TYPE:
> >>  		switch (idx) {
> >> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> >> +		case 0: {
> >> +			size_t blen = buf_len;
> >> +
> >> +			if ((payload & 0x89) == 0x08) {
> >> +				ret = snprintf(buf, buf_len, "SVE");
> >> +				buf += ret;
> >> +				blen -= ret;
> > 
> > (Nit: can ret be < 0 ?  I've never been 100% clear on this myself for
> > the s*printf() family -- if this assumption is widespread in perf tool
> > a lready that I guess just go with the flow.)
> 
> Yeah, some parts of the code in here check for -1, actually, but doing
> this on every call to snprintf would push this current code over the
> edge - and I cowardly avoided a refactoring ;-)
> 
> Please note that his is perf userland, and also we are printing constant
> strings here.
> Although admittedly this starts to sounds like an excuse now ...
> 
> > I wonder if this snprintf+increment+decrement sequence could be wrapped
> > up as a helper, rather than having to be repeated all over the place.
> 
> Yes, I was hoping nobody would notice ;-)

It's probably not worth losing sleep over.

snprintf(3) says, under NOTES:

	Until glibc 2.0.6, they would return -1 when the output was
	truncated.

which is probably ancient enough history that we don't care.  C11 does
say that a negative return value can happen "if an encoding error
occurred".  _Probably_ not a problem if perf tool never calls
setlocale(), but ...


> >> +				if (payload & 0x2)
> >> +					ret = snprintf(buf, buf_len, " FP");
> >> +				else
> >> +					ret = snprintf(buf, buf_len, " INT");
> >> +				buf += ret;
> >> +				blen -= ret;
> >> +				if (payload & 0x4) {
> >> +					ret = snprintf(buf, buf_len, " PRED");
> >> +					buf += ret;
> >> +					blen -= ret;
> >> +				}
> >> +				/* Bits [7..4] encode the vector length */
> >> +				ret = snprintf(buf, buf_len, " EVLEN%d",
> >> +					       32 << ((payload >> 4) & 0x7));
> > 
> > Isn't this just extracting 3 bits (0x7)? 
> 
> Ah, right, the comment is wrong. It's actually bits [6:4].
> 
> > And what unit are we aiming
> > for here: is it the number of bytes per vector, or something else?  I'm
> > confused by the fact that this will go up in steps of 32, which doesn't
> > seem to match up to the architecure.
> 
> So this is how SPE encodes the effective vector length in its payload:
> the format is described in section "D10.2.7 Operation Type packet" in a
> (recent) ARMv8 ARM. I put the above statement in a C file and ran all
> input values through it, it produced the exact *bit* length values as in
> the spec.
> 
> Is there any particular pattern you are concerned about?
> I admit this is somewhat hackish, I can do an extra function to put some
> comments in there.

Mostly I'm curious because the encoding doesn't match the SVE
architecture: SVE requires 4 bits to specify the vector length, not 3.
This might have been a deliberate limitation in the SPE spec., but it
raises questions about what should happen when 3 bits is not enough.

For SVE, valid vector lengths are 16 bytes * n
or equivalently 128 bits * n), where 1 <= n <= 16.

The code here though cannot print EVLEN16 or EVLEN48 etc.  This might
not be a bug, but I'd like to understand where it comes from...

> 
> > 
> > I notice that bit 7 has to be zero to get into this if() though.
> > 
> >> +				buf += ret;
> >> +				blen -= ret;
> >> +				return buf_len - blen;
> >> +			}
> >> +
> >> +			return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> >>  					"COND-SELECT" : "INSN-OTHER");
> >> +			}
> >>  		case 1:	{
> >>  			size_t blen = buf_len;
> >>  
> >> @@ -403,6 +430,25 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> >>  				ret = snprintf(buf, buf_len, " NV-SYSREG");
> >>  				buf += ret;
> >>  				blen -= ret;
> >> +			} else if ((payload & 0x0a) == 0x08) {
> >> +				ret = snprintf(buf, buf_len, " SVE");
> >> +				buf += ret;
> >> +				blen -= ret;
> >> +				if (payload & 0x4) {
> >> +					ret = snprintf(buf, buf_len, " PRED");
> >> +					buf += ret;
> >> +					blen -= ret;
> >> +				}
> >> +				if (payload & 0x80) {
> >> +					ret = snprintf(buf, buf_len, " SG");
> >> +					buf += ret;
> >> +					blen -= ret;
> >> +				}
> >> +				/* Bits [7..4] encode the vector length */
> >> +				ret = snprintf(buf, buf_len, " EVLEN%d",
> >> +					       32 << ((payload >> 4) & 0x7));
> > 
> > Same comment as above.  Maybe have a common helper for decoding the
> > vector length bits so it can be fixed in a single place?
> 
> Yup. Although I wonder if this is the smallest of the problems with this
> function going forward.
> 
> Cheers,
> Andre

Fair enough.

Cheers
---Dave
Leo Yan Sept. 29, 2020, 2:19 a.m. UTC | #7
On Mon, Sep 28, 2020 at 03:47:56PM +0100, Dave Martin wrote:
> On Mon, Sep 28, 2020 at 02:59:34PM +0100, André Przywara wrote:
> > On 28/09/2020 14:21, Dave Martin wrote:
> > 
> > Hi Dave,
> > 
> > > On Tue, Sep 22, 2020 at 11:12:25AM +0100, Andre Przywara wrote:
> > >> The Scalable Vector Extension (SVE) is an ARMv8 architecture extension
> > >> that introduces very long vector operations (up to 2048 bits).
> > > 
> > > (8192, in fact, though don't expect to see that on real hardware any
> > > time soon...  qemu and the Arm fast model can do it, though.)
> > > 
> > >> The SPE profiling feature can tag SVE instructions with additional
> > >> properties like predication or the effective vector length.
> > >>
> > >> Decode the new operation type bits in the SPE decoder to allow the perf
> > >> tool to correctly report about SVE instructions.
> > > 
> > > 
> > > I don't know anything about SPE, so just commenting on a few minor
> > > things that catch my eye here.
> > 
> > Many thanks for taking a look!
> > Please note that I actually missed a prior submission by Wei, so the
> > code changes here will end up in:
> > https://lore.kernel.org/patchwork/patch/1288413/
> > 
> > But your two points below magically apply to his patch as well, so....
> > 
> > > 
> > >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > >> ---
> > >>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 48 ++++++++++++++++++-
> > >>  1 file changed, 47 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > >> index a033f34846a6..f0c369259554 100644
> > >> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > >> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > >> @@ -372,8 +372,35 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> > >>  	}
> > >>  	case ARM_SPE_OP_TYPE:
> > >>  		switch (idx) {
> > >> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> > >> +		case 0: {
> > >> +			size_t blen = buf_len;
> > >> +
> > >> +			if ((payload & 0x89) == 0x08) {
> > >> +				ret = snprintf(buf, buf_len, "SVE");
> > >> +				buf += ret;
> > >> +				blen -= ret;
> > > 
> > > (Nit: can ret be < 0 ?  I've never been 100% clear on this myself for
> > > the s*printf() family -- if this assumption is widespread in perf tool
> > > a lready that I guess just go with the flow.)
> > 
> > Yeah, some parts of the code in here check for -1, actually, but doing
> > this on every call to snprintf would push this current code over the
> > edge - and I cowardly avoided a refactoring ;-)
> > 
> > Please note that his is perf userland, and also we are printing constant
> > strings here.
> > Although admittedly this starts to sounds like an excuse now ...
> > 
> > > I wonder if this snprintf+increment+decrement sequence could be wrapped
> > > up as a helper, rather than having to be repeated all over the place.
> > 
> > Yes, I was hoping nobody would notice ;-)
> 
> It's probably not worth losing sleep over.
> 
> snprintf(3) says, under NOTES:
> 
> 	Until glibc 2.0.6, they would return -1 when the output was
> 	truncated.
> 
> which is probably ancient enough history that we don't care.  C11 does
> say that a negative return value can happen "if an encoding error
> occurred".  _Probably_ not a problem if perf tool never calls
> setlocale(), but ...

I have one patch which tried to fix the snprintf+increment sequence
[1], to be honest, the change seems urgly for me.  I agree it's better
to use a helper to wrap up.

[1] https://lore.kernel.org/patchwork/patch/1288410/

> > >> +				if (payload & 0x2)
> > >> +					ret = snprintf(buf, buf_len, " FP");
> > >> +				else
> > >> +					ret = snprintf(buf, buf_len, " INT");
> > >> +				buf += ret;
> > >> +				blen -= ret;
> > >> +				if (payload & 0x4) {
> > >> +					ret = snprintf(buf, buf_len, " PRED");
> > >> +					buf += ret;
> > >> +					blen -= ret;
> > >> +				}
> > >> +				/* Bits [7..4] encode the vector length */
> > >> +				ret = snprintf(buf, buf_len, " EVLEN%d",
> > >> +					       32 << ((payload >> 4) & 0x7));
> > > 
> > > Isn't this just extracting 3 bits (0x7)? 
> > 
> > Ah, right, the comment is wrong. It's actually bits [6:4].
> > 
> > > And what unit are we aiming
> > > for here: is it the number of bytes per vector, or something else?  I'm
> > > confused by the fact that this will go up in steps of 32, which doesn't
> > > seem to match up to the architecure.
> > 
> > So this is how SPE encodes the effective vector length in its payload:
> > the format is described in section "D10.2.7 Operation Type packet" in a
> > (recent) ARMv8 ARM. I put the above statement in a C file and ran all
> > input values through it, it produced the exact *bit* length values as in
> > the spec.
> > 
> > Is there any particular pattern you are concerned about?
> > I admit this is somewhat hackish, I can do an extra function to put some
> > comments in there.
> 
> Mostly I'm curious because the encoding doesn't match the SVE
> architecture: SVE requires 4 bits to specify the vector length, not 3.
> This might have been a deliberate limitation in the SPE spec., but it
> raises questions about what should happen when 3 bits is not enough.
> 
> For SVE, valid vector lengths are 16 bytes * n
> or equivalently 128 bits * n), where 1 <= n <= 16.
> 
> The code here though cannot print EVLEN16 or EVLEN48 etc.  This might
> not be a bug, but I'd like to understand where it comes from...

In the SPE's spec, the defined values for EVL are:

  0b'000 -> EVLEN: 32 bits.
  0b'001 -> EVLEN: 64 bits.
  0b'010 -> EVLEN: 128 bits.
  0b'011 -> EVLEN: 256 bits.
  0b'100 -> EVLEN: 512 bits.
  0b'101 -> EVLEN: 1024 bits.
  0b'110 -> EVLEN: 2048 bits.

Note that 0b'111 is reserved.  In theory, I think SPE Operation packet
can support up to 4196 bits (32 << 7) when the EVL field is 0b'111; but
it's impossible to express vector length for 8192 bits as you mentioned.

Thanks,
Leo

> > > I notice that bit 7 has to be zero to get into this if() though.
> > > 
> > >> +				buf += ret;
> > >> +				blen -= ret;
> > >> +				return buf_len - blen;
> > >> +			}
> > >> +
> > >> +			return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> > >>  					"COND-SELECT" : "INSN-OTHER");
> > >> +			}
> > >>  		case 1:	{
> > >>  			size_t blen = buf_len;
> > >>  
> > >> @@ -403,6 +430,25 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> > >>  				ret = snprintf(buf, buf_len, " NV-SYSREG");
> > >>  				buf += ret;
> > >>  				blen -= ret;
> > >> +			} else if ((payload & 0x0a) == 0x08) {
> > >> +				ret = snprintf(buf, buf_len, " SVE");
> > >> +				buf += ret;
> > >> +				blen -= ret;
> > >> +				if (payload & 0x4) {
> > >> +					ret = snprintf(buf, buf_len, " PRED");
> > >> +					buf += ret;
> > >> +					blen -= ret;
> > >> +				}
> > >> +				if (payload & 0x80) {
> > >> +					ret = snprintf(buf, buf_len, " SG");
> > >> +					buf += ret;
> > >> +					blen -= ret;
> > >> +				}
> > >> +				/* Bits [7..4] encode the vector length */
> > >> +				ret = snprintf(buf, buf_len, " EVLEN%d",
> > >> +					       32 << ((payload >> 4) & 0x7));
> > > 
> > > Same comment as above.  Maybe have a common helper for decoding the
> > > vector length bits so it can be fixed in a single place?
> > 
> > Yup. Although I wonder if this is the smallest of the problems with this
> > function going forward.
> > 
> > Cheers,
> > Andre
> 
> Fair enough.
> 
> Cheers
> ---Dave
Dave Martin Sept. 29, 2020, 2:03 p.m. UTC | #8
On Tue, Sep 29, 2020 at 10:19:02AM +0800, Leo Yan wrote:
> On Mon, Sep 28, 2020 at 03:47:56PM +0100, Dave Martin wrote:
> > On Mon, Sep 28, 2020 at 02:59:34PM +0100, André Przywara wrote:
> > > On 28/09/2020 14:21, Dave Martin wrote:
> > > 
> > > Hi Dave,
> > > 
> > > > On Tue, Sep 22, 2020 at 11:12:25AM +0100, Andre Przywara wrote:
> > > >> The Scalable Vector Extension (SVE) is an ARMv8 architecture extension
> > > >> that introduces very long vector operations (up to 2048 bits).
> > > > 
> > > > (8192, in fact, though don't expect to see that on real hardware any
> > > > time soon...  qemu and the Arm fast model can do it, though.)

[...]

> > Mostly I'm curious because the encoding doesn't match the SVE
> > architecture: SVE requires 4 bits to specify the vector length, not 3.
> > This might have been a deliberate limitation in the SPE spec., but it
> > raises questions about what should happen when 3 bits is not enough.
> > 
> > For SVE, valid vector lengths are 16 bytes * n
> > or equivalently 128 bits * n), where 1 <= n <= 16.
> > 
> > The code here though cannot print EVLEN16 or EVLEN48 etc.  This might
> > not be a bug, but I'd like to understand where it comes from...
> 
> In the SPE's spec, the defined values for EVL are:
> 
>   0b'000 -> EVLEN: 32 bits.
>   0b'001 -> EVLEN: 64 bits.
>   0b'010 -> EVLEN: 128 bits.
>   0b'011 -> EVLEN: 256 bits.
>   0b'100 -> EVLEN: 512 bits.
>   0b'101 -> EVLEN: 1024 bits.
>   0b'110 -> EVLEN: 2048 bits.
> 
> Note that 0b'111 is reserved.  In theory, I think SPE Operation packet
> can support up to 4196 bits (32 << 7) when the EVL field is 0b'111; but

OK, having looked at the spec I can now confirm that this look correct.
I was expecting a more direct correspondence between the SVE ISA and
these events, but it looks like SPE may report on a finer granularity
than whole instructions, hence showing effective vector lengths smaller
than 32; also SPE rounds the reported effective vector length up to a
power of two, which allows the full range of lengths to be reported via
the 3-bit EVL field.

> it's impossible to express vector length for 8192 bits as you mentioned.

Yes, ignore my comment about 8192-bit vectors: I was confusing myself
(the Linux API extensions support up to 8192 _bytes_ per vector in order
to have some expansion room just in case; however the SVE architecture
limits vectors to at most 2048 bits).

So I don't see any obvious issues.

It might be a good idea to explicitly reject the encoding 0b111, since
we can't be certain what it is going to mean -- however, I don't have a
strong opinion on this.

Cheers
---Dave
Dave Martin Sept. 30, 2020, 10:34 a.m. UTC | #9
On Tue, Sep 29, 2020 at 10:19:02AM +0800, Leo Yan wrote:
> On Mon, Sep 28, 2020 at 03:47:56PM +0100, Dave Martin wrote:
> > On Mon, Sep 28, 2020 at 02:59:34PM +0100, André Przywara wrote:
> > > On 28/09/2020 14:21, Dave Martin wrote:
> > > 
> > > Hi Dave,
> > > 
> > > > On Tue, Sep 22, 2020 at 11:12:25AM +0100, Andre Przywara wrote:
> > > >> The Scalable Vector Extension (SVE) is an ARMv8 architecture extension
> > > >> that introduces very long vector operations (up to 2048 bits).
> > > > 
> > > > (8192, in fact, though don't expect to see that on real hardware any
> > > > time soon...  qemu and the Arm fast model can do it, though.)
> > > > 
> > > >> The SPE profiling feature can tag SVE instructions with additional
> > > >> properties like predication or the effective vector length.
> > > >>
> > > >> Decode the new operation type bits in the SPE decoder to allow the perf
> > > >> tool to correctly report about SVE instructions.
> > > > 
> > > > 
> > > > I don't know anything about SPE, so just commenting on a few minor
> > > > things that catch my eye here.
> > > 
> > > Many thanks for taking a look!
> > > Please note that I actually missed a prior submission by Wei, so the
> > > code changes here will end up in:
> > > https://lore.kernel.org/patchwork/patch/1288413/
> > > 
> > > But your two points below magically apply to his patch as well, so....
> > > 
> > > > 
> > > >> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > >> ---
> > > >>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 48 ++++++++++++++++++-
> > > >>  1 file changed, 47 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > > >> index a033f34846a6..f0c369259554 100644
> > > >> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > > >> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > > >> @@ -372,8 +372,35 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> > > >>  	}
> > > >>  	case ARM_SPE_OP_TYPE:
> > > >>  		switch (idx) {
> > > >> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> > > >> +		case 0: {
> > > >> +			size_t blen = buf_len;
> > > >> +
> > > >> +			if ((payload & 0x89) == 0x08) {
> > > >> +				ret = snprintf(buf, buf_len, "SVE");
> > > >> +				buf += ret;
> > > >> +				blen -= ret;
> > > > 
> > > > (Nit: can ret be < 0 ?  I've never been 100% clear on this myself for
> > > > the s*printf() family -- if this assumption is widespread in perf tool
> > > > a lready that I guess just go with the flow.)
> > > 
> > > Yeah, some parts of the code in here check for -1, actually, but doing
> > > this on every call to snprintf would push this current code over the
> > > edge - and I cowardly avoided a refactoring ;-)
> > > 
> > > Please note that his is perf userland, and also we are printing constant
> > > strings here.
> > > Although admittedly this starts to sounds like an excuse now ...
> > > 
> > > > I wonder if this snprintf+increment+decrement sequence could be wrapped
> > > > up as a helper, rather than having to be repeated all over the place.
> > > 
> > > Yes, I was hoping nobody would notice ;-)
> > 
> > It's probably not worth losing sleep over.
> > 
> > snprintf(3) says, under NOTES:
> > 
> > 	Until glibc 2.0.6, they would return -1 when the output was
> > 	truncated.
> > 
> > which is probably ancient enough history that we don't care.  C11 does
> > say that a negative return value can happen "if an encoding error
> > occurred".  _Probably_ not a problem if perf tool never calls
> > setlocale(), but ...
> 
> I have one patch which tried to fix the snprintf+increment sequence
> [1], to be honest, the change seems urgly for me.  I agree it's better
> to use a helper to wrap up.
> 
> [1] https://lore.kernel.org/patchwork/patch/1288410/

Sure, putting explicit checks all over the place makes a lot of noise in
the code.

I was wondering whether something along the following lines would work:

	/* ... */

	if (payload & SVE_EVT_PKT_GEN_EXCEPTION)
		buf_appendf_err(&buf, &buf_len, &ret, " EXCEPTION-GEN");
	if (payload & SVE_EVT_PKT_ARCH_RETIRED)
		buf_appendf_err(&buf, &buf_len, &ret, " RETIRED");
	if (payload & SVE_EVT_PKT_L1D_ACCESS)
		buf_appendf_err(&buf, &buf_len, &ret, " L1D-ACCESS");

	/* ... */

	if (ret)
		return ret;

[...]

Best to keep such refactoring independent of this series though.

Cheers
---Dave
Leo Yan Sept. 30, 2020, 11:04 a.m. UTC | #10
Hi Dave,

On Wed, Sep 30, 2020 at 11:34:11AM +0100, Dave Martin wrote:

[...]

> > > > >> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > > > >> index a033f34846a6..f0c369259554 100644
> > > > >> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > > > >> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > > > >> @@ -372,8 +372,35 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> > > > >>  	}
> > > > >>  	case ARM_SPE_OP_TYPE:
> > > > >>  		switch (idx) {
> > > > >> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> > > > >> +		case 0: {
> > > > >> +			size_t blen = buf_len;
> > > > >> +
> > > > >> +			if ((payload & 0x89) == 0x08) {
> > > > >> +				ret = snprintf(buf, buf_len, "SVE");
> > > > >> +				buf += ret;
> > > > >> +				blen -= ret;
> > > > > 
> > > > > (Nit: can ret be < 0 ?  I've never been 100% clear on this myself for
> > > > > the s*printf() family -- if this assumption is widespread in perf tool
> > > > > a lready that I guess just go with the flow.)
> > > > 
> > > > Yeah, some parts of the code in here check for -1, actually, but doing
> > > > this on every call to snprintf would push this current code over the
> > > > edge - and I cowardly avoided a refactoring ;-)
> > > > 
> > > > Please note that his is perf userland, and also we are printing constant
> > > > strings here.
> > > > Although admittedly this starts to sounds like an excuse now ...
> > > > 
> > > > > I wonder if this snprintf+increment+decrement sequence could be wrapped
> > > > > up as a helper, rather than having to be repeated all over the place.
> > > > 
> > > > Yes, I was hoping nobody would notice ;-)
> > > 
> > > It's probably not worth losing sleep over.
> > > 
> > > snprintf(3) says, under NOTES:
> > > 
> > > 	Until glibc 2.0.6, they would return -1 when the output was
> > > 	truncated.
> > > 
> > > which is probably ancient enough history that we don't care.  C11 does
> > > say that a negative return value can happen "if an encoding error
> > > occurred".  _Probably_ not a problem if perf tool never calls
> > > setlocale(), but ...
> > 
> > I have one patch which tried to fix the snprintf+increment sequence
> > [1], to be honest, the change seems urgly for me.  I agree it's better
> > to use a helper to wrap up.
> > 
> > [1] https://lore.kernel.org/patchwork/patch/1288410/
> 
> Sure, putting explicit checks all over the place makes a lot of noise in
> the code.
> 
> I was wondering whether something along the following lines would work:
> 
> 	/* ... */
> 
> 	if (payload & SVE_EVT_PKT_GEN_EXCEPTION)
> 		buf_appendf_err(&buf, &buf_len, &ret, " EXCEPTION-GEN");
> 	if (payload & SVE_EVT_PKT_ARCH_RETIRED)
> 		buf_appendf_err(&buf, &buf_len, &ret, " RETIRED");
> 	if (payload & SVE_EVT_PKT_L1D_ACCESS)
> 		buf_appendf_err(&buf, &buf_len, &ret, " L1D-ACCESS");
> 
> 	/* ... */
> 
> 	if (ret)
> 		return ret;
> 
> [...]

I have sent out the patch v2 [1] and Cc'ed you; I used a similiar API
definition with your suggestion:

  static int arm_spe_pkt_snprintf(char **buf_p, size_t *blen,
 				  const char *fmt, ...)

Only a difference is when return from arm_spe_pkt_snprintf(), will check
the return value and directly bail out when detect failure.  Your input
will be considered for next spin.

> Best to keep such refactoring independent of this series though.

Yeah, the patch set [2] is quite heavy; after get some reviewing,
maybe need to consider to split into 2 or even 3 small patch sets.

Thanks a lot for your suggestions!

Leo

[1] https://lore.kernel.org/patchwork/patch/1314603/
[2] https://lore.kernel.org/patchwork/cover/1314599/
Dave Martin Oct. 5, 2020, 10:15 a.m. UTC | #11
On Wed, Sep 30, 2020 at 07:04:53PM +0800, Leo Yan wrote:

[...]

> > > > > >> diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > > > > >> index a033f34846a6..f0c369259554 100644
> > > > > >> --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > > > > >> +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
> > > > > >> @@ -372,8 +372,35 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> > > > > >>  	}
> > > > > >>  	case ARM_SPE_OP_TYPE:
> > > > > >>  		switch (idx) {
> > > > > >> -		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
> > > > > >> +		case 0: {
> > > > > >> +			size_t blen = buf_len;
> > > > > >> +
> > > > > >> +			if ((payload & 0x89) == 0x08) {
> > > > > >> +				ret = snprintf(buf, buf_len, "SVE");
> > > > > >> +				buf += ret;
> > > > > >> +				blen -= ret;
> > > > > > 
> > > > > > (Nit: can ret be < 0 ?  I've never been 100% clear on this myself for
> > > > > > the s*printf() family -- if this assumption is widespread in perf tool
> > > > > > a lready that I guess just go with the flow.)
> > > > > 
> > > > > Yeah, some parts of the code in here check for -1, actually, but doing
> > > > > this on every call to snprintf would push this current code over the
> > > > > edge - and I cowardly avoided a refactoring ;-)
> > > > > 
> > > > > Please note that his is perf userland, and also we are printing constant
> > > > > strings here.
> > > > > Although admittedly this starts to sounds like an excuse now ...
> > > > > 
> > > > > > I wonder if this snprintf+increment+decrement sequence could be wrapped
> > > > > > up as a helper, rather than having to be repeated all over the place.
> > > > > 
> > > > > Yes, I was hoping nobody would notice ;-)
> > > > 
> > > > It's probably not worth losing sleep over.
> > > > 
> > > > snprintf(3) says, under NOTES:
> > > > 
> > > > 	Until glibc 2.0.6, they would return -1 when the output was
> > > > 	truncated.
> > > > 
> > > > which is probably ancient enough history that we don't care.  C11 does
> > > > say that a negative return value can happen "if an encoding error
> > > > occurred".  _Probably_ not a problem if perf tool never calls
> > > > setlocale(), but ...
> > > 
> > > I have one patch which tried to fix the snprintf+increment sequence
> > > [1], to be honest, the change seems urgly for me.  I agree it's better
> > > to use a helper to wrap up.
> > > 
> > > [1] https://lore.kernel.org/patchwork/patch/1288410/
> > 
> > Sure, putting explicit checks all over the place makes a lot of noise in
> > the code.
> > 
> > I was wondering whether something along the following lines would work:
> > 
> > 	/* ... */
> > 
> > 	if (payload & SVE_EVT_PKT_GEN_EXCEPTION)
> > 		buf_appendf_err(&buf, &buf_len, &ret, " EXCEPTION-GEN");
> > 	if (payload & SVE_EVT_PKT_ARCH_RETIRED)
> > 		buf_appendf_err(&buf, &buf_len, &ret, " RETIRED");
> > 	if (payload & SVE_EVT_PKT_L1D_ACCESS)
> > 		buf_appendf_err(&buf, &buf_len, &ret, " L1D-ACCESS");
> > 
> > 	/* ... */
> > 
> > 	if (ret)
> > 		return ret;
> > 
> > [...]
> 
> I have sent out the patch v2 [1] and Cc'ed you; I used a similiar API
> definition with your suggestion:
> 
>   static int arm_spe_pkt_snprintf(char **buf_p, size_t *blen,
>  				  const char *fmt, ...)
> 
> Only a difference is when return from arm_spe_pkt_snprintf(), will check
> the return value and directly bail out when detect failure.  Your input
> will be considered for next spin.
> 
> > Best to keep such refactoring independent of this series though.
> 
> Yeah, the patch set [2] is quite heavy; after get some reviewing,
> maybe need to consider to split into 2 or even 3 small patch sets.
> 
> Thanks a lot for your suggestions!
>
> Leo

No problem, your approach seems reasonable to me.

Cheers
---Dave

> [1] https://lore.kernel.org/patchwork/patch/1314603/
> [2] https://lore.kernel.org/patchwork/cover/1314599/
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff mbox series

Patch

diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
index a033f34846a6..f0c369259554 100644
--- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
+++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.c
@@ -372,8 +372,35 @@  int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 	}
 	case ARM_SPE_OP_TYPE:
 		switch (idx) {
-		case 0:	return snprintf(buf, buf_len, "%s", payload & 0x1 ?
+		case 0: {
+			size_t blen = buf_len;
+
+			if ((payload & 0x89) == 0x08) {
+				ret = snprintf(buf, buf_len, "SVE");
+				buf += ret;
+				blen -= ret;
+				if (payload & 0x2)
+					ret = snprintf(buf, buf_len, " FP");
+				else
+					ret = snprintf(buf, buf_len, " INT");
+				buf += ret;
+				blen -= ret;
+				if (payload & 0x4) {
+					ret = snprintf(buf, buf_len, " PRED");
+					buf += ret;
+					blen -= ret;
+				}
+				/* Bits [7..4] encode the vector length */
+				ret = snprintf(buf, buf_len, " EVLEN%d",
+					       32 << ((payload >> 4) & 0x7));
+				buf += ret;
+				blen -= ret;
+				return buf_len - blen;
+			}
+
+			return snprintf(buf, buf_len, "%s", payload & 0x1 ?
 					"COND-SELECT" : "INSN-OTHER");
+			}
 		case 1:	{
 			size_t blen = buf_len;
 
@@ -403,6 +430,25 @@  int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
 				ret = snprintf(buf, buf_len, " NV-SYSREG");
 				buf += ret;
 				blen -= ret;
+			} else if ((payload & 0x0a) == 0x08) {
+				ret = snprintf(buf, buf_len, " SVE");
+				buf += ret;
+				blen -= ret;
+				if (payload & 0x4) {
+					ret = snprintf(buf, buf_len, " PRED");
+					buf += ret;
+					blen -= ret;
+				}
+				if (payload & 0x80) {
+					ret = snprintf(buf, buf_len, " SG");
+					buf += ret;
+					blen -= ret;
+				}
+				/* Bits [7..4] encode the vector length */
+				ret = snprintf(buf, buf_len, " EVLEN%d",
+					       32 << ((payload >> 4) & 0x7));
+				buf += ret;
+				blen -= ret;
 			} else if (payload & 0x4) {
 				ret = snprintf(buf, buf_len, " SIMD-FP");
 				buf += ret;