Messages in this thread | | | Date | Fri, 02 May 2014 17:04:08 +0200 | From | Denys Vlasenko <> | Subject | Re: [PATCH] uprobes: fix scratch register selection for rip-relative fixups |
| |
On 05/02/2014 02:48 AM, Jim Keniston wrote: > On Thu, 2014-05-01 at 19:09 +0200, Denys Vlasenko wrote: >> +#define VEX2_VVVV(insn) X86_VEX_V((insn)->vex_prefix.bytes[1]) >> +#define VEX3_VVVV(insn) X86_VEX_V((insn)->vex_prefix.bytes[2]) > > I disclaim any knowledge about VEX* stuff. ... > skipped this next part... > >> + /* Similar treatment for VEX3 prefix */ >> + /* TODO: add XOP/EVEX treatment when insn decoder supports them */ >> + if (insn->vex_prefix.nbytes == 3) { >> + /* >> + * vex2: c5 rvvvvLpp (has no b bit) >> + * vex3/xop: c4/8f rxbmmmmm wvvvvLpp >> + * evex: 62 rxbR00mm.wvvvv1pp.zllBVaaa >> + * (evex will need setting of both b and x since >> + * in non-sib encoding evex.x is 4th bit of MODRM.rm) >> + * Setting VEX3.b (setting because it has inverted meaning): >> + */ >> + cursor = auprobe->insn + insn_offset_vex_prefix(insn) + 1; >> + *cursor |= 0x20; >> } > > ... resumed reviewing here.
I do realize that most people reading this code won't be aficionados of memorizing x86 insn encoding quirks.
To help them, I added the comment which spells out bit layout of vex/evex/xop prefixes (see above). Does it help?
> VEX again. Covering my ears and humming... > >> + if (insn->vex_prefix.nbytes == 2) { >> + reg2 = VEX2_VVVV(insn) ^ 0xf; >> + } >> + if (insn->vex_prefix.nbytes == 3) { >> + reg2 = VEX3_VVVV(insn) ^ 0xf; >> + } >> + /* TODO: add XOP, EXEV vvvv reading */ >> +
In fact there is room for improvement in this part: it's better to ignore most-significant bit of this field (32-bit mode strikes back).
>> + reg2 = 3; /* BX */ >> + if (can_use_regs & UPROBE_FIX_RIP_DI) >> + reg2 = 7; >> + if (can_use_regs & UPROBE_FIX_RIP_SI) >> + reg2 = 6; > > It seems more natural to code this as: > if (can_use_regs & UPROBE_FIX_RIP_SI) > reg2 = 6; > else if (can_use_regs & UPROBE_FIX_RIP_DI) > reg2 = 7; > else > reg2 = 3; /* BX */ > ... which is pretty much how you do it in scratch_reg().
Okay. I can also get rid of "can_use_regs" variable altogether then! Sending new version in a jiffy. -- vda
| |