[v6,4/6] xen/x86: Allow stubdom access to irq created for msi.

Commit Message

Marek Marczykowski-Górecki Sept. 14, 2019, 3:37 p.m. UTC

Stubdomains need to be given sufficient privilege over the guest which it
provides emulation for in order for PCI passthrough to work correctly.
When a HVM domain try to enable MSI, QEMU in stubdomain calls
PHYSDEVOP_map_pirq, but later it needs to call XEN_DOMCTL_bind_pt_irq as
part of xc_domain_update_msi_irq. Allow for that as part of
PHYSDEVOP_map_pirq.

This is not needed for PCI INTx, because IRQ in that case is known
beforehand and the stubdomain is given permissions over this IRQ by
libxl__device_pci_add (there's a do_pci_add against the stubdomain).

create_irq() already grant IRQ access to hardware_domain, with
assumption the device model (something managing this IRQ) lives there.
Modify create_irq() to take additional parameter pointing at device
model domain - which may be dom0 or stubdomain.  Save ID of the domain
given permission, to revoke it in destroy_irq() - easier and cleaner
than replaying logic of create_irq() parameter. Use domid instead of
actual reference to the domain, because it might get destroyed before
destroying IRQ (stubdomain is destroyed before its target domain). And
it is not an issue, because IRQ permissions live within domain
structure, so destroying a domain also implicitly revoke the permission.
Potential domid reuse is detected by by checking if that domain does
have permission over the IRQ being destroyed.

Then, adjust all callers to provide the parameter. In case of calls not
related to stubdomain-initiated allocations, give it either
hardware_domain (so the behavior is unchanged there), or NULL for
interrupts used by Xen internally.

Inspired by https://github.com/OpenXT/xenclient-oe/blob/5e0e7304a5a3c75ef01240a1e3673665b2aaf05e/recipes-extended/xen/files/stubdomain-msi-irq-access.patch by Eric Chanudet <[e-mail]>.

Signed-off-by: Simon Gaiser <[e-mail]>
Signed-off-by: Marek Marczykowski-Górecki <[e-mail]>
---
Changes in v3:
 - extend commit message
Changes in v4:
 - add missing destroy_irq on error path
Changes in v5:
 - move irq_{grant,revoke}_access() to {create,destroy}_irq(), which
   basically make it a different patch
 - add get_dm_domain() helper
 - do not give hardware_domain permission over IRQs used in Xen
   internally
 - rename create_irq argument to just 'd', to avoid confusion
   when it's called by hardware domain
 - verify that device is de-assigned before pci_remove_device call
 - save ID of domain given permission in create_irq(), to revoke it in
 destroy_irq()
 - drop domain parameter from destroy_irq() and msi_free_irq()
 - do not give hardware domain permission over IRQ created in
 iommu_set_interrupt()
Changes in v6:
 - do not give permission over hpet irq to hardware_domain
 - move creator_domid to arch_irq_desc
 - fix creator_domid initialization
 - always give current->domain permission instead of using
 get_dm_domain() helper. Analysis of all its use cases tells that it is
 the only value it returns.
 - drop unrelated change
---
 xen/arch/x86/hpet.c                      |  3 +-
 xen/arch/x86/irq.c                       | 51 ++++++++++++++++++-------
 xen/drivers/char/ns16550.c               |  2 +-
 xen/drivers/passthrough/amd/iommu_init.c |  2 +-
 xen/drivers/passthrough/vtd/iommu.c      |  3 +-
 xen/include/asm-x86/irq.h                |  7 ++-
 6 files changed, 50 insertions(+), 18 deletions(-)

Comments

Jan Beulich Sept. 20, 2019, 9:23 a.m. UTC | #1

On 14.09.2019 17:37, Marek Marczykowski-Górecki  wrote:
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -254,7 +254,13 @@ void __init clear_irq_vector(int irq)
>  /*
>   * Dynamic irq allocate and deallocation for MSI
>   */
> -int create_irq(nodeid_t node)
> +
> +/*
> + * create_irq - allocate irq for MSI
> + * @d domain that will get permission over the allocated irq; this permission
> + * will automatically be revoked on destroy_irq
> + */
> +int create_irq(nodeid_t node, struct domain *d)

I think there's nothing wrong with the pointer getting constified,
but see also below.

> @@ -282,23 +288,30 @@ int create_irq(nodeid_t node)
>          }
>          ret = assign_irq_vector(irq, mask);
>      }
> +    ASSERT(desc->arch.creator_domid == DOMID_INVALID);
>      if (ret < 0)

I think this insertion wants to gain blank lines on both sides.

>      {
>          desc->arch.used = IRQ_UNUSED;
>          irq = ret;
>      }
> -    else if ( hardware_domain )
> +    else if ( d )
>      {
> -        ret = irq_permit_access(hardware_domain, irq);
> +        ASSERT(d == current->domain);

Why pass in the domain then in the first place? Could by just a
boolean, couldn't it? Suitably named it might even eliminate
the need for the explanatory comment (see also below).

> +        ret = irq_permit_access(d, irq);
>          if ( ret )
>              printk(XENLOG_G_ERR
> -                   "Could not grant Dom0 access to IRQ%d (error %d)\n",
> -                   irq, ret);
> +                   "Could not grant Dom%u access to IRQ%d (error %d)\n",
> +                   d->domain_id, irq, ret);

Please use %pd here (and elsewhere).

> +        else
> +            desc->arch.creator_domid = d->domain_id;
>      }
>  
>      return irq;
>  }
>  
> +/*
> + * destroy_irq - deallocate irq for MSI
> + */
>  void destroy_irq(unsigned int irq)

I don't think this is a very helpful comment to add; in fact I think
the respective part on the other function would better be dropped as
well, seeing the further comment ahead of both functions. (Otherwise
I'd have to point out that this is a single line comment.)

> @@ -307,14 +320,25 @@ void destroy_irq(unsigned int irq)
>  
>      BUG_ON(!MSI_IRQ(irq));
>  
> -    if ( hardware_domain )
> +    if ( desc->arch.creator_domid != DOMID_INVALID )
>      {
> -        int err = irq_deny_access(hardware_domain, irq);
> +        struct domain *d = get_domain_by_id(desc->arch.creator_domid);
>  
> -        if ( err )
> -            printk(XENLOG_G_ERR
> -                   "Could not revoke Dom0 access to IRQ%u (error %d)\n",
> -                   irq, err);
> +        if ( d && irq_access_permitted(d, irq) )
> +        {
> +            int err;
> +
> +            err = irq_deny_access(d, irq);

Please keep prior code structure, i.e. the function call being the
initializer of the variable.

> +            if ( err )
> +                printk(XENLOG_G_ERR
> +                       "Could not revoke Dom%u access to IRQ%u (error %d)\n",
> +                       d->domain_id, irq, err);
> +        }

Why the irq_access_permitted() check around this? You go to some
lengths to explain this in the description, but if the domain has
no permission over the IRQ (because of domain ID re-use),
irq_deny_access() will simply do nothing, won't it? I.e. the way
this gets done and explained (saying that MSI IRQs can't be
shared between domains) wants to change a little.

> --- a/xen/include/asm-x86/irq.h
> +++ b/xen/include/asm-x86/irq.h
> @@ -45,6 +45,11 @@ struct arch_irq_desc {
>          unsigned move_cleanup_count;
>          u8 move_in_progress : 1;
>          s8 used;
> +        /*
> +         * weak reference to domain having permission over this IRQ (which can
> +         * be different from the domain actually havint the IRQ assigned)
> +         */
> +        domid_t creator_domid;

Comment style (should start with a capital letter).

Jan

Patch

diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 4b08488..5ed4405 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -11,6 +11,7 @@ 
 #include <xen/softirq.h>
 #include <xen/irq.h>
 #include <xen/numa.h>
+#include <xen/sched.h>
 #include <asm/fixmap.h>
 #include <asm/div64.h>
 #include <asm/hpet.h>
@@ -368,7 +369,7 @@  static int __init hpet_assign_irq(struct hpet_event_channel *ch)
 {
     int irq;
 
-    if ( (irq = create_irq(NUMA_NO_NODE)) < 0 )
+    if ( (irq = create_irq(NUMA_NO_NODE, NULL)) < 0 )
         return irq;
 
     ch->msi.irq = irq;
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 0ee3346..0b4c20a 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -254,7 +254,13 @@  void __init clear_irq_vector(int irq)
 /*
  * Dynamic irq allocate and deallocation for MSI
  */
-int create_irq(nodeid_t node)
+
+/*
+ * create_irq - allocate irq for MSI
+ * @d domain that will get permission over the allocated irq; this permission
+ * will automatically be revoked on destroy_irq
+ */
+int create_irq(nodeid_t node, struct domain *d)
 {
     int irq, ret;
     struct irq_desc *desc;
@@ -282,23 +288,30 @@  int create_irq(nodeid_t node)
         }
         ret = assign_irq_vector(irq, mask);
     }
+    ASSERT(desc->arch.creator_domid == DOMID_INVALID);
     if (ret < 0)
     {
         desc->arch.used = IRQ_UNUSED;
         irq = ret;
     }
-    else if ( hardware_domain )
+    else if ( d )
     {
-        ret = irq_permit_access(hardware_domain, irq);
+        ASSERT(d == current->domain);
+        ret = irq_permit_access(d, irq);
         if ( ret )
             printk(XENLOG_G_ERR
-                   "Could not grant Dom0 access to IRQ%d (error %d)\n",
-                   irq, ret);
+                   "Could not grant Dom%u access to IRQ%d (error %d)\n",
+                   d->domain_id, irq, ret);
+        else
+            desc->arch.creator_domid = d->domain_id;
     }
 
     return irq;
 }
 
+/*
+ * destroy_irq - deallocate irq for MSI
+ */
 void destroy_irq(unsigned int irq)
 {
     struct irq_desc *desc = irq_to_desc(irq);
@@ -307,14 +320,25 @@  void destroy_irq(unsigned int irq)
 
     BUG_ON(!MSI_IRQ(irq));
 
-    if ( hardware_domain )
+    if ( desc->arch.creator_domid != DOMID_INVALID )
     {
-        int err = irq_deny_access(hardware_domain, irq);
+        struct domain *d = get_domain_by_id(desc->arch.creator_domid);
 
-        if ( err )
-            printk(XENLOG_G_ERR
-                   "Could not revoke Dom0 access to IRQ%u (error %d)\n",
-                   irq, err);
+        if ( d && irq_access_permitted(d, irq) )
+        {
+            int err;
+
+            err = irq_deny_access(d, irq);
+            if ( err )
+                printk(XENLOG_G_ERR
+                       "Could not revoke Dom%u access to IRQ%u (error %d)\n",
+                       d->domain_id, irq, err);
+        }
+
+        if ( d )
+            put_domain(d);
+
+        desc->arch.creator_domid = DOMID_INVALID;
     }
 
     spin_lock_irqsave(&desc->lock, flags);
@@ -381,6 +405,7 @@  int arch_init_one_irq_desc(struct irq_desc *desc)
 
     desc->arch.vector = IRQ_VECTOR_UNASSIGNED;
     desc->arch.old_vector = IRQ_VECTOR_UNASSIGNED;
+    desc->arch.creator_domid = DOMID_INVALID;
 
     return 0;
 }
@@ -2133,7 +2158,7 @@  int map_domain_pirq(
             spin_unlock_irqrestore(&desc->lock, flags);
 
             info = NULL;
-            irq = create_irq(NUMA_NO_NODE);
+            irq = create_irq(NUMA_NO_NODE, current->domain);
             ret = irq >= 0 ? prepare_domain_irq_pirq(d, irq, pirq + nr, &info)
                            : irq;
             if ( ret < 0 )
@@ -2818,7 +2843,7 @@  int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p,
         if ( irq == -1 )
         {
     case MAP_PIRQ_TYPE_MULTI_MSI:
-            irq = create_irq(NUMA_NO_NODE);
+            irq = create_irq(NUMA_NO_NODE, current->domain);
         }
 
         if ( irq < nr_irqs_gsi || irq >= nr_irqs )
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 8667de6..66cc680 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -722,7 +722,7 @@  static void __init ns16550_init_irq(struct serial_port *port)
     struct ns16550 *uart = port->uart;
 
     if ( uart->msi )
-        uart->irq = create_irq(0);
+        uart->irq = create_irq(0, NULL);
 #endif
 }
 
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index bb9f33e..9af4b7c 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -765,7 +765,7 @@  static bool_t __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
 {
     int irq, ret;
 
-    irq = create_irq(NUMA_NO_NODE);
+    irq = create_irq(NUMA_NO_NODE, NULL);
     if ( irq <= 0 )
     {
         dprintk(XENLOG_ERR, "IOMMU: no irqs\n");
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 5d72270..7440bac 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1138,7 +1138,8 @@  static int __init iommu_set_interrupt(struct acpi_drhd_unit *drhd)
     struct irq_desc *desc;
 
     irq = create_irq(rhsa ? pxm_to_node(rhsa->proximity_domain)
-                          : NUMA_NO_NODE);
+                          : NUMA_NO_NODE,
+                     NULL);
     if ( irq <= 0 )
     {
         dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: no irq available!\n");
diff --git a/xen/include/asm-x86/irq.h b/xen/include/asm-x86/irq.h
index bc0c0c1..7cf8a1b 100644
--- a/xen/include/asm-x86/irq.h
+++ b/xen/include/asm-x86/irq.h
@@ -45,6 +45,11 @@  struct arch_irq_desc {
         unsigned move_cleanup_count;
         u8 move_in_progress : 1;
         s8 used;
+        /*
+         * weak reference to domain having permission over this IRQ (which can
+         * be different from the domain actually havint the IRQ assigned)
+         */
+        domid_t creator_domid;
 };
 
 /* For use with irq_desc.arch.used */
@@ -161,7 +166,7 @@  int  init_irq_data(void);
 void clear_irq_vector(int irq);
 
 int irq_to_vector(int irq);
-int create_irq(nodeid_t node);
+int create_irq(nodeid_t node, struct domain *d);
 void destroy_irq(unsigned int irq);
 int assign_irq_vector(int irq, const cpumask_t *);