[PATCH v2 0/3] Skip unneeded VMENTRY & VMEXIT

[PATCH v2 0/3] Skip unneeded VMENTRY & VMEXIT

[Don Slutz]

Changes v2:

  Paul Durrant:
    I think the two changes only make sense in combination.
      folded old #3 and old #5 into #3.
    Actually that comment is not right. The operation is not binned;
    it's just already been done.
      Comment has been dropped.
    I think you can ASSERT(s) here, as you should never get here ...
      Dropped call to hvm_complete_assist_req(), added ASSERT.
    So basically collapses down to just this call.
      Yes, so changed to call on hvm_send_assist_req_to_ioreq_server()
      and removed hvm_send_assist_req() which had 1 caller.

  Jan Beulich:
    The change on what to do when hvm_send_assist_req() fails is bad.
      That is correct.  Made hvm_has_dm() do full checking so
      that the extra VMEXIT and VMENTRY can be skipped.
    Add Suggested-by to old #4
      Done (Now #2)
    hvm_complete_assist_req() be a separate function yet having only
    a single caller ...
      Folded hvm_complete_assist_req() in to hvm_has_dm() which
      is the only place it is called.


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 (3):
  xentrace: Adjust IOPORT & MMIO format
  hvm_complete_assist_req: We should not be able to get here on
    IOREQ_TYPE_PCI_CONFIG
  hvm_has_dm: Do a full check for backing DM

 tools/xentrace/formats        |  8 ++++----
 xen/arch/x86/hvm/emulate.c    |  8 ++++++--
 xen/arch/x86/hvm/hvm.c        | 26 +++++++++-----------------
 xen/include/asm-x86/hvm/hvm.h |  6 ++++--
 4 files changed, 23 insertions(+), 25 deletions(-)

-- 
1.8.4

[PATCH v2 1/3] 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 da658bf..5d7b72a 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 v2 2/3] hvm_complete_assist_req: We should not be able to get here on IOREQ_TYPE_PCI_CONFIG

[Don Slutz]

Suggested-by: Paul Durrant <paul.durrant@citrix.com>
Signed-off-by: Don Slutz <dslutz@verizon.com>
---
v2:
   Add Suggested-by

 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 v2 3/3] hvm_has_dm: Do a full check for backing DM

[Don Slutz]

This saves a VMENTRY and a VMEXIT since we not longer retry the
ioport read on backing DM not handling a given ioreq.

To only call on hvm_select_ioreq_server() once in this code path, return s.
Also switch to hvm_send_assist_req_to_ioreq_server().

Signed-off-by: Don Slutz <dslutz@verizon.com>
---
v2:
  Paul Durrant:
    I think the two changes only make sense in combination.
      folded old #3 and old #5 into #3.
    Actually that comment is not right. The operation is not binned;
    it's just already been done.
      Comment has been dropped.
    I think you can ASSERT(s) here, as you should never get here ...
      Dropped call to hvm_complete_assist_req(), added ASSERT.
    So basically collapses down to just this call.
      Yes, so changed to call on hvm_send_assist_req_to_ioreq_server()
      and removed hvm_send_assist_req() which had 1 caller.

  Jan Beulich:
    The change on what to do when hvm_send_assist_req() fails is bad.
      That is correct.  Made hvm_has_dm() do full checking so
      that the extra VMEXIT and VMENTRY can be skipped.
    hvm_complete_assist_req() be a separate function yet having only
    a single caller ...
      Folded hvm_complete_assist_req() in to hvm_has_dm() which
      is the only place it is called.


 xen/arch/x86/hvm/emulate.c    |  8 ++++++--
 xen/arch/x86/hvm/hvm.c        | 25 ++++++++-----------------
 xen/include/asm-x86/hvm/hvm.h |  6 ++++--
 3 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 2ed4344..dca6d2e 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:
+    {
+        struct hvm_ioreq_server *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_to_ioreq_server(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 6f7b407..81bdf18 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2507,11 +2507,6 @@ int hvm_buffered_io_send(ioreq_t *p)
     return 1;
 }
 
-bool_t hvm_has_dm(struct domain *d)
-{
-    return !list_empty(&d->arch.hvm_domain.ioreq_server.list);
-}
-
 bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
                                            ioreq_t *proto_p)
 {
@@ -2519,6 +2514,7 @@ bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
     struct domain *d = curr->domain;
     struct hvm_ioreq_vcpu *sv;
 
+    ASSERT(s);
     if ( unlikely(!vcpu_start_shutdown_deferral(curr)) )
         return 0; /* implicitly bins the i/o operation */
 
@@ -2571,8 +2567,13 @@ 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)
+struct hvm_ioreq_server *hvm_has_dm(struct domain *d, ioreq_t *p)
 {
+    struct hvm_ioreq_server *s = hvm_select_ioreq_server(current->domain, p);
+
+    if ( s )
+        return s;
+
     ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG);
     switch ( p->type )
     {
@@ -2599,17 +2600,7 @@ static bool_t hvm_complete_assist_req(ioreq_t *p)
         break;
     }
 
-    return 1;
-}
-
-bool_t hvm_send_assist_req(ioreq_t *p)
-{
-    struct hvm_ioreq_server *s = hvm_select_ioreq_server(current->domain, p);
-
-    if ( !s )
-        return hvm_complete_assist_req(p);
-
-    return hvm_send_assist_req_to_ioreq_server(s, p);
+    return NULL;
 }
 
 void hvm_broadcast_assist_req(ioreq_t *p)
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index e3d2d9a..4bcc61c 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -228,7 +228,9 @@ 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);
+struct hvm_ioreq_server;
+bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *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 +361,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);
+struct hvm_ioreq_server *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);
-- 
1.8.4

Re: [PATCH v2 3/3] hvm_has_dm: Do a full check for backing DM

[Paul Durrant]

> -----Original Message-----
> From: Don Slutz [mailto:dslutz@verizon.com]
> Sent: 02 February 2015 15:22
> To: xen-devel@lists.xen.org; Jan Beulich; Paul Durrant
> Cc: Andrew Cooper; George Dunlap; Ian Campbell; Ian Jackson; Keir
> (Xen.org); Stefano Stabellini; Wei Liu; Don Slutz
> Subject: [PATCH v2 3/3] hvm_has_dm: Do a full check for backing DM
> 
> This saves a VMENTRY and a VMEXIT since we not longer retry the
> ioport read on backing DM not handling a given ioreq.
> 
> To only call on hvm_select_ioreq_server() once in this code path, return s.
> Also switch to hvm_send_assist_req_to_ioreq_server().
> 
> Signed-off-by: Don Slutz <dslutz@verizon.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com> 

> ---
> v2:
>   Paul Durrant:
>     I think the two changes only make sense in combination.
>       folded old #3 and old #5 into #3.
>     Actually that comment is not right. The operation is not binned;
>     it's just already been done.
>       Comment has been dropped.
>     I think you can ASSERT(s) here, as you should never get here ...
>       Dropped call to hvm_complete_assist_req(), added ASSERT.
>     So basically collapses down to just this call.
>       Yes, so changed to call on hvm_send_assist_req_to_ioreq_server()
>       and removed hvm_send_assist_req() which had 1 caller.
> 
>   Jan Beulich:
>     The change on what to do when hvm_send_assist_req() fails is bad.
>       That is correct.  Made hvm_has_dm() do full checking so
>       that the extra VMEXIT and VMENTRY can be skipped.
>     hvm_complete_assist_req() be a separate function yet having only
>     a single caller ...
>       Folded hvm_complete_assist_req() in to hvm_has_dm() which
>       is the only place it is called.
> 
> 
>  xen/arch/x86/hvm/emulate.c    |  8 ++++++--
>  xen/arch/x86/hvm/hvm.c        | 25 ++++++++-----------------
>  xen/include/asm-x86/hvm/hvm.h |  6 ++++--
>  3 files changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 2ed4344..dca6d2e 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:
> +    {
> +        struct hvm_ioreq_server *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_to_ioreq_server(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 6f7b407..81bdf18 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2507,11 +2507,6 @@ int hvm_buffered_io_send(ioreq_t *p)
>      return 1;
>  }
> 
> -bool_t hvm_has_dm(struct domain *d)
> -{
> -    return !list_empty(&d->arch.hvm_domain.ioreq_server.list);
> -}
> -
>  bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
>                                             ioreq_t *proto_p)
>  {
> @@ -2519,6 +2514,7 @@ bool_t
> hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
>      struct domain *d = curr->domain;
>      struct hvm_ioreq_vcpu *sv;
> 
> +    ASSERT(s);
>      if ( unlikely(!vcpu_start_shutdown_deferral(curr)) )
>          return 0; /* implicitly bins the i/o operation */
> 
> @@ -2571,8 +2567,13 @@ 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)
> +struct hvm_ioreq_server *hvm_has_dm(struct domain *d, ioreq_t *p)
>  {
> +    struct hvm_ioreq_server *s = hvm_select_ioreq_server(current-
> >domain, p);
> +
> +    if ( s )
> +        return s;
> +
>      ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG);
>      switch ( p->type )
>      {
> @@ -2599,17 +2600,7 @@ static bool_t hvm_complete_assist_req(ioreq_t
> *p)
>          break;
>      }
> 
> -    return 1;
> -}
> -
> -bool_t hvm_send_assist_req(ioreq_t *p)
> -{
> -    struct hvm_ioreq_server *s = hvm_select_ioreq_server(current-
> >domain, p);
> -
> -    if ( !s )
> -        return hvm_complete_assist_req(p);
> -
> -    return hvm_send_assist_req_to_ioreq_server(s, p);
> +    return NULL;
>  }
> 
>  void hvm_broadcast_assist_req(ioreq_t *p)
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-
> x86/hvm/hvm.h
> index e3d2d9a..4bcc61c 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -228,7 +228,9 @@ 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);
> +struct hvm_ioreq_server;
> +bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server
> *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 +361,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);
> +struct hvm_ioreq_server *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);
> --
> 1.8.4

Re: [PATCH v2 2/3] hvm_complete_assist_req: We should not be able to get here on IOREQ_TYPE_PCI_CONFIG

[Jan Beulich]

>>> On 02.02.15 at 16:22, <dslutz@verizon.com> wrote:
> --- 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:

The better variant would seem to be case IOREQ_TYPE_PCI_CONFIG
with ASSERT_UNREACHABLE().

Jan

Re: [PATCH v2 3/3] hvm_has_dm: Do a full check for backing DM

[Jan Beulich]

>>> On 02.02.15 at 16:22, <dslutz@verizon.com> wrote:
>   Jan Beulich:
>     The change on what to do when hvm_send_assist_req() fails is bad.
>       That is correct.  Made hvm_has_dm() do full checking so
>       that the extra VMEXIT and VMENTRY can be skipped.
>     hvm_complete_assist_req() be a separate function yet having only
>     a single caller ...
>       Folded hvm_complete_assist_req() in to hvm_has_dm() which
>       is the only place it is called.

This makes pretty little sense to me - previously the only caller was
hvm_send_assist_req(), folding into which would make sense. But
moving this code into hvm_has_dm() doesn't look right both from
an abstract perspective (completely contrary to the function's name)
and from what operations get carried out when:

> --- 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:
> +    {
> +        struct hvm_ioreq_server *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;

You alter the flow here, but leave the (now stale) comment
untouched - !hvm_has_dm() no longer has the original meaning.

> @@ -227,11 +230,12 @@ static int hvmemul_do_io(
>          else
>          {
>              rc = X86EMUL_RETRY;
> -            if ( !hvm_send_assist_req(&p) )
> +            if ( !hvm_send_assist_req_to_ioreq_server(s, &p) )
>                  vio->io_state = HVMIO_none;
>              else if ( p_data == NULL )
>                  rc = X86EMUL_OKAY;
>          }

In particular, the returning of X86EMUL_RETRY vs X86EMUL_OKAY
now change for the case where !s but also
!list_empty(&d->arch.hvm_domain.ioreq_server.list). While this
_may_ be intended, the description of the patch still doesn't make
clear why this is so (again keeping in mind why the behavior prior to
this patch was done in this particular way).

So in the end, considering that Paul approved of what you do, maybe
all is well and it's just one step too many folded together for me to
grok. In which case - please take the time to describe your changes
suitably; this isn't the first time I have to ask you to do so.

> +    }
>          break;

For indentation to be meaningful, the closing brace should follow
the break statement.

> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -228,7 +228,9 @@ 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);
> +struct hvm_ioreq_server;

I think you could avoid this by simply moving up here the
hvm_has_dm() declaration.

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

If, with the comments above in mind, you still intend to remove
hvm_send_assist_req(), I'd much favor you renaming
hvm_send_assist_req_to_ioreq_server() to hvm_send_assist_req().

Jan

Re: [PATCH v2 2/3] hvm_complete_assist_req: We should not be able to get here on IOREQ_TYPE_PCI_CONFIG

[Don Slutz]

On 02/03/15 06:05, Jan Beulich wrote:
>>>> On 02.02.15 at 16:22, <dslutz@verizon.com> wrote:
>> --- 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:
> 
> The better variant would seem to be case IOREQ_TYPE_PCI_CONFIG
> with ASSERT_UNREACHABLE().
> 

Ok, Will change (since there will be a v3)
    -Don Slutz


> Jan
> 

Re: [PATCH v2 3/3] hvm_has_dm: Do a full check for backing DM

[Don Slutz]

On 02/03/15 09:58, Jan Beulich wrote:
>>>> On 02.02.15 at 16:22, <dslutz@verizon.com> wrote:
>>   Jan Beulich:
>>     The change on what to do when hvm_send_assist_req() fails is bad.
>>       That is correct.  Made hvm_has_dm() do full checking so
>>       that the extra VMEXIT and VMENTRY can be skipped.
>>     hvm_complete_assist_req() be a separate function yet having only
>>     a single caller ...
>>       Folded hvm_complete_assist_req() in to hvm_has_dm() which
>>       is the only place it is called.
> 
> This makes pretty little sense to me - previously the only caller was
> hvm_send_assist_req(), folding into which would make sense. But
> moving this code into hvm_has_dm() doesn't look right both from
> an abstract perspective (completely contrary to the function's name)
> and from what operations get carried out when:
> 

Ok, will be working on a much better commit message.  Do you want the
new commit message copied here (in the summary of the changes), or just
that fact that there is a new commit message?


>> --- 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:
>> +    {
>> +        struct hvm_ioreq_server *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;
> 
> You alter the flow here, but leave the (now stale) comment
> untouched - !hvm_has_dm() no longer has the original meaning.
> 

I will say that the comment is not stale. Also this code flow change
is what I am trying to do in this patch.  Since I am talking about
Paul's code also, I might be off base.

It is true that hvm_has_dm() has changed.  However the change
is from "there is no backing DM" to "there is no backing DM
that will process this request".  To me they mean the same thing.

That is why I did not change the name and did not see a need
to change the comment.  However I can adjust the comment to be much longer.

How does:

/*
 * If there is no backing DM, or there is no backing DM to handle
 * this request, act just like a missing device.
 */

look as a new comment?

Would it help to s/hvm_has_dm/hvm_has_dm_for_req/?

Now I should have included that this patch has also changed for
PVH guests, the result of doing a inl (etc) is now to return 1's instead
of not changing eax.  This also includes the mmio cases just like
hvm_complete_assist_req() use to do.

Clearly I should have included this in the commit message.

Paul did say (in another thread):
"Actually one thing that's in your patch series does 'help' a bit. The
folding together of the has_dm check and the IO completion means that at
least hvmloader gets back f-s rather than 0-s for the emulation that
doesn't hit :-/ "


In my understanding, a proper PVH guest will not attempt to use
inl to QEMU devices.  The commit:

commit 68209bce0f551dcae991877ffd58211498bb0755
Author: George Dunlap <george.dunlap@eu.citrix.com>
Date:   Wed Nov 13 09:29:02 2013 +0100

    pvh: tolerate HVM guests having no ioreq page

    PVH guests don't have a backing device model emulator (qemu); just
    tolerate this situation explicitly, rather than special-casing PVH.
...

is where this comment comes from and leads me to thinking that having
inl be more like real hardware is not a problem.  However, if you want
to have no change to PVH, that code can be added.  It is a matter
of checking for list_empty() just like hvm_had_dm() use to do.


My understanding of what is going on here started with:

commit 434ae6517af2388572fa24c6c9904822cc75180b
Author: Paul Durrant <paul.durrant@citrix.com>
Date:   Mon Jun 2 09:40:43 2014 +0200

    ioreq-server: add support for multiple servers
...


However instead of the statement:

    The previous single ioreq server that was created on demand now
    becomes the default server and an API is created to allow secondary
    servers, which handle specific IO ranges or PCI devices, to be
    added.

is not how QEMU was changed.  The change to QEMU:

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

causes QEMU to register a ioreq server that does not handle
all requests.  And no default ioreq server is created.

This lead me to (which did not get sent to xen-devel):

------------------------------------------------------------

References: <1417776605-36309-1-git-send-email-paul.durrant@citrix.com>
	<1417776605-36309-3-git-send-email-paul.durrant@citrix.com>
In-Reply-To: <1417776605-36309-3-git-send-email-paul.durrant@citrix.com>
Subject: Re: [Qemu-devel] [PATCH v5 2/2] Xen: Use the ioreq-server API
when available
Date: Wed, 28 Jan 2015 14:32:04 -0500
From: Don Slutz <dslutz@verizon.com>
To: Paul Durrant <paul.durrant@citrix.com>, qemu-devel@nongnu.org,
Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Peter Maydell <peter.maydell@linaro.org>, Olaf Hering
<olaf@aepfle.de>, Alexey Kardashevskiy <aik@ozlabs.ru>, Stefan Weil
<sw@weilnetz.de>, Michael Tokarev <mjt@tls.msk.ru>, Alexander Graf
<agraf@suse.de>, Gerd Hoffmann <kraxel@redhat.com>, Stefan Hajnoczi
<stefanha@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>

On 12/05/14 05:50, Paul Durrant wrote:
> The ioreq-server API added to Xen 4.5 offers better security than
> the existing Xen/QEMU interface because the shared pages that are
> used to pass emulation request/results back and forth are removed
> from the guest's memory space before any requests are serviced.
> This prevents the guest from mapping these pages (they are in a
> well known location) and attempting to attack QEMU by synthesizing
> its own request structures. Hence, this patch modifies configure
> to detect whether the API is available, and adds the necessary
> code to use the API if it is.

This patch (which is now on xenbits qemu staging) is causing me
issues.

So far I have tracked it back to hvm_select_ioreq_server()
which selects the "default_ioreq_server".  Since I have one 1
QEMU, it is both the "default_ioreq_server" and an enabled
2nd ioreq_server.  I am continuing to understand why my changes
are causing this.  More below.

...


Which did lead to:


Message-ID: <54CAEF19.1030205@terremark.com>
References: <1417776605-36309-1-git-send-email-paul.durrant@citrix.com>	
 <1417776605-36309-3-git-send-email-paul.durrant@citrix.com>
 <54C93934.2020707@terremark.com>
 <54C97947.5080908@terremark.com>
 <54C98586.3020509@terremark.com>
 <9AAE0902D5BC7E449B7C8E4E778ABCD0257DC103@AMSPEX01CL01.citrite.net>
 <54CA868A.6050407@terremark.com>
In-Reply-To: <54CA868A.6050407@terremark.com>
Subject: Re: [Qemu-devel] [PATCH v5 2/2] Xen: Use the ioreq-server API
when available
Date: Thu, 29 Jan 2015 21:40:25 -0500
From: Don Slutz <dslutz@verizon.com>
To: Paul Durrant <Paul.Durrant@citrix.com>, Don Slutz
<dslutz@verizon.com>, qemu-devel@nongnu.org <qemu-devel@nongnu.org>,
Stefano Stabellini <Stefano.Stabellini@citrix.com>
CC: Peter Maydell <peter.maydell@linaro.org>, Olaf Hering
<olaf@aepfle.de>, Alexey Kardashevskiy <aik@ozlabs.ru>, Stefan Weil
<sw@weilnetz.de>, Michael Tokarev <mjt@tls.msk.ru>, Alexander Graf
<agraf@suse.de>, Gerd Hoffmann <kraxel@redhat.com>, Stefan Hajnoczi
<stefanha@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>,
xen-devel@lists.xen.org <xen-devel@lists.xen.org>
...
The one line patch:


>From 5269b1fb947f207057ca69e320c79b397db3e8f5 Mon Sep 17 00:00:00 2001
From: Don Slutz <dslutz@verizon.com>
Date: Thu, 29 Jan 2015 21:24:05 -0500
Subject: [PATCH] Prevent hang if read of HVM_PARAM_IOREQ_PFN,
 HVM_PARAM_BUFIOREQ_PFN, HVM_PARAM_BUFIOREQ_EVTCHN is done
 before hvmloader starts.

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 bad410e..7ac4b45 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -993,7 +993,7 @@ static int hvm_create_ioreq_server(struct domain *d,
domid_t domid,
     spin_lock(&d->arch.hvm_domain.ioreq_server.lock);

     rc = -EEXIST;
-    if ( is_default && d->arch.hvm_domain.default_ioreq_server != NULL )
+    if ( is_default && !list_empty(&d->arch.hvm_domain.ioreq_server.list) )
         goto fail2;

     rc = hvm_ioreq_server_init(s, d, domid, is_default, handle_bufioreq,
-- 1.7.11.7

Does "fix this" but no idea if this is the way to go.

------------------------------------------------------------------

Which did end up on xen-devel -- so far no progress on that
bug (this patch set does not help here).


To sum this all up, there is no default_ioreq_server I.E.
d->arch.hvm_domain.default_ioreq_server is NULL in the normal case.

So hvm_select_ioreq_server() will either return a struct
hvm_ioreq_server* that will handle the request or NULL.

Based on this, currently hvm_send_assist_req() will either send the
request to QEMU or call on hvm_complete_assist_req().  Now currently
hvm_complete_assist_req() returns 1 and so hvm_send_assist_req()
will return 1 in the 2 cases:

1) The request was sent to QEMU

2) The request was completed by hvm_complete_assist_req

In either case, the code in hvmemul_do_io() will return X86EMUL_RETRY
and so you will get to:

  hvm_do_resume()
   -> hvm_wait_for_io() which checks the state of the ioreq.

For case 1, you will (most likely) call wait_on_xen_event_channel().
For case 2, you will just return.

Continuing with case 2, you get to VMENTRY which does a VMEXIT since
the rip has not changed.  This time around vio->io_state will be
HVMIO_completed, and so you will get to finish_access.

The whole point of this change is to avoid the extra VMENTRY and VMEXIT.

As you (correctly) pointed out:
"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, "


Handling the 3 possible returns from hvm_send_assist_req() was the
"correct" way to go.


>> @@ -227,11 +230,12 @@ static int hvmemul_do_io(
>>          else
>>          {
>>              rc = X86EMUL_RETRY;
>> -            if ( !hvm_send_assist_req(&p) )
>> +            if ( !hvm_send_assist_req_to_ioreq_server(s, &p) )
>>                  vio->io_state = HVMIO_none;
>>              else if ( p_data == NULL )
>>                  rc = X86EMUL_OKAY;
>>          }
> 
> In particular, the returning of X86EMUL_RETRY vs X86EMUL_OKAY
> now change for the case where !s but also
> !list_empty(&d->arch.hvm_domain.ioreq_server.list). While this
> _may_ be intended, the description of the patch still doesn't make
> clear why this is so (again keeping in mind why the behavior prior to
> this patch was done in this particular way).
> 

This is the intended change.  Clearly I did not describe this.


> So in the end, considering that Paul approved of what you do, maybe
> all is well and it's just one step too many folded together for me to
> grok. In which case - please take the time to describe your changes
> suitably; this isn't the first time I have to ask you to do so.
> 

Sorry, I missed this this when adjusting to v2.  I have been having
the issue of "short", "cryptic", "un-readable", etc commit messages (or
the equivalent) for many years.  So please do not stop telling me to
fix them up.

Here is the proposed new commit message:

-----
Subject: [PATCH] hvmemul_do_io: Do not retry if no ioreq server exists for
 this I/O.

This saves a VMENTRY and a VMEXIT since we no longer retry the
ioport read on backing DM not handling a given ioreq.

There are 2 case about "no ioreq server exists for this I/O":

1) No ioreq servers (PVH case)
2) No ioreq servers for this I/O (non PVH case)

The routine hvm_has_dm() used to check for the empty list, the PVH
case (#1).

By changing hvm_has_dm() to check for both cases (the 1st thing
hvm_ioreq_server() does is check for an empty list), this code moves
the case of "no ioreq servers for this I/O" out of
hvm_complete_assist_req() and into hvm_has_dm().  Doing it this way
allows hvm_send_assist_req() to only have 2 possible return values.

The key part of skipping the retry is to do "rc = X86EMUL_OKAY"
which is what the error path on the call to hvm_has_dm() does in
hvmemul_do_io() (the only call on hvm_has_dm()).

Since this case is no longer handled in hvm_send_assist_req(), move
the call to hvm_complete_assist_req() into hvm_has_dm().

As part of this change, do the work of hvm_complete_assist_req() in
the PVH case.  Acting more like real hardware looks to be better.

Since there is only one caller of hvm_complete_assist_req(), fold
that routine into the changed hvm_has_dm().

Adding "rc = X86EMUL_OKAY" in the failing case of
hvm_send_assist_req() would unfix what was done in commit
bac0999325056a3b3a92f7622df7ffbc5388b1c3 and commit
f20f3c8ece5c10fa7626f253d28f570a43b23208.  We are currently doing
the succeeding case of hvm_send_assist_req() and retying the I/O.

To only call hvm_select_ioreq_server() once in this code path,
change hvm_has_dm() to return pointer to ioreq_server instead of a
bool.  This will now be passed on to hvm_send_assist_req(), which
was renamed from hvm_send_assist_req_to_ioreq_server().

Since hvm_send_assist_req() is an extern, add an ASSERT() on s.
-----


>> +    }
>>          break;
> 
> For indentation to be meaningful, the closing brace should follow
> the break statement.
> 

Ok, will fix.


>> --- a/xen/include/asm-x86/hvm/hvm.h
>> +++ b/xen/include/asm-x86/hvm/hvm.h
>> @@ -228,7 +228,9 @@ 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);
>> +struct hvm_ioreq_server;
> 
> I think you could avoid this by simply moving up here the
> hvm_has_dm() declaration.
>

Will check that this works.


> 
>> +bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s,
>> +                                           ioreq_t *p);
> 
> If, with the comments above in mind, you still intend to remove
> hvm_send_assist_req(), I'd much favor you renaming
> hvm_send_assist_req_to_ioreq_server() to hvm_send_assist_req().
> 

Happy to do this rename.

   -Don Slutz

> Jan
> 

Re: [PATCH v2 1/3] xentrace: Adjust IOPORT & MMIO format

[George Dunlap]

On Mon, Feb 2, 2015 at 10:22 AM, Don Slutz <dslutz@verizon.com> wrote:
> The 1st item is not data, but the port (address).
> The 2nd item is the data.
>
> Signed-off-by: Don Slutz <dslutz@verizon.com>

Thanks:

Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

> ---
>  tools/xentrace/formats | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tools/xentrace/formats b/tools/xentrace/formats
> index da658bf..5d7b72a 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
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v2 3/3] hvm_has_dm: Do a full check for backing DM

[Jan Beulich]

>>> On 05.02.15 at 20:02, <dslutz@verizon.com> wrote:
> On 02/03/15 09:58, Jan Beulich wrote:
>>>>> On 02.02.15 at 16:22, <dslutz@verizon.com> wrote:
> Ok, will be working on a much better commit message.  Do you want the
> new commit message copied here (in the summary of the changes), or just
> that fact that there is a new commit message?

Only mention it there.

>>> --- 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:
>>> +    {
>>> +        struct hvm_ioreq_server *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;
>> 
>> You alter the flow here, but leave the (now stale) comment
>> untouched - !hvm_has_dm() no longer has the original meaning.
>> 
> 
> I will say that the comment is not stale. Also this code flow change
> is what I am trying to do in this patch.  Since I am talking about
> Paul's code also, I might be off base.
> 
> It is true that hvm_has_dm() has changed.  However the change
> is from "there is no backing DM" to "there is no backing DM
> that will process this request".  To me they mean the same thing.
> 
> That is why I did not change the name and did not see a need
> to change the comment.  However I can adjust the comment to be much longer.
> 
> How does:
> 
> /*
>  * If there is no backing DM, or there is no backing DM to handle
>  * this request, act just like a missing device.
>  */
> 
> look as a new comment?

Simply inserting "suitable" into the original comment would do imo.

> Would it help to s/hvm_has_dm/hvm_has_dm_for_req/?

No, not really. But see also below for the name.

> Here is the proposed new commit message:

Looks a lot better.

> The key part of skipping the retry is to do "rc = X86EMUL_OKAY"
> which is what the error path on the call to hvm_has_dm() does in
> hvmemul_do_io() (the only call on hvm_has_dm()).
> 
> Since this case is no longer handled in hvm_send_assist_req(), move
> the call to hvm_complete_assist_req() into hvm_has_dm().
> 
> As part of this change, do the work of hvm_complete_assist_req() in
> the PVH case.  Acting more like real hardware looks to be better.
> 
> Since there is only one caller of hvm_complete_assist_req(), fold
> that routine into the changed hvm_has_dm().

As said before, with the current name it's not reasonable for the
function to also complete the request (the name strictly suggests
a "read-only" operation). But as it looks this one has only a single
caller too, so maybe apart from renaming an alternative might
again be to fold it into that single caller?

> Adding "rc = X86EMUL_OKAY" in the failing case of
> hvm_send_assist_req() would unfix what was done in commit
> bac0999325056a3b3a92f7622df7ffbc5388b1c3 and commit
> f20f3c8ece5c10fa7626f253d28f570a43b23208.  We are currently doing
> the succeeding case of hvm_send_assist_req() and retying the I/O.

Maybe s/unfix/break/ ?

Jan

Re: [PATCH v2 3/3] hvm_has_dm: Do a full check for backing DM

[Don Slutz]

On 02/06/15 02:35, Jan Beulich wrote:
>>>> On 05.02.15 at 20:02, <dslutz@verizon.com> wrote:
>> On 02/03/15 09:58, Jan Beulich wrote:
>>>>>> On 02.02.15 at 16:22, <dslutz@verizon.com> wrote:
>> Ok, will be working on a much better commit message.  Do you want the
>> new commit message copied here (in the summary of the changes), or just
>> that fact that there is a new commit message?
> 
> Only mention it there.
> 

Ok

>>>> --- 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:
>>>> +    {
>>>> +        struct hvm_ioreq_server *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;
>>>
>>> You alter the flow here, but leave the (now stale) comment
>>> untouched - !hvm_has_dm() no longer has the original meaning.
>>>
>>
>> I will say that the comment is not stale. Also this code flow change
>> is what I am trying to do in this patch.  Since I am talking about
>> Paul's code also, I might be off base.
>>
>> It is true that hvm_has_dm() has changed.  However the change
>> is from "there is no backing DM" to "there is no backing DM
>> that will process this request".  To me they mean the same thing.
>>
>> That is why I did not change the name and did not see a need
>> to change the comment.  However I can adjust the comment to be much longer.
>>
>> How does:
>>
>> /*
>>  * If there is no backing DM, or there is no backing DM to handle
>>  * this request, act just like a missing device.
>>  */
>>
>> look as a new comment?
> 
> Simply inserting "suitable" into the original comment would do imo.
> 

Will change to:

/* If there is no suitable backing DM, just ignore accesses */


>> Would it help to s/hvm_has_dm/hvm_has_dm_for_req/?
> 
> No, not really. But see also below for the name.
> 

ok

>> Here is the proposed new commit message:
> 
> Looks a lot better.
> 
>> The key part of skipping the retry is to do "rc = X86EMUL_OKAY"
>> which is what the error path on the call to hvm_has_dm() does in
>> hvmemul_do_io() (the only call on hvm_has_dm()).
>>
>> Since this case is no longer handled in hvm_send_assist_req(), move
>> the call to hvm_complete_assist_req() into hvm_has_dm().
>>
>> As part of this change, do the work of hvm_complete_assist_req() in
>> the PVH case.  Acting more like real hardware looks to be better.
>>
>> Since there is only one caller of hvm_complete_assist_req(), fold
>> that routine into the changed hvm_has_dm().
> 
> As said before, with the current name it's not reasonable for the
> function to also complete the request (the name strictly suggests
> a "read-only" operation). But as it looks this one has only a single
> caller too, so maybe apart from renaming an alternative might
> again be to fold it into that single caller?
> 

I think that folding routines between files is not the way I would
normally go.  So how about not folding hvm_complete_assist_req()
anywhere, just change it to return void.  Drop the routine
hvm_has_dm(), and have hvmemul_do_io() call hvm_select_ioreq_server()
and in the error path call hvm_complete_assist_req().  This does
leave only one caller of hvm_complete_assist_req(), but the code blocks
are not changing files.  Here is the diff in hvmemul_do_io() to match
these words:

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -218,21 +218,27 @@ static int hvmemul_do_io(
             vio->io_state = HVMIO_handle_mmio_awaiting_completion;
         break;
     case X86EMUL_UNHANDLEABLE:
-        /* If there is no backing DM, just ignore accesses */
-        if ( !hvm_has_dm(curr->domain) )
+    {
+        struct hvm_ioreq_server *s =
+            hvm_select_ioreq_server(curr->domain, &p);
+
+        /* If there is no suitable backing DM, just ignore accesses */
+        if ( !s )
         {
+            hvm_complete_assist_req(&p);
             rc = X86EMUL_OKAY;
             vio->io_state = HVMIO_none;
         }
         else


>> Adding "rc = X86EMUL_OKAY" in the failing case of
>> hvm_send_assist_req() would unfix what was done in commit
>> bac0999325056a3b3a92f7622df7ffbc5388b1c3 and commit
>> f20f3c8ece5c10fa7626f253d28f570a43b23208.  We are currently doing
>> the succeeding case of hvm_send_assist_req() and retying the I/O.
> 
> Maybe s/unfix/break/ ?
> 

Yes.

   -Don Slutz

> Jan
>