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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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