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 <liwei...@huawei.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 <liwei...@huawei.com>
> >>> Signed-off-by: Leo Yan <leo....@linaro.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
