Re: [PATCH v8 06/12] x86/HVM: make hvmemul_blk() capable of handling r/o operations

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.05.2020 22:34, Andrew Cooper wrote:
> On 05/05/2020 09:15, Jan Beulich wrote:
>> In preparation for handling e.g. FLDENV or {F,FX,X}RSTOR here as well.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> v8: New (could be folded into "x86emul: support MOVDIR{I,64B} insns",
>>     but would invalidate Paul's R-b there).
>>
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -1453,7 +1453,7 @@ static int hvmemul_blk(
>>      struct hvm_emulate_ctxt *hvmemul_ctxt =
>>          container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
>>      unsigned long addr;
>> -    uint32_t pfec = PFEC_page_present | PFEC_write_access;
>> +    uint32_t pfec = PFEC_page_present;
>>      int rc;
>>      void *mapping = NULL;
>>  
>> @@ -1462,6 +1462,9 @@ static int hvmemul_blk(
>>      if ( rc != X86EMUL_OKAY || !bytes )
>>          return rc;
>>  
>> +    if ( x86_insn_is_mem_write(state, ctxt) )
>> +        pfec |= PFEC_write_access;
>> +
> 
> For the instructions with two memory operands, it conflates the
> read-only side with the read-write side.

"Conflates" is probably the wrong word? The bug you point out is
really in x86_insn_is_mem_write(), in that so far it would return
false for MOVDIR64B and ENQCMD{,S}. As a result I'll fix the
issue there while, at the same time, folding this patch into
"x86emul: support MOVDIR{I,64B} insns".

Jan