[PATCH for-4.9 v3 0/2] x86/io: rename dpci_ IO port handlers

[PATCH for-4.9 v3 0/2] x86/io: rename dpci_ IO port handlers

[Roger Pau Monne]

Hello,

The following series contain two mostly cosmetic fixes for the g2m IO port handlers.
The first one renames them to use the g2m_ prefix instead of the dpci_ one,
since those handlers are not trapping the PCI config space in any way. The
second fix moves the list of ports from the domain_iommu struct to the
hvm_struct, where it fits more naturally.

I think they are trivial enough so that they can be committed, the risk of them
breaking anything (that's not already broken) is very low IMHO. Julien, can I
have your opinion please?

Thanks, Roger.


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

[PATCH for-4.9 v3 1/2] x86/io: rename misleading dpci_ prefixed functions to g2m_

[Roger Pau Monne]

The dpci_ prefix used on those IO handlers is misleading, there's nothing PCI
specific in them, they simply map a guest IO port into a machine (physical) IO
port. They don't specifically trap the PCI IO port range in any way
(0xcf8/0xcfc).

Rename them to use the g2m_ prefix in order to avoid this confusion.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Paul Durrant <paul.durrant@citrix.com>
---
 xen/arch/x86/hvm/hvm.c       |  2 +-
 xen/arch/x86/hvm/io.c        | 28 ++++++++++++----------------
 xen/include/asm-x86/hvm/io.h |  6 +++++-
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 2e76c2345b..b1da2d3dfa 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -649,7 +649,7 @@ int hvm_domain_initialise(struct domain *d)
     else
         d->arch.hvm_domain.io_bitmap = hvm_io_bitmap;
 
-    register_dpci_portio_handler(d);
+    register_g2m_portio_handler(d);
 
     hvm_ioreq_init(d);
 
diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
index 2770ff4fb2..3b3a600983 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -167,8 +167,8 @@ bool handle_pio(uint16_t port, unsigned int size, int dir)
     return true;
 }
 
-static bool_t dpci_portio_accept(const struct hvm_io_handler *handler,
-                                 const ioreq_t *p)
+static bool_t g2m_portio_accept(const struct hvm_io_handler *handler,
+                                const ioreq_t *p)
 {
     struct vcpu *curr = current;
     const struct domain_iommu *dio = dom_iommu(curr->domain);
@@ -190,10 +190,8 @@ static bool_t dpci_portio_accept(const struct hvm_io_handler *handler,
     return 0;
 }
 
-static int dpci_portio_read(const struct hvm_io_handler *handler,
-                            uint64_t addr,
-                            uint32_t size,
-                            uint64_t *data)
+static int g2m_portio_read(const struct hvm_io_handler *handler,
+                           uint64_t addr, uint32_t size, uint64_t *data)
 {
     struct hvm_vcpu_io *vio = &current->arch.hvm_vcpu.hvm_io;
     const struct g2m_ioport *g2m_ioport = vio->g2m_ioport;
@@ -217,10 +215,8 @@ static int dpci_portio_read(const struct hvm_io_handler *handler,
     return X86EMUL_OKAY;
 }
 
-static int dpci_portio_write(const struct hvm_io_handler *handler,
-                             uint64_t addr,
-                             uint32_t size,
-                             uint64_t data)
+static int g2m_portio_write(const struct hvm_io_handler *handler,
+                            uint64_t addr, uint32_t size, uint64_t data)
 {
     struct hvm_vcpu_io *vio = &current->arch.hvm_vcpu.hvm_io;
     const struct g2m_ioport *g2m_ioport = vio->g2m_ioport;
@@ -244,13 +240,13 @@ static int dpci_portio_write(const struct hvm_io_handler *handler,
     return X86EMUL_OKAY;
 }
 
-static const struct hvm_io_ops dpci_portio_ops = {
-    .accept = dpci_portio_accept,
-    .read = dpci_portio_read,
-    .write = dpci_portio_write
+static const struct hvm_io_ops g2m_portio_ops = {
+    .accept = g2m_portio_accept,
+    .read = g2m_portio_read,
+    .write = g2m_portio_write
 };
 
-void register_dpci_portio_handler(struct domain *d)
+void register_g2m_portio_handler(struct domain *d)
 {
     struct hvm_io_handler *handler = hvm_next_io_handler(d);
 
@@ -258,7 +254,7 @@ void register_dpci_portio_handler(struct domain *d)
         return;
 
     handler->type = IOREQ_TYPE_PIO;
-    handler->ops = &dpci_portio_ops;
+    handler->ops = &g2m_portio_ops;
 }
 
 /*
diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
index d6801c17e0..5ae9225baa 100644
--- a/xen/include/asm-x86/hvm/io.h
+++ b/xen/include/asm-x86/hvm/io.h
@@ -148,7 +148,11 @@ void stdvga_deinit(struct domain *d);
 
 extern void hvm_dpci_msi_eoi(struct domain *d, int vector);
 
-void register_dpci_portio_handler(struct domain *d);
+/*
+ * HVM port IO handler that performs forwarding of guest IO ports into machine
+ * IO ports.
+ */
+void register_g2m_portio_handler(struct domain *d);
 
 #endif /* __ASM_X86_HVM_IO_H__ */
 
-- 
2.11.0 (Apple Git-81)


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

[PATCH for-4.9 v3 2/2] x86/io: move the list of guest to machine IO ports out of domain_iommu

[Roger Pau Monne]

There's no reason to store that list inside of the domain_iommu struct, the
forwarding of guest IO ports into machine IO ports is not tied to the presence
of an IOMMU.

Move it inside of the hvm_domain struct instead.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Paul Durrant <paul.durrant@citrix.com>
---
Changes since v2:
 - Added a check in XEN_DOMCTL_ioport_mapping to make sure it's only used
   against HVM domains.
---
 xen/arch/x86/domctl.c               | 16 +++++++++++-----
 xen/arch/x86/hvm/hvm.c              | 12 ++++++++++++
 xen/arch/x86/hvm/io.c               |  4 ++--
 xen/drivers/passthrough/x86/iommu.c | 11 -----------
 xen/include/asm-x86/hvm/domain.h    |  3 +++
 xen/include/asm-x86/iommu.h         |  1 -
 6 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 1220224f70..45fad81de1 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -721,14 +721,20 @@ long arch_do_domctl(
 
     case XEN_DOMCTL_ioport_mapping:
     {
-        struct domain_iommu *hd;
         unsigned int fgp = domctl->u.ioport_mapping.first_gport;
         unsigned int fmp = domctl->u.ioport_mapping.first_mport;
         unsigned int np = domctl->u.ioport_mapping.nr_ports;
         unsigned int add = domctl->u.ioport_mapping.add_mapping;
+        struct hvm_domain *hvm_domain;
         struct g2m_ioport *g2m_ioport;
         int found = 0;
 
+        ret = -EOPNOTSUPP;
+        if ( !is_hvm_domain(d) )
+        {
+            printk(XENLOG_G_ERR "ioport_map against non-HVM domain\n");
+            break;
+        }
         ret = -EINVAL;
         if ( ((fgp | fmp | (np - 1)) >= MAX_IOPORTS) ||
             ((fgp + np) > MAX_IOPORTS) || ((fmp + np) > MAX_IOPORTS) )
@@ -747,14 +753,14 @@ long arch_do_domctl(
         if ( ret )
             break;
 
-        hd = dom_iommu(d);
+        hvm_domain = &d->arch.hvm_domain;
         if ( add )
         {
             printk(XENLOG_G_INFO
                    "ioport_map:add: dom%d gport=%x mport=%x nr=%x\n",
                    d->domain_id, fgp, fmp, np);
 
-            list_for_each_entry(g2m_ioport, &hd->arch.g2m_ioport_list, list)
+            list_for_each_entry(g2m_ioport, &hvm_domain->g2m_ioport_list, list)
                 if (g2m_ioport->mport == fmp )
                 {
                     g2m_ioport->gport = fgp;
@@ -773,7 +779,7 @@ long arch_do_domctl(
                 g2m_ioport->gport = fgp;
                 g2m_ioport->mport = fmp;
                 g2m_ioport->np = np;
-                list_add_tail(&g2m_ioport->list, &hd->arch.g2m_ioport_list);
+                list_add_tail(&g2m_ioport->list, &hvm_domain->g2m_ioport_list);
             }
             if ( !ret )
                 ret = ioports_permit_access(d, fmp, fmp + np - 1);
@@ -788,7 +794,7 @@ long arch_do_domctl(
             printk(XENLOG_G_INFO
                    "ioport_map:remove: dom%d gport=%x mport=%x nr=%x\n",
                    d->domain_id, fgp, fmp, np);
-            list_for_each_entry(g2m_ioport, &hd->arch.g2m_ioport_list, list)
+            list_for_each_entry(g2m_ioport, &hvm_domain->g2m_ioport_list, list)
                 if ( g2m_ioport->mport == fmp )
                 {
                     list_del(&g2m_ioport->list);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index b1da2d3dfa..6c3c944abd 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -608,6 +608,7 @@ int hvm_domain_initialise(struct domain *d)
     spin_lock_init(&d->arch.hvm_domain.uc_lock);
     spin_lock_init(&d->arch.hvm_domain.write_map.lock);
     INIT_LIST_HEAD(&d->arch.hvm_domain.write_map.list);
+    INIT_LIST_HEAD(&d->arch.hvm_domain.g2m_ioport_list);
 
     hvm_init_cacheattr_region_list(d);
 
@@ -716,6 +717,9 @@ void hvm_domain_relinquish_resources(struct domain *d)
 
 void hvm_domain_destroy(struct domain *d)
 {
+    struct list_head *ioport_list, *tmp;
+    struct g2m_ioport *ioport;
+
     xfree(d->arch.hvm_domain.io_handler);
     d->arch.hvm_domain.io_handler = NULL;
 
@@ -734,6 +738,14 @@ void hvm_domain_destroy(struct domain *d)
 
     xfree(d->arch.hvm_domain.irq);
     d->arch.hvm_domain.irq = NULL;
+
+    list_for_each_safe ( ioport_list, tmp,
+                         &d->arch.hvm_domain.g2m_ioport_list )
+    {
+        ioport = list_entry(ioport_list, struct g2m_ioport, list);
+        list_del(&ioport->list);
+        xfree(ioport);
+    }
 }
 
 static int hvm_save_tsc_adjust(struct domain *d, hvm_domain_context_t *h)
diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
index 3b3a600983..9e004098cc 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -171,12 +171,12 @@ static bool_t g2m_portio_accept(const struct hvm_io_handler *handler,
                                 const ioreq_t *p)
 {
     struct vcpu *curr = current;
-    const struct domain_iommu *dio = dom_iommu(curr->domain);
+    const struct hvm_domain *hvm_domain = &curr->domain->arch.hvm_domain;
     struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io;
     struct g2m_ioport *g2m_ioport;
     unsigned int start, end;
 
-    list_for_each_entry( g2m_ioport, &dio->arch.g2m_ioport_list, list )
+    list_for_each_entry( g2m_ioport, &hvm_domain->g2m_ioport_list, list )
     {
         start = g2m_ioport->gport;
         end = start + g2m_ioport->np;
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 750c663567..0253823173 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -123,7 +123,6 @@ int arch_iommu_domain_init(struct domain *d)
     struct domain_iommu *hd = dom_iommu(d);
 
     spin_lock_init(&hd->arch.mapping_lock);
-    INIT_LIST_HEAD(&hd->arch.g2m_ioport_list);
     INIT_LIST_HEAD(&hd->arch.mapped_rmrrs);
 
     return 0;
@@ -131,16 +130,6 @@ int arch_iommu_domain_init(struct domain *d)
 
 void arch_iommu_domain_destroy(struct domain *d)
 {
-    const struct domain_iommu *hd = dom_iommu(d);
-    struct list_head *ioport_list, *tmp;
-    struct g2m_ioport *ioport;
-
-    list_for_each_safe ( ioport_list, tmp, &hd->arch.g2m_ioport_list )
-    {
-        ioport = list_entry(ioport_list, struct g2m_ioport, list);
-        list_del(&ioport->list);
-        xfree(ioport);
-    }
 }
 
 /*
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index c3cca94a97..622e1ca12d 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -180,6 +180,9 @@ struct hvm_domain {
 
     unsigned long *io_bitmap;
 
+    /* List of guest to machine IO ports mapping. */
+    struct list_head g2m_ioport_list;
+
     /* List of permanently write-mapped pages. */
     struct {
         spinlock_t lock;
diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
index be951068d8..0008505699 100644
--- a/xen/include/asm-x86/iommu.h
+++ b/xen/include/asm-x86/iommu.h
@@ -34,7 +34,6 @@ struct arch_iommu
     u64 pgd_maddr;                 /* io page directory machine address */
     spinlock_t mapping_lock;            /* io page table lock */
     int agaw;     /* adjusted guest address width, 0 is level 2 30-bit */
-    struct list_head g2m_ioport_list;   /* guest to machine ioport mapping */
     u64 iommu_bitmap;              /* bitmap of iommu(s) that the domain uses */
     struct list_head mapped_rmrrs;
 
-- 
2.11.0 (Apple Git-81)


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

Re: [PATCH for-4.9 v3 2/2] x86/io: move the list of guest to machine IO ports out of domain_iommu

[Jan Beulich]

>>> On 05.04.17 at 11:00, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -721,14 +721,20 @@ long arch_do_domctl(
>  
>      case XEN_DOMCTL_ioport_mapping:
>      {
> -        struct domain_iommu *hd;
>          unsigned int fgp = domctl->u.ioport_mapping.first_gport;
>          unsigned int fmp = domctl->u.ioport_mapping.first_mport;
>          unsigned int np = domctl->u.ioport_mapping.nr_ports;
>          unsigned int add = domctl->u.ioport_mapping.add_mapping;
> +        struct hvm_domain *hvm_domain;
>          struct g2m_ioport *g2m_ioport;
>          int found = 0;
>  
> +        ret = -EOPNOTSUPP;
> +        if ( !is_hvm_domain(d) )
> +        {
> +            printk(XENLOG_G_ERR "ioport_map against non-HVM domain\n");
> +            break;
> +        }
>          ret = -EINVAL;

There should be a blank line above this one, which can of course
be added while committing. With that
Reviewed-by: Jan Beulich <jbeulich@suse.com>
but I think we should still get clarification on the supposed earlier
regression before committing this.

Jan


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

Re: [PATCH for-4.9 v3 2/2] x86/io: move the list of guest to machine IO ports out of domain_iommu

[Roger Pau Monne]

On Wed, Apr 05, 2017 at 03:17:17AM -0600, Jan Beulich wrote:
> >>> On 05.04.17 at 11:00, <roger.pau@citrix.com> wrote:
> > --- a/xen/arch/x86/domctl.c
> > +++ b/xen/arch/x86/domctl.c
> > @@ -721,14 +721,20 @@ long arch_do_domctl(
> >  
> >      case XEN_DOMCTL_ioport_mapping:
> >      {
> > -        struct domain_iommu *hd;
> >          unsigned int fgp = domctl->u.ioport_mapping.first_gport;
> >          unsigned int fmp = domctl->u.ioport_mapping.first_mport;
> >          unsigned int np = domctl->u.ioport_mapping.nr_ports;
> >          unsigned int add = domctl->u.ioport_mapping.add_mapping;
> > +        struct hvm_domain *hvm_domain;
> >          struct g2m_ioport *g2m_ioport;
> >          int found = 0;
> >  
> > +        ret = -EOPNOTSUPP;
> > +        if ( !is_hvm_domain(d) )
> > +        {
> > +            printk(XENLOG_G_ERR "ioport_map against non-HVM domain\n");
> > +            break;
> > +        }
> >          ret = -EINVAL;
> 
> There should be a blank line above this one, which can of course
> be added while committing. With that
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> but I think we should still get clarification on the supposed earlier
> regression before committing this.

Thanks.

The only callers of this domctl (xc_domain_ioport_mapping) are in QEMU, so it's
very unlikely that this domctl was ever used with PV guests (also taking into
account what Jan said about the iommu struct).

Roger.

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