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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.05.2020 20:59, Andrew Cooper wrote:
> 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.

In which way (beyond the question of whether to use
goto unrecognized_insn in the code instead)? The doc doesn't
mention ModRM.mod specifics in any way.

>> --- 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]

Oops, indeed.

>> +              : [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.

Well, *data can't be used, as data is of void* type. It would
need to have a suitable cast on it, but since that's not
going to avoid the memory clobber I didn't think it was worth
it (also together with the comment ahead of the switch()).

>> --- 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,

Ah, yes, I should have remembered to re-base this over your
change there.

> and drop
> _IA32.  Intel's ideal of architectural-ness isn't interesting or worth
> the added code volume.

We'd been there before, and you know I disagree. I think it
is wrong for me to make the change, but I will do so just
to retain your ack.

> 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>

Thanks.

Jan