Re: [Qemu-devel] [PATCH] linux-user: ppc64: fix ARCH_206 bit in AT_HWCAP

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3316 bytes --]

On Tue, Sep 20, 2016 at 08:55:09AM +0200, Michael Walle wrote:
> Am 2016-09-20 04:23, schrieb David Gibson:
> > On Tue, Aug 16, 2016 at 03:40:50PM +0200, Michael Walle wrote:
> > > Only the POWER[789] CPUs should have the ARCH_206 bit set. This is
> > > what the
> > > linux kernel does. I guess this was also the intention of commit
> > > 0e019746.
> > > We have to make sure all *206 bits are set.
> > 
> > Hrm.. it's not clear to me how this patch fixes things.  What was
> > incorrect with the previous logic?
> 
> ah, i guess the patch has too few context. in the previous logic, only one
> bit in "flag" had to be set and the expression would evaluate to true. This
> is correct if there is only one bit set in "flag", but GET_FEATURE2 is also
> used with multiple bits set in "flag":
> 
>     GET_FEATURE2((PPC2_BCTAR_ISA207 | PPC2_LSQ_ISA207 | PPC2_ALTIVEC_207 |
>                   PPC2_ISA207S), QEMU_PPC_FEATURE2_ARCH_2_07);

Ah, right, that makes sense.

> 
> Ahh, I've just seen that Tom's mail wasn't CCed to the mailinglist:

Ok.  But regardless of that, this explanation needs to be in the
commit message for the benefit of people looking back in future.

Please resend with a commit message expanded to include the above
explanation, and I expect I'll commit it.

> 
> 
> Am 2016-08-01 14:17, schrieb Tom Musta:
> > I took a quick look at the qemu and linux code and I think I agree
> > with Michael's analysis.  The HWCAP bit seems to mean that the entire
> > ISA is supported and therefore excludes all of these implementations
> > (like e5500) which picked and chose some aspects of that ISA version.
> > 
> > Hope all is well with you, Alex!
> > 
> > On Mon, Jul 25, 2016 at 10:14 AM, Michael Walle <michael@walle.cc>
> > wrote:
> > 
> > > Hi,
> > > 
> > > ok this was a tough one. Programs which links to ceil() aborts with
> > > invalid instruction. The instruction was "frip", which isn't
> > > supported on e5500 or e500mc. Turns out the libc checks the AT_HWCAP
> > > field (dunno if that has to do with multilib support) and uses the
> > > optimized power5 code if the ARCH_2_06 bit is set. linux-user sets
> > > the ARCH_2_06 bit in case of a e5500 core, which is wrong IMHO. In
> > > fact the linux kernel sets this bit only for POWER[789] CPUs.
> > > 
> > > The line which sets this bit in linux-user is:
> > > 
> > > #define GET_FEATURE2(flag, feature)
> > > \
> > > do { if (cpu->env.insns_flags2 & flag) { features |= feature; }
> > > } while (0)
> > > 
> > > GET_FEATURE2((PPC2_PERM_ISA206 | PPC2_DIVE_ISA206 |
> > > PPC2_ATOMIC_ISA206 |
> > > PPC2_FP_CVT_ISA206 | PPC2_FP_TST_ISA206),
> > > QEMU_PPC_FEATURE_ARCH_2_06);
> > > 
> > > PPC2_PERM_ISA206 is set for the e5500/e500mc core. Is this really
> > > intended to set the ARCH_2_06 bit if _any_ of the listed bits is set
> > > or should it set the ARCH_2_06 bit only if _all_ bits are set. Eg.
> > > 
> > > #define GET_FEATURES(flag, feature)
> > > do { if (cpu->env.insns_flags2 & flag == flag) { features |=
> > > feature; } } while (0)
> 
> -michael
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]