* [Xen-devel] [PATCH v5 0/6] Fix PCI passthrough for HVM with stubdomain
@ 2019-07-17 1:00 Marek Marczykowski-Górecki
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 1/6] libxl: do not attach xen-pciback to HVM domain, if stubdomain is in use Marek Marczykowski-Górecki
` (5 more replies)
0 siblings, 6 replies; 39+ messages in thread
From: Marek Marczykowski-Górecki @ 2019-07-17 1:00 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Stefano Stabellini, Suravee Suthikulpanit, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Marek Marczykowski-Górecki, Tim Deegan, Julien Grall,
Jan Beulich, Anthony PERARD, Daniel De Graaf, Brian Woods,
Roger Pau Monné
In this version, I add PHYSDEVOP_msi_control to allow stubdomain
enabling MSI after mapping it.
Related article:
https://www.qubes-os.org/news/2017/10/18/msi-support/
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
---
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 (6):
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.
xen/x86: add PHYSDEVOP_msi_control
tools/libxc: add wrapper for PHYSDEVOP_msi_control
tools/libxc/include/xenctrl.h | 6 ++-
tools/libxc/xc_physdev.c | 19 ++++++++-
tools/libxl/libxl_pci.c | 63 +++++++++++++++++--------
xen/arch/x86/hpet.c | 3 +-
xen/arch/x86/irq.c | 51 ++++++++++++++------
xen/arch/x86/msi.c | 42 +++++++++++++++++-
xen/arch/x86/physdev.c | 25 ++++++++++-
xen/arch/x86/x86_64/physdev.c | 4 ++-
xen/common/irq.c | 1 +-
xen/drivers/char/ns16550.c | 2 +-
xen/drivers/passthrough/amd/iommu_init.c | 2 +-
xen/drivers/passthrough/pci.c | 3 +-
xen/drivers/passthrough/vtd/iommu.c | 3 +-
xen/include/asm-x86/irq.h | 2 +-
xen/include/asm-x86/msi.h | 1 +-
xen/include/public/physdev.h | 16 ++++++-
xen/include/xen/irq.h | 1 +-
xen/include/xlat.lst | 1 +-
xen/include/xsm/dummy.h | 7 +++-
xen/include/xsm/xsm.h | 6 ++-
xen/xsm/dummy.c | 1 +-
xen/xsm/flask/hooks.c | 24 ++++++++++-
xen/xsm/flask/policy/access_vectors | 1 +-
23 files changed, 246 insertions(+), 38 deletions(-)
base-commit: b541287c3600713feaaaf7608cd405e7b2e4efd0
--
git-series 0.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Xen-devel] [PATCH v5 1/6] libxl: do not attach xen-pciback to HVM domain, if stubdomain is in use
2019-07-17 1:00 [Xen-devel] [PATCH v5 0/6] Fix PCI passthrough for HVM with stubdomain Marek Marczykowski-Górecki
@ 2019-07-17 1:00 ` Marek Marczykowski-Górecki
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 2/6] libxl: attach PCI device to qemu only after setting pciback/pcifront Marek Marczykowski-Górecki
` (4 subsequent siblings)
5 siblings, 0 replies; 39+ messages in thread
From: Marek Marczykowski-Górecki @ 2019-07-17 1:00 UTC (permalink / raw)
To: xen-devel
Cc: Anthony PERARD, Ian Jackson, Wei Liu,
Marek Marczykowski-Górecki, Wei Liu
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 4ec6872..7ffab89 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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Xen-devel] [PATCH v5 2/6] libxl: attach PCI device to qemu only after setting pciback/pcifront
2019-07-17 1:00 [Xen-devel] [PATCH v5 0/6] Fix PCI passthrough for HVM with stubdomain Marek Marczykowski-Górecki
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 1/6] libxl: do not attach xen-pciback to HVM domain, if stubdomain is in use Marek Marczykowski-Górecki
@ 2019-07-17 1:00 ` Marek Marczykowski-Górecki
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 3/6] libxl: don't try to manipulate json config for stubdomain Marek Marczykowski-Górecki
` (3 subsequent siblings)
5 siblings, 0 replies; 39+ messages in thread
From: Marek Marczykowski-Górecki @ 2019-07-17 1:00 UTC (permalink / raw)
To: xen-devel
Cc: Anthony PERARD, Ian Jackson, Wei Liu,
Marek Marczykowski-Górecki, Wei Liu
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 7ffab89..18089ea 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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Xen-devel] [PATCH v5 3/6] libxl: don't try to manipulate json config for stubdomain
2019-07-17 1:00 [Xen-devel] [PATCH v5 0/6] Fix PCI passthrough for HVM with stubdomain Marek Marczykowski-Górecki
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 1/6] libxl: do not attach xen-pciback to HVM domain, if stubdomain is in use Marek Marczykowski-Górecki
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 2/6] libxl: attach PCI device to qemu only after setting pciback/pcifront Marek Marczykowski-Górecki
@ 2019-07-17 1:00 ` Marek Marczykowski-Górecki
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 4/6] xen/x86: Allow stubdom access to irq created for msi Marek Marczykowski-Górecki
` (2 subsequent siblings)
5 siblings, 0 replies; 39+ messages in thread
From: Marek Marczykowski-Górecki @ 2019-07-17 1:00 UTC (permalink / raw)
To: xen-devel
Cc: Anthony PERARD, Ian Jackson, Wei Liu,
Marek Marczykowski-Górecki, Wei Liu
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 18089ea..9144815 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
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Xen-devel] [PATCH v5 4/6] xen/x86: Allow stubdom access to irq created for msi.
2019-07-17 1:00 [Xen-devel] [PATCH v5 0/6] Fix PCI passthrough for HVM with stubdomain Marek Marczykowski-Górecki
` (2 preceding siblings ...)
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 3/6] libxl: don't try to manipulate json config for stubdomain Marek Marczykowski-Górecki
@ 2019-07-17 1:00 ` Marek Marczykowski-Górecki
2019-07-17 9:54 ` Roger Pau Monné
2019-07-20 16:48 ` Julien Grall
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control Marek Marczykowski-Górecki
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 6/6] tools/libxc: add wrapper for PHYSDEVOP_msi_control Marek Marczykowski-Górecki
5 siblings, 2 replies; 39+ messages in thread
From: Marek Marczykowski-Górecki @ 2019-07-17 1:00 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Stefano Stabellini, Suravee Suthikulpanit, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Marek Marczykowski-Górecki, Tim Deegan, Simon Gaiser,
Julien Grall, Jan Beulich, Brian Woods, Roger Pau Monné
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()
---
xen/arch/x86/hpet.c | 3 +-
xen/arch/x86/irq.c | 51 ++++++++++++++++++-------
xen/common/irq.c | 1 +-
xen/drivers/char/ns16550.c | 2 +-
xen/drivers/passthrough/amd/iommu_init.c | 2 +-
xen/drivers/passthrough/pci.c | 3 +-
xen/drivers/passthrough/vtd/iommu.c | 3 +-
xen/include/asm-x86/irq.h | 2 +-
xen/include/xen/irq.h | 1 +-
9 files changed, 50 insertions(+), 18 deletions(-)
diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 4b08488..b4854ff 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, hardware_domain)) < 0 )
return irq;
ch->msi.irq = irq;
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 2cac28a..dc5d302 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -164,10 +164,21 @@ int __init bind_irq_vector(int irq, int vector, const cpumask_t *cpu_mask)
return ret;
}
+static struct domain *get_dm_domain(struct domain *d)
+{
+ return current->domain->target == d ? current->domain : hardware_domain;
+}
+
/*
* 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;
@@ -200,18 +211,24 @@ int create_irq(nodeid_t node)
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->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);
@@ -220,14 +237,22 @@ void destroy_irq(unsigned int irq)
BUG_ON(!MSI_IRQ(irq));
- if ( hardware_domain )
+ if ( desc->creator_domid != DOMID_INVALID )
{
- int err = irq_deny_access(hardware_domain, irq);
+ struct domain *d = get_domain_by_id(desc->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);
}
spin_lock_irqsave(&desc->lock, flags);
@@ -2058,7 +2083,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, get_dm_domain(d));
ret = irq >= 0 ? prepare_domain_irq_pirq(d, irq, pirq + nr, &info)
: irq;
if ( ret < 0 )
@@ -2691,7 +2716,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, get_dm_domain(d));
}
if ( irq < nr_irqs_gsi || irq >= nr_irqs )
diff --git a/xen/common/irq.c b/xen/common/irq.c
index f42512d..42b27a9 100644
--- a/xen/common/irq.c
+++ b/xen/common/irq.c
@@ -16,6 +16,7 @@ int init_one_irq_desc(struct irq_desc *desc)
spin_lock_init(&desc->lock);
cpumask_setall(desc->affinity);
INIT_LIST_HEAD(&desc->rl_link);
+ desc->creator_domid = DOMID_INVALID;
err = arch_init_one_irq_desc(desc);
if ( err )
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 189e121..ccc8b04 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -719,7 +719,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 4e76b26..50785e0 100644
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -781,7 +781,7 @@ static bool_t __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
hw_irq_controller *handler;
u16 control;
- 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/pci.c b/xen/drivers/passthrough/pci.c
index e886894..507b3d1 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -845,6 +845,9 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
if ( pdev->bus == bus && pdev->devfn == devfn )
{
+ ret = -EBUSY;
+ if ( pdev->domain && pdev->domain != hardware_domain )
+ break;
ret = iommu_remove_device(pdev);
if ( pdev->domain )
list_del(&pdev->domain_list);
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 8b27d7e..79f9682 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 c0c6e7c..5b24428 100644
--- a/xen/include/asm-x86/irq.h
+++ b/xen/include/asm-x86/irq.h
@@ -155,7 +155,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 *);
diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
index 586b783..c7a6a83 100644
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -91,6 +91,7 @@ typedef struct irq_desc {
spinlock_t lock;
struct arch_irq_desc arch;
cpumask_var_t affinity;
+ domid_t creator_domid; /* weak reference to domain handling this IRQ */
/* irq ratelimit */
s_time_t rl_quantum_start;
--
git-series 0.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
2019-07-17 1:00 [Xen-devel] [PATCH v5 0/6] Fix PCI passthrough for HVM with stubdomain Marek Marczykowski-Górecki
` (3 preceding siblings ...)
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 4/6] xen/x86: Allow stubdom access to irq created for msi Marek Marczykowski-Górecki
@ 2019-07-17 1:00 ` Marek Marczykowski-Górecki
2019-07-17 10:18 ` Roger Pau Monné
2019-07-19 10:40 ` Jan Beulich
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 6/6] tools/libxc: add wrapper for PHYSDEVOP_msi_control Marek Marczykowski-Górecki
5 siblings, 2 replies; 39+ messages in thread
From: Marek Marczykowski-Górecki @ 2019-07-17 1:00 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson,
Marek Marczykowski-Górecki, Tim Deegan, Julien Grall,
Jan Beulich, Daniel De Graaf, Roger Pau Monné
Allow device model running in stubdomain to enable/disable MSI(-X),
bypassing pciback. While pciback is still used to access config space
from within stubdomain, it refuse to write to
PCI_MSI_FLAGS_ENABLE/PCI_MSIX_FLAGS_ENABLE in non-permissive mode. Which
is the right thing to do for PV domain (the main use case for pciback),
as PV domain should use XEN_PCI_OP_* commands for that. Unfortunately
those commands are not good for stubdomain use, as they configure MSI in
dom0's kernel too, which should not happen for HVM domain.
This new physdevop is allowed only for stubdomain controlling the domain
which own the device.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v3:
- new patch
Changes in v4:
- adjust code style
- s/msi_msix/msi/
- add msi_set_enable XSM hook
- flatten struct physdev_msi_set_enable
- add to include/xlat.lst
Changes in v5:
- rename to PHYSDEVOP_msi_control
- combine "mode" and "enable" into "flags"
- refuse to enable both MSI and MSI-X, and also to enable MSI(-X) on
incapable device
- disable/enable INTx when enabling/disabling MSI (?)
- refuse if !use_msi
- adjust flask hook to make more sense (require "setup" access on
device, not on domain)
- rebase on master
I'm not sure if XSM part is correct, compile-tested only, as I'm not
sure how to set the policy.
---
xen/arch/x86/msi.c | 42 ++++++++++++++++++++++++++++++-
xen/arch/x86/physdev.c | 25 ++++++++++++++++++-
xen/arch/x86/x86_64/physdev.c | 4 +++-
xen/include/asm-x86/msi.h | 1 +-
xen/include/public/physdev.h | 16 +++++++++++-
xen/include/xlat.lst | 1 +-
xen/include/xsm/dummy.h | 7 +++++-
xen/include/xsm/xsm.h | 6 ++++-
xen/xsm/dummy.c | 1 +-
xen/xsm/flask/hooks.c | 24 +++++++++++++++++-
xen/xsm/flask/policy/access_vectors | 1 +-
11 files changed, 128 insertions(+)
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
index 89e6116..fca1d04 100644
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -1475,6 +1475,48 @@ int pci_restore_msi_state(struct pci_dev *pdev)
return 0;
}
+int msi_control(struct pci_dev *pdev, bool msix, bool enable)
+{
+ int ret;
+ struct msi_desc *old_desc;
+
+ if ( !use_msi )
+ return -EOPNOTSUPP;
+
+ ret = xsm_msi_control(XSM_DM_PRIV, pdev->domain, pdev->sbdf.sbdf, msix, enable);
+ if ( ret )
+ return ret;
+
+ if ( msix )
+ {
+ if ( !pdev->msix )
+ return -ENODEV;
+ old_desc = find_msi_entry(pdev, -1, PCI_CAP_ID_MSI);
+ if ( old_desc )
+ return -EBUSY;
+ if ( enable )
+ pci_intx(pdev, false);
+ msix_set_enable(pdev, enable);
+ }
+ else
+ {
+ if ( !pci_find_cap_offset(pdev->seg,
+ pdev->bus,
+ PCI_SLOT(pdev->devfn),
+ PCI_FUNC(pdev->devfn),
+ PCI_CAP_ID_MSI) )
+ return -ENODEV;
+ old_desc = find_msi_entry(pdev, -1, PCI_CAP_ID_MSIX);
+ if ( old_desc )
+ return -EBUSY;
+ if ( enable )
+ pci_intx(pdev, false);
+ msi_set_enable(pdev, enable);
+ }
+
+ return 0;
+}
+
void __init early_msi_init(void)
{
if ( use_msi < 0 )
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 3a3c158..5000998 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -662,6 +662,31 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
break;
}
+ case PHYSDEVOP_msi_control: {
+ struct physdev_msi_control op;
+ struct pci_dev *pdev;
+
+ ret = -EFAULT;
+ if ( copy_from_guest(&op, arg, 1) )
+ break;
+
+ ret = -EINVAL;
+ if ( op.flags & ~(PHYSDEVOP_MSI_CONTROL_MSIX | PHYSDEVOP_MSI_CONTROL_ENABLE) )
+ break;
+
+ pcidevs_lock();
+ pdev = pci_get_pdev(op.seg, op.bus, op.devfn);
+ if ( pdev )
+ ret = msi_control(pdev,
+ op.flags & PHYSDEVOP_MSI_CONTROL_MSIX,
+ op.flags & PHYSDEVOP_MSI_CONTROL_ENABLE);
+ else
+ ret = -ENODEV;
+ pcidevs_unlock();
+ break;
+
+ }
+
default:
ret = -ENOSYS;
break;
diff --git a/xen/arch/x86/x86_64/physdev.c b/xen/arch/x86/x86_64/physdev.c
index c5a00ea..69b4ce3 100644
--- a/xen/arch/x86/x86_64/physdev.c
+++ b/xen/arch/x86/x86_64/physdev.c
@@ -76,6 +76,10 @@ CHECK_physdev_pci_device_add
CHECK_physdev_pci_device
#undef xen_physdev_pci_device
+#define xen_physdev_msi_control physdev_msi_control
+CHECK_physdev_msi_control
+#undef xen_physdev_msi_control
+
#define COMPAT
#undef guest_handle_okay
#define guest_handle_okay compat_handle_okay
diff --git a/xen/include/asm-x86/msi.h b/xen/include/asm-x86/msi.h
index 10387dc..05296de 100644
--- a/xen/include/asm-x86/msi.h
+++ b/xen/include/asm-x86/msi.h
@@ -252,5 +252,6 @@ void guest_mask_msi_irq(struct irq_desc *, bool mask);
void ack_nonmaskable_msi_irq(struct irq_desc *);
void end_nonmaskable_msi_irq(struct irq_desc *, u8 vector);
void set_msi_affinity(struct irq_desc *, const cpumask_t *);
+int msi_control(struct pci_dev *pdev, bool msix, bool enable);
#endif /* __ASM_MSI_H */
diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
index b6faf83..f9b728f 100644
--- a/xen/include/public/physdev.h
+++ b/xen/include/public/physdev.h
@@ -344,6 +344,22 @@ struct physdev_dbgp_op {
typedef struct physdev_dbgp_op physdev_dbgp_op_t;
DEFINE_XEN_GUEST_HANDLE(physdev_dbgp_op_t);
+/* when PHYSDEVOP_MSI_CONTROL_MSIX not set, control MSI */
+#define PHYSDEVOP_MSI_CONTROL_MSIX 1
+/* when PHYSDEVOP_MSI_CONTROL_ENABLE not set, disable */
+#define PHYSDEVOP_MSI_CONTROL_ENABLE 2
+
+#define PHYSDEVOP_msi_control 32
+struct physdev_msi_control {
+ /* IN */
+ uint16_t seg;
+ uint8_t bus;
+ uint8_t devfn;
+ uint8_t flags;
+};
+typedef struct physdev_msi_control physdev_msi_control_t;
+DEFINE_XEN_GUEST_HANDLE(physdev_msi_control_t);
+
/*
* Notify that some PIRQ-bound event channels have been unmasked.
* ** This command is obsolete since interface version 0x00030202 and is **
diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
index 95f5e55..3082761 100644
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -108,6 +108,7 @@
? physdev_irq_status_query physdev.h
? physdev_manage_pci physdev.h
? physdev_manage_pci_ext physdev.h
+? physdev_msi_control physdev.h
? physdev_pci_device physdev.h
? physdev_pci_device_add physdev.h
? physdev_pci_mmcfg_reserved physdev.h
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 01d2814..4801838 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -514,6 +514,13 @@ static XSM_INLINE int xsm_pci_config_permission(XSM_DEFAULT_ARG struct domain *d
return xsm_default_action(action, current->domain, d);
}
+static XSM_INLINE int xsm_msi_control(XSM_DEFAULT_ARG struct domain *d, uint32_t machine_bdf,
+ uint8_t msix, uint8_t enable)
+{
+ XSM_ASSERT_ACTION(XSM_DM_PRIV);
+ return xsm_default_action(action, current->domain, d);
+}
+
static XSM_INLINE int xsm_add_to_physmap(XSM_DEFAULT_ARG struct domain *d1, struct domain *d2)
{
XSM_ASSERT_ACTION(XSM_TARGET);
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index b6141f6..bf39dbd 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -106,6 +106,7 @@ struct xsm_operations {
int (*iomem_permission) (struct domain *d, uint64_t s, uint64_t e, uint8_t allow);
int (*iomem_mapping) (struct domain *d, uint64_t s, uint64_t e, uint8_t allow);
int (*pci_config_permission) (struct domain *d, uint32_t machine_bdf, uint16_t start, uint16_t end, uint8_t access);
+ int (*msi_control) (struct domain *d, uint32_t machine_bdf, uint8_t msix, uint8_t enable);
#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI)
int (*get_device_group) (uint32_t machine_bdf);
@@ -464,6 +465,11 @@ static inline int xsm_pci_config_permission (xsm_default_t def, struct domain *d
return xsm_ops->pci_config_permission(d, machine_bdf, start, end, access);
}
+static inline int xsm_msi_control (xsm_default_t def, struct domain *d, uint32_t machine_bdf, uint8_t msix, uint8_t enable)
+{
+ return xsm_ops->msi_control(d, machine_bdf, msix, enable);
+}
+
#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI)
static inline int xsm_get_device_group(xsm_default_t def, uint32_t machine_bdf)
{
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index c9a566f..878eefe 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -81,6 +81,7 @@ void __init xsm_fixup_ops (struct xsm_operations *ops)
set_to_dummy_if_null(ops, iomem_permission);
set_to_dummy_if_null(ops, iomem_mapping);
set_to_dummy_if_null(ops, pci_config_permission);
+ set_to_dummy_if_null(ops, msi_control);
set_to_dummy_if_null(ops, get_vnumainfo);
#if defined(CONFIG_HAS_PASSTHROUGH) && defined(CONFIG_HAS_PCI)
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index a7d690a..5fb755e 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1083,6 +1083,29 @@ static int flask_pci_config_permission(struct domain *d, uint32_t machine_bdf, u
}
+static int flask_msi_control(struct domain *d, uint32_t machine_bdf, uint8_t msix, uint8_t enable)
+{
+ uint32_t dsid, rsid;
+ int rc = -EPERM;
+ struct avc_audit_data ad;
+ uint32_t perm;
+
+ AVC_AUDIT_DATA_INIT(&ad, DEV);
+ ad.device = machine_bdf;
+
+ rc = security_device_sid(machine_bdf, &rsid);
+ if ( rc )
+ return rc;
+
+ rc = avc_current_has_perm(rsid, SECCLASS_RESOURCE, RESOURCE__SETUP, &ad);
+ if ( rc )
+ return rc;
+
+ perm = flask_iommu_resource_use_perm();
+ dsid = domain_sid(d);
+ return avc_has_perm(dsid, rsid, SECCLASS_RESOURCE, perm, &ad);
+}
+
static int flask_resource_plug_core(void)
{
return avc_current_has_perm(SECINITSID_DOMXEN, SECCLASS_RESOURCE, RESOURCE__PLUG, NULL);
@@ -1800,6 +1823,7 @@ static struct xsm_operations flask_ops = {
.iomem_permission = flask_iomem_permission,
.iomem_mapping = flask_iomem_mapping,
.pci_config_permission = flask_pci_config_permission,
+ .msi_control = flask_msi_control,
.resource_plug_core = flask_resource_plug_core,
.resource_unplug_core = flask_resource_unplug_core,
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index 194d743..0ddfc91 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -466,6 +466,7 @@ class resource
# checked for PHYSDEVOP_restore_msi* (target PCI device)
# checked for PHYSDEVOP_setup_gsi (target IRQ)
# checked for PHYSDEVOP_pci_mmcfg_reserved (target xen_t)
+# checked for PHYSDEVOP_msi_control (target PCI device)
setup
}
--
git-series 0.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [Xen-devel] [PATCH v5 6/6] tools/libxc: add wrapper for PHYSDEVOP_msi_control
2019-07-17 1:00 [Xen-devel] [PATCH v5 0/6] Fix PCI passthrough for HVM with stubdomain Marek Marczykowski-Górecki
` (4 preceding siblings ...)
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control Marek Marczykowski-Górecki
@ 2019-07-17 1:00 ` Marek Marczykowski-Górecki
2019-07-17 10:21 ` Roger Pau Monné
5 siblings, 1 reply; 39+ messages in thread
From: Marek Marczykowski-Górecki @ 2019-07-17 1:00 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Marek Marczykowski-Górecki, Wei Liu
Add libxc wrapper for PHYSDEVOP_msi_control introduced in previous
commit.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v3:
- new patch
Changes in v4:
- adjust for updated previous patch
Changes in v5:
- rename to PHYSDEVOP_msi_control, adjust arguments
---
tools/libxc/include/xenctrl.h | 6 ++++++
tools/libxc/xc_physdev.c | 19 +++++++++++++++++++
2 files changed, 25 insertions(+)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 538007a..826d10d 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1638,6 +1638,12 @@ int xc_physdev_unmap_pirq(xc_interface *xch,
uint32_t domid,
int pirq);
+int xc_physdev_msi_control(xc_interface *xch,
+ int seg,
+ int bus,
+ int devfn,
+ int flags);
+
/*
* LOGGING AND ERROR REPORTING
*/
diff --git a/tools/libxc/xc_physdev.c b/tools/libxc/xc_physdev.c
index 460a8e7..a25a117 100644
--- a/tools/libxc/xc_physdev.c
+++ b/tools/libxc/xc_physdev.c
@@ -111,3 +111,22 @@ int xc_physdev_unmap_pirq(xc_interface *xch,
return rc;
}
+int xc_physdev_msi_control(xc_interface *xch,
+ int seg,
+ int bus,
+ int devfn,
+ int flags)
+{
+ int rc;
+ struct physdev_msi_control op;
+
+ memset(&op, 0, sizeof(struct physdev_msi_control));
+ op.seg = seg;
+ op.bus = bus;
+ op.devfn = devfn;
+ op.flags = flags;
+
+ rc = do_physdev_op(xch, PHYSDEVOP_msi_control, &op, sizeof(op));
+
+ return rc;
+}
--
git-series 0.9.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 4/6] xen/x86: Allow stubdom access to irq created for msi.
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 4/6] xen/x86: Allow stubdom access to irq created for msi Marek Marczykowski-Górecki
@ 2019-07-17 9:54 ` Roger Pau Monné
2019-07-17 15:09 ` Marek Marczykowski-Górecki
2019-07-20 16:48 ` Julien Grall
1 sibling, 1 reply; 39+ messages in thread
From: Roger Pau Monné @ 2019-07-17 9:54 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Kevin Tian, Stefano Stabellini, Suravee Suthikulpanit, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Simon Gaiser, Julien Grall, Jan Beulich, xen-devel,
Brian Woods
On Wed, Jul 17, 2019 at 03:00:42AM +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.
>
> 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()
> ---
> xen/arch/x86/hpet.c | 3 +-
> xen/arch/x86/irq.c | 51 ++++++++++++++++++-------
> xen/common/irq.c | 1 +-
> xen/drivers/char/ns16550.c | 2 +-
> xen/drivers/passthrough/amd/iommu_init.c | 2 +-
> xen/drivers/passthrough/pci.c | 3 +-
> xen/drivers/passthrough/vtd/iommu.c | 3 +-
> xen/include/asm-x86/irq.h | 2 +-
> xen/include/xen/irq.h | 1 +-
> 9 files changed, 50 insertions(+), 18 deletions(-)
>
> diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
> index 4b08488..b4854ff 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, hardware_domain)) < 0 )
Shouldn't this be NULL? I don't think the hardware domain should be
allowed to play with the HPET IRQs?
> return irq;
>
> ch->msi.irq = irq;
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index 2cac28a..dc5d302 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -164,10 +164,21 @@ int __init bind_irq_vector(int irq, int vector, const cpumask_t *cpu_mask)
> return ret;
> }
>
> +static struct domain *get_dm_domain(struct domain *d)
^ const
> +{
> + return current->domain->target == d ? current->domain : hardware_domain;
> +}
> +
> /*
> * 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;
> @@ -200,18 +211,24 @@ int create_irq(nodeid_t node)
> desc->arch.used = IRQ_UNUSED;
> irq = ret;
> }
I would assert that desc->creator_domid == DOMID_INVALID here, since
in the failure case creator_domid is not overwritten.
> - 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->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);
> @@ -220,14 +237,22 @@ void destroy_irq(unsigned int irq)
>
> BUG_ON(!MSI_IRQ(irq));
>
> - if ( hardware_domain )
> + if ( desc->creator_domid != DOMID_INVALID )
> {
> - int err = irq_deny_access(hardware_domain, irq);
> + struct domain *d = get_domain_by_id(desc->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);
Don't you need to set creator_domid = DOMID_INVALID in destroy_irq at
some point?
Or else a failure in create_irq could leak the irq to it's previous
owner. Note that init_one_irq_desc would only init the fields the
first time the IRQ is used, but not for subsequent usages AFAICT.
> }
>
> spin_lock_irqsave(&desc->lock, flags);
> @@ -2058,7 +2083,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, get_dm_domain(d));
Isn't it fine to just use current->domain here directly?
It's always going to be the current domain the one that calls
map_domain_pirq in order to get a PIRQ mapped for it's target
domain I think.
> ret = irq >= 0 ? prepare_domain_irq_pirq(d, irq, pirq + nr, &info)
> : irq;
> if ( ret < 0 )
> @@ -2691,7 +2716,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, get_dm_domain(d));
> }
>
> if ( irq < nr_irqs_gsi || irq >= nr_irqs )
> diff --git a/xen/common/irq.c b/xen/common/irq.c
> index f42512d..42b27a9 100644
> --- a/xen/common/irq.c
> +++ b/xen/common/irq.c
> @@ -16,6 +16,7 @@ int init_one_irq_desc(struct irq_desc *desc)
> spin_lock_init(&desc->lock);
> cpumask_setall(desc->affinity);
> INIT_LIST_HEAD(&desc->rl_link);
> + desc->creator_domid = DOMID_INVALID;
>
> err = arch_init_one_irq_desc(desc);
> if ( err )
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 189e121..ccc8b04 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -719,7 +719,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 4e76b26..50785e0 100644
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -781,7 +781,7 @@ static bool_t __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
> hw_irq_controller *handler;
> u16 control;
>
> - 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/pci.c b/xen/drivers/passthrough/pci.c
> index e886894..507b3d1 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -845,6 +845,9 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
> list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> if ( pdev->bus == bus && pdev->devfn == devfn )
> {
> + ret = -EBUSY;
> + if ( pdev->domain && pdev->domain != hardware_domain )
> + break;
This seems like an unlrelated fix?
ie: preventing device removal while in use by a domain different than
dom0?
Note that you don't need the pdev->domain != NULL check, just doing
pdev->domain != hardware_domain seems enough, since you don't
dereference the pdev->domain pointer in the expression (unless I'm
missing other usages below).
> ret = iommu_remove_device(pdev);
> if ( pdev->domain )
> list_del(&pdev->domain_list);
> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
> index 8b27d7e..79f9682 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 c0c6e7c..5b24428 100644
> --- a/xen/include/asm-x86/irq.h
> +++ b/xen/include/asm-x86/irq.h
> @@ -155,7 +155,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 *);
>
> diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
> index 586b783..c7a6a83 100644
> --- a/xen/include/xen/irq.h
> +++ b/xen/include/xen/irq.h
> @@ -91,6 +91,7 @@ typedef struct irq_desc {
> spinlock_t lock;
> struct arch_irq_desc arch;
> cpumask_var_t affinity;
> + domid_t creator_domid; /* weak reference to domain handling this IRQ */
I feel like handling is too vague here, but I'm not a native speaker
so I'm not sure. I would maybe write:
... domain having permissions over this IRQ (which can be different
from the domain actually having the IRQ assigned) */
Which I think is less ambiguous.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control Marek Marczykowski-Górecki
@ 2019-07-17 10:18 ` Roger Pau Monné
2019-07-17 23:54 ` Marek Marczykowski-Górecki
2019-07-19 10:40 ` Jan Beulich
1 sibling, 1 reply; 39+ messages in thread
From: Roger Pau Monné @ 2019-07-17 10:18 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Jan Beulich, xen-devel, Daniel De Graaf
On Wed, Jul 17, 2019 at 03:00:43AM +0200, Marek Marczykowski-Górecki wrote:
> Allow device model running in stubdomain to enable/disable MSI(-X),
> bypassing pciback. While pciback is still used to access config space
> from within stubdomain, it refuse to write to
> PCI_MSI_FLAGS_ENABLE/PCI_MSIX_FLAGS_ENABLE in non-permissive mode. Which
> is the right thing to do for PV domain (the main use case for pciback),
> as PV domain should use XEN_PCI_OP_* commands for that. Unfortunately
> those commands are not good for stubdomain use, as they configure MSI in
> dom0's kernel too, which should not happen for HVM domain.
>
> This new physdevop is allowed only for stubdomain controlling the domain
> which own the device.
I think I'm missing that part, is this maybe done by the XSM stuff?
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> Changes in v3:
> - new patch
> Changes in v4:
> - adjust code style
> - s/msi_msix/msi/
> - add msi_set_enable XSM hook
> - flatten struct physdev_msi_set_enable
> - add to include/xlat.lst
> Changes in v5:
> - rename to PHYSDEVOP_msi_control
> - combine "mode" and "enable" into "flags"
> - refuse to enable both MSI and MSI-X, and also to enable MSI(-X) on
> incapable device
> - disable/enable INTx when enabling/disabling MSI (?)
You don't enable INTx when MSI is disabled.
> - refuse if !use_msi
> - adjust flask hook to make more sense (require "setup" access on
> device, not on domain)
> - rebase on master
>
> I'm not sure if XSM part is correct, compile-tested only, as I'm not
> sure how to set the policy.
I'm also not familiar with XSM, so I will have to defer to Daniel on
this one.
> ---
> xen/arch/x86/msi.c | 42 ++++++++++++++++++++++++++++++-
> xen/arch/x86/physdev.c | 25 ++++++++++++++++++-
> xen/arch/x86/x86_64/physdev.c | 4 +++-
> xen/include/asm-x86/msi.h | 1 +-
> xen/include/public/physdev.h | 16 +++++++++++-
> xen/include/xlat.lst | 1 +-
> xen/include/xsm/dummy.h | 7 +++++-
> xen/include/xsm/xsm.h | 6 ++++-
> xen/xsm/dummy.c | 1 +-
> xen/xsm/flask/hooks.c | 24 +++++++++++++++++-
> xen/xsm/flask/policy/access_vectors | 1 +-
> 11 files changed, 128 insertions(+)
>
> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index 89e6116..fca1d04 100644
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -1475,6 +1475,48 @@ int pci_restore_msi_state(struct pci_dev *pdev)
> return 0;
> }
>
> +int msi_control(struct pci_dev *pdev, bool msix, bool enable)
> +{
> + int ret;
> + struct msi_desc *old_desc;
> +
> + if ( !use_msi )
> + return -EOPNOTSUPP;
> +
> + ret = xsm_msi_control(XSM_DM_PRIV, pdev->domain, pdev->sbdf.sbdf, msix, enable);
> + if ( ret )
> + return ret;
> +
> + if ( msix )
> + {
> + if ( !pdev->msix )
> + return -ENODEV;
> + old_desc = find_msi_entry(pdev, -1, PCI_CAP_ID_MSI);
> + if ( old_desc )
> + return -EBUSY;
> + if ( enable )
> + pci_intx(pdev, false);
> + msix_set_enable(pdev, enable);
> + }
> + else
> + {
> + if ( !pci_find_cap_offset(pdev->seg,
> + pdev->bus,
> + PCI_SLOT(pdev->devfn),
> + PCI_FUNC(pdev->devfn),
> + PCI_CAP_ID_MSI) )
> + return -ENODEV;
> + old_desc = find_msi_entry(pdev, -1, PCI_CAP_ID_MSIX);
> + if ( old_desc )
> + return -EBUSY;
> + if ( enable )
> + pci_intx(pdev, false);
> + msi_set_enable(pdev, enable);
> + }
I think you could just do:
unsigned int cap = msix ? PCI_CAP_ID_MSIX : PCI_CAP_ID_MSI;
[...]
if ( !pci_find_cap_offset(pdev->seg,
pdev->bus,
PCI_SLOT(pdev->devfn),
PCI_FUNC(pdev->devfn), cap) )
return -ENODEV;
old_desc = find_msi_entry(pdev, -1, cap);
if ( old_desc )
return -EBUSY;
if ( enable )
{
pci_intx(pdev, false);
if ( msix )
msi_set_enable(pdev, false);
else
msix_set_enable(pdev, false);
}
if ( msix )
msix_set_enable(pdev, enable);
else
msi_set_enable(pdev, enable);
Note that in the same way you make sure INTx is disabled, you should
also make sure MSI and MSI-X are not enabled at the same time.
> +
> + return 0;
> +}
> +
> void __init early_msi_init(void)
> {
> if ( use_msi < 0 )
> diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
> index 3a3c158..5000998 100644
> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -662,6 +662,31 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> break;
> }
>
> + case PHYSDEVOP_msi_control: {
> + struct physdev_msi_control op;
> + struct pci_dev *pdev;
> +
> + ret = -EFAULT;
> + if ( copy_from_guest(&op, arg, 1) )
> + break;
> +
> + ret = -EINVAL;
> + if ( op.flags & ~(PHYSDEVOP_MSI_CONTROL_MSIX | PHYSDEVOP_MSI_CONTROL_ENABLE) )
> + break;
> +
> + pcidevs_lock();
> + pdev = pci_get_pdev(op.seg, op.bus, op.devfn);
> + if ( pdev )
> + ret = msi_control(pdev,
> + op.flags & PHYSDEVOP_MSI_CONTROL_MSIX,
> + op.flags & PHYSDEVOP_MSI_CONTROL_ENABLE);
> + else
> + ret = -ENODEV;
> + pcidevs_unlock();
> + break;
> +
Extra newline.
> + }
> +
> default:
> ret = -ENOSYS;
> break;
> diff --git a/xen/arch/x86/x86_64/physdev.c b/xen/arch/x86/x86_64/physdev.c
> index c5a00ea..69b4ce3 100644
> --- a/xen/arch/x86/x86_64/physdev.c
> +++ b/xen/arch/x86/x86_64/physdev.c
> @@ -76,6 +76,10 @@ CHECK_physdev_pci_device_add
> CHECK_physdev_pci_device
> #undef xen_physdev_pci_device
>
> +#define xen_physdev_msi_control physdev_msi_control
> +CHECK_physdev_msi_control
> +#undef xen_physdev_msi_control
> +
> #define COMPAT
> #undef guest_handle_okay
> #define guest_handle_okay compat_handle_okay
> diff --git a/xen/include/asm-x86/msi.h b/xen/include/asm-x86/msi.h
> index 10387dc..05296de 100644
> --- a/xen/include/asm-x86/msi.h
> +++ b/xen/include/asm-x86/msi.h
> @@ -252,5 +252,6 @@ void guest_mask_msi_irq(struct irq_desc *, bool mask);
> void ack_nonmaskable_msi_irq(struct irq_desc *);
> void end_nonmaskable_msi_irq(struct irq_desc *, u8 vector);
> void set_msi_affinity(struct irq_desc *, const cpumask_t *);
> +int msi_control(struct pci_dev *pdev, bool msix, bool enable);
>
> #endif /* __ASM_MSI_H */
> diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
> index b6faf83..f9b728f 100644
> --- a/xen/include/public/physdev.h
> +++ b/xen/include/public/physdev.h
> @@ -344,6 +344,22 @@ struct physdev_dbgp_op {
> typedef struct physdev_dbgp_op physdev_dbgp_op_t;
> DEFINE_XEN_GUEST_HANDLE(physdev_dbgp_op_t);
>
> +/* when PHYSDEVOP_MSI_CONTROL_MSIX not set, control MSI */
> +#define PHYSDEVOP_MSI_CONTROL_MSIX 1
> +/* when PHYSDEVOP_MSI_CONTROL_ENABLE not set, disable */
> +#define PHYSDEVOP_MSI_CONTROL_ENABLE 2
> +
> +#define PHYSDEVOP_msi_control 32
> +struct physdev_msi_control {
> + /* IN */
> + uint16_t seg;
> + uint8_t bus;
> + uint8_t devfn;
> + uint8_t flags;
I would just make flags uint32_t, the padding to align is going to be
added in any case.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 6/6] tools/libxc: add wrapper for PHYSDEVOP_msi_control
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 6/6] tools/libxc: add wrapper for PHYSDEVOP_msi_control Marek Marczykowski-Górecki
@ 2019-07-17 10:21 ` Roger Pau Monné
2019-07-18 0:12 ` Marek Marczykowski-Górecki
0 siblings, 1 reply; 39+ messages in thread
From: Roger Pau Monné @ 2019-07-17 10:21 UTC (permalink / raw)
To: Marek Marczykowski-Górecki; +Cc: xen-devel, Ian Jackson, Wei Liu
On Wed, Jul 17, 2019 at 03:00:44AM +0200, Marek Marczykowski-Górecki wrote:
> Add libxc wrapper for PHYSDEVOP_msi_control introduced in previous
> commit.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
LGTM, albeit I find the usage of int instead of unsigned int for the
SBDF kind of weird, but it's inline with the other functions, so I
guess there's a reason for it?
I assume this will be used by an upcoming QEMU patch?
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 4/6] xen/x86: Allow stubdom access to irq created for msi.
2019-07-17 9:54 ` Roger Pau Monné
@ 2019-07-17 15:09 ` Marek Marczykowski-Górecki
2019-07-18 9:29 ` Roger Pau Monné
0 siblings, 1 reply; 39+ messages in thread
From: Marek Marczykowski-Górecki @ 2019-07-17 15:09 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Kevin Tian, Stefano Stabellini, Suravee Suthikulpanit, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Simon Gaiser, Julien Grall, Jan Beulich, xen-devel,
Brian Woods
[-- Attachment #1.1: Type: text/plain, Size: 14075 bytes --]
On Wed, Jul 17, 2019 at 11:54:35AM +0200, Roger Pau Monné wrote:
> On Wed, Jul 17, 2019 at 03:00:42AM +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.
> >
> > 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()
> > ---
> > xen/arch/x86/hpet.c | 3 +-
> > xen/arch/x86/irq.c | 51 ++++++++++++++++++-------
> > xen/common/irq.c | 1 +-
> > xen/drivers/char/ns16550.c | 2 +-
> > xen/drivers/passthrough/amd/iommu_init.c | 2 +-
> > xen/drivers/passthrough/pci.c | 3 +-
> > xen/drivers/passthrough/vtd/iommu.c | 3 +-
> > xen/include/asm-x86/irq.h | 2 +-
> > xen/include/xen/irq.h | 1 +-
> > 9 files changed, 50 insertions(+), 18 deletions(-)
> >
> > diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
> > index 4b08488..b4854ff 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, hardware_domain)) < 0 )
>
> Shouldn't this be NULL? I don't think the hardware domain should be
> allowed to play with the HPET IRQs?
Good point.
> > return irq;
> >
> > ch->msi.irq = irq;
> > diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> > index 2cac28a..dc5d302 100644
> > --- a/xen/arch/x86/irq.c
> > +++ b/xen/arch/x86/irq.c
> > @@ -164,10 +164,21 @@ int __init bind_irq_vector(int irq, int vector, const cpumask_t *cpu_mask)
> > return ret;
> > }
> >
> > +static struct domain *get_dm_domain(struct domain *d)
> ^ const
> > +{
> > + return current->domain->target == d ? current->domain : hardware_domain;
> > +}
> > +
> > /*
> > * 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;
> > @@ -200,18 +211,24 @@ int create_irq(nodeid_t node)
> > desc->arch.used = IRQ_UNUSED;
> > irq = ret;
> > }
>
> I would assert that desc->creator_domid == DOMID_INVALID here, since
> in the failure case creator_domid is not overwritten.
Yes, see below.
> > - 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->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);
> > @@ -220,14 +237,22 @@ void destroy_irq(unsigned int irq)
> >
> > BUG_ON(!MSI_IRQ(irq));
> >
> > - if ( hardware_domain )
> > + if ( desc->creator_domid != DOMID_INVALID )
> > {
> > - int err = irq_deny_access(hardware_domain, irq);
> > + struct domain *d = get_domain_by_id(desc->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);
>
> Don't you need to set creator_domid = DOMID_INVALID in destroy_irq at
> some point?
>
> Or else a failure in create_irq could leak the irq to it's previous
> owner. Note that init_one_irq_desc would only init the fields the
> first time the IRQ is used, but not for subsequent usages AFAICT.
I assumed init_one_irq_desc do the work on subsequent usages too. If not,
indeed I need to modify creator_domid in few more places.
> > }
> >
> > spin_lock_irqsave(&desc->lock, flags);
> > @@ -2058,7 +2083,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, get_dm_domain(d));
>
> Isn't it fine to just use current->domain here directly?
>
> It's always going to be the current domain the one that calls
> map_domain_pirq in order to get a PIRQ mapped for it's target
> domain I think.
I wasn't sure if that's true if all the cases. Especially if hardware
domain != toolstack domain. How is it then? Is it hardware domain
calling map_domain_pirq in that case?
> > ret = irq >= 0 ? prepare_domain_irq_pirq(d, irq, pirq + nr, &info)
> > : irq;
> > if ( ret < 0 )
> > @@ -2691,7 +2716,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, get_dm_domain(d));
> > }
> >
> > if ( irq < nr_irqs_gsi || irq >= nr_irqs )
> > diff --git a/xen/common/irq.c b/xen/common/irq.c
> > index f42512d..42b27a9 100644
> > --- a/xen/common/irq.c
> > +++ b/xen/common/irq.c
> > @@ -16,6 +16,7 @@ int init_one_irq_desc(struct irq_desc *desc)
> > spin_lock_init(&desc->lock);
> > cpumask_setall(desc->affinity);
> > INIT_LIST_HEAD(&desc->rl_link);
> > + desc->creator_domid = DOMID_INVALID;
> >
> > err = arch_init_one_irq_desc(desc);
> > if ( err )
> > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> > index 189e121..ccc8b04 100644
> > --- a/xen/drivers/char/ns16550.c
> > +++ b/xen/drivers/char/ns16550.c
> > @@ -719,7 +719,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 4e76b26..50785e0 100644
> > --- a/xen/drivers/passthrough/amd/iommu_init.c
> > +++ b/xen/drivers/passthrough/amd/iommu_init.c
> > @@ -781,7 +781,7 @@ static bool_t __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
> > hw_irq_controller *handler;
> > u16 control;
> >
> > - 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/pci.c b/xen/drivers/passthrough/pci.c
> > index e886894..507b3d1 100644
> > --- a/xen/drivers/passthrough/pci.c
> > +++ b/xen/drivers/passthrough/pci.c
> > @@ -845,6 +845,9 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
> > list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> > if ( pdev->bus == bus && pdev->devfn == devfn )
> > {
> > + ret = -EBUSY;
> > + if ( pdev->domain && pdev->domain != hardware_domain )
> > + break;
>
> This seems like an unlrelated fix?
>
> ie: preventing device removal while in use by a domain different than
> dom0?
Indeed it may warrant separate commit now.
> Note that you don't need the pdev->domain != NULL check, just doing
> pdev->domain != hardware_domain seems enough, since you don't
> dereference the pdev->domain pointer in the expression (unless I'm
> missing other usages below).
I don't want to prevent removal if pdev->domain is NULL (if that's even
possible).
> > ret = iommu_remove_device(pdev);
> > if ( pdev->domain )
> > list_del(&pdev->domain_list);
> > diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
> > index 8b27d7e..79f9682 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 c0c6e7c..5b24428 100644
> > --- a/xen/include/asm-x86/irq.h
> > +++ b/xen/include/asm-x86/irq.h
> > @@ -155,7 +155,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 *);
> >
> > diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
> > index 586b783..c7a6a83 100644
> > --- a/xen/include/xen/irq.h
> > +++ b/xen/include/xen/irq.h
> > @@ -91,6 +91,7 @@ typedef struct irq_desc {
> > spinlock_t lock;
> > struct arch_irq_desc arch;
> > cpumask_var_t affinity;
> > + domid_t creator_domid; /* weak reference to domain handling this IRQ */
>
> I feel like handling is too vague here, but I'm not a native speaker
> so I'm not sure. I would maybe write:
>
> ... domain having permissions over this IRQ (which can be different
> from the domain actually having the IRQ assigned) */
>
> Which I think is less ambiguous.
I wanted to fit the comment in one line. But your version indeed may be
better.
--
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
2019-07-17 10:18 ` Roger Pau Monné
@ 2019-07-17 23:54 ` Marek Marczykowski-Górecki
2019-07-18 13:46 ` Roger Pau Monné
0 siblings, 1 reply; 39+ messages in thread
From: Marek Marczykowski-Górecki @ 2019-07-17 23:54 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Jan Beulich, xen-devel, Daniel De Graaf
[-- Attachment #1.1: Type: text/plain, Size: 9526 bytes --]
On Wed, Jul 17, 2019 at 12:18:43PM +0200, Roger Pau Monné wrote:
> On Wed, Jul 17, 2019 at 03:00:43AM +0200, Marek Marczykowski-Górecki wrote:
> > Allow device model running in stubdomain to enable/disable MSI(-X),
> > bypassing pciback. While pciback is still used to access config space
> > from within stubdomain, it refuse to write to
> > PCI_MSI_FLAGS_ENABLE/PCI_MSIX_FLAGS_ENABLE in non-permissive mode. Which
> > is the right thing to do for PV domain (the main use case for pciback),
> > as PV domain should use XEN_PCI_OP_* commands for that. Unfortunately
> > those commands are not good for stubdomain use, as they configure MSI in
> > dom0's kernel too, which should not happen for HVM domain.
> >
> > This new physdevop is allowed only for stubdomain controlling the domain
> > which own the device.
>
> I think I'm missing that part, is this maybe done by the XSM stuff?
Yes, specifically xsm_msi_control(XSM_DM_PRIV, pdev->domain, ...) call.
XSM_DM_PRIV allows call if src->is_privileged, or if src->target ==
target. Note the target being owner of the device here.
> >
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > ---
> > Changes in v3:
> > - new patch
> > Changes in v4:
> > - adjust code style
> > - s/msi_msix/msi/
> > - add msi_set_enable XSM hook
> > - flatten struct physdev_msi_set_enable
> > - add to include/xlat.lst
> > Changes in v5:
> > - rename to PHYSDEVOP_msi_control
> > - combine "mode" and "enable" into "flags"
> > - refuse to enable both MSI and MSI-X, and also to enable MSI(-X) on
> > incapable device
> > - disable/enable INTx when enabling/disabling MSI (?)
>
> You don't enable INTx when MSI is disabled.
Ah, indeed. When I look for similar code in Xen elsewhere, I found
__pci_disable_msi() has extra conditions for pci_intx(dev, true):
if ( entry->irq > 0 && !(irq_to_desc(entry->irq)->status & IRQ_GUEST) )
pci_intx(dev, true);
Should this be mirrored there too? Isn't IRQ_GUEST always set in case of
passthrough to HVM (the case this patch care)?
> > - refuse if !use_msi
> > - adjust flask hook to make more sense (require "setup" access on
> > device, not on domain)
> > - rebase on master
> >
> > I'm not sure if XSM part is correct, compile-tested only, as I'm not
> > sure how to set the policy.
>
> I'm also not familiar with XSM, so I will have to defer to Daniel on
> this one.
>
> > ---
> > xen/arch/x86/msi.c | 42 ++++++++++++++++++++++++++++++-
> > xen/arch/x86/physdev.c | 25 ++++++++++++++++++-
> > xen/arch/x86/x86_64/physdev.c | 4 +++-
> > xen/include/asm-x86/msi.h | 1 +-
> > xen/include/public/physdev.h | 16 +++++++++++-
> > xen/include/xlat.lst | 1 +-
> > xen/include/xsm/dummy.h | 7 +++++-
> > xen/include/xsm/xsm.h | 6 ++++-
> > xen/xsm/dummy.c | 1 +-
> > xen/xsm/flask/hooks.c | 24 +++++++++++++++++-
> > xen/xsm/flask/policy/access_vectors | 1 +-
> > 11 files changed, 128 insertions(+)
> >
> > diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> > index 89e6116..fca1d04 100644
> > --- a/xen/arch/x86/msi.c
> > +++ b/xen/arch/x86/msi.c
> > @@ -1475,6 +1475,48 @@ int pci_restore_msi_state(struct pci_dev *pdev)
> > return 0;
> > }
> >
> > +int msi_control(struct pci_dev *pdev, bool msix, bool enable)
> > +{
> > + int ret;
> > + struct msi_desc *old_desc;
> > +
> > + if ( !use_msi )
> > + return -EOPNOTSUPP;
> > +
> > + ret = xsm_msi_control(XSM_DM_PRIV, pdev->domain, pdev->sbdf.sbdf, msix, enable);
> > + if ( ret )
> > + return ret;
> > +
> > + if ( msix )
> > + {
> > + if ( !pdev->msix )
> > + return -ENODEV;
> > + old_desc = find_msi_entry(pdev, -1, PCI_CAP_ID_MSI);
> > + if ( old_desc )
> > + return -EBUSY;
> > + if ( enable )
> > + pci_intx(pdev, false);
> > + msix_set_enable(pdev, enable);
> > + }
> > + else
> > + {
> > + if ( !pci_find_cap_offset(pdev->seg,
> > + pdev->bus,
> > + PCI_SLOT(pdev->devfn),
> > + PCI_FUNC(pdev->devfn),
> > + PCI_CAP_ID_MSI) )
> > + return -ENODEV;
> > + old_desc = find_msi_entry(pdev, -1, PCI_CAP_ID_MSIX);
> > + if ( old_desc )
> > + return -EBUSY;
> > + if ( enable )
> > + pci_intx(pdev, false);
> > + msi_set_enable(pdev, enable);
> > + }
>
> I think you could just do:
>
> unsigned int cap = msix ? PCI_CAP_ID_MSIX : PCI_CAP_ID_MSI;
>
> [...]
>
> if ( !pci_find_cap_offset(pdev->seg,
> pdev->bus,
> PCI_SLOT(pdev->devfn),
> PCI_FUNC(pdev->devfn), cap) )
> return -ENODEV;
>
> old_desc = find_msi_entry(pdev, -1, cap);
> if ( old_desc )
> return -EBUSY;
Note the check prevents enabling MSI when MSI-X is enabled and vice
versa. Not enabling already enabled MSI. Anyway, if using
pci_find_cap_offset for PCI_CAP_ID_MSIX too, this code indeed can be
deduplicated a little.
> if ( enable )
> {
> pci_intx(pdev, false);
> if ( msix )
> msi_set_enable(pdev, false);
> else
> msix_set_enable(pdev, false);
> }
>
> if ( msix )
> msix_set_enable(pdev, enable);
> else
> msi_set_enable(pdev, enable);
>
> Note that in the same way you make sure INTx is disabled, you should
> also make sure MSI and MSI-X are not enabled at the same time.
This is exactly what the code in the patch already does.
> > +
> > + return 0;
> > +}
> > +
> > void __init early_msi_init(void)
> > {
> > if ( use_msi < 0 )
> > diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
> > index 3a3c158..5000998 100644
> > --- a/xen/arch/x86/physdev.c
> > +++ b/xen/arch/x86/physdev.c
> > @@ -662,6 +662,31 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> > break;
> > }
> >
> > + case PHYSDEVOP_msi_control: {
> > + struct physdev_msi_control op;
> > + struct pci_dev *pdev;
> > +
> > + ret = -EFAULT;
> > + if ( copy_from_guest(&op, arg, 1) )
> > + break;
> > +
> > + ret = -EINVAL;
> > + if ( op.flags & ~(PHYSDEVOP_MSI_CONTROL_MSIX | PHYSDEVOP_MSI_CONTROL_ENABLE) )
> > + break;
> > +
> > + pcidevs_lock();
> > + pdev = pci_get_pdev(op.seg, op.bus, op.devfn);
> > + if ( pdev )
> > + ret = msi_control(pdev,
> > + op.flags & PHYSDEVOP_MSI_CONTROL_MSIX,
> > + op.flags & PHYSDEVOP_MSI_CONTROL_ENABLE);
> > + else
> > + ret = -ENODEV;
> > + pcidevs_unlock();
> > + break;
> > +
>
> Extra newline.
>
> > + }
> > +
> > default:
> > ret = -ENOSYS;
> > break;
> > diff --git a/xen/arch/x86/x86_64/physdev.c b/xen/arch/x86/x86_64/physdev.c
> > index c5a00ea..69b4ce3 100644
> > --- a/xen/arch/x86/x86_64/physdev.c
> > +++ b/xen/arch/x86/x86_64/physdev.c
> > @@ -76,6 +76,10 @@ CHECK_physdev_pci_device_add
> > CHECK_physdev_pci_device
> > #undef xen_physdev_pci_device
> >
> > +#define xen_physdev_msi_control physdev_msi_control
> > +CHECK_physdev_msi_control
> > +#undef xen_physdev_msi_control
> > +
> > #define COMPAT
> > #undef guest_handle_okay
> > #define guest_handle_okay compat_handle_okay
> > diff --git a/xen/include/asm-x86/msi.h b/xen/include/asm-x86/msi.h
> > index 10387dc..05296de 100644
> > --- a/xen/include/asm-x86/msi.h
> > +++ b/xen/include/asm-x86/msi.h
> > @@ -252,5 +252,6 @@ void guest_mask_msi_irq(struct irq_desc *, bool mask);
> > void ack_nonmaskable_msi_irq(struct irq_desc *);
> > void end_nonmaskable_msi_irq(struct irq_desc *, u8 vector);
> > void set_msi_affinity(struct irq_desc *, const cpumask_t *);
> > +int msi_control(struct pci_dev *pdev, bool msix, bool enable);
> >
> > #endif /* __ASM_MSI_H */
> > diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h
> > index b6faf83..f9b728f 100644
> > --- a/xen/include/public/physdev.h
> > +++ b/xen/include/public/physdev.h
> > @@ -344,6 +344,22 @@ struct physdev_dbgp_op {
> > typedef struct physdev_dbgp_op physdev_dbgp_op_t;
> > DEFINE_XEN_GUEST_HANDLE(physdev_dbgp_op_t);
> >
> > +/* when PHYSDEVOP_MSI_CONTROL_MSIX not set, control MSI */
> > +#define PHYSDEVOP_MSI_CONTROL_MSIX 1
> > +/* when PHYSDEVOP_MSI_CONTROL_ENABLE not set, disable */
> > +#define PHYSDEVOP_MSI_CONTROL_ENABLE 2
> > +
> > +#define PHYSDEVOP_msi_control 32
> > +struct physdev_msi_control {
> > + /* IN */
> > + uint16_t seg;
> > + uint8_t bus;
> > + uint8_t devfn;
> > + uint8_t flags;
>
> I would just make flags uint32_t, the padding to align is going to be
> added in any case.
That would make the structure 8 bytes, not 6. Did you mean uint16_t?
But structure size here doesn't really matter anyway.
--
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 6/6] tools/libxc: add wrapper for PHYSDEVOP_msi_control
2019-07-17 10:21 ` Roger Pau Monné
@ 2019-07-18 0:12 ` Marek Marczykowski-Górecki
2019-07-18 13:53 ` Roger Pau Monné
0 siblings, 1 reply; 39+ messages in thread
From: Marek Marczykowski-Górecki @ 2019-07-18 0:12 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: xen-devel, Ian Jackson, Wei Liu
[-- Attachment #1.1: Type: text/plain, Size: 859 bytes --]
On Wed, Jul 17, 2019 at 12:21:58PM +0200, Roger Pau Monné wrote:
> On Wed, Jul 17, 2019 at 03:00:44AM +0200, Marek Marczykowski-Górecki wrote:
> > Add libxc wrapper for PHYSDEVOP_msi_control introduced in previous
> > commit.
> >
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>
> LGTM, albeit I find the usage of int instead of unsigned int for the
> SBDF kind of weird, but it's inline with the other functions, so I
> guess there's a reason for it?
Yes, it was based on looking at other places. But I don't know if there
is any specific reason for it.
> I assume this will be used by an upcoming QEMU patch?
Yes.
--
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 4/6] xen/x86: Allow stubdom access to irq created for msi.
2019-07-17 15:09 ` Marek Marczykowski-Górecki
@ 2019-07-18 9:29 ` Roger Pau Monné
2019-07-18 15:12 ` Marek Marczykowski-Górecki
0 siblings, 1 reply; 39+ messages in thread
From: Roger Pau Monné @ 2019-07-18 9:29 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Kevin Tian, Stefano Stabellini, Suravee Suthikulpanit, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Simon Gaiser, Julien Grall, Jan Beulich, xen-devel,
Brian Woods
On Wed, Jul 17, 2019 at 05:09:12PM +0200, Marek Marczykowski-Górecki wrote:
> On Wed, Jul 17, 2019 at 11:54:35AM +0200, Roger Pau Monné wrote:
> > On Wed, Jul 17, 2019 at 03:00:42AM +0200, Marek Marczykowski-Górecki wrote:
> > > @@ -220,14 +237,22 @@ void destroy_irq(unsigned int irq)
> > >
> > > BUG_ON(!MSI_IRQ(irq));
> > >
> > > - if ( hardware_domain )
> > > + if ( desc->creator_domid != DOMID_INVALID )
> > > {
> > > - int err = irq_deny_access(hardware_domain, irq);
> > > + struct domain *d = get_domain_by_id(desc->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);
> >
> > Don't you need to set creator_domid = DOMID_INVALID in destroy_irq at
> > some point?
> >
> > Or else a failure in create_irq could leak the irq to it's previous
> > owner. Note that init_one_irq_desc would only init the fields the
> > first time the IRQ is used, but not for subsequent usages AFAICT.
>
> I assumed init_one_irq_desc do the work on subsequent usages too. If not,
> indeed I need to modify creator_domid in few more places.
I don't think so, init_one_irq_desc will only init the fields if
handler == NULL, which will only happen the first time the IRQ is
used, afterwards handler is set to &no_irq_type by destroy_irq.
Just setting creator_domid = DOMID_INVALID in destroy_irq and adding
the assert to create_irq should be enough AFAICT, since those
functions are used exclusively by non-shared IRQs (MSI and MSI-X).
> > > }
> > >
> > > spin_lock_irqsave(&desc->lock, flags);
> > > @@ -2058,7 +2083,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, get_dm_domain(d));
> >
> > Isn't it fine to just use current->domain here directly?
> >
> > It's always going to be the current domain the one that calls
> > map_domain_pirq in order to get a PIRQ mapped for it's target
> > domain I think.
>
> I wasn't sure if that's true if all the cases. Especially if hardware
> domain != toolstack domain. How is it then? Is it hardware domain
> calling map_domain_pirq in that case?
But then it's going to be the hardware domain the one that runs the
QEMU instance, and hence the one that issues the hypercalls to
map/unmap PIRQs to a target domain?
ie: the PCI backend (either pciback or QEMU) is not going to run on
the toolstack domain.
I'm afraid I don't see a case where current->domain isn't the domain
also requiring permissions over the IRQ, but I could be wrong. Can you
come up with a detailed scenario where this might happen?
>
> > > ret = irq >= 0 ? prepare_domain_irq_pirq(d, irq, pirq + nr, &info)
> > > : irq;
> > > if ( ret < 0 )
> > > @@ -2691,7 +2716,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, get_dm_domain(d));
> > > }
> > >
> > > if ( irq < nr_irqs_gsi || irq >= nr_irqs )
> > > diff --git a/xen/common/irq.c b/xen/common/irq.c
> > > index f42512d..42b27a9 100644
> > > --- a/xen/common/irq.c
> > > +++ b/xen/common/irq.c
> > > @@ -16,6 +16,7 @@ int init_one_irq_desc(struct irq_desc *desc)
> > > spin_lock_init(&desc->lock);
> > > cpumask_setall(desc->affinity);
> > > INIT_LIST_HEAD(&desc->rl_link);
> > > + desc->creator_domid = DOMID_INVALID;
> > >
> > > err = arch_init_one_irq_desc(desc);
> > > if ( err )
> > > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> > > index 189e121..ccc8b04 100644
> > > --- a/xen/drivers/char/ns16550.c
> > > +++ b/xen/drivers/char/ns16550.c
> > > @@ -719,7 +719,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 4e76b26..50785e0 100644
> > > --- a/xen/drivers/passthrough/amd/iommu_init.c
> > > +++ b/xen/drivers/passthrough/amd/iommu_init.c
> > > @@ -781,7 +781,7 @@ static bool_t __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
> > > hw_irq_controller *handler;
> > > u16 control;
> > >
> > > - 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/pci.c b/xen/drivers/passthrough/pci.c
> > > index e886894..507b3d1 100644
> > > --- a/xen/drivers/passthrough/pci.c
> > > +++ b/xen/drivers/passthrough/pci.c
> > > @@ -845,6 +845,9 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
> > > list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> > > if ( pdev->bus == bus && pdev->devfn == devfn )
> > > {
> > > + ret = -EBUSY;
> > > + if ( pdev->domain && pdev->domain != hardware_domain )
> > > + break;
> >
> > This seems like an unlrelated fix?
> >
> > ie: preventing device removal while in use by a domain different than
> > dom0?
>
> Indeed it may warrant separate commit now.
>
> > Note that you don't need the pdev->domain != NULL check, just doing
> > pdev->domain != hardware_domain seems enough, since you don't
> > dereference the pdev->domain pointer in the expression (unless I'm
> > missing other usages below).
>
> I don't want to prevent removal if pdev->domain is NULL (if that's even
> possible).
But if pdev->domain == NULL, then it's certainly going to be different
from hardware_domain, so just using pdev->domain != hardware_domain
achieves both.
> > > ret = iommu_remove_device(pdev);
> > > if ( pdev->domain )
> > > list_del(&pdev->domain_list);
> > > diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
> > > index 8b27d7e..79f9682 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 c0c6e7c..5b24428 100644
> > > --- a/xen/include/asm-x86/irq.h
> > > +++ b/xen/include/asm-x86/irq.h
> > > @@ -155,7 +155,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 *);
> > >
> > > diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
> > > index 586b783..c7a6a83 100644
> > > --- a/xen/include/xen/irq.h
> > > +++ b/xen/include/xen/irq.h
> > > @@ -91,6 +91,7 @@ typedef struct irq_desc {
> > > spinlock_t lock;
> > > struct arch_irq_desc arch;
> > > cpumask_var_t affinity;
> > > + domid_t creator_domid; /* weak reference to domain handling this IRQ */
> >
> > I feel like handling is too vague here, but I'm not a native speaker
> > so I'm not sure. I would maybe write:
> >
> > ... domain having permissions over this IRQ (which can be different
> > from the domain actually having the IRQ assigned) */
> >
> > Which I think is less ambiguous.
>
> I wanted to fit the comment in one line. But your version indeed may be
> better.
I would always error towards being more verbose, even if that means
using more that one line. As said, I don't think the comment is wrong,
just feel it could be more detailed.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
2019-07-17 23:54 ` Marek Marczykowski-Górecki
@ 2019-07-18 13:46 ` Roger Pau Monné
2019-07-18 15:17 ` Jan Beulich
0 siblings, 1 reply; 39+ messages in thread
From: Roger Pau Monné @ 2019-07-18 13:46 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, Jan Beulich, xen-devel, Daniel De Graaf
On Thu, Jul 18, 2019 at 01:54:26AM +0200, Marek Marczykowski-Górecki wrote:
> On Wed, Jul 17, 2019 at 12:18:43PM +0200, Roger Pau Monné wrote:
> > On Wed, Jul 17, 2019 at 03:00:43AM +0200, Marek Marczykowski-Górecki wrote:
> > > Allow device model running in stubdomain to enable/disable MSI(-X),
> > > bypassing pciback. While pciback is still used to access config space
> > > from within stubdomain, it refuse to write to
> > > PCI_MSI_FLAGS_ENABLE/PCI_MSIX_FLAGS_ENABLE in non-permissive mode. Which
> > > is the right thing to do for PV domain (the main use case for pciback),
> > > as PV domain should use XEN_PCI_OP_* commands for that. Unfortunately
> > > those commands are not good for stubdomain use, as they configure MSI in
> > > dom0's kernel too, which should not happen for HVM domain.
> > >
> > > This new physdevop is allowed only for stubdomain controlling the domain
> > > which own the device.
> >
> > I think I'm missing that part, is this maybe done by the XSM stuff?
>
> Yes, specifically xsm_msi_control(XSM_DM_PRIV, pdev->domain, ...) call.
> XSM_DM_PRIV allows call if src->is_privileged, or if src->target ==
> target. Note the target being owner of the device here.
Oh thanks, I'm certainly quite lost when it comes to XSM.
> > >
> > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > > ---
> > > Changes in v3:
> > > - new patch
> > > Changes in v4:
> > > - adjust code style
> > > - s/msi_msix/msi/
> > > - add msi_set_enable XSM hook
> > > - flatten struct physdev_msi_set_enable
> > > - add to include/xlat.lst
> > > Changes in v5:
> > > - rename to PHYSDEVOP_msi_control
> > > - combine "mode" and "enable" into "flags"
> > > - refuse to enable both MSI and MSI-X, and also to enable MSI(-X) on
> > > incapable device
> > > - disable/enable INTx when enabling/disabling MSI (?)
> >
> > You don't enable INTx when MSI is disabled.
>
> Ah, indeed. When I look for similar code in Xen elsewhere, I found
> __pci_disable_msi() has extra conditions for pci_intx(dev, true):
>
> if ( entry->irq > 0 && !(irq_to_desc(entry->irq)->status & IRQ_GUEST) )
> pci_intx(dev, true);
>
> Should this be mirrored there too? Isn't IRQ_GUEST always set in case of
> passthrough to HVM (the case this patch care)?
It is, but you would have to iterate over all the entries, which I
don't think it's intended here. A guest could disable MSI(-X) while
having entries setup, and I don't think tearing down those entries
should be done here.
In fact I don't think INTx should be enabled when MSI(-X) is disabled,
QEMU already traps writes to the command register, and it will manage
INTx enabling/disabling by itself. I think the only check required is
that MSI(-X) cannot be enabled if INTx is also enabled. In the same
way both MSI caspabilities cannot be enabled simultaneously. The
function should not explicitly disable any of the other capabilities,
and just return -EBUSY if the caller attempts for example to enable
MSI while INTx or MSI-X is enabled.
> > if ( enable )
> > {
> > pci_intx(pdev, false);
> > if ( msix )
> > msi_set_enable(pdev, false);
> > else
> > msix_set_enable(pdev, false);
> > }
> >
> > if ( msix )
> > msix_set_enable(pdev, enable);
> > else
> > msi_set_enable(pdev, enable);
> >
> > Note that in the same way you make sure INTx is disabled, you should
> > also make sure MSI and MSI-X are not enabled at the same time.
>
> This is exactly what the code in the patch already does.
See my rant above, I don't think this hypercall should be touching
INTx, or disabling/enabling other MSI capabilities, and instead just
returning -EBUSY if the requested operation is not accepted because
there are other capabilities enabled.
> > > +/* when PHYSDEVOP_MSI_CONTROL_MSIX not set, control MSI */
> > > +#define PHYSDEVOP_MSI_CONTROL_MSIX 1
> > > +/* when PHYSDEVOP_MSI_CONTROL_ENABLE not set, disable */
> > > +#define PHYSDEVOP_MSI_CONTROL_ENABLE 2
> > > +
> > > +#define PHYSDEVOP_msi_control 32
> > > +struct physdev_msi_control {
> > > + /* IN */
> > > + uint16_t seg;
> > > + uint8_t bus;
> > > + uint8_t devfn;
> > > + uint8_t flags;
> >
> > I would just make flags uint32_t, the padding to align is going to be
> > added in any case.
>
> That would make the structure 8 bytes, not 6. Did you mean uint16_t?
> But structure size here doesn't really matter anyway.
Yes sorry, uint16_t. I don't have a strong opinion, but since the
structure is already 6bytes in size, I thought it might be better to
have that padding assigned to flags instead of being hidden.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 6/6] tools/libxc: add wrapper for PHYSDEVOP_msi_control
2019-07-18 0:12 ` Marek Marczykowski-Górecki
@ 2019-07-18 13:53 ` Roger Pau Monné
0 siblings, 0 replies; 39+ messages in thread
From: Roger Pau Monné @ 2019-07-18 13:53 UTC (permalink / raw)
To: Marek Marczykowski-Górecki; +Cc: xen-devel, Ian Jackson, Wei Liu
On Thu, Jul 18, 2019 at 02:12:20AM +0200, Marek Marczykowski-Górecki wrote:
> On Wed, Jul 17, 2019 at 12:21:58PM +0200, Roger Pau Monné wrote:
> > On Wed, Jul 17, 2019 at 03:00:44AM +0200, Marek Marczykowski-Górecki wrote:
> > > Add libxc wrapper for PHYSDEVOP_msi_control introduced in previous
> > > commit.
> > >
> > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> >
> > LGTM, albeit I find the usage of int instead of unsigned int for the
> > SBDF kind of weird, but it's inline with the other functions, so I
> > guess there's a reason for it?
>
> Yes, it was based on looking at other places. But I don't know if there
> is any specific reason for it.
Anyway, since using unsigned or signed is not really that relevant
here, and seeing how other functions are defined:
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
With just a couple of nits: you don't actually need rc, you can just `return
do_physdev_op...`, and you could also initialize physdev_msi_control
at declaration, but I don't have a strong opinion on any of those, so
you can keep the RB regardless.
Thanks!
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 4/6] xen/x86: Allow stubdom access to irq created for msi.
2019-07-18 9:29 ` Roger Pau Monné
@ 2019-07-18 15:12 ` Marek Marczykowski-Górecki
2019-07-18 16:55 ` Roger Pau Monné
0 siblings, 1 reply; 39+ messages in thread
From: Marek Marczykowski-Górecki @ 2019-07-18 15:12 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Kevin Tian, Stefano Stabellini, Suravee Suthikulpanit, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Simon Gaiser, Julien Grall, Jan Beulich, xen-devel,
Brian Woods
[-- Attachment #1.1: Type: text/plain, Size: 7521 bytes --]
On Thu, Jul 18, 2019 at 11:29:39AM +0200, Roger Pau Monné wrote:
> On Wed, Jul 17, 2019 at 05:09:12PM +0200, Marek Marczykowski-Górecki wrote:
> > On Wed, Jul 17, 2019 at 11:54:35AM +0200, Roger Pau Monné wrote:
> > > On Wed, Jul 17, 2019 at 03:00:42AM +0200, Marek Marczykowski-Górecki wrote:
> > > > @@ -220,14 +237,22 @@ void destroy_irq(unsigned int irq)
> > > >
> > > > BUG_ON(!MSI_IRQ(irq));
> > > >
> > > > - if ( hardware_domain )
> > > > + if ( desc->creator_domid != DOMID_INVALID )
> > > > {
> > > > - int err = irq_deny_access(hardware_domain, irq);
> > > > + struct domain *d = get_domain_by_id(desc->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);
> > >
> > > Don't you need to set creator_domid = DOMID_INVALID in destroy_irq at
> > > some point?
> > >
> > > Or else a failure in create_irq could leak the irq to it's previous
> > > owner. Note that init_one_irq_desc would only init the fields the
> > > first time the IRQ is used, but not for subsequent usages AFAICT.
> >
> > I assumed init_one_irq_desc do the work on subsequent usages too. If not,
> > indeed I need to modify creator_domid in few more places.
>
> I don't think so, init_one_irq_desc will only init the fields if
> handler == NULL, which will only happen the first time the IRQ is
> used, afterwards handler is set to &no_irq_type by destroy_irq.
>
> Just setting creator_domid = DOMID_INVALID in destroy_irq and adding
> the assert to create_irq should be enough AFAICT, since those
> functions are used exclusively by non-shared IRQs (MSI and MSI-X).
Ok.
> > > > }
> > > >
> > > > spin_lock_irqsave(&desc->lock, flags);
> > > > @@ -2058,7 +2083,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, get_dm_domain(d));
> > >
> > > Isn't it fine to just use current->domain here directly?
> > >
> > > It's always going to be the current domain the one that calls
> > > map_domain_pirq in order to get a PIRQ mapped for it's target
> > > domain I think.
> >
> > I wasn't sure if that's true if all the cases. Especially if hardware
> > domain != toolstack domain. How is it then? Is it hardware domain
> > calling map_domain_pirq in that case?
>
> But then it's going to be the hardware domain the one that runs the
> QEMU instance, and hence the one that issues the hypercalls to
> map/unmap PIRQs to a target domain?
>
> ie: the PCI backend (either pciback or QEMU) is not going to run on
> the toolstack domain.
Indeed, you're right. This also means get_dm_domain() helper wouldn't be
needed anymore.
> I'm afraid I don't see a case where current->domain isn't the domain
> also requiring permissions over the IRQ, but I could be wrong. Can you
> come up with a detailed scenario where this might happen?
>
> >
> > > > ret = irq >= 0 ? prepare_domain_irq_pirq(d, irq, pirq + nr, &info)
> > > > : irq;
> > > > if ( ret < 0 )
> > > > @@ -2691,7 +2716,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, get_dm_domain(d));
> > > > }
> > > >
> > > > if ( irq < nr_irqs_gsi || irq >= nr_irqs )
> > > > diff --git a/xen/common/irq.c b/xen/common/irq.c
> > > > index f42512d..42b27a9 100644
> > > > --- a/xen/common/irq.c
> > > > +++ b/xen/common/irq.c
> > > > @@ -16,6 +16,7 @@ int init_one_irq_desc(struct irq_desc *desc)
> > > > spin_lock_init(&desc->lock);
> > > > cpumask_setall(desc->affinity);
> > > > INIT_LIST_HEAD(&desc->rl_link);
> > > > + desc->creator_domid = DOMID_INVALID;
> > > >
> > > > err = arch_init_one_irq_desc(desc);
> > > > if ( err )
> > > > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> > > > index 189e121..ccc8b04 100644
> > > > --- a/xen/drivers/char/ns16550.c
> > > > +++ b/xen/drivers/char/ns16550.c
> > > > @@ -719,7 +719,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 4e76b26..50785e0 100644
> > > > --- a/xen/drivers/passthrough/amd/iommu_init.c
> > > > +++ b/xen/drivers/passthrough/amd/iommu_init.c
> > > > @@ -781,7 +781,7 @@ static bool_t __init set_iommu_interrupt_handler(struct amd_iommu *iommu)
> > > > hw_irq_controller *handler;
> > > > u16 control;
> > > >
> > > > - 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/pci.c b/xen/drivers/passthrough/pci.c
> > > > index e886894..507b3d1 100644
> > > > --- a/xen/drivers/passthrough/pci.c
> > > > +++ b/xen/drivers/passthrough/pci.c
> > > > @@ -845,6 +845,9 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
> > > > list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> > > > if ( pdev->bus == bus && pdev->devfn == devfn )
> > > > {
> > > > + ret = -EBUSY;
> > > > + if ( pdev->domain && pdev->domain != hardware_domain )
> > > > + break;
> > >
> > > This seems like an unlrelated fix?
> > >
> > > ie: preventing device removal while in use by a domain different than
> > > dom0?
> >
> > Indeed it may warrant separate commit now.
> >
> > > Note that you don't need the pdev->domain != NULL check, just doing
> > > pdev->domain != hardware_domain seems enough, since you don't
> > > dereference the pdev->domain pointer in the expression (unless I'm
> > > missing other usages below).
> >
> > I don't want to prevent removal if pdev->domain is NULL (if that's even
> > possible).
>
> But if pdev->domain == NULL, then it's certainly going to be different
> from hardware_domain,
Exactly. And I do _not_ want to hit that break if pdev->domain == NULL.
> so just using pdev->domain != hardware_domain
> achieves both.
--
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
2019-07-18 13:46 ` Roger Pau Monné
@ 2019-07-18 15:17 ` Jan Beulich
2019-07-18 16:52 ` Roger Pau Monné
0 siblings, 1 reply; 39+ messages in thread
From: Jan Beulich @ 2019-07-18 15:17 UTC (permalink / raw)
To: Roger Pau Monné, Marek Marczykowski-Górecki
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, AndrewCooper, Ian Jackson, TimDeegan,
Julien Grall, xen-devel, Daniel De Graaf
On 18.07.2019 15:46, Roger Pau Monné wrote:
> In fact I don't think INTx should be enabled when MSI(-X) is disabled,
> QEMU already traps writes to the command register, and it will manage
> INTx enabling/disabling by itself. I think the only check required is
> that MSI(-X) cannot be enabled if INTx is also enabled. In the same
> way both MSI caspabilities cannot be enabled simultaneously. The
> function should not explicitly disable any of the other capabilities,
> and just return -EBUSY if the caller attempts for example to enable
> MSI while INTx or MSI-X is enabled.
You do realize that pci_intx() only ever gets called for Xen
internally used interrupts, i.e. mainly the serial console one?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
2019-07-18 15:17 ` Jan Beulich
@ 2019-07-18 16:52 ` Roger Pau Monné
2019-07-19 8:04 ` Jan Beulich
0 siblings, 1 reply; 39+ messages in thread
From: Roger Pau Monné @ 2019-07-18 16:52 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, AndrewCooper, Ian Jackson,
Marek Marczykowski-Górecki, TimDeegan, Julien Grall,
xen-devel, Daniel De Graaf
On Thu, Jul 18, 2019 at 03:17:27PM +0000, Jan Beulich wrote:
> On 18.07.2019 15:46, Roger Pau Monné wrote:
> > In fact I don't think INTx should be enabled when MSI(-X) is disabled,
> > QEMU already traps writes to the command register, and it will manage
> > INTx enabling/disabling by itself. I think the only check required is
> > that MSI(-X) cannot be enabled if INTx is also enabled. In the same
> > way both MSI caspabilities cannot be enabled simultaneously. The
> > function should not explicitly disable any of the other capabilities,
> > and just return -EBUSY if the caller attempts for example to enable
> > MSI while INTx or MSI-X is enabled.
>
> You do realize that pci_intx() only ever gets called for Xen
> internally used interrupts, i.e. mainly the serial console one?
You will have to bear with me because I'm not sure I understand why
it does matter. Do you mean to point out that dom0 is the one in full
control of INTx, and thus Xen shouldn't care of whether INTx and
MSI(-X) are enabled at the same time?
I still think that at least a warning should be printed if a caller
tries to enable MSI(-X) while INTx is also enabled, but unless there's
a reason to have both MSI(-X) and INTx enabled at the same time (maybe
a quirk for some hardware issue?) it shouldn't be allowed on this new
interface.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 4/6] xen/x86: Allow stubdom access to irq created for msi.
2019-07-18 15:12 ` Marek Marczykowski-Górecki
@ 2019-07-18 16:55 ` Roger Pau Monné
0 siblings, 0 replies; 39+ messages in thread
From: Roger Pau Monné @ 2019-07-18 16:55 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Kevin Tian, Stefano Stabellini, Suravee Suthikulpanit, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Simon Gaiser, Julien Grall, Jan Beulich, xen-devel,
Brian Woods
On Thu, Jul 18, 2019 at 05:12:54PM +0200, Marek Marczykowski-Górecki wrote:
> On Thu, Jul 18, 2019 at 11:29:39AM +0200, Roger Pau Monné wrote:
> > On Wed, Jul 17, 2019 at 05:09:12PM +0200, Marek Marczykowski-Górecki wrote:
> > > On Wed, Jul 17, 2019 at 11:54:35AM +0200, Roger Pau Monné wrote:
> > > > On Wed, Jul 17, 2019 at 03:00:42AM +0200, Marek Marczykowski-Górecki wrote:
> > > > > diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> > > > > index e886894..507b3d1 100644
> > > > > --- a/xen/drivers/passthrough/pci.c
> > > > > +++ b/xen/drivers/passthrough/pci.c
> > > > > @@ -845,6 +845,9 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
> > > > > list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> > > > > if ( pdev->bus == bus && pdev->devfn == devfn )
> > > > > {
> > > > > + ret = -EBUSY;
> > > > > + if ( pdev->domain && pdev->domain != hardware_domain )
> > > > > + break;
> > > >
> > > > This seems like an unlrelated fix?
> > > >
> > > > ie: preventing device removal while in use by a domain different than
> > > > dom0?
> > >
> > > Indeed it may warrant separate commit now.
> > >
> > > > Note that you don't need the pdev->domain != NULL check, just doing
> > > > pdev->domain != hardware_domain seems enough, since you don't
> > > > dereference the pdev->domain pointer in the expression (unless I'm
> > > > missing other usages below).
> > >
> > > I don't want to prevent removal if pdev->domain is NULL (if that's even
> > > possible).
> >
> > But if pdev->domain == NULL, then it's certainly going to be different
> > from hardware_domain,
>
> Exactly. And I do _not_ want to hit that break if pdev->domain == NULL.
Oh sorry for being obtuse, you are right. In any case, please send
this as a separate patch.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
2019-07-18 16:52 ` Roger Pau Monné
@ 2019-07-19 8:04 ` Jan Beulich
2019-07-19 9:02 ` Roger Pau Monné
0 siblings, 1 reply; 39+ messages in thread
From: Jan Beulich @ 2019-07-19 8:04 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, AndrewCooper, Ian Jackson,
Marek Marczykowski-Górecki, TimDeegan, Julien Grall,
xen-devel, Daniel De Graaf
On 18.07.2019 18:52, Roger Pau Monné wrote:
> On Thu, Jul 18, 2019 at 03:17:27PM +0000, Jan Beulich wrote:
>> On 18.07.2019 15:46, Roger Pau Monné wrote:
>>> In fact I don't think INTx should be enabled when MSI(-X) is disabled,
>>> QEMU already traps writes to the command register, and it will manage
>>> INTx enabling/disabling by itself. I think the only check required is
>>> that MSI(-X) cannot be enabled if INTx is also enabled. In the same
>>> way both MSI caspabilities cannot be enabled simultaneously. The
>>> function should not explicitly disable any of the other capabilities,
>>> and just return -EBUSY if the caller attempts for example to enable
>>> MSI while INTx or MSI-X is enabled.
>>
>> You do realize that pci_intx() only ever gets called for Xen
>> internally used interrupts, i.e. mainly the serial console one?
>
> You will have to bear with me because I'm not sure I understand why
> it does matter. Do you mean to point out that dom0 is the one in full
> control of INTx, and thus Xen shouldn't care of whether INTx and
> MSI(-X) are enabled at the same time?
>
> I still think that at least a warning should be printed if a caller
> tries to enable MSI(-X) while INTx is also enabled, but unless there's
> a reason to have both MSI(-X) and INTx enabled at the same time (maybe
> a quirk for some hardware issue?) it shouldn't be allowed on this new
> interface.
I don't mind improvements to the current situation (i.e. such a
warning may indeed make sense); I merely stated how things currently
are. INTx treatment was completely left aside when MSI support was
introduced into Xen.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
2019-07-19 8:04 ` Jan Beulich
@ 2019-07-19 9:02 ` Roger Pau Monné
2019-07-19 9:43 ` Jan Beulich
0 siblings, 1 reply; 39+ messages in thread
From: Roger Pau Monné @ 2019-07-19 9:02 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, AndrewCooper, Ian Jackson,
Marek Marczykowski-Górecki, TimDeegan, Julien Grall,
xen-devel, Daniel De Graaf
On Fri, Jul 19, 2019 at 08:04:45AM +0000, Jan Beulich wrote:
> On 18.07.2019 18:52, Roger Pau Monné wrote:
> > On Thu, Jul 18, 2019 at 03:17:27PM +0000, Jan Beulich wrote:
> >> On 18.07.2019 15:46, Roger Pau Monné wrote:
> >>> In fact I don't think INTx should be enabled when MSI(-X) is disabled,
> >>> QEMU already traps writes to the command register, and it will manage
> >>> INTx enabling/disabling by itself. I think the only check required is
> >>> that MSI(-X) cannot be enabled if INTx is also enabled. In the same
> >>> way both MSI caspabilities cannot be enabled simultaneously. The
> >>> function should not explicitly disable any of the other capabilities,
> >>> and just return -EBUSY if the caller attempts for example to enable
> >>> MSI while INTx or MSI-X is enabled.
> >>
> >> You do realize that pci_intx() only ever gets called for Xen
> >> internally used interrupts, i.e. mainly the serial console one?
> >
> > You will have to bear with me because I'm not sure I understand why
> > it does matter. Do you mean to point out that dom0 is the one in full
> > control of INTx, and thus Xen shouldn't care of whether INTx and
> > MSI(-X) are enabled at the same time?
> >
> > I still think that at least a warning should be printed if a caller
> > tries to enable MSI(-X) while INTx is also enabled, but unless there's
> > a reason to have both MSI(-X) and INTx enabled at the same time (maybe
> > a quirk for some hardware issue?) it shouldn't be allowed on this new
> > interface.
>
> I don't mind improvements to the current situation (i.e. such a
> warning may indeed make sense); I merely stated how things currently
> are. INTx treatment was completely left aside when MSI support was
> introduced into Xen.
In order to give Marek a more concise reply, would you agree to return
-EBUSY (or some error code) and print a warning message if the caller
attempts to enable MSI(-X) while INTx is also enabled?
The same would apply to enabling both MSI(-X) capabilities at the
same time.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
2019-07-19 9:02 ` Roger Pau Monné
@ 2019-07-19 9:43 ` Jan Beulich
2019-08-05 13:44 ` Marek Marczykowski-Górecki
0 siblings, 1 reply; 39+ messages in thread
From: Jan Beulich @ 2019-07-19 9:43 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, AndrewCooper, Ian Jackson,
Marek Marczykowski-Górecki, TimDeegan, Julien Grall,
xen-devel, Daniel De Graaf
On 19.07.2019 11:02, Roger Pau Monné wrote:
> On Fri, Jul 19, 2019 at 08:04:45AM +0000, Jan Beulich wrote:
>> On 18.07.2019 18:52, Roger Pau Monné wrote:
>>> On Thu, Jul 18, 2019 at 03:17:27PM +0000, Jan Beulich wrote:
>>>> On 18.07.2019 15:46, Roger Pau Monné wrote:
>>>>> In fact I don't think INTx should be enabled when MSI(-X) is disabled,
>>>>> QEMU already traps writes to the command register, and it will manage
>>>>> INTx enabling/disabling by itself. I think the only check required is
>>>>> that MSI(-X) cannot be enabled if INTx is also enabled. In the same
>>>>> way both MSI caspabilities cannot be enabled simultaneously. The
>>>>> function should not explicitly disable any of the other capabilities,
>>>>> and just return -EBUSY if the caller attempts for example to enable
>>>>> MSI while INTx or MSI-X is enabled.
>>>>
>>>> You do realize that pci_intx() only ever gets called for Xen
>>>> internally used interrupts, i.e. mainly the serial console one?
>>>
>>> You will have to bear with me because I'm not sure I understand why
>>> it does matter. Do you mean to point out that dom0 is the one in full
>>> control of INTx, and thus Xen shouldn't care of whether INTx and
>>> MSI(-X) are enabled at the same time?
>>>
>>> I still think that at least a warning should be printed if a caller
>>> tries to enable MSI(-X) while INTx is also enabled, but unless there's
>>> a reason to have both MSI(-X) and INTx enabled at the same time (maybe
>>> a quirk for some hardware issue?) it shouldn't be allowed on this new
>>> interface.
>>
>> I don't mind improvements to the current situation (i.e. such a
>> warning may indeed make sense); I merely stated how things currently
>> are. INTx treatment was completely left aside when MSI support was
>> introduced into Xen.
>
> In order to give Marek a more concise reply, would you agree to return
> -EBUSY (or some error code) and print a warning message if the caller
> attempts to enable MSI(-X) while INTx is also enabled?
As to returning an error - I think so, yes. I'm less sure about logging
a message.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control Marek Marczykowski-Górecki
2019-07-17 10:18 ` Roger Pau Monné
@ 2019-07-19 10:40 ` Jan Beulich
1 sibling, 0 replies; 39+ messages in thread
From: Jan Beulich @ 2019-07-19 10:40 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall, xen-devel, Daniel De Graaf, Roger Pau Monné
On 17.07.2019 03:00, Marek Marczykowski-Górecki wrote:
> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -662,6 +662,31 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> break;
> }
>
> + case PHYSDEVOP_msi_control: {
> + struct physdev_msi_control op;
> + struct pci_dev *pdev;
> +
> + ret = -EFAULT;
> + if ( copy_from_guest(&op, arg, 1) )
> + break;
> +
> + ret = -EINVAL;
> + if ( op.flags & ~(PHYSDEVOP_MSI_CONTROL_MSIX | PHYSDEVOP_MSI_CONTROL_ENABLE) )
> + break;
> +
> + pcidevs_lock();
> + pdev = pci_get_pdev(op.seg, op.bus, op.devfn);
> + if ( pdev )
> + ret = msi_control(pdev,
> + op.flags & PHYSDEVOP_MSI_CONTROL_MSIX,
> + op.flags & PHYSDEVOP_MSI_CONTROL_ENABLE);
Note that pci_get_pdev() returns hidden devices as well. That's
not a problem for the other two uses in this file, but I think
you want to explicitly deny access to hidden ones here,
irrespective of the XSM check.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 4/6] xen/x86: Allow stubdom access to irq created for msi.
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 4/6] xen/x86: Allow stubdom access to irq created for msi Marek Marczykowski-Górecki
2019-07-17 9:54 ` Roger Pau Monné
@ 2019-07-20 16:48 ` Julien Grall
2019-07-20 21:21 ` Marek Marczykowski-Górecki
1 sibling, 1 reply; 39+ messages in thread
From: Julien Grall @ 2019-07-20 16:48 UTC (permalink / raw)
To: Marek Marczykowski-Górecki, xen-devel
Cc: Kevin Tian, Stefano Stabellini, Suravee Suthikulpanit, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Simon Gaiser, Jan Beulich, Brian Woods,
Roger Pau Monné
Hi,
Sorry for jumping late in the discussion.
On 7/17/19 2:00 AM, Marek Marczykowski-Górecki wrote:
> diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
> index 586b783..c7a6a83 100644
> --- a/xen/include/xen/irq.h
> +++ b/xen/include/xen/irq.h
> @@ -91,6 +91,7 @@ typedef struct irq_desc {
> spinlock_t lock;
> struct arch_irq_desc arch;
> cpumask_var_t affinity;
> + domid_t creator_domid; /* weak reference to domain handling this IRQ */
This x86 only. Can this be moved in arch_irq_desc to avoid increasing
the structure on Arm?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 4/6] xen/x86: Allow stubdom access to irq created for msi.
2019-07-20 16:48 ` Julien Grall
@ 2019-07-20 21:21 ` Marek Marczykowski-Górecki
2019-07-21 18:05 ` Julien Grall
0 siblings, 1 reply; 39+ messages in thread
From: Marek Marczykowski-Górecki @ 2019-07-20 21:21 UTC (permalink / raw)
To: Julien Grall
Cc: Kevin Tian, Stefano Stabellini, Suravee Suthikulpanit, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Simon Gaiser, Jan Beulich, xen-devel, Brian Woods,
Roger Pau Monné
[-- Attachment #1.1: Type: text/plain, Size: 1172 bytes --]
On Sat, Jul 20, 2019 at 05:48:56PM +0100, Julien Grall wrote:
> Hi,
>
> Sorry for jumping late in the discussion.
>
> On 7/17/19 2:00 AM, Marek Marczykowski-Górecki wrote:
> > diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
> > index 586b783..c7a6a83 100644
> > --- a/xen/include/xen/irq.h
> > +++ b/xen/include/xen/irq.h
> > @@ -91,6 +91,7 @@ typedef struct irq_desc {
> > spinlock_t lock;
> > struct arch_irq_desc arch;
> > cpumask_var_t affinity;
> > + domid_t creator_domid; /* weak reference to domain handling this IRQ */
>
> This x86 only. Can this be moved in arch_irq_desc to avoid increasing the
> structure on Arm?
How (if at all) PCI passthrough is supported on ARM? Is qemu involved?
If so, and if that qemu would be isolated in stubdomain, I think ARM
would need a similar feature. If it not the case right now, but it is
planned, do you think it's worth moving it to arch_irq_desc and possibly
move back later?
--
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 4/6] xen/x86: Allow stubdom access to irq created for msi.
2019-07-20 21:21 ` Marek Marczykowski-Górecki
@ 2019-07-21 18:05 ` Julien Grall
2019-07-22 8:45 ` Roger Pau Monné
0 siblings, 1 reply; 39+ messages in thread
From: Julien Grall @ 2019-07-21 18:05 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Kevin Tian, Stefano Stabellini, Suravee Suthikulpanit, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Simon Gaiser, Jan Beulich, xen-devel, Brian Woods,
Roger Pau Monné
Hi,
On 7/20/19 10:21 PM, Marek Marczykowski-Górecki wrote:
> On Sat, Jul 20, 2019 at 05:48:56PM +0100, Julien Grall wrote:
>> Hi,
>>
>> Sorry for jumping late in the discussion.
>>
>> On 7/17/19 2:00 AM, Marek Marczykowski-Górecki wrote:
>>> diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
>>> index 586b783..c7a6a83 100644
>>> --- a/xen/include/xen/irq.h
>>> +++ b/xen/include/xen/irq.h
>>> @@ -91,6 +91,7 @@ typedef struct irq_desc {
>>> spinlock_t lock;
>>> struct arch_irq_desc arch;
>>> cpumask_var_t affinity;
>>> + domid_t creator_domid; /* weak reference to domain handling this IRQ */
>>
>> This x86 only. Can this be moved in arch_irq_desc to avoid increasing the
>> structure on Arm?
>
> How (if at all) PCI passthrough is supported on ARM? Is qemu involved?
> If so, and if that qemu would be isolated in stubdomain, I think ARM
> would need a similar feature. If it not the case right now, but it is
> planned, do you think it's worth moving it to arch_irq_desc and possibly
> move back later?
PCI passthrough is not yet supported on Arm. However, I would not expect
QEMU to be involved at all for Arm.
In any case, I would still prefer to keep field in arch_irq_desc until
we see any usage on Arm.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 4/6] xen/x86: Allow stubdom access to irq created for msi.
2019-07-21 18:05 ` Julien Grall
@ 2019-07-22 8:45 ` Roger Pau Monné
2019-07-22 9:06 ` Julien Grall
0 siblings, 1 reply; 39+ messages in thread
From: Roger Pau Monné @ 2019-07-22 8:45 UTC (permalink / raw)
To: Julien Grall
Cc: Kevin Tian, Stefano Stabellini, Suravee Suthikulpanit, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Marek Marczykowski-Górecki, Tim Deegan, Simon Gaiser,
Jan Beulich, xen-devel, Brian Woods
On Sun, Jul 21, 2019 at 07:05:16PM +0100, Julien Grall wrote:
> Hi,
>
> On 7/20/19 10:21 PM, Marek Marczykowski-Górecki wrote:
> > On Sat, Jul 20, 2019 at 05:48:56PM +0100, Julien Grall wrote:
> > > Hi,
> > >
> > > Sorry for jumping late in the discussion.
> > >
> > > On 7/17/19 2:00 AM, Marek Marczykowski-Górecki wrote:
> > > > diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
> > > > index 586b783..c7a6a83 100644
> > > > --- a/xen/include/xen/irq.h
> > > > +++ b/xen/include/xen/irq.h
> > > > @@ -91,6 +91,7 @@ typedef struct irq_desc {
> > > > spinlock_t lock;
> > > > struct arch_irq_desc arch;
> > > > cpumask_var_t affinity;
> > > > + domid_t creator_domid; /* weak reference to domain handling this IRQ */
> > >
> > > This x86 only. Can this be moved in arch_irq_desc to avoid increasing the
> > > structure on Arm?
> >
> > How (if at all) PCI passthrough is supported on ARM? Is qemu involved?
> > If so, and if that qemu would be isolated in stubdomain, I think ARM
> > would need a similar feature. If it not the case right now, but it is
> > planned, do you think it's worth moving it to arch_irq_desc and possibly
> > move back later?
>
> PCI passthrough is not yet supported on Arm. However, I would not expect
> QEMU to be involved at all for Arm.
>
> In any case, I would still prefer to keep field in arch_irq_desc until we
> see any usage on Arm.
I'm fine with putting it inside of the arch struct, but there's
nothing x86 specific about this field. Regardless of whether you use
QEMU or something different, if you want to allow passthrough on ARM
from domains != dom0 you will need this field anyway, so that the
domain running the passthrough emulator can properly manage interrupts
on behalf of the guest.
If you only plan to support something like vPCI inside of Xen then I
guess there's no need for the field, since Xen is always going to be
the one doing the passthrough.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 4/6] xen/x86: Allow stubdom access to irq created for msi.
2019-07-22 8:45 ` Roger Pau Monné
@ 2019-07-22 9:06 ` Julien Grall
2019-07-22 9:16 ` Julien Grall
0 siblings, 1 reply; 39+ messages in thread
From: Julien Grall @ 2019-07-22 9:06 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Kevin Tian, Stefano Stabellini, Suravee Suthikulpanit, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Marek Marczykowski-Górecki, Tim Deegan, Simon Gaiser,
Jan Beulich, xen-devel, Brian Woods
Hi Roger,
On 22/07/2019 09:45, Roger Pau Monné wrote:
> On Sun, Jul 21, 2019 at 07:05:16PM +0100, Julien Grall wrote:
>> Hi,
>>
>> On 7/20/19 10:21 PM, Marek Marczykowski-Górecki wrote:
>>> On Sat, Jul 20, 2019 at 05:48:56PM +0100, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> Sorry for jumping late in the discussion.
>>>>
>>>> On 7/17/19 2:00 AM, Marek Marczykowski-Górecki wrote:
>>>>> diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
>>>>> index 586b783..c7a6a83 100644
>>>>> --- a/xen/include/xen/irq.h
>>>>> +++ b/xen/include/xen/irq.h
>>>>> @@ -91,6 +91,7 @@ typedef struct irq_desc {
>>>>> spinlock_t lock;
>>>>> struct arch_irq_desc arch;
>>>>> cpumask_var_t affinity;
>>>>> + domid_t creator_domid; /* weak reference to domain handling this IRQ */
>>>>
>>>> This x86 only. Can this be moved in arch_irq_desc to avoid increasing the
>>>> structure on Arm?
>>>
>>> How (if at all) PCI passthrough is supported on ARM? Is qemu involved?
>>> If so, and if that qemu would be isolated in stubdomain, I think ARM
>>> would need a similar feature. If it not the case right now, but it is
>>> planned, do you think it's worth moving it to arch_irq_desc and possibly
>>> move back later?
>>
>> PCI passthrough is not yet supported on Arm. However, I would not expect
>> QEMU to be involved at all for Arm.
>>
>> In any case, I would still prefer to keep field in arch_irq_desc until we
>> see any usage on Arm.
>
> I'm fine with putting it inside of the arch struct, but there's
> nothing x86 specific about this field.
In theory yes, in practice this is only used by x86 today. I don't want to
increase the size of irq_desc for unused field.
We can move because the field in irq_desc if there are a need on Arm.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 4/6] xen/x86: Allow stubdom access to irq created for msi.
2019-07-22 9:06 ` Julien Grall
@ 2019-07-22 9:16 ` Julien Grall
0 siblings, 0 replies; 39+ messages in thread
From: Julien Grall @ 2019-07-22 9:16 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Kevin Tian, Stefano Stabellini, Suravee Suthikulpanit, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Marek Marczykowski-Górecki, Tim Deegan, Simon Gaiser,
Jan Beulich, xen-devel, Brian Woods
On 22/07/2019 10:06, Julien Grall wrote:
> Hi Roger,
>
> On 22/07/2019 09:45, Roger Pau Monné wrote:
>> On Sun, Jul 21, 2019 at 07:05:16PM +0100, Julien Grall wrote:
>>> Hi,
>>>
>>> On 7/20/19 10:21 PM, Marek Marczykowski-Górecki wrote:
>>>> On Sat, Jul 20, 2019 at 05:48:56PM +0100, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> Sorry for jumping late in the discussion.
>>>>>
>>>>> On 7/17/19 2:00 AM, Marek Marczykowski-Górecki wrote:
>>>>>> diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
>>>>>> index 586b783..c7a6a83 100644
>>>>>> --- a/xen/include/xen/irq.h
>>>>>> +++ b/xen/include/xen/irq.h
>>>>>> @@ -91,6 +91,7 @@ typedef struct irq_desc {
>>>>>> spinlock_t lock;
>>>>>> struct arch_irq_desc arch;
>>>>>> cpumask_var_t affinity;
>>>>>> + domid_t creator_domid; /* weak reference to domain handling this IRQ */
>>>>>
>>>>> This x86 only. Can this be moved in arch_irq_desc to avoid increasing the
>>>>> structure on Arm?
>>>>
>>>> How (if at all) PCI passthrough is supported on ARM? Is qemu involved?
>>>> If so, and if that qemu would be isolated in stubdomain, I think ARM
>>>> would need a similar feature. If it not the case right now, but it is
>>>> planned, do you think it's worth moving it to arch_irq_desc and possibly
>>>> move back later?
>>>
>>> PCI passthrough is not yet supported on Arm. However, I would not expect
>>> QEMU to be involved at all for Arm.
>>>
>>> In any case, I would still prefer to keep field in arch_irq_desc until we
>>> see any usage on Arm.
>>
>> I'm fine with putting it inside of the arch struct, but there's
>> nothing x86 specific about this field.
>
> In theory yes, in practice this is only used by x86 today. I don't want to
> increase the size of irq_desc for unused field.
>
> We can move because the field in irq_desc if there are a need on Arm.
s/because/later/
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
2019-07-19 9:43 ` Jan Beulich
@ 2019-08-05 13:44 ` Marek Marczykowski-Górecki
2019-08-06 7:56 ` Jan Beulich
0 siblings, 1 reply; 39+ messages in thread
From: Marek Marczykowski-Górecki @ 2019-08-05 13:44 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, AndrewCooper, Ian Jackson, TimDeegan,
Julien Grall, xen-devel, Daniel De Graaf, Roger Pau Monné
[-- Attachment #1.1: Type: text/plain, Size: 3180 bytes --]
On Fri, Jul 19, 2019 at 09:43:26AM +0000, Jan Beulich wrote:
> On 19.07.2019 11:02, Roger Pau Monné wrote:
> > On Fri, Jul 19, 2019 at 08:04:45AM +0000, Jan Beulich wrote:
> >> On 18.07.2019 18:52, Roger Pau Monné wrote:
> >>> On Thu, Jul 18, 2019 at 03:17:27PM +0000, Jan Beulich wrote:
> >>>> On 18.07.2019 15:46, Roger Pau Monné wrote:
> >>>>> In fact I don't think INTx should be enabled when MSI(-X) is disabled,
> >>>>> QEMU already traps writes to the command register, and it will manage
> >>>>> INTx enabling/disabling by itself. I think the only check required is
> >>>>> that MSI(-X) cannot be enabled if INTx is also enabled. In the same
> >>>>> way both MSI caspabilities cannot be enabled simultaneously. The
> >>>>> function should not explicitly disable any of the other capabilities,
> >>>>> and just return -EBUSY if the caller attempts for example to enable
> >>>>> MSI while INTx or MSI-X is enabled.
> >>>>
> >>>> You do realize that pci_intx() only ever gets called for Xen
> >>>> internally used interrupts, i.e. mainly the serial console one?
> >>>
> >>> You will have to bear with me because I'm not sure I understand why
> >>> it does matter. Do you mean to point out that dom0 is the one in full
> >>> control of INTx, and thus Xen shouldn't care of whether INTx and
> >>> MSI(-X) are enabled at the same time?
> >>>
> >>> I still think that at least a warning should be printed if a caller
> >>> tries to enable MSI(-X) while INTx is also enabled, but unless there's
> >>> a reason to have both MSI(-X) and INTx enabled at the same time (maybe
> >>> a quirk for some hardware issue?) it shouldn't be allowed on this new
> >>> interface.
> >>
> >> I don't mind improvements to the current situation (i.e. such a
> >> warning may indeed make sense); I merely stated how things currently
> >> are. INTx treatment was completely left aside when MSI support was
> >> introduced into Xen.
> >
> > In order to give Marek a more concise reply, would you agree to return
> > -EBUSY (or some error code) and print a warning message if the caller
> > attempts to enable MSI(-X) while INTx is also enabled?
>
> As to returning an error - I think so, yes. I'm less sure about logging
> a message.
I'm trying to get it working and it isn't clear to me what should I
check for "INTx is also enabled". I assumed PCI_COMMAND_INTX_DISABLE
bit, but it looks like guest has no control over this bit, even in
permissive mode. This means enabling MSI(-X) always fails because guest
has no way to set PCI_COMMAND_INTX_DISABLE bit before.
Should I check something different? Or change back to disabling/enabling
INTx as part of msi_control call? Or maybe allow guest/qemu to control
PCI_COMMAND_INTX_DISABLE bit? In that case, I'd have very similar
problem as with MSI - xen-pciback doesn't allow that, so I'd either need
to patch pciback (and I could move this whole patch into Linux
instead), or get around with a hypercall.
--
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
2019-08-05 13:44 ` Marek Marczykowski-Górecki
@ 2019-08-06 7:56 ` Jan Beulich
2019-08-06 9:46 ` Marek Marczykowski-Górecki
0 siblings, 1 reply; 39+ messages in thread
From: Jan Beulich @ 2019-08-06 7:56 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, AndrewCooper, Ian Jackson, TimDeegan,
Julien Grall, xen-devel, Daniel De Graaf, Roger Pau Monné
On 05.08.2019 15:44, Marek Marczykowski-Górecki wrote:
> On Fri, Jul 19, 2019 at 09:43:26AM +0000, Jan Beulich wrote:
>> On 19.07.2019 11:02, Roger Pau Monné wrote:
>>> On Fri, Jul 19, 2019 at 08:04:45AM +0000, Jan Beulich wrote:
>>>> On 18.07.2019 18:52, Roger Pau Monné wrote:
>>>>> On Thu, Jul 18, 2019 at 03:17:27PM +0000, Jan Beulich wrote:
>>>>>> On 18.07.2019 15:46, Roger Pau Monné wrote:
>>>>>>> In fact I don't think INTx should be enabled when MSI(-X) is disabled,
>>>>>>> QEMU already traps writes to the command register, and it will manage
>>>>>>> INTx enabling/disabling by itself. I think the only check required is
>>>>>>> that MSI(-X) cannot be enabled if INTx is also enabled. In the same
>>>>>>> way both MSI caspabilities cannot be enabled simultaneously. The
>>>>>>> function should not explicitly disable any of the other capabilities,
>>>>>>> and just return -EBUSY if the caller attempts for example to enable
>>>>>>> MSI while INTx or MSI-X is enabled.
>>>>>>
>>>>>> You do realize that pci_intx() only ever gets called for Xen
>>>>>> internally used interrupts, i.e. mainly the serial console one?
>>>>>
>>>>> You will have to bear with me because I'm not sure I understand why
>>>>> it does matter. Do you mean to point out that dom0 is the one in full
>>>>> control of INTx, and thus Xen shouldn't care of whether INTx and
>>>>> MSI(-X) are enabled at the same time?
>>>>>
>>>>> I still think that at least a warning should be printed if a caller
>>>>> tries to enable MSI(-X) while INTx is also enabled, but unless there's
>>>>> a reason to have both MSI(-X) and INTx enabled at the same time (maybe
>>>>> a quirk for some hardware issue?) it shouldn't be allowed on this new
>>>>> interface.
>>>>
>>>> I don't mind improvements to the current situation (i.e. such a
>>>> warning may indeed make sense); I merely stated how things currently
>>>> are. INTx treatment was completely left aside when MSI support was
>>>> introduced into Xen.
>>>
>>> In order to give Marek a more concise reply, would you agree to return
>>> -EBUSY (or some error code) and print a warning message if the caller
>>> attempts to enable MSI(-X) while INTx is also enabled?
>>
>> As to returning an error - I think so, yes. I'm less sure about logging
>> a message.
>
> I'm trying to get it working and it isn't clear to me what should I
> check for "INTx is also enabled". I assumed PCI_COMMAND_INTX_DISABLE
> bit, but it looks like guest has no control over this bit, even in
> permissive mode. This means enabling MSI(-X) always fails because guest
> has no way to set PCI_COMMAND_INTX_DISABLE bit before.
Well, the guest has no control, but in order to enable MSI{,-X} I'd
have expected qemu or the Dom0 kernel to set this bit up front. If
that's not the case, then of course neither checking nor logging a
message is appropriate at this point in time. It may be worthwhile
calling out this anomaly then in the description.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
2019-08-06 7:56 ` Jan Beulich
@ 2019-08-06 9:46 ` Marek Marczykowski-Górecki
2019-08-06 10:33 ` Jan Beulich
0 siblings, 1 reply; 39+ messages in thread
From: Marek Marczykowski-Górecki @ 2019-08-06 9:46 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, AndrewCooper, Ian Jackson, TimDeegan,
Julien Grall, xen-devel, Daniel De Graaf, Roger Pau Monné
[-- Attachment #1.1: Type: text/plain, Size: 3742 bytes --]
On Tue, Aug 06, 2019 at 07:56:39AM +0000, Jan Beulich wrote:
> On 05.08.2019 15:44, Marek Marczykowski-Górecki wrote:
> > On Fri, Jul 19, 2019 at 09:43:26AM +0000, Jan Beulich wrote:
> >> On 19.07.2019 11:02, Roger Pau Monné wrote:
> >>> On Fri, Jul 19, 2019 at 08:04:45AM +0000, Jan Beulich wrote:
> >>>> On 18.07.2019 18:52, Roger Pau Monné wrote:
> >>>>> On Thu, Jul 18, 2019 at 03:17:27PM +0000, Jan Beulich wrote:
> >>>>>> On 18.07.2019 15:46, Roger Pau Monné wrote:
> >>>>>>> In fact I don't think INTx should be enabled when MSI(-X) is disabled,
> >>>>>>> QEMU already traps writes to the command register, and it will manage
> >>>>>>> INTx enabling/disabling by itself. I think the only check required is
> >>>>>>> that MSI(-X) cannot be enabled if INTx is also enabled. In the same
> >>>>>>> way both MSI caspabilities cannot be enabled simultaneously. The
> >>>>>>> function should not explicitly disable any of the other capabilities,
> >>>>>>> and just return -EBUSY if the caller attempts for example to enable
> >>>>>>> MSI while INTx or MSI-X is enabled.
> >>>>>>
> >>>>>> You do realize that pci_intx() only ever gets called for Xen
> >>>>>> internally used interrupts, i.e. mainly the serial console one?
> >>>>>
> >>>>> You will have to bear with me because I'm not sure I understand why
> >>>>> it does matter. Do you mean to point out that dom0 is the one in full
> >>>>> control of INTx, and thus Xen shouldn't care of whether INTx and
> >>>>> MSI(-X) are enabled at the same time?
> >>>>>
> >>>>> I still think that at least a warning should be printed if a caller
> >>>>> tries to enable MSI(-X) while INTx is also enabled, but unless there's
> >>>>> a reason to have both MSI(-X) and INTx enabled at the same time (maybe
> >>>>> a quirk for some hardware issue?) it shouldn't be allowed on this new
> >>>>> interface.
> >>>>
> >>>> I don't mind improvements to the current situation (i.e. such a
> >>>> warning may indeed make sense); I merely stated how things currently
> >>>> are. INTx treatment was completely left aside when MSI support was
> >>>> introduced into Xen.
> >>>
> >>> In order to give Marek a more concise reply, would you agree to return
> >>> -EBUSY (or some error code) and print a warning message if the caller
> >>> attempts to enable MSI(-X) while INTx is also enabled?
> >>
> >> As to returning an error - I think so, yes. I'm less sure about logging
> >> a message.
> >
> > I'm trying to get it working and it isn't clear to me what should I
> > check for "INTx is also enabled". I assumed PCI_COMMAND_INTX_DISABLE
> > bit, but it looks like guest has no control over this bit, even in
> > permissive mode. This means enabling MSI(-X) always fails because guest
> > has no way to set PCI_COMMAND_INTX_DISABLE bit before.
>
> Well, the guest has no control, but in order to enable MSI{,-X} I'd
> have expected qemu or the Dom0 kernel to set this bit up front.
qemu would do that, when running in dom0. But in PV stubdomain it talks
to pciback, which filters it out.
> If
> that's not the case, then of course neither checking nor logging a
> message is appropriate at this point in time. It may be worthwhile
> calling out this anomaly then in the description.
Ok, so I'll go back to setting PCI_COMMAND_INTX_DISABLE instead of just
verification.
Just to clarify: should I also clear PCI_COMMAND_INTX_DISABLE when
disabling MSI? Now I think yes, because nothing else would do that
otherwise, but I would like to double check.
--
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
2019-08-06 9:46 ` Marek Marczykowski-Górecki
@ 2019-08-06 10:33 ` Jan Beulich
2019-08-06 10:53 ` Marek Marczykowski-Górecki
0 siblings, 1 reply; 39+ messages in thread
From: Jan Beulich @ 2019-08-06 10:33 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, AndrewCooper, Ian Jackson, TimDeegan,
Julien Grall, xen-devel, Daniel De Graaf, Roger Pau Monné
On 06.08.2019 11:46, Marek Marczykowski-Górecki wrote:
> On Tue, Aug 06, 2019 at 07:56:39AM +0000, Jan Beulich wrote:
>> On 05.08.2019 15:44, Marek Marczykowski-Górecki wrote:
>>> On Fri, Jul 19, 2019 at 09:43:26AM +0000, Jan Beulich wrote:
>>>> On 19.07.2019 11:02, Roger Pau Monné wrote:
>>>>> On Fri, Jul 19, 2019 at 08:04:45AM +0000, Jan Beulich wrote:
>>>>>> On 18.07.2019 18:52, Roger Pau Monné wrote:
>>>>>>> On Thu, Jul 18, 2019 at 03:17:27PM +0000, Jan Beulich wrote:
>>>>>>>> On 18.07.2019 15:46, Roger Pau Monné wrote:
>>>>>>>>> In fact I don't think INTx should be enabled when MSI(-X) is disabled,
>>>>>>>>> QEMU already traps writes to the command register, and it will manage
>>>>>>>>> INTx enabling/disabling by itself. I think the only check required is
>>>>>>>>> that MSI(-X) cannot be enabled if INTx is also enabled. In the same
>>>>>>>>> way both MSI caspabilities cannot be enabled simultaneously. The
>>>>>>>>> function should not explicitly disable any of the other capabilities,
>>>>>>>>> and just return -EBUSY if the caller attempts for example to enable
>>>>>>>>> MSI while INTx or MSI-X is enabled.
>>>>>>>>
>>>>>>>> You do realize that pci_intx() only ever gets called for Xen
>>>>>>>> internally used interrupts, i.e. mainly the serial console one?
>>>>>>>
>>>>>>> You will have to bear with me because I'm not sure I understand why
>>>>>>> it does matter. Do you mean to point out that dom0 is the one in full
>>>>>>> control of INTx, and thus Xen shouldn't care of whether INTx and
>>>>>>> MSI(-X) are enabled at the same time?
>>>>>>>
>>>>>>> I still think that at least a warning should be printed if a caller
>>>>>>> tries to enable MSI(-X) while INTx is also enabled, but unless there's
>>>>>>> a reason to have both MSI(-X) and INTx enabled at the same time (maybe
>>>>>>> a quirk for some hardware issue?) it shouldn't be allowed on this new
>>>>>>> interface.
>>>>>>
>>>>>> I don't mind improvements to the current situation (i.e. such a
>>>>>> warning may indeed make sense); I merely stated how things currently
>>>>>> are. INTx treatment was completely left aside when MSI support was
>>>>>> introduced into Xen.
>>>>>
>>>>> In order to give Marek a more concise reply, would you agree to return
>>>>> -EBUSY (or some error code) and print a warning message if the caller
>>>>> attempts to enable MSI(-X) while INTx is also enabled?
>>>>
>>>> As to returning an error - I think so, yes. I'm less sure about logging
>>>> a message.
>>>
>>> I'm trying to get it working and it isn't clear to me what should I
>>> check for "INTx is also enabled". I assumed PCI_COMMAND_INTX_DISABLE
>>> bit, but it looks like guest has no control over this bit, even in
>>> permissive mode. This means enabling MSI(-X) always fails because guest
>>> has no way to set PCI_COMMAND_INTX_DISABLE bit before.
>>
>> Well, the guest has no control, but in order to enable MSI{,-X} I'd
>> have expected qemu or the Dom0 kernel to set this bit up front.
>
> qemu would do that, when running in dom0. But in PV stubdomain it talks
> to pciback, which filters it out.
Filtering out the guest attempt is fine, but it should then set the
bit while preparing MSI/MSI-X for the guest. (I'm not sure it would
need to do so explicitly, or whether it couldn't rely on code
elsewhere in the kernel doing so.)
>> If
>> that's not the case, then of course neither checking nor logging a
>> message is appropriate at this point in time. It may be worthwhile
>> calling out this anomaly then in the description.
>
> Ok, so I'll go back to setting PCI_COMMAND_INTX_DISABLE instead of just
> verification.
>
> Just to clarify: should I also clear PCI_COMMAND_INTX_DISABLE when
> disabling MSI? Now I think yes, because nothing else would do that
> otherwise, but I would like to double check.
Well, I think I've made my position clear: So far we use pci_intx()
only on devices used by Xen itself. I think this should remain to be
that way. Devices in possession of domains (including Dom0) should
be under Dom0's control in this regard.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
2019-08-06 10:33 ` Jan Beulich
@ 2019-08-06 10:53 ` Marek Marczykowski-Górecki
2019-08-06 12:05 ` Jan Beulich
0 siblings, 1 reply; 39+ messages in thread
From: Marek Marczykowski-Górecki @ 2019-08-06 10:53 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, AndrewCooper, Ian Jackson, TimDeegan,
Julien Grall, xen-devel, Daniel De Graaf, Roger Pau Monné
[-- Attachment #1.1: Type: text/plain, Size: 4563 bytes --]
On Tue, Aug 06, 2019 at 12:33:39PM +0200, Jan Beulich wrote:
> On 06.08.2019 11:46, Marek Marczykowski-Górecki wrote:
> > On Tue, Aug 06, 2019 at 07:56:39AM +0000, Jan Beulich wrote:
> > > On 05.08.2019 15:44, Marek Marczykowski-Górecki wrote:
> > > > On Fri, Jul 19, 2019 at 09:43:26AM +0000, Jan Beulich wrote:
> > > > > On 19.07.2019 11:02, Roger Pau Monné wrote:
> > > > > > On Fri, Jul 19, 2019 at 08:04:45AM +0000, Jan Beulich wrote:
> > > > > > > On 18.07.2019 18:52, Roger Pau Monné wrote:
> > > > > > > > On Thu, Jul 18, 2019 at 03:17:27PM +0000, Jan Beulich wrote:
> > > > > > > > > On 18.07.2019 15:46, Roger Pau Monné wrote:
> > > > > > > > > > In fact I don't think INTx should be enabled when MSI(-X) is disabled,
> > > > > > > > > > QEMU already traps writes to the command register, and it will manage
> > > > > > > > > > INTx enabling/disabling by itself. I think the only check required is
> > > > > > > > > > that MSI(-X) cannot be enabled if INTx is also enabled. In the same
> > > > > > > > > > way both MSI caspabilities cannot be enabled simultaneously. The
> > > > > > > > > > function should not explicitly disable any of the other capabilities,
> > > > > > > > > > and just return -EBUSY if the caller attempts for example to enable
> > > > > > > > > > MSI while INTx or MSI-X is enabled.
> > > > > > > > >
> > > > > > > > > You do realize that pci_intx() only ever gets called for Xen
> > > > > > > > > internally used interrupts, i.e. mainly the serial console one?
> > > > > > > >
> > > > > > > > You will have to bear with me because I'm not sure I understand why
> > > > > > > > it does matter. Do you mean to point out that dom0 is the one in full
> > > > > > > > control of INTx, and thus Xen shouldn't care of whether INTx and
> > > > > > > > MSI(-X) are enabled at the same time?
> > > > > > > >
> > > > > > > > I still think that at least a warning should be printed if a caller
> > > > > > > > tries to enable MSI(-X) while INTx is also enabled, but unless there's
> > > > > > > > a reason to have both MSI(-X) and INTx enabled at the same time (maybe
> > > > > > > > a quirk for some hardware issue?) it shouldn't be allowed on this new
> > > > > > > > interface.
> > > > > > >
> > > > > > > I don't mind improvements to the current situation (i.e. such a
> > > > > > > warning may indeed make sense); I merely stated how things currently
> > > > > > > are. INTx treatment was completely left aside when MSI support was
> > > > > > > introduced into Xen.
> > > > > >
> > > > > > In order to give Marek a more concise reply, would you agree to return
> > > > > > -EBUSY (or some error code) and print a warning message if the caller
> > > > > > attempts to enable MSI(-X) while INTx is also enabled?
> > > > >
> > > > > As to returning an error - I think so, yes. I'm less sure about logging
> > > > > a message.
> > > >
> > > > I'm trying to get it working and it isn't clear to me what should I
> > > > check for "INTx is also enabled". I assumed PCI_COMMAND_INTX_DISABLE
> > > > bit, but it looks like guest has no control over this bit, even in
> > > > permissive mode. This means enabling MSI(-X) always fails because guest
> > > > has no way to set PCI_COMMAND_INTX_DISABLE bit before.
> > >
> > > Well, the guest has no control, but in order to enable MSI{,-X} I'd
> > > have expected qemu or the Dom0 kernel to set this bit up front.
> >
> > qemu would do that, when running in dom0. But in PV stubdomain it talks
> > to pciback, which filters it out.
>
> Filtering out the guest attempt is fine, but it should then set the
> bit while preparing MSI/MSI-X for the guest. (I'm not sure it would
> need to do so explicitly, or whether it couldn't rely on code
> elsewhere in the kernel doing so.)
...
> Well, I think I've made my position clear: So far we use pci_intx()
> only on devices used by Xen itself. I think this should remain to be
> that way. Devices in possession of domains (including Dom0) should
> be under Dom0's control in this regard.
The thing is, in case of using stubdomain, it's mostly stubdomain
handling it. Especially all the config space interception and applying
logic to it is done by qemu in stubdomain. Do you suggest duplicating /
moving that part to dom0 instead? What would be the point for stubdomain
then?
--
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
2019-08-06 10:53 ` Marek Marczykowski-Górecki
@ 2019-08-06 12:05 ` Jan Beulich
2019-08-06 12:43 ` Marek Marczykowski-Górecki
0 siblings, 1 reply; 39+ messages in thread
From: Jan Beulich @ 2019-08-06 12:05 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, AndrewCooper, Ian Jackson, TimDeegan,
Julien Grall, xen-devel, Daniel De Graaf, Roger Pau Monné
On 06.08.2019 12:53, Marek Marczykowski-Górecki wrote:
> On Tue, Aug 06, 2019 at 12:33:39PM +0200, Jan Beulich wrote:
>> On 06.08.2019 11:46, Marek Marczykowski-Górecki wrote:
>>> On Tue, Aug 06, 2019 at 07:56:39AM +0000, Jan Beulich wrote:
>>>> On 05.08.2019 15:44, Marek Marczykowski-Górecki wrote:
>>>>> I'm trying to get it working and it isn't clear to me what should I
>>>>> check for "INTx is also enabled". I assumed PCI_COMMAND_INTX_DISABLE
>>>>> bit, but it looks like guest has no control over this bit, even in
>>>>> permissive mode. This means enabling MSI(-X) always fails because guest
>>>>> has no way to set PCI_COMMAND_INTX_DISABLE bit before.
>>>>
>>>> Well, the guest has no control, but in order to enable MSI{,-X} I'd
>>>> have expected qemu or the Dom0 kernel to set this bit up front.
>>>
>>> qemu would do that, when running in dom0. But in PV stubdomain it talks
>>> to pciback, which filters it out.
>>
>> Filtering out the guest attempt is fine, but it should then set the
>> bit while preparing MSI/MSI-X for the guest. (I'm not sure it would
>> need to do so explicitly, or whether it couldn't rely on code
>> elsewhere in the kernel doing so.)
> ...
>> Well, I think I've made my position clear: So far we use pci_intx()
>> only on devices used by Xen itself. I think this should remain to be
>> that way. Devices in possession of domains (including Dom0) should
>> be under Dom0's control in this regard.
>
> The thing is, in case of using stubdomain, it's mostly stubdomain
> handling it. Especially all the config space interception and applying
> logic to it is done by qemu in stubdomain. Do you suggest duplicating /
> moving that part to dom0 instead? What would be the point for stubdomain
> then?
Nothing should be moved between components if possible (and if placed
sensibly). But isn't stubdom (being a PV DomU) relying on pciback in
Dom0 anyway? And hence doesn't the flow of events include
pci_enable_msi{,x}() invoked by pciback? I'd have expected that to
(also) take care of INTx.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
2019-08-06 12:05 ` Jan Beulich
@ 2019-08-06 12:43 ` Marek Marczykowski-Górecki
2019-08-06 13:19 ` Jan Beulich
2019-08-06 13:41 ` Roger Pau Monné
0 siblings, 2 replies; 39+ messages in thread
From: Marek Marczykowski-Górecki @ 2019-08-06 12:43 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, AndrewCooper, Ian Jackson, TimDeegan,
Julien Grall, xen-devel, Daniel De Graaf, Roger Pau Monné
[-- Attachment #1.1: Type: text/plain, Size: 3259 bytes --]
On Tue, Aug 06, 2019 at 02:05:48PM +0200, Jan Beulich wrote:
> On 06.08.2019 12:53, Marek Marczykowski-Górecki wrote:
> > On Tue, Aug 06, 2019 at 12:33:39PM +0200, Jan Beulich wrote:
> > > On 06.08.2019 11:46, Marek Marczykowski-Górecki wrote:
> > > > On Tue, Aug 06, 2019 at 07:56:39AM +0000, Jan Beulich wrote:
> > > > > On 05.08.2019 15:44, Marek Marczykowski-Górecki wrote:
> > > > > > I'm trying to get it working and it isn't clear to me what should I
> > > > > > check for "INTx is also enabled". I assumed PCI_COMMAND_INTX_DISABLE
> > > > > > bit, but it looks like guest has no control over this bit, even in
> > > > > > permissive mode. This means enabling MSI(-X) always fails because guest
> > > > > > has no way to set PCI_COMMAND_INTX_DISABLE bit before.
> > > > >
> > > > > Well, the guest has no control, but in order to enable MSI{,-X} I'd
> > > > > have expected qemu or the Dom0 kernel to set this bit up front.
> > > >
> > > > qemu would do that, when running in dom0. But in PV stubdomain it talks
> > > > to pciback, which filters it out.
> > >
> > > Filtering out the guest attempt is fine, but it should then set the
> > > bit while preparing MSI/MSI-X for the guest. (I'm not sure it would
> > > need to do so explicitly, or whether it couldn't rely on code
> > > elsewhere in the kernel doing so.)
> > ...
> > > Well, I think I've made my position clear: So far we use pci_intx()
> > > only on devices used by Xen itself. I think this should remain to be
> > > that way. Devices in possession of domains (including Dom0) should
> > > be under Dom0's control in this regard.
> >
> > The thing is, in case of using stubdomain, it's mostly stubdomain
> > handling it. Especially all the config space interception and applying
> > logic to it is done by qemu in stubdomain. Do you suggest duplicating /
> > moving that part to dom0 instead? What would be the point for stubdomain
> > then?
>
> Nothing should be moved between components if possible (and if placed
> sensibly). But isn't stubdom (being a PV DomU) relying on pciback in
> Dom0 anyway? And hence doesn't the flow of events include
> pci_enable_msi{,x}() invoked by pciback? I'd have expected that to
> (also) take care of INTx.
This was discussed in v2 of this series[1], where you also responded.
Relevant part of Roger's email:
Oh great, that's unfortunate. Both pciback functions end up calling
into msi_capability_init in the Linux kernel, which does indeed more
than just toggling the PCI config space enable bit.
OTOH adding a bypass to pciback so the stubdom is able to write to the
PCI register in order to toggle the enable bit seems quite clumsy. Not
to mention that you would be required to update Dom0 kernel in order to
fix the issue.
Do you think it makes sense to add a domctl to enable/disable MSI(X)?
This way the bug could be fixed by just updating Xen (and the
stubdomain).
[1] https://lists.xenproject.org/archives/html/xen-devel/2019-01/msg01271.html
--
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
2019-08-06 12:43 ` Marek Marczykowski-Górecki
@ 2019-08-06 13:19 ` Jan Beulich
2019-08-06 13:41 ` Roger Pau Monné
1 sibling, 0 replies; 39+ messages in thread
From: Jan Beulich @ 2019-08-06 13:19 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, AndrewCooper, Ian Jackson, TimDeegan,
Julien Grall, xen-devel, Daniel De Graaf, Roger Pau Monné
On 06.08.2019 14:43, Marek Marczykowski-Górecki wrote:
> On Tue, Aug 06, 2019 at 02:05:48PM +0200, Jan Beulich wrote:
>> On 06.08.2019 12:53, Marek Marczykowski-Górecki wrote:
>>> On Tue, Aug 06, 2019 at 12:33:39PM +0200, Jan Beulich wrote:
>>>> On 06.08.2019 11:46, Marek Marczykowski-Górecki wrote:
>>>>> On Tue, Aug 06, 2019 at 07:56:39AM +0000, Jan Beulich wrote:
>>>>>> On 05.08.2019 15:44, Marek Marczykowski-Górecki wrote:
>>>>>>> I'm trying to get it working and it isn't clear to me what should I
>>>>>>> check for "INTx is also enabled". I assumed PCI_COMMAND_INTX_DISABLE
>>>>>>> bit, but it looks like guest has no control over this bit, even in
>>>>>>> permissive mode. This means enabling MSI(-X) always fails because guest
>>>>>>> has no way to set PCI_COMMAND_INTX_DISABLE bit before.
>>>>>>
>>>>>> Well, the guest has no control, but in order to enable MSI{,-X} I'd
>>>>>> have expected qemu or the Dom0 kernel to set this bit up front.
>>>>>
>>>>> qemu would do that, when running in dom0. But in PV stubdomain it talks
>>>>> to pciback, which filters it out.
>>>>
>>>> Filtering out the guest attempt is fine, but it should then set the
>>>> bit while preparing MSI/MSI-X for the guest. (I'm not sure it would
>>>> need to do so explicitly, or whether it couldn't rely on code
>>>> elsewhere in the kernel doing so.)
>>> ...
>>>> Well, I think I've made my position clear: So far we use pci_intx()
>>>> only on devices used by Xen itself. I think this should remain to be
>>>> that way. Devices in possession of domains (including Dom0) should
>>>> be under Dom0's control in this regard.
>>>
>>> The thing is, in case of using stubdomain, it's mostly stubdomain
>>> handling it. Especially all the config space interception and applying
>>> logic to it is done by qemu in stubdomain. Do you suggest duplicating /
>>> moving that part to dom0 instead? What would be the point for stubdomain
>>> then?
>>
>> Nothing should be moved between components if possible (and if placed
>> sensibly). But isn't stubdom (being a PV DomU) relying on pciback in
>> Dom0 anyway? And hence doesn't the flow of events include
>> pci_enable_msi{,x}() invoked by pciback? I'd have expected that to
>> (also) take care of INTx.
>
> This was discussed in v2 of this series[1], where you also responded.
> Relevant part of Roger's email:
>
> Oh great, that's unfortunate. Both pciback functions end up calling
> into msi_capability_init in the Linux kernel, which does indeed more
> than just toggling the PCI config space enable bit.
>
> OTOH adding a bypass to pciback so the stubdom is able to write to the
> PCI register in order to toggle the enable bit seems quite clumsy. Not
> to mention that you would be required to update Dom0 kernel in order to
> fix the issue.
>
> Do you think it makes sense to add a domctl to enable/disable MSI(X)?
>
> This way the bug could be fixed by just updating Xen (and the
> stubdomain).
>
> [1] https://lists.xenproject.org/archives/html/xen-devel/2019-01/msg01271.html
Hmm, right. But us having closed a circle this way (as it seems)
suggests to me that bypassing Dom0 may not have been the right
idea here after all (i.e. is, as Roger said, clumsy). Also may I
ask whether the quoted there "so this most likely won’t work
correctly" has actually been proven to not work, and not be
possible to make work?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control
2019-08-06 12:43 ` Marek Marczykowski-Górecki
2019-08-06 13:19 ` Jan Beulich
@ 2019-08-06 13:41 ` Roger Pau Monné
1 sibling, 0 replies; 39+ messages in thread
From: Roger Pau Monné @ 2019-08-06 13:41 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, AndrewCooper, Ian Jackson, TimDeegan,
Julien Grall, Jan Beulich, xen-devel, Daniel De Graaf
On Tue, Aug 06, 2019 at 02:43:49PM +0200, Marek Marczykowski-Górecki wrote:
> On Tue, Aug 06, 2019 at 02:05:48PM +0200, Jan Beulich wrote:
> > On 06.08.2019 12:53, Marek Marczykowski-Górecki wrote:
> > > On Tue, Aug 06, 2019 at 12:33:39PM +0200, Jan Beulich wrote:
> > > > On 06.08.2019 11:46, Marek Marczykowski-Górecki wrote:
> > > > > On Tue, Aug 06, 2019 at 07:56:39AM +0000, Jan Beulich wrote:
> > > > > > On 05.08.2019 15:44, Marek Marczykowski-Górecki wrote:
> > > > > > > I'm trying to get it working and it isn't clear to me what should I
> > > > > > > check for "INTx is also enabled". I assumed PCI_COMMAND_INTX_DISABLE
> > > > > > > bit, but it looks like guest has no control over this bit, even in
> > > > > > > permissive mode. This means enabling MSI(-X) always fails because guest
> > > > > > > has no way to set PCI_COMMAND_INTX_DISABLE bit before.
> > > > > >
> > > > > > Well, the guest has no control, but in order to enable MSI{,-X} I'd
> > > > > > have expected qemu or the Dom0 kernel to set this bit up front.
> > > > >
> > > > > qemu would do that, when running in dom0. But in PV stubdomain it talks
> > > > > to pciback, which filters it out.
> > > >
> > > > Filtering out the guest attempt is fine, but it should then set the
> > > > bit while preparing MSI/MSI-X for the guest. (I'm not sure it would
> > > > need to do so explicitly, or whether it couldn't rely on code
> > > > elsewhere in the kernel doing so.)
> > > ...
> > > > Well, I think I've made my position clear: So far we use pci_intx()
> > > > only on devices used by Xen itself. I think this should remain to be
> > > > that way. Devices in possession of domains (including Dom0) should
> > > > be under Dom0's control in this regard.
> > >
> > > The thing is, in case of using stubdomain, it's mostly stubdomain
> > > handling it. Especially all the config space interception and applying
> > > logic to it is done by qemu in stubdomain. Do you suggest duplicating /
> > > moving that part to dom0 instead? What would be the point for stubdomain
> > > then?
> >
> > Nothing should be moved between components if possible (and if placed
> > sensibly). But isn't stubdom (being a PV DomU) relying on pciback in
> > Dom0 anyway? And hence doesn't the flow of events include
> > pci_enable_msi{,x}() invoked by pciback? I'd have expected that to
> > (also) take care of INTx.
>
> This was discussed in v2 of this series[1], where you also responded.
> Relevant part of Roger's email:
>
> Oh great, that's unfortunate. Both pciback functions end up calling
> into msi_capability_init in the Linux kernel, which does indeed more
> than just toggling the PCI config space enable bit.
>
> OTOH adding a bypass to pciback so the stubdom is able to write to the
> PCI register in order to toggle the enable bit seems quite clumsy. Not
> to mention that you would be required to update Dom0 kernel in order to
> fix the issue.
>
> Do you think it makes sense to add a domctl to enable/disable MSI(X)?
>
> This way the bug could be fixed by just updating Xen (and the
> stubdomain).
>
> [1] https://lists.xenproject.org/archives/html/xen-devel/2019-01/msg01271.html
The main problem here is that PCI passthrough for PV is completely
different than passthrough for HVM, and hence trying to use the PV
passthrough interface as a proxy to the HVM interface (and
implementation in QEMU) is likely to find issues as we have seen.
I still think that modifying pciback is not the right call, as that
is just adding bodges to the pciif PV interface to suit a non-PV
use-case, which pciback/pcifront wasn't designed to handle (as likely
no one thought of handling passthrough in a stubdomain).
I still think that adding these missing pieces to an hypercall is
likely the best solution here, but we need to know beforehand whatever
the studomain needs to do so that it can be put inside of a single
hypercall if possible. So far this is: modify the MSI/MSI-X control
bit and the INTx bit in the command register?
In which case the hypercall should maybe be named
PHYSDEVOP_interrupt_control or some such?
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2019-08-06 13:42 UTC | newest]
Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-17 1:00 [Xen-devel] [PATCH v5 0/6] Fix PCI passthrough for HVM with stubdomain Marek Marczykowski-Górecki
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 1/6] libxl: do not attach xen-pciback to HVM domain, if stubdomain is in use Marek Marczykowski-Górecki
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 2/6] libxl: attach PCI device to qemu only after setting pciback/pcifront Marek Marczykowski-Górecki
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 3/6] libxl: don't try to manipulate json config for stubdomain Marek Marczykowski-Górecki
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 4/6] xen/x86: Allow stubdom access to irq created for msi Marek Marczykowski-Górecki
2019-07-17 9:54 ` Roger Pau Monné
2019-07-17 15:09 ` Marek Marczykowski-Górecki
2019-07-18 9:29 ` Roger Pau Monné
2019-07-18 15:12 ` Marek Marczykowski-Górecki
2019-07-18 16:55 ` Roger Pau Monné
2019-07-20 16:48 ` Julien Grall
2019-07-20 21:21 ` Marek Marczykowski-Górecki
2019-07-21 18:05 ` Julien Grall
2019-07-22 8:45 ` Roger Pau Monné
2019-07-22 9:06 ` Julien Grall
2019-07-22 9:16 ` Julien Grall
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 5/6] xen/x86: add PHYSDEVOP_msi_control Marek Marczykowski-Górecki
2019-07-17 10:18 ` Roger Pau Monné
2019-07-17 23:54 ` Marek Marczykowski-Górecki
2019-07-18 13:46 ` Roger Pau Monné
2019-07-18 15:17 ` Jan Beulich
2019-07-18 16:52 ` Roger Pau Monné
2019-07-19 8:04 ` Jan Beulich
2019-07-19 9:02 ` Roger Pau Monné
2019-07-19 9:43 ` Jan Beulich
2019-08-05 13:44 ` Marek Marczykowski-Górecki
2019-08-06 7:56 ` Jan Beulich
2019-08-06 9:46 ` Marek Marczykowski-Górecki
2019-08-06 10:33 ` Jan Beulich
2019-08-06 10:53 ` Marek Marczykowski-Górecki
2019-08-06 12:05 ` Jan Beulich
2019-08-06 12:43 ` Marek Marczykowski-Górecki
2019-08-06 13:19 ` Jan Beulich
2019-08-06 13:41 ` Roger Pau Monné
2019-07-19 10:40 ` Jan Beulich
2019-07-17 1:00 ` [Xen-devel] [PATCH v5 6/6] tools/libxc: add wrapper for PHYSDEVOP_msi_control Marek Marczykowski-Górecki
2019-07-17 10:21 ` Roger Pau Monné
2019-07-18 0:12 ` Marek Marczykowski-Górecki
2019-07-18 13:53 ` Roger Pau Monné
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).