Re: [PATCH v8 03/12] x86emul: support ENQCMD insns

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/05/2020 09:13, Jan Beulich wrote:
> Note that the ISA extensions document revision 038 doesn't specify
> exception behavior for ModRM.mod == 0b11; assuming #UD here.

Stale.

> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -11480,11 +11513,36 @@ int x86_emul_blk(
>  {
>      switch ( state->blk )
>      {
> +        bool zf;
> +
>          /*
>           * Throughout this switch(), memory clobbers are used to compensate
>           * that other operands may not properly express the (full) memory
>           * ranges covered.
>           */
> +    case blk_enqcmd:
> +        ASSERT(bytes == 64);
> +        if ( ((unsigned long)ptr & 0x3f) )
> +        {
> +            ASSERT_UNREACHABLE();
> +            return X86EMUL_UNHANDLEABLE;
> +        }
> +        *eflags &= ~EFLAGS_MASK;
> +#ifdef HAVE_AS_ENQCMD
> +        asm ( "enqcmds (%[src]), %[dst]" ASM_FLAG_OUT(, "; setz %0")

%[zf]

> +              : [zf] ASM_FLAG_OUT("=@ccz", "=qm") (zf)
> +              : [src] "r" (data), [dst] "r" (ptr) : "memory" );

Can't src get away with being "m" (*data)?  There is no need to force it
into a single register, even if it is overwhelmingly likely to end up
with %rsi scheduled here.

> +#else
> +        /* enqcmds (%rsi), %rdi */
> +        asm ( ".byte 0xf3, 0x0f, 0x38, 0xf8, 0x3e"
> +              ASM_FLAG_OUT(, "; setz %[zf]")
> +              : [zf] ASM_FLAG_OUT("=@ccz", "=qm") (zf)
> +              : "S" (data), "D" (ptr) : "memory" );
> +#endif
> +        if ( zf )
> +            *eflags |= X86_EFLAGS_ZF;
> +        break;
> +
>      case blk_movdir:
>          switch ( bytes )
>          {
> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -420,6 +420,10 @@
>  #define MSR_IA32_TSC_DEADLINE                0x000006E0
>  #define MSR_IA32_ENERGY_PERF_BIAS    0x000001b0
>  
> +#define MSR_IA32_PASID                       0x00000d93
> +#define  PASID_PASID_MASK            0x000fffff
> +#define  PASID_VALID                 0x80000000
> +

Above the legacy line please as this is using the newer style, and drop
_IA32.  Intel's ideal of architectural-ness isn't interesting or worth
the added code volume.

PASSID_PASSID_MASK isn't great, but I can't suggest anything better, and
MSR_PASSID_MAS doesn't work either.

Otherwise, Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>