|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC 1/9] xen: Emulate with no writes; compute current instruction length
>>> On 02.07.14 at 15:33, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> @@ -688,6 +689,17 @@ static int hvmemul_write(
> return X86EMUL_OKAY;
> }
>
> +static int hvmemul_write_dummy(
> + enum x86_segment __attribute__((unused)) seg,
> + unsigned long __attribute__((unused)) offset,
> + void __attribute__((unused)) *p_data,
> + unsigned int __attribute__((unused)) bytes,
> + struct x86_emulate_ctxt __attribute__((unused)) *ctxt)
We don't mark unused function arguments like this (and if we did,
we'd want you to use __maybe_unused).
> @@ -1239,6 +1251,139 @@ int hvm_emulate_one(
> return X86EMUL_OKAY;
> }
>
> +int hvm_emulate_one_no_write(
> + struct hvm_emulate_ctxt *hvmemul_ctxt)
> +{
This must be pretty redundant with hvm_emulate_one(), and hence
most if not all of the redundancy should be factored out.
> +void hvm_emulate_one_full(bool_t nowrite)
> +{
> + struct hvm_emulate_ctxt ctx[1] = {};
> + int rc = X86EMUL_RETRY;
> +
> + hvm_emulate_prepare(ctx, guest_cpu_user_regs());
> +
> + while ( rc == X86EMUL_RETRY )
> + {
> + if ( nowrite )
> + rc = hvm_emulate_one_no_write(ctx);
> + else
> + rc = hvm_emulate_one(ctx);
> + }
> +
> + switch ( rc )
> + {
> + case X86EMUL_UNHANDLEABLE:
> + hvm_inject_hw_exception(TRAP_invalid_op, HVM_DELIVER_NO_ERROR_CODE);
Is it certain that #UD is always the right exception here?
> @@ -1278,6 +1423,53 @@ struct segment_register *hvmemul_get_seg_reg(
> return &hvmemul_ctxt->seg_reg[seg];
> }
>
> +int hvm_get_insn_length(
> + struct hvm_emulate_ctxt *hvmemul_ctxt)
> +{
There again looks to be quite a bit of redundancy here. Please let's
avoid having n copies of (almost) the same code.
> --- /dev/null
> +++ b/xen/arch/x86/inat-tables.c
I'm not going to look at this in much detail, just a couple of general
notes:
- at least some of the information is redundant with the full x86
emulator; as before redundancy should be avoided
- many if not all of the arrays here appear to only be used locally,
and hence ought to be static
- some of the tables are extremely sparse (take
inat_escape_table_3_3[] as an example); would be nice to collapse
those
- making future changes/additions to these tables is going to be
pretty hard with them having neither suitable names nor comments
- coding style (also elsewhere) seems to be Linux'es, yet you don't
mention in the description that they come from Linux (and if they
don't you'd be asked to convert them to Xen style)
> +/**
> + * insn_init() - initialize struct insn
> + * @insn: &struct insn to be initialized
> + * @kaddr: address (in kernel memory) of instruction (or copy thereof)
> + * @x86_64: !0 for 64-bit kernel or 64-bit app
> + */
> +void insn_init(struct insn *insn, const void *kaddr, int x86_64)
> +{
> + memset(insn, 0, sizeof(*insn));
> + insn->kaddr = kaddr;
> + insn->next_byte = kaddr;
> + insn->x86_64 = x86_64 ? 1 : 0;
If the argument was bool_t, you wouldn't need the conditional
operator here.
> + insn->opnd_bytes = 4;
> + if (x86_64)
> + insn->addr_bytes = 8;
> + else
> + insn->addr_bytes = 4;
Yet here the conditional operator would improve readability imo.
> +void insn_get_prefixes(struct insn *insn)
> +{
> + struct insn_field *prefixes = &insn->prefixes;
> + insn_attr_t attr;
> + insn_byte_t b, lb;
> + int i, nb;
> +
> + if (prefixes->got)
> + return;
> +
> + nb = 0;
> + lb = 0;
> + b = peek_next(insn_byte_t, insn);
> + attr = inat_get_opcode_attribute(b);
> + while (inat_is_legacy_prefix(attr)) {
> + /* Skip if same prefix */
> + for (i = 0; i < nb; i++)
> + if (prefixes->bytes[i] == b)
> + goto found;
This discarding of duplicates won't always work correctly with multiple
redundant segment override prefixes.
> + if (nb == 4)
> + /* Invalid instruction */
> + break;
> + prefixes->bytes[nb++] = b;
> + if (inat_is_address_size_prefix(attr)) {
> + /* address size switches 2/4 or 4/8 */
> + if (insn->x86_64)
> + insn->addr_bytes ^= 12;
> + else
> + insn->addr_bytes ^= 6;
> + } else if (inat_is_operand_size_prefix(attr)) {
> + /* oprand size switches 2/4 */
> + insn->opnd_bytes ^= 6;
> + }
Neither the address size nor the operand size prefix work this way:
Redundant prefixes don't undo the address/operand size change.
> +found:
> + prefixes->nbytes++;
> + insn->next_byte++;
> + lb = b;
> + b = peek_next(insn_byte_t, insn);
> + attr = inat_get_opcode_attribute(b);
> + }
> + /* Set the last prefix */
> + if (lb && lb != insn->prefixes.bytes[3]) {
> + if (unlikely(insn->prefixes.bytes[3])) {
> + /* Swap the last prefix */
> + b = insn->prefixes.bytes[3];
> + for (i = 0; i < nb; i++)
> + if (prefixes->bytes[i] == lb)
> + prefixes->bytes[i] = b;
> + }
> + insn->prefixes.bytes[3] = lb;
> + }
> +
> + /* Decode REX prefix */
> + if (insn->x86_64) {
> + b = peek_next(insn_byte_t, insn);
> + attr = inat_get_opcode_attribute(b);
> + if (inat_is_rex_prefix(attr)) {
> + insn->rex_prefix.value = b;
> + insn->rex_prefix.nbytes = 1;
> + insn->next_byte++;
> + if (X86_REX_W(b))
> + /* REX.W overrides opnd_size */
> + insn->opnd_bytes = 8;
> + }
> + }
> + insn->rex_prefix.got = 1;
> +
> + /* Decode VEX prefix */
> + b = peek_next(insn_byte_t, insn);
> + attr = inat_get_opcode_attribute(b);
> + if (inat_is_vex_prefix(attr)) {
This all looks not really correct: A VEX prefix can't follow a REX one,
and legacy prefixes following a REX prefix invalidate the REX one.
> +void insn_get_opcode(struct insn *insn)
> +{
> + struct insn_field *opcode = &insn->opcode;
> + insn_byte_t op;
> + int pfx_id;
> + if (opcode->got)
> + return;
> + if (!insn->prefixes.got)
insn_get_prefixes() already checks this - please settle on whether you
want to avoid the call, or bail early from the function.
> +static int __get_immptr(struct insn *insn)
> +{
> + switch (insn->opnd_bytes) {
> + case 2:
> + insn->immediate1.value = get_next(short, insn);
> + insn->immediate1.nbytes = 2;
> + break;
> + case 4:
> + insn->immediate1.value = get_next(int, insn);
> + insn->immediate1.nbytes = 4;
> + break;
> + case 8:
> + /* ptr16:64 is not exist (no segment) */
> + return 0;
> + default: /* opnd_bytes must be modified manually */
> + goto err_out;
Considering that err_out: is followed by just "return 0", why does case
8 above not "goto err_out" too, or why are "invalid" and "must be
modified manually" being indicated with the same return value? (And
I consider goto-s to a label that is followed by just a single simple
statement bad practice anyway.)
> +void insn_get_length(struct insn *insn)
> +{
> + if (insn->length)
> + return;
> + if (!insn->immediate.got)
> + insn_get_immediate(insn);
> + insn->length = (unsigned char)((unsigned long)insn->next_byte
> + - (unsigned long)insn->kaddr);
> +}
That doesn't seem correct in boundary cases, e.g. a 32-bit segment
wrap at an instruction boundary.
> +/* Legacy last prefixes */
> +#define INAT_PFX_OPNDSZ 1 /* 0x66 */ /* LPFX1 */
> +#define INAT_PFX_REPE 2 /* 0xF3 */ /* LPFX2 */
> +#define INAT_PFX_REPNE 3 /* 0xF2 */ /* LPFX3 */
What does "last" here mean? There's nothing in the architecture
requiring the other possibly prefixes to com first.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |