[Xen-devel] [PATCH v7 0/4] Fix PCI passthrough for HVM with stubdomain

[Xen-devel] [PATCH v7 0/4] Fix PCI passthrough for HVM with stubdomain

[Marek Marczykowski-Górecki]

In this version, I drop PHYSDEVOP_interrupt_control patch, since Jan prefer not
to mix pciif and hypercalls for serving device model. Enabling MSI by the
stubdomain remains unsolved here, but other patches are improvement anyway.

Changes in v2:
 - new "xen/x86: Allow stubdom access to irq created for msi" patch
 - applied review comments from v1
Changes is v3:
 - apply suggestions by Roger
 - add PHYSDEVOP_msi_msix_set_enable
Changes in v4:
 - implement suggestions by Wei, Roger, Jan
 - plug new physdevop into XSM
Changes in v5:
 - rebase on master
 - rename to PHYSDEVOP_msi_control
 - move granting access to IRQ into create_irq
Changes in v6:
 - simplify granting IRQ access, record dm domid for cleanup
 - rename to PHYSDEVOP_interrupt_control
 - include INTx control in the hypercall
Changes in v7:
 - update "xen/x86: Allow stubdom access to irq created for msi"
 - drop "xen/x86: add PHYSDEVOP_interrupt_control"
 - drop "tools/libxc: add wrapper for PHYSDEVOP_interrupt_control"

---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Brian Woods <brian.woods@amd.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Marek Marczykowski-Górecki (4):
  libxl: do not attach xen-pciback to HVM domain, if stubdomain is in use
  libxl: attach PCI device to qemu only after setting pciback/pcifront
  libxl: don't try to manipulate json config for stubdomain
  xen/x86: Allow stubdom access to irq created for msi.

 tools/libxl/libxl_pci.c                  | 63 +++++++++++++++++--------
 xen/arch/x86/hpet.c                      |  3 +-
 xen/arch/x86/irq.c                       | 42 +++++++++++------
 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 ++-
 7 files changed, 84 insertions(+), 38 deletions(-)

base-commit: 6c9639a72f0ca3a9430ef75f375877182281fdef
-- 
git-series 0.9.1

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

[Xen-devel] [PATCH v7 1/4] libxl: do not attach xen-pciback to HVM domain, if stubdomain is in use

[Marek Marczykowski-Górecki]

HVM domains use IOMMU and device model assistance for communicating with
PCI devices, xen-pcifront/pciback isn't directly needed by HVM domain.
But pciback serve also second function - it reset the device when it is
deassigned from the guest and for this reason pciback needs to be used
with HVM domain too.
When HVM domain has device model in stubdomain, attaching xen-pciback to
the target domain itself may prevent attaching xen-pciback to the
(PV) stubdomain, effectively breaking PCI passthrough.

Fix this by attaching pciback only to one domain: if PV stubdomain is in
use, let it be stubdomain (the commit prevents attaching device to target
HVM in this case); otherwise, attach it to the target domain.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
Changes in v2:
 - previously called "libxl: attach xen-pciback only to PV domains"
 - instead of excluding all HVMs, change the condition to what actually
   matters here - check if stubdomain is in use; this way xen-pciback is
   always in use (either for the target domain, or it's stubdomain),
   fixing PCI reset by xen-pciback concerns
Changes in v3:
 - adjust commit message
---
 tools/libxl/libxl_pci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 03beb86..2e06a45 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1106,7 +1106,7 @@ out:
         }
     }
 
-    if (!starting)
+    if (!starting && !libxl_get_stubdom_id(CTX, domid))
         rc = libxl__device_pci_add_xenstore(gc, domid, pcidev, starting);
     else
         rc = 0;
@@ -1302,7 +1302,7 @@ static void libxl__add_pcidevs(libxl__egc *egc, libxl__ao *ao, uint32_t domid,
         }
     }
 
-    if (d_config->num_pcidevs > 0) {
+    if (d_config->num_pcidevs > 0 && !libxl_get_stubdom_id(CTX, domid)) {
         rc = libxl__create_pci_backend(gc, domid, d_config->pcidevs,
             d_config->num_pcidevs);
         if (rc < 0) {
-- 
git-series 0.9.1

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

[Xen-devel] [PATCH v7 2/4] libxl: attach PCI device to qemu only after setting pciback/pcifront

[Marek Marczykowski-Górecki]

When qemu is running in stubdomain, handling "pci-ins" command will fail
if pcifront is not initialized already. Fix this by sending such command
only after confirming that pciback/front is running.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
Changes in v2:
- Fixed code style since previous version.
---
 tools/libxl/libxl_pci.c |  9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 2e06a45..578535f 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -1191,6 +1191,7 @@ int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcide
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     unsigned int orig_vdev, pfunc_mask;
+    char *be_path;
     libxl_device_pci *assigned;
     int num_assigned, i, rc;
     int stubdomid = 0;
@@ -1245,6 +1246,14 @@ int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pci *pcide
         rc = do_pci_add(gc, stubdomid, &pcidev_s, 0);
         if ( rc )
             goto out;
+        /* Wait for the device actually being connected, otherwise device model
+         * running there will fail to find the device. */
+        be_path = libxl__sprintf(gc, "%s/backend/pci/%d/0",
+                                 libxl__xs_get_dompath(gc, 0), stubdomid);
+        rc = libxl__wait_for_backend(gc, be_path,
+                                     GCSPRINTF("%d", XenbusStateConnected));
+        if (rc)
+            goto out;
     }
 
     orig_vdev = pcidev->vdevfn & ~7U;
-- 
git-series 0.9.1

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

[Xen-devel] [PATCH v7 3/4] libxl: don't try to manipulate json config for stubdomain

[Marek Marczykowski-Górecki]

Stubdomain do not have it's own config file - its configuration is
derived from target domains. Do not try to manipulate it when attaching
PCI device.

This bug prevented starting HVM with stubdomain and PCI passthrough
device attached.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
Changes in v3:
 - skip libxl__dm_check_start too, as stubdomain is guaranteed to be
   running at this stage already
 - do not init d_config at all, as it is used only for json manipulation
Changes in v4:
 - adjust comment style
---
 tools/libxl/libxl_pci.c | 50 ++++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 578535f..d26fc9a 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -120,10 +120,14 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
     libxl_domain_config d_config;
     libxl_device_pci pcidev_saved;
     libxl__domain_userdata_lock *lock = NULL;
+    bool is_stubdomain = libxl_is_stubdom(CTX, domid, NULL);
 
-    libxl_domain_config_init(&d_config);
-    libxl_device_pci_init(&pcidev_saved);
-    libxl_device_pci_copy(CTX, &pcidev_saved, pcidev);
+    /* Stubdomain doesn't have own config. */
+    if (!is_stubdomain) {
+        libxl_domain_config_init(&d_config);
+        libxl_device_pci_init(&pcidev_saved);
+        libxl_device_pci_copy(CTX, &pcidev_saved, pcidev);
+    }
 
     be_path = libxl__domain_device_backend_path(gc, 0, domid, 0,
                                                 LIBXL__DEVICE_KIND_PCI);
@@ -152,27 +156,35 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
     GCNEW(device);
     libxl__device_from_pcidev(gc, domid, pcidev, device);
 
-    lock = libxl__lock_domain_userdata(gc, domid);
-    if (!lock) {
-        rc = ERROR_LOCK_FAIL;
-        goto out;
-    }
+    /* 
+     * Stubdomin config is derived from its target domain, it doesn't have
+     * its own file.
+     */
+    if (!is_stubdomain) {
+        lock = libxl__lock_domain_userdata(gc, domid);
+        if (!lock) {
+            rc = ERROR_LOCK_FAIL;
+            goto out;
+        }
 
-    rc = libxl__get_domain_configuration(gc, domid, &d_config);
-    if (rc) goto out;
+        rc = libxl__get_domain_configuration(gc, domid, &d_config);
+        if (rc) goto out;
 
-    device_add_domain_config(gc, &d_config, &libxl__pcidev_devtype,
-                             &pcidev_saved);
+        device_add_domain_config(gc, &d_config, &libxl__pcidev_devtype,
+                                 &pcidev_saved);
 
-    rc = libxl__dm_check_start(gc, &d_config, domid);
-    if (rc) goto out;
+        rc = libxl__dm_check_start(gc, &d_config, domid);
+        if (rc) goto out;
+    }
 
     for (;;) {
         rc = libxl__xs_transaction_start(gc, &t);
         if (rc) goto out;
 
-        rc = libxl__set_domain_configuration(gc, domid, &d_config);
-        if (rc) goto out;
+        if (lock) {
+            rc = libxl__set_domain_configuration(gc, domid, &d_config);
+            if (rc) goto out;
+        }
 
         libxl__xs_writev(gc, t, be_path, libxl__xs_kvs_of_flexarray(gc, back));
 
@@ -184,8 +196,10 @@ static int libxl__device_pci_add_xenstore(libxl__gc *gc, uint32_t domid, libxl_d
 out:
     libxl__xs_transaction_abort(gc, &t);
     if (lock) libxl__unlock_domain_userdata(lock);
-    libxl_device_pci_dispose(&pcidev_saved);
-    libxl_domain_config_dispose(&d_config);
+    if (!is_stubdomain) {
+        libxl_device_pci_dispose(&pcidev_saved);
+        libxl_domain_config_dispose(&d_config);
+    }
     return rc;
 }
 
-- 
git-series 0.9.1

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

[Xen-devel] [PATCH v7 4/4] xen/x86: Allow stubdom access to irq created for msi.

[Marek Marczykowski-Górecki]

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 <chanudete@ainfosec.com>.

Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
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
Changes in v7:
 - Code style improvements (spaces, use %pd etc)
 - use bool parameter to create_irq, as it's only getting
 current->domain or NULL
 - remove redundant irq_access_permitted()
---
 xen/arch/x86/hpet.c                      |  3 +-
 xen/arch/x86/irq.c                       | 42 +++++++++++++++++--------
 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, 41 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 4b08488..57f68fa 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, false)) < 0 )
         return irq;
 
     ch->msi.irq = irq;
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 0ee3346..256dd02 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -254,7 +254,8 @@ void __init clear_irq_vector(int irq)
 /*
  * Dynamic irq allocate and deallocation for MSI
  */
-int create_irq(nodeid_t node)
+
+int create_irq(nodeid_t node, bool grant_access)
 {
     int irq, ret;
     struct irq_desc *desc;
@@ -282,18 +283,23 @@ 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 ( grant_access )
     {
-        ret = irq_permit_access(hardware_domain, irq);
+        ret = irq_permit_access(current->domain, irq);
         if ( ret )
             printk(XENLOG_G_ERR
-                   "Could not grant Dom0 access to IRQ%d (error %d)\n",
-                   irq, ret);
+                   "Could not grant %pd access to IRQ%d (error %d)\n",
+                   current->domain, irq, ret);
+        else
+            desc->arch.creator_domid = current->domain->domain_id;
     }
 
     return irq;
@@ -307,14 +313,23 @@ 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 )
+        {
+            int err = irq_deny_access(d, irq);
+            if ( err )
+                printk(XENLOG_G_ERR
+                       "Could not revoke %pd access to IRQ%u (error %d)\n",
+                       d, irq, err);
+        }
+
+        if ( d )
+            put_domain(d);
+
+        desc->arch.creator_domid = DOMID_INVALID;
     }
 
     spin_lock_irqsave(&desc->lock, flags);
@@ -381,6 +396,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 +2149,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, true);
             ret = irq >= 0 ? prepare_domain_irq_pirq(d, irq, pirq + nr, &info)
                            : irq;
             if ( ret < 0 )
@@ -2818,7 +2834,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, true);
         }
 
         if ( irq < nr_irqs_gsi || irq >= nr_irqs )
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 8667de6..fcd3979 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, false);
 #endif
 }
 
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index bb9f33e..233a8ae 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, false);
     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..24a1e92 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,
+                     false);
     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..79853d0 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, bool grant_access);
 void destroy_irq(unsigned int irq);
 int assign_irq_vector(int irq, const cpumask_t *);
 
-- 
git-series 0.9.1

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

Re: [Xen-devel] [PATCH v7 4/4] xen/x86: Allow stubdom access to irq created for msi.

[Roger Pau Monné]

On Wed, Sep 25, 2019 at 04:41:26AM +0200, Marek Marczykowski-Górecki wrote:
> 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.

I would replace the last sentence with: "Give the stubdomain enough
permissions over the mapped interrupt in order to bind it successfully
to it's target domain."

> 
> 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.

I would remove the "(something managing this IRQ)", I think it makes
the sentence harder to understand while not adding any valuable
information.

> 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
                                    ^ double by
> 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 <chanudete@ainfosec.com>.
> 
> Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

I have some minor comments below, but the patch LGTM, so:

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

It would still be nice to get the missing bits (interrupt enabling),
or else this patch is kind of pointless, since it still doesn't allow
stubdomains to work correctly with passed through devices.

> ---
> 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
> Changes in v7:
>  - Code style improvements (spaces, use %pd etc)
>  - use bool parameter to create_irq, as it's only getting
>  current->domain or NULL
>  - remove redundant irq_access_permitted()
> ---
>  xen/arch/x86/hpet.c                      |  3 +-
>  xen/arch/x86/irq.c                       | 42 +++++++++++++++++--------
>  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, 41 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
> index 4b08488..57f68fa 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, false)) < 0 )
>          return irq;
>  
>      ch->msi.irq = irq;
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index 0ee3346..256dd02 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -254,7 +254,8 @@ void __init clear_irq_vector(int irq)
>  /*
>   * Dynamic irq allocate and deallocation for MSI
>   */
> -int create_irq(nodeid_t node)
> +

Extra newline.

> +int create_irq(nodeid_t node, bool grant_access)
>  {
>      int irq, ret;
>      struct irq_desc *desc;
> @@ -282,18 +283,23 @@ 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 ( grant_access )
>      {
> -        ret = irq_permit_access(hardware_domain, irq);
> +        ret = irq_permit_access(current->domain, irq);
>          if ( ret )
>              printk(XENLOG_G_ERR
> -                   "Could not grant Dom0 access to IRQ%d (error %d)\n",
> -                   irq, ret);
> +                   "Could not grant %pd access to IRQ%d (error %d)\n",
> +                   current->domain, irq, ret);
> +        else
> +            desc->arch.creator_domid = current->domain->domain_id;
>      }
>  
>      return irq;
> @@ -307,14 +313,23 @@ 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 )
> +        {
> +            int err = irq_deny_access(d, irq);
> +            if ( err )
> +                printk(XENLOG_G_ERR
> +                       "Could not revoke %pd access to IRQ%u (error %d)\n",
> +                       d, irq, err);
> +        }
> +
> +        if ( d )
> +            put_domain(d);

You can place the put_domain inside the previous if AFAICT, since it's
the same condition.

> diff --git a/xen/include/asm-x86/irq.h b/xen/include/asm-x86/irq.h
> index bc0c0c1..79853d0 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)
                                                    ^ having
> +         */
> +        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);

I would add:

/*
 * If grant_access is set the current domain is given permissions over
 * the created IRQ.
 */

> +int create_irq(nodeid_t node, bool grant_access);
>  void destroy_irq(unsigned int irq);
>  int assign_irq_vector(int irq, const cpumask_t *);

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v7 4/4] xen/x86: Allow stubdom access to irq created for msi.

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 626 bytes --]

On Wed, Sep 25, 2019 at 11:41:50AM +0200, Roger Pau Monné wrote:
> It would still be nice to get the missing bits (interrupt enabling),
> or else this patch is kind of pointless, since it still doesn't allow
> stubdomains to work correctly with passed through devices.

Well, that part, as discussed, doesn't need to be in Xen. For example
the solution deployed in current Qubes stable version is based on
pciback for this purpose.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [Xen-devel] [PATCH v7 4/4] xen/x86: Allow stubdom access to irq created for msi.

[Roger Pau Monné]

On Wed, Sep 25, 2019 at 02:29:41PM +0200, Marek Marczykowski-Górecki wrote:
> On Wed, Sep 25, 2019 at 11:41:50AM +0200, Roger Pau Monné wrote:
> > It would still be nice to get the missing bits (interrupt enabling),
> > or else this patch is kind of pointless, since it still doesn't allow
> > stubdomains to work correctly with passed through devices.
> 
> Well, that part, as discussed, doesn't need to be in Xen. For example
> the solution deployed in current Qubes stable version is based on
> pciback for this purpose.

Ack. Do you think it would make sense to submit that part to Linux
then?

It would be nice to have this feature working upstream IMO, and will
also allow Qubes to get rid of those patches.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v7 4/4] xen/x86: Allow stubdom access to irq created for msi.

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 1524 bytes --]

On Wed, Sep 25, 2019 at 03:26:17PM +0200, Roger Pau Monné wrote:
> On Wed, Sep 25, 2019 at 02:29:41PM +0200, Marek Marczykowski-Górecki wrote:
> > On Wed, Sep 25, 2019 at 11:41:50AM +0200, Roger Pau Monné wrote:
> > > It would still be nice to get the missing bits (interrupt enabling),
> > > or else this patch is kind of pointless, since it still doesn't allow
> > > stubdomains to work correctly with passed through devices.
> > 
> > Well, that part, as discussed, doesn't need to be in Xen. For example
> > the solution deployed in current Qubes stable version is based on
> > pciback for this purpose.
> 
> Ack. Do you think it would make sense to submit that part to Linux
> then?

How would an interface with toolstack (when to allow enabling MSI)
should look like? Right now I have it as extra attribute in sysfs of
pciback and libxl writes to it. Or rather should it be in xenstore?
Or maybe pciback should somehow detect itself if it's talking to
stubdomain while the device is assigned to a HVM domain, or to a target
PV domain itself?

The actual patch is here:
https://github.com/QubesOS/qubes-linux-kernel/blob/master/0014-xen-pciback-add-attribute-to-allow-MSI-enable-flag-w.patch
and the toolstack part:
https://github.com/QubesOS/qubes-vmm-xen/blob/xen-4.8/patch-stubdom-allow-msi-enable.patch

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [Xen-devel] [PATCH v7 4/4] xen/x86: Allow stubdom access to irq created for msi.

[Roger Pau Monné]

On Thu, Sep 26, 2019 at 06:16:06AM +0200, Marek Marczykowski-Górecki wrote:
> On Wed, Sep 25, 2019 at 03:26:17PM +0200, Roger Pau Monné wrote:
> > On Wed, Sep 25, 2019 at 02:29:41PM +0200, Marek Marczykowski-Górecki wrote:
> > > On Wed, Sep 25, 2019 at 11:41:50AM +0200, Roger Pau Monné wrote:
> > > > It would still be nice to get the missing bits (interrupt enabling),
> > > > or else this patch is kind of pointless, since it still doesn't allow
> > > > stubdomains to work correctly with passed through devices.
> > > 
> > > Well, that part, as discussed, doesn't need to be in Xen. For example
> > > the solution deployed in current Qubes stable version is based on
> > > pciback for this purpose.
> > 
> > Ack. Do you think it would make sense to submit that part to Linux
> > then?
> 
> How would an interface with toolstack (when to allow enabling MSI)
> should look like? Right now I have it as extra attribute in sysfs of
> pciback and libxl writes to it. Or rather should it be in xenstore?

I think xenstore would be more suitable for this. There are already
some flags passed to pciback there: msitranslate, power_mgmt and
permissive (see libxl_pci.c:63).

> Or maybe pciback should somehow detect itself if it's talking to
> stubdomain while the device is assigned to a HVM domain, or to a target
> PV domain itself?

I think doing it in the toolstack and just passing an option to
pciback is likely easier than adding more logic to pciback.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v7 4/4] xen/x86: Allow stubdom access to irq created for msi.

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 2146 bytes --]

On Thu, Sep 26, 2019 at 09:10:17AM +0200, Roger Pau Monné wrote:
> On Thu, Sep 26, 2019 at 06:16:06AM +0200, Marek Marczykowski-Górecki wrote:
> > On Wed, Sep 25, 2019 at 03:26:17PM +0200, Roger Pau Monné wrote:
> > > On Wed, Sep 25, 2019 at 02:29:41PM +0200, Marek Marczykowski-Górecki wrote:
> > > > On Wed, Sep 25, 2019 at 11:41:50AM +0200, Roger Pau Monné wrote:
> > > > > It would still be nice to get the missing bits (interrupt enabling),
> > > > > or else this patch is kind of pointless, since it still doesn't allow
> > > > > stubdomains to work correctly with passed through devices.
> > > > 
> > > > Well, that part, as discussed, doesn't need to be in Xen. For example
> > > > the solution deployed in current Qubes stable version is based on
> > > > pciback for this purpose.
> > > 
> > > Ack. Do you think it would make sense to submit that part to Linux
> > > then?
> > 
> > How would an interface with toolstack (when to allow enabling MSI)
> > should look like? Right now I have it as extra attribute in sysfs of
> > pciback and libxl writes to it. Or rather should it be in xenstore?
> 
> I think xenstore would be more suitable for this. There are already
> some flags passed to pciback there: msitranslate, power_mgmt and
> permissive (see libxl_pci.c:63).

Hmm, I see permissive is also set in sysfs
(libxl_pci.c:pci_add_dm_done). And I think that is used by pciback,
based on inspection of its source code.
In fact, I don't see anything in pciback parsing opts-%d xenstore entry
at all. It looks like it's only used by the toolstack to reconstruct
libxl_device_pci struct from xenstore.

> > Or maybe pciback should somehow detect itself if it's talking to
> > stubdomain while the device is assigned to a HVM domain, or to a target
> > PV domain itself?
> 
> I think doing it in the toolstack and just passing an option to
> pciback is likely easier than adding more logic to pciback.

Agree.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [Xen-devel] [PATCH v7 4/4] xen/x86: Allow stubdom access to irq created for msi.

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 938 bytes --]

On Sun, Sep 29, 2019 at 03:35:57AM +0200, Marek Marczykowski-Górecki wrote:
> On Thu, Sep 26, 2019 at 09:10:17AM +0200, Roger Pau Monné wrote:
> > On Thu, Sep 26, 2019 at 06:16:06AM +0200, Marek Marczykowski-Górecki wrote:
> > > On Wed, Sep 25, 2019 at 03:26:17PM +0200, Roger Pau Monné wrote:
> > > > On Wed, Sep 25, 2019 at 02:29:41PM +0200, Marek Marczykowski-Górecki wrote:
> > > > > On Wed, Sep 25, 2019 at 11:41:50AM +0200, Roger Pau Monné wrote:
> > > > > > It would still be nice to get the missing bits (interrupt enabling),
> > > > > > or else this patch is kind of pointless, since it still doesn't allow
> > > > > > stubdomains to work correctly with passed through devices.

BTW it is useful with permissive mode enabled. 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

Re: [Xen-devel] [PATCH v7 4/4] xen/x86: Allow stubdom access to irq created for msi.

[Roger Pau Monné]

On Sun, Sep 29, 2019 at 03:35:54AM +0200, Marek Marczykowski-Górecki wrote:
> On Thu, Sep 26, 2019 at 09:10:17AM +0200, Roger Pau Monné wrote:
> > On Thu, Sep 26, 2019 at 06:16:06AM +0200, Marek Marczykowski-Górecki wrote:
> > > On Wed, Sep 25, 2019 at 03:26:17PM +0200, Roger Pau Monné wrote:
> > > > On Wed, Sep 25, 2019 at 02:29:41PM +0200, Marek Marczykowski-Górecki wrote:
> > > > > On Wed, Sep 25, 2019 at 11:41:50AM +0200, Roger Pau Monné wrote:
> > > > > > It would still be nice to get the missing bits (interrupt enabling),
> > > > > > or else this patch is kind of pointless, since it still doesn't allow
> > > > > > stubdomains to work correctly with passed through devices.
> > > > > 
> > > > > Well, that part, as discussed, doesn't need to be in Xen. For example
> > > > > the solution deployed in current Qubes stable version is based on
> > > > > pciback for this purpose.
> > > > 
> > > > Ack. Do you think it would make sense to submit that part to Linux
> > > > then?
> > > 
> > > How would an interface with toolstack (when to allow enabling MSI)
> > > should look like? Right now I have it as extra attribute in sysfs of
> > > pciback and libxl writes to it. Or rather should it be in xenstore?
> > 
> > I think xenstore would be more suitable for this. There are already
> > some flags passed to pciback there: msitranslate, power_mgmt and
> > permissive (see libxl_pci.c:63).
> 
> Hmm, I see permissive is also set in sysfs
> (libxl_pci.c:pci_add_dm_done). And I think that is used by pciback,
> based on inspection of its source code.
> In fact, I don't see anything in pciback parsing opts-%d xenstore entry
> at all. It looks like it's only used by the toolstack to reconstruct
> libxl_device_pci struct from xenstore.

Then please use sysfs, using whatever mechanism is used by other
options seems fine to me.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v7 4/4] xen/x86: Allow stubdom access to irq created for msi.

[Jan Beulich]

On 25.09.2019 11:41, Roger Pau Monné  wrote:
> On Wed, Sep 25, 2019 at 04:41:26AM +0200, Marek Marczykowski-Górecki wrote:
>> --- a/xen/arch/x86/irq.c
>> +++ b/xen/arch/x86/irq.c
>> @@ -254,7 +254,8 @@ void __init clear_irq_vector(int irq)
>>  /*
>>   * Dynamic irq allocate and deallocation for MSI
>>   */
>> -int create_irq(nodeid_t node)
>> +
> 
> Extra newline.
> 
>> +int create_irq(nodeid_t node, bool grant_access)
>>  {
>>      int irq, ret;
>>      struct irq_desc *desc;

I did notice this too (on an earlier version), and it was my
understanding that the addition was deliberate - the comment
is for more than just this one function. I wouldn't insist
on either variant, i.e. I'm fine with the blank line added
and I'm also fine with the addition dropped again.

Jan

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

[Xen-devel] [PATCH v7.1 4/4] xen/x86: Allow stubdom access to irq created for msi.

[Marek Marczykowski-Górecki]

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. Give the stubdomain enough permissions
over the mapped interrupt in order to bind it successfully to it's
target domain.

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 lives there.
Modify create_irq() to take additional parameter, whether to grant
permissions to the domain creating the IRQ, which may be dom0 or a
stubdomain. Do this instead of granting access always to
hardware_domain. 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 checking if that domain does have permission
over the IRQ being destroyed.

Then, adjust all callers to provide the parameter. In case of Xen
internal allocations, set it to false, but for domain accessible
interrupt set it to true.

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

Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
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
Changes in v7:
 - Code style improvements (spaces, use %pd etc)
 - use bool parameter to create_irq, as it's only getting
 current->domain or NULL
 - remove redundant irq_access_permitted()
Changes in v7.1:
 - adjust comments, merge if
 - update commit message
---
 xen/arch/x86/hpet.c                      |  3 +-
 xen/arch/x86/irq.c                       | 42 +++++++++++++++++--------
 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                | 11 ++++++-
 6 files changed, 45 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 4b08488..57f68fa 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, false)) < 0 )
         return irq;
 
     ch->msi.irq = irq;
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 0ee3346..4304896 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -254,7 +254,8 @@ void __init clear_irq_vector(int irq)
 /*
  * Dynamic irq allocate and deallocation for MSI
  */
-int create_irq(nodeid_t node)
+
+int create_irq(nodeid_t node, bool grant_access)
 {
     int irq, ret;
     struct irq_desc *desc;
@@ -282,18 +283,23 @@ 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 ( grant_access )
     {
-        ret = irq_permit_access(hardware_domain, irq);
+        ret = irq_permit_access(current->domain, irq);
         if ( ret )
             printk(XENLOG_G_ERR
-                   "Could not grant Dom0 access to IRQ%d (error %d)\n",
-                   irq, ret);
+                   "Could not grant %pd access to IRQ%d (error %d)\n",
+                   current->domain, irq, ret);
+        else
+            desc->arch.creator_domid = current->domain->domain_id;
     }
 
     return irq;
@@ -307,14 +313,23 @@ 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 )
+        {
+            int err = irq_deny_access(d, irq);
+            if ( err )
+                printk(XENLOG_G_ERR
+                       "Could not revoke %pd access to IRQ%u (error %d)\n",
+                       d, irq, err);
+
+            put_domain(d);
+
+        }
+
+        desc->arch.creator_domid = DOMID_INVALID;
     }
 
     spin_lock_irqsave(&desc->lock, flags);
@@ -381,6 +396,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 +2149,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, true);
             ret = irq >= 0 ? prepare_domain_irq_pirq(d, irq, pirq + nr, &info)
                            : irq;
             if ( ret < 0 )
@@ -2818,7 +2834,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, true);
         }
 
         if ( irq < nr_irqs_gsi || irq >= nr_irqs )
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 8667de6..fcd3979 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, false);
 #endif
 }
 
diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c
index bb9f33e..233a8ae 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, false);
     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..24a1e92 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,
+                     false);
     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..a75c054 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 having the IRQ assigned)
+         */
+        domid_t creator_domid;
 };
 
 /* For use with irq_desc.arch.used */
@@ -161,7 +166,11 @@ int  init_irq_data(void);
 void clear_irq_vector(int irq);
 
 int irq_to_vector(int irq);
-int create_irq(nodeid_t node);
+/*
+ * If grant_access is set the current domain is given permissions over
+ * the created IRQ.
+ */
+int create_irq(nodeid_t node, bool grant_access);
 void destroy_irq(unsigned int irq);
 int assign_irq_vector(int irq, const cpumask_t *);
 
-- 
git-series 0.9.1

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

Re: [Xen-devel] [PATCH v7.1 4/4] xen/x86: Allow stubdom access to irq created for msi.

[Jan Beulich]

On 26.09.2019 06:05, Marek Marczykowski-Górecki  wrote:
> 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. Give the stubdomain enough permissions
> over the mapped interrupt in order to bind it successfully to it's
> target domain.
> 
> 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 lives there.
> Modify create_irq() to take additional parameter, whether to grant
> permissions to the domain creating the IRQ, which may be dom0 or a
> stubdomain. Do this instead of granting access always to
> hardware_domain. 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 checking if that domain does have permission
> over the IRQ being destroyed.
> 
> Then, adjust all callers to provide the parameter. In case of Xen
> internal allocations, set it to false, but for domain accessible
> interrupt set it to true.
> 
> Inspired by https://github.com/OpenXT/xenclient-oe/blob/5e0e7304a5a3c75ef01240a1e3673665b2aaf05e/recipes-extended/xen/files/stubdomain-msi-irq-access.patch by Eric Chanudet <chanudete@ainfosec.com>.
> 
> Signed-off-by: Simon Gaiser <simon@invisiblethingslab.com>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
with a couple of cosmetic things addressed, which I'll do while
committing.

Jan

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

Re: [Xen-devel] [PATCH v7 0/4] Fix PCI passthrough for HVM with stubdomain

[Jan Beulich]

Ian, Wei,

On 25.09.2019 04:41, Marek Marczykowski-Górecki  wrote:
> In this version, I drop PHYSDEVOP_interrupt_control patch, since Jan prefer not
> to mix pciif and hypercalls for serving device model. Enabling MSI by the
> stubdomain remains unsolved here, but other patches are improvement anyway.
> 
> Changes in v2:
>  - new "xen/x86: Allow stubdom access to irq created for msi" patch
>  - applied review comments from v1
> Changes is v3:
>  - apply suggestions by Roger
>  - add PHYSDEVOP_msi_msix_set_enable
> Changes in v4:
>  - implement suggestions by Wei, Roger, Jan
>  - plug new physdevop into XSM
> Changes in v5:
>  - rebase on master
>  - rename to PHYSDEVOP_msi_control
>  - move granting access to IRQ into create_irq
> Changes in v6:
>  - simplify granting IRQ access, record dm domid for cleanup
>  - rename to PHYSDEVOP_interrupt_control
>  - include INTx control in the hypercall
> Changes in v7:
>  - update "xen/x86: Allow stubdom access to irq created for msi"
>  - drop "xen/x86: add PHYSDEVOP_interrupt_control"
>  - drop "tools/libxc: add wrapper for PHYSDEVOP_interrupt_control"
> 
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wl@xen.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Brian Woods <brian.woods@amd.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> 
> Marek Marczykowski-Górecki (4):
>   libxl: do not attach xen-pciback to HVM domain, if stubdomain is in use
>   libxl: attach PCI device to qemu only after setting pciback/pcifront
>   libxl: don't try to manipulate json config for stubdomain
>   xen/x86: Allow stubdom access to irq created for msi.

I did commit the last one, but I'd prefer if one of you could take
care of the three libxl ones.

Thanks, Jan

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

Re: [Xen-devel] [PATCH v7 0/4] Fix PCI passthrough for HVM with stubdomain

[Wei Liu]

On Fri, Sep 27, 2019 at 04:21:55PM +0200, Jan Beulich wrote:
> > 
> > Marek Marczykowski-Górecki (4):
> >   libxl: do not attach xen-pciback to HVM domain, if stubdomain is in use
> >   libxl: attach PCI device to qemu only after setting pciback/pcifront
> >   libxl: don't try to manipulate json config for stubdomain
> >   xen/x86: Allow stubdom access to irq created for msi.
> 
> I did commit the last one, but I'd prefer if one of you could take
> care of the three libxl ones.
> 

I tried to apply the first three. They don't apply cleanly. That's
perhaps due to Anthony's series which got committed recently.

Marek, do you have a branch?

Wei.

> Thanks, Jan

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

Re: [Xen-devel] [PATCH v7 0/4] Fix PCI passthrough for HVM with stubdomain

[Marek Marczykowski-Górecki]

[-- Attachment #1.1: Type: text/plain, Size: 1250 bytes --]

On Fri, Sep 27, 2019 at 03:36:08PM +0100, Wei Liu wrote:
> On Fri, Sep 27, 2019 at 04:21:55PM +0200, Jan Beulich wrote:
> > > 
> > > Marek Marczykowski-Górecki (4):
> > >   libxl: do not attach xen-pciback to HVM domain, if stubdomain is in use
> > >   libxl: attach PCI device to qemu only after setting pciback/pcifront
> > >   libxl: don't try to manipulate json config for stubdomain
> > >   xen/x86: Allow stubdom access to irq created for msi.
> > 
> > I did commit the last one, but I'd prefer if one of you could take
> > care of the three libxl ones.
> > 
> 
> I tried to apply the first three. They don't apply cleanly. That's
> perhaps due to Anthony's series which got committed recently.
> 
> Marek, do you have a branch?

This rebase turned out to be fairly complex, because of the whole pci
attach got reworked for async api. So, I've done it, but dropped your
ack on the patch needing rework for this reason. And also found one
regression introduced by Anthony series.

All in all - v8 on its way, still 4 patches (+1, -1).

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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