| 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
| ||
|
Date: Mon, 12 Oct 2020 09:00:38 +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 06/14] perf arm-spe: Refactor packet header parsing
On Thu, Oct 08, 2020 at 08:49:11PM +0100, André Przywara wrote:
> On 29/09/2020 14:39, Leo Yan wrote:
>
> Hi Leo,
>
> > The packet header parsing uses the hard coded values and it uses nested
> > if-else statements.
> >
> > To improve the readability, this patch refactors the macros for packet
> > header format so it removes the hard coded values. Furthermore, based
> > on the new mask macros it reduces the nested if-else statements and
> > changes to use the flat conditions checking, this is directive and can
> > easily map to the descriptions in ARMv8-a architecture reference manual
> > (ARM DDI 0487E.a), chapter 'D10.1.5 Statistical Profiling Extension
> > protocol packet headers'.
>
> Yeah, that's so much better, thank you!
Welcome :)
> I checked all the bits and comparisons against the ARM ARM.
>
> Two minor things below ...
>
> >
> > Signed-off-by: Leo Yan <leo.yan@...aro.org>
> > ---
> > .../arm-spe-decoder/arm-spe-pkt-decoder.c | 92 +++++++++----------
> > .../arm-spe-decoder/arm-spe-pkt-decoder.h | 21 +++++
> > 2 files changed, 62 insertions(+), 51 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 96b717a19163..e738bd04f209 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
> > @@ -16,28 +16,6 @@
> > #define NS_FLAG BIT(63)
> > #define EL_FLAG (BIT(62) | BIT(61))
> >
> > -#define SPE_HEADER0_PAD 0x0
> > -#define SPE_HEADER0_END 0x1
> > -#define SPE_HEADER0_ADDRESS 0x30 /* address packet (short) */
> > -#define SPE_HEADER0_ADDRESS_MASK 0x38
> > -#define SPE_HEADER0_COUNTER 0x18 /* counter packet (short) */
> > -#define SPE_HEADER0_COUNTER_MASK 0x38
> > -#define SPE_HEADER0_TIMESTAMP 0x71
> > -#define SPE_HEADER0_TIMESTAMP 0x71
> > -#define SPE_HEADER0_EVENTS 0x2
> > -#define SPE_HEADER0_EVENTS_MASK 0xf
> > -#define SPE_HEADER0_SOURCE 0x3
> > -#define SPE_HEADER0_SOURCE_MASK 0xf
> > -#define SPE_HEADER0_CONTEXT 0x24
> > -#define SPE_HEADER0_CONTEXT_MASK 0x3c
> > -#define SPE_HEADER0_OP_TYPE 0x8
> > -#define SPE_HEADER0_OP_TYPE_MASK 0x3c
> > -#define SPE_HEADER1_ALIGNMENT 0x0
> > -#define SPE_HEADER1_ADDRESS 0xb0 /* address packet (extended) */
> > -#define SPE_HEADER1_ADDRESS_MASK 0xf8
> > -#define SPE_HEADER1_COUNTER 0x98 /* counter packet (extended) */
> > -#define SPE_HEADER1_COUNTER_MASK 0xf8
> > -
> > #if __BYTE_ORDER == __BIG_ENDIAN
> > #define le16_to_cpu bswap_16
> > #define le32_to_cpu bswap_32
> > @@ -198,46 +176,58 @@ static int arm_spe_get_addr(const unsigned char *buf, size_t len,
> > static int arm_spe_do_get_packet(const unsigned char *buf, size_t len,
> > struct arm_spe_pkt *packet)
> > {
> > - unsigned int byte;
> > + unsigned int hdr;
> > + unsigned char ext_hdr = 0;
> >
> > memset(packet, 0, sizeof(struct arm_spe_pkt));
> >
> > if (!len)
> > return ARM_SPE_NEED_MORE_BYTES;
> >
> > - byte = buf[0];
> > - if (byte == SPE_HEADER0_PAD)
> > + hdr = buf[0];
> > +
> > + if (hdr == SPE_HEADER0_PAD)
> > return arm_spe_get_pad(packet);
> > - else if (byte == SPE_HEADER0_END) /* no timestamp at end of record */
> > +
> > + if (hdr == SPE_HEADER0_END) /* no timestamp at end of record */
> > return arm_spe_get_end(packet);
> > - else if (byte & 0xc0 /* 0y11xxxxxx */) {
> > - if (byte & 0x80) {
> > - if ((byte & SPE_HEADER0_ADDRESS_MASK) == SPE_HEADER0_ADDRESS)
> > - return arm_spe_get_addr(buf, len, 0, packet);
> > - if ((byte & SPE_HEADER0_COUNTER_MASK) == SPE_HEADER0_COUNTER)
> > - return arm_spe_get_counter(buf, len, 0, packet);
> > - } else
> > - if (byte == SPE_HEADER0_TIMESTAMP)
> > - return arm_spe_get_timestamp(buf, len, packet);
> > - else if ((byte & SPE_HEADER0_EVENTS_MASK) == SPE_HEADER0_EVENTS)
> > - return arm_spe_get_events(buf, len, packet);
> > - else if ((byte & SPE_HEADER0_SOURCE_MASK) == SPE_HEADER0_SOURCE)
> > - return arm_spe_get_data_source(buf, len, packet);
> > - else if ((byte & SPE_HEADER0_CONTEXT_MASK) == SPE_HEADER0_CONTEXT)
> > - return arm_spe_get_context(buf, len, packet);
> > - else if ((byte & SPE_HEADER0_OP_TYPE_MASK) == SPE_HEADER0_OP_TYPE)
> > - return arm_spe_get_op_type(buf, len, packet);
> > - } else if ((byte & 0xe0) == 0x20 /* 0y001xxxxx */) {
> > - /* 16-bit header */
> > - byte = buf[1];
> > - if (byte == SPE_HEADER1_ALIGNMENT)
> > +
> > + if (hdr == SPE_HEADER0_TIMESTAMP)
> > + return arm_spe_get_timestamp(buf, len, packet);
> > +
> > + if ((hdr & SPE_HEADER0_MASK1) == SPE_HEADER0_EVENTS)
> > + return arm_spe_get_events(buf, len, packet);
> > +
> > + if ((hdr & SPE_HEADER0_MASK1) == SPE_HEADER0_SOURCE)
> > + return arm_spe_get_data_source(buf, len, packet);
> > +
> > + if ((hdr & SPE_HEADER0_MASK2) == SPE_HEADER0_CONTEXT)
> > + return arm_spe_get_context(buf, len, packet);
> > +
> > + if ((hdr & SPE_HEADER0_MASK2) == SPE_HEADER0_OPERATION)
> > + return arm_spe_get_op_type(buf, len, packet);
> > +
> > + if ((hdr & SPE_HEADER0_MASK3) == SPE_HEADER0_EXTENDED) {
>
> Is there any reason you are using MASK3 here, and not MASK2? The ARM ARM
> seems to suggest that bits [7:2] make up the mask for the extended
> header type, as the actual subtype is handled in the next byte.
You are right, here I introduced confusion for MASK3 for bits [7:5];
will change to use MASK2 for the mask of the extended header type.
> > + /* 16-bit extended format header */
> > + ext_hdr = 1;
> > +
> > + hdr = buf[1];
> > + if (hdr == SPE_HEADER1_ALIGNMENT)
> > return arm_spe_get_alignment(buf, len, packet);
> > - else if ((byte & SPE_HEADER1_ADDRESS_MASK) == SPE_HEADER1_ADDRESS)
> > - return arm_spe_get_addr(buf, len, 1, packet);
> > - else if ((byte & SPE_HEADER1_COUNTER_MASK) == SPE_HEADER1_COUNTER)
> > - return arm_spe_get_counter(buf, len, 1, packet);
> > }
> >
> > + /*
> > + * The short format header's byte 0 or the extended format header's
> > + * byte 1 has been assigned to 'hdr', which uses the same encoding for
> > + * address packet and counter packet, so don't need to distinguish if
> > + * it's short format or extended format and handle in once.
> > + */
> > + if ((hdr & SPE_HEADER0_MASK4) == SPE_HEADER0_ADDRESS)
> > + return arm_spe_get_addr(buf, len, ext_hdr, packet);
> > +
> > + if ((hdr & SPE_HEADER0_MASK4) == SPE_HEADER0_COUNTER)
> > + return arm_spe_get_counter(buf, len, ext_hdr, packet);
> > +
> > return ARM_SPE_BAD_PACKET;
> > }
> >
> > diff --git a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
> > index f2d0af39a58c..a30fe3c5ab67 100644
> > --- a/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
> > +++ b/tools/perf/util/arm-spe-decoder/arm-spe-pkt-decoder.h
> > @@ -37,6 +37,27 @@ struct arm_spe_pkt {
> > uint64_t payload;
> > };
> >
> > +/* Short header (HEADER0) and extended header (HEADER1) */
> > +#define SPE_HEADER0_PAD 0x0
> > +#define SPE_HEADER0_END 0x1
> > +#define SPE_HEADER0_TIMESTAMP 0x71
> > +/* Mask for event & data source */
> > +#define SPE_HEADER0_MASK1 (GENMASK_ULL(7, 6) | GENMASK_ULL(3, 0))
> > +#define SPE_HEADER0_EVENTS 0x42
> > +#define SPE_HEADER0_SOURCE 0x43
> > +/* Mask for context & operation */
> > +#define SPE_HEADER0_MASK2 GENMASK_ULL(7, 2)
> > +#define SPE_HEADER0_CONTEXT 0x64
> > +#define SPE_HEADER0_OPERATION 0x48
>
> Just a nit, but should the name be ..._OP_TYPE instead?
Exactly, will apply this in next spin.
Very appreicate for your detailed reviewing.
Thanks,
Leo
Powered by blists - more mailing lists