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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 07 September 2020 10:43
> To: paul@xxxxxxx
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; 'Andrew Cooper' 
> <andrew.cooper3@xxxxxxxxxx>; 'Wei Liu'
> <wl@xxxxxxx>; 'Roger Pau Monné' <roger.pau@xxxxxxxxxx>; 'Jun Nakajima' 
> <jun.nakajima@xxxxxxxxx>;
> 'Kevin Tian' <kevin.tian@xxxxxxxxx>; 'George Dunlap' 
> <George.Dunlap@xxxxxxxxxxxxx>
> Subject: Re: [PATCH v3] x86/HVM: more consistently set I/O completion
> 
> 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."

That sounds ok.

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

No, that's ok; I was just seeking clarification of when this style is 
acceptable.

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

Oh yes, I see it now.

With the comment addition...

Reviewed-by: Paul Durrant <paul@xxxxxxx>

> Jan