Reducing the Number of Context-Sensitive Macros in x86_emulate.c

Reducing the Number of Context-Sensitive Macros in x86_emulate.c

[John McDermott]

Xen Developers,

Would there be any interest in replacing some of the
context-sensitive macros in x86_emulate.c, to make it more
maintainable?

I work on the Xenon project, which researches ways to transform
Xen into a higher-security hypervisor. One of the things we do is
modify Xen code to make it more maintainable. As part of that
work, we have some experience in improving the maintainability of
x86_emulate.c.

The design of the x86_emulate function depends on labels in such
a way that it is probably not practical to remove _all_
context-sensitive macros. The code could be made more
maintainable by reducing the number. The ultimate goal would be
to have only 2 context-sensitive macros, say “chk” and “chkw”,
referencing the global labels. Everything else would be static
always_inline functions. This would separate the
context-sensitive macro concerns into a small amount of code and
reduce the number of macros in the code. (Two context-sensitive
macros would be needed because one needs to reference only the
label “done" but the other needs to reference the label
“writeback".)

If there is some interest, we could submit a series of patches,
each one replacing a single context-sensitive macro in the
code.

Sincerely,

John

Example
- - - -

As an example, here is a diff that shows how the
context-sensitive macro “fail-if” could be replaced by an inline
function "fail". The diff is not a patch, just shows a suggested
design pattern.

Taken by itself as a single change, this example does not really
reduce the number of context-sensitive macros, but if all of the
macros listed later in this email were replaced, there would be a
significant improvement. Macro "fail_if", shown here

    #define fail_if(p)                                      \
    do {                                                    \
        rc = (p) ? X86EMUL_UNHANDLEABLE : X86EMUL_OKAY;     \
        if ( rc ) goto done;                                \
    } while (0)

can be replaced by making the changes shown in the following
diff. The diff only shows 2 example replacements of “fail_if”, a
patch would replace all uses.

    diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x
    index 10a2959..96b96f2 100644
    --- a/xen/arch/x86/x86_emulate/x86_emulate.c
    +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
    @@ -604,6 +604,24 @@ do {                                                    \
         }                                                                     \
     })
 
    +typedef enum {STAT_OK,
    +              STAT_DONE,
    +              STAT_WRITEBACK} x86_emulate_status_t;
    +
    +#define chk(erc)                                \
    +do {                                            \
    +        if ( (erc) == STAT_DONE )               \
    +            goto done;                          \
    +} while (0)
    +
    +static always_inline x86_emulate_status_t fail(int p, int *rc)
    +{
    +    *rc = p ? X86EMUL_UNHANDLEABLE : X86EMUL_OKAY;
    +    if ( *rc )
    +        return STAT_DONE;
    +    return STAT_OK;
    +}
    +
     /*
      * Given byte has even parity (even number of 1s)? SDM Vol. 1 Sec. 3.4.3.1,
      * "Status Flags": EFLAGS.PF reflects parity of least-sig. byte of result only.
    @@ -989,7 +1007,7 @@ static int ioport_access_check(
         if ( !(ctxt->regs->eflags & EFLG_VM) && mode_iopl() )
             return X86EMUL_OKAY;
 
    -    fail_if(ops->read_segment == NULL);
    +    chk(fail(ops->read_segment == NULL, &rc));
         if ( (rc = ops->read_segment(x86_seg_tr, &tr, ctxt)) != 0 )
             return rc;
 
    @@ -1018,7 +1036,7 @@ static int ioport_access_check(
         return rc;
 
      raise_exception:
    -    fail_if(ops->inject_hw_exception == NULL);
    +    chk(fail(ops->inject_hw_exception == NULL, &rc));
         return ops->inject_hw_exception(EXC_GP, 0, ctxt) ? : X86EMUL_EXCEPTION;
     }

List Of Macros To Replace
- - - - - - - - - - - - -

insn_fetch_bytes
fail_if
generate_exception_if
jmp_rel
get_fpu
get_rep_prefix

Other Macros
- - - - - - -

There are other macros that are context-sensitive because they
use the ones listed above. Examples would be “mode_ring0” and
“mode_iopl”. Since these will have to change as part of the
replacement, we could also implement them directly as inline
functions that expect “chk” or “chkw”.




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: Reducing the Number of Context-Sensitive Macros in x86_emulate.c

[Andrew Cooper]

On 20/04/16 17:42, John McDermott wrote:
> Xen Developers,
>
> Would there be any interest in replacing some of the
> context-sensitive macros in x86_emulate.c, to make it more
> maintainable?
>
> I work on the Xenon project, which researches ways to transform
> Xen into a higher-security hypervisor. One of the things we do is
> modify Xen code to make it more maintainable. As part of that
> work, we have some experience in improving the maintainability of
> x86_emulate.c.
>
> The design of the x86_emulate function depends on labels in such
> a way that it is probably not practical to remove _all_
> context-sensitive macros. The code could be made more
> maintainable by reducing the number. The ultimate goal would be
> to have only 2 context-sensitive macros, say “chk” and “chkw”,
> referencing the global labels. Everything else would be static
> always_inline functions. This would separate the
> context-sensitive macro concerns into a small amount of code and
> reduce the number of macros in the code. (Two context-sensitive
> macros would be needed because one needs to reference only the
> label “done" but the other needs to reference the label
> “writeback".)
>
> If there is some interest, we could submit a series of patches,
> each one replacing a single context-sensitive macro in the
> code.

I have been wanting to do this for a while.  An item was discussed at
Xen Hackathon (two days ago) which involves rewriting x86_emulate().

The primary motivation is to split instruction decode from emulate, to
be able to have an audit stage in the middle.  A side effect of this is
that we can remove 4 of our 5 pieces of code which currently do x86
instruction prefix decoding (each slightly differently).  A second
motivation is to address the lack of support for non-memory-target
instructions, which is an issue for introspection activities.

Rest assured that I dislike the code as much as you do, and I plan for
the result to be far easier to follow and maintain.

Shortly, I will be submitting notes from that session, and a design
showing how I plan to proceed.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel