diff mbox

[v2,REPOST,08/12] x86/hvm/ioreq: move is_default into struct hvm_ioreq_server

Message ID 20170822145107.6877-9-paul.durrant@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paul Durrant Aug. 22, 2017, 2:51 p.m. UTC 
Legacy emulators use the 'default' IOREQ server which has slightly
different semantics than other, explicitly created, IOREQ servers.

Because of this, most of the initialization and teardown code needs to
know whether the server is default or not. This is currently achieved
by passing an is_default boolean argument to the functions in question,
whereas this argument could be avoided by adding a field to the
hvm_ioreq_server structure which is also passed as an argument to all
the relevant functions.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/ioreq.c         | 80 ++++++++++++++++++----------------------
 xen/include/asm-x86/hvm/domain.h |  1 +
 2 files changed, 36 insertions(+), 45 deletions(-)

Comments

Roger Pau Monné Aug. 24, 2017, 4:21 p.m. UTC | #1 
On Tue, Aug 22, 2017 at 03:51:02PM +0100, Paul Durrant wrote:
> Legacy emulators use the 'default' IOREQ server which has slightly
> different semantics than other, explicitly created, IOREQ servers.
> 
> Because of this, most of the initialization and teardown code needs to
> know whether the server is default or not. This is currently achieved
> by passing an is_default boolean argument to the functions in question,
> whereas this argument could be avoided by adding a field to the
> hvm_ioreq_server structure which is also passed as an argument to all
> the relevant functions.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

This looks fine, I've also seen that there's a hvm_domain.default_ioreq_server, which is used in a bunch of loops in the file like:

if ( s == d->arch.hvm_domain.default_ioreq_server )

AFAICT those could also be replaced with s->is_default.

Roger.
Paul Durrant Aug. 24, 2017, 4:31 p.m. UTC | #2 
> -----Original Message-----
> From: Roger Pau Monne
> Sent: 24 August 2017 17:22
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Jan Beulich <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH v2 REPOST 08/12] x86/hvm/ioreq: move
> is_default into struct hvm_ioreq_server
> 
> On Tue, Aug 22, 2017 at 03:51:02PM +0100, Paul Durrant wrote:
> > Legacy emulators use the 'default' IOREQ server which has slightly
> > different semantics than other, explicitly created, IOREQ servers.
> >
> > Because of this, most of the initialization and teardown code needs to
> > know whether the server is default or not. This is currently achieved
> > by passing an is_default boolean argument to the functions in question,
> > whereas this argument could be avoided by adding a field to the
> > hvm_ioreq_server structure which is also passed as an argument to all
> > the relevant functions.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> 
> This looks fine, I've also seen that there's a
> hvm_domain.default_ioreq_server, which is used in a bunch of loops in the
> file like:
> 
> if ( s == d->arch.hvm_domain.default_ioreq_server )
> 
> AFAICT those could also be replaced with s->is_default.

Yes, they should be. Thanks for spotting that.

Cheers,

  Paul

> 
> Roger.
diff mbox

Patch

diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index 5e01e1a6d2..5737082238 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -302,7 +302,7 @@  static void hvm_update_ioreq_evtchn(struct hvm_ioreq_server *s,
 }
 
 static int hvm_ioreq_server_add_vcpu(struct hvm_ioreq_server *s,
-                                     bool is_default, struct vcpu *v)
+                                     struct vcpu *v)
 {
     struct hvm_ioreq_vcpu *sv;
     int rc;
@@ -331,7 +331,7 @@  static int hvm_ioreq_server_add_vcpu(struct hvm_ioreq_server *s,
             goto fail3;
 
         s->bufioreq_evtchn = rc;
-        if ( is_default )
+        if ( s->is_default )
             d->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_EVTCHN] =
                 s->bufioreq_evtchn;
     }
@@ -431,7 +431,6 @@  static int hvm_ioreq_server_map_pages(struct hvm_ioreq_server *s,
 }
 
 static int hvm_ioreq_server_setup_pages(struct hvm_ioreq_server *s,
-                                        bool is_default,
                                         bool handle_bufioreq)
 {
     struct domain *d = s->domain;
@@ -439,7 +438,7 @@  static int hvm_ioreq_server_setup_pages(struct hvm_ioreq_server *s,
     unsigned long bufioreq_gfn = gfn_x(INVALID_GFN);
     int rc;
 
-    if ( is_default )
+    if ( s->is_default )
     {
         /*
          * The default ioreq server must handle buffered ioreqs, for
@@ -468,8 +467,7 @@  static int hvm_ioreq_server_setup_pages(struct hvm_ioreq_server *s,
     return rc;
 }
 
-static void hvm_ioreq_server_unmap_pages(struct hvm_ioreq_server *s,
-                                         bool is_default)
+static void hvm_ioreq_server_unmap_pages(struct hvm_ioreq_server *s)
 {
     struct domain *d = s->domain;
     bool handle_bufioreq = !!s->bufioreq.va;
@@ -479,7 +477,7 @@  static void hvm_ioreq_server_unmap_pages(struct hvm_ioreq_server *s,
 
     hvm_unmap_ioreq_page(s, false);
 
-    if ( !is_default )
+    if ( !s->is_default )
     {
         if ( handle_bufioreq )
             hvm_free_ioreq_gfn(d, s->bufioreq.gfn);
@@ -488,25 +486,23 @@  static void hvm_ioreq_server_unmap_pages(struct hvm_ioreq_server *s,
     }
 }
 
-static void hvm_ioreq_server_free_rangesets(struct hvm_ioreq_server *s,
-                                            bool is_default)
+static void hvm_ioreq_server_free_rangesets(struct hvm_ioreq_server *s)
 {
     unsigned int i;
 
-    if ( is_default )
+    if ( s->is_default )
         return;
 
     for ( i = 0; i < NR_IO_RANGE_TYPES; i++ )
         rangeset_destroy(s->range[i]);
 }
 
-static int hvm_ioreq_server_alloc_rangesets(struct hvm_ioreq_server *s,
-                                            bool is_default)
+static int hvm_ioreq_server_alloc_rangesets(struct hvm_ioreq_server *s)
 {
     unsigned int i;
     int rc;
 
-    if ( is_default )
+    if ( s->is_default )
         goto done;
 
     for ( i = 0; i < NR_IO_RANGE_TYPES; i++ )
@@ -537,13 +533,12 @@  static int hvm_ioreq_server_alloc_rangesets(struct hvm_ioreq_server *s,
     return 0;
 
  fail:
-    hvm_ioreq_server_free_rangesets(s, false);
+    hvm_ioreq_server_free_rangesets(s);
 
     return rc;
 }
 
-static void hvm_ioreq_server_enable(struct hvm_ioreq_server *s,
-                                    bool is_default)
+static void hvm_ioreq_server_enable(struct hvm_ioreq_server *s)
 {
     struct domain *d = s->domain;
     struct hvm_ioreq_vcpu *sv;
@@ -554,7 +549,7 @@  static void hvm_ioreq_server_enable(struct hvm_ioreq_server *s,
     if ( s->enabled )
         goto done;
 
-    if ( !is_default )
+    if ( !s->is_default )
     {
         hvm_remove_ioreq_gfn(d, &s->ioreq);
 
@@ -573,8 +568,7 @@  static void hvm_ioreq_server_enable(struct hvm_ioreq_server *s,
     spin_unlock(&s->lock);
 }
 
-static void hvm_ioreq_server_disable(struct hvm_ioreq_server *s,
-                                     bool is_default)
+static void hvm_ioreq_server_disable(struct hvm_ioreq_server *s)
 {
     struct domain *d = s->domain;
     bool handle_bufioreq = !!s->bufioreq.va;
@@ -584,7 +578,7 @@  static void hvm_ioreq_server_disable(struct hvm_ioreq_server *s,
     if ( !s->enabled )
         goto done;
 
-    if ( !is_default )
+    if ( !s->is_default )
     {
         if ( handle_bufioreq )
             hvm_add_ioreq_gfn(d, &s->bufioreq);
@@ -600,8 +594,7 @@  static void hvm_ioreq_server_disable(struct hvm_ioreq_server *s,
 
 static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
                                  struct domain *d, domid_t domid,
-                                 bool is_default, int bufioreq_handling,
-                                 ioservid_t id)
+                                 int bufioreq_handling, ioservid_t id)
 {
     struct vcpu *v;
     int rc;
@@ -614,7 +607,7 @@  static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
     INIT_LIST_HEAD(&s->ioreq_vcpu_list);
     spin_lock_init(&s->bufioreq_lock);
 
-    rc = hvm_ioreq_server_alloc_rangesets(s, is_default);
+    rc = hvm_ioreq_server_alloc_rangesets(s);
     if ( rc )
         return rc;
 
@@ -622,13 +615,13 @@  static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
         s->bufioreq_atomic = true;
 
     rc = hvm_ioreq_server_setup_pages(
-             s, is_default, bufioreq_handling != HVM_IOREQSRV_BUFIOREQ_OFF);
+             s, bufioreq_handling != HVM_IOREQSRV_BUFIOREQ_OFF);
     if ( rc )
         goto fail_map;
 
     for_each_vcpu ( d, v )
     {
-        rc = hvm_ioreq_server_add_vcpu(s, is_default, v);
+        rc = hvm_ioreq_server_add_vcpu(s, v);
         if ( rc )
             goto fail_add;
     }
@@ -637,21 +630,20 @@  static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
 
  fail_add:
     hvm_ioreq_server_remove_all_vcpus(s);
-    hvm_ioreq_server_unmap_pages(s, is_default);
+    hvm_ioreq_server_unmap_pages(s);
 
  fail_map:
-    hvm_ioreq_server_free_rangesets(s, is_default);
+    hvm_ioreq_server_free_rangesets(s);
 
     return rc;
 }
 
-static void hvm_ioreq_server_deinit(struct hvm_ioreq_server *s,
-                                    bool is_default)
+static void hvm_ioreq_server_deinit(struct hvm_ioreq_server *s)
 {
     ASSERT(!s->enabled);
     hvm_ioreq_server_remove_all_vcpus(s);
-    hvm_ioreq_server_unmap_pages(s, is_default);
-    hvm_ioreq_server_free_rangesets(s, is_default);
+    hvm_ioreq_server_unmap_pages(s);
+    hvm_ioreq_server_free_rangesets(s);
 }
 
 static ioservid_t next_ioservid(struct domain *d)
@@ -695,6 +687,8 @@  int hvm_create_ioreq_server(struct domain *d, domid_t domid,
     if ( !s )
         goto fail1;
 
+    s->is_default = is_default;
+
     domain_pause(d);
     spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
 
@@ -702,7 +696,7 @@  int hvm_create_ioreq_server(struct domain *d, domid_t domid,
     if ( is_default && d->arch.hvm_domain.default_ioreq_server != NULL )
         goto fail2;
 
-    rc = hvm_ioreq_server_init(s, d, domid, is_default, bufioreq_handling,
+    rc = hvm_ioreq_server_init(s, d, domid, bufioreq_handling,
                                next_ioservid(d));
     if ( rc )
         goto fail3;
@@ -713,7 +707,7 @@  int hvm_create_ioreq_server(struct domain *d, domid_t domid,
     if ( is_default )
     {
         d->arch.hvm_domain.default_ioreq_server = s;
-        hvm_ioreq_server_enable(s, true);
+        hvm_ioreq_server_enable(s);
     }
 
     if ( id )
@@ -756,11 +750,11 @@  int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id)
 
         p2m_set_ioreq_server(d, 0, s);
 
-        hvm_ioreq_server_disable(s, false);
+        hvm_ioreq_server_disable(s);
 
         list_del(&s->list_entry);
 
-        hvm_ioreq_server_deinit(s, false);
+        hvm_ioreq_server_deinit(s);
 
         domain_unpause(d);
 
@@ -992,9 +986,9 @@  int hvm_set_ioreq_server_state(struct domain *d, ioservid_t id,
         domain_pause(d);
 
         if ( enabled )
-            hvm_ioreq_server_enable(s, false);
+            hvm_ioreq_server_enable(s);
         else
-            hvm_ioreq_server_disable(s, false);
+            hvm_ioreq_server_disable(s);
 
         domain_unpause(d);
 
@@ -1017,9 +1011,7 @@  int hvm_all_ioreq_servers_add_vcpu(struct domain *d, struct vcpu *v)
                           &d->arch.hvm_domain.ioreq_server.list,
                           list_entry )
     {
-        bool is_default = (s == d->arch.hvm_domain.default_ioreq_server);
-
-        rc = hvm_ioreq_server_add_vcpu(s, is_default, v);
+        rc = hvm_ioreq_server_add_vcpu(s, v);
         if ( rc )
             goto fail;
     }
@@ -1066,16 +1058,14 @@  void hvm_destroy_all_ioreq_servers(struct domain *d)
                                &d->arch.hvm_domain.ioreq_server.list,
                                list_entry )
     {
-        bool is_default = (s == d->arch.hvm_domain.default_ioreq_server);
-
-        hvm_ioreq_server_disable(s, is_default);
+        hvm_ioreq_server_disable(s);
 
-        if ( is_default )
+        if ( s->is_default )
             d->arch.hvm_domain.default_ioreq_server = NULL;
 
         list_del(&s->list_entry);
 
-        hvm_ioreq_server_deinit(s, is_default);
+        hvm_ioreq_server_deinit(s);
 
         xfree(s);
     }
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 7f128c05ff..16344d173b 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -71,6 +71,7 @@  struct hvm_ioreq_server {
     struct rangeset        *range[NR_IO_RANGE_TYPES];
     bool                   enabled;
     bool                   bufioreq_atomic;
+    bool                   is_default;
 };
 
 /*