Re: [Xen-devel] [PATCH -tip v3 1/2] x86: xen: insn: Decode Xen and KVM emulate-prefix signature

[Masami Hiramatsu <mhiramat@xxxxxxxxxx>]

On Fri, 6 Sep 2019 09:34:36 +0200
Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:

> On Fri, Sep 06, 2019 at 10:45:48AM +0900, Masami Hiramatsu wrote:
> 
> > diff --git a/arch/x86/include/asm/xen/interface.h 
> > b/arch/x86/include/asm/xen/interface.h
> > index 62ca03ef5c65..fe33a9798708 100644
> > --- a/arch/x86/include/asm/xen/interface.h
> > +++ b/arch/x86/include/asm/xen/interface.h
> > @@ -379,12 +379,15 @@ struct xen_pmu_arch {
> >   * Prefix forces emulation of some non-trapping instructions.
> >   * Currently only CPUID.
> >   */
> > +#include <asm/xen/prefix.h>
> > +
> >  #ifdef __ASSEMBLY__
> > -#define XEN_EMULATE_PREFIX .byte 0x0f,0x0b,0x78,0x65,0x6e ;
> > +#define XEN_EMULATE_PREFIX .byte __XEN_EMULATE_PREFIX ;
> >  #define XEN_CPUID          XEN_EMULATE_PREFIX cpuid
> >  #else
> > -#define XEN_EMULATE_PREFIX ".byte 0x0f,0x0b,0x78,0x65,0x6e ; "
> > +#define XEN_EMULATE_PREFIX ".byte " __XEN_EMULATE_PREFIX_STR " ; "
> >  #define XEN_CPUID          XEN_EMULATE_PREFIX "cpuid"
> > +
> >  #endif
> 
> Possibly you can do something like:
> 
> #define XEN_EMULATE_PREFIX    __ASM_FORM(.byte __XEN_EMULATE_PREFIX ;)
> #define XEN_CPUID             XEN_EMULATE_PREFIX __ASM_FORM(cpuid)

Hmm, OK. But should I split this change from insn decoder change?

> 
> >  #endif /* _ASM_X86_XEN_INTERFACE_H */
> > diff --git a/arch/x86/include/asm/xen/prefix.h 
> > b/arch/x86/include/asm/xen/prefix.h
> > new file mode 100644
> > index 000000000000..f901be0d7a95
> > --- /dev/null
> > +++ b/arch/x86/include/asm/xen/prefix.h
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _TOOLS_ASM_X86_XEN_PREFIX_H
> > +#define _TOOLS_ASM_X86_XEN_PREFIX_H
> > +
> > +#include <linux/stringify.h>
> > +
> > +#define __XEN_EMULATE_PREFIX  0x0f,0x0b,0x78,0x65,0x6e
> > +#define __XEN_EMULATE_PREFIX_STR  __stringify(__XEN_EMULATE_PREFIX)
> > +
> > +#endif
> 
> How about we make this asm/virt_prefix.h or something and include:
> 
> /*
>  * Virt escape sequences to trigger instruction emulation;
>  * ideally these would decode to 'whole' instruction and not destroy
>  * the instruction stream; sadly this is not true for the 'kvm' one :/
>  */
> 
> #define __XEN_EMULATE_PREFIX  0x0f,0x0b,0x78,0x65,0x6e  /* ud2 ; .ascii "xen" 
> */
> #define __KVM_EMULATE_PREFIX  0x0f,0x0b,0x6b,0x76,0x6d        /* ud2 ; .ascii 
> "kvm" */

OK, and in that case I think we should do this in separated patch from
this change.

> 
> > diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
> > index 0b5862ba6a75..b7eb50187db9 100644
> > --- a/arch/x86/lib/insn.c
> > +++ b/arch/x86/lib/insn.c
> 
> > @@ -58,6 +61,37 @@ void insn_init(struct insn *insn, const void *kaddr, int 
> > buf_len, int x86_64)
> >             insn->addr_bytes = 4;
> >  }
> >  
> > +static const insn_byte_t xen_prefix[] = { __XEN_EMULATE_PREFIX };
> > +/* See handle_ud()@arch/x86/kvm/x86.c */
> > +static const insn_byte_t kvm_prefix[] = "\xf\xbkvm";
> 
> Then you can make this consistent; maybe even something like:
> 
> static const insn_byte_t *virt_prefix[] = {
>       { __XEN_EMULATE_PREFIX },
>       { __KVM_EMULATE_PREFIX },
>       { NULL },
> };
> 
> And then change emulate_prefix_size to emulate_prefix_index ?

Hmm, how we can get the length of those emulate prefixes?
For struct insn, since the size information is important, other
sub fields have "nbyte" field so that caller can find the actual
bytes from original byte stream.

Maybe we can have struct emulate_prefix { .nbyte, .type }; and
define enum etc.. but for me, it seems a bit over engineering.
(since no one use that feature)

Thank you,

-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel