Add support for signed div operations

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
iovisor · Aug 11, 2023 · a3670ec · a3670ec
1 parent f5eb301
commit a3670ec
Show file tree

Hide file tree

Showing 11 changed files with 111 additions and 10 deletions.
diff --git a/tests/sdiv32-by-zero-imm.data b/tests/sdiv32-by-zero-imm.data
@@ -0,0 +1,6 @@
+-- asm
+mov32 %r0, 1
+sdiv32 %r0, 0
+exit
+-- result
+0x0
diff --git a/tests/sdiv32-by-zero-reg.data b/tests/sdiv32-by-zero-reg.data
@@ -0,0 +1,7 @@
+-- asm
+mov32 %r0, 1
+mov32 %r1, 0
+sdiv32 %r0, %r1
+exit
+-- result
+0x0
diff --git a/tests/sdiv32-high-divisor.data b/tests/sdiv32-high-divisor.data
@@ -0,0 +1,7 @@
+-- asm
+mov %r0, 12
+lddw %r1, 0xfffffffff
+sdiv32 %r0, %r1
+exit
+-- result
+0xfffffff4
diff --git a/tests/sdiv32-imm.data b/tests/sdiv32-imm.data
@@ -0,0 +1,6 @@
+-- asm
+lddw %r0, 0x1000000c
+sdiv32 %r0, 0xffffffff
+exit
+-- result
+0xeffffff4
diff --git a/tests/sdiv32-reg.data b/tests/sdiv32-reg.data
@@ -0,0 +1,7 @@
+-- asm
+lddw %r0, 0x1000000c
+mov32 %r1, 0xffffffff
+sdiv32 %r0, %r1
+exit
+-- result
+0xeffffff4
diff --git a/tests/sdiv64-by-zero-imm.data b/tests/sdiv64-by-zero-imm.data
@@ -0,0 +1,6 @@
+-- asm
+mov32 %r0, 1
+sdiv %r0, 0
+exit
+-- result
+0x0
diff --git a/tests/sdiv64-by-zero-reg.data b/tests/sdiv64-by-zero-reg.data
@@ -0,0 +1,7 @@
+-- asm
+mov32 %r0, 1
+mov32 %r1, 0
+sdiv %r0, %r1
+exit
+-- result
+0x0
diff --git a/tests/sdiv64-negative-imm.data b/tests/sdiv64-negative-imm.data
@@ -0,0 +1,6 @@
+-- asm
+lddw %r0, 0xFFFFFFFFFFFFFF0F
+sdiv %r0, -5
+exit
+-- result
+0x30
diff --git a/tests/sdiv64-negative-reg.data b/tests/sdiv64-negative-reg.data
@@ -0,0 +1,7 @@
+-- asm
+lddw %r0, 0xFFFFFFFFFFFFFF0F
+mov %r1, 0xFFFFFFFFFFFFFFFF
+sdiv %r0, %r1
+exit
+-- result
+0xf1
diff --git a/vm/ubpf_jit_x86_64.c b/vm/ubpf_jit_x86_64.c
@@ -37,7 +37,7 @@
 #endif
 
 static void
-muldivmod(struct jit_state* state, uint8_t opcode, int src, int dst, int32_t imm);
+muldivmod(struct jit_state* state, uint8_t opcode, uint8_t offset, int src, int dst, int32_t imm);
 
 #define REGISTER_MAP_SIZE 11
 
@@ -285,7 +285,7 @@ translate(struct ubpf_vm* vm, struct jit_state* state, char** errmsg)
         case EBPF_OP_DIV_REG:
         case EBPF_OP_MOD_IMM:
         case EBPF_OP_MOD_REG:
-            muldivmod(state, inst.opcode, src, dst, inst.imm);
+            muldivmod(state, inst.opcode, inst.offset, src, dst, inst.imm);
             break;
         case EBPF_OP_OR_IMM:
             emit_alu32_imm32(state, 0x81, 1, dst, inst.imm);
@@ -372,7 +372,7 @@ translate(struct ubpf_vm* vm, struct jit_state* state, char** errmsg)
         case EBPF_OP_DIV64_REG:
         case EBPF_OP_MOD64_IMM:
         case EBPF_OP_MOD64_REG:
-            muldivmod(state, inst.opcode, src, dst, inst.imm);
+            muldivmod(state, inst.opcode, inst.offset, src, dst, inst.imm);
             break;
         case EBPF_OP_OR64_IMM:
             emit_alu64_imm32(state, 0x81, 1, dst, inst.imm);
@@ -705,13 +705,14 @@ translate(struct ubpf_vm* vm, struct jit_state* state, char** errmsg)
 }
 
 static void
-muldivmod(struct jit_state* state, uint8_t opcode, int src, int dst, int32_t imm)
+muldivmod(struct jit_state* state, uint8_t opcode, uint8_t offset, int src, int dst, int32_t imm)
 {
     bool mul = (opcode & EBPF_ALU_OP_MASK) == (EBPF_OP_MUL_IMM & EBPF_ALU_OP_MASK);
     bool div = (opcode & EBPF_ALU_OP_MASK) == (EBPF_OP_DIV_IMM & EBPF_ALU_OP_MASK);
     bool mod = (opcode & EBPF_ALU_OP_MASK) == (EBPF_OP_MOD_IMM & EBPF_ALU_OP_MASK);
     bool is64 = (opcode & EBPF_CLS_MASK) == EBPF_CLS_ALU64;
     bool reg = (opcode & EBPF_SRC_REG) == EBPF_SRC_REG;
+    bool isSigned = offset != 0;
 
     // Short circuit for imm == 0.
     if (!reg && imm == 0) {
@@ -777,12 +778,24 @@ muldivmod(struct jit_state* state, uint8_t opcode, int src, int dst, int32_t imm
         emit_alu32(state, 0x31, RDX, RDX);
     }
 
+    // If we are doing a signed operation (of either 64 or 32 bit width),
+    // we are responsible for appropriately setting _x_dx because _x_dx:_x_cx
+    // is the divisor. For an unsigned operation, we know that _x_dx is 0, but
+    // we need to do better when it is a signed operation.
+    // See, e.g., https://www.felixcloutier.com/x86/div.
+    if (isSigned) {
+        if (is64) {
+            emit_rex(state, 1, 0, 0, 0);
+        }
+        emit1(state, 0x99);
+    }
+
     if (is64) {
         emit_rex(state, 1, 0, 0, 0);
     }
 
-    // Multiply or divide.
-    emit_alu32(state, 0xf7, mul ? 4 : 6, RCX);
+    // (signed) multiply or divide.
+    emit_alu32(state, 0xf7, isSigned + (mul ? 4 : 6), RCX);
 
     // Division operation stores the remainder in RDX and the quotient in RAX.
     if (div || mod) {

diff --git a/vm/ubpf_vm.c b/vm/ubpf_vm.c
@@ -30,6 +30,7 @@
 #include <endian.h>
 #include "ubpf_int.h"
 #include <unistd.h>
+#include <stdint.h>
 
 #define MAX_EXT_FUNCS 64
 #define SHIFT_MASK_32_BIT(X) ((X)&0x1f)
@@ -230,6 +231,18 @@ i32(uint64_t x)
     return x;
 }
 
+static uint64_t
+u64(uint64_t x)
+{
+    return x;
+}
+
+static int64_t
+i64(uint64_t x)
+{
+    return x;
+}
+
 #define IS_ALIGNED(x, a) (((uintptr_t)(x) & ((a)-1)) == 0)
 
 inline static uint64_t
@@ -364,11 +377,19 @@ ubpf_exec(const struct ubpf_vm* vm, void* mem, size_t mem_len, uint64_t* bpf_ret
             reg[inst.dst] &= UINT32_MAX;
             break;
         case EBPF_OP_DIV_IMM:
-            reg[inst.dst] = u32(inst.imm) ? u32(reg[inst.dst]) / u32(inst.imm) : 0;
+            if (inst.offset != 0) {
+                reg[inst.dst] = i32(inst.imm) ? i32(reg[inst.dst]) / i32(inst.imm) : 0;
+            } else {
+                reg[inst.dst] = u32(inst.imm) ? u32(reg[inst.dst]) / u32(inst.imm) : 0;
+            }
             reg[inst.dst] &= UINT32_MAX;
             break;
         case EBPF_OP_DIV_REG:
-            reg[inst.dst] = reg[inst.src] ? u32(reg[inst.dst]) / u32(reg[inst.src]) : 0;
+            if (inst.offset != 0) {
+                reg[inst.dst] = reg[inst.src] ? i32(reg[inst.dst]) / i32(reg[inst.src]) : 0;
+            } else {
+                reg[inst.dst] = reg[inst.src] ? u32(reg[inst.dst]) / u32(reg[inst.src]) : 0;
+            }
             reg[inst.dst] &= UINT32_MAX;
             break;
         case EBPF_OP_OR_IMM:
@@ -475,10 +496,18 @@ ubpf_exec(const struct ubpf_vm* vm, void* mem, size_t mem_len, uint64_t* bpf_ret
             reg[inst.dst] *= reg[inst.src];
             break;
         case EBPF_OP_DIV64_IMM:
-            reg[inst.dst] = inst.imm ? reg[inst.dst] / inst.imm : 0;
+            if (inst.offset != 0) {
+                reg[inst.dst] = inst.imm ? i64(reg[inst.dst]) / i64(inst.imm) : 0;
+            } else {
+                reg[inst.dst] = inst.imm ? u64(reg[inst.dst]) / u64(inst.imm) : 0;
+            }
             break;
         case EBPF_OP_DIV64_REG:
-            reg[inst.dst] = reg[inst.src] ? reg[inst.dst] / reg[inst.src] : 0;
+            if (inst.offset != 0) {
+                reg[inst.dst] = reg[inst.src] ? i64(reg[inst.dst]) / i64(reg[inst.src]) : 0;
+            } else {
+                reg[inst.dst] = reg[inst.src] ? u64(reg[inst.dst]) / u64(reg[inst.src]) : 0;
+            }
             break;
         case EBPF_OP_OR64_IMM:
             reg[inst.dst] |= inst.imm;