| Message ID | 20220131110811.619053-3-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/41] spapr: Force 32bit when resetting a core | expand |
On 31/01/2022 11:07, Cédric Le Goater wrote: > From: Christophe Leroy <christophe.leroy@csgroup.eu> > > After a TLB miss exception, GPRs 0-3 must be restored on rfi. > > This is managed by hreg_store_msr() which is called by do_rfi() > > However, hreg_store_msr() does it if MSR[TGPR] is unset in the > passed MSR value. > > The problem is that do_rfi() is given the content of SRR1 as > the value to be set in MSR, but TGPR bit is not part of SRR1 > and that bit is used for something else and is sometimes set > to 1, leading to hreg_store_msr() not restoring GPRs. > > So, do the same way as for POW bit, force clearing it. > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > Cc: Cedric Le Goater <clg@kaod.org> > Cc: Fabiano Rosas <farosas@linux.ibm.com> > Reviewed-by: Cédric Le Goater <clg@kaod.org> > Message-Id: <20220120103824.239573-1-christophe.leroy@csgroup.eu> > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > target/ppc/excp_helper.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > index bc646c67a0f5..980f62fd7964 100644 > --- a/target/ppc/excp_helper.c > +++ b/target/ppc/excp_helper.c > @@ -1164,6 +1164,10 @@ static void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr) > /* MSR:POW cannot be set by any form of rfi */ > msr &= ~(1ULL << MSR_POW); > > + /* MSR:TGPR cannot be set by any form of rfi */ > + if (env->flags & POWERPC_FLAG_TGPR) > + msr &= ~(1ULL << MSR_TGPR); > + > #if defined(TARGET_PPC64) > /* Switching to 32-bit ? Crop the nip */ > if (!msr_is_64bit(env, msr)) { Have you tried a pre-PR push to Gitlab CI for your pull-ppc-20220130 tag? I'd expect this to fail the check-patch job due to the missing braces around the if() statement. ATB, Mark.
On 1/31/22 13:01, Mark Cave-Ayland wrote: > On 31/01/2022 11:07, Cédric Le Goater wrote: > >> From: Christophe Leroy <christophe.leroy@csgroup.eu> >> >> After a TLB miss exception, GPRs 0-3 must be restored on rfi. >> >> This is managed by hreg_store_msr() which is called by do_rfi() >> >> However, hreg_store_msr() does it if MSR[TGPR] is unset in the >> passed MSR value. >> >> The problem is that do_rfi() is given the content of SRR1 as >> the value to be set in MSR, but TGPR bit is not part of SRR1 >> and that bit is used for something else and is sometimes set >> to 1, leading to hreg_store_msr() not restoring GPRs. >> >> So, do the same way as for POW bit, force clearing it. >> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> Cc: Cedric Le Goater <clg@kaod.org> >> Cc: Fabiano Rosas <farosas@linux.ibm.com> >> Reviewed-by: Cédric Le Goater <clg@kaod.org> >> Message-Id: <20220120103824.239573-1-christophe.leroy@csgroup.eu> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> target/ppc/excp_helper.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c >> index bc646c67a0f5..980f62fd7964 100644 >> --- a/target/ppc/excp_helper.c >> +++ b/target/ppc/excp_helper.c >> @@ -1164,6 +1164,10 @@ static void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr) >> /* MSR:POW cannot be set by any form of rfi */ >> msr &= ~(1ULL << MSR_POW); >> + /* MSR:TGPR cannot be set by any form of rfi */ >> + if (env->flags & POWERPC_FLAG_TGPR) >> + msr &= ~(1ULL << MSR_TGPR); >> + >> #if defined(TARGET_PPC64) >> /* Switching to 32-bit ? Crop the nip */ >> if (!msr_is_64bit(env, msr)) { > > Have you tried a pre-PR push to Gitlab CI for your pull-ppc-20220130 tag? I'd expect this to fail the check-patch job due to the missing braces around the if() statement. All is fine : https://gitlab.com/legoater/qemu/-/pipelines/458888936 I even did a checkpatch before sending and it did not complain :/ Thanks, C.
On 31/01/2022 14:11, Cédric Le Goater wrote: > On 1/31/22 13:01, Mark Cave-Ayland wrote: >> On 31/01/2022 11:07, Cédric Le Goater wrote: >> >>> From: Christophe Leroy <christophe.leroy@csgroup.eu> >>> >>> After a TLB miss exception, GPRs 0-3 must be restored on rfi. >>> >>> This is managed by hreg_store_msr() which is called by do_rfi() >>> >>> However, hreg_store_msr() does it if MSR[TGPR] is unset in the >>> passed MSR value. >>> >>> The problem is that do_rfi() is given the content of SRR1 as >>> the value to be set in MSR, but TGPR bit is not part of SRR1 >>> and that bit is used for something else and is sometimes set >>> to 1, leading to hreg_store_msr() not restoring GPRs. >>> >>> So, do the same way as for POW bit, force clearing it. >>> >>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >>> Cc: Cedric Le Goater <clg@kaod.org> >>> Cc: Fabiano Rosas <farosas@linux.ibm.com> >>> Reviewed-by: Cédric Le Goater <clg@kaod.org> >>> Message-Id: <20220120103824.239573-1-christophe.leroy@csgroup.eu> >>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> --- >>> target/ppc/excp_helper.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c >>> index bc646c67a0f5..980f62fd7964 100644 >>> --- a/target/ppc/excp_helper.c >>> +++ b/target/ppc/excp_helper.c >>> @@ -1164,6 +1164,10 @@ static void do_rfi(CPUPPCState *env, target_ulong nip, >>> target_ulong msr) >>> /* MSR:POW cannot be set by any form of rfi */ >>> msr &= ~(1ULL << MSR_POW); >>> + /* MSR:TGPR cannot be set by any form of rfi */ >>> + if (env->flags & POWERPC_FLAG_TGPR) >>> + msr &= ~(1ULL << MSR_TGPR); >>> + >>> #if defined(TARGET_PPC64) >>> /* Switching to 32-bit ? Crop the nip */ >>> if (!msr_is_64bit(env, msr)) { >> >> Have you tried a pre-PR push to Gitlab CI for your pull-ppc-20220130 tag? I'd >> expect this to fail the check-patch job due to the missing braces around the if() >> statement. > > All is fine : > > https://gitlab.com/legoater/qemu/-/pipelines/458888936 > > I even did a checkpatch before sending and it did not complain :/ This is really odd: it seems checkpatch.pl is broken here. For example, applying that single patch onto git master: $ wget -O- https://patchew.org/QEMU/20220131110811.619053-3-clg@kaod.org/mbox | git am -3 $ ./scripts/checkpatch.pl HEAD~1..HEAD total: 0 errors, 0 warnings, 10 lines checked Commit d9565580c846 (target/ppc: 603: fix restore of GPRs 0-3 on rfi) has no obvious style problems and is ready for submission. Here we see checkpatch.pl has no issues with the output of git diff, but when you run it on the whole file: $ ./scripts/checkpatch.pl -f target/ppc/excp_helper.c ERROR: if this code is redundant consider removing it #904: FILE: target/ppc/excp_helper.c:904: +#if 0 /* TODO */ ERROR: braces {} are necessary for all arms of this statement #1168: FILE: target/ppc/excp_helper.c:1168: + if (env->flags & POWERPC_FLAG_TGPR) [...] total: 2 errors, 0 warnings, 1491 lines checked target/ppc/excp_helper.c has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. ... it shows up. How is it possible for checkpatch.pl to miss things when processing diffs instead of whole files? ATB, Mark.
On Mon, 31 Jan 2022 at 17:00, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > Here we see checkpatch.pl has no issues with the output of git diff, but when you run > it on the whole file: > ... it shows up. How is it possible for checkpatch.pl to miss things when processing > diffs instead of whole files? Probably because checkpatch is a pretty hairy perl script and sometimes it mis-parses stuff, especially when it's working with a diff hunk and it has a limited view of the context around the statement. It's always been best-effort rather than guaranteed to catch all formatting issues. For this particular error, it doesn't seem to me worth making Cédric reroll the pullreq to fix it unless there's some other issue with it; we can fix it with a followup patch. thanks -- PMM
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c >> index bc646c67a0f5..980f62fd7964 100644 >> --- a/target/ppc/excp_helper.c >> +++ b/target/ppc/excp_helper.c >> @@ -1164,6 +1164,10 @@ static void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr) >> /* MSR:POW cannot be set by any form of rfi */ >> msr &= ~(1ULL << MSR_POW); >> + /* MSR:TGPR cannot be set by any form of rfi */ >> + if (env->flags & POWERPC_FLAG_TGPR) >> + msr &= ~(1ULL << MSR_TGPR); >> + >> #if defined(TARGET_PPC64) >> /* Switching to 32-bit ? Crop the nip */ >> if (!msr_is_64bit(env, msr)) { > > Have you tried a pre-PR push to Gitlab CI for your pull-ppc-20220130 tag? I'd expect this to fail the check-patch job due to the missing braces around the if() statement. It seems that ctx_statement_block() is confused because the patch ends with a '{'. Thanks, C.
On 31/01/2022 17:04, Peter Maydell wrote: > On Mon, 31 Jan 2022 at 17:00, Mark Cave-Ayland > <mark.cave-ayland@ilande.co.uk> wrote: >> Here we see checkpatch.pl has no issues with the output of git diff, but when you run >> it on the whole file: >> ... it shows up. How is it possible for checkpatch.pl to miss things when processing >> diffs instead of whole files? > > Probably because checkpatch is a pretty hairy perl script That's a good enough explanation for me ;) > and sometimes > it mis-parses stuff, especially when it's working with a diff hunk > and it has a limited view of the context around the statement. > It's always been best-effort rather than guaranteed to catch all > formatting issues. > > For this particular error, it doesn't seem to me worth making Cédric > reroll the pullreq to fix it unless there's some other issue with > it; we can fix it with a followup patch. Right. I should clarify that I wasn't asking to NAK the PR (there's plenty of good stuff in there), more aiming to document the particular case in the hope someone familiar with Perl could figure out what was happening. ATB, Mark.
On 31/01/2022 17:50, Cédric Le Goater wrote: >>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c >>> index bc646c67a0f5..980f62fd7964 100644 >>> --- a/target/ppc/excp_helper.c >>> +++ b/target/ppc/excp_helper.c >>> @@ -1164,6 +1164,10 @@ static void do_rfi(CPUPPCState *env, target_ulong nip, >>> target_ulong msr) >>> /* MSR:POW cannot be set by any form of rfi */ >>> msr &= ~(1ULL << MSR_POW); >>> + /* MSR:TGPR cannot be set by any form of rfi */ >>> + if (env->flags & POWERPC_FLAG_TGPR) >>> + msr &= ~(1ULL << MSR_TGPR); >>> + >>> #if defined(TARGET_PPC64) >>> /* Switching to 32-bit ? Crop the nip */ >>> if (!msr_is_64bit(env, msr)) { >> >> Have you tried a pre-PR push to Gitlab CI for your pull-ppc-20220130 tag? I'd >> expect this to fail the check-patch job due to the missing braces around the if() >> statement. > > It seems that ctx_statement_block() is confused because > the patch ends with a '{'. Nice work! My experience with Perl is fairly minimal so that would have probably taken me some time to figure out. ATB, Mark.
On 1/31/22 20:08, Mark Cave-Ayland wrote: > On 31/01/2022 17:50, Cédric Le Goater wrote: > >>>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c >>>> index bc646c67a0f5..980f62fd7964 100644 >>>> --- a/target/ppc/excp_helper.c >>>> +++ b/target/ppc/excp_helper.c >>>> @@ -1164,6 +1164,10 @@ static void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr) >>>> /* MSR:POW cannot be set by any form of rfi */ >>>> msr &= ~(1ULL << MSR_POW); >>>> + /* MSR:TGPR cannot be set by any form of rfi */ >>>> + if (env->flags & POWERPC_FLAG_TGPR) >>>> + msr &= ~(1ULL << MSR_TGPR); >>>> + >>>> #if defined(TARGET_PPC64) >>>> /* Switching to 32-bit ? Crop the nip */ >>>> if (!msr_is_64bit(env, msr)) { >>> >>> Have you tried a pre-PR push to Gitlab CI for your pull-ppc-20220130 tag? I'd expect this to fail the check-patch job due to the missing braces around the if() statement. >> >> It seems that ctx_statement_block() is confused because >> the patch ends with a '{'. > > Nice work! My experience with Perl is fairly minimal so that would have probably taken me some time to figure out. Fixing the issue is another story. This part looks like forth to me ! Thanks, C.
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c index bc646c67a0f5..980f62fd7964 100644 --- a/target/ppc/excp_helper.c +++ b/target/ppc/excp_helper.c @@ -1164,6 +1164,10 @@ static void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr) /* MSR:POW cannot be set by any form of rfi */ msr &= ~(1ULL << MSR_POW); + /* MSR:TGPR cannot be set by any form of rfi */ + if (env->flags & POWERPC_FLAG_TGPR) + msr &= ~(1ULL << MSR_TGPR); + #if defined(TARGET_PPC64) /* Switching to 32-bit ? Crop the nip */ if (!msr_is_64bit(env, msr)) {