Re: [PATCH v10 1/9] x86emul: address x86_insn_is_mem_{access, write}() omissions

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/05/2020 15:26, Jan Beulich wrote:
> First of all explain in comments what the functions' purposes are. Then
> make them actually match their comments.
>
> Note that fc6fa977be54 ("x86emul: extend x86_insn_is_mem_write()
> coverage") didn't actually fix the function's behavior for {,V}STMXCSR:
> Both are covered by generic code higher up in the function, due to
> x86_decode_twobyte() already doing suitable adjustments. And VSTMXCSR
> wouldn't have been covered anyway without a further X86EMUL_OPC_VEX()
> case label. Keep the inner case label in a comment for reference.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v10: Move ARPL case to earlier switch() x86_insn_is_mem_write(). Make
>      group 5 handling actually work there. Drop VMPTRST case. Also
>      handle CLFLUSH*, CLWB, UDn, and remaining PREFETCH* in
>      x86_insn_is_mem_access().
> v9: New.
>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -11474,25 +11474,87 @@ x86_insn_operand_ea(const struct x86_emu
>      return state->ea.mem.off;
>  }
>  
> +/*
> + * This function means to return 'true' for all supported insns with explicit
> + * accesses to memory.  This means also insns which don't have an explicit
> + * memory operand (like POP), but it does not mean e.g. segment selector
> + * loads, where the descriptor table access is considered an implicit one.
> + */
>  bool
>  x86_insn_is_mem_access(const struct x86_emulate_state *state,
>                         const struct x86_emulate_ctxt *ctxt)
>  {
> +    if ( mode_64bit() && state->not_64bit )
> +        return false;

Is this path actually used?  state->not_64bit ought to fail instruction
decode, at which point we wouldn't have a valid state to be used here.

Everything else looks ok, so Acked-by: Andrew Cooper
<andrew.cooper3@xxxxxxxxxx>