Re: [Xen-devel] [PATCH 02/15] x86/emul: Simplfy emulation state setup

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.11.16 at 14:59, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 24/11/16 13:44, Jan Beulich wrote:
>>>>> On 23.11.16 at 16:38, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -1904,6 +1904,8 @@ x86_decode(
>>>      state->regs = ctxt->regs;
>>>      state->eip = ctxt->regs->eip;
>>>  
>>> +    /* Initialise output state in x86_emulate_ctxt */
>>> +    ctxt->opcode = ~0u;
>> I have to admit that I don't see the point of this.
> 
> There are early exit paths which leave it uninitialised.  An alternative
> would be explicitly document that it is only valid for X86EMUL_OKAY?

Well, to me that goes without saying, but I'm fine with it being added.

>> So I'd suggest to have three groups: input, input/output, output,
>> even if for your purpose here you only want to tell apart fields which
>> need to be initialized before calling x86_emulate() / x86_decode()
>> (the first two groups) from those which don't (the last group).
> 
> Right - proposed net result looks like:
> 
> struct x86_emulate_ctxt
> {
>     /*
>      * Input-only state:
>      */
> 
>     /* Software event injection support. */
>     enum x86_swint_emulation swint_emulate;
> 
>     /* Set this if writes may have side effects. */
>     bool force_writeback;
> 
>     /* Caller data that can be used by x86_emulate_ops' routines. */
>     void *data;
> 
>     /*
>      * Input/output state:
>      */
> 
>     /* Register state before/after emulation. */
>     struct cpu_user_regs *regs;
> 
>     /* Default address size in current execution mode (16, 32, or 64). */
>     unsigned int addr_size;
> 
>     /* Stack pointer width in bits (16, 32 or 64). */
>     unsigned int sp_size;
> 
>     /*
>      * Output-only state:
>      */
> 
>     /* Canonical opcode (see below) (valid only on X86EMUL_OKAY). */
>     unsigned int opcode;
> 
>     /* Retirement state, set by the emulator (valid only on
> X86EMUL_OKAY). */
>     union {
>         struct {
>             uint8_t hlt:1;          /* Instruction HLTed. */
>             uint8_t mov_ss:1;       /* Instruction sets MOV-SS irq
> shadow. */
>             uint8_t sti:1;          /* Instruction sets STI irq shadow. */
>         } flags;
>         uint8_t byte;
>     } retire;
> };
> 
> If that is ok?

Looks good, thanks. With that
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan


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