Re: [Qemu-devel] [PATCH v1 5/6] target/s390x: add basic MSA features

[David Hildenbrand]

>> @@ -792,6 +792,7 @@ static void add_qemu_cpu_model_features(S390FeatBitmap 
>> fbm)
>>          S390_FEAT_STFLE,
>>          S390_FEAT_EXTENDED_IMMEDIATE,
>>          S390_FEAT_EXTENDED_TRANSLATION_2,
>> +        S390_FEAT_MSA,
>>          S390_FEAT_EXTENDED_TRANSLATION_3,
>>          S390_FEAT_LONG_DISPLACEMENT,
>>          S390_FEAT_LONG_DISPLACEMENT_FAST,
>> @@ -808,6 +809,9 @@ static void add_qemu_cpu_model_features(S390FeatBitmap 
>> fbm)
>>          S390_FEAT_STFLE_49,
>>          S390_FEAT_LOCAL_TLB_CLEARING,
>>          S390_FEAT_STFLE_53,
>> +        S390_FEAT_MSA_EXT_5,
>> +        S390_FEAT_MSA_EXT_3,
>> +        S390_FEAT_MSA_EXT_4,
> 
> I first thought that this looks weird, but it is the actual sequence of
> the facility bits (probably the bit for MSA_EXT_5 has been reused?)

Probably, or they had it reserved for something else. Another reason
could be the non-hypervisor managed vs. hypervisor managed stuff (didn't
check the facility bit numbers, but this could be a reason).

[...]
> 
> A comment which subfunction this is might be helpful.

Indeed.

/* query subfunction */

> 
>> +        for (i = 0; i < 16; i++) {
>> +            param_addr = wrap_address(env, env->regs[1] + i);
> 
> This does not compile for me (after massaging the Makefile above), as
> wrap_address does not seem to be exported... can you do a respin,
> please?
> 

Contained in patch nr4 in this series. But I'm planning to do a respin
(most likely introducing internal.h).

>> +            cpu_stb_data_ra(env, param_addr, subfunc[i], ra);
>> +        }
>> +        break;
>> +    default:
>> +        /* we don't implement any other subfunction yet */
>> +        g_assert_not_reached();
>> +    }
>> +
>> +    return 0;
>> +}

Thanks!

-- 

Thanks,

David