Re: [Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386

2019-05-15 Thread Jan Bobek
Hi Alex,

I'm very sorry for the late reply, your emails got mixed up with
everything else in qemu-devel; I didn't setup my mail filters very
well (my bad).

On 4/25/19 9:45 AM, Alex Bennée wrote:
> 
> Jan Bobek  writes:
> 
>> Hi all,
>>
> 
>> Thanks,
>> -Jan Bobek
>>
>> P.S. This is my first time using git send-email, so please bear with
>>  me if something goes wrong and/or let me know how I can improve
>>  my future submissions. Thank you!
> 
> Looks good to me. Excellent first patch series ;-)

Thank you :) And thanks a lot for the review!

> I assume you'll post a v2 once you've integrated Richard's fixups to the
> appropriate patches.

Sounds reasonable, I'll get that done.

> Have you had a chance to look at getting a .risu and risugen working yet?

That's early work-in-progress as we speak (also, I'm picking up
Perl...).

Thanks again,
-Jan



signature.asc
Description: OpenPGP digital signature


Re: [Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386

2019-04-25 Thread Alex Bennée


Jan Bobek  writes:

> Hi all,
>

> Thanks,
> -Jan Bobek
>
> P.S. This is my first time using git send-email, so please bear with
>  me if something goes wrong and/or let me know how I can improve
>  my future submissions. Thank you!

Looks good to me. Excellent first patch series ;-)

I assume you'll post a v2 once you've integrated Richard's fixups to the
appropriate patches.

Have you had a chance to look at getting a .risu and risugen working yet?

--
Alex Bennée



Re: [Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386

2019-04-11 Thread Jan Bobek
Sorry for the delayed reply, the U.S. tax deadline has caught up with
me, so I spent the last two evenings doing my taxes. (Yuck!)

Anyway...

On 4/8/19 6:18 PM, Richard Henderson wrote:
> On 4/8/19 8:27 AM, Jan Bobek wrote:
>> 2. Note the '-std=c99' switch in the command-line above; without it,
>>GCC defines the symbol 'i386' to 1 and the preprocessor magic for
>>including arch-specific headers in risu.h breaks. Does anyone have
>>an idea how to fix this in a more robust way?
> 
> Adding -U$(ARCH) to the command line is probably as good a fix as any.

I didn't know about -U, nice!

>> 3. gas (the GNU assembler) chokes on the syntax of test_i386.s; that's
>>why I'm using nasm as the assembler above. Is that intentional? I
>>haven't found the nasm dependency mentioned anywhere.
> 
> I think rewriting to not require nasm is better.

Agreed.

>>Also, nasm will happily emit the UD1 opcode (0F B9) with no
>>operands (see test_i386.s). That's a bit surprising to me, since
>>Intel's Software Developer's Manual says UD1 has two operands; I'd
>>expect at least a follow-up ModR/M byte. gas refuses to assemble
>>UD1 with no operands, and gdb's disassembler gets confused when I
>>load up the nasm's binary into risu. Is there something obvious
>>that I'm missing?
> 
> You are not missing anything -- ud1 should require a modrm byte.
> 
> My suggestion is to use only UD1 as the "break" insn, with the different OP_*
> codes encoded into the modrm byte.

I had to laugh when I read this; this is *exactly* what I had in mind,
but then I found out there was no ModR/M byte.

>> P.S. This is my first time using git send-email, so please bear with
>>  me if something goes wrong and/or let me know how I can improve
>>  my future submissions. Thank you!
> 
> You've done well with git send-email.  ;-)

Thanks a lot! :)

-Jan



signature.asc
Description: OpenPGP digital signature


Re: [Qemu-devel] [RISU PATCH 0/5] Fix RISU build for i386

2019-04-08 Thread Richard Henderson
On 4/8/19 8:27 AM, Jan Bobek wrote:
> 1. Most of it is just moving stuff around, however I've implemented
>reginfo_dump_mismatch (based on reginfo_dump and code in other
>architectures) and defined EAX as the param register. There is no
>support for more registers yet, that will need to be added later.

It's probably worthwhile to get x86_64 working at the same time.
This isn't too difficult -- see below.

> 2. Note the '-std=c99' switch in the command-line above; without it,
>GCC defines the symbol 'i386' to 1 and the preprocessor magic for
>including arch-specific headers in risu.h breaks. Does anyone have
>an idea how to fix this in a more robust way?

Adding -U$(ARCH) to the command line is probably as good a fix as any.

> 3. gas (the GNU assembler) chokes on the syntax of test_i386.s; that's
>why I'm using nasm as the assembler above. Is that intentional? I
>haven't found the nasm dependency mentioned anywhere.

I think rewriting to not require nasm is better.

I'm not sure why it was done this way.  Perhaps Peter's unfamiliarity with x86
assembler, combined with the fact that the Intel manual is not in AT syntax?

In any case...

>Also, nasm will happily emit the UD1 opcode (0F B9) with no
>operands (see test_i386.s). That's a bit surprising to me, since
>Intel's Software Developer's Manual says UD1 has two operands; I'd
>expect at least a follow-up ModR/M byte. gas refuses to assemble
>UD1 with no operands, and gdb's disassembler gets confused when I
>load up the nasm's binary into risu. Is there something obvious
>that I'm missing?

You are not missing anything -- ud1 should require a modrm byte.

My suggestion is to use only UD1 as the "break" insn, with the different OP_*
codes encoded into the modrm byte.  E.g.

void advance_pc(void *vuc)
{
ucontext_t *uc = (ucontext_t *) vuc;

/*
 * We assume that this is UD1 as per get_risuop below.
 * This would need tweaking if we want to test expected undefs.
 */
uc->uc_mcontext.gregs[REG_E(IP)] += 3;
}

int get_risuop(struct reginfo *ri)
{
if ((ri->faulting_insn & 0xf8) == 0xc0b90f) { /* UD1 %xxx,%eax */
return (ri->faulting_insn >> 16) & 7;
}
return -1;
}

This leads to:

  IENUMINSN
  0OP_COMPARE  ud1 %eax,%eax
  1OP_TESTEND  ud1 %ecx,%eax
  2OP_SETMEMBLOCK  ud1 %edx,%eax
  3OP_GETMEMBLOCK  ud1 %ebx,%eax
  4OP_COMPAREMEM   ud1 %esp,%eax


> P.S. This is my first time using git send-email, so please bear with
>  me if something goes wrong and/or let me know how I can improve
>  my future submissions. Thank you!

You've done well with git send-email.  ;-)

The following set of changes Works For Me for i386 and x86_64.  For clarity(?),
I've included diff from master and diff on top of your patch set.


r~
diff --git a/Makefile b/Makefile
index 4aad448..6ab014a 100644
--- a/Makefile
+++ b/Makefile
@@ -17,7 +17,7 @@ VPATH=$(SRCDIR)
 
 CFLAGS ?= -g
 
-ALL_CFLAGS = -Wall -D_GNU_SOURCE -DARCH=$(ARCH) $(BUILD_INC) $(CFLAGS) 
$(EXTRA_CFLAGS)
+ALL_CFLAGS = -Wall -D_GNU_SOURCE -DARCH=$(ARCH) -U$(ARCH) $(BUILD_INC) 
$(CFLAGS) $(EXTRA_CFLAGS)
 
 PROG=risu
 SRCS=risu.c comms.c reginfo.c risu_$(ARCH).c risu_reginfo_$(ARCH).c
@@ -49,6 +49,9 @@ $(PROG): $(OBJS)
 %_$(ARCH).elf: %_$(ARCH).s
$(AS) -o $@ $<
 
+%_$(ARCH).elf: %_$(ARCH).S
+   $(CC) $(CPPFLAGS) -o $@ -c $<
+
 clean:
rm -f $(PROG) $(OBJS) $(BINS)
 
diff --git a/risu_reginfo_i386.h b/risu_reginfo_i386.h
index 4ad90e1..755283a 100644
--- a/risu_reginfo_i386.h
+++ b/risu_reginfo_i386.h
@@ -12,7 +12,8 @@
 #ifndef RISU_REGINFO_I386_H
 #define RISU_REGINFO_I386_H
 
-/* This is the data structure we pass over the socket.
+/*
+ * This is the data structure we pass over the socket.
  * It is a simplified and reduced subset of what can
  * be obtained with a ucontext_t*
  */
@@ -21,18 +22,14 @@ struct reginfo {
 gregset_t gregs;
 };
 
-#ifndef REG_GS
-/* Assume that either we get all these defines or none */
-#   define REG_GS  0
-#   define REG_FS  1
-#   define REG_ES  2
-#   define REG_DS  3
-#   define REG_ESP 7
-#   define REG_EAX11
-#   define REG_TRAPNO 12
-#   define REG_EIP14
-#   define REG_EFL16
-#   define REG_UESP   17
-#endif /* !defined(REG_GS) */
+/*
+ * For i386, the defines are named REG_EAX, etc.
+ * For x86_64, the defines are name REG_RAX, etc.
+ */
+#ifdef __x86_64__
+# define REG_E(X)   REG_R##X
+#else
+# define REG_E(X)   REG_E##X
+#endif
 
 #endif /* RISU_REGINFO_I386_H */
diff --git a/risu_i386.c b/risu_i386.c
index 14b0db3..9962b8f 100644
--- a/risu_i386.c
+++ b/risu_i386.c
@@ -20,37 +20,33 @@ void advance_pc(void *vuc)
 {
 ucontext_t *uc = (ucontext_t *) vuc;
 
-/* We assume that this is either UD1 or UD2.
- * This would need tweaking if we want to test
- * expected undefs on x86.
+/*
+ * We assume that this is UD1 as per get_risuop below.
+ * This would