[PATCH 0/5] Skip unneeded VMENTRY & VMEXIT

[PATCH 0/5] Skip unneeded VMENTRY & VMEXIT

[Don Slutz]

Using a new enough QEMU that has:

commit 7665d6ba98e20fb05c420de947c1750fd47e5c07
Author: Paul Durrant <paul.durrant@citrix.com>
Date:   Tue Jan 20 11:06:19 2015 +0000

    Xen: Use the ioreq-server API when available
    

means that hvmloader and the guest will do pci config accesses that
will not be sent to QEMU.  However hvm_complete_assist_req() (which
is the routine that does the work) returns a 1 which
hvm_send_assist_req() returns to the caller which means that the
request was sent to QEMU and need to be waited for.

This is not the case.  hvm_complete_assist_req() has called on
hvm_io_assist() which has changed the io_state from
HVMIO_awaiting_completion to HVMIO_completed if needed.  But
hvmemul_do_io() thinks that it needs to wait on the IOREQ_READ case,
and returns X86EMUL_RETRY.

[PATCH 1/5] DEBUG: xentrace: Add debug of HANDLE_PIO
  -- No need to apply

[PATCH 2/5] xentrace: Adjust IOPORT & MMIO format
  -- Simple bugfix.  xentrace_format is not converting these correctly

[PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do
  -- Simple bugfix.  Do the same thing as when hvm_has_dm() returns
     a zero.

[PATCH 4/5] hvm_complete_assist_req: We should not be able to get
  -- This ASSERT was sugested by Paul Durrant.  Since I was in the
     file, just add it.

[PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send
  -- The real fix.  Does depend on [PATCH 3/5] hvmemul_do_io: If the send ...



Here is the before xentrace output using the "[PATCH 1/5] DEBUG:
xentrace: Add debug of HANDLE_PIO" (which I do not expect to be
applied.  It is first because that is the order you need to use to
reproduce the xentrace output):


CPU0  685992932190 (+    2292)  VMEXIT      [ exitcode = 0x0000001e, rIP  = 0x00101581 ]
CPU0  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 io_state = 5 ret = 0 ]
CPU0  685992934938 (+    2748)  vlapic_accept_pic_intr [ i8259_target = 1, accept_pic_int = 1 ]
CPU0  685992935526 (+     588)  VMENTRY
CPU0  685992937650 (+    2124)  VMEXIT      [ exitcode = 0x0000001e, rIP  = 0x00101581 ]
CPU0  0 (+       0)  IOPORT_READ [ port = 0x0cfe, data = 0x00000000 ]
CPU0  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 io_state = 0 ret = 1 ]
CPU0  685992940248 (+    2598)  vlapic_accept_pic_intr [ i8259_target = 1, accept_pic_int = 1 ]
CPU0  685992940968 (+     720)  VMENTRY


And after:


CPU2  1028654638584 (+    2388)  VMEXIT      [ exitcode = 0x0000001e, rIP  = 0x00101581 ]
CPU2  0 (+       0)  IOPORT_READ [ port = 0x0cfe, data = 0xffffffff ]
CPU2  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 io_state = 0 ret = 1 ]
CPU2  1028654641932 (+    3348)  vlapic_accept_pic_intr [ i8259_target = 1, accept_pic_int = 1 ]
CPU2  1028654642586 (+     654)  VMENTRY


Don Slutz (5):
  DEBUG: xentrace: Add debug of HANDLE_PIO
  xentrace: Adjust IOPORT & MMIO format
  hvmemul_do_io: If the send to the ioreq server failed do not retry.
  hvm_complete_assist_req: We should not be able to get here on
    IOREQ_TYPE_PCI_CONFIG
  hvm_complete_assist_req: Tell caller we failed to send

 tools/xentrace/formats          | 10 ++++++----
 xen/arch/x86/hvm/emulate.c      |  4 ++++
 xen/arch/x86/hvm/hvm.c          |  3 ++-
 xen/arch/x86/hvm/io.c           |  4 ++++
 xen/include/asm-x86/hvm/trace.h |  2 +-
 xen/include/public/trace.h      |  2 ++
 6 files changed, 19 insertions(+), 6 deletions(-)

-- 
1.8.4

[PATCH 1/5] DEBUG: xentrace: Add debug of HANDLE_PIO

[Don Slutz]

This is not needed.  However I provide it as it is used to generate
data in the cover letter.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 tools/xentrace/formats          | 2 ++
 xen/arch/x86/hvm/io.c           | 4 ++++
 xen/include/asm-x86/hvm/trace.h | 2 +-
 xen/include/public/trace.h      | 2 ++
 4 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/xentrace/formats b/tools/xentrace/formats
index da658bf..089022a 100644
--- a/tools/xentrace/formats
+++ b/tools/xentrace/formats
@@ -80,6 +80,8 @@
 0x00082021  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  NPF         [ gpa = 0x%(2)08x%(1)08x mfn = 0x%(4)08x%(3)08x qual = 0x%(5)04x p2mt = 0x%(6)04x ]
 0x00082023  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  TRAP        [ vector = 0x%(1)02x ]
 
+0x00082040  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  HANDLE_PIO [ port = 0x%(1)04x size = %(2)d dir = %(3)d io_state = %(4)d ret = %(5)d ]
+
 0x0010f001  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  page_grant_map      [ domid = %(1)d ]
 0x0010f002  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  page_grant_unmap    [ domid = %(1)d ]
 0x0010f003  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  page_grant_transfer [ domid = %(1)d ]
diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
index 68fb890..8584d96 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -155,7 +155,10 @@ int handle_pio(uint16_t port, unsigned int size, int dir)
         break;
     case X86EMUL_RETRY:
         if ( vio->io_state != HVMIO_awaiting_completion )
+        {
+            HVMTRACE_5D(HANDLE_PIO, port, size, dir, vio->io_state, 0);
             return 0;
+        }
         /* Completion in hvm_io_assist() with no re-emulation required. */
         ASSERT(dir == IOREQ_READ);
         vio->io_state = HVMIO_handle_pio_awaiting_completion;
@@ -166,6 +169,7 @@ int handle_pio(uint16_t port, unsigned int size, int dir)
         break;
     }
 
+    HVMTRACE_5D(HANDLE_PIO, port, size, dir, vio->io_state, 1);
     return 1;
 }
 
diff --git a/xen/include/asm-x86/hvm/trace.h b/xen/include/asm-x86/hvm/trace.h
index de802a6..32af344 100644
--- a/xen/include/asm-x86/hvm/trace.h
+++ b/xen/include/asm-x86/hvm/trace.h
@@ -54,7 +54,7 @@
 #define DO_TRC_HVM_TRAP             DEFAULT_HVM_MISC
 #define DO_TRC_HVM_TRAP_DEBUG       DEFAULT_HVM_MISC
 #define DO_TRC_HVM_VLAPIC           DEFAULT_HVM_MISC
-
+#define DO_TRC_HVM_HANDLE_PIO  DEFAULT_HVM_MISC
 
 #define TRC_PAR_LONG(par) ((par)&0xFFFFFFFF),((par)>>32)
 
diff --git a/xen/include/public/trace.h b/xen/include/public/trace.h
index 5211ae7..5703551 100644
--- a/xen/include/public/trace.h
+++ b/xen/include/public/trace.h
@@ -228,6 +228,8 @@
 #define TRC_HVM_TRAP_DEBUG       (TRC_HVM_HANDLER + 0x24)
 #define TRC_HVM_VLAPIC           (TRC_HVM_HANDLER + 0x25)
 
+#define TRC_HVM_HANDLE_PIO      (TRC_HVM_HANDLER + 0x40)
+
 #define TRC_HVM_IOPORT_WRITE    (TRC_HVM_HANDLER + 0x216)
 #define TRC_HVM_IOMEM_WRITE     (TRC_HVM_HANDLER + 0x217)
 
-- 
1.8.4

[PATCH 2/5] xentrace: Adjust IOPORT & MMIO format

[Don Slutz]

The 1st item is not data, but the port (address).
The 2nd item is the data.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 tools/xentrace/formats | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/xentrace/formats b/tools/xentrace/formats
index 089022a..51b652c 100644
--- a/tools/xentrace/formats
+++ b/tools/xentrace/formats
@@ -68,10 +68,10 @@
 0x00082014  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  INVLPG      [ is invlpga? = %(1)d, virt = 0x%(2)08x ]
 0x00082114  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  INVLPG      [ is invlpga? = %(1)d, virt = 0x%(3)08x%(2)08x ]
 0x00082015  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  MCE
-0x00082016  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  IOPORT_READ [ data = 0x%(1)04x ]
-0x00082216  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  IOPORT_WRITE [ data = 0x%(1)04x ]
-0x00082017  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  MMIO_READ   [ data = 0x%(1)04x ]
-0x00082217  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  MMIO_WRITE  [ data = 0x%(1)04x ]
+0x00082016  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  IOPORT_READ [ port = 0x%(1)04x, data = 0x%(2)08x ]
+0x00082216  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  IOPORT_WRITE [ port = 0x%(1)04x, data = 0x%(2)08x ]
+0x00082017  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  MMIO_READ   [ port = 0x%(1)08x, data = 0x%(2)08x ]
+0x00082217  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  MMIO_WRITE  [ port = 0x%(1)08x, data = 0x%(2)08x ]
 0x00082018  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  CLTS
 0x00082019  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  LMSW        [ value = 0x%(1)08x ]
 0x00082119  CPU%(cpu)d  %(tsc)d (+%(reltsc)8d)  LMSW        [ value = 0x%(2)08x%(1)08x ]
-- 
1.8.4

[PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry.

[Don Slutz]

I.E. do just what no backing DM does.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 xen/arch/x86/hvm/emulate.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 2ed4344..e9fc070 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -228,7 +228,11 @@ static int hvmemul_do_io(
         {
             rc = X86EMUL_RETRY;
             if ( !hvm_send_assist_req(&p) )
+            {
+                /* Since the send failed, do not retry */
+                rc = X86EMUL_OKAY;
                 vio->io_state = HVMIO_none;
+            }
             else if ( p_data == NULL )
                 rc = X86EMUL_OKAY;
         }
-- 
1.8.4

[PATCH 4/5] hvm_complete_assist_req: We should not be able to get here on IOREQ_TYPE_PCI_CONFIG

[Don Slutz]

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 xen/arch/x86/hvm/hvm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index c7984d1..6f7b407 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2573,6 +2573,7 @@ bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
 
 static bool_t hvm_complete_assist_req(ioreq_t *p)
 {
+    ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG);
     switch ( p->type )
     {
     case IOREQ_TYPE_COPY:
-- 
1.8.4

[PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send

[Don Slutz]

This saves a VMENTRY and a VMEXIT since we not longer retry the
ioport read.

hvmemul_do_io() will retry in order to complete an ioport read when
hvm_send_assist_req() is successful.

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
 xen/arch/x86/hvm/hvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 6f7b407..bad410e 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2599,7 +2599,7 @@ static bool_t hvm_complete_assist_req(ioreq_t *p)
         break;
     }
 
-    return 1;
+    return 0; /* implicitly bins the i/o operation */
 }
 
 bool_t hvm_send_assist_req(ioreq_t *p)
-- 
1.8.4

Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry.

[Jan Beulich]

>>> On 30.01.15 at 01:52, <dslutz@verizon.com> wrote:
> I.E. do just what no backing DM does.

_If_ this is correct, the if() modified here should be folded with the
one a few lines up. But looking at the description of the commit that
introduced this (bac0999325 "x86 hvm: Do not incorrectly retire an
instruction emulation...", almost immediately modified by f20f3c8ece
"x86 hvm: On failed hvm_send_assist_req(), io emulation...") I doubt
this is really what we want, or at the very least your change
description should explain what was wrong with the original commit.

Jan

> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -228,7 +228,11 @@ static int hvmemul_do_io(
>          {
>              rc = X86EMUL_RETRY;
>              if ( !hvm_send_assist_req(&p) )
> +            {
> +                /* Since the send failed, do not retry */
> +                rc = X86EMUL_OKAY;
>                  vio->io_state = HVMIO_none;
> +            }
>              else if ( p_data == NULL )
>                  rc = X86EMUL_OKAY;
>          }
> -- 
> 1.8.4

Re: [PATCH 4/5] hvm_complete_assist_req: We should not be able to get here on IOREQ_TYPE_PCI_CONFIG

[Jan Beulich]

>>> On 30.01.15 at 01:52, <dslutz@verizon.com> wrote:

According to your 0/5
Suggested-by: Paul Durrant <paul.durrant@citrix.com>
?

Jan

> Signed-off-by: Don Slutz <dslutz@verizon.com>
> ---
>  xen/arch/x86/hvm/hvm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index c7984d1..6f7b407 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2573,6 +2573,7 @@ bool_t hvm_send_assist_req_to_ioreq_server(struct 
> hvm_ioreq_server *s,
>  
>  static bool_t hvm_complete_assist_req(ioreq_t *p)
>  {
> +    ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG);
>      switch ( p->type )
>      {
>      case IOREQ_TYPE_COPY:
> -- 
> 1.8.4

Re: [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send

[Jan Beulich]

>>> On 30.01.15 at 01:52, <dslutz@verizon.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2599,7 +2599,7 @@ static bool_t hvm_complete_assist_req(ioreq_t *p)
>          break;
>      }
>  
> -    return 1;
> +    return 0; /* implicitly bins the i/o operation */
>  }

This change points out that having hvm_complete_assist_req() be a
separate function yet having only a single caller, and it returning
non-void with only a single possible return value isn't the best
arrangement. I think this should be brought back into
hvm_send_assist_req(), by inverting the if() condition there. Unless
there are intentions for it to have another caller, but in that case
it should still be made return void, with the caller choosing what to
return.

Jan

Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry.

[Paul Durrant]

> -----Original Message-----
> From: Don Slutz [mailto:dslutz@verizon.com]
> Sent: 30 January 2015 00:52
> To: xen-devel@lists.xen.org
> Cc: Andrew Cooper; George Dunlap; Ian Campbell; Ian Jackson; Jan Beulich;
> Keir (Xen.org); Stefano Stabellini; Wei Liu; Paul Durrant; Don Slutz
> Subject: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed
> do not retry.
> 
> I.E. do just what no backing DM does.
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> ---
>  xen/arch/x86/hvm/emulate.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 2ed4344..e9fc070 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -228,7 +228,11 @@ static int hvmemul_do_io(
>          {
>              rc = X86EMUL_RETRY;
>              if ( !hvm_send_assist_req(&p) )
> +            {
> +                /* Since the send failed, do not retry */
> +                rc = X86EMUL_OKAY;

I guess this is probably ok. It's a bit moot though as the only circumstance under which hvm_send_assist_req() will return 0 AFAICT, domain_crash() will have been called, or the vcpu is shutting down.

  Paul

>                  vio->io_state = HVMIO_none;
> +            }
>              else if ( p_data == NULL )
>                  rc = X86EMUL_OKAY;
>          }
> --
> 1.8.4

Re: [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send

[Paul Durrant]

> -----Original Message-----
> From: Don Slutz [mailto:dslutz@verizon.com]
> Sent: 30 January 2015 00:53
> To: xen-devel@lists.xen.org
> Cc: Andrew Cooper; George Dunlap; Ian Campbell; Ian Jackson; Jan Beulich;
> Keir (Xen.org); Stefano Stabellini; Wei Liu; Paul Durrant; Don Slutz
> Subject: [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send
> 
> This saves a VMENTRY and a VMEXIT since we not longer retry the
> ioport read.
> 
> hvmemul_do_io() will retry in order to complete an ioport read when
> hvm_send_assist_req() is successful.
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>

It seems like this change should be folded into patch 3? I think the two changes only make sense in combination.

  Paul

> ---
>  xen/arch/x86/hvm/hvm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 6f7b407..bad410e 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2599,7 +2599,7 @@ static bool_t hvm_complete_assist_req(ioreq_t *p)
>          break;
>      }
> 
> -    return 1;
> +    return 0; /* implicitly bins the i/o operation */
>  }
> 
>  bool_t hvm_send_assist_req(ioreq_t *p)
> --
> 1.8.4

Re: [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send

[Paul Durrant]

> -----Original Message-----
> From: Don Slutz [mailto:dslutz@verizon.com]
> Sent: 30 January 2015 00:53
> To: xen-devel@lists.xen.org
> Cc: Andrew Cooper; George Dunlap; Ian Campbell; Ian Jackson; Jan Beulich;
> Keir (Xen.org); Stefano Stabellini; Wei Liu; Paul Durrant; Don Slutz
> Subject: [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send
> 
> This saves a VMENTRY and a VMEXIT since we not longer retry the
> ioport read.
> 
> hvmemul_do_io() will retry in order to complete an ioport read when
> hvm_send_assist_req() is successful.
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> ---
>  xen/arch/x86/hvm/hvm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 6f7b407..bad410e 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2599,7 +2599,7 @@ static bool_t hvm_complete_assist_req(ioreq_t *p)
>          break;
>      }
> 
> -    return 1;
> +    return 0; /* implicitly bins the i/o operation */

Actually that comment is not right. The operation is not binned; it's just already been done.

  Paul

>  }
> 
>  bool_t hvm_send_assist_req(ioreq_t *p)
> --
> 1.8.4

Re: [PATCH 4/5] hvm_complete_assist_req: We should not be able to get here on IOREQ_TYPE_PCI_CONFIG

[Don Slutz]

On 01/30/15 05:24, Jan Beulich wrote:
>>>> >>> On 30.01.15 at 01:52, <dslutz@verizon.com> wrote:
> According to your 0/5
> Suggested-by: Paul Durrant <paul.durrant@citrix.com>
> ?

Yes, added.
   -Don Slutz

> 
> Jan
> 
>> > Signed-off-by: Don Slutz <dslutz@verizon.com>
>> > ---
>> >  xen/arch/x86/hvm/hvm.c | 1 +
>> >  1 file changed, 1 insertion(+)
>> > 
>> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> > index c7984d1..6f7b407 100644
>> > --- a/xen/arch/x86/hvm/hvm.c
>> > +++ b/xen/arch/x86/hvm/hvm.c
>> > @@ -2573,6 +2573,7 @@ bool_t hvm_send_assist_req_to_ioreq_server(struct 
>> > hvm_ioreq_server *s,
>> >  
>> >  static bool_t hvm_complete_assist_req(ioreq_t *p)
>> >  {
>> > +    ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG);
>> >      switch ( p->type )
>> >      {
>> >      case IOREQ_TYPE_COPY:
>> > -- 
>> > 1.8.4
> 
> 

Re: [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send

[Don Slutz]

On 01/30/15 05:24, Jan Beulich wrote:
>>>> On 30.01.15 at 01:52, <dslutz@verizon.com> wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -2599,7 +2599,7 @@ static bool_t hvm_complete_assist_req(ioreq_t *p)
>>          break;
>>      }
>>  
>> -    return 1;
>> +    return 0; /* implicitly bins the i/o operation */
>>  }
> 
> This change points out that having hvm_complete_assist_req() be a
> separate function yet having only a single caller, and it returning
> non-void with only a single possible return value isn't the best
> arrangement. I think this should be brought back into
> hvm_send_assist_req(), by inverting the if() condition there. Unless
> there are intentions for it to have another caller, but in that case
> it should still be made return void, with the caller choosing what to
> return.
> 

Sounds good to me will do.
   -Don Slutz

> Jan
> 

Re: [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send

[Don Slutz]

On 01/30/15 05:40, Paul Durrant wrote:
>> -----Original Message-----
>> From: Don Slutz [mailto:dslutz@verizon.com]
>> Sent: 30 January 2015 00:53
>> To: xen-devel@lists.xen.org
>> Cc: Andrew Cooper; George Dunlap; Ian Campbell; Ian Jackson; Jan Beulich;
>> Keir (Xen.org); Stefano Stabellini; Wei Liu; Paul Durrant; Don Slutz
>> Subject: [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send
>>
>> This saves a VMENTRY and a VMEXIT since we not longer retry the
>> ioport read.
>>
>> hvmemul_do_io() will retry in order to complete an ioport read when
>> hvm_send_assist_req() is successful.
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
> 
> It seems like this change should be folded into patch 3? I think the two changes only make sense in combination.
> 

Happy to do so.
   -Don Slutz


>   Paul
> 
>> ---
>>  xen/arch/x86/hvm/hvm.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 6f7b407..bad410e 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -2599,7 +2599,7 @@ static bool_t hvm_complete_assist_req(ioreq_t *p)
>>          break;
>>      }
>>
>> -    return 1;
>> +    return 0; /* implicitly bins the i/o operation */
>>  }
>>
>>  bool_t hvm_send_assist_req(ioreq_t *p)
>> --
>> 1.8.4
> 

Re: [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send

[Don Slutz]

On 01/30/15 05:53, Paul Durrant wrote:
>> -----Original Message-----
>> From: Don Slutz [mailto:dslutz@verizon.com]
>> Sent: 30 January 2015 00:53
>> To: xen-devel@lists.xen.org
>> Cc: Andrew Cooper; George Dunlap; Ian Campbell; Ian Jackson; Jan Beulich;
>> Keir (Xen.org); Stefano Stabellini; Wei Liu; Paul Durrant; Don Slutz
>> Subject: [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send
>>
>> This saves a VMENTRY and a VMEXIT since we not longer retry the
>> ioport read.
>>
>> hvmemul_do_io() will retry in order to complete an ioport read when
>> hvm_send_assist_req() is successful.
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> ---
>>  xen/arch/x86/hvm/hvm.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 6f7b407..bad410e 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -2599,7 +2599,7 @@ static bool_t hvm_complete_assist_req(ioreq_t *p)
>>          break;
>>      }
>>
>> -    return 1;
>> +    return 0; /* implicitly bins the i/o operation */
> 
> Actually that comment is not right. The operation is not binned; it's just already been done.
> 

Will adjust, I expect the return to go away.
   -Don Slutz

>   Paul
> 
>>  }
>>
>>  bool_t hvm_send_assist_req(ioreq_t *p)
>> --
>> 1.8.4
> 

Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry.

[Don Slutz]

On 01/30/15 05:37, Paul Durrant wrote:
>> -----Original Message-----
>> From: Don Slutz [mailto:dslutz@verizon.com]
>> Sent: 30 January 2015 00:52
>> To: xen-devel@lists.xen.org
>> Cc: Andrew Cooper; George Dunlap; Ian Campbell; Ian Jackson; Jan Beulich;
>> Keir (Xen.org); Stefano Stabellini; Wei Liu; Paul Durrant; Don Slutz
>> Subject: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed
>> do not retry.
>>
>> I.E. do just what no backing DM does.
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> ---
>>  xen/arch/x86/hvm/emulate.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>> index 2ed4344..e9fc070 100644
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -228,7 +228,11 @@ static int hvmemul_do_io(
>>          {
>>              rc = X86EMUL_RETRY;
>>              if ( !hvm_send_assist_req(&p) )
>> +            {
>> +                /* Since the send failed, do not retry */
>> +                rc = X86EMUL_OKAY;
> 
> I guess this is probably ok. It's a bit moot though as the only circumstance under which hvm_send_assist_req() will return 0 AFAICT, domain_crash() will have been called, or the vcpu is shutting down.
> 


Turns out this is not ok.  Jan pointed out that for guest suspend to
work correctly, you need to retry.

   -Don Slutz


>   Paul
> 
>>                  vio->io_state = HVMIO_none;
>> +            }
>>              else if ( p_data == NULL )
>>                  rc = X86EMUL_OKAY;
>>          }
>> --
>> 1.8.4
> 

Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry.

[Don Slutz]

On 01/30/15 05:23, Jan Beulich wrote:
>>>> On 30.01.15 at 01:52, <dslutz@verizon.com> wrote:
>> I.E. do just what no backing DM does.
> 
> _If_ this is correct, the if() modified here should be folded with the
> one a few lines up.

Ok, will fold with the one a few lines up.  As expected without this
change the guest will hang (more details below).


> But looking at the description of the commit that
> introduced this (bac0999325 "x86 hvm: Do not incorrectly retire an
> instruction emulation...", almost immediately modified by f20f3c8ece
> "x86 hvm: On failed hvm_send_assist_req(), io emulation...") I doubt
> this is really what we want, or at the very least your change
> description should explain what was wrong with the original commit.
> 

Looking at these 2 commits, it looks to me like I need to handle these
cases also.  So it looks like having hvm_send_assist_req() return an int
0, 1, & 2 is the simpler way.  V2 on the way soon.

Which is the better codding style:

1) Add #defines for the return values?
2) Just use numbers?
3) Invert the sense 0 means worked, 1 is shutdown_deferral or
   domain_crash, 2 is hvm_complete_assist_req()?


I.E. (for just adding 2):


diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 2ed4344..c565151 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -227,10 +227,19 @@ static int hvmemul_do_io(
         else
         {
             rc = X86EMUL_RETRY;
-            if ( !hvm_send_assist_req(&p) )
-                vio->io_state = HVMIO_none;
-            else if ( p_data == NULL )
+            switch ( hvm_send_assist_req(&p) )
+            {
+            case 2:
                 rc = X86EMUL_OKAY;
+                /* fall through */
+            case 0:
+                vio->io_state = HVMIO_none;
+                break;
+            case 1:
+                if ( p_data == NULL )
+                    rc = X86EMUL_OKAY;
+                break;
+            }
         }


???



I would think it would be good for the code using hvm_has_dm()
to also call on hvm_complete_assist_req().  hvm_has_dm() is a subset of
the cases that hvm_select_ioreq_server() checks for.

Based on this, I think it would be better to remove the call on
hvm_has_dm() instead of adding a call to hvm_complete_assist_req().


   -Don Slutz
P.S. Details for hang:

Using:

 static bool_t hvm_complete_assist_req(ioreq_t *p)
 {
+    HVMTRACE_1D(COMPLETE_ASSIST, p->type);
...
):

I get the trace:

CPU1  745455716325 (+    1362)  VMEXIT      [ exitcode = 0x0000001e, rIP
 = 0x00101581 ]
CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
io_state = 0 ret = 0 ]
CPU1  745455717846 (+    1521)  vlapic_accept_pic_intr [ i8259_target =
1, accept_pic_int = 1 ]
CPU1  745455718209 (+     363)  VMENTRY
CPU1  745455719568 (+    1359)  VMEXIT      [ exitcode = 0x0000001e, rIP
 = 0x00101581 ]
CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
io_state = 0 ret = 0 ]
CPU1  745455721083 (+    1515)  vlapic_accept_pic_intr [ i8259_target =
1, accept_pic_int = 1 ]
CPU1  745455721422 (+     339)  VMENTRY
CPU1  745455722781 (+    1359)  VMEXIT      [ exitcode = 0x0000001e, rIP
 = 0x00101581 ]
CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
io_state = 0 ret = 0 ]
CPU1  745455724299 (+    1518)  vlapic_accept_pic_intr [ i8259_target =
1, accept_pic_int = 1 ]
CPU1  745455724656 (+     357)  VMENTRY
CPU1  745455726009 (+    1353)  VMEXIT      [ exitcode = 0x0000001e, rIP
 = 0x00101581 ]
CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
io_state = 0 ret = 0 ]
CPU1  745455727539 (+    1530)  vlapic_accept_pic_intr [ i8259_target =
1, accept_pic_int = 1 ]
CPU1  745455727899 (+     360)  VMENTRY
CPU1  745455729276 (+    1377)  VMEXIT      [ exitcode = 0x0000001e, rIP
 = 0x00101581 ]
CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
io_state = 0 ret = 0 ]
CPU1  745455730803 (+    1527)  vlapic_accept_pic_intr [ i8259_target =
1, accept_pic_int = 1 ]
CPU1  745455731163 (+     360)  VMENTRY
CPU1  745455732525 (+    1362)  VMEXIT      [ exitcode = 0x0000001e, rIP
 = 0x00101581 ]
CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
io_state = 0 ret = 0 ]
CPU1  745455734049 (+    1524)  vlapic_accept_pic_intr [ i8259_target =
1, accept_pic_int = 1 ]
CPU1  745455734385 (+     336)  VMENTRY
...


> Jan
> 
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -228,7 +228,11 @@ static int hvmemul_do_io(
>>          {
>>              rc = X86EMUL_RETRY;
>>              if ( !hvm_send_assist_req(&p) )
>> +            {
>> +                /* Since the send failed, do not retry */
>> +                rc = X86EMUL_OKAY;
>>                  vio->io_state = HVMIO_none;
>> +            }
>>              else if ( p_data == NULL )
>>                  rc = X86EMUL_OKAY;
>>          }
>> -- 
>> 1.8.4
> 
> 
> 

Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry.

[Don Slutz]

On 01/30/15 13:17, Don Slutz wrote:
> On 01/30/15 05:23, Jan Beulich wrote:
>>>>> On 30.01.15 at 01:52, <dslutz@verizon.com> wrote:
>>> I.E. do just what no backing DM does.
>>
>> _If_ this is correct, the if() modified here should be folded with the
>> one a few lines up.
> 
> Ok, will fold with the one a few lines up.  As expected without this
> change the guest will hang (more details below).
> 
> 
>> But looking at the description of the commit that
>> introduced this (bac0999325 "x86 hvm: Do not incorrectly retire an
>> instruction emulation...", almost immediately modified by f20f3c8ece
>> "x86 hvm: On failed hvm_send_assist_req(), io emulation...") I doubt
>> this is really what we want, or at the very least your change
>> description should explain what was wrong with the original commit.
>>
> 
> Looking at these 2 commits, it looks to me like I need to handle these
> cases also.  So it looks like having hvm_send_assist_req() return an int
> 0, 1, & 2 is the simpler way.  V2 on the way soon.
> 

Soon after sending this, I came up with a 2nd way for fix.
Change hvm_has_dm() to use hvm_select_ioreq_server().  Then
the correct answer will be found, and so will not retry.

To avoid 2 calls to hvm_select_ioreq_server(), it makes
snes to me to return s.  Also adding a call to hvm_complete_assist_req()
would help.  So based on all this is is a possible v2:


commit 24eb5a839427ba80c1adf16ab656c19729f906be
Author: Don Slutz <dslutz@verizon.com>
Date:   Fri Jan 30 13:54:43 2015 -0500

    fixup use of hvm_send_assist_req

    Signed-off-by: Don Slutz <dslutz@verizon.com>

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 2ed4344..7c3c654 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -218,8 +218,11 @@ static int hvmemul_do_io(
             vio->io_state = HVMIO_handle_mmio_awaiting_completion;
         break;
     case X86EMUL_UNHANDLEABLE:
+    {
+        void *s = hvm_has_dm(curr->domain, &p);
+
         /* If there is no backing DM, just ignore accesses */
-        if ( !hvm_has_dm(curr->domain) )
+        if ( !s )
         {
             rc = X86EMUL_OKAY;
             vio->io_state = HVMIO_none;
@@ -227,11 +230,12 @@ static int hvmemul_do_io(
         else
         {
             rc = X86EMUL_RETRY;
-            if ( !hvm_send_assist_req(&p) )
+            if ( !hvm_send_assist_req(s, &p) )
                 vio->io_state = HVMIO_none;
             else if ( p_data == NULL )
                 rc = X86EMUL_OKAY;
         }
+    }
         break;
     default:
         BUG();
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 984af81..21d4a73 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2507,9 +2507,45 @@ int hvm_buffered_io_send(ioreq_t *p)
     return 1;
 }

-bool_t hvm_has_dm(struct domain *d)
+static bool_t hvm_complete_assist_req(ioreq_t *p)
 {
-    return !list_empty(&d->arch.hvm_domain.ioreq_server.list);
+    HVMTRACE_1D(COMPLETE_ASSIST, p->type);
+    ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG);
+    switch ( p->type )
+    {
+    case IOREQ_TYPE_COPY:
+    case IOREQ_TYPE_PIO:
+        if ( p->dir == IOREQ_READ )
+        {
+            if ( !p->data_is_ptr )
+                p->data = ~0ul;
+            else
+            {
+                int i, step = p->df ? -p->size : p->size;
+                uint32_t data = ~0;
+
+                for ( i = 0; i < p->count; i++ )
+                    hvm_copy_to_guest_phys(p->data + step * i, &data,
+                                           p->size);
+            }
+        }
+        /* FALLTHRU */
+    default:
+        p->state = STATE_IORESP_READY;
+        hvm_io_assist(p);
+        break;
+    }
+
+    return 1;
+}
+
+void *hvm_has_dm(struct domain *d, ioreq_t *p)
+{
+    struct hvm_ioreq_server *s = hvm_select_ioreq_server(d, p);
+
+    if ( !s )
+        hvm_complete_assist_req(p);
+    return (void *)s;
 }

 bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
@@ -2571,44 +2607,15 @@ bool_t
hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
     return 0;
 }

-static bool_t hvm_complete_assist_req(ioreq_t *p)
-{
-    HVMTRACE_1D(COMPLETE_ASSIST, p->type);
-    ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG);
-    switch ( p->type )
-    {
-    case IOREQ_TYPE_COPY:
-    case IOREQ_TYPE_PIO:
-        if ( p->dir == IOREQ_READ )
-        {
-            if ( !p->data_is_ptr )
-                p->data = ~0ul;
-            else
-            {
-                int i, step = p->df ? -p->size : p->size;
-                uint32_t data = ~0;
-
-                for ( i = 0; i < p->count; i++ )
-                    hvm_copy_to_guest_phys(p->data + step * i, &data,
-                                           p->size);
-            }
-        }
-        /* FALLTHRU */
-    default:
-        p->state = STATE_IORESP_READY;
-        hvm_io_assist(p);
-        break;
-    }
-
-    return 0; /* implicitly bins the i/o operation */
-}
-
-bool_t hvm_send_assist_req(ioreq_t *p)
+bool_t hvm_send_assist_req(void *vs, ioreq_t *p)
 {
-    struct hvm_ioreq_server *s =
hvm_select_ioreq_server(current->domain, p);
+    struct hvm_ioreq_server *s = vs;

     if ( !s )
-        return hvm_complete_assist_req(p);
+    {
+        hvm_complete_assist_req(p);
+        return 1;
+    }

     return hvm_send_assist_req_to_ioreq_server(s, p);
 }
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index e3d2d9a..1923842 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -228,7 +228,7 @@ int hvm_vcpu_cacheattr_init(struct vcpu *v);
 void hvm_vcpu_cacheattr_destroy(struct vcpu *v);
 void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip);

-bool_t hvm_send_assist_req(ioreq_t *p);
+bool_t hvm_send_assist_req(void *s, ioreq_t *p);
 void hvm_broadcast_assist_req(ioreq_t *p);

 void hvm_get_guest_pat(struct vcpu *v, u64 *guest_pat);
@@ -359,7 +359,7 @@ void hvm_hypervisor_cpuid_leaf(uint32_t sub_idx,
 void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
                                    unsigned int *ecx, unsigned int *edx);
 void hvm_migrate_timers(struct vcpu *v);
-bool_t hvm_has_dm(struct domain *d);
+void *hvm_has_dm(struct domain *d, ioreq_t *p);
 bool_t hvm_io_pending(struct vcpu *v);
 void hvm_do_resume(struct vcpu *v);
 void hvm_migrate_pirqs(struct vcpu *v);


   -Don Slutz



> Which is the better codding style:
> 
> 1) Add #defines for the return values?
> 2) Just use numbers?
> 3) Invert the sense 0 means worked, 1 is shutdown_deferral or
>    domain_crash, 2 is hvm_complete_assist_req()?
> 
> 
> I.E. (for just adding 2):
> 
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 2ed4344..c565151 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -227,10 +227,19 @@ static int hvmemul_do_io(
>          else
>          {
>              rc = X86EMUL_RETRY;
> -            if ( !hvm_send_assist_req(&p) )
> -                vio->io_state = HVMIO_none;
> -            else if ( p_data == NULL )
> +            switch ( hvm_send_assist_req(&p) )
> +            {
> +            case 2:
>                  rc = X86EMUL_OKAY;
> +                /* fall through */
> +            case 0:
> +                vio->io_state = HVMIO_none;
> +                break;
> +            case 1:
> +                if ( p_data == NULL )
> +                    rc = X86EMUL_OKAY;
> +                break;
> +            }
>          }
> 
> 
> ???
> 
> 
> 
> I would think it would be good for the code using hvm_has_dm()
> to also call on hvm_complete_assist_req().  hvm_has_dm() is a subset of
> the cases that hvm_select_ioreq_server() checks for.
> 
> Based on this, I think it would be better to remove the call on
> hvm_has_dm() instead of adding a call to hvm_complete_assist_req().
> 
> 
>    -Don Slutz
> P.S. Details for hang:
> 
> Using:
> 
>  static bool_t hvm_complete_assist_req(ioreq_t *p)
>  {
> +    HVMTRACE_1D(COMPLETE_ASSIST, p->type);
> ...
> ):
> 
> I get the trace:
> 
> CPU1  745455716325 (+    1362)  VMEXIT      [ exitcode = 0x0000001e, rIP
>  = 0x00101581 ]
> CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
> CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
> io_state = 0 ret = 0 ]
> CPU1  745455717846 (+    1521)  vlapic_accept_pic_intr [ i8259_target =
> 1, accept_pic_int = 1 ]
> CPU1  745455718209 (+     363)  VMENTRY
> CPU1  745455719568 (+    1359)  VMEXIT      [ exitcode = 0x0000001e, rIP
>  = 0x00101581 ]
> CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
> CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
> io_state = 0 ret = 0 ]
> CPU1  745455721083 (+    1515)  vlapic_accept_pic_intr [ i8259_target =
> 1, accept_pic_int = 1 ]
> CPU1  745455721422 (+     339)  VMENTRY
> CPU1  745455722781 (+    1359)  VMEXIT      [ exitcode = 0x0000001e, rIP
>  = 0x00101581 ]
> CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
> CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
> io_state = 0 ret = 0 ]
> CPU1  745455724299 (+    1518)  vlapic_accept_pic_intr [ i8259_target =
> 1, accept_pic_int = 1 ]
> CPU1  745455724656 (+     357)  VMENTRY
> CPU1  745455726009 (+    1353)  VMEXIT      [ exitcode = 0x0000001e, rIP
>  = 0x00101581 ]
> CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
> CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
> io_state = 0 ret = 0 ]
> CPU1  745455727539 (+    1530)  vlapic_accept_pic_intr [ i8259_target =
> 1, accept_pic_int = 1 ]
> CPU1  745455727899 (+     360)  VMENTRY
> CPU1  745455729276 (+    1377)  VMEXIT      [ exitcode = 0x0000001e, rIP
>  = 0x00101581 ]
> CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
> CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
> io_state = 0 ret = 0 ]
> CPU1  745455730803 (+    1527)  vlapic_accept_pic_intr [ i8259_target =
> 1, accept_pic_int = 1 ]
> CPU1  745455731163 (+     360)  VMENTRY
> CPU1  745455732525 (+    1362)  VMEXIT      [ exitcode = 0x0000001e, rIP
>  = 0x00101581 ]
> CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
> CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
> io_state = 0 ret = 0 ]
> CPU1  745455734049 (+    1524)  vlapic_accept_pic_intr [ i8259_target =
> 1, accept_pic_int = 1 ]
> CPU1  745455734385 (+     336)  VMENTRY
> ...
> 
> 
>> Jan
>>
>>> --- a/xen/arch/x86/hvm/emulate.c
>>> +++ b/xen/arch/x86/hvm/emulate.c
>>> @@ -228,7 +228,11 @@ static int hvmemul_do_io(
>>>          {
>>>              rc = X86EMUL_RETRY;
>>>              if ( !hvm_send_assist_req(&p) )
>>> +            {
>>> +                /* Since the send failed, do not retry */
>>> +                rc = X86EMUL_OKAY;
>>>                  vio->io_state = HVMIO_none;
>>> +            }
>>>              else if ( p_data == NULL )
>>>                  rc = X86EMUL_OKAY;
>>>          }
>>> -- 
>>> 1.8.4
>>
>>
>>

Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry.

[Jan Beulich]

>>> On 30.01.15 at 20:19, <dslutz@verizon.com> wrote:
> Soon after sending this, I came up with a 2nd way for fix.
> Change hvm_has_dm() to use hvm_select_ioreq_server().  Then
> the correct answer will be found, and so will not retry.
> 
> To avoid 2 calls to hvm_select_ioreq_server(), it makes
> snes to me to return s.  Also adding a call to hvm_complete_assist_req()
> would help.  So based on all this is is a possible v2:

Looks reasonable (awaiting Paul's opinion though) except for
mechanical aspects:

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2507,9 +2507,45 @@ int hvm_buffered_io_send(ioreq_t *p)
>      return 1;
>  }
> 
> -bool_t hvm_has_dm(struct domain *d)
> +static bool_t hvm_complete_assist_req(ioreq_t *p)
>  {
> -    return !list_empty(&d->arch.hvm_domain.ioreq_server.list);
> +    HVMTRACE_1D(COMPLETE_ASSIST, p->type);
> +    ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG);
> +    switch ( p->type )
> +    {
> +    case IOREQ_TYPE_COPY:
> +    case IOREQ_TYPE_PIO:
> +        if ( p->dir == IOREQ_READ )
> +        {
> +            if ( !p->data_is_ptr )
> +                p->data = ~0ul;
> +            else
> +            {
> +                int i, step = p->df ? -p->size : p->size;
> +                uint32_t data = ~0;
> +
> +                for ( i = 0; i < p->count; i++ )
> +                    hvm_copy_to_guest_phys(p->data + step * i, &data,
> +                                           p->size);
> +            }
> +        }
> +        /* FALLTHRU */
> +    default:
> +        p->state = STATE_IORESP_READY;
> +        hvm_io_assist(p);
> +        break;
> +    }
> +
> +    return 1;
> +}
> +
> +void *hvm_has_dm(struct domain *d, ioreq_t *p)

I can't see why this needs to return void * instead of a properly typed
pointer.

> @@ -2571,44 +2607,15 @@ bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
>      return 0;
>  }
> 
> -static bool_t hvm_complete_assist_req(ioreq_t *p)
> -{
> -    HVMTRACE_1D(COMPLETE_ASSIST, p->type);
> -    ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG);
> -    switch ( p->type )
> -    {
> -    case IOREQ_TYPE_COPY:
> -    case IOREQ_TYPE_PIO:
> -        if ( p->dir == IOREQ_READ )
> -        {
> -            if ( !p->data_is_ptr )
> -                p->data = ~0ul;
> -            else
> -            {
> -                int i, step = p->df ? -p->size : p->size;
> -                uint32_t data = ~0;
> -
> -                for ( i = 0; i < p->count; i++ )
> -                    hvm_copy_to_guest_phys(p->data + step * i, &data,
> -                                           p->size);
> -            }
> -        }
> -        /* FALLTHRU */
> -    default:
> -        p->state = STATE_IORESP_READY;
> -        hvm_io_assist(p);
> -        break;
> -    }
> -
> -    return 0; /* implicitly bins the i/o operation */
> -}

Moving hvm_has_dm() down here would make the patch smaller and
hence quite a bit easier to review (as one doesn't need to manually
check the differences between the much larger added/removed
pieces of code.

Jan

Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry.

[Paul Durrant]

> -----Original Message-----
> From: Don Slutz [mailto:dslutz@verizon.com]
> Sent: 30 January 2015 19:19
> To: Don Slutz; Jan Beulich; Paul Durrant
> Cc: Andrew Cooper; Ian Campbell; Wei Liu; George Dunlap; Ian Jackson;
> Stefano Stabellini; xen-devel@lists.xen.org; Keir (Xen.org)
> Subject: Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server
> failed do not retry.
> 
> On 01/30/15 13:17, Don Slutz wrote:
> > On 01/30/15 05:23, Jan Beulich wrote:
> >>>>> On 30.01.15 at 01:52, <dslutz@verizon.com> wrote:
> >>> I.E. do just what no backing DM does.
> >>
> >> _If_ this is correct, the if() modified here should be folded with the
> >> one a few lines up.
> >
> > Ok, will fold with the one a few lines up.  As expected without this
> > change the guest will hang (more details below).
> >
> >
> >> But looking at the description of the commit that
> >> introduced this (bac0999325 "x86 hvm: Do not incorrectly retire an
> >> instruction emulation...", almost immediately modified by f20f3c8ece
> >> "x86 hvm: On failed hvm_send_assist_req(), io emulation...") I doubt
> >> this is really what we want, or at the very least your change
> >> description should explain what was wrong with the original commit.
> >>
> >
> > Looking at these 2 commits, it looks to me like I need to handle these
> > cases also.  So it looks like having hvm_send_assist_req() return an int
> > 0, 1, & 2 is the simpler way.  V2 on the way soon.
> >
> 
> Soon after sending this, I came up with a 2nd way for fix.
> Change hvm_has_dm() to use hvm_select_ioreq_server().  Then
> the correct answer will be found, and so will not retry.
> 
> To avoid 2 calls to hvm_select_ioreq_server(), it makes
> snes to me to return s.  Also adding a call to hvm_complete_assist_req()
> would help.  So based on all this is is a possible v2:
> 

Yes, I think this approach does make a lot of sense.

> 
> commit 24eb5a839427ba80c1adf16ab656c19729f906be
> Author: Don Slutz <dslutz@verizon.com>
> Date:   Fri Jan 30 13:54:43 2015 -0500
> 
>     fixup use of hvm_send_assist_req
> 
>     Signed-off-by: Don Slutz <dslutz@verizon.com>
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 2ed4344..7c3c654 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -218,8 +218,11 @@ static int hvmemul_do_io(
>              vio->io_state = HVMIO_handle_mmio_awaiting_completion;
>          break;
>      case X86EMUL_UNHANDLEABLE:
> +    {
> +        void *s = hvm_has_dm(curr->domain, &p);
> +
>          /* If there is no backing DM, just ignore accesses */
> -        if ( !hvm_has_dm(curr->domain) )
> +        if ( !s )
>          {
>              rc = X86EMUL_OKAY;
>              vio->io_state = HVMIO_none;
> @@ -227,11 +230,12 @@ static int hvmemul_do_io(
>          else
>          {
>              rc = X86EMUL_RETRY;
> -            if ( !hvm_send_assist_req(&p) )
> +            if ( !hvm_send_assist_req(s, &p) )
>                  vio->io_state = HVMIO_none;
>              else if ( p_data == NULL )
>                  rc = X86EMUL_OKAY;
>          }
> +    }
>          break;
>      default:
>          BUG();
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 984af81..21d4a73 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2507,9 +2507,45 @@ int hvm_buffered_io_send(ioreq_t *p)
>      return 1;
>  }
> 
> -bool_t hvm_has_dm(struct domain *d)
> +static bool_t hvm_complete_assist_req(ioreq_t *p)

You'll need to change this to a void return as Jan requested.

>  {
> -    return !list_empty(&d->arch.hvm_domain.ioreq_server.list);
> +    HVMTRACE_1D(COMPLETE_ASSIST, p->type);
> +    ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG);
> +    switch ( p->type )
> +    {
> +    case IOREQ_TYPE_COPY:
> +    case IOREQ_TYPE_PIO:
> +        if ( p->dir == IOREQ_READ )
> +        {
> +            if ( !p->data_is_ptr )
> +                p->data = ~0ul;
> +            else
> +            {
> +                int i, step = p->df ? -p->size : p->size;
> +                uint32_t data = ~0;
> +
> +                for ( i = 0; i < p->count; i++ )
> +                    hvm_copy_to_guest_phys(p->data + step * i, &data,
> +                                           p->size);
> +            }
> +        }
> +        /* FALLTHRU */
> +    default:
> +        p->state = STATE_IORESP_READY;
> +        hvm_io_assist(p);
> +        break;
> +    }
> +
> +    return 1;
> +}
> +
> +void *hvm_has_dm(struct domain *d, ioreq_t *p)
> +{
> +    struct hvm_ioreq_server *s = hvm_select_ioreq_server(d, p);
> +
> +    if ( !s )
> +        hvm_complete_assist_req(p);

If you're going to complete the I/O here then...

> +    return (void *)s;
>  }
> 
>  bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
> @@ -2571,44 +2607,15 @@ bool_t
> hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
>      return 0;
>  }
> 
> -static bool_t hvm_complete_assist_req(ioreq_t *p)
> -{
> -    HVMTRACE_1D(COMPLETE_ASSIST, p->type);
> -    ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG);
> -    switch ( p->type )
> -    {
> -    case IOREQ_TYPE_COPY:
> -    case IOREQ_TYPE_PIO:
> -        if ( p->dir == IOREQ_READ )
> -        {
> -            if ( !p->data_is_ptr )
> -                p->data = ~0ul;
> -            else
> -            {
> -                int i, step = p->df ? -p->size : p->size;
> -                uint32_t data = ~0;
> -
> -                for ( i = 0; i < p->count; i++ )
> -                    hvm_copy_to_guest_phys(p->data + step * i, &data,
> -                                           p->size);
> -            }
> -        }
> -        /* FALLTHRU */
> -    default:
> -        p->state = STATE_IORESP_READY;
> -        hvm_io_assist(p);
> -        break;
> -    }
> -
> -    return 0; /* implicitly bins the i/o operation */
> -}
> -
> -bool_t hvm_send_assist_req(ioreq_t *p)
> +bool_t hvm_send_assist_req(void *vs, ioreq_t *p)
>  {
> -    struct hvm_ioreq_server *s =
> hvm_select_ioreq_server(current->domain, p);
> +    struct hvm_ioreq_server *s = vs;
> 
>      if ( !s )

... I think you can ASSERT(s) here, as you should never get here if an ioreq server was chosen.

> -        return hvm_complete_assist_req(p);
> +    {
> +        hvm_complete_assist_req(p);
> +        return 1;
> +    }

So, this function basically collapses down to just this call:

> 
>      return hvm_send_assist_req_to_ioreq_server(s, p);
>  }

  Paul

> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-
> x86/hvm/hvm.h
> index e3d2d9a..1923842 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -228,7 +228,7 @@ int hvm_vcpu_cacheattr_init(struct vcpu *v);
>  void hvm_vcpu_cacheattr_destroy(struct vcpu *v);
>  void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip);
> 
> -bool_t hvm_send_assist_req(ioreq_t *p);
> +bool_t hvm_send_assist_req(void *s, ioreq_t *p);
>  void hvm_broadcast_assist_req(ioreq_t *p);
> 
>  void hvm_get_guest_pat(struct vcpu *v, u64 *guest_pat);
> @@ -359,7 +359,7 @@ void hvm_hypervisor_cpuid_leaf(uint32_t sub_idx,
>  void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
>                                     unsigned int *ecx, unsigned int *edx);
>  void hvm_migrate_timers(struct vcpu *v);
> -bool_t hvm_has_dm(struct domain *d);
> +void *hvm_has_dm(struct domain *d, ioreq_t *p);
>  bool_t hvm_io_pending(struct vcpu *v);
>  void hvm_do_resume(struct vcpu *v);
>  void hvm_migrate_pirqs(struct vcpu *v);
> 
> 
>    -Don Slutz
> 
> 
> 
> > Which is the better codding style:
> >
> > 1) Add #defines for the return values?
> > 2) Just use numbers?
> > 3) Invert the sense 0 means worked, 1 is shutdown_deferral or
> >    domain_crash, 2 is hvm_complete_assist_req()?
> >
> >
> > I.E. (for just adding 2):
> >
> >
> > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> > index 2ed4344..c565151 100644
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -227,10 +227,19 @@ static int hvmemul_do_io(
> >          else
> >          {
> >              rc = X86EMUL_RETRY;
> > -            if ( !hvm_send_assist_req(&p) )
> > -                vio->io_state = HVMIO_none;
> > -            else if ( p_data == NULL )
> > +            switch ( hvm_send_assist_req(&p) )
> > +            {
> > +            case 2:
> >                  rc = X86EMUL_OKAY;
> > +                /* fall through */
> > +            case 0:
> > +                vio->io_state = HVMIO_none;
> > +                break;
> > +            case 1:
> > +                if ( p_data == NULL )
> > +                    rc = X86EMUL_OKAY;
> > +                break;
> > +            }
> >          }
> >
> >
> > ???
> >
> >
> >
> > I would think it would be good for the code using hvm_has_dm()
> > to also call on hvm_complete_assist_req().  hvm_has_dm() is a subset of
> > the cases that hvm_select_ioreq_server() checks for.
> >
> > Based on this, I think it would be better to remove the call on
> > hvm_has_dm() instead of adding a call to hvm_complete_assist_req().
> >
> >
> >    -Don Slutz
> > P.S. Details for hang:
> >
> > Using:
> >
> >  static bool_t hvm_complete_assist_req(ioreq_t *p)
> >  {
> > +    HVMTRACE_1D(COMPLETE_ASSIST, p->type);
> > ...
> > ):
> >
> > I get the trace:
> >
> > CPU1  745455716325 (+    1362)  VMEXIT      [ exitcode = 0x0000001e, rIP
> >  = 0x00101581 ]
> > CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
> > CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
> > io_state = 0 ret = 0 ]
> > CPU1  745455717846 (+    1521)  vlapic_accept_pic_intr [ i8259_target =
> > 1, accept_pic_int = 1 ]
> > CPU1  745455718209 (+     363)  VMENTRY
> > CPU1  745455719568 (+    1359)  VMEXIT      [ exitcode = 0x0000001e, rIP
> >  = 0x00101581 ]
> > CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
> > CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
> > io_state = 0 ret = 0 ]
> > CPU1  745455721083 (+    1515)  vlapic_accept_pic_intr [ i8259_target =
> > 1, accept_pic_int = 1 ]
> > CPU1  745455721422 (+     339)  VMENTRY
> > CPU1  745455722781 (+    1359)  VMEXIT      [ exitcode = 0x0000001e, rIP
> >  = 0x00101581 ]
> > CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
> > CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
> > io_state = 0 ret = 0 ]
> > CPU1  745455724299 (+    1518)  vlapic_accept_pic_intr [ i8259_target =
> > 1, accept_pic_int = 1 ]
> > CPU1  745455724656 (+     357)  VMENTRY
> > CPU1  745455726009 (+    1353)  VMEXIT      [ exitcode = 0x0000001e, rIP
> >  = 0x00101581 ]
> > CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
> > CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
> > io_state = 0 ret = 0 ]
> > CPU1  745455727539 (+    1530)  vlapic_accept_pic_intr [ i8259_target =
> > 1, accept_pic_int = 1 ]
> > CPU1  745455727899 (+     360)  VMENTRY
> > CPU1  745455729276 (+    1377)  VMEXIT      [ exitcode = 0x0000001e, rIP
> >  = 0x00101581 ]
> > CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
> > CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
> > io_state = 0 ret = 0 ]
> > CPU1  745455730803 (+    1527)  vlapic_accept_pic_intr [ i8259_target =
> > 1, accept_pic_int = 1 ]
> > CPU1  745455731163 (+     360)  VMENTRY
> > CPU1  745455732525 (+    1362)  VMEXIT      [ exitcode = 0x0000001e, rIP
> >  = 0x00101581 ]
> > CPU1  0 (+       0)  COMPLETE_ASSIST [ type = 0 ]
> > CPU1  0 (+       0)  HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1
> > io_state = 0 ret = 0 ]
> > CPU1  745455734049 (+    1524)  vlapic_accept_pic_intr [ i8259_target =
> > 1, accept_pic_int = 1 ]
> > CPU1  745455734385 (+     336)  VMENTRY
> > ...
> >
> >
> >> Jan
> >>
> >>> --- a/xen/arch/x86/hvm/emulate.c
> >>> +++ b/xen/arch/x86/hvm/emulate.c
> >>> @@ -228,7 +228,11 @@ static int hvmemul_do_io(
> >>>          {
> >>>              rc = X86EMUL_RETRY;
> >>>              if ( !hvm_send_assist_req(&p) )
> >>> +            {
> >>> +                /* Since the send failed, do not retry */
> >>> +                rc = X86EMUL_OKAY;
> >>>                  vio->io_state = HVMIO_none;
> >>> +            }
> >>>              else if ( p_data == NULL )
> >>>                  rc = X86EMUL_OKAY;
> >>>          }
> >>> --
> >>> 1.8.4
> >>
> >>
> >>

Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry.

[Jan Beulich]

>>> On 02.02.15 at 10:51, <Paul.Durrant@citrix.com> wrote:
>> From: Don Slutz [mailto:dslutz@verizon.com]
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -2507,9 +2507,45 @@ int hvm_buffered_io_send(ioreq_t *p)
>>      return 1;
>>  }
>> 
>> -bool_t hvm_has_dm(struct domain *d)
>> +static bool_t hvm_complete_assist_req(ioreq_t *p)
> 
> You'll need to change this to a void return as Jan requested.

I don't think I did.

Jan

Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry.

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 02 February 2015 10:04
> To: Paul Durrant; Don Slutz
> Cc: Andrew Cooper; George Dunlap; Ian Campbell; Ian Jackson; Stefano
> Stabellini; Wei Liu; xen-devel@lists.xen.org; Keir (Xen.org)
> Subject: RE: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server
> failed do not retry.
> 
> >>> On 02.02.15 at 10:51, <Paul.Durrant@citrix.com> wrote:
> >> From: Don Slutz [mailto:dslutz@verizon.com]
> >> --- a/xen/arch/x86/hvm/hvm.c
> >> +++ b/xen/arch/x86/hvm/hvm.c
> >> @@ -2507,9 +2507,45 @@ int hvm_buffered_io_send(ioreq_t *p)
> >>      return 1;
> >>  }
> >>
> >> -bool_t hvm_has_dm(struct domain *d)
> >> +static bool_t hvm_complete_assist_req(ioreq_t *p)
> >
> > You'll need to change this to a void return as Jan requested.
> 
> I don't think I did.
> 

Oh, sorry. I thought you suggested that the constant return of 1 from hvm_complete_assist_req() was a bad idea.

  Paul

> Jan

Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry.

[Jan Beulich]

>>> On 02.02.15 at 11:06, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 02 February 2015 10:04
>> To: Paul Durrant; Don Slutz
>> Cc: Andrew Cooper; George Dunlap; Ian Campbell; Ian Jackson; Stefano
>> Stabellini; Wei Liu; xen-devel@lists.xen.org; Keir (Xen.org)
>> Subject: RE: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server
>> failed do not retry.
>> 
>> >>> On 02.02.15 at 10:51, <Paul.Durrant@citrix.com> wrote:
>> >> From: Don Slutz [mailto:dslutz@verizon.com]
>> >> --- a/xen/arch/x86/hvm/hvm.c
>> >> +++ b/xen/arch/x86/hvm/hvm.c
>> >> @@ -2507,9 +2507,45 @@ int hvm_buffered_io_send(ioreq_t *p)
>> >>      return 1;
>> >>  }
>> >>
>> >> -bool_t hvm_has_dm(struct domain *d)
>> >> +static bool_t hvm_complete_assist_req(ioreq_t *p)
>> >
>> > You'll need to change this to a void return as Jan requested.
>> 
>> I don't think I did.
> 
> Oh, sorry. I thought you suggested that the constant return of 1 from 
> hvm_complete_assist_req() was a bad idea.

Oh, I see, but that was in another thread, and I really suggested to
remove the function altogether (folding it back into its only caller).

Jan

Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry.

[Don Slutz]

On 02/02/15 03:36, Jan Beulich wrote:
>>>> On 30.01.15 at 20:19, <dslutz@verizon.com> wrote:
>> Soon after sending this, I came up with a 2nd way for fix.
>> Change hvm_has_dm() to use hvm_select_ioreq_server().  Then
>> the correct answer will be found, and so will not retry.
>>
>> To avoid 2 calls to hvm_select_ioreq_server(), it makes
>> snes to me to return s.  Also adding a call to hvm_complete_assist_req()
>> would help.  So based on all this is is a possible v2:
> 
> Looks reasonable (awaiting Paul's opinion though) except for
> mechanical aspects:
> 
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -2507,9 +2507,45 @@ int hvm_buffered_io_send(ioreq_t *p)
>>      return 1;
>>  }
>>
>> -bool_t hvm_has_dm(struct domain *d)
>> +static bool_t hvm_complete_assist_req(ioreq_t *p)
>>  {
...
>> +}
>> +
>> +void *hvm_has_dm(struct domain *d, ioreq_t *p)
> 
> I can't see why this needs to return void * instead of a properly typed
> pointer.
> 

Using "struct hvm_ioreq_server *s" did not build:

In file included from /home/don/xen-master/xen/include/asm/hvm/irq.h:26:0,
                 from /home/don/xen-master/xen/include/asm/hvm/vpt.h:32,
                 from /home/don/xen-master/xen/include/asm/hvm/vlapic.h:27,
                 from /home/don/xen-master/xen/include/asm/hvm/vcpu.h:25,
                 from /home/don/xen-master/xen/include/asm/domain.h:7,
                 from /home/don/xen-master/xen/include/xen/domain.h:6,
                 from /home/don/xen-master/xen/include/xen/sched.h:10,
                 from x86_64/asm-offsets.c:10:
/home/don/xen-master/xen/include/asm/hvm/hvm.h:231:35: error: 'struct
hvm_ioreq_server' declared inside parameter list [-Werror]
/home/don/xen-master/xen/include/asm/hvm/hvm.h:231:35: error: its scope
is only this definition or declaration, which is probably not what you
want [-Werror]
cc1: all warnings being treated as errors

So I went with void * to check the that the code would pass my unit
testing.


No clue why I did not say:

+struct hvm_ioreq_server;
+bool_t hvm_send_assist_req(struct hvm_ioreq_server *s, ioreq_t *p);

Unless I hear otherwise, this is the way I will go.



>> @@ -2571,44 +2607,15 @@ bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
>>      return 0;
>>  }
>>
>> -static bool_t hvm_complete_assist_req(ioreq_t *p)
>> -{
...
>> -    return 0; /* implicitly bins the i/o operation */
>> -}
> 
> Moving hvm_has_dm() down here would make the patch smaller and
> hence quite a bit easier to review (as one doesn't need to manually
> check the differences between the much larger added/removed
> pieces of code.
> 

Will do.

   -Don Slutz


> Jan
> 

Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry.

[Don Slutz]

On 02/02/15 05:14, Jan Beulich wrote:
>>>> On 02.02.15 at 11:06, <Paul.Durrant@citrix.com> wrote:
>>>  -----Original Message-----
>>> From: Jan Beulich [mailto:JBeulich@suse.com]
>>> Sent: 02 February 2015 10:04
>>> To: Paul Durrant; Don Slutz
>>> Cc: Andrew Cooper; George Dunlap; Ian Campbell; Ian Jackson; Stefano
>>> Stabellini; Wei Liu; xen-devel@lists.xen.org; Keir (Xen.org)
>>> Subject: RE: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server
>>> failed do not retry.
>>>
>>>>>> On 02.02.15 at 10:51, <Paul.Durrant@citrix.com> wrote:
>>>>> From: Don Slutz [mailto:dslutz@verizon.com]
>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>> @@ -2507,9 +2507,45 @@ int hvm_buffered_io_send(ioreq_t *p)
>>>>>      return 1;
>>>>>  }
>>>>>
>>>>> -bool_t hvm_has_dm(struct domain *d)
>>>>> +static bool_t hvm_complete_assist_req(ioreq_t *p)
>>>>
>>>> You'll need to change this to a void return as Jan requested.
>>>
>>> I don't think I did.
>>
>> Oh, sorry. I thought you suggested that the constant return of 1 from 
>> hvm_complete_assist_req() was a bad idea.
> 
> Oh, I see, but that was in another thread, and I really suggested to
> remove the function altogether (folding it back into its only caller).
> 

I was planing to also do this.  This is why the code was not in patch form.

   -Don Slutz

> Jan
>