Hi Leo,
On 2020/7/29 15:28, Leo Yan wrote:
> 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.
>
Yes, thanks for spotting this.
And thanks for you review.
Thanks,
Wei
