xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [Xen-devel] [PATCH v3 00/10] ioreq: add support for internal servers
@ 2019-09-30 13:32 Roger Pau Monne
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 01/10] ioreq: terminate cf8 handling at hypervisor level Roger Pau Monne
                   ` (9 more replies)
  0 siblings, 10 replies; 24+ messages in thread
From: Roger Pau Monne @ 2019-09-30 13:32 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Wei Liu, Paul Durrant, George Dunlap,
	Andrew Cooper, Konrad Rzeszutek Wilk, Ian Jackson, Tim Deegan,
	Julien Grall, Jan Beulich, Roger Pau Monne

Such internal servers are implemented by a single function that handles
ioreqs inside the hypervisor.

The motivation behind this change is to switch vPCI to become an
internal ioreq server, so that accesses to the PCI config space can be
multiplexed between devices handled by vPCI and devices handled by other
ioreq servers.

The implementation is fairly simple and limited to what's needed by
vPCI, but can be expanded in the future if other more complex users
appear.

The series can also be found at:

git://xenbits.xen.org/people/royger/xen.git ioreq_vpci_v3

Thanks, Roger.

Roger Pau Monne (10):
  ioreq: terminate cf8 handling at hypervisor level
  ioreq: switch selection and forwarding to use ioservid_t
  ioreq: add fields to allow internal ioreq servers
  ioreq: add internal ioreq initialization support
  ioreq: allow dispatching ioreqs to internal servers
  ioreq: allow registering internal ioreq server handler
  ioreq: allow decoding accesses to MMCFG regions
  vpci: register as an internal ioreq server
  ioreq: split the code to detect PCI config space accesses
  ioreq: provide support for long-running operations...

 tools/tests/vpci/Makefile           |   5 +-
 tools/tests/vpci/emul.h             |   4 +
 xen/arch/x86/hvm/dm.c               |  19 +-
 xen/arch/x86/hvm/dom0_build.c       |   9 +-
 xen/arch/x86/hvm/emulate.c          |  14 +-
 xen/arch/x86/hvm/hvm.c              |   7 +-
 xen/arch/x86/hvm/io.c               | 248 ++------------------
 xen/arch/x86/hvm/ioreq.c            | 348 ++++++++++++++++++++--------
 xen/arch/x86/hvm/stdvga.c           |   8 +-
 xen/arch/x86/mm/p2m.c               |  20 +-
 xen/arch/x86/physdev.c              |   5 +-
 xen/drivers/passthrough/x86/iommu.c |   2 +-
 xen/drivers/vpci/header.c           |  60 ++---
 xen/drivers/vpci/vpci.c             |  70 +++++-
 xen/include/asm-x86/hvm/domain.h    |  35 ++-
 xen/include/asm-x86/hvm/io.h        |  29 ++-
 xen/include/asm-x86/hvm/ioreq.h     |  17 +-
 xen/include/asm-x86/hvm/vcpu.h      |   3 +-
 xen/include/asm-x86/p2m.h           |   9 +-
 xen/include/public/hvm/dm_op.h      |   1 +
 xen/include/xen/vpci.h              |  28 +--
 21 files changed, 495 insertions(+), 446 deletions(-)

-- 
2.23.0


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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [Xen-devel] [PATCH v3 01/10] ioreq: terminate cf8 handling at hypervisor level
  2019-09-30 13:32 [Xen-devel] [PATCH v3 00/10] ioreq: add support for internal servers Roger Pau Monne
@ 2019-09-30 13:32 ` Roger Pau Monne
  2019-10-01  9:50   ` Paul Durrant
  2019-10-02 14:19   ` Jan Beulich
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 02/10] ioreq: switch selection and forwarding to use ioservid_t Roger Pau Monne
                   ` (8 subsequent siblings)
  9 siblings, 2 replies; 24+ messages in thread
From: Roger Pau Monne @ 2019-09-30 13:32 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, Paul Durrant, Wei Liu, Jan Beulich, Roger Pau Monne

Do not forward accesses to cf8 to external emulators, decoding of PCI
accesses is handled by Xen, and emulators can request handling of
config space accesses of devices using the provided ioreq interface.

Fully terminate cf8 accesses at the hypervisor level, by improving the
existing hvm_access_cf8 helper to also handle register reads, and
always return X86EMUL_OKAY in order to terminate the emulation.

Note that without this change in the absence of some external emulator
that catches accesses to cf8 read requests to the register would
misbehave, as the ioreq internal handler did not handle those.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Allow ioreq servers to map 0xcf8 and 0xcfc, even if those are
   handled by the hypervisor.

Changes since v1:
 - New in this version.
---
 xen/arch/x86/hvm/ioreq.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index d347144096..5e503ce498 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -1518,11 +1518,15 @@ static int hvm_access_cf8(
 {
     struct domain *d = current->domain;
 
-    if ( dir == IOREQ_WRITE && bytes == 4 )
+    if ( bytes != 4 )
+        return X86EMUL_OKAY;
+
+    if ( dir == IOREQ_WRITE )
         d->arch.hvm.pci_cf8 = *val;
+    else
+        *val = d->arch.hvm.pci_cf8;
 
-    /* We always need to fall through to the catch all emulator */
-    return X86EMUL_UNHANDLEABLE;
+    return X86EMUL_OKAY;
 }
 
 void hvm_ioreq_init(struct domain *d)
-- 
2.23.0


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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Xen-devel] [PATCH v3 02/10] ioreq: switch selection and forwarding to use ioservid_t
  2019-09-30 13:32 [Xen-devel] [PATCH v3 00/10] ioreq: add support for internal servers Roger Pau Monne
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 01/10] ioreq: terminate cf8 handling at hypervisor level Roger Pau Monne
@ 2019-09-30 13:32 ` Roger Pau Monne
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 03/10] ioreq: add fields to allow internal ioreq servers Roger Pau Monne
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 24+ messages in thread
From: Roger Pau Monne @ 2019-09-30 13:32 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
	George Dunlap, Andrew Cooper, Paul Durrant, Ian Jackson,
	Tim Deegan, Julien Grall, Paul Durrant, Jan Beulich,
	Roger Pau Monne

hvm_select_ioreq_server and hvm_send_ioreq where both using
hvm_ioreq_server directly, switch to use ioservid_t in order to select
and forward ioreqs.

This is a preparatory change, since future patches will use the ioreq
server id in order to differentiate between internal and external
ioreq servers.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes since v2:
 - Don't hardcode 0xffff for XEN_INVALID_IOSERVID.

Changes since v1:
 - New in this version.
---
 xen/arch/x86/hvm/dm.c           |  2 +-
 xen/arch/x86/hvm/emulate.c      | 14 +++++++-------
 xen/arch/x86/hvm/ioreq.c        | 24 ++++++++++++------------
 xen/arch/x86/hvm/stdvga.c       |  8 ++++----
 xen/arch/x86/mm/p2m.c           | 20 ++++++++++----------
 xen/include/asm-x86/hvm/ioreq.h |  5 ++---
 xen/include/asm-x86/p2m.h       |  9 ++++-----
 xen/include/public/hvm/dm_op.h  |  1 +
 8 files changed, 41 insertions(+), 42 deletions(-)

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index d6d0e8be89..c2fca9f729 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -263,7 +263,7 @@ static int set_mem_type(struct domain *d,
             return -EOPNOTSUPP;
 
         /* Do not change to HVMMEM_ioreq_server if no ioreq server mapped. */
-        if ( !p2m_get_ioreq_server(d, &flags) )
+        if ( p2m_get_ioreq_server(d, &flags) == XEN_INVALID_IOSERVID )
             return -EINVAL;
     }
 
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 637034b6a1..c37bd020c8 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -255,7 +255,7 @@ static int hvmemul_do_io(
          * However, there's no cheap approach to avoid above situations in xen,
          * so the device model side needs to check the incoming ioreq event.
          */
-        struct hvm_ioreq_server *s = NULL;
+        ioservid_t id = XEN_INVALID_IOSERVID;
         p2m_type_t p2mt = p2m_invalid;
 
         if ( is_mmio )
@@ -268,9 +268,9 @@ static int hvmemul_do_io(
             {
                 unsigned int flags;
 
-                s = p2m_get_ioreq_server(currd, &flags);
+                id = p2m_get_ioreq_server(currd, &flags);
 
-                if ( s == NULL )
+                if ( id == XEN_INVALID_IOSERVID )
                 {
                     rc = X86EMUL_RETRY;
                     vio->io_req.state = STATE_IOREQ_NONE;
@@ -290,18 +290,18 @@ static int hvmemul_do_io(
             }
         }
 
-        if ( !s )
-            s = hvm_select_ioreq_server(currd, &p);
+        if ( id == XEN_INVALID_IOSERVID )
+            id = hvm_select_ioreq_server(currd, &p);
 
         /* If there is no suitable backing DM, just ignore accesses */
-        if ( !s )
+        if ( id == XEN_INVALID_IOSERVID )
         {
             rc = hvm_process_io_intercept(&null_handler, &p);
             vio->io_req.state = STATE_IOREQ_NONE;
         }
         else
         {
-            rc = hvm_send_ioreq(s, &p, 0);
+            rc = hvm_send_ioreq(id, &p, 0);
             if ( rc != X86EMUL_RETRY || currd->is_shutting_down )
                 vio->io_req.state = STATE_IOREQ_NONE;
             else if ( !hvm_ioreq_needs_completion(&vio->io_req) )
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index 5e503ce498..ed0142c4e1 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -39,6 +39,7 @@ static void set_ioreq_server(struct domain *d, unsigned int id,
 {
     ASSERT(id < MAX_NR_IOREQ_SERVERS);
     ASSERT(!s || !d->arch.hvm.ioreq_server.server[id]);
+    BUILD_BUG_ON(MAX_NR_IOREQ_SERVERS >= XEN_INVALID_IOSERVID);
 
     d->arch.hvm.ioreq_server.server[id] = s;
 }
@@ -868,7 +869,7 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id)
 
     domain_pause(d);
 
-    p2m_set_ioreq_server(d, 0, s);
+    p2m_set_ioreq_server(d, 0, id);
 
     hvm_ioreq_server_disable(s);
 
@@ -1125,7 +1126,7 @@ int hvm_map_mem_type_to_ioreq_server(struct domain *d, ioservid_t id,
     if ( s->emulator != current->domain )
         goto out;
 
-    rc = p2m_set_ioreq_server(d, flags, s);
+    rc = p2m_set_ioreq_server(d, flags, id);
 
  out:
     spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
@@ -1249,8 +1250,7 @@ void hvm_destroy_all_ioreq_servers(struct domain *d)
     spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
 }
 
-struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
-                                                 ioreq_t *p)
+ioservid_t hvm_select_ioreq_server(struct domain *d, ioreq_t *p)
 {
     struct hvm_ioreq_server *s;
     uint32_t cf8;
@@ -1259,7 +1259,7 @@ struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
     unsigned int id;
 
     if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO )
-        return NULL;
+        return XEN_INVALID_IOSERVID;
 
     cf8 = d->arch.hvm.pci_cf8;
 
@@ -1314,7 +1314,7 @@ struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
             start = addr;
             end = start + p->size - 1;
             if ( rangeset_contains_range(r, start, end) )
-                return s;
+                return id;
 
             break;
 
@@ -1323,7 +1323,7 @@ struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
             end = hvm_mmio_last_byte(p);
 
             if ( rangeset_contains_range(r, start, end) )
-                return s;
+                return id;
 
             break;
 
@@ -1332,14 +1332,14 @@ struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
             {
                 p->type = IOREQ_TYPE_PCI_CONFIG;
                 p->addr = addr;
-                return s;
+                return id;
             }
 
             break;
         }
     }
 
-    return NULL;
+    return XEN_INVALID_IOSERVID;
 }
 
 static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p)
@@ -1435,12 +1435,12 @@ static int hvm_send_buffered_ioreq(struct hvm_ioreq_server *s, ioreq_t *p)
     return X86EMUL_OKAY;
 }
 
-int hvm_send_ioreq(struct hvm_ioreq_server *s, ioreq_t *proto_p,
-                   bool buffered)
+int hvm_send_ioreq(ioservid_t id, ioreq_t *proto_p, bool buffered)
 {
     struct vcpu *curr = current;
     struct domain *d = curr->domain;
     struct hvm_ioreq_vcpu *sv;
+    struct hvm_ioreq_server *s = get_ioreq_server(d, id);
 
     ASSERT(s);
 
@@ -1506,7 +1506,7 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered)
         if ( !s->enabled )
             continue;
 
-        if ( hvm_send_ioreq(s, p, buffered) == X86EMUL_UNHANDLEABLE )
+        if ( hvm_send_ioreq(id, p, buffered) == X86EMUL_UNHANDLEABLE )
             failed++;
     }
 
diff --git a/xen/arch/x86/hvm/stdvga.c b/xen/arch/x86/hvm/stdvga.c
index bd398dbb1b..a689269712 100644
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -466,7 +466,7 @@ static int stdvga_mem_write(const struct hvm_io_handler *handler,
         .dir = IOREQ_WRITE,
         .data = data,
     };
-    struct hvm_ioreq_server *srv;
+    ioservid_t id;
 
     if ( !stdvga_cache_is_enabled(s) || !s->stdvga )
         goto done;
@@ -507,11 +507,11 @@ static int stdvga_mem_write(const struct hvm_io_handler *handler,
     }
 
  done:
-    srv = hvm_select_ioreq_server(current->domain, &p);
-    if ( !srv )
+    id = hvm_select_ioreq_server(current->domain, &p);
+    if ( id == XEN_INVALID_IOSERVID )
         return X86EMUL_UNHANDLEABLE;
 
-    return hvm_send_ioreq(srv, &p, 1);
+    return hvm_send_ioreq(id, &p, 1);
 }
 
 static bool_t stdvga_mem_accept(const struct hvm_io_handler *handler,
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index e5e4349dea..c0edb9a319 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -102,6 +102,7 @@ static int p2m_initialise(struct domain *d, struct p2m_domain *p2m)
         p2m_pt_init(p2m);
 
     spin_lock_init(&p2m->ioreq.lock);
+    p2m->ioreq.server = XEN_INVALID_IOSERVID;
 
     return ret;
 }
@@ -361,7 +362,7 @@ void p2m_memory_type_changed(struct domain *d)
 
 int p2m_set_ioreq_server(struct domain *d,
                          unsigned int flags,
-                         struct hvm_ioreq_server *s)
+                         ioservid_t id)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     int rc;
@@ -376,16 +377,16 @@ int p2m_set_ioreq_server(struct domain *d,
     if ( flags == 0 )
     {
         rc = -EINVAL;
-        if ( p2m->ioreq.server != s )
+        if ( p2m->ioreq.server != id )
             goto out;
 
-        p2m->ioreq.server = NULL;
+        p2m->ioreq.server = XEN_INVALID_IOSERVID;
         p2m->ioreq.flags = 0;
     }
     else
     {
         rc = -EBUSY;
-        if ( p2m->ioreq.server != NULL )
+        if ( p2m->ioreq.server != XEN_INVALID_IOSERVID )
             goto out;
 
         /*
@@ -397,7 +398,7 @@ int p2m_set_ioreq_server(struct domain *d,
         if ( read_atomic(&p2m->ioreq.entry_count) )
             goto out;
 
-        p2m->ioreq.server = s;
+        p2m->ioreq.server = id;
         p2m->ioreq.flags = flags;
     }
 
@@ -409,19 +410,18 @@ int p2m_set_ioreq_server(struct domain *d,
     return rc;
 }
 
-struct hvm_ioreq_server *p2m_get_ioreq_server(struct domain *d,
-                                              unsigned int *flags)
+ioservid_t p2m_get_ioreq_server(struct domain *d, unsigned int *flags)
 {
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
-    struct hvm_ioreq_server *s;
+    ioservid_t id;
 
     spin_lock(&p2m->ioreq.lock);
 
-    s = p2m->ioreq.server;
+    id = p2m->ioreq.server;
     *flags = p2m->ioreq.flags;
 
     spin_unlock(&p2m->ioreq.lock);
-    return s;
+    return id;
 }
 
 void p2m_enable_hardware_log_dirty(struct domain *d)
diff --git a/xen/include/asm-x86/hvm/ioreq.h b/xen/include/asm-x86/hvm/ioreq.h
index e2588e912f..65491c48d2 100644
--- a/xen/include/asm-x86/hvm/ioreq.h
+++ b/xen/include/asm-x86/hvm/ioreq.h
@@ -47,9 +47,8 @@ int hvm_all_ioreq_servers_add_vcpu(struct domain *d, struct vcpu *v);
 void hvm_all_ioreq_servers_remove_vcpu(struct domain *d, struct vcpu *v);
 void hvm_destroy_all_ioreq_servers(struct domain *d);
 
-struct hvm_ioreq_server *hvm_select_ioreq_server(struct domain *d,
-                                                 ioreq_t *p);
-int hvm_send_ioreq(struct hvm_ioreq_server *s, ioreq_t *proto_p,
+ioservid_t hvm_select_ioreq_server(struct domain *d, ioreq_t *p);
+int hvm_send_ioreq(ioservid_t id, ioreq_t *proto_p,
                    bool buffered);
 unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered);
 
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 94285db1b4..99a1dab311 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -354,7 +354,7 @@ struct p2m_domain {
           * ioreq server who's responsible for the emulation of
           * gfns with specific p2m type(for now, p2m_ioreq_server).
           */
-         struct hvm_ioreq_server *server;
+         ioservid_t server;
          /*
           * flags specifies whether read, write or both operations
           * are to be emulated by an ioreq server.
@@ -819,7 +819,7 @@ static inline p2m_type_t p2m_recalc_type_range(bool recalc, p2m_type_t t,
     if ( !recalc || !p2m_is_changeable(t) )
         return t;
 
-    if ( t == p2m_ioreq_server && p2m->ioreq.server != NULL )
+    if ( t == p2m_ioreq_server && p2m->ioreq.server != XEN_INVALID_IOSERVID )
         return t;
 
     return p2m_is_logdirty_range(p2m, gfn_start, gfn_end) ? p2m_ram_logdirty
@@ -938,9 +938,8 @@ static inline unsigned int p2m_get_iommu_flags(p2m_type_t p2mt, mfn_t mfn)
 }
 
 int p2m_set_ioreq_server(struct domain *d, unsigned int flags,
-                         struct hvm_ioreq_server *s);
-struct hvm_ioreq_server *p2m_get_ioreq_server(struct domain *d,
-                                              unsigned int *flags);
+                         ioservid_t id);
+ioservid_t p2m_get_ioreq_server(struct domain *d, unsigned int *flags);
 
 static inline int p2m_entry_modify(struct p2m_domain *p2m, p2m_type_t nt,
                                    p2m_type_t ot, mfn_t nfn, mfn_t ofn,
diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
index d3b554d019..ee3963e54f 100644
--- a/xen/include/public/hvm/dm_op.h
+++ b/xen/include/public/hvm/dm_op.h
@@ -54,6 +54,7 @@
  */
 
 typedef uint16_t ioservid_t;
+#define XEN_INVALID_IOSERVID ((ioservid_t)~0)
 
 /*
  * XEN_DMOP_create_ioreq_server: Instantiate a new IOREQ Server for a
-- 
2.23.0


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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Xen-devel] [PATCH v3 03/10] ioreq: add fields to allow internal ioreq servers
  2019-09-30 13:32 [Xen-devel] [PATCH v3 00/10] ioreq: add support for internal servers Roger Pau Monne
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 01/10] ioreq: terminate cf8 handling at hypervisor level Roger Pau Monne
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 02/10] ioreq: switch selection and forwarding to use ioservid_t Roger Pau Monne
@ 2019-09-30 13:32 ` Roger Pau Monne
  2019-10-02 14:22   ` Jan Beulich
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 04/10] ioreq: add internal ioreq initialization support Roger Pau Monne
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 24+ messages in thread
From: Roger Pau Monne @ 2019-09-30 13:32 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, Paul Durrant, Wei Liu, Jan Beulich, Roger Pau Monne

Internal ioreq servers are plain function handlers implemented inside
of the hypervisor. Note that most fields used by current (external)
ioreq servers are not needed for internal ones, and hence have been
placed inside of a struct and packed in an union together with the
only internal specific field, a function pointer to a handler.

This is required in order to have PCI config accesses forwarded to
external ioreq servers or to internal ones (ie: QEMU emulated devices
vs vPCI passthrough), and is the first step in order to allow
unprivileged domains to use vPCI.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
---
Changes since v2:
 - Drop the vcpu parameter from the handler.

Changes since v1:
 - Do not add an internal field to the ioreq server struct, whether a
   server is internal or external can already be inferred from the id.
 - Add an extra parameter to the internal handler in order to pass
   user-provided opaque data to the handler.
---
 xen/include/asm-x86/hvm/domain.h | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index bcc5621797..56a32e3e35 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -52,21 +52,29 @@ struct hvm_ioreq_vcpu {
 #define MAX_NR_IO_RANGES  256
 
 struct hvm_ioreq_server {
-    struct domain          *target, *emulator;
-
+    struct domain          *target;
     /* Lock to serialize toolstack modifications */
     spinlock_t             lock;
-
-    struct hvm_ioreq_page  ioreq;
-    struct list_head       ioreq_vcpu_list;
-    struct hvm_ioreq_page  bufioreq;
-
-    /* Lock to serialize access to buffered ioreq ring */
-    spinlock_t             bufioreq_lock;
-    evtchn_port_t          bufioreq_evtchn;
     struct rangeset        *range[NR_IO_RANGE_TYPES];
     bool                   enabled;
-    uint8_t                bufioreq_handling;
+
+    union {
+        struct {
+            struct domain          *emulator;
+            struct hvm_ioreq_page  ioreq;
+            struct list_head       ioreq_vcpu_list;
+            struct hvm_ioreq_page  bufioreq;
+
+            /* Lock to serialize access to buffered ioreq ring */
+            spinlock_t             bufioreq_lock;
+            evtchn_port_t          bufioreq_evtchn;
+            uint8_t                bufioreq_handling;
+        };
+        struct {
+            void                   *data;
+            int (*handler)(ioreq_t *, void *);
+        };
+    };
 };
 
 /*
-- 
2.23.0


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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Xen-devel] [PATCH v3 04/10] ioreq: add internal ioreq initialization support
  2019-09-30 13:32 [Xen-devel] [PATCH v3 00/10] ioreq: add support for internal servers Roger Pau Monne
                   ` (2 preceding siblings ...)
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 03/10] ioreq: add fields to allow internal ioreq servers Roger Pau Monne
@ 2019-09-30 13:32 ` Roger Pau Monne
  2019-10-01  9:57   ` Andrew Cooper
  2019-10-02 14:47   ` Jan Beulich
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 05/10] ioreq: allow dispatching ioreqs to internal servers Roger Pau Monne
                   ` (5 subsequent siblings)
  9 siblings, 2 replies; 24+ messages in thread
From: Roger Pau Monne @ 2019-09-30 13:32 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, Paul Durrant, Wei Liu, Jan Beulich, Roger Pau Monne

Add support for internal ioreq servers to initialization and
deinitialization routines, prevent some functions from being executed
against internal ioreq servers and add guards to only allow internal
callers to modify internal ioreq servers. External callers (ie: from
hypercalls) are only allowed to deal with external ioreq servers.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Return early from hvm_ioreq_server_init and hvm_ioreq_server_deinit
   if server is internal.
 - hvm_destroy_ioreq_server, hvm_get_ioreq_server_info and
   hvm_map_mem_type_to_ioreq_server can only be used against external
   servers, hence add an assert to that effect.
 - Simplify ASSERT in hvm_create_ioreq_server.

Changes since v1:
 - Do not pass an 'internal' parameter to most functions, and instead
   use the id to key whether an ioreq server is internal or external.
 - Prevent enabling an internal server without a handler.
---
 xen/arch/x86/hvm/dm.c            |  17 ++++-
 xen/arch/x86/hvm/ioreq.c         | 119 ++++++++++++++++++++-----------
 xen/include/asm-x86/hvm/domain.h |   5 +-
 xen/include/asm-x86/hvm/ioreq.h  |   8 ++-
 4 files changed, 105 insertions(+), 44 deletions(-)

diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
index c2fca9f729..6a3682e58c 100644
--- a/xen/arch/x86/hvm/dm.c
+++ b/xen/arch/x86/hvm/dm.c
@@ -417,7 +417,7 @@ static int dm_op(const struct dmop_args *op_args)
             break;
 
         rc = hvm_create_ioreq_server(d, data->handle_bufioreq,
-                                     &data->id);
+                                     &data->id, false);
         break;
     }
 
@@ -450,6 +450,9 @@ static int dm_op(const struct dmop_args *op_args)
         rc = -EINVAL;
         if ( data->pad )
             break;
+        rc = -EPERM;
+        if ( hvm_ioreq_is_internal(data->id) )
+            break;
 
         rc = hvm_map_io_range_to_ioreq_server(d, data->id, data->type,
                                               data->start, data->end);
@@ -464,6 +467,9 @@ static int dm_op(const struct dmop_args *op_args)
         rc = -EINVAL;
         if ( data->pad )
             break;
+        rc = -EPERM;
+        if ( hvm_ioreq_is_internal(data->id) )
+            break;
 
         rc = hvm_unmap_io_range_from_ioreq_server(d, data->id, data->type,
                                                   data->start, data->end);
@@ -481,6 +487,9 @@ static int dm_op(const struct dmop_args *op_args)
         rc = -EOPNOTSUPP;
         if ( !hap_enabled(d) )
             break;
+        rc = -EPERM;
+        if ( hvm_ioreq_is_internal(data->id) )
+            break;
 
         if ( first_gfn == 0 )
             rc = hvm_map_mem_type_to_ioreq_server(d, data->id,
@@ -528,6 +537,9 @@ static int dm_op(const struct dmop_args *op_args)
         rc = -EINVAL;
         if ( data->pad )
             break;
+        rc = -EPERM;
+        if ( hvm_ioreq_is_internal(data->id) )
+            break;
 
         rc = hvm_set_ioreq_server_state(d, data->id, !!data->enabled);
         break;
@@ -541,6 +553,9 @@ static int dm_op(const struct dmop_args *op_args)
         rc = -EINVAL;
         if ( data->pad )
             break;
+        rc = -EPERM;
+        if ( hvm_ioreq_is_internal(data->id) )
+            break;
 
         rc = hvm_destroy_ioreq_server(d, data->id);
         break;
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index ed0142c4e1..cdbd4244a4 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -59,10 +59,11 @@ static struct hvm_ioreq_server *get_ioreq_server(const struct domain *d,
 /*
  * Iterate over all possible ioreq servers.
  *
- * NOTE: The iteration is backwards such that more recently created
- *       ioreq servers are favoured in hvm_select_ioreq_server().
- *       This is a semantic that previously existed when ioreq servers
- *       were held in a linked list.
+ * NOTE: The iteration is backwards such that internal and more recently
+ *       created external ioreq servers are favoured in
+ *       hvm_select_ioreq_server().
+ *       This is a semantic that previously existed for external servers when
+ *       ioreq servers were held in a linked list.
  */
 #define FOR_EACH_IOREQ_SERVER(d, id, s) \
     for ( (id) = MAX_NR_IOREQ_SERVERS; (id) != 0; ) \
@@ -70,6 +71,12 @@ static struct hvm_ioreq_server *get_ioreq_server(const struct domain *d,
             continue; \
         else
 
+#define FOR_EACH_EXTERNAL_IOREQ_SERVER(d, id, s) \
+    for ( (id) = MAX_NR_EXTERNAL_IOREQ_SERVERS; (id) != 0; ) \
+        if ( !(s = GET_IOREQ_SERVER(d, --(id))) ) \
+            continue; \
+        else
+
 static ioreq_t *get_ioreq(struct hvm_ioreq_server *s, struct vcpu *v)
 {
     shared_iopage_t *p = s->ioreq.va;
@@ -86,7 +93,7 @@ bool hvm_io_pending(struct vcpu *v)
     struct hvm_ioreq_server *s;
     unsigned int id;
 
-    FOR_EACH_IOREQ_SERVER(d, id, s)
+    FOR_EACH_EXTERNAL_IOREQ_SERVER(d, id, s)
     {
         struct hvm_ioreq_vcpu *sv;
 
@@ -190,7 +197,7 @@ bool handle_hvm_io_completion(struct vcpu *v)
         return false;
     }
 
-    FOR_EACH_IOREQ_SERVER(d, id, s)
+    FOR_EACH_EXTERNAL_IOREQ_SERVER(d, id, s)
     {
         struct hvm_ioreq_vcpu *sv;
 
@@ -430,7 +437,7 @@ bool is_ioreq_server_page(struct domain *d, const struct page_info *page)
 
     spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
 
-    FOR_EACH_IOREQ_SERVER(d, id, s)
+    FOR_EACH_EXTERNAL_IOREQ_SERVER(d, id, s)
     {
         if ( (s->ioreq.page == page) || (s->bufioreq.page == page) )
         {
@@ -688,7 +695,7 @@ static int hvm_ioreq_server_alloc_rangesets(struct hvm_ioreq_server *s,
     return rc;
 }
 
-static void hvm_ioreq_server_enable(struct hvm_ioreq_server *s)
+static void hvm_ioreq_server_enable(struct hvm_ioreq_server *s, bool internal)
 {
     struct hvm_ioreq_vcpu *sv;
 
@@ -697,29 +704,40 @@ static void hvm_ioreq_server_enable(struct hvm_ioreq_server *s)
     if ( s->enabled )
         goto done;
 
-    hvm_remove_ioreq_gfn(s, false);
-    hvm_remove_ioreq_gfn(s, true);
+    if ( !internal )
+    {
+        hvm_remove_ioreq_gfn(s, false);
+        hvm_remove_ioreq_gfn(s, true);
 
-    s->enabled = true;
+        list_for_each_entry ( sv,
+                              &s->ioreq_vcpu_list,
+                              list_entry )
+            hvm_update_ioreq_evtchn(s, sv);
+    }
+    else if ( !s->handler )
+    {
+        ASSERT_UNREACHABLE();
+        goto done;
+    }
 
-    list_for_each_entry ( sv,
-                          &s->ioreq_vcpu_list,
-                          list_entry )
-        hvm_update_ioreq_evtchn(s, sv);
+    s->enabled = true;
 
   done:
     spin_unlock(&s->lock);
 }
 
-static void hvm_ioreq_server_disable(struct hvm_ioreq_server *s)
+static void hvm_ioreq_server_disable(struct hvm_ioreq_server *s, bool internal)
 {
     spin_lock(&s->lock);
 
     if ( !s->enabled )
         goto done;
 
-    hvm_add_ioreq_gfn(s, true);
-    hvm_add_ioreq_gfn(s, false);
+    if ( !internal )
+    {
+        hvm_add_ioreq_gfn(s, true);
+        hvm_add_ioreq_gfn(s, false);
+    }
 
     s->enabled = false;
 
@@ -736,21 +754,21 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
     int rc;
 
     s->target = d;
+    spin_lock_init(&s->lock);
+
+    rc = hvm_ioreq_server_alloc_rangesets(s, id);
+    if ( hvm_ioreq_is_internal(id) || rc )
+        return rc;
 
     get_knownalive_domain(currd);
-    s->emulator = currd;
 
-    spin_lock_init(&s->lock);
+    s->emulator = currd;
     INIT_LIST_HEAD(&s->ioreq_vcpu_list);
     spin_lock_init(&s->bufioreq_lock);
 
     s->ioreq.gfn = INVALID_GFN;
     s->bufioreq.gfn = INVALID_GFN;
 
-    rc = hvm_ioreq_server_alloc_rangesets(s, id);
-    if ( rc )
-        return rc;
-
     s->bufioreq_handling = bufioreq_handling;
 
     for_each_vcpu ( d, v )
@@ -763,6 +781,7 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
     return 0;
 
  fail_add:
+    ASSERT(!hvm_ioreq_is_internal(id));
     hvm_ioreq_server_remove_all_vcpus(s);
     hvm_ioreq_server_unmap_pages(s);
 
@@ -772,9 +791,15 @@ static int hvm_ioreq_server_init(struct hvm_ioreq_server *s,
     return rc;
 }
 
-static void hvm_ioreq_server_deinit(struct hvm_ioreq_server *s)
+static void hvm_ioreq_server_deinit(struct hvm_ioreq_server *s, bool internal)
 {
     ASSERT(!s->enabled);
+
+    hvm_ioreq_server_free_rangesets(s);
+
+    if ( internal )
+        return;
+
     hvm_ioreq_server_remove_all_vcpus(s);
 
     /*
@@ -789,13 +814,11 @@ static void hvm_ioreq_server_deinit(struct hvm_ioreq_server *s)
     hvm_ioreq_server_unmap_pages(s);
     hvm_ioreq_server_free_pages(s);
 
-    hvm_ioreq_server_free_rangesets(s);
-
     put_domain(s->emulator);
 }
 
 int hvm_create_ioreq_server(struct domain *d, int bufioreq_handling,
-                            ioservid_t *id)
+                            ioservid_t *id, bool internal)
 {
     struct hvm_ioreq_server *s;
     unsigned int i;
@@ -811,7 +834,9 @@ int hvm_create_ioreq_server(struct domain *d, int bufioreq_handling,
     domain_pause(d);
     spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
 
-    for ( i = 0; i < MAX_NR_IOREQ_SERVERS; i++ )
+    for ( i = (internal ? MAX_NR_EXTERNAL_IOREQ_SERVERS : 0);
+          i < (internal ? MAX_NR_IOREQ_SERVERS : MAX_NR_EXTERNAL_IOREQ_SERVERS);
+          i++ )
     {
         if ( !GET_IOREQ_SERVER(d, i) )
             break;
@@ -821,6 +846,10 @@ int hvm_create_ioreq_server(struct domain *d, int bufioreq_handling,
     if ( i >= MAX_NR_IOREQ_SERVERS )
         goto fail;
 
+    ASSERT(i < MAX_NR_EXTERNAL_IOREQ_SERVERS
+           ? !internal
+           : internal && i < MAX_NR_IOREQ_SERVERS);
+
     /*
      * It is safe to call set_ioreq_server() prior to
      * hvm_ioreq_server_init() since the target domain is paused.
@@ -855,6 +884,8 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id)
     struct hvm_ioreq_server *s;
     int rc;
 
+    ASSERT(!hvm_ioreq_is_internal(id));
+
     spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
 
     s = get_ioreq_server(d, id);
@@ -864,6 +895,7 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id)
         goto out;
 
     rc = -EPERM;
+    /* NB: internal servers cannot be destroyed. */
     if ( s->emulator != current->domain )
         goto out;
 
@@ -871,13 +903,13 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id)
 
     p2m_set_ioreq_server(d, 0, id);
 
-    hvm_ioreq_server_disable(s);
+    hvm_ioreq_server_disable(s, hvm_ioreq_is_internal(id));
 
     /*
      * It is safe to call hvm_ioreq_server_deinit() prior to
      * set_ioreq_server() since the target domain is paused.
      */
-    hvm_ioreq_server_deinit(s);
+    hvm_ioreq_server_deinit(s, false);
     set_ioreq_server(d, id, NULL);
 
     domain_unpause(d);
@@ -900,6 +932,8 @@ int hvm_get_ioreq_server_info(struct domain *d, ioservid_t id,
     struct hvm_ioreq_server *s;
     int rc;
 
+    ASSERT(!hvm_ioreq_is_internal(id));
+
     spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
 
     s = get_ioreq_server(d, id);
@@ -909,6 +943,7 @@ int hvm_get_ioreq_server_info(struct domain *d, ioservid_t id,
         goto out;
 
     rc = -EPERM;
+    /* NB: don't allow fetching information from internal ioreq servers. */
     if ( s->emulator != current->domain )
         goto out;
 
@@ -956,7 +991,7 @@ int hvm_get_ioreq_server_frame(struct domain *d, ioservid_t id,
         goto out;
 
     rc = -EPERM;
-    if ( s->emulator != current->domain )
+    if ( hvm_ioreq_is_internal(id) || s->emulator != current->domain )
         goto out;
 
     rc = hvm_ioreq_server_alloc_pages(s);
@@ -1010,7 +1045,7 @@ int hvm_map_io_range_to_ioreq_server(struct domain *d, ioservid_t id,
         goto out;
 
     rc = -EPERM;
-    if ( s->emulator != current->domain )
+    if ( !hvm_ioreq_is_internal(id) && s->emulator != current->domain )
         goto out;
 
     switch ( type )
@@ -1062,7 +1097,7 @@ int hvm_unmap_io_range_from_ioreq_server(struct domain *d, ioservid_t id,
         goto out;
 
     rc = -EPERM;
-    if ( s->emulator != current->domain )
+    if ( !hvm_ioreq_is_internal(id) && s->emulator != current->domain )
         goto out;
 
     switch ( type )
@@ -1108,6 +1143,8 @@ int hvm_map_mem_type_to_ioreq_server(struct domain *d, ioservid_t id,
     struct hvm_ioreq_server *s;
     int rc;
 
+    ASSERT(!hvm_ioreq_is_internal(id));
+
     if ( type != HVMMEM_ioreq_server )
         return -EINVAL;
 
@@ -1157,15 +1194,15 @@ int hvm_set_ioreq_server_state(struct domain *d, ioservid_t id,
         goto out;
 
     rc = -EPERM;
-    if ( s->emulator != current->domain )
+    if ( !hvm_ioreq_is_internal(id) && s->emulator != current->domain )
         goto out;
 
     domain_pause(d);
 
     if ( enabled )
-        hvm_ioreq_server_enable(s);
+        hvm_ioreq_server_enable(s, hvm_ioreq_is_internal(id));
     else
-        hvm_ioreq_server_disable(s);
+        hvm_ioreq_server_disable(s, hvm_ioreq_is_internal(id));
 
     domain_unpause(d);
 
@@ -1184,7 +1221,7 @@ int hvm_all_ioreq_servers_add_vcpu(struct domain *d, struct vcpu *v)
 
     spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
 
-    FOR_EACH_IOREQ_SERVER(d, id, s)
+    FOR_EACH_EXTERNAL_IOREQ_SERVER(d, id, s)
     {
         rc = hvm_ioreq_server_add_vcpu(s, v);
         if ( rc )
@@ -1218,7 +1255,7 @@ void hvm_all_ioreq_servers_remove_vcpu(struct domain *d, struct vcpu *v)
 
     spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
 
-    FOR_EACH_IOREQ_SERVER(d, id, s)
+    FOR_EACH_EXTERNAL_IOREQ_SERVER(d, id, s)
         hvm_ioreq_server_remove_vcpu(s, v);
 
     spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
@@ -1235,13 +1272,13 @@ void hvm_destroy_all_ioreq_servers(struct domain *d)
 
     FOR_EACH_IOREQ_SERVER(d, id, s)
     {
-        hvm_ioreq_server_disable(s);
+        hvm_ioreq_server_disable(s, hvm_ioreq_is_internal(id));
 
         /*
          * It is safe to call hvm_ioreq_server_deinit() prior to
          * set_ioreq_server() since the target domain is being destroyed.
          */
-        hvm_ioreq_server_deinit(s);
+        hvm_ioreq_server_deinit(s, hvm_ioreq_is_internal(id));
         set_ioreq_server(d, id, NULL);
 
         xfree(s);
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index 56a32e3e35..f09ce9b417 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -97,7 +97,10 @@ struct hvm_pi_ops {
     void (*vcpu_block)(struct vcpu *);
 };
 
-#define MAX_NR_IOREQ_SERVERS 8
+#define MAX_NR_EXTERNAL_IOREQ_SERVERS 8
+#define MAX_NR_INTERNAL_IOREQ_SERVERS 1
+#define MAX_NR_IOREQ_SERVERS \
+    (MAX_NR_EXTERNAL_IOREQ_SERVERS + MAX_NR_INTERNAL_IOREQ_SERVERS)
 
 struct hvm_domain {
     /* Guest page range used for non-default ioreq servers */
diff --git a/xen/include/asm-x86/hvm/ioreq.h b/xen/include/asm-x86/hvm/ioreq.h
index 65491c48d2..c3917aa74d 100644
--- a/xen/include/asm-x86/hvm/ioreq.h
+++ b/xen/include/asm-x86/hvm/ioreq.h
@@ -24,7 +24,7 @@ bool handle_hvm_io_completion(struct vcpu *v);
 bool is_ioreq_server_page(struct domain *d, const struct page_info *page);
 
 int hvm_create_ioreq_server(struct domain *d, int bufioreq_handling,
-                            ioservid_t *id);
+                            ioservid_t *id, bool internal);
 int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id);
 int hvm_get_ioreq_server_info(struct domain *d, ioservid_t id,
                               unsigned long *ioreq_gfn,
@@ -54,6 +54,12 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered);
 
 void hvm_ioreq_init(struct domain *d);
 
+static inline bool hvm_ioreq_is_internal(unsigned int id)
+{
+    ASSERT(id < MAX_NR_IOREQ_SERVERS);
+    return id >= MAX_NR_EXTERNAL_IOREQ_SERVERS;
+}
+
 #endif /* __ASM_X86_HVM_IOREQ_H__ */
 
 /*
-- 
2.23.0


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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Xen-devel] [PATCH v3 05/10] ioreq: allow dispatching ioreqs to internal servers
  2019-09-30 13:32 [Xen-devel] [PATCH v3 00/10] ioreq: add support for internal servers Roger Pau Monne
                   ` (3 preceding siblings ...)
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 04/10] ioreq: add internal ioreq initialization support Roger Pau Monne
@ 2019-09-30 13:32 ` Roger Pau Monne
  2019-10-02 15:05   ` Jan Beulich
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 06/10] ioreq: allow registering internal ioreq server handler Roger Pau Monne
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 24+ messages in thread
From: Roger Pau Monne @ 2019-09-30 13:32 UTC (permalink / raw)
  To: xen-devel
  Cc: Wei Liu, Paul Durrant, Andrew Cooper, Paul Durrant, Jan Beulich,
	Roger Pau Monne

Internal ioreq servers will be processed first due to the implementation
of FOR_EACH_IOREQ_SERVER, and ioreqs are dispatched simply by calling
the handler function.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
---
Changes since v2:
 - Have a single condition for buffered ioreqs.

Changes since v1:
 - Avoid having to iterate twice over the list of ioreq servers since
   now internal servers are always processed first by
   FOR_EACH_IOREQ_SERVER.
 - Obtain ioreq server id using pointer arithmetic.
---
 xen/arch/x86/hvm/ioreq.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index cdbd4244a4..0649b7e02d 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -1482,7 +1482,16 @@ int hvm_send_ioreq(ioservid_t id, ioreq_t *proto_p, bool buffered)
     ASSERT(s);
 
     if ( buffered )
-        return hvm_send_buffered_ioreq(s, proto_p);
+    {
+        if ( likely(!hvm_ioreq_is_internal(id)) )
+            return hvm_send_buffered_ioreq(s, proto_p);
+
+        ASSERT_UNREACHABLE();
+        return X86EMUL_UNHANDLEABLE;
+    }
+
+    if ( hvm_ioreq_is_internal(id) )
+        return s->handler(proto_p, s->data);
 
     if ( unlikely(!vcpu_start_shutdown_deferral(curr)) )
         return X86EMUL_RETRY;
-- 
2.23.0


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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Xen-devel] [PATCH v3 06/10] ioreq: allow registering internal ioreq server handler
  2019-09-30 13:32 [Xen-devel] [PATCH v3 00/10] ioreq: add support for internal servers Roger Pau Monne
                   ` (4 preceding siblings ...)
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 05/10] ioreq: allow dispatching ioreqs to internal servers Roger Pau Monne
@ 2019-09-30 13:32 ` Roger Pau Monne
  2019-10-01 10:03   ` Paul Durrant
  2019-10-02 15:08   ` Jan Beulich
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 07/10] ioreq: allow decoding accesses to MMCFG regions Roger Pau Monne
                   ` (3 subsequent siblings)
  9 siblings, 2 replies; 24+ messages in thread
From: Roger Pau Monne @ 2019-09-30 13:32 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, Paul Durrant, Wei Liu, Jan Beulich, Roger Pau Monne

Provide a routine to register the handler for an internal ioreq
server.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - s/hvm_add_ioreq_handler/hvm_set_ioreq_handler.
 - Do not goto the out label if ioreq is not internal.

Changes since v1:
 - Allow to provide an opaque data parameter to pass to the handler.
 - Allow changing the handler as long as the server is not enabled.
---
 xen/arch/x86/hvm/ioreq.c        | 32 ++++++++++++++++++++++++++++++++
 xen/include/asm-x86/hvm/ioreq.h |  4 ++++
 2 files changed, 36 insertions(+)

diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index 0649b7e02d..57719c607c 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -485,6 +485,38 @@ static int hvm_add_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
     return rc;
 }
 
+int hvm_set_ioreq_handler(struct domain *d, ioservid_t id,
+                          int (*handler)(ioreq_t *, void *),
+                          void *data)
+{
+    struct hvm_ioreq_server *s;
+    int rc = 0;
+
+    if ( !hvm_ioreq_is_internal(id) )
+        return -EINVAL;
+
+    spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
+    s = get_ioreq_server(d, id);
+    if ( !s )
+    {
+        rc = -ENOENT;
+        goto out;
+    }
+    if ( s->enabled )
+    {
+        rc = -EBUSY;
+        goto out;
+    }
+
+    s->handler = handler;
+    s->data = data;
+
+ out:
+    spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
+
+    return rc;
+}
+
 static void hvm_update_ioreq_evtchn(struct hvm_ioreq_server *s,
                                     struct hvm_ioreq_vcpu *sv)
 {
diff --git a/xen/include/asm-x86/hvm/ioreq.h b/xen/include/asm-x86/hvm/ioreq.h
index c3917aa74d..bfd2b9925e 100644
--- a/xen/include/asm-x86/hvm/ioreq.h
+++ b/xen/include/asm-x86/hvm/ioreq.h
@@ -54,6 +54,10 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered);
 
 void hvm_ioreq_init(struct domain *d);
 
+int hvm_set_ioreq_handler(struct domain *d, ioservid_t id,
+                          int (*handler)(ioreq_t *, void *),
+                          void *data);
+
 static inline bool hvm_ioreq_is_internal(unsigned int id)
 {
     ASSERT(id < MAX_NR_IOREQ_SERVERS);
-- 
2.23.0


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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Xen-devel] [PATCH v3 07/10] ioreq: allow decoding accesses to MMCFG regions
  2019-09-30 13:32 [Xen-devel] [PATCH v3 00/10] ioreq: add support for internal servers Roger Pau Monne
                   ` (5 preceding siblings ...)
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 06/10] ioreq: allow registering internal ioreq server handler Roger Pau Monne
@ 2019-09-30 13:32 ` Roger Pau Monne
  2019-10-01 10:37   ` Paul Durrant
  2019-10-07 11:45   ` Jan Beulich
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 08/10] vpci: register as an internal ioreq server Roger Pau Monne
                   ` (2 subsequent siblings)
  9 siblings, 2 replies; 24+ messages in thread
From: Roger Pau Monne @ 2019-09-30 13:32 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, Paul Durrant, Wei Liu, Jan Beulich, Roger Pau Monne

Pick up on the infrastructure already added for vPCI and allow ioreq
to decode accesses to MMCFG regions registered for a domain. This
infrastructure is still only accessible from internal callers, so
MMCFG regions can only be registered from the internal domain builder
used by PVH dom0.

Note that the vPCI infrastructure to decode and handle accesses to
MMCFG regions will be removed in following patches when vPCI is
switched to become an internal ioreq server.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Don't prevent mapping MCFG ranges by ioreq servers.

Changes since v1:
 - Remove prototype for destroy_vpci_mmcfg.
 - Keep the code in io.c so PCI accesses to MMCFG regions can be
   decoded before ioreq processing.
---
 xen/arch/x86/hvm/dom0_build.c       |  8 +--
 xen/arch/x86/hvm/hvm.c              |  2 +-
 xen/arch/x86/hvm/io.c               | 79 ++++++++++++-----------------
 xen/arch/x86/hvm/ioreq.c            | 18 +++++--
 xen/arch/x86/physdev.c              |  5 +-
 xen/drivers/passthrough/x86/iommu.c |  2 +-
 xen/include/asm-x86/hvm/io.h        | 29 ++++++++---
 7 files changed, 75 insertions(+), 68 deletions(-)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 831325150b..b30042d8f3 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -1108,10 +1108,10 @@ static void __hwdom_init pvh_setup_mmcfg(struct domain *d)
 
     for ( i = 0; i < pci_mmcfg_config_num; i++ )
     {
-        rc = register_vpci_mmcfg_handler(d, pci_mmcfg_config[i].address,
-                                         pci_mmcfg_config[i].start_bus_number,
-                                         pci_mmcfg_config[i].end_bus_number,
-                                         pci_mmcfg_config[i].pci_segment);
+        rc = hvm_register_mmcfg(d, pci_mmcfg_config[i].address,
+                                pci_mmcfg_config[i].start_bus_number,
+                                pci_mmcfg_config[i].end_bus_number,
+                                pci_mmcfg_config[i].pci_segment);
         if ( rc )
             printk("Unable to setup MMCFG handler at %#lx for segment %u\n",
                    pci_mmcfg_config[i].address,
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index c22cb39cf3..5348186c0c 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -753,7 +753,7 @@ void hvm_domain_destroy(struct domain *d)
         xfree(ioport);
     }
 
-    destroy_vpci_mmcfg(d);
+    hvm_free_mmcfg(d);
 }
 
 static int hvm_save_tsc_adjust(struct vcpu *v, hvm_domain_context_t *h)
diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
index a5b0a23f06..3334888136 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -279,6 +279,18 @@ unsigned int hvm_pci_decode_addr(unsigned int cf8, unsigned int addr,
     return CF8_ADDR_LO(cf8) | (addr & 3);
 }
 
+unsigned int hvm_mmcfg_decode_addr(const struct hvm_mmcfg *mmcfg,
+                                   paddr_t addr, pci_sbdf_t *sbdf)
+{
+    addr -= mmcfg->addr;
+    sbdf->bdf = MMCFG_BDF(addr);
+    sbdf->bus += mmcfg->start_bus;
+    sbdf->seg = mmcfg->segment;
+
+    return addr & (PCI_CFG_SPACE_EXP_SIZE - 1);
+}
+
+
 /* Do some sanity checks. */
 static bool vpci_access_allowed(unsigned int reg, unsigned int len)
 {
@@ -383,50 +395,14 @@ void register_vpci_portio_handler(struct domain *d)
     handler->ops = &vpci_portio_ops;
 }
 
-struct hvm_mmcfg {
-    struct list_head next;
-    paddr_t addr;
-    unsigned int size;
-    uint16_t segment;
-    uint8_t start_bus;
-};
-
 /* Handlers to trap PCI MMCFG config accesses. */
-static const struct hvm_mmcfg *vpci_mmcfg_find(const struct domain *d,
-                                               paddr_t addr)
-{
-    const struct hvm_mmcfg *mmcfg;
-
-    list_for_each_entry ( mmcfg, &d->arch.hvm.mmcfg_regions, next )
-        if ( addr >= mmcfg->addr && addr < mmcfg->addr + mmcfg->size )
-            return mmcfg;
-
-    return NULL;
-}
-
-bool vpci_is_mmcfg_address(const struct domain *d, paddr_t addr)
-{
-    return vpci_mmcfg_find(d, addr);
-}
-
-static unsigned int vpci_mmcfg_decode_addr(const struct hvm_mmcfg *mmcfg,
-                                           paddr_t addr, pci_sbdf_t *sbdf)
-{
-    addr -= mmcfg->addr;
-    sbdf->bdf = MMCFG_BDF(addr);
-    sbdf->bus += mmcfg->start_bus;
-    sbdf->seg = mmcfg->segment;
-
-    return addr & (PCI_CFG_SPACE_EXP_SIZE - 1);
-}
-
 static int vpci_mmcfg_accept(struct vcpu *v, unsigned long addr)
 {
     struct domain *d = v->domain;
     bool found;
 
     read_lock(&d->arch.hvm.mmcfg_lock);
-    found = vpci_mmcfg_find(d, addr);
+    found = hvm_is_mmcfg_address(d, addr);
     read_unlock(&d->arch.hvm.mmcfg_lock);
 
     return found;
@@ -443,14 +419,14 @@ static int vpci_mmcfg_read(struct vcpu *v, unsigned long addr,
     *data = ~0ul;
 
     read_lock(&d->arch.hvm.mmcfg_lock);
-    mmcfg = vpci_mmcfg_find(d, addr);
+    mmcfg = hvm_mmcfg_find(d, addr);
     if ( !mmcfg )
     {
         read_unlock(&d->arch.hvm.mmcfg_lock);
         return X86EMUL_RETRY;
     }
 
-    reg = vpci_mmcfg_decode_addr(mmcfg, addr, &sbdf);
+    reg = hvm_mmcfg_decode_addr(mmcfg, addr, &sbdf);
     read_unlock(&d->arch.hvm.mmcfg_lock);
 
     if ( !vpci_access_allowed(reg, len) ||
@@ -485,14 +461,14 @@ static int vpci_mmcfg_write(struct vcpu *v, unsigned long addr,
     pci_sbdf_t sbdf;
 
     read_lock(&d->arch.hvm.mmcfg_lock);
-    mmcfg = vpci_mmcfg_find(d, addr);
+    mmcfg = hvm_mmcfg_find(d, addr);
     if ( !mmcfg )
     {
         read_unlock(&d->arch.hvm.mmcfg_lock);
         return X86EMUL_RETRY;
     }
 
-    reg = vpci_mmcfg_decode_addr(mmcfg, addr, &sbdf);
+    reg = hvm_mmcfg_decode_addr(mmcfg, addr, &sbdf);
     read_unlock(&d->arch.hvm.mmcfg_lock);
 
     if ( !vpci_access_allowed(reg, len) ||
@@ -512,9 +488,9 @@ static const struct hvm_mmio_ops vpci_mmcfg_ops = {
     .write = vpci_mmcfg_write,
 };
 
-int register_vpci_mmcfg_handler(struct domain *d, paddr_t addr,
-                                unsigned int start_bus, unsigned int end_bus,
-                                unsigned int seg)
+int hvm_register_mmcfg(struct domain *d, paddr_t addr,
+                       unsigned int start_bus, unsigned int end_bus,
+                       unsigned int seg)
 {
     struct hvm_mmcfg *mmcfg, *new;
 
@@ -549,7 +525,7 @@ int register_vpci_mmcfg_handler(struct domain *d, paddr_t addr,
             return ret;
         }
 
-    if ( list_empty(&d->arch.hvm.mmcfg_regions) )
+    if ( list_empty(&d->arch.hvm.mmcfg_regions) && has_vpci(d) )
         register_mmio_handler(d, &vpci_mmcfg_ops);
 
     list_add(&new->next, &d->arch.hvm.mmcfg_regions);
@@ -558,7 +534,7 @@ int register_vpci_mmcfg_handler(struct domain *d, paddr_t addr,
     return 0;
 }
 
-void destroy_vpci_mmcfg(struct domain *d)
+void hvm_free_mmcfg(struct domain *d)
 {
     struct list_head *mmcfg_regions = &d->arch.hvm.mmcfg_regions;
 
@@ -574,6 +550,17 @@ void destroy_vpci_mmcfg(struct domain *d)
     write_unlock(&d->arch.hvm.mmcfg_lock);
 }
 
+const struct hvm_mmcfg *hvm_mmcfg_find(const struct domain *d, paddr_t addr)
+{
+    const struct hvm_mmcfg *mmcfg;
+
+    list_for_each_entry ( mmcfg, &d->arch.hvm.mmcfg_regions, next )
+        if ( addr >= mmcfg->addr && addr < mmcfg->addr + mmcfg->size )
+            return mmcfg;
+
+    return NULL;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index 57719c607c..6b87a55db5 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -1326,27 +1326,34 @@ ioservid_t hvm_select_ioreq_server(struct domain *d, ioreq_t *p)
     uint8_t type;
     uint64_t addr;
     unsigned int id;
+    const struct hvm_mmcfg *mmcfg;
 
     if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO )
         return XEN_INVALID_IOSERVID;
 
     cf8 = d->arch.hvm.pci_cf8;
 
-    if ( p->type == IOREQ_TYPE_PIO &&
-         (p->addr & ~3) == 0xcfc &&
-         CF8_ENABLED(cf8) )
+    read_lock(&d->arch.hvm.mmcfg_lock);
+    if ( (p->type == IOREQ_TYPE_PIO &&
+          (p->addr & ~3) == 0xcfc &&
+          CF8_ENABLED(cf8)) ||
+         (p->type == IOREQ_TYPE_COPY &&
+          (mmcfg = hvm_mmcfg_find(d, p->addr)) != NULL) )
     {
         uint32_t x86_fam;
         pci_sbdf_t sbdf;
         unsigned int reg;
 
-        reg = hvm_pci_decode_addr(cf8, p->addr, &sbdf);
+        reg = p->type == IOREQ_TYPE_PIO ? hvm_pci_decode_addr(cf8, p->addr,
+                                                              &sbdf)
+                                        : hvm_mmcfg_decode_addr(mmcfg, p->addr,
+                                                                &sbdf);
 
         /* PCI config data cycle */
         type = XEN_DMOP_IO_RANGE_PCI;
         addr = ((uint64_t)sbdf.sbdf << 32) | reg;
         /* AMD extended configuration space access? */
-        if ( CF8_ADDR_HI(cf8) &&
+        if ( p->type == IOREQ_TYPE_PIO && CF8_ADDR_HI(cf8) &&
              d->arch.cpuid->x86_vendor == X86_VENDOR_AMD &&
              (x86_fam = get_cpu_family(
                  d->arch.cpuid->basic.raw_fms, NULL, NULL)) > 0x10 &&
@@ -1365,6 +1372,7 @@ ioservid_t hvm_select_ioreq_server(struct domain *d, ioreq_t *p)
                 XEN_DMOP_IO_RANGE_PORT : XEN_DMOP_IO_RANGE_MEMORY;
         addr = p->addr;
     }
+    read_unlock(&d->arch.hvm.mmcfg_lock);
 
     FOR_EACH_IOREQ_SERVER(d, id, s)
     {
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 3a3c15890b..f61f66df5f 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -562,9 +562,8 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
              * For HVM (PVH) domains try to add the newly found MMCFG to the
              * domain.
              */
-            ret = register_vpci_mmcfg_handler(currd, info.address,
-                                              info.start_bus, info.end_bus,
-                                              info.segment);
+            ret = hvm_register_mmcfg(currd, info.address, info.start_bus,
+                                     info.end_bus, info.segment);
         }
 
         break;
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 59905629e1..53cdbb45f0 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -152,7 +152,7 @@ static bool __hwdom_init hwdom_iommu_map(const struct domain *d,
      * TODO: runtime added MMCFG regions are not checked to make sure they
      * don't overlap with already mapped regions, thus preventing trapping.
      */
-    if ( has_vpci(d) && vpci_is_mmcfg_address(d, pfn_to_paddr(pfn)) )
+    if ( has_vpci(d) && hvm_is_mmcfg_address(d, pfn_to_paddr(pfn)) )
         return false;
 
     return true;
diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
index 7ceb119b64..86ebbd1e7e 100644
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -165,9 +165,19 @@ void stdvga_deinit(struct domain *d);
 
 extern void hvm_dpci_msi_eoi(struct domain *d, int vector);
 
-/* Decode a PCI port IO access into a bus/slot/func/reg. */
+struct hvm_mmcfg {
+    struct list_head next;
+    paddr_t addr;
+    unsigned int size;
+    uint16_t segment;
+    uint8_t start_bus;
+};
+
+/* Decode a PCI port IO or MMCFG access into a bus/slot/func/reg. */
 unsigned int hvm_pci_decode_addr(unsigned int cf8, unsigned int addr,
                                  pci_sbdf_t *sbdf);
+unsigned int hvm_mmcfg_decode_addr(const struct hvm_mmcfg *mmcfg,
+                                   paddr_t addr, pci_sbdf_t *sbdf);
 
 /*
  * HVM port IO handler that performs forwarding of guest IO ports into machine
@@ -178,15 +188,18 @@ void register_g2m_portio_handler(struct domain *d);
 /* HVM port IO handler for vPCI accesses. */
 void register_vpci_portio_handler(struct domain *d);
 
-/* HVM MMIO handler for PCI MMCFG accesses. */
-int register_vpci_mmcfg_handler(struct domain *d, paddr_t addr,
-                                unsigned int start_bus, unsigned int end_bus,
-                                unsigned int seg);
-/* Destroy tracked MMCFG areas. */
-void destroy_vpci_mmcfg(struct domain *d);
+/* HVM PCI MMCFG regions registration. */
+int hvm_register_mmcfg(struct domain *d, paddr_t addr,
+                       unsigned int start_bus, unsigned int end_bus,
+                       unsigned int seg);
+void hvm_free_mmcfg(struct domain *d);
+const struct hvm_mmcfg *hvm_mmcfg_find(const struct domain *d, paddr_t addr);
 
 /* Check if an address is between a MMCFG region for a domain. */
-bool vpci_is_mmcfg_address(const struct domain *d, paddr_t addr);
+static inline bool hvm_is_mmcfg_address(const struct domain *d, paddr_t addr)
+{
+    return hvm_mmcfg_find(d, addr);
+}
 
 #endif /* __ASM_X86_HVM_IO_H__ */
 
-- 
2.23.0


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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Xen-devel] [PATCH v3 08/10] vpci: register as an internal ioreq server
  2019-09-30 13:32 [Xen-devel] [PATCH v3 00/10] ioreq: add support for internal servers Roger Pau Monne
                   ` (6 preceding siblings ...)
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 07/10] ioreq: allow decoding accesses to MMCFG regions Roger Pau Monne
@ 2019-09-30 13:32 ` Roger Pau Monne
  2019-10-07 12:32   ` Jan Beulich
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 09/10] ioreq: split the code to detect PCI config space accesses Roger Pau Monne
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 10/10] ioreq: provide support for long-running operations Roger Pau Monne
  9 siblings, 1 reply; 24+ messages in thread
From: Roger Pau Monne @ 2019-09-30 13:32 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
	George Dunlap, Andrew Cooper, Paul Durrant, Ian Jackson,
	Tim Deegan, Julien Grall, Jan Beulich, Roger Pau Monne

Switch vPCI to become an internal ioreq server, and hence drop all the
vPCI specific decoding and trapping to PCI IO ports and MMCFG regions.

This allows to unify the vPCI code with the ioreq infrastructure,
opening the door for domains having PCI accesses handled by vPCI and
other ioreq servers at the same time.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Remove stray addition of ioreq header to physdev.c.

Changes since v1:
 - Remove prototypes for register_vpci_portio_handler and
   register_vpci_mmcfg_handler.
 - Re-add vpci check in hwdom_iommu_map.
 - Fix test harness.
 - Remove vpci_{read/write} prototypes and make the functions static.
---
 tools/tests/vpci/Makefile     |   5 +-
 tools/tests/vpci/emul.h       |   4 +
 xen/arch/x86/hvm/dom0_build.c |   1 +
 xen/arch/x86/hvm/hvm.c        |   5 +-
 xen/arch/x86/hvm/io.c         | 201 ----------------------------------
 xen/drivers/vpci/vpci.c       |  63 ++++++++++-
 xen/include/xen/vpci.h        |  22 +---
 7 files changed, 79 insertions(+), 222 deletions(-)

diff --git a/tools/tests/vpci/Makefile b/tools/tests/vpci/Makefile
index 5075bc2be2..c365c4522a 100644
--- a/tools/tests/vpci/Makefile
+++ b/tools/tests/vpci/Makefile
@@ -25,7 +25,10 @@ install:
 
 vpci.c: $(XEN_ROOT)/xen/drivers/vpci/vpci.c
 	# Remove includes and add the test harness header
-	sed -e '/#include/d' -e '1s/^/#include "emul.h"/' <$< >$@
+	sed -e '/#include/d' -e '1s/^/#include "emul.h"/' \
+	    -e 's/^static uint32_t read/uint32_t vpci_read/' \
+	    -e 's/^static void write/void vpci_write/' <$< >$@
+
 
 list.h: $(XEN_ROOT)/xen/include/xen/list.h
 vpci.h: $(XEN_ROOT)/xen/include/xen/vpci.h
diff --git a/tools/tests/vpci/emul.h b/tools/tests/vpci/emul.h
index 2e1d3057c9..5a6494a797 100644
--- a/tools/tests/vpci/emul.h
+++ b/tools/tests/vpci/emul.h
@@ -125,6 +125,10 @@ typedef union {
         tx > ty ? tx : ty;              \
 })
 
+uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size);
+void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
+                uint32_t data);
+
 #endif
 
 /*
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index b30042d8f3..dff4d6663c 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -29,6 +29,7 @@
 
 #include <asm/bzimage.h>
 #include <asm/dom0_build.h>
+#include <asm/hvm/ioreq.h>
 #include <asm/hvm/support.h>
 #include <asm/io_apic.h>
 #include <asm/p2m.h>
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 5348186c0c..c5c0e3fa2c 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -656,10 +656,13 @@ int hvm_domain_initialise(struct domain *d)
         d->arch.hvm.io_bitmap = hvm_io_bitmap;
 
     register_g2m_portio_handler(d);
-    register_vpci_portio_handler(d);
 
     hvm_ioreq_init(d);
 
+    rc = vpci_register_ioreq(d);
+    if ( rc )
+        goto fail1;
+
     hvm_init_guest_time(d);
 
     d->arch.hvm.params[HVM_PARAM_TRIPLE_FAULT_REASON] = SHUTDOWN_reboot;
diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
index 3334888136..4c72e68a5b 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -290,204 +290,6 @@ unsigned int hvm_mmcfg_decode_addr(const struct hvm_mmcfg *mmcfg,
     return addr & (PCI_CFG_SPACE_EXP_SIZE - 1);
 }
 
-
-/* Do some sanity checks. */
-static bool vpci_access_allowed(unsigned int reg, unsigned int len)
-{
-    /* Check access size. */
-    if ( len != 1 && len != 2 && len != 4 && len != 8 )
-        return false;
-
-    /* Check that access is size aligned. */
-    if ( (reg & (len - 1)) )
-        return false;
-
-    return true;
-}
-
-/* vPCI config space IO ports handlers (0xcf8/0xcfc). */
-static bool vpci_portio_accept(const struct hvm_io_handler *handler,
-                               const ioreq_t *p)
-{
-    return (p->addr == 0xcf8 && p->size == 4) || (p->addr & ~3) == 0xcfc;
-}
-
-static int vpci_portio_read(const struct hvm_io_handler *handler,
-                            uint64_t addr, uint32_t size, uint64_t *data)
-{
-    const struct domain *d = current->domain;
-    unsigned int reg;
-    pci_sbdf_t sbdf;
-    uint32_t cf8;
-
-    *data = ~(uint64_t)0;
-
-    if ( addr == 0xcf8 )
-    {
-        ASSERT(size == 4);
-        *data = d->arch.hvm.pci_cf8;
-        return X86EMUL_OKAY;
-    }
-
-    ASSERT((addr & ~3) == 0xcfc);
-    cf8 = ACCESS_ONCE(d->arch.hvm.pci_cf8);
-    if ( !CF8_ENABLED(cf8) )
-        return X86EMUL_UNHANDLEABLE;
-
-    reg = hvm_pci_decode_addr(cf8, addr, &sbdf);
-
-    if ( !vpci_access_allowed(reg, size) )
-        return X86EMUL_OKAY;
-
-    *data = vpci_read(sbdf, reg, size);
-
-    return X86EMUL_OKAY;
-}
-
-static int vpci_portio_write(const struct hvm_io_handler *handler,
-                             uint64_t addr, uint32_t size, uint64_t data)
-{
-    struct domain *d = current->domain;
-    unsigned int reg;
-    pci_sbdf_t sbdf;
-    uint32_t cf8;
-
-    if ( addr == 0xcf8 )
-    {
-        ASSERT(size == 4);
-        d->arch.hvm.pci_cf8 = data;
-        return X86EMUL_OKAY;
-    }
-
-    ASSERT((addr & ~3) == 0xcfc);
-    cf8 = ACCESS_ONCE(d->arch.hvm.pci_cf8);
-    if ( !CF8_ENABLED(cf8) )
-        return X86EMUL_UNHANDLEABLE;
-
-    reg = hvm_pci_decode_addr(cf8, addr, &sbdf);
-
-    if ( !vpci_access_allowed(reg, size) )
-        return X86EMUL_OKAY;
-
-    vpci_write(sbdf, reg, size, data);
-
-    return X86EMUL_OKAY;
-}
-
-static const struct hvm_io_ops vpci_portio_ops = {
-    .accept = vpci_portio_accept,
-    .read = vpci_portio_read,
-    .write = vpci_portio_write,
-};
-
-void register_vpci_portio_handler(struct domain *d)
-{
-    struct hvm_io_handler *handler;
-
-    if ( !has_vpci(d) )
-        return;
-
-    handler = hvm_next_io_handler(d);
-    if ( !handler )
-        return;
-
-    handler->type = IOREQ_TYPE_PIO;
-    handler->ops = &vpci_portio_ops;
-}
-
-/* Handlers to trap PCI MMCFG config accesses. */
-static int vpci_mmcfg_accept(struct vcpu *v, unsigned long addr)
-{
-    struct domain *d = v->domain;
-    bool found;
-
-    read_lock(&d->arch.hvm.mmcfg_lock);
-    found = hvm_is_mmcfg_address(d, addr);
-    read_unlock(&d->arch.hvm.mmcfg_lock);
-
-    return found;
-}
-
-static int vpci_mmcfg_read(struct vcpu *v, unsigned long addr,
-                           unsigned int len, unsigned long *data)
-{
-    struct domain *d = v->domain;
-    const struct hvm_mmcfg *mmcfg;
-    unsigned int reg;
-    pci_sbdf_t sbdf;
-
-    *data = ~0ul;
-
-    read_lock(&d->arch.hvm.mmcfg_lock);
-    mmcfg = hvm_mmcfg_find(d, addr);
-    if ( !mmcfg )
-    {
-        read_unlock(&d->arch.hvm.mmcfg_lock);
-        return X86EMUL_RETRY;
-    }
-
-    reg = hvm_mmcfg_decode_addr(mmcfg, addr, &sbdf);
-    read_unlock(&d->arch.hvm.mmcfg_lock);
-
-    if ( !vpci_access_allowed(reg, len) ||
-         (reg + len) > PCI_CFG_SPACE_EXP_SIZE )
-        return X86EMUL_OKAY;
-
-    /*
-     * According to the PCIe 3.1A specification:
-     *  - Configuration Reads and Writes must usually be DWORD or smaller
-     *    in size.
-     *  - Because Root Complex implementations are not required to support
-     *    accesses to a RCRB that cross DW boundaries [...] software
-     *    should take care not to cause the generation of such accesses
-     *    when accessing a RCRB unless the Root Complex will support the
-     *    access.
-     *  Xen however supports 8byte accesses by splitting them into two
-     *  4byte accesses.
-     */
-    *data = vpci_read(sbdf, reg, min(4u, len));
-    if ( len == 8 )
-        *data |= (uint64_t)vpci_read(sbdf, reg + 4, 4) << 32;
-
-    return X86EMUL_OKAY;
-}
-
-static int vpci_mmcfg_write(struct vcpu *v, unsigned long addr,
-                            unsigned int len, unsigned long data)
-{
-    struct domain *d = v->domain;
-    const struct hvm_mmcfg *mmcfg;
-    unsigned int reg;
-    pci_sbdf_t sbdf;
-
-    read_lock(&d->arch.hvm.mmcfg_lock);
-    mmcfg = hvm_mmcfg_find(d, addr);
-    if ( !mmcfg )
-    {
-        read_unlock(&d->arch.hvm.mmcfg_lock);
-        return X86EMUL_RETRY;
-    }
-
-    reg = hvm_mmcfg_decode_addr(mmcfg, addr, &sbdf);
-    read_unlock(&d->arch.hvm.mmcfg_lock);
-
-    if ( !vpci_access_allowed(reg, len) ||
-         (reg + len) > PCI_CFG_SPACE_EXP_SIZE )
-        return X86EMUL_OKAY;
-
-    vpci_write(sbdf, reg, min(4u, len), data);
-    if ( len == 8 )
-        vpci_write(sbdf, reg + 4, 4, data >> 32);
-
-    return X86EMUL_OKAY;
-}
-
-static const struct hvm_mmio_ops vpci_mmcfg_ops = {
-    .check = vpci_mmcfg_accept,
-    .read = vpci_mmcfg_read,
-    .write = vpci_mmcfg_write,
-};
-
 int hvm_register_mmcfg(struct domain *d, paddr_t addr,
                        unsigned int start_bus, unsigned int end_bus,
                        unsigned int seg)
@@ -525,9 +327,6 @@ int hvm_register_mmcfg(struct domain *d, paddr_t addr,
             return ret;
         }
 
-    if ( list_empty(&d->arch.hvm.mmcfg_regions) && has_vpci(d) )
-        register_mmio_handler(d, &vpci_mmcfg_ops);
-
     list_add(&new->next, &d->arch.hvm.mmcfg_regions);
     write_unlock(&d->arch.hvm.mmcfg_lock);
 
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index cbd1bac7fc..206fcadbc6 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -20,6 +20,8 @@
 #include <xen/sched.h>
 #include <xen/vpci.h>
 
+#include <asm/hvm/ioreq.h>
+
 /* Internal struct to store the emulated PCI registers. */
 struct vpci_register {
     vpci_read_t *read;
@@ -302,7 +304,7 @@ static uint32_t merge_result(uint32_t data, uint32_t new, unsigned int size,
     return (data & ~(mask << (offset * 8))) | ((new & mask) << (offset * 8));
 }
 
-uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
+static uint32_t read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
 {
     const struct domain *d = current->domain;
     const struct pci_dev *pdev;
@@ -404,8 +406,8 @@ static void vpci_write_helper(const struct pci_dev *pdev,
              r->private);
 }
 
-void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
-                uint32_t data)
+static void write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
+                  uint32_t data)
 {
     const struct domain *d = current->domain;
     const struct pci_dev *pdev;
@@ -478,6 +480,61 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
     spin_unlock(&pdev->vpci->lock);
 }
 
+#ifdef __XEN__
+static int ioreq_handler(ioreq_t *req, void *data)
+{
+    pci_sbdf_t sbdf;
+
+    /*
+     * NB: certain requests of type different than PCI are broadcasted to all
+     * registered ioreq servers, ignored those.
+     */
+    if ( req->type != IOREQ_TYPE_PCI_CONFIG || req->data_is_ptr )
+        return X86EMUL_UNHANDLEABLE;
+
+    sbdf.sbdf = req->addr >> 32;
+
+    if ( req->dir )
+        req->data = read(sbdf, req->addr, req->size);
+    else
+        write(sbdf, req->addr, req->size, req->data);
+
+    return X86EMUL_OKAY;
+}
+
+int vpci_register_ioreq(struct domain *d)
+{
+    ioservid_t id;
+    int rc;
+
+    if ( !has_vpci(d) )
+        return 0;
+
+    rc = hvm_create_ioreq_server(d, HVM_IOREQSRV_BUFIOREQ_OFF, &id, true);
+    if ( rc )
+        return rc;
+
+    rc = hvm_set_ioreq_handler(d, id, ioreq_handler, NULL);
+    if ( rc )
+        return rc;
+
+    if ( is_hardware_domain(d) )
+    {
+        /* Handle all devices in vpci. */
+        rc = hvm_map_io_range_to_ioreq_server(d, id, XEN_DMOP_IO_RANGE_PCI,
+                                              0, ~(uint64_t)0);
+        if ( rc )
+            return rc;
+    }
+
+    rc = hvm_set_ioreq_server_state(d, id, true);
+    if ( rc )
+        return rc;
+
+    return rc;
+}
+#endif
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 5295d4c990..4e9591c020 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -23,6 +23,9 @@ typedef int vpci_register_init_t(struct pci_dev *dev);
   static vpci_register_init_t *const x##_entry  \
                __used_section(".data.vpci." p) = x
 
+/* Register vPCI handler with ioreq. */
+int vpci_register_ioreq(struct domain *d);
+
 /* Add vPCI handlers to device. */
 int __must_check vpci_add_handlers(struct pci_dev *dev);
 
@@ -38,11 +41,6 @@ int __must_check vpci_add_register(struct vpci *vpci,
 int __must_check vpci_remove_register(struct vpci *vpci, unsigned int offset,
                                       unsigned int size);
 
-/* Generic read/write handlers for the PCI config space. */
-uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size);
-void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
-                uint32_t data);
-
 /* Passthrough handlers. */
 uint32_t vpci_hw_read16(const struct pci_dev *pdev, unsigned int reg,
                         void *data);
@@ -219,20 +217,12 @@ static inline int vpci_add_handlers(struct pci_dev *pdev)
     return 0;
 }
 
-static inline void vpci_dump_msi(void) { }
-
-static inline uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg,
-                                 unsigned int size)
+static inline int vpci_register_ioreq(struct domain *d)
 {
-    ASSERT_UNREACHABLE();
-    return ~(uint32_t)0;
+    return 0;
 }
 
-static inline void vpci_write(pci_sbdf_t sbdf, unsigned int reg,
-                              unsigned int size, uint32_t data)
-{
-    ASSERT_UNREACHABLE();
-}
+static inline void vpci_dump_msi(void) { }
 
 static inline bool vpci_process_pending(struct vcpu *v)
 {
-- 
2.23.0


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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Xen-devel] [PATCH v3 09/10] ioreq: split the code to detect PCI config space accesses
  2019-09-30 13:32 [Xen-devel] [PATCH v3 00/10] ioreq: add support for internal servers Roger Pau Monne
                   ` (7 preceding siblings ...)
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 08/10] vpci: register as an internal ioreq server Roger Pau Monne
@ 2019-09-30 13:32 ` Roger Pau Monne
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 10/10] ioreq: provide support for long-running operations Roger Pau Monne
  9 siblings, 0 replies; 24+ messages in thread
From: Roger Pau Monne @ 2019-09-30 13:32 UTC (permalink / raw)
  To: xen-devel
  Cc: Andrew Cooper, Paul Durrant, Wei Liu, Jan Beulich, Roger Pau Monne

Place the code that converts a PIO/COPY ioreq into a PCI_CONFIG one
into a separate function, and adjust the code to make use of this
newly introduced function.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - New in this version.
---
 xen/arch/x86/hvm/ioreq.c | 111 +++++++++++++++++++++++----------------
 1 file changed, 67 insertions(+), 44 deletions(-)

diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index 6b87a55db5..f3684fc648 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -183,6 +183,54 @@ static bool hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, ioreq_t *p)
     return true;
 }
 
+static void convert_pci_ioreq(struct domain *d, ioreq_t *p)
+{
+    const struct hvm_mmcfg *mmcfg;
+    uint32_t cf8 = d->arch.hvm.pci_cf8;
+
+    if ( p->type != IOREQ_TYPE_PIO && p->type != IOREQ_TYPE_COPY )
+    {
+        ASSERT_UNREACHABLE();
+        return;
+    }
+
+    read_lock(&d->arch.hvm.mmcfg_lock);
+    if ( (p->type == IOREQ_TYPE_PIO &&
+          (p->addr & ~3) == 0xcfc &&
+          CF8_ENABLED(cf8)) ||
+         (p->type == IOREQ_TYPE_COPY &&
+          (mmcfg = hvm_mmcfg_find(d, p->addr)) != NULL) )
+    {
+        uint32_t x86_fam;
+        pci_sbdf_t sbdf;
+        unsigned int reg;
+
+        reg = p->type == IOREQ_TYPE_PIO ? hvm_pci_decode_addr(cf8, p->addr,
+                                                              &sbdf)
+                                        : hvm_mmcfg_decode_addr(mmcfg, p->addr,
+                                                                &sbdf);
+
+        /* PCI config data cycle */
+        p->addr = ((uint64_t)sbdf.sbdf << 32) | reg;
+        /* AMD extended configuration space access? */
+        if ( p->type == IOREQ_TYPE_PIO && CF8_ADDR_HI(cf8) &&
+             d->arch.cpuid->x86_vendor == X86_VENDOR_AMD &&
+             (x86_fam = get_cpu_family(
+                 d->arch.cpuid->basic.raw_fms, NULL, NULL)) > 0x10 &&
+             x86_fam < 0x17 )
+        {
+            uint64_t msr_val;
+
+            if ( !rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) &&
+                 (msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT)) )
+                p->addr |= CF8_ADDR_HI(cf8);
+        }
+        p->type = IOREQ_TYPE_PCI_CONFIG;
+
+    }
+    read_unlock(&d->arch.hvm.mmcfg_lock);
+}
+
 bool handle_hvm_io_completion(struct vcpu *v)
 {
     struct domain *d = v->domain;
@@ -1322,57 +1370,36 @@ void hvm_destroy_all_ioreq_servers(struct domain *d)
 ioservid_t hvm_select_ioreq_server(struct domain *d, ioreq_t *p)
 {
     struct hvm_ioreq_server *s;
-    uint32_t cf8;
     uint8_t type;
-    uint64_t addr;
     unsigned int id;
-    const struct hvm_mmcfg *mmcfg;
 
     if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO )
         return XEN_INVALID_IOSERVID;
 
-    cf8 = d->arch.hvm.pci_cf8;
+    /*
+     * Check and convert the PIO/MMIO ioreq to a PCI config space
+     * access.
+     */
+    convert_pci_ioreq(d, p);
 
-    read_lock(&d->arch.hvm.mmcfg_lock);
-    if ( (p->type == IOREQ_TYPE_PIO &&
-          (p->addr & ~3) == 0xcfc &&
-          CF8_ENABLED(cf8)) ||
-         (p->type == IOREQ_TYPE_COPY &&
-          (mmcfg = hvm_mmcfg_find(d, p->addr)) != NULL) )
+    switch ( p->type )
     {
-        uint32_t x86_fam;
-        pci_sbdf_t sbdf;
-        unsigned int reg;
+    case IOREQ_TYPE_PIO:
+        type = XEN_DMOP_IO_RANGE_PORT;
+        break;
 
-        reg = p->type == IOREQ_TYPE_PIO ? hvm_pci_decode_addr(cf8, p->addr,
-                                                              &sbdf)
-                                        : hvm_mmcfg_decode_addr(mmcfg, p->addr,
-                                                                &sbdf);
+    case IOREQ_TYPE_COPY:
+        type = XEN_DMOP_IO_RANGE_MEMORY;
+        break;
 
-        /* PCI config data cycle */
+    case IOREQ_TYPE_PCI_CONFIG:
         type = XEN_DMOP_IO_RANGE_PCI;
-        addr = ((uint64_t)sbdf.sbdf << 32) | reg;
-        /* AMD extended configuration space access? */
-        if ( p->type == IOREQ_TYPE_PIO && CF8_ADDR_HI(cf8) &&
-             d->arch.cpuid->x86_vendor == X86_VENDOR_AMD &&
-             (x86_fam = get_cpu_family(
-                 d->arch.cpuid->basic.raw_fms, NULL, NULL)) > 0x10 &&
-             x86_fam < 0x17 )
-        {
-            uint64_t msr_val;
+        break;
 
-            if ( !rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) &&
-                 (msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT)) )
-                addr |= CF8_ADDR_HI(cf8);
-        }
-    }
-    else
-    {
-        type = (p->type == IOREQ_TYPE_PIO) ?
-                XEN_DMOP_IO_RANGE_PORT : XEN_DMOP_IO_RANGE_MEMORY;
-        addr = p->addr;
+    default:
+        ASSERT_UNREACHABLE();
+        return XEN_INVALID_IOSERVID;
     }
-    read_unlock(&d->arch.hvm.mmcfg_lock);
 
     FOR_EACH_IOREQ_SERVER(d, id, s)
     {
@@ -1388,7 +1415,7 @@ ioservid_t hvm_select_ioreq_server(struct domain *d, ioreq_t *p)
             unsigned long start, end;
 
         case XEN_DMOP_IO_RANGE_PORT:
-            start = addr;
+            start = p->addr;
             end = start + p->size - 1;
             if ( rangeset_contains_range(r, start, end) )
                 return id;
@@ -1405,12 +1432,8 @@ ioservid_t hvm_select_ioreq_server(struct domain *d, ioreq_t *p)
             break;
 
         case XEN_DMOP_IO_RANGE_PCI:
-            if ( rangeset_contains_singleton(r, addr >> 32) )
-            {
-                p->type = IOREQ_TYPE_PCI_CONFIG;
-                p->addr = addr;
+            if ( rangeset_contains_singleton(r, p->addr >> 32) )
                 return id;
-            }
 
             break;
         }
-- 
2.23.0


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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [Xen-devel] [PATCH v3 10/10] ioreq: provide support for long-running operations...
  2019-09-30 13:32 [Xen-devel] [PATCH v3 00/10] ioreq: add support for internal servers Roger Pau Monne
                   ` (8 preceding siblings ...)
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 09/10] ioreq: split the code to detect PCI config space accesses Roger Pau Monne
@ 2019-09-30 13:32 ` Roger Pau Monne
  2019-10-25 14:35   ` Jan Beulich
  9 siblings, 1 reply; 24+ messages in thread
From: Roger Pau Monne @ 2019-09-30 13:32 UTC (permalink / raw)
  To: xen-devel
  Cc: Stefano Stabellini, Wei Liu, Paul Durrant, George Dunlap,
	Andrew Cooper, Konrad Rzeszutek Wilk, Ian Jackson, Tim Deegan,
	Julien Grall, Jan Beulich, Roger Pau Monne

...and switch vPCI to use this infrastructure for long running
physmap modification operations.

This allows to get rid of the vPCI specific modifications done to
handle_hvm_io_completion and allows generalizing the support for
long-running operations to other internal ioreq servers. Such support
is implemented as a specific handler that can be registers by internal
ioreq servers and that will be called to check for pending work.
Returning true from this handler will prevent the vcpu from running
until the handler returns false.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Remove extra newline in vpci_process_pending.
 - Continue early in handle_hvm_io_completion in order to avoid one
   extra level of indentation.
 - Switch setting the ioreq state to a ternary conditional operator.
---
 xen/arch/x86/hvm/ioreq.c       | 55 ++++++++++++++++++++++++++-----
 xen/drivers/vpci/header.c      | 60 ++++++++++++++++++----------------
 xen/drivers/vpci/vpci.c        |  9 ++++-
 xen/include/asm-x86/hvm/vcpu.h |  3 +-
 xen/include/xen/vpci.h         |  6 ----
 5 files changed, 89 insertions(+), 44 deletions(-)

diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index f3684fc648..78322dfa67 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -239,16 +239,48 @@ bool handle_hvm_io_completion(struct vcpu *v)
     enum hvm_io_completion io_completion;
     unsigned int id;
 
-    if ( has_vpci(d) && vpci_process_pending(v) )
-    {
-        raise_softirq(SCHEDULE_SOFTIRQ);
-        return false;
-    }
-
-    FOR_EACH_EXTERNAL_IOREQ_SERVER(d, id, s)
+    FOR_EACH_IOREQ_SERVER(d, id, s)
     {
         struct hvm_ioreq_vcpu *sv;
 
+        if ( hvm_ioreq_is_internal(id) )
+        {
+            ioreq_t req = vio->io_req;
+
+            if ( vio->io_req.state != STATE_IOREQ_INPROCESS )
+                continue;
+
+            /*
+             * Check and convert the PIO/MMIO ioreq to a PCI config space
+             * access.
+             */
+            convert_pci_ioreq(d, &req);
+
+            if ( s->handler(&req, s->data) == X86EMUL_RETRY )
+            {
+                /*
+                 * Need to raise a scheduler irq in order to prevent the
+                 * guest vcpu from resuming execution.
+                 *
+                 * Note this is not required for external ioreq operations
+                 * because in that case the vcpu is marked as blocked, but
+                 * this cannot be done for long-running internal
+                 * operations, since it would prevent the vcpu from being
+                 * scheduled and thus the long running operation from
+                 * finishing.
+                 */
+                raise_softirq(SCHEDULE_SOFTIRQ);
+                return false;
+            }
+
+            /* Finished processing the ioreq. */
+            vio->io_req.state = hvm_ioreq_needs_completion(&vio->io_req)
+                                ? STATE_IORESP_READY
+                                : STATE_IOREQ_NONE;
+
+            continue;
+        }
+
         list_for_each_entry ( sv,
                               &s->ioreq_vcpu_list,
                               list_entry )
@@ -1554,7 +1586,14 @@ int hvm_send_ioreq(ioservid_t id, ioreq_t *proto_p, bool buffered)
     }
 
     if ( hvm_ioreq_is_internal(id) )
-        return s->handler(proto_p, s->data);
+    {
+        int rc = s->handler(proto_p, s->data);
+
+        if ( rc == X86EMUL_RETRY )
+            curr->arch.hvm.hvm_io.io_req.state = STATE_IOREQ_INPROCESS;
+
+        return rc;
+    }
 
     if ( unlikely(!vcpu_start_shutdown_deferral(curr)) )
         return X86EMUL_RETRY;
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 3c794f486d..9360d19a50 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -129,37 +129,41 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
 
 bool vpci_process_pending(struct vcpu *v)
 {
-    if ( v->vpci.mem )
+    struct map_data data = {
+        .d = v->domain,
+        .map = v->vpci.cmd & PCI_COMMAND_MEMORY,
+    };
+    int rc;
+
+    if ( !v->vpci.mem )
     {
-        struct map_data data = {
-            .d = v->domain,
-            .map = v->vpci.cmd & PCI_COMMAND_MEMORY,
-        };
-        int rc = rangeset_consume_ranges(v->vpci.mem, map_range, &data);
-
-        if ( rc == -ERESTART )
-            return true;
-
-        spin_lock(&v->vpci.pdev->vpci->lock);
-        /* Disable memory decoding unconditionally on failure. */
-        modify_decoding(v->vpci.pdev,
-                        rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : v->vpci.cmd,
-                        !rc && v->vpci.rom_only);
-        spin_unlock(&v->vpci.pdev->vpci->lock);
-
-        rangeset_destroy(v->vpci.mem);
-        v->vpci.mem = NULL;
-        if ( rc )
-            /*
-             * FIXME: in case of failure remove the device from the domain.
-             * Note that there might still be leftover mappings. While this is
-             * safe for Dom0, for DomUs the domain will likely need to be
-             * killed in order to avoid leaking stale p2m mappings on
-             * failure.
-             */
-            vpci_remove_device(v->vpci.pdev);
+        ASSERT_UNREACHABLE();
+        return false;
     }
 
+    rc = rangeset_consume_ranges(v->vpci.mem, map_range, &data);
+    if ( rc == -ERESTART )
+        return true;
+
+    spin_lock(&v->vpci.pdev->vpci->lock);
+    /* Disable memory decoding unconditionally on failure. */
+    modify_decoding(v->vpci.pdev,
+                    rc ? v->vpci.cmd & ~PCI_COMMAND_MEMORY : v->vpci.cmd,
+                    !rc && v->vpci.rom_only);
+    spin_unlock(&v->vpci.pdev->vpci->lock);
+
+    rangeset_destroy(v->vpci.mem);
+    v->vpci.mem = NULL;
+    if ( rc )
+        /*
+         * FIXME: in case of failure remove the device from the domain.
+         * Note that there might still be leftover mappings. While this is
+         * safe for Dom0, for DomUs the domain will likely need to be
+         * killed in order to avoid leaking stale p2m mappings on
+         * failure.
+         */
+        vpci_remove_device(v->vpci.pdev);
+
     return false;
 }
 
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 206fcadbc6..0cc8543eb8 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -484,6 +484,7 @@ static void write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
 static int ioreq_handler(ioreq_t *req, void *data)
 {
     pci_sbdf_t sbdf;
+    struct vcpu *curr = current;
 
     /*
      * NB: certain requests of type different than PCI are broadcasted to all
@@ -492,6 +493,12 @@ static int ioreq_handler(ioreq_t *req, void *data)
     if ( req->type != IOREQ_TYPE_PCI_CONFIG || req->data_is_ptr )
         return X86EMUL_UNHANDLEABLE;
 
+    if ( curr->vpci.mem )
+    {
+        ASSERT(req->state == STATE_IOREQ_INPROCESS);
+        return vpci_process_pending(curr) ? X86EMUL_RETRY : X86EMUL_OKAY;
+    }
+
     sbdf.sbdf = req->addr >> 32;
 
     if ( req->dir )
@@ -499,7 +506,7 @@ static int ioreq_handler(ioreq_t *req, void *data)
     else
         write(sbdf, req->addr, req->size, req->data);
 
-    return X86EMUL_OKAY;
+    return curr->vpci.mem ? X86EMUL_RETRY : X86EMUL_OKAY;
 }
 
 int vpci_register_ioreq(struct domain *d)
diff --git a/xen/include/asm-x86/hvm/vcpu.h b/xen/include/asm-x86/hvm/vcpu.h
index 38f5c2bb9b..4563746466 100644
--- a/xen/include/asm-x86/hvm/vcpu.h
+++ b/xen/include/asm-x86/hvm/vcpu.h
@@ -92,7 +92,8 @@ struct hvm_vcpu_io {
 
 static inline bool hvm_ioreq_needs_completion(const ioreq_t *ioreq)
 {
-    return ioreq->state == STATE_IOREQ_READY &&
+    return (ioreq->state == STATE_IOREQ_READY ||
+            ioreq->state == STATE_IOREQ_INPROCESS) &&
            !ioreq->data_is_ptr &&
            (ioreq->type != IOREQ_TYPE_PIO || ioreq->dir != IOREQ_WRITE);
 }
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 4e9591c020..bad406b21d 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -223,12 +223,6 @@ static inline int vpci_register_ioreq(struct domain *d)
 }
 
 static inline void vpci_dump_msi(void) { }
-
-static inline bool vpci_process_pending(struct vcpu *v)
-{
-    ASSERT_UNREACHABLE();
-    return false;
-}
 #endif
 
 #endif
-- 
2.23.0


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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [Xen-devel] [PATCH v3 01/10] ioreq: terminate cf8 handling at hypervisor level
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 01/10] ioreq: terminate cf8 handling at hypervisor level Roger Pau Monne
@ 2019-10-01  9:50   ` Paul Durrant
  2019-10-02 14:19   ` Jan Beulich
  1 sibling, 0 replies; 24+ messages in thread
From: Paul Durrant @ 2019-10-01  9:50 UTC (permalink / raw)
  To: Roger Pau Monne, xen-devel
  Cc: Andrew Cooper, Roger Pau Monne, Jan Beulich, Wei Liu, Paul Durrant

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Roger Pau Monne
> Sent: 30 September 2019 14:32
> To: xen-devel@lists.xenproject.org
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant <paul@xen.org>; Wei Liu <wl@xen.org>; Jan
> Beulich <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
> Subject: [Xen-devel] [PATCH v3 01/10] ioreq: terminate cf8 handling at hypervisor level
> 
> Do not forward accesses to cf8 to external emulators, decoding of PCI
> accesses is handled by Xen, and emulators can request handling of
> config space accesses of devices using the provided ioreq interface.
> 
> Fully terminate cf8 accesses at the hypervisor level, by improving the
> existing hvm_access_cf8 helper to also handle register reads, and
> always return X86EMUL_OKAY in order to terminate the emulation.
> 
> Note that without this change in the absence of some external emulator
> that catches accesses to cf8 read requests to the register would
> misbehave, as the ioreq internal handler did not handle those.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Paul Durrant <paul@xen.org>

> ---
> Changes since v2:
>  - Allow ioreq servers to map 0xcf8 and 0xcfc, even if those are
>    handled by the hypervisor.
> 
> Changes since v1:
>  - New in this version.
> ---
>  xen/arch/x86/hvm/ioreq.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> index d347144096..5e503ce498 100644
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -1518,11 +1518,15 @@ static int hvm_access_cf8(
>  {
>      struct domain *d = current->domain;
> 
> -    if ( dir == IOREQ_WRITE && bytes == 4 )
> +    if ( bytes != 4 )
> +        return X86EMUL_OKAY;
> +
> +    if ( dir == IOREQ_WRITE )
>          d->arch.hvm.pci_cf8 = *val;
> +    else
> +        *val = d->arch.hvm.pci_cf8;
> 
> -    /* We always need to fall through to the catch all emulator */
> -    return X86EMUL_UNHANDLEABLE;
> +    return X86EMUL_OKAY;
>  }
> 
>  void hvm_ioreq_init(struct domain *d)
> --
> 2.23.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Xen-devel] [PATCH v3 04/10] ioreq: add internal ioreq initialization support
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 04/10] ioreq: add internal ioreq initialization support Roger Pau Monne
@ 2019-10-01  9:57   ` Andrew Cooper
  2019-10-01 10:43     ` Roger Pau Monné
  2019-10-02 14:47   ` Jan Beulich
  1 sibling, 1 reply; 24+ messages in thread
From: Andrew Cooper @ 2019-10-01  9:57 UTC (permalink / raw)
  To: Roger Pau Monne, xen-devel; +Cc: Wei Liu, Jan Beulich, Paul Durrant

On 30/09/2019 14:32, Roger Pau Monne wrote:
> diff --git a/xen/include/asm-x86/hvm/ioreq.h b/xen/include/asm-x86/hvm/ioreq.h
> index 65491c48d2..c3917aa74d 100644
> --- a/xen/include/asm-x86/hvm/ioreq.h
> +++ b/xen/include/asm-x86/hvm/ioreq.h
> @@ -54,6 +54,12 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered);
>  
>  void hvm_ioreq_init(struct domain *d);
>  
> +static inline bool hvm_ioreq_is_internal(unsigned int id)
> +{
> +    ASSERT(id < MAX_NR_IOREQ_SERVERS);
> +    return id >= MAX_NR_EXTERNAL_IOREQ_SERVERS;

You cannot ASSERT() here.  id is guest-controlled data in the dm_op() path.

~Andrew

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Xen-devel] [PATCH v3 06/10] ioreq: allow registering internal ioreq server handler
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 06/10] ioreq: allow registering internal ioreq server handler Roger Pau Monne
@ 2019-10-01 10:03   ` Paul Durrant
  2019-10-02 15:08   ` Jan Beulich
  1 sibling, 0 replies; 24+ messages in thread
From: Paul Durrant @ 2019-10-01 10:03 UTC (permalink / raw)
  To: Roger Pau Monne, xen-devel
  Cc: Andrew Cooper, Roger Pau Monne, Jan Beulich, Wei Liu, Paul Durrant

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Roger Pau Monne
> Sent: 30 September 2019 14:33
> To: xen-devel@lists.xenproject.org
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant <paul@xen.org>; Wei Liu <wl@xen.org>; Jan
> Beulich <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
> Subject: [Xen-devel] [PATCH v3 06/10] ioreq: allow registering internal ioreq server handler
> 
> Provide a routine to register the handler for an internal ioreq
> server.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Paul Durrant <paul@xen.org>

> ---
> Changes since v2:
>  - s/hvm_add_ioreq_handler/hvm_set_ioreq_handler.
>  - Do not goto the out label if ioreq is not internal.
> 
> Changes since v1:
>  - Allow to provide an opaque data parameter to pass to the handler.
>  - Allow changing the handler as long as the server is not enabled.
> ---
>  xen/arch/x86/hvm/ioreq.c        | 32 ++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/hvm/ioreq.h |  4 ++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> index 0649b7e02d..57719c607c 100644
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -485,6 +485,38 @@ static int hvm_add_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
>      return rc;
>  }
> 
> +int hvm_set_ioreq_handler(struct domain *d, ioservid_t id,
> +                          int (*handler)(ioreq_t *, void *),
> +                          void *data)
> +{
> +    struct hvm_ioreq_server *s;
> +    int rc = 0;
> +
> +    if ( !hvm_ioreq_is_internal(id) )
> +        return -EINVAL;
> +
> +    spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
> +    s = get_ioreq_server(d, id);
> +    if ( !s )
> +    {
> +        rc = -ENOENT;
> +        goto out;
> +    }
> +    if ( s->enabled )
> +    {
> +        rc = -EBUSY;
> +        goto out;
> +    }
> +
> +    s->handler = handler;
> +    s->data = data;
> +
> + out:
> +    spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
> +
> +    return rc;
> +}
> +
>  static void hvm_update_ioreq_evtchn(struct hvm_ioreq_server *s,
>                                      struct hvm_ioreq_vcpu *sv)
>  {
> diff --git a/xen/include/asm-x86/hvm/ioreq.h b/xen/include/asm-x86/hvm/ioreq.h
> index c3917aa74d..bfd2b9925e 100644
> --- a/xen/include/asm-x86/hvm/ioreq.h
> +++ b/xen/include/asm-x86/hvm/ioreq.h
> @@ -54,6 +54,10 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered);
> 
>  void hvm_ioreq_init(struct domain *d);
> 
> +int hvm_set_ioreq_handler(struct domain *d, ioservid_t id,
> +                          int (*handler)(ioreq_t *, void *),
> +                          void *data);
> +
>  static inline bool hvm_ioreq_is_internal(unsigned int id)
>  {
>      ASSERT(id < MAX_NR_IOREQ_SERVERS);
> --
> 2.23.0
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Xen-devel] [PATCH v3 07/10] ioreq: allow decoding accesses to MMCFG regions
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 07/10] ioreq: allow decoding accesses to MMCFG regions Roger Pau Monne
@ 2019-10-01 10:37   ` Paul Durrant
  2019-10-07 11:45   ` Jan Beulich
  1 sibling, 0 replies; 24+ messages in thread
From: Paul Durrant @ 2019-10-01 10:37 UTC (permalink / raw)
  To: Roger Pau Monne, xen-devel
  Cc: Andrew Cooper, Roger Pau Monne, Jan Beulich, Wei Liu, Paul Durrant

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Roger Pau Monne
> Sent: 30 September 2019 14:33
> To: xen-devel@lists.xenproject.org
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant <paul@xen.org>; Wei Liu <wl@xen.org>; Jan
> Beulich <jbeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
> Subject: [Xen-devel] [PATCH v3 07/10] ioreq: allow decoding accesses to MMCFG regions
> 
> Pick up on the infrastructure already added for vPCI and allow ioreq
> to decode accesses to MMCFG regions registered for a domain. This
> infrastructure is still only accessible from internal callers, so
> MMCFG regions can only be registered from the internal domain builder
> used by PVH dom0.
> 
> Note that the vPCI infrastructure to decode and handle accesses to
> MMCFG regions will be removed in following patches when vPCI is
> switched to become an internal ioreq server.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Paul Durrant <paul@xen.org>

...with one nit below...

> ---
> Changes since v2:
>  - Don't prevent mapping MCFG ranges by ioreq servers.
> 
> Changes since v1:
>  - Remove prototype for destroy_vpci_mmcfg.
>  - Keep the code in io.c so PCI accesses to MMCFG regions can be
>    decoded before ioreq processing.
> ---
>  xen/arch/x86/hvm/dom0_build.c       |  8 +--
>  xen/arch/x86/hvm/hvm.c              |  2 +-
>  xen/arch/x86/hvm/io.c               | 79 ++++++++++++-----------------
>  xen/arch/x86/hvm/ioreq.c            | 18 +++++--
>  xen/arch/x86/physdev.c              |  5 +-
>  xen/drivers/passthrough/x86/iommu.c |  2 +-
>  xen/include/asm-x86/hvm/io.h        | 29 ++++++++---
>  7 files changed, 75 insertions(+), 68 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index 831325150b..b30042d8f3 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -1108,10 +1108,10 @@ static void __hwdom_init pvh_setup_mmcfg(struct domain *d)
> 
>      for ( i = 0; i < pci_mmcfg_config_num; i++ )
>      {
> -        rc = register_vpci_mmcfg_handler(d, pci_mmcfg_config[i].address,
> -                                         pci_mmcfg_config[i].start_bus_number,
> -                                         pci_mmcfg_config[i].end_bus_number,
> -                                         pci_mmcfg_config[i].pci_segment);
> +        rc = hvm_register_mmcfg(d, pci_mmcfg_config[i].address,
> +                                pci_mmcfg_config[i].start_bus_number,
> +                                pci_mmcfg_config[i].end_bus_number,
> +                                pci_mmcfg_config[i].pci_segment);
>          if ( rc )
>              printk("Unable to setup MMCFG handler at %#lx for segment %u\n",
>                     pci_mmcfg_config[i].address,
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index c22cb39cf3..5348186c0c 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -753,7 +753,7 @@ void hvm_domain_destroy(struct domain *d)
>          xfree(ioport);
>      }
> 
> -    destroy_vpci_mmcfg(d);
> +    hvm_free_mmcfg(d);
>  }
> 
>  static int hvm_save_tsc_adjust(struct vcpu *v, hvm_domain_context_t *h)
> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
> index a5b0a23f06..3334888136 100644
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -279,6 +279,18 @@ unsigned int hvm_pci_decode_addr(unsigned int cf8, unsigned int addr,
>      return CF8_ADDR_LO(cf8) | (addr & 3);
>  }
> 
> +unsigned int hvm_mmcfg_decode_addr(const struct hvm_mmcfg *mmcfg,
> +                                   paddr_t addr, pci_sbdf_t *sbdf)
> +{
> +    addr -= mmcfg->addr;
> +    sbdf->bdf = MMCFG_BDF(addr);
> +    sbdf->bus += mmcfg->start_bus;
> +    sbdf->seg = mmcfg->segment;
> +
> +    return addr & (PCI_CFG_SPACE_EXP_SIZE - 1);
> +}
> +
> +

Extraneous blank line here.

  Paul

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Xen-devel] [PATCH v3 04/10] ioreq: add internal ioreq initialization support
  2019-10-01  9:57   ` Andrew Cooper
@ 2019-10-01 10:43     ` Roger Pau Monné
  0 siblings, 0 replies; 24+ messages in thread
From: Roger Pau Monné @ 2019-10-01 10:43 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: xen-devel, Wei Liu, Jan Beulich, Paul Durrant

On Tue, Oct 01, 2019 at 10:57:13AM +0100, Andrew Cooper wrote:
> On 30/09/2019 14:32, Roger Pau Monne wrote:
> > diff --git a/xen/include/asm-x86/hvm/ioreq.h b/xen/include/asm-x86/hvm/ioreq.h
> > index 65491c48d2..c3917aa74d 100644
> > --- a/xen/include/asm-x86/hvm/ioreq.h
> > +++ b/xen/include/asm-x86/hvm/ioreq.h
> > @@ -54,6 +54,12 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered);
> >  
> >  void hvm_ioreq_init(struct domain *d);
> >  
> > +static inline bool hvm_ioreq_is_internal(unsigned int id)
> > +{
> > +    ASSERT(id < MAX_NR_IOREQ_SERVERS);
> > +    return id >= MAX_NR_EXTERNAL_IOREQ_SERVERS;
> 
> You cannot ASSERT() here.  id is guest-controlled data in the dm_op() path.

Urg, right, thanks for noticing. There's no check prior to calling
hvm_ioreq_is_internal on the dm_op path.

I guess just returning true if id >= MAX_NR_EXTERNAL_IOREQ_SERVERS
would be OK, get_ioreq_server already copes with overflowing ids.

Thanks, Roger.

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Xen-devel] [PATCH v3 01/10] ioreq: terminate cf8 handling at hypervisor level
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 01/10] ioreq: terminate cf8 handling at hypervisor level Roger Pau Monne
  2019-10-01  9:50   ` Paul Durrant
@ 2019-10-02 14:19   ` Jan Beulich
  1 sibling, 0 replies; 24+ messages in thread
From: Jan Beulich @ 2019-10-02 14:19 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: xen-devel, Paul Durrant, WeiLiu, Andrew Cooper

On 30.09.2019 15:32, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -1518,11 +1518,15 @@ static int hvm_access_cf8(
>  {
>      struct domain *d = current->domain;
>  
> -    if ( dir == IOREQ_WRITE && bytes == 4 )
> +    if ( bytes != 4 )
> +        return X86EMUL_OKAY;

I think it was already on v1 that Andrew had pointed out that e.g.
a 1-bye access to CF9 should still be forwarded. I guess you mean
to use X86EMUL_UNHANDLEABLE here, just like was done ...

> +    if ( dir == IOREQ_WRITE )
>          d->arch.hvm.pci_cf8 = *val;
> +    else
> +        *val = d->arch.hvm.pci_cf8;
>  
> -    /* We always need to fall through to the catch all emulator */
> -    return X86EMUL_UNHANDLEABLE;
> +    return X86EMUL_OKAY;
>  }

... universally before. The comment (suitably adjusted) may then
also want to move up.

Jan

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Xen-devel] [PATCH v3 03/10] ioreq: add fields to allow internal ioreq servers
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 03/10] ioreq: add fields to allow internal ioreq servers Roger Pau Monne
@ 2019-10-02 14:22   ` Jan Beulich
  0 siblings, 0 replies; 24+ messages in thread
From: Jan Beulich @ 2019-10-02 14:22 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: xen-devel, Paul Durrant, Wei Liu, Andrew Cooper

On 30.09.2019 15:32, Roger Pau Monne wrote:
> Internal ioreq servers are plain function handlers implemented inside
> of the hypervisor. Note that most fields used by current (external)
> ioreq servers are not needed for internal ones, and hence have been
> placed inside of a struct and packed in an union together with the
> only internal specific field, a function pointer to a handler.
> 
> This is required in order to have PCI config accesses forwarded to
> external ioreq servers or to internal ones (ie: QEMU emulated devices
> vs vPCI passthrough), and is the first step in order to allow
> unprivileged domains to use vPCI.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Xen-devel] [PATCH v3 04/10] ioreq: add internal ioreq initialization support
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 04/10] ioreq: add internal ioreq initialization support Roger Pau Monne
  2019-10-01  9:57   ` Andrew Cooper
@ 2019-10-02 14:47   ` Jan Beulich
  1 sibling, 0 replies; 24+ messages in thread
From: Jan Beulich @ 2019-10-02 14:47 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: xen-devel, Paul Durrant, Wei Liu, Andrew Cooper

On 30.09.2019 15:32, Roger Pau Monne wrote:
> @@ -855,6 +884,8 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id)
>      struct hvm_ioreq_server *s;
>      int rc;
>  
> +    ASSERT(!hvm_ioreq_is_internal(id));

With this, ...

> @@ -871,13 +903,13 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t id)
>  
>      p2m_set_ioreq_server(d, 0, id);
>  
> -    hvm_ioreq_server_disable(s);
> +    hvm_ioreq_server_disable(s, hvm_ioreq_is_internal(id));

... why not simply "false" here?

>      /*
>       * It is safe to call hvm_ioreq_server_deinit() prior to
>       * set_ioreq_server() since the target domain is paused.
>       */
> -    hvm_ioreq_server_deinit(s);
> +    hvm_ioreq_server_deinit(s, false);

The more that here you do so.

> @@ -900,6 +932,8 @@ int hvm_get_ioreq_server_info(struct domain *d, ioservid_t id,
>      struct hvm_ioreq_server *s;
>      int rc;
>  
> +    ASSERT(!hvm_ioreq_is_internal(id));
> +
>      spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
>  
>      s = get_ioreq_server(d, id);
> @@ -909,6 +943,7 @@ int hvm_get_ioreq_server_info(struct domain *d, ioservid_t id,
>          goto out;
>  
>      rc = -EPERM;
> +    /* NB: don't allow fetching information from internal ioreq servers. */
>      if ( s->emulator != current->domain )
>          goto out;

The comment doesn't really seem to be applicable to the code here:
->emulator lives in the "external" part of the union, and hence if
anywhere I think the comment should go next to the ASSERT() above.

> @@ -1010,7 +1045,7 @@ int hvm_map_io_range_to_ioreq_server(struct domain *d, ioservid_t id,
>          goto out;
>  
>      rc = -EPERM;
> -    if ( s->emulator != current->domain )
> +    if ( !hvm_ioreq_is_internal(id) && s->emulator != current->domain )
>          goto out;
>  
>      switch ( type )
> @@ -1062,7 +1097,7 @@ int hvm_unmap_io_range_from_ioreq_server(struct domain *d, ioservid_t id,
>          goto out;
>  
>      rc = -EPERM;
> -    if ( s->emulator != current->domain )
> +    if ( !hvm_ioreq_is_internal(id) && s->emulator != current->domain )
>          goto out;
>  
>      switch ( type )
> @@ -1108,6 +1143,8 @@ int hvm_map_mem_type_to_ioreq_server(struct domain *d, ioservid_t id,
>      struct hvm_ioreq_server *s;
>      int rc;
>  
> +    ASSERT(!hvm_ioreq_is_internal(id));
> +
>      if ( type != HVMMEM_ioreq_server )
>          return -EINVAL;

Taking just these three, things seem pretty inconsistent: Why ASSERT()
here but if() above? I think it would be better if dm.c was left
unchanged (not sure if I'm in opposition with this to prior review
comments by someone else), in particular making it unnecessary (as it
seems) to expose hvm_ioreq_is_internal() outside of this CU.

> @@ -1184,7 +1221,7 @@ int hvm_all_ioreq_servers_add_vcpu(struct domain *d, struct vcpu *v)
>  
>      spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
>  
> -    FOR_EACH_IOREQ_SERVER(d, id, s)
> +    FOR_EACH_EXTERNAL_IOREQ_SERVER(d, id, s)

Still remembering the error path fix you likely spotted as necessary
while doing this work (commit 215f2576b0): Don't you need to again
adjust this same error path here (MAX_NR_IOREQ_SERVERS ->
MAX_NR_EXTERNAL_IOREQ_SERVERS)?

Jan

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Xen-devel] [PATCH v3 05/10] ioreq: allow dispatching ioreqs to internal servers
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 05/10] ioreq: allow dispatching ioreqs to internal servers Roger Pau Monne
@ 2019-10-02 15:05   ` Jan Beulich
  0 siblings, 0 replies; 24+ messages in thread
From: Jan Beulich @ 2019-10-02 15:05 UTC (permalink / raw)
  To: Roger Pau Monne
  Cc: Paul Durrant, xen-devel, Paul Durrant, WeiLiu, Andrew Cooper

On 30.09.2019 15:32, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -1482,7 +1482,16 @@ int hvm_send_ioreq(ioservid_t id, ioreq_t *proto_p, bool buffered)
>      ASSERT(s);
>  
>      if ( buffered )
> -        return hvm_send_buffered_ioreq(s, proto_p);
> +    {
> +        if ( likely(!hvm_ioreq_is_internal(id)) )
> +            return hvm_send_buffered_ioreq(s, proto_p);
> +
> +        ASSERT_UNREACHABLE();
> +        return X86EMUL_UNHANDLEABLE;
> +    }
> +
> +    if ( hvm_ioreq_is_internal(id) )
> +        return s->handler(proto_p, s->data);
>  
>      if ( unlikely(!vcpu_start_shutdown_deferral(curr)) )
>          return X86EMUL_RETRY;

Wouldn't it be better to handle internal ones first, before even
looking at the "buffered" input? The difference between buffered
and non-buffered for external ones is whether to wait for a reply
iirc; such a difference simply doesn't exist for internal ones.

Jan

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Xen-devel] [PATCH v3 06/10] ioreq: allow registering internal ioreq server handler
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 06/10] ioreq: allow registering internal ioreq server handler Roger Pau Monne
  2019-10-01 10:03   ` Paul Durrant
@ 2019-10-02 15:08   ` Jan Beulich
  1 sibling, 0 replies; 24+ messages in thread
From: Jan Beulich @ 2019-10-02 15:08 UTC (permalink / raw)
  To: Roger Pau Monne, Paul Durrant; +Cc: xen-devel, WeiLiu, Andrew Cooper

On 30.09.2019 15:32, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -485,6 +485,38 @@ static int hvm_add_ioreq_gfn(struct hvm_ioreq_server *s, bool buf)
>      return rc;
>  }
>  
> +int hvm_set_ioreq_handler(struct domain *d, ioservid_t id,
> +                          int (*handler)(ioreq_t *, void *),
> +                          void *data)
> +{
> +    struct hvm_ioreq_server *s;
> +    int rc = 0;
> +
> +    if ( !hvm_ioreq_is_internal(id) )
> +        return -EINVAL;

Isn't BUG_ON() more applicable here?

> +    spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
> +    s = get_ioreq_server(d, id);
> +    if ( !s )
> +    {
> +        rc = -ENOENT;
> +        goto out;
> +    }
> +    if ( s->enabled )
> +    {
> +        rc = -EBUSY;
> +        goto out;
> +    }
> +
> +    s->handler = handler;
> +    s->data = data;

Is it really intended to blindly replace a disabled handler?
Wouldn't the s->enabled check better be evaluating s->handler?

Jan

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Xen-devel] [PATCH v3 07/10] ioreq: allow decoding accesses to MMCFG regions
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 07/10] ioreq: allow decoding accesses to MMCFG regions Roger Pau Monne
  2019-10-01 10:37   ` Paul Durrant
@ 2019-10-07 11:45   ` Jan Beulich
  1 sibling, 0 replies; 24+ messages in thread
From: Jan Beulich @ 2019-10-07 11:45 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: xen-devel, Paul Durrant, Wei Liu, Andrew Cooper

On 30.09.2019 15:32, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/io.c
> +++ b/xen/arch/x86/hvm/io.c
> @@ -279,6 +279,18 @@ unsigned int hvm_pci_decode_addr(unsigned int cf8, unsigned int addr,
>      return CF8_ADDR_LO(cf8) | (addr & 3);
>  }
>  
> +unsigned int hvm_mmcfg_decode_addr(const struct hvm_mmcfg *mmcfg,
> +                                   paddr_t addr, pci_sbdf_t *sbdf)
> +{
> +    addr -= mmcfg->addr;

As the function isn't even static, wouldn't it be helpful to at least
assert that addr >= mmcfg->addr ahead of this, and addr < mmcfg->size
afterwards?

> @@ -574,6 +550,17 @@ void destroy_vpci_mmcfg(struct domain *d)
>      write_unlock(&d->arch.hvm.mmcfg_lock);
>  }
>  
> +const struct hvm_mmcfg *hvm_mmcfg_find(const struct domain *d, paddr_t addr)
> +{
> +    const struct hvm_mmcfg *mmcfg;
> +
> +    list_for_each_entry ( mmcfg, &d->arch.hvm.mmcfg_regions, next )
> +        if ( addr >= mmcfg->addr && addr < mmcfg->addr + mmcfg->size )

As a minor remark, this could be coded without even a theoretical risk
of overflow as

        if ( addr >= mmcfg->addr && addr - mmcfg->addr < mmcfg->size )

> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -1326,27 +1326,34 @@ ioservid_t hvm_select_ioreq_server(struct domain *d, ioreq_t *p)
>      uint8_t type;
>      uint64_t addr;
>      unsigned int id;
> +    const struct hvm_mmcfg *mmcfg;
>  
>      if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO )
>          return XEN_INVALID_IOSERVID;
>  
>      cf8 = d->arch.hvm.pci_cf8;
>  
> -    if ( p->type == IOREQ_TYPE_PIO &&
> -         (p->addr & ~3) == 0xcfc &&
> -         CF8_ENABLED(cf8) )
> +    read_lock(&d->arch.hvm.mmcfg_lock);
> +    if ( (p->type == IOREQ_TYPE_PIO &&
> +          (p->addr & ~3) == 0xcfc &&
> +          CF8_ENABLED(cf8)) ||
> +         (p->type == IOREQ_TYPE_COPY &&
> +          (mmcfg = hvm_mmcfg_find(d, p->addr)) != NULL) )
>      {
>          uint32_t x86_fam;
>          pci_sbdf_t sbdf;
>          unsigned int reg;
>  
> -        reg = hvm_pci_decode_addr(cf8, p->addr, &sbdf);
> +        reg = p->type == IOREQ_TYPE_PIO ? hvm_pci_decode_addr(cf8, p->addr,
> +                                                              &sbdf)
> +                                        : hvm_mmcfg_decode_addr(mmcfg, p->addr,
> +                                                                &sbdf);

I'm afraid old gcc will consider this use of mmcfg "potentially
uninitialized". I.e. I think the variable wants to gain a NULL
initializer, at which point you can slightly shorten the above
to

        reg = mmcfg ? hvm_mmcfg_decode_addr(mmcfg, p->addr, &sbdf)
                    : hvm_pci_decode_addr(cf8, p->addr, &sbdf);


also eliminating a pointer deref (i.e. likely producing minimally
more efficient code).

>          /* PCI config data cycle */
>          type = XEN_DMOP_IO_RANGE_PCI;
>          addr = ((uint64_t)sbdf.sbdf << 32) | reg;
>          /* AMD extended configuration space access? */
> -        if ( CF8_ADDR_HI(cf8) &&
> +        if ( p->type == IOREQ_TYPE_PIO && CF8_ADDR_HI(cf8) &&

Would be "!mmcfg && ..." then here.

> --- a/xen/include/asm-x86/hvm/io.h
> +++ b/xen/include/asm-x86/hvm/io.h
> @@ -165,9 +165,19 @@ void stdvga_deinit(struct domain *d);
>  
>  extern void hvm_dpci_msi_eoi(struct domain *d, int vector);
>  
> -/* Decode a PCI port IO access into a bus/slot/func/reg. */
> +struct hvm_mmcfg {
> +    struct list_head next;
> +    paddr_t addr;
> +    unsigned int size;
> +    uint16_t segment;
> +    uint8_t start_bus;
> +};
> +
> +/* Decode a PCI port IO or MMCFG access into a bus/slot/func/reg. */
>  unsigned int hvm_pci_decode_addr(unsigned int cf8, unsigned int addr,
>                                   pci_sbdf_t *sbdf);
> +unsigned int hvm_mmcfg_decode_addr(const struct hvm_mmcfg *mmcfg,
> +                                   paddr_t addr, pci_sbdf_t *sbdf);

The latest now the comment also wants to include "seg/", I think.

> @@ -178,15 +188,18 @@ void register_g2m_portio_handler(struct domain *d);
>  /* HVM port IO handler for vPCI accesses. */
>  void register_vpci_portio_handler(struct domain *d);
>  
> -/* HVM MMIO handler for PCI MMCFG accesses. */
> -int register_vpci_mmcfg_handler(struct domain *d, paddr_t addr,
> -                                unsigned int start_bus, unsigned int end_bus,
> -                                unsigned int seg);
> -/* Destroy tracked MMCFG areas. */
> -void destroy_vpci_mmcfg(struct domain *d);
> +/* HVM PCI MMCFG regions registration. */
> +int hvm_register_mmcfg(struct domain *d, paddr_t addr,
> +                       unsigned int start_bus, unsigned int end_bus,
> +                       unsigned int seg);

As - from a hierarchy perspective - the segment is more significant, can
you put it ahead of the start/end bus numbers please?

> +void hvm_free_mmcfg(struct domain *d);
> +const struct hvm_mmcfg *hvm_mmcfg_find(const struct domain *d, paddr_t addr);
>  
>  /* Check if an address is between a MMCFG region for a domain. */
> -bool vpci_is_mmcfg_address(const struct domain *d, paddr_t addr);
> +static inline bool hvm_is_mmcfg_address(const struct domain *d, paddr_t addr)
> +{
> +    return hvm_mmcfg_find(d, addr);
> +}

Hmm, yet another instance where the size of an access doesn't matter
in such a predicate. Is it a good idea (or is there a fair reason)
to extend this bad practice? But yes, I do see that vpci_mmcfg_accept()
has no way to pass a size here, and that you you effectively only
move the code here.

Jan

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Xen-devel] [PATCH v3 08/10] vpci: register as an internal ioreq server
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 08/10] vpci: register as an internal ioreq server Roger Pau Monne
@ 2019-10-07 12:32   ` Jan Beulich
  0 siblings, 0 replies; 24+ messages in thread
From: Jan Beulich @ 2019-10-07 12:32 UTC (permalink / raw)
  To: Roger Pau Monne
  Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
	George Dunlap, Andrew Cooper, Paul Durrant, Ian Jackson,
	Tim Deegan, Julien Grall, xen-devel

On 30.09.2019 15:32, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -29,6 +29,7 @@
>  
>  #include <asm/bzimage.h>
>  #include <asm/dom0_build.h>
> +#include <asm/hvm/ioreq.h>
>  #include <asm/hvm/support.h>
>  #include <asm/io_apic.h>
>  #include <asm/p2m.h>

This is the only change to this file, and there's no addition to
asm/hvm/ioreq.h - how come this #include is needed?

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -20,6 +20,8 @@
>  #include <xen/sched.h>
>  #include <xen/vpci.h>
>  
> +#include <asm/hvm/ioreq.h>

This of course means a step away from the code here being generic
enough such that Arm could eventually use it. Independent of this
aspect perhaps the #include would better move ...

> @@ -478,6 +480,61 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned int size,
>      spin_unlock(&pdev->vpci->lock);
>  }
>  
> +#ifdef __XEN__

... here?

> +static int ioreq_handler(ioreq_t *req, void *data)
> +{
> +    pci_sbdf_t sbdf;
> +
> +    /*
> +     * NB: certain requests of type different than PCI are broadcasted to all
> +     * registered ioreq servers, ignored those.
> +     */
> +    if ( req->type != IOREQ_TYPE_PCI_CONFIG || req->data_is_ptr )
> +        return X86EMUL_UNHANDLEABLE;

I understand the left side of the || , but why the right side? There
shouldn't be any IOREQ_TYPE_PCI_CONFIG requests with data_is_ptr set,
should there? I also didn't think requests with data_is_ptr set would
get broadcast?

> +int vpci_register_ioreq(struct domain *d)
> +{
> +    ioservid_t id;
> +    int rc;
> +
> +    if ( !has_vpci(d) )
> +        return 0;
> +
> +    rc = hvm_create_ioreq_server(d, HVM_IOREQSRV_BUFIOREQ_OFF, &id, true);
> +    if ( rc )
> +        return rc;
> +
> +    rc = hvm_set_ioreq_handler(d, id, ioreq_handler, NULL);
> +    if ( rc )
> +        return rc;
> +
> +    if ( is_hardware_domain(d) )
> +    {
> +        /* Handle all devices in vpci. */
> +        rc = hvm_map_io_range_to_ioreq_server(d, id, XEN_DMOP_IO_RANGE_PCI,
> +                                              0, ~(uint64_t)0);
> +        if ( rc )
> +            return rc;
> +    }
> +
> +    rc = hvm_set_ioreq_server_state(d, id, true);
> +    if ( rc )
> +        return rc;
> +
> +    return rc;

This last sequence of statements looks a little odd - is this in
anticipation of further additions to the function?

Furthermore the only caller expects the function to clean up after
itself (i.e. partially completed setup be undone), which doesn't
look to be the case here. I can't seem to be able to convince
myself that all of this gets cleaned up.

Jan

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [Xen-devel] [PATCH v3 10/10] ioreq: provide support for long-running operations...
  2019-09-30 13:32 ` [Xen-devel] [PATCH v3 10/10] ioreq: provide support for long-running operations Roger Pau Monne
@ 2019-10-25 14:35   ` Jan Beulich
  0 siblings, 0 replies; 24+ messages in thread
From: Jan Beulich @ 2019-10-25 14:35 UTC (permalink / raw)
  To: Roger Pau Monne
  Cc: Stefano Stabellini, WeiLiu, KonradRzeszutek Wilk, George Dunlap,
	Andrew Cooper, Paul Durrant, Ian Jackson, Tim Deegan,
	Julien Grall, xen-devel

On 30.09.2019 15:32, Roger Pau Monne wrote:
> ...and switch vPCI to use this infrastructure for long running
> physmap modification operations.
> 
> This allows to get rid of the vPCI specific modifications done to
> handle_hvm_io_completion and allows generalizing the support for
> long-running operations to other internal ioreq servers. Such support
> is implemented as a specific handler that can be registers by internal
> ioreq servers and that will be called to check for pending work.
> Returning true from this handler will prevent the vcpu from running
> until the handler returns false.

Is the mentioning of a special handler stale from perhaps a
different earlier implementation? Unless I'm overlooking /
misunderstanding something here, with this adjusted ...

> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2019-10-25 14:35 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-30 13:32 [Xen-devel] [PATCH v3 00/10] ioreq: add support for internal servers Roger Pau Monne
2019-09-30 13:32 ` [Xen-devel] [PATCH v3 01/10] ioreq: terminate cf8 handling at hypervisor level Roger Pau Monne
2019-10-01  9:50   ` Paul Durrant
2019-10-02 14:19   ` Jan Beulich
2019-09-30 13:32 ` [Xen-devel] [PATCH v3 02/10] ioreq: switch selection and forwarding to use ioservid_t Roger Pau Monne
2019-09-30 13:32 ` [Xen-devel] [PATCH v3 03/10] ioreq: add fields to allow internal ioreq servers Roger Pau Monne
2019-10-02 14:22   ` Jan Beulich
2019-09-30 13:32 ` [Xen-devel] [PATCH v3 04/10] ioreq: add internal ioreq initialization support Roger Pau Monne
2019-10-01  9:57   ` Andrew Cooper
2019-10-01 10:43     ` Roger Pau Monné
2019-10-02 14:47   ` Jan Beulich
2019-09-30 13:32 ` [Xen-devel] [PATCH v3 05/10] ioreq: allow dispatching ioreqs to internal servers Roger Pau Monne
2019-10-02 15:05   ` Jan Beulich
2019-09-30 13:32 ` [Xen-devel] [PATCH v3 06/10] ioreq: allow registering internal ioreq server handler Roger Pau Monne
2019-10-01 10:03   ` Paul Durrant
2019-10-02 15:08   ` Jan Beulich
2019-09-30 13:32 ` [Xen-devel] [PATCH v3 07/10] ioreq: allow decoding accesses to MMCFG regions Roger Pau Monne
2019-10-01 10:37   ` Paul Durrant
2019-10-07 11:45   ` Jan Beulich
2019-09-30 13:32 ` [Xen-devel] [PATCH v3 08/10] vpci: register as an internal ioreq server Roger Pau Monne
2019-10-07 12:32   ` Jan Beulich
2019-09-30 13:32 ` [Xen-devel] [PATCH v3 09/10] ioreq: split the code to detect PCI config space accesses Roger Pau Monne
2019-09-30 13:32 ` [Xen-devel] [PATCH v3 10/10] ioreq: provide support for long-running operations Roger Pau Monne
2019-10-25 14:35   ` Jan Beulich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).