Re: [Qemu-devel] [PATCH v1] linux-user: elf: ELF_HWCAP for s390x

[David Hildenbrand <david@redhat.com>]

On 04.06.19 15:00, Richard Henderson wrote:
> On 6/4/19 4:36 AM, David Hildenbrand wrote:
>> +    if (s390_has_feat(S390_FEAT_ESAN3)) {
>> +        hwcap |= HWCAP_S390_ESAN3;
>> +    }
>> +    if (s390_has_feat(S390_FEAT_ZARCH)) {
>> +        hwcap |= HWCAP_S390_ZARCH;
>> +    }
> 
> While it's nice and symetrical testing these two features, I don't think they
> can ever be false.
> 
>> +    if (s390_has_feat(S390_FEAT_STFLE)) {
>> +        hwcap |= HWCAP_S390_STFLE;
>> +    }
>> +    if (s390_has_feat(S390_FEAT_MSA)) {
>> +        hwcap |= HWCAP_S390_MSA;
>> +    }
>> +    if (s390_has_feat(S390_FEAT_LONG_DISPLACEMENT)) {
>> +        hwcap |= HWCAP_S390_LDISP;
>> +    }
>> +    if (s390_has_feat(S390_FEAT_EXTENDED_IMMEDIATE)) {
>> +        hwcap |= HWCAP_S390_EIMM;
>> +    }
>> +    if (s390_has_feat(S390_FEAT_EXTENDED_TRANSLATION_3) &&
>> +        s390_has_feat(S390_FEAT_ETF3_ENH)) {
>> +        hwcap |= HWCAP_S390_ETF3EH;
>> +    }
>> +    /* 31-bit processes can use 64-bit registers */
>> +    hwcap |= HWCAP_S390_HIGH_GPRS;
> 
> And certainly this could never be set unless ZARCH, otherwise you have no
> 64-bit registers.  ;-)
> 
> So maybe clearer to just start with
> 
>   hwcap = HWCAP_S390_ESAN3 | HWCAP_S390_ZARCH | HWCAP_S390_HIGH_GPRS;

Makes sense, and fits into a single line ;)

Thanks!

> 
> and continue from there.
> 
> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> 
> r~
> 


-- 

Thanks,

David / dhildenb