Re: [Xen-devel] [PATCH RFC] x86/HVM: suppress I/O completion for port output

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 29 March 2018 10:41
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: RE: [PATCH RFC] x86/HVM: suppress I/O completion for port output
> 
> >>> On 29.03.18 at 11:13, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 29 March 2018 10:10
> >>
> >> >>> On 29.03.18 at 10:54, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> >> --- a/xen/arch/x86/hvm/emulate.c
> >> >> +++ b/xen/arch/x86/hvm/emulate.c
> >> >> @@ -282,7 +282,7 @@ static int hvmemul_do_io(
> >> >>              rc = hvm_send_ioreq(s, &p, 0);
> >> >>              if ( rc != X86EMUL_RETRY || currd->is_shutting_down )
> >> >>                  vio->io_req.state = STATE_IOREQ_NONE;
> >> >> -            else if ( data_is_addr )
> >> >> +            else if ( data_is_addr || (!is_mmio && dir == IOREQ_WRITE) 
> >> >> )
> >> >
> >> > I'm not entirely sure, but it seems like this test might actually be
> >> > !hvm_vcpu_io_need_completion()...
> >> >
> >> >>                  rc = X86EMUL_OKAY;
> >> >>          }
> >> >>          break;
> >> >> --- a/xen/include/asm-x86/hvm/vcpu.h
> >> >> +++ b/xen/include/asm-x86/hvm/vcpu.h
> >> >> @@ -91,10 +91,12 @@ struct hvm_vcpu_io {
> >> >>      const struct g2m_ioport *g2m_ioport;
> >> >>  };
> >> >>
> >> >> -static inline bool_t hvm_vcpu_io_need_completion(const struct
> >> >> hvm_vcpu_io *vio)
> >> >> +static inline bool hvm_vcpu_io_need_completion(const struct
> >> >> hvm_vcpu_io *vio)
> >> >>  {
> >> >>      return (vio->io_req.state == STATE_IOREQ_READY) &&
> >> >> -           !vio->io_req.data_is_ptr;
> >> >> +           !vio->io_req.data_is_ptr &&
> >> >> +           (vio->io_req.type != IOREQ_TYPE_PIO ||
> >> >> +            vio->io_req.dir != IOREQ_WRITE);
> >> >
> >> > ... now that you've updated it here.
> >>
> >> It could have been before, and it wasn't, so I didn't want to change
> >> that. My assumption is that the function wasn't used to leverage
> >> local variables (and avoid the .state comparison altogether).
> >
> > Yes, that's why it was like it is.
> >
> >> Technically it could be switched, I agree. I guess I should at least
> >> attach a comment, clarifying that this is an open-coded, slightly
> >> optimized variant of the function.
> >>
> >
> > Alternatively if the macro is modified to take an ioreq_t pointer directly
> > rather than a struct hvm_vcpu_io pointer, then I think you could just pass
> > the on-stack ioreq_t to it in hvmemul_do_io() and avoid any real need for
> the
> > open-coded test.
> 
> Hmm, yes, but even then I'm not sure the compiler would realize
> it can omit the .state check. I may try out that transformation once
> I know whether this helps in the first place.
> 

Ok. Fair enough.

  Paul

> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel