Bug 434840

Created attachment 136990 [details]
patch to add darn instruction support

The attached patch to add support for the darn instruction uses a clean helper to add the support.  The clean helper uses an inline assembly statement to call the
darn (Deliver a Random Number) instruction.  This is not the normal or preferred way to support an instruction.  AMD 64 used a dirty helper to support its random number generator.  I looked at C code random number generators but did not a good solution.  

Julian, please let me know if you are ok with this non-standard implementation for supporting the random number generation instruction.

(In reply to Carl Love from comment #1)

Handing it off to the hardware via a helper function is the right
thing to do.  But you can't use a clean helper for this; that breaks
the rules for clean helpers.  A clean helper

* must produce a value which depends only on its arguments
* given the same argument, always produces the same result.

and the IR optimiser relies (or may rely) on the above being true.
You need to use a dirty helper.

Also I'm not happy about the replacement C implementations; I'm not
sure when they would ever get used.  I'd prefer if they were removed.

Argh, I inadvertently added the alternate C code solution.  That was for playing.  I thought I had removed that.  

I will change to a dirty helper and get rid of the C code solution.  Thanks

Created attachment 136999 [details]
patch to add darn instruction support

Updated the patch.
 -  Removed the C code random number generator code.  Was not supposed to be included in previous patch.

  -  Reworked the helper to make it a dirty helper that returns the random value.

Patch tested on Power 8, Power 9 and on ISA 3.1 prototype hardware.
  
Please let me know if the updated patch is good to go.  Thanks.

(In reply to Carl Love from comment #4)
> Created attachment 136999 [details]
> patch to add darn instruction support

+ULong darn_dirty_helper ( UInt L )
+{
+
...
+# else
+   val = 0xFFFFFFFFFFFFFFFFULL;  /* error */
+# endif

Given that you initialise `val` to the error value outside of any ifdef,
this #else clause is redundant.  You could remove it.


+static Int dis_darn ( UInt prefix, UInt theInstr,
+                      const VexAbiInfo* vbi )
..
+   if (L == 3)
+      /*  Hardware reports illegal instruction if L = 3.  */
+      vpanic("ERROR: darn instruction L=3 is not valid\n");

Generate SIGILL for an unrecognised instruction, don't assert.
Please always do this -- I've asked multiple times before.

Created attachment 137204 [details]
patch to add darn instruction support

Updated patch

Fixed:

  Return false on L=3 to generate SIGILL on illegal instruction.

  Removed #else clause in dirty helper as the return value, val, is already set   
  to the error value.

(In reply to Carl Love from comment #6)
> Created attachment 137204 [details]
> patch to add darn instruction support

+static Int dis_darn ( UInt prefix, UInt theInstr,
+

+   IRExpr** args = mkIRExprVec_1( mkU32( L ) );
+
+   d = unsafeIRDirty_1_N (
+      rD,
+      0/*regparms*/,
+      "darn_dirty_helper",
+      fnptr_to_fnentry( vbi, &darn_dirty_helper ),
+      args );

This wants a comment saying that there are no effects on memory or guest
state, since it is unusual for a dirty helper to have no such effects.

Otherwise looks fine to me.

Created attachment 138144 [details]
patch to add darn instruction support

Updated the patch.  Added a comment to thedis_darn() function in guest_ppc_toIR.c and darn_dirty_helper in guest_ppc_helpers stating the dirty helper does not change the guest state or guest memory.  It simply returns the value from running the darn instruction on the host.

Patch committed.

commit 8afb49abe04a341d60b441c1f09a956aeccf0bbb (HEAD -> master, origin/master, origin/HEAD)
Author: Carl Love <cel@us.ibm.com>
Date:   Mon Mar 22 17:55:05 2021 -0500

    PPC64: Add support for the darn instruction

Closing issue