Re: [Qemu-ppc] [PATCH for-4.0 2/3] target/ppc: Enable "decrement and test CTR" version of bcctr

[David Gibson]

On Mon, Mar 25, 2019 at 12:25:15PM +0100, Greg Kurz wrote:
> On Mon, 25 Mar 2019 17:38:49 +1100
> David Gibson <address@hidden> wrote:
> 
> > On Fri, Mar 22, 2019 at 07:03:46PM +0100, Greg Kurz wrote:
> > > Even if all ISAs up to v3 indeed mention:
> > > 
> > >     If the "decrement and test CTR" option is specified (BO2=0), the
> > >     instruction form is invalid.
> > > 
> > > The UMs of all existing 64-bit server class processors say:  
> > 
> > I've applied this series because it fixes an immediate problem, but I
> > have some significant reservations about it, read on..
> > 
> > >     If BO[2] = 0, the contents of CTR (before any update) are used as the
> > >     target address and for the test of the contents of CTR to resolve the
> > >     branch. The contents of the CTR are then decremented and written back
> > >     to the CTR.  
> > 
> > So, if that's what the hardware does, I guess that's what we need to
> > do.  That behaviour seems totally bizarre though - how can it make
> > sense for the same register value to act as both the branch target and
> > a flag/counter?  Or am I misreading something?
> > 
> 
> I must confess this puzzles me as well :-\ ... and the linux commit that
> introduces the use of this opcode, ie. ee13cb249fab ("powerpc/64s: Add
> support for software count cache flush") doesn't provide any details,
> at least for someone ignorant like me :)
> 
> Maybe Michael can enlight us ?

Ok, I talked to Michael on Slack.  Here's my understanding:

- The behaviour as described in the UM is correct, although bizarre
  and basically useless

- I'm guessing the original reason for not making it an explicit
  invalid form is that the strange behaviour arises naturally out of
  orthogonality, and this avoided having an explicit check to trap on
  this form

- Because this instruction form is otherwise useless and never used,
  it was chosen to trigger the extra micro-architectural side-effect
  needed for the Spectre workaround

- The workaround code in the kernel isn't actually *using* the
  bizarre architected behaviour - the arguments and context are chosen
  to make it basically a no-op (w.r.t. architected state).  The kernel
  only cares about the micro-architectural side effect

So, I think the patches as applied are correct as they are.  However,
adding some comments saying how this situation arose would be a very
good idea.

-- 
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

signature.asc
Description: PGP signature