[Xen-devel] Reducing the Number of Context-Sensitive Macros in x86_emulate.c

[John McDermott <john.mcdermott@xxxxxxxxxxxx>]

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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel