[RFC,09/11] powerpc/tm: Do not restore default DSCR

In the previous TM code, trecheckpoint was being executed in the middle of
an exception, thus, DSCR was being restored to default kernel DSCR value
after trecheckpoint was done.

With this current patchset, trecheckpoint is executed just before getting
to userspace, at ret_from_except_lite, for example. Thus, we do not need to
set default kernel DSCR value anymore, as we are leaving kernel space.  It
is OK to keep the checkpointed DSCR value into the live SPR, mainly because
the transaction is doomed and it will fail soon (after RFID), so,
continuing with the pre-checkpointed DSCR value is what seems correct.

That said, we must set the DSCR value that will be used in userspace now.
Current trecheckpoint() function sets it to the pre-checkpointed value
prior to lines being changed in this patch, so, removing these lines would
keep the pre-checkpointed values.

Important to say that we do not need to do the same thing with tm_reclaim,
since it already set the DSCR to the default value, after TRECLAIM is
called, in the following lines:

        /* Load CPU's default DSCR */
        ld      r0, PACA_DSCR_DEFAULT(r13)
        mtspr   SPRN_DSCR, r0

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 arch/powerpc/kernel/tm.S | 4 ----
 1 file changed, 4 deletions(-)

On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> In the previous TM code, trecheckpoint was being executed in the middle of
> an exception, thus, DSCR was being restored to default kernel DSCR value
> after trecheckpoint was done.
> 
> With this current patchset, trecheckpoint is executed just before getting
> to userspace, at ret_from_except_lite, for example. Thus, we do not need to
> set default kernel DSCR value anymore, as we are leaving kernel space.  It
> is OK to keep the checkpointed DSCR value into the live SPR, mainly because
> the transaction is doomed and it will fail soon (after RFID), 

What if we are going back to a suspended transaction?  It will remain live until
userspace does a tresume

> so,
> continuing with the pre-checkpointed DSCR value is what seems correct.

Reading this description suggests this patch isn't really needed. Right?

Mikey

> That said, we must set the DSCR value that will be used in userspace now.
> Current trecheckpoint() function sets it to the pre-checkpointed value
> prior to lines being changed in this patch, so, removing these lines would
> keep the pre-checkpointed values.
> 
> Important to say that we do not need to do the same thing with tm_reclaim,
> since it already set the DSCR to the default value, after TRECLAIM is
> called, in the following lines:
> 
>         /* Load CPU's default DSCR */
>         ld      r0, PACA_DSCR_DEFAULT(r13)
>         mtspr   SPRN_DSCR, r0
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  arch/powerpc/kernel/tm.S | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S
> index 6bffbc5affe7..5427eda69846 100644
> --- a/arch/powerpc/kernel/tm.S
> +++ b/arch/powerpc/kernel/tm.S
> @@ -493,10 +493,6 @@ restore_gprs:
>  	mtlr	r0
>  	ld	r2, STK_GOT(r1)
>  
> -	/* Load CPU's default DSCR */
> -	ld	r0, PACA_DSCR_DEFAULT(r13)
> -	mtspr	SPRN_DSCR, r0
> -
>  	blr
>  
>  	/* ****************************************************************** */

Hi Mikey,

On 09/18/2018 02:41 AM, Michael Neuling wrote:
> On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
>> In the previous TM code, trecheckpoint was being executed in the middle of
>> an exception, thus, DSCR was being restored to default kernel DSCR value
>> after trecheckpoint was done.
>>
>> With this current patchset, trecheckpoint is executed just before getting
>> to userspace, at ret_from_except_lite, for example. Thus, we do not need to
>> set default kernel DSCR value anymore, as we are leaving kernel space.  It
>> is OK to keep the checkpointed DSCR value into the live SPR, mainly because
>> the transaction is doomed and it will fail soon (after RFID), 
> 
> What if we are going back to a suspended transaction?  It will remain live until
> userspace does a tresume

Hmm, I understand that once we get in kernel space, and call
treclaim/trecheckpoint, the transaction will be doomed and it will abort and
rollback when we leave kernel space. I.e., if we can treclaim/trecheckpointn
in kernel space, the transaction will *always* abort just after RFID, so, a
possible tresume will never be executed. Is my understanding wrong?

> 
>> so,
>> continuing with the pre-checkpointed DSCR value is what seems correct.
> 
> Reading this description suggests this patch isn't really needed. Right?

Maybe the description is not clear, but I understand this patch is required,
otherwise we will leave userspace with a default DSCR value.

By the way, do you know if there is a change in DSCR inside a transaction,
will it be reverted if the transaction is aborted?

Thank you

On Thu, 2018-09-27 at 17:51 -0300, Breno Leitao wrote:
> Hi Mikey,
> 
> On 09/18/2018 02:41 AM, Michael Neuling wrote:
> > On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> > > In the previous TM code, trecheckpoint was being executed in the middle of
> > > an exception, thus, DSCR was being restored to default kernel DSCR value
> > > after trecheckpoint was done.
> > > 
> > > With this current patchset, trecheckpoint is executed just before getting
> > > to userspace, at ret_from_except_lite, for example. Thus, we do not need
> > > to
> > > set default kernel DSCR value anymore, as we are leaving kernel space.  It
> > > is OK to keep the checkpointed DSCR value into the live SPR, mainly
> > > because
> > > the transaction is doomed and it will fail soon (after RFID), 
> > 
> > What if we are going back to a suspended transaction?  It will remain live
> > until
> > userspace does a tresume
> 
> Hmm, I understand that once we get in kernel space, and call
> treclaim/trecheckpoint, the transaction will be doomed and it will abort and
> rollback when we leave kernel space. I.e., if we can treclaim/trecheckpointn
> in kernel space, the transaction will *always* abort just after RFID, so, a
> possible tresume will never be executed. Is my understanding wrong?

Your understanding is wrong. We don't roll back until MSR[TS] = T.

There are two cases for the RFID.

1) if you RFID back to userspace that is transactional (ie MSR[TS] = T), then it
will immediately rollback as soon as the RFID happens since we are going
directly to T.

2) If you RFID back to userspace that is suspended (ie MSR[TS] = S), then it
won't roll back until userspace does a tresume. It doesn't rollback until we go
MSR[TS] = S -> T).

> > 
> > > so,
> > > continuing with the pre-checkpointed DSCR value is what seems correct.
> > 
> > Reading this description suggests this patch isn't really needed. Right?
> 
> Maybe the description is not clear, but I understand this patch is required,
> otherwise we will leave userspace with a default DSCR value.
> 
> By the way, do you know if there is a change in DSCR inside a transaction,
> will it be reverted if the transaction is aborted?

Yes it will be reverted. We even have a selftest for it in
tools/testing/selftests/powerpc/tm/tm-resched-dscr.c

There are a bunch of checkpointed SPRs. From the arch:
   Checkpointed registers: The set of registers that are
   saved to the “checkpoint area” when a transaction is
   initiated, and restored upon transaction failure, is a
   subset of the architected register state, consisting of
   the General Purpose Registers, Floating-Point Regis-
   ters, Vector Registers, Vector-Scalar Registers, and the
   following Special Registers and fields: CR fields other
   than CR0, XER, LR, CTR, FPSCR, AMR, PPR,
   VRSAVE, VSCR, DSCR, and TAR.

Mikey