| Message ID | 1471354850-5549-1-git-send-email-michael@walle.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi,
Your series seems to have some coding style problems. See output below for
more information:
Message-id: 1471354850-5549-1-git-send-email-michael@walle.cc
Subject: [Qemu-devel] [PATCH] linux-user: ppc64: fix ARCH_206 bit in AT_HWCAP
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/1471354850-5549-1-git-send-email-michael@walle.cc -> patchew/1471354850-5549-1-git-send-email-michael@walle.cc
Switched to a new branch 'test'
b0ee12f linux-user: ppc64: fix ARCH_206 bit in AT_HWCAP
=== OUTPUT BEGIN ===
Checking PATCH 1/1: linux-user: ppc64: fix ARCH_206 bit in AT_HWCAP...
ERROR: braces {} are necessary for all arms of this statement
#22: FILE: linux-user/elfload.c:745:
+ do { if ((cpu->env.insns_flags2 & flag) == flag) \
[...]
total: 1 errors, 0 warnings, 9 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
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? > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > checkpatch.pl flags one warning, but I think this is a false positive. Yes, I think so to, but.. > linux-user/elfload.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/linux-user/elfload.c b/linux-user/elfload.c > index f807baf..4945d48 100644 > --- a/linux-user/elfload.c > +++ b/linux-user/elfload.c > @@ -742,7 +742,8 @@ static uint32_t get_elf_hwcap(void) > #define GET_FEATURE(flag, feature) \ > do { if (cpu->env.insns_flags & flag) { features |= feature; } } while (0) > #define GET_FEATURE2(flag, feature) \ > - do { if (cpu->env.insns_flags2 & flag) { features |= feature; } } while (0) > + do { if ((cpu->env.insns_flags2 & flag) == flag) \ > + { features |= feature; } } while (0) ..given that you're splitting this to >1 line, I think you might as well expand it fully into a more normal indent style, which should also shut up the stylebot. > GET_FEATURE(PPC_64B, QEMU_PPC_FEATURE_64); > GET_FEATURE(PPC_FLOAT, QEMU_PPC_FEATURE_HAS_FPU); > GET_FEATURE(PPC_ALTIVEC, QEMU_PPC_FEATURE_HAS_ALTIVEC);
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); Ahh, I've just seen that Tom's mail wasn't CCed to the mailinglist: 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
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 >
diff --git a/linux-user/elfload.c b/linux-user/elfload.c index f807baf..4945d48 100644 --- a/linux-user/elfload.c +++ b/linux-user/elfload.c @@ -742,7 +742,8 @@ static uint32_t get_elf_hwcap(void) #define GET_FEATURE(flag, feature) \ do { if (cpu->env.insns_flags & flag) { features |= feature; } } while (0) #define GET_FEATURE2(flag, feature) \ - do { if (cpu->env.insns_flags2 & flag) { features |= feature; } } while (0) + do { if ((cpu->env.insns_flags2 & flag) == flag) \ + { features |= feature; } } while (0) GET_FEATURE(PPC_64B, QEMU_PPC_FEATURE_64); GET_FEATURE(PPC_FLOAT, QEMU_PPC_FEATURE_HAS_FPU); GET_FEATURE(PPC_ALTIVEC, QEMU_PPC_FEATURE_HAS_ALTIVEC);
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. Signed-off-by: Michael Walle <michael@walle.cc> --- checkpatch.pl flags one warning, but I think this is a false positive. linux-user/elfload.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)