[PATCH RFC] x86/HVM: suppress I/O completion for port output

[PATCH RFC] x86/HVM: suppress I/O completion for port output

[Jan Beulich]

We don't break up port requests in case they cross emulation entity
boundaries, and a write to an I/O port is necessarily the last
operation of an instruction instance, so there's no need to re-invoke
the full emulation path upon receiving the result from an external
emulator.

In case we want to properly split port accesses in the future, this
change will need to be reverted, as it would prevent things working
correctly when e.g. the first part needs to go to an external emulator,
while the second part is to be handled internally.

While this addresses the reported problem of Windows paging out the
buffer underneath an in-process REP OUTS, it does not address the wider
problem of the re-issued insn (to the insn emulator) being prone to
raise an exception (#PF) during a replayed, previously successful memory
access (we only record prior MMIO accesses).

Leaving aside the problem tried to be worked around here, I think the
performance aspect alone is a good reason to change the behavior.

Also take the opportunity and change bool_t -> bool as
hvm_vcpu_io_need_completion()'s return type.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
RFC: At this point it is only a hypothesis that this change addresses
     the observed issue. IOW testing in the actual environment is still
     pending.

--- 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) )
                 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);
 }
 
 struct nestedvcpu {




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

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 29 March 2018 08:52
> To: xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>
> Subject: [PATCH RFC] x86/HVM: suppress I/O completion for port output
> 
> We don't break up port requests in case they cross emulation entity
> boundaries, and a write to an I/O port is necessarily the last
> operation of an instruction instance, so there's no need to re-invoke
> the full emulation path upon receiving the result from an external
> emulator.
> 
> In case we want to properly split port accesses in the future, this
> change will need to be reverted, as it would prevent things working
> correctly when e.g. the first part needs to go to an external emulator,
> while the second part is to be handled internally.
> 
> While this addresses the reported problem of Windows paging out the
> buffer underneath an in-process REP OUTS, it does not address the wider
> problem of the re-issued insn (to the insn emulator) being prone to
> raise an exception (#PF) during a replayed, previously successful memory
> access (we only record prior MMIO accesses).
> 
> Leaving aside the problem tried to be worked around here, I think the
> performance aspect alone is a good reason to change the behavior.
> 
> Also take the opportunity and change bool_t -> bool as
> hvm_vcpu_io_need_completion()'s return type.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> RFC: At this point it is only a hypothesis that this change addresses
>      the observed issue. IOW testing in the actual environment is still
>      pending.
> 
> --- 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.

  Paul

>  }
> 
>  struct nestedvcpu {
> 
> 


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

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

[Jan Beulich]

>>> On 29.03.18 at 10:54, <Paul.Durrant@citrix.com> 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).
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.

Jan


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

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 29 March 2018 10:10
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-devel <xen-
> devel@lists.xenproject.org>
> Subject: RE: [PATCH RFC] x86/HVM: suppress I/O completion for port output
> 
> >>> On 29.03.18 at 10:54, <Paul.Durrant@citrix.com> 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.

  Paul

> Jan


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

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

[Jan Beulich]

>>> On 29.03.18 at 11:13, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 29 March 2018 10:10
>> 
>> >>> On 29.03.18 at 10:54, <Paul.Durrant@citrix.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 29 March 2018 10:41
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-devel <xen-
> devel@lists.xenproject.org>
> Subject: RE: [PATCH RFC] x86/HVM: suppress I/O completion for port output
> 
> >>> On 29.03.18 at 11:13, <Paul.Durrant@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 29 March 2018 10:10
> >>
> >> >>> On 29.03.18 at 10:54, <Paul.Durrant@citrix.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Jan Beulich]

>>> On 29.03.18 at 11:13, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 29 March 2018 10:10
>> >> --- 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.

We can't use the on-stack ioreq_t, as hvm_send_ioreq() alters its
state field. I'm now going to see whether &vio->io_req could be
used here, but the first try already produced worse code, and I
don't expect this extra change to improve things (but rather make
them slightly worse).

Jan


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