[PATCH 0/2] x86/io: fix dpci_ IO port handlers

[PATCH 0/2] x86/io: fix dpci_ IO port handlers

[Roger Pau Monne]

Hello,

The following series contain two cosmetic fixes for the HVM IO port handlers.
The first one renames them to use the hvm_ 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.

Thanks, Roger.

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

[PATCH 1/2] x86/io: rename misleading dpci_ prefixed functions to hvm_

[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 hvm_ prefix in order to avoid this confusion.

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>
---
 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 0282986..f9d4234 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -641,7 +641,7 @@ int hvm_domain_initialise(struct domain *d)
     else
         d->arch.hvm_domain.io_bitmap = hvm_io_bitmap;
 
-    register_dpci_portio_handler(d);
+    register_hvm_portio_handler(d);
 
     hvm_ioreq_init(d);
 
diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
index 2770ff4..cb567f1 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 hvm_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 hvm_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 hvm_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 hvm_portio_ops = {
+    .accept = hvm_portio_accept,
+    .read = hvm_portio_read,
+    .write = hvm_portio_write
 };
 
-void register_dpci_portio_handler(struct domain *d)
+void register_hvm_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 = &hvm_portio_ops;
 }
 
 /*
diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
index d6801c1..4b0546a 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_hvm_portio_handler(struct domain *d);
 
 #endif /* __ASM_X86_HVM_IO_H__ */
 
-- 
2.10.1 (Apple Git-78)


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

[PATCH 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>
---
 xen/arch/x86/domctl.c               | 10 +++++-----
 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, 22 insertions(+), 19 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 1220224..b7b8748 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -721,11 +721,11 @@ 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;
 
@@ -747,14 +747,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 +773,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 +788,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 f9d4234..9374073 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);
 
@@ -707,6 +708,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;
 
@@ -722,6 +726,14 @@ void hvm_domain_destroy(struct domain *d)
 
     xfree(d->arch.hvm_domain.pl_time);
     d->arch.hvm_domain.pl_time = 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 cb567f1..d2ab883 100644
--- a/xen/arch/x86/hvm/io.c
+++ b/xen/arch/x86/hvm/io.c
@@ -171,12 +171,12 @@ static bool_t hvm_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 750c663..0253823 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 420cbdc..bdfc082 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 be95106..0008505 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.10.1 (Apple Git-78)


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

Re: [PATCH 1/2] x86/io: rename misleading dpci_ prefixed functions to hvm_

[Paul Durrant]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 27 March 2017 16:35
> To: xen-devel@lists.xenproject.org
> Cc: Roger Pau Monne <roger.pau@citrix.com>; Jan Beulich
> <jbeulich@suse.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul
> Durrant <Paul.Durrant@citrix.com>
> Subject: [PATCH 1/2] x86/io: rename misleading dpci_ prefixed functions to
> hvm_
> 
> 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 hvm_ prefix in order to avoid this confusion.

Actually, I wonder whether a prefix of 'g2m' would be better since  'hvm' sounds pretty generic.

  Paul

> 
> 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>
> ---
>  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 0282986..f9d4234 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -641,7 +641,7 @@ int hvm_domain_initialise(struct domain *d)
>      else
>          d->arch.hvm_domain.io_bitmap = hvm_io_bitmap;
> 
> -    register_dpci_portio_handler(d);
> +    register_hvm_portio_handler(d);
> 
>      hvm_ioreq_init(d);
> 
> diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c
> index 2770ff4..cb567f1 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 hvm_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 hvm_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 hvm_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 hvm_portio_ops = {
> +    .accept = hvm_portio_accept,
> +    .read = hvm_portio_read,
> +    .write = hvm_portio_write
>  };
> 
> -void register_dpci_portio_handler(struct domain *d)
> +void register_hvm_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 = &hvm_portio_ops;
>  }
> 
>  /*
> diff --git a/xen/include/asm-x86/hvm/io.h b/xen/include/asm-x86/hvm/io.h
> index d6801c1..4b0546a 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_hvm_portio_handler(struct domain *d);
> 
>  #endif /* __ASM_X86_HVM_IO_H__ */
> 
> --
> 2.10.1 (Apple Git-78)

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

Re: [PATCH 1/2] x86/io: rename misleading dpci_ prefixed functions to hvm_

[Roger Pau Monne]

On Tue, Mar 28, 2017 at 09:31:47AM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Roger Pau Monne
> > Sent: 27 March 2017 16:35
> > To: xen-devel@lists.xenproject.org
> > Cc: Roger Pau Monne <roger.pau@citrix.com>; Jan Beulich
> > <jbeulich@suse.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul
> > Durrant <Paul.Durrant@citrix.com>
> > Subject: [PATCH 1/2] x86/io: rename misleading dpci_ prefixed functions to
> > hvm_
> > 
> > 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 hvm_ prefix in order to avoid this confusion.
> 
> Actually, I wonder whether a prefix of 'g2m' would be better since  'hvm' sounds pretty generic.

Yes, I've also been wandering whether to use "direct_", but g2m sounds better.

Thanks, Roger.

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