Re: [PATCH v3] x86/HVM: more consistently set I/O completion

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.09.2020 18:17, Paul Durrant wrote:
>> @@ -2610,8 +2612,13 @@ static const struct x86_emulate_ops hvm_
>>      .vmfunc        = hvmemul_vmfunc,
>>  };
>>
>> +/*
>> + * Note that passing HVMIO_no_completion into this function serves as kind
>> + * of (but not fully) an "auto select completion" indicator.
>> + */
>>  static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt,
>> -    const struct x86_emulate_ops *ops)
>> +    const struct x86_emulate_ops *ops,
>> +    enum hvm_io_completion completion)
>>  {
>>      const struct cpu_user_regs *regs = hvmemul_ctxt->ctxt.regs;
>>      struct vcpu *curr = current;
>> @@ -2642,16 +2649,31 @@ static int _hvm_emulate_one(struct hvm_e
>>          rc = X86EMUL_RETRY;
>>
>>      if ( !hvm_ioreq_needs_completion(&vio->io_req) )
>> +        completion = HVMIO_no_completion;
> 
> The comment doesn't mention that passing in something other than
> HVMIO_no_completion could get overridden. Is that intentional?

Well, it was, but since you seem to be asking for it I've added
"When there's no completion needed, the passed in value will be
 ignored in any case."

>> +    else if ( completion == HVMIO_no_completion )
>> +        completion = (vio->io_req.type != IOREQ_TYPE_PIO ||
>> +                      hvmemul_ctxt->is_mem_access) ? HVMIO_mmio_completion
>> +                                                   : HVMIO_pio_completion;
>> +
>> +    switch ( vio->io_completion = completion )
> 
> I thought we tended to avoid assignments in control flow statements.

In if() we do, because of the ambiguity whether == might have
been meant instead. But in switch() there's imo no such
ambiguity - if == was really meant, if() would better be used
anyway. We have quite a few cases of this elsewhere, and I think
some of them are reasonably recent introductions. As you're the
maintainer of the file, if you strongly think I shouldn't do so,
I'll switch of course.

>> @@ -2727,8 +2752,8 @@ int hvm_emulate_one_mmio(unsigned long m
>>      hvm_emulate_init_once(&ctxt, x86_insn_is_mem_write,
>>                            guest_cpu_user_regs());
>>      ctxt.ctxt.data = &mmio_ro_ctxt;
>> -    rc = _hvm_emulate_one(&ctxt, ops);
>> -    switch ( rc )
>> +
>> +    switch ( rc = _hvm_emulate_one(&ctxt, ops, HVMIO_no_completion) )
> 
> Again, why move the assignment into the switch statement?

For consistency with the other cases where this gets introduced
in this patch, at least. I for one consider this the more concise
way of writing such code.

>> --- a/xen/arch/x86/hvm/vmx/realmode.c
>> +++ b/xen/arch/x86/hvm/vmx/realmode.c
>> @@ -97,15 +97,11 @@ static void realmode_deliver_exception(
>>  void vmx_realmode_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt)
>>  {
>>      struct vcpu *curr = current;
>> -    struct hvm_vcpu_io *vio = &curr->arch.hvm.hvm_io;
>>      int rc;
>>
>>      perfc_incr(realmode_emulations);
>>
>> -    rc = hvm_emulate_one(hvmemul_ctxt);
>> -
>> -    if ( hvm_ioreq_needs_completion(&vio->io_req) )
>> -        vio->io_completion = HVMIO_realmode_completion;
>> +    rc = hvm_emulate_one(hvmemul_ctxt, HVMIO_realmode_completion);
> 
> Ok, I guess the override of completion is intentional to deal with
> this case. Perhaps expand the comment above _hvm_emulate_one() then.

Right, done as per above. Let me know whether the text still isn't
sufficient.

>> --- a/xen/include/asm-x86/hvm/emulate.h
>> +++ b/xen/include/asm-x86/hvm/emulate.h
>> @@ -48,6 +48,8 @@ struct hvm_emulate_ctxt {
>>
>>      uint32_t intr_shadow;
>>
>> +    bool is_mem_access;
>> +
> 
> Whilst you mention in the commit comment why this is added, I don't
> see any consumer if it in this patch. Will the come later?

hvmemul_validate() sets the field, and _hvm_emulate_one() consumes
it.

Jan