On Wed, Jul 29, 2020 at 03:21:20PM +0800, liwei (GF) wrote:
[...]
> >> @@ -354,8 +372,38 @@ 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 ?
> >> - "COND-SELECT" : "INSN-OTHER");
> >> + case 0: {
> >> + if (payload & 0x8) {
> >
> > Some nitpicks for packet format checking ...
> >
> > For SVE operation, the payload partten is: 0b0xxx1xx0.
> >
> > So it's good to check the partten like:
> >
> > /* SVE operation subclass is: 0b0xxx1xx0 */
> > if ((payload & 0x8081) == 0x80) {
> > ....
> > }
> >
> > If later the packet format is extended, this will not introduce any
> > confliction.
>
> Get it, but i think what you are really meaning is:
> if ((payload & 0x89) == 0x80) {
> ...
> }
Yes.
> >
> >> + size_t blen = buf_len;
> >> +
> >> + ret = snprintf(buf, buf_len, "SVE-OTHER");
> >> + buf += ret;
> >> + blen -= ret;
> >> + if (payload & 0x2) {
> >
> > Here should express as binary results: " FP" or " INT".
>
> I think this is a style choice, i add these just like the current code where
> processing "AT", "EXCL", "AR", "COND" and so on. So should we modify all the
> corresponding code together?
Okay, understood. Let's just follow the existed style and later can
enhance the output log with more readable format.
[...]
> >
> >> + ret = snprintf(buf, buf_len, " FP");
> >> + buf += ret;
> >> + blen -= ret;
> >> + }
> >> + if (payload & 0x4) {
> >> + ret = snprintf(buf, buf_len, " PRED");
> >
> > Here should express as binary results: " PRED" or " NOT-PRED".
>
> Ditto.
>
> >
> >> + buf += ret;
> >> + blen -= ret;
> >> + }
> >> + if (payload & 0x70) {
> >
> > This is incorrect. If bits[6:4] is zero, it presents vector length is 32
> > bits.
> >
>
> I am a little confused here.
> Refer to the ARM DDI 0487F.b (ID040120), page D10-2830, if bits[6:4] is zero,
> it presents vector length is 32 bits indeed.
Yes, if bits[6:4] is zero, your current code will not output any info.
Thanks,
Leo
> >> + ret = snprintf(buf, buf_len, " EVL %d",
> >> + 32 << ((payload & 0x70) >> 4));
> >> + buf += ret;
> >> + blen -= ret;
> >> + }
> >> + if (ret < 0)
> >> + return ret;
> >> + blen -= ret;
> >> + return buf_len - blen;
> >> + } else {
> >
> > Here we can check with more accurate format as defined in ARMv8 ARM:
> >
> > /* Other operation subclass is: 0b0000000x */
> > if ((payload & 0xfe) == 0x0) {
> > ....
> > }
> >
> >> + return snprintf(buf, buf_len, "%s", payload &
> >> 0x1 ?
> >> + "COND-SELECT" : "INSN-OTHER");
> >> + }
> >> + }
> >> case 1: {
> >> size_t blen = buf_len;
> >>
> >> @@ -385,6 +433,23 @@ int arm_spe_pkt_desc(const struct arm_spe_pkt
> >> *packet, char *buf,
> >> ret = snprintf(buf, buf_len, " SIMD-FP");
> >> buf += ret;
> >> blen -= ret;
> >> + } else if (payload & 0x8) {
> >> + if (payload & 0x4) {
> >> + ret = snprintf(buf, buf_len, " PRED");
> >
> > Here should express as binary results: " PRED" or " NOT-PRED".
>
> Ditto.
>
> >> + buf += ret;
> >> + blen -= ret;
> >> + }
> >> + if (payload & 0x70) {
> >
> > This is incorrect. If bits[6:4] is zero, it presents vector length is 32
> > bits.
>
> Refer to the ARM DDI 0487F.b (ID040120), page D10-2832, if bits[6:4] is zero,
> it presents vector length is 32 bits indeed.
>
> >> + ret = snprintf(buf, buf_len, " EVL %d",
> >> + 32 << ((payload & 0x70) >> 4));
> >> + buf += ret;
> >> + blen -= ret;
> >> + }
> >> + if (payload & 0x80) {
> >> + ret = snprintf(buf, buf_len, " SG");
> >
> > Here should express as binary results: " SG" or " NOT-SG".
>
>
> Thanks,
> Wei
