linux-kernel - Re: [PATCH v2 14/14] perf arm-spe: Add support for ARMv8.3-SPE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201021101748.GB3194@leoy-ThinkPad-X240s>
Date:   Wed, 21 Oct 2020 18:17:48 +0800
From:   Leo Yan <leo.yan@...aro.org>
To:     André Przywara <andre.przywara@....com>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Wei Li <liwei391@...wei.com>,
        James Clark <james.clark@....com>,
        Dave Martin <Dave.Martin@....com>,
        linux-kernel@...r.kernel.org, Al Grant <Al.Grant@....com>
Subject: Re: [PATCH v2 14/14] perf arm-spe: Add support for ARMv8.3-SPE

On Wed, Oct 21, 2020 at 10:26:07AM +0100, André Przywara wrote:
> On 21/10/2020 06:10, Leo Yan wrote:
> 
> Hi,
> 
> > On Tue, Oct 20, 2020 at 10:54:44PM +0100, Andrï¿½ Przywara wrote:
> >> On 29/09/2020 14:39, Leo Yan wrote:
> >>
> >> Hi,
> >>
> >>> From: Wei Li <liwei391@...wei.com>
> >>>
> >>> This patch is to support Armv8.3 extension for SPE, it adds alignment
> >>> field in the Events packet and it supports the Scalable Vector Extension
> >>> (SVE) for Operation packet and Events packet with two additions:
> >>>
> >>>   - The vector length for SVE operations in the Operation Type packet;
> >>>   - The incomplete predicate and empty predicate fields in the Events
> >>>     packet.
> >>>
> >>> Signed-off-by: Wei Li <liwei391@...wei.com>
> >>> Signed-off-by: Leo Yan <leo.yan@...aro.org>
> >>> ---
> >>>  .../arm-spe-decoder/arm-spe-pkt-decoder.c     | 84 ++++++++++++++++++-
> >>>  .../arm-spe-decoder/arm-spe-pkt-decoder.h     |  6 ++
> >>>  2 files changed, 87 insertions(+), 3 deletions(-)
> >>>
> >>> 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 05a4c74399d7..3ec381fddfcb 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
> >>> @@ -342,14 +342,73 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt *packet, char *buf,
> >>>  					return ret;
> >>>  			}
> >>>  		}
> >>> +		if (idx > 2) {
> >>
> >> As I mentioned in the other patch, I doubt this extra comparison is
> >> useful. Does that protect us from anything?
> > 
> > It's the same reason with Event packet which have explained for replying
> > patch 10, the condition is to respect the SPE specifiction:
> > 
> >   E[11], byte 1, bit [11], when SZ == 0b10 , or SZ == 0b11
> >      Alignment.
> >      ...
> >      Otherwise this bit reads-as-zero.
> > 
> > So we gives higher priority for checking payload size than the Event
> > bit setting; if you have other thinking for this, please let me know.
> 
> Ah, thanks for pointing this out. It looks like a bug in the manual
> then, because I don't see why bit 11 should be any different from bits
> [10:8] and bits [15:12] in this respect. And in the diagrams above you
> clearly see bit 11 being shown even when SZ == 0b01.
> 
> I will try to follow this up here.

Thanks for following up!

> >>> +			if (payload & SPE_EVT_PKT_ALIGNMENT) {
> >>
> >> Mmh, but this is bit 11, right?
> > 
> > Yes.
> > 
> >> So would need to go into the (idx > 1)
> >> section (covering bits 8-15)? Another reason to ditch this comparison above.
> > 
> > As has explained in patch 10, idx is not the same thing with "sz"
> > field; "idx" stands for payload length in bytes, so:
> > 
> >   idx = 1 << sz
> > 
> > The spec defines the sz is 2 or 3, thus idx is 4 or 8; so this is why
> > here use the condition "(idx > 2)".
> > 
> > I think here need to refine code for more explict expression so can
> > avoid confusion.  So I think it's better to condition such like:
> > 
> >   if (payload_len >= 4) {
> 
> Yes, that would be (or have been) more helpful, but as mentioned in the
> other patch, I'd rather see those comparisons go entirely.

Agree.  Will remove comparisons in next version.

Thanks,
Leo