* [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough
@ 2015-04-09 15:09 Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 01/19] xen/arm: Let the toolstack configure the number of SPIs Julien Grall
` (18 more replies)
0 siblings, 19 replies; 48+ messages in thread
From: Julien Grall @ 2015-04-09 15:09 UTC (permalink / raw)
To: xen-devel
Cc: ian.campbell, manish.jaggi, tim, robert.vanvossen, Julien Grall,
stefano.stabellini, suravee.suthikulpanit, Josh.Whitehead,
andrii.tseglytskyi
Hello all,
This is the fifth version of this patch series to add support for platform
device passthrough on ARM.
In order to passthrough a non-PCI device, the user will have to:
- Map manually MMIO/IRQ
- Describe the device in the newly partial device tree support
- Specify the list of device protected by an IOMMU to assign to the
guest.
While this solution is primitive, this is allow us to support more complex
device in Xen with an little additionnal work for the user. Attempting to
do it automatically is more difficult because we may not know the dependencies
between devices (for instance a Network card and a phy).
To avoid adding code in DOM0 to manage platform device deassignment, the
user has to add the property "xen,passthrough" to the device tree node
describing the device. This can be easily done via U-Boot. For instance,
if we want to passthrough the second network card of a Midway server to the
guest. The user will have to add the following line the u-boot script:
fdt set /soc/ethernet@fff51000 xen,passthrough
This series has been tested on Midway by assigning the secondary network card
to a guest (see instruction below). Though, it requires a separate patch as
we decide to not support the Midway SMMU within the new drivers.
I plan to do futher testing on other boards.
A working tree can be found here:
git://xenbits.xen.org/julieng/xen-unstable.git branch passthrough-v5.2
Major changes in v5:
- Series divided in 2 parts. The first part [6] has been pushed to
Xen upstream
- Add more documentation
- Modify XSM check for bind_pt_irq on ARM
Major changes in v4:
- The partial device tree option can only be used with trusted
device tree
- Add more documentation
- Use DOMCTL_bind_pt_irq rather than PHYSDEVOP_map_pirq
- Add XSM support for non-PCI passthrough
Major changes in v3:
- Rework the approach to passthrough a device (xen,passthrough +
partial device tree).
- Extend the existing hypercalls to assign/deassign device rather than
adding new one.
- Merge series [4] and [5] in this serie.
Major changes in v2:
- Drop the patch #1 of the previous version
- Virtual IRQ are not anymore equal to the physical interrupt
- Move the hypercall to get DT informations for privcmd to domctl
- Split the domain creation in 2 two parts to allow per guest
VGIC configuration (such as the number of SPIs).
- Bunch of typoes, commit improvement, function renaming.
For all changes see in each patch.
Sincerely yours,
[1] http://lists.xen.org/archives/html/xen-devel/2014-07/msg04090.html
[2] http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg01386.html
[3] http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg01612.html
[4] http://lists.xen.org/archives/html/xen-devel/2014-11/msg01672.html
[5] http://lists.xenproject.org/archives/html/xen-devel/2014-07/msg02098.html
[6] http://lists.xenproject.org/archives/html/xen-devel/2015-04/msg00248.html
Cc: manish.jaggi@caviumnetworks.com
Cc: suravee.suthikulpanit@amd.com
Cc: andrii.tseglytskyi@globallogic.com
Cc: robert.vanvossen@dornerworks.com
Cc: Josh.Whitehead@dornerworks.com
Julien Grall (19):
xen/arm: Let the toolstack configure the number of SPIs
xen/arm: vgic: Add spi_to_pending
xen/arm: Release IRQ routed to a domain when it's destroying
xen/arm: Implement hypercall DOMCTL_{,un}bind_pt_pirq
xen: guestcopy: Provide an helper to safely copy string from guest
xen/dts: Provide an helper to get a DT node from a path provided by a
guest
xen/passthrough: Introduce iommu_construct
xen/passthrough: arm: release the DT devices assigned to a guest
earlier
xen/passthrough: iommu_deassign_device_dt: By default reassign device
to nobody
xen/iommu: arm: Wire iommu DOMCTL for ARM
xen/xsm: Add helpers to check permission for device tree passthrough
xen/passthrough: Extend XEN_DOMCTL_*assign_device to support DT device
tools/libxl: Create a per-arch function to map IRQ to a domain
tools/libxl: Check if fdt_{first,next}_subnode are present in libfdt
tools/(lib)xl: Add partial device tree support for ARM
tools/libxl: arm: Use an higher value for the GIC phandle
libxl: Add support for Device Tree passthrough
xl: Add new option dtdev
docs/misc: arm: Add documentation about Device Tree passthrough
docs/man/xl.cfg.pod.5 | 15 +++
docs/misc/arm/passthrough.txt | 61 +++++++++++
tools/config.h.in | 6 ++
tools/configure.ac | 5 +
tools/libxc/include/xenctrl.h | 18 ++++
tools/libxc/xc_domain.c | 170 +++++++++++++++++++++++++++---
tools/libxl/Makefile | 2 +-
tools/libxl/libxl.h | 6 ++
tools/libxl/libxl_arch.h | 4 +
tools/libxl/libxl_arm.c | 192 +++++++++++++++++++++++++++++++++-
tools/libxl/libxl_create.c | 38 ++++++-
tools/libxl/libxl_internal.h | 5 +
tools/libxl/libxl_libfdt_compat.c | 92 ++++++++++++++++
tools/libxl/libxl_types.idl | 9 ++
tools/libxl/libxl_x86.c | 13 +++
tools/libxl/xl_cmdimpl.c | 23 +++-
xen/arch/arm/domain.c | 6 +-
xen/arch/arm/domctl.c | 88 +++++++++++++++-
xen/arch/arm/gic.c | 45 ++++++++
xen/arch/arm/irq.c | 46 ++++++++
xen/arch/arm/setup.c | 1 +
xen/arch/arm/vgic.c | 34 +++++-
xen/common/Makefile | 1 +
xen/common/device_tree.c | 18 ++++
xen/common/guestcopy.c | 31 ++++++
xen/drivers/passthrough/arm/iommu.c | 7 +-
xen/drivers/passthrough/arm/smmu.c | 8 +-
xen/drivers/passthrough/device_tree.c | 136 ++++++++++++++++++++++--
xen/drivers/passthrough/iommu.c | 33 +++++-
xen/drivers/passthrough/pci.c | 69 ++++++------
xen/include/asm-arm/gic.h | 4 +
xen/include/asm-arm/irq.h | 2 +
xen/include/asm-arm/vgic.h | 3 +-
xen/include/public/arch-arm.h | 2 +
xen/include/public/domctl.h | 28 ++++-
xen/include/xen/device_tree.h | 14 +++
xen/include/xen/guest_access.h | 5 +
xen/include/xen/iommu.h | 7 +-
xen/include/xsm/dummy.h | 47 ++++++---
xen/include/xsm/xsm.h | 55 +++++++---
xen/xsm/dummy.c | 10 +-
xen/xsm/flask/avc.c | 3 +
xen/xsm/flask/flask_op.c | 49 +++------
xen/xsm/flask/hooks.c | 139 +++++++++++++++++-------
xen/xsm/flask/include/avc.h | 2 +
xen/xsm/flask/policy/access_vectors | 2 +-
46 files changed, 1373 insertions(+), 181 deletions(-)
create mode 100644 docs/misc/arm/passthrough.txt
create mode 100644 tools/libxl/libxl_libfdt_compat.c
create mode 100644 xen/common/guestcopy.c
--
2.1.4
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v5 p2 01/19] xen/arm: Let the toolstack configure the number of SPIs
2015-04-09 15:09 [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough Julien Grall
@ 2015-04-09 15:09 ` Julien Grall
2015-04-16 14:39 ` Ian Campbell
2015-04-09 15:09 ` [PATCH v5 p2 02/19] xen/arm: vgic: Add spi_to_pending Julien Grall
` (17 subsequent siblings)
18 siblings, 1 reply; 48+ messages in thread
From: Julien Grall @ 2015-04-09 15:09 UTC (permalink / raw)
To: xen-devel
Cc: Wei Liu, ian.campbell, tim, Julien Grall, Ian Jackson,
stefano.stabellini, Jan Beulich
From: Julien Grall <julien.grall@linaro.org>
Each domain may have a different number of IRQs depending on the devices
assigned to it.
Rather re-using the number of IRQs used by the hardwared GIC, let the
toolstack specify the number of SPIs when the domain is created. This
will avoid to waste memory.
To calculate the number of SPIs, we take advantage of the fact that the
libxl interface can only expose 1:1 mapping and look for the largest SPI
in the list.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Changes in v5:
- Remove stray change
- Limit the number of SPIs to (1020 - 32) rather than the number
supported by the hardware
Changes in v4:
- Check the number of SPIs supported by the virtual GIC against
the number supported by the hardware GIC.
- Use uint32_t rather than int in the toolstack code.
- Initialize spi after the check in the toolstack code
- Typoes
Changes in v3:
- Fix typoes
- A separate has been created to extend the DOMCTL create domain
Changes in v2:
- Patch added
---
tools/libxc/xc_domain.c | 1 +
tools/libxl/libxl_arm.c | 21 +++++++++++++++++++++
xen/arch/arm/domain.c | 2 +-
xen/arch/arm/setup.c | 1 +
xen/arch/arm/vgic.c | 11 ++++++-----
xen/include/asm-arm/vgic.h | 2 +-
xen/include/public/arch-arm.h | 2 ++
7 files changed, 33 insertions(+), 7 deletions(-)
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 95e3098..1f2c80c 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -67,6 +67,7 @@ int xc_domain_create(xc_interface *xch,
/* No arch-specific configuration for now */
#elif defined (__arm__) || defined(__aarch64__)
config.gic_version = XEN_DOMCTL_CONFIG_GIC_DEFAULT;
+ config.nr_spis = 0;
#else
errno = ENOSYS;
return -1;
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 946618c..5a5cb3f 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -39,6 +39,27 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
libxl_domain_config *d_config,
xc_domain_configuration_t *xc_config)
{
+ uint32_t nr_spis = 0;
+ unsigned int i;
+
+ for (i = 0; i < d_config->b_info.num_irqs; i++) {
+ uint32_t irq = d_config->b_info.irqs[i];
+ uint32_t spi;
+
+ if (irq < 32)
+ continue;
+
+ spi = irq - 32;
+
+ if (nr_spis <= spi)
+ nr_spis = spi + 1;
+ }
+
+ LOG(DEBUG, "Configure the domain");
+
+ xc_config->nr_spis = nr_spis;
+ LOG(DEBUG, " - Allocate %u SPIs", nr_spis);
+
xc_config->gic_version = XEN_DOMCTL_CONFIG_GIC_DEFAULT;
return 0;
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index e4d6fc8..180bccc 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -590,7 +590,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
if ( (rc = gicv_setup(d)) != 0 )
goto fail;
- if ( (rc = domain_vgic_init(d)) != 0 )
+ if ( (rc = domain_vgic_init(d, config->nr_spis)) != 0 )
goto fail;
if ( (rc = domain_vtimer_init(d)) != 0 )
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 4ec7c13..711562cf 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -830,6 +830,7 @@ void __init start_xen(unsigned long boot_phys_offset,
/* Create initial domain 0. */
/* The vGIC for DOM0 is exactly emulating the hardware GIC */
config.gic_version = XEN_DOMCTL_CONFIG_GIC_DEFAULT;
+ config.nr_spis = gic_number_lines() - 32;
dom0 = domain_create(0, 0, 0, &config);
if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 8f91962..150624a 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -68,16 +68,17 @@ static void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq)
p->irq = virq;
}
-int domain_vgic_init(struct domain *d)
+int domain_vgic_init(struct domain *d, unsigned int nr_spis)
{
int i;
d->arch.vgic.ctlr = 0;
- if ( is_hardware_domain(d) )
- d->arch.vgic.nr_spis = gic_number_lines() - 32;
- else
- d->arch.vgic.nr_spis = 0; /* We don't need SPIs for the guest */
+ /* Limit the number of virtual SPIs supported to (1020 - 32) = 988 */
+ if ( (nr_spis + NR_LOCAL_IRQS) > 1020 )
+ return -EINVAL;
+
+ d->arch.vgic.nr_spis = nr_spis;
switch ( gic_hw_version() )
{
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index aba0d80..647f2fe 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -177,7 +177,7 @@ enum gic_sgi_mode;
#define vgic_num_irqs(d) ((d)->arch.vgic.nr_spis + 32)
-extern int domain_vgic_init(struct domain *d);
+extern int domain_vgic_init(struct domain *d, unsigned int nr_spis);
extern void domain_vgic_free(struct domain *d);
extern int vcpu_vgic_init(struct vcpu *v);
extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq);
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index ed7e98f..c029e0f 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -315,6 +315,8 @@ typedef uint64_t xen_callback_t;
struct xen_arch_domainconfig {
/* IN/OUT */
uint8_t gic_version;
+ /* IN */
+ uint32_t nr_spis;
};
#endif
--
2.1.4
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v5 p2 02/19] xen/arm: vgic: Add spi_to_pending
2015-04-09 15:09 [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 01/19] xen/arm: Let the toolstack configure the number of SPIs Julien Grall
@ 2015-04-09 15:09 ` Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 03/19] xen/arm: Release IRQ routed to a domain when it's destroying Julien Grall
` (16 subsequent siblings)
18 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2015-04-09 15:09 UTC (permalink / raw)
To: xen-devel; +Cc: stefano.stabellini, Julien Grall, tim, ian.campbell
From: Julien Grall <julien.grall@linaro.org>
Introduce spi_to_pending in order retrieve the irq_pending structure for
a specific SPI.
It's not possible to re-use irq_to_pending because it's required a VCPU
and some call of the new function may during domain destruction after
the VCPUs are freed.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
Changes in v5:
- Add Ian's Ack
Changes in v4:
- Patch added
---
xen/arch/arm/vgic.c | 7 +++++++
xen/include/asm-arm/vgic.h | 1 +
2 files changed, 8 insertions(+)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 150624a..55c8927 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -371,6 +371,13 @@ struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq)
return n;
}
+struct pending_irq *spi_to_pending(struct domain *d, unsigned int irq)
+{
+ ASSERT(irq >= NR_LOCAL_IRQS);
+
+ return &d->arch.vgic.pending_irqs[irq - 32];
+}
+
void vgic_clear_pending_irqs(struct vcpu *v)
{
struct pending_irq *p, *t;
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 647f2fe..8d22532 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -185,6 +185,7 @@ extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq);
extern void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq);
extern void vgic_clear_pending_irqs(struct vcpu *v);
extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
+extern struct pending_irq *spi_to_pending(struct domain *d, unsigned int irq);
extern struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, int b, int n, int s);
extern struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq);
extern int vgic_emulate(struct cpu_user_regs *regs, union hsr hsr);
--
2.1.4
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v5 p2 03/19] xen/arm: Release IRQ routed to a domain when it's destroying
2015-04-09 15:09 [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 01/19] xen/arm: Let the toolstack configure the number of SPIs Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 02/19] xen/arm: vgic: Add spi_to_pending Julien Grall
@ 2015-04-09 15:09 ` Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 04/19] xen/arm: Implement hypercall DOMCTL_{, un}bind_pt_pirq Julien Grall
` (15 subsequent siblings)
18 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2015-04-09 15:09 UTC (permalink / raw)
To: xen-devel; +Cc: stefano.stabellini, Julien Grall, tim, ian.campbell
From: Julien Grall <julien.grall@linaro.org>
Xen has to release IRQ routed to a domain in order to reuse later.
Currently only SPIs can be routed to the guest so we only need to
browse SPIs for a specific domain.
Furthermore, a guest can crash and leave the IRQ in an incorrect state
(i.e has not been EOIed). Xen will have to reset the IRQ in order to
be able to reuse the IRQ later.
Introduce 2 new functions for release an IRQ routed to a domain:
- release_guest_irq: upper level to retrieve the IRQ, call the GIC
code and release the action
- gic_remove_guest_irq: Check if we can remove the IRQ, and reset
it if necessary
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
Changes in v5:
- Typoes in the commit message
- Add Ian's Ack
Changes in v4:
- Reorder the code flow
- Typoes and coding style
- Use the newly helper spi_to_pending
Changes in v3:
- Take the vgic rank lock to protect p->desc
- Correctly check if the IRQ is disabled
- Extend the check on the virq in release_guest_irq
- Use vgic_get_target_vcpu to get the target vCPU
- Remove spurious change
Changes in v2:
- Drop the desc->handler = &no_irq_type in release_irq as it's
buggy if the IRQ is routed to Xen
- Add release_guest_irq and gic_remove_guest_irq
---
xen/arch/arm/gic.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
xen/arch/arm/irq.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
xen/arch/arm/vgic.c | 16 ++++++++++++++++
xen/include/asm-arm/gic.h | 4 ++++
xen/include/asm-arm/irq.h | 2 ++
5 files changed, 113 insertions(+)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 5f34997..f023e4f 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -163,6 +163,51 @@ out:
return res;
}
+/* This function only works with SPIs for now */
+int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
+ struct irq_desc *desc)
+{
+ struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
+ struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
+ struct pending_irq *p = irq_to_pending(v_target, virq);
+ unsigned long flags;
+
+ ASSERT(spin_is_locked(&desc->lock));
+ ASSERT(test_bit(_IRQ_GUEST, &desc->status));
+ ASSERT(p->desc == desc);
+
+ vgic_lock_rank(v_target, rank, flags);
+
+ if ( d->is_dying )
+ {
+ desc->handler->shutdown(desc);
+
+ /* EOI the IRQ it it has not been done by the guest */
+ if ( test_bit(_IRQ_INPROGRESS, &desc->status) )
+ gic_hw_ops->deactivate_irq(desc);
+ clear_bit(_IRQ_INPROGRESS, &desc->status);
+ }
+ else
+ {
+ /*
+ * TODO: Handle eviction from LRs For now, deny
+ * remove if the IRQ is inflight or not disabled.
+ */
+ if ( test_bit(_IRQ_INPROGRESS, &desc->status) ||
+ !test_bit(_IRQ_DISABLED, &desc->status) )
+ return -EBUSY;
+ }
+
+ clear_bit(_IRQ_GUEST, &desc->status);
+ desc->handler = &no_irq_type;
+
+ p->desc = NULL;
+
+ vgic_unlock_rank(v_target, rank, flags);
+
+ return 0;
+}
+
int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
unsigned int *out_hwirq,
unsigned int *out_type)
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index b2ddf6b..376c9f2 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -513,6 +513,52 @@ free_info:
return retval;
}
+int release_guest_irq(struct domain *d, unsigned int virq)
+{
+ struct irq_desc *desc;
+ struct irq_guest *info;
+ unsigned long flags;
+ struct pending_irq *p;
+ int ret;
+
+ /* Only SPIs are supported */
+ if ( virq < NR_LOCAL_IRQS || virq >= vgic_num_irqs(d) )
+ return -EINVAL;
+
+ p = spi_to_pending(d, virq);
+ if ( !p->desc )
+ return -EINVAL;
+
+ desc = p->desc;
+
+ spin_lock_irqsave(&desc->lock, flags);
+
+ ret = -EINVAL;
+ if ( !test_bit(_IRQ_GUEST, &desc->status) )
+ goto unlock;
+
+ info = irq_get_guest_info(desc);
+ ret = -EINVAL;
+ if ( d != info->d )
+ goto unlock;
+
+ ret = gic_remove_irq_from_guest(d, virq, desc);
+ if ( ret )
+ goto unlock;
+
+ spin_unlock_irqrestore(&desc->lock, flags);
+
+ release_irq(desc->irq, info);
+ xfree(info);
+
+ return 0;
+
+unlock:
+ spin_unlock_irqrestore(&desc->lock, flags);
+
+ return ret;
+}
+
/*
* pirq event channels. We don't use these on ARM, instead we use the
* features of the GIC to inject virtualised normal interrupts.
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 55c8927..05c5010 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -135,6 +135,22 @@ void register_vgic_ops(struct domain *d, const struct vgic_ops *ops)
void domain_vgic_free(struct domain *d)
{
+ int i;
+ int ret;
+
+ for ( i = 0; i < (d->arch.vgic.nr_spis); i++ )
+ {
+ struct pending_irq *p = spi_to_pending(d, i + 32);
+
+ if ( p->desc )
+ {
+ ret = release_guest_irq(d, p->irq);
+ if ( ret )
+ dprintk(XENLOG_G_WARNING, "d%u: Failed to release virq %u ret = %d\n",
+ d->domain_id, p->irq, ret);
+ }
+ }
+
xfree(d->arch.vgic.shared_irqs);
xfree(d->arch.vgic.pending_irqs);
xfree(d->arch.vgic.allocated_irqs);
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index ef4bf9a..9e2acb7 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -220,6 +220,10 @@ extern int gic_route_irq_to_guest(struct domain *, unsigned int virq,
struct irq_desc *desc,
unsigned int priority);
+/* Remove an IRQ passthrough to a guest */
+int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
+ struct irq_desc *desc);
+
extern void gic_inject(void);
extern void gic_clear_pending_irqs(struct vcpu *v);
extern int gic_events_need_delivery(void);
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index 71b39e7..34b492b 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -44,6 +44,8 @@ void init_secondary_IRQ(void);
int route_irq_to_guest(struct domain *d, unsigned int virq,
unsigned int irq, const char *devname);
+int release_guest_irq(struct domain *d, unsigned int irq);
+
void arch_move_irqs(struct vcpu *v);
/* Set IRQ type for an SPI */
--
2.1.4
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v5 p2 04/19] xen/arm: Implement hypercall DOMCTL_{, un}bind_pt_pirq
2015-04-09 15:09 [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough Julien Grall
` (2 preceding siblings ...)
2015-04-09 15:09 ` [PATCH v5 p2 03/19] xen/arm: Release IRQ routed to a domain when it's destroying Julien Grall
@ 2015-04-09 15:09 ` Julien Grall
2015-04-16 14:55 ` Ian Campbell
2015-04-09 15:09 ` [PATCH v5 p2 05/19] xen: guestcopy: Provide an helper to safely copy string from guest Julien Grall
` (14 subsequent siblings)
18 siblings, 1 reply; 48+ messages in thread
From: Julien Grall @ 2015-04-09 15:09 UTC (permalink / raw)
To: xen-devel
Cc: stefano.stabellini, Julien Grall, tim, ian.campbell, Jan Beulich
From: Julien Grall <julien.grall@linaro.org>
On x86, an IRQ is assigned in 2 steps to an HVM guest:
- The toolstack is calling PHYSDEVOP_map_pirq in order to create a
guest PIRQ (IRQ bound to an event channel)
- The emulator (QEMU) is calling DOMCTL_bind_pt_irq in order to
bind the IRQ
On ARM, there is no concept of PIRQ as the IRQ can be assigned to a
virtual IRQ using the interrupt controller.
It's not clear if we will need 2 different hypercalls on ARM to assign
IRQ and, for now, only the toolstack will manage IRQ.
In order to avoid re-using a fixed ABI hypercall (PHYSDEVOP_*) for a
different purpose and allow us more time to figure out the right out,
only DOMCTL_{,un}bind_pt_pirq is implemented on ARM.
The DOMCTL is extended with a new type PT_IRQ_TYPE_SPI and only IRQ ==
vIRQ (i.e machine_irq == spi) is supported.
Concerning XSM, even if ARM is using one hypercall rather than 2, the
resulting check is nearly the same.
XSM PHYSDEVOP_map_pirq:
1) Check if the current domain can add resource to the domain
2) Check if the current domain has permission to add the IRQ
3) Check if the target domain has permission to use the IRQ
XSM DOMCTL_bind_pirq_irq:
1) Check if the current domain can add resource to the domain
2) Check if the current domain has permission to bind the IRQ
3) Check if the target domain has permission to use the IRQ
In order to keep the same XSM checks done by the 2 hypercalls on x86,
call both xsm_map_domain_irq & xsm_bind_pt_irq in the ARM implementation.
Note: The toolstack changes for routing an IRQ to a guest will be done
in a separate patch.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Jan Beulich <jbeulich@suse.com>
---
Contrawise PHYSDEV, DOMCTL interface is not fixed. This version is
using a DOMCTL in order to let us more to to see if we need a new
PHYSDEV op for vIRQ assignation.
DOMCTL_unbind_pt_irq has been implemented, although I haven't test
it. I'm not sure if we want to keep it.
xc_domain_{,un}bind_pt_irq are left unchanged because QEMU upstream
& traditional are using them. I will send a follow-up series to drop this 2
functions in order to avoid defering this series while awaiting
changes pushed for QEMU upstream.
Changes in v5:
- Do the check of XSM PHYSDEVOP_map_pirq in the ARM implementation
- Left xc_domain_{,un}bind_pt_irq unchanged
- Introduce xc_domain_{,un}bind_pt_spi_irq
Changes in v4:
- Move the implementation from PHYSDEV to DOMCTL. Reuse
DOMCTL_{,un}bind_pt_irq for this purpose.
Changes in v3:
- Functions to allocate/release/reserved a VIRQ has been moved
in a separate patch
- Make clear that only MAP_PIRQ_GSI is only supported for now
Changes in v2:
- Add PHYSDEVOP_unmap_pirq
- Rework commit message
- Add functions to allocate/release a VIRQ
- is_routable_irq has been renamed into is_assignable_irq
---
tools/libxc/include/xenctrl.h | 8 +++++
tools/libxc/xc_domain.c | 74 +++++++++++++++++++++++++++++++++++------
xen/arch/arm/domctl.c | 77 +++++++++++++++++++++++++++++++++++++++++++
xen/include/public/domctl.h | 4 +++
xen/include/xsm/dummy.h | 24 +++++++-------
xen/include/xsm/xsm.h | 28 ++++++++--------
xen/xsm/dummy.c | 4 +--
xen/xsm/flask/hooks.c | 70 ++++++++++++++++++++-------------------
8 files changed, 217 insertions(+), 72 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 02d0db8..cc78ed6 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2114,6 +2114,14 @@ int xc_domain_bind_pt_isa_irq(xc_interface *xch,
uint32_t domid,
uint8_t machine_irq);
+int xc_domain_bind_pt_spi_irq(xc_interface *xch,
+ uint32_t domid,
+ uint16_t spi);
+
+int xc_domain_unbind_pt_spi_irq(xc_interface *xch,
+ uint32_t domid,
+ uint16_t spi);
+
int xc_domain_set_machine_address_size(xc_interface *xch,
uint32_t domid,
unsigned int width);
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 1f2c80c..676ec50 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1773,15 +1773,16 @@ int xc_domain_unbind_msi_irq(
}
/* Pass-through: binds machine irq to guests irq */
-int xc_domain_bind_pt_irq(
- xc_interface *xch,
- uint32_t domid,
- uint8_t machine_irq,
- uint8_t irq_type,
- uint8_t bus,
- uint8_t device,
- uint8_t intx,
- uint8_t isa_irq)
+static int xc_domain_bind_pt_irq_int(
+ xc_interface *xch,
+ uint32_t domid,
+ uint8_t machine_irq,
+ uint8_t irq_type,
+ uint8_t bus,
+ uint8_t device,
+ uint8_t intx,
+ uint8_t isa_irq,
+ uint16_t spi)
{
int rc;
xen_domctl_bind_pt_irq_t * bind;
@@ -1805,6 +1806,9 @@ int xc_domain_bind_pt_irq(
case PT_IRQ_TYPE_ISA:
bind->u.isa.isa_irq = isa_irq;
break;
+ case PT_IRQ_TYPE_SPI:
+ bind->u.spi.spi = spi;
+ break;
default:
errno = EINVAL;
return -1;
@@ -1814,7 +1818,7 @@ int xc_domain_bind_pt_irq(
return rc;
}
-int xc_domain_unbind_pt_irq(
+int xc_domain_bind_pt_irq(
xc_interface *xch,
uint32_t domid,
uint8_t machine_irq,
@@ -1824,6 +1828,21 @@ int xc_domain_unbind_pt_irq(
uint8_t intx,
uint8_t isa_irq)
{
+ return xc_domain_bind_pt_irq_int(xch, domid, machine_irq, irq_type,
+ bus, device, intx, isa_irq, 0);
+}
+
+static int xc_domain_unbind_pt_irq_int(
+ xc_interface *xch,
+ uint32_t domid,
+ uint8_t machine_irq,
+ uint8_t irq_type,
+ uint8_t bus,
+ uint8_t device,
+ uint8_t intx,
+ uint8_t isa_irq,
+ uint8_t spi)
+{
int rc;
xen_domctl_bind_pt_irq_t * bind;
DECLARE_DOMCTL;
@@ -1846,6 +1865,8 @@ int xc_domain_unbind_pt_irq(
case PT_IRQ_TYPE_ISA:
bind->u.isa.isa_irq = isa_irq;
break;
+ case PT_IRQ_TYPE_SPI:
+ bind->u.spi.spi = spi;
default:
errno = EINVAL;
return -1;
@@ -1855,6 +1876,20 @@ int xc_domain_unbind_pt_irq(
return rc;
}
+int xc_domain_unbind_pt_irq(
+ xc_interface *xch,
+ uint32_t domid,
+ uint8_t machine_irq,
+ uint8_t irq_type,
+ uint8_t bus,
+ uint8_t device,
+ uint8_t intx,
+ uint8_t isa_irq)
+{
+ return xc_domain_unbind_pt_irq_int(xch, domid, machine_irq, irq_type,
+ bus, device, intx, isa_irq, 0);
+}
+
int xc_domain_bind_pt_pci_irq(
xc_interface *xch,
uint32_t domid,
@@ -1878,6 +1913,25 @@ int xc_domain_bind_pt_isa_irq(
PT_IRQ_TYPE_ISA, 0, 0, 0, machine_irq));
}
+int xc_domain_bind_pt_spi_irq(
+ xc_interface *xch,
+ uint32_t domid,
+ uint16_t spi)
+{
+ /* vSPI == SPI */
+ return (xc_domain_bind_pt_irq_int(xch, domid, spi,
+ PT_IRQ_TYPE_SPI, 0, 0, 0, 0, spi));
+}
+
+int xc_domain_unbind_pt_spi_irq(
+ xc_interface *xch,
+ uint32_t domid,
+ uint16_t spi)
+{
+ return (xc_domain_unbind_pt_irq_int(xch, domid, spi,
+ PT_IRQ_TYPE_SPI, 0, 0, 0, 0, spi));
+}
+
int xc_unmap_domain_meminfo(xc_interface *xch, struct xc_domain_meminfo *minfo)
{
struct domain_info_context _di = { .guest_width = minfo->guest_width,
diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index 6f30af7..531bb13 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -10,6 +10,8 @@
#include <xen/errno.h>
#include <xen/sched.h>
#include <xen/hypercall.h>
+#include <xen/iocap.h>
+#include <xsm/xsm.h>
#include <public/domctl.h>
long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
@@ -30,6 +32,81 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
return p2m_cache_flush(d, s, e);
}
+ case XEN_DOMCTL_bind_pt_irq:
+ {
+ int rc;
+ xen_domctl_bind_pt_irq_t *bind = &domctl->u.bind_pt_irq;
+ uint32_t irq = bind->u.spi.spi;
+ uint32_t virq = bind->machine_irq;
+
+ /* We only support PT_IRQ_TYPE_SPI */
+ if ( bind->irq_type != PT_IRQ_TYPE_SPI )
+ return -EOPNOTSUPP;
+
+ /*
+ * XXX: For now map the interrupt 1:1. Other support will require to
+ * modify domain_pirq_to_irq macro.
+ */
+ if ( irq != virq )
+ return -EINVAL;
+
+ /*
+ * ARM doesn't require to separate IRQ assignation in 2
+ * hypercalls (PHYSDEVOP_map_pirq and DOMCTL_bind_pt_irq).
+ *
+ * Call xsm_map_domain_irq in order to keep the same XSM checks
+ * done by the 2 hypercalls
+ */
+ rc = xsm_map_domain_irq(XSM_HOOK, d, irq, NULL);
+ if ( rc )
+ return rc;
+
+ rc = xsm_bind_pt_irq(XSM_HOOK, d, bind);
+ if ( rc )
+ return rc;
+
+ if ( !irq_access_permitted(current->domain, irq) )
+ return -EPERM;
+
+ if ( !vgic_reserve_virq(d, virq) )
+ return -EBUSY;
+
+ rc = route_irq_to_guest(d, virq, irq, "routed IRQ");
+ if ( rc )
+ vgic_free_virq(d, virq);
+
+ return rc;
+ }
+ case XEN_DOMCTL_unbind_pt_irq:
+ {
+ int rc;
+ xen_domctl_bind_pt_irq_t *bind = &domctl->u.bind_pt_irq;
+ uint32_t irq = bind->u.spi.spi;
+ uint32_t virq = bind->machine_irq;
+
+ /* We only support PT_IRQ_TYPE_SPI */
+ if ( bind->irq_type != PT_IRQ_TYPE_SPI )
+ return -EOPNOTSUPP;
+
+ /* For now map the interrupt 1:1 */
+ if ( irq != virq )
+ return -EINVAL;
+
+ rc = xsm_unbind_pt_irq(XSM_HOOK, d, bind);
+ if ( rc )
+ return rc;
+
+ if ( !irq_access_permitted(current->domain, irq) )
+ return -EPERM;
+
+ rc = release_guest_irq(d, virq);
+ if ( rc )
+ return rc;
+
+ vgic_free_virq(d, virq);
+
+ return 0;
+ }
default:
return subarch_do_domctl(domctl, d, u_domctl);
}
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index d2e8db0..d94c647 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -504,6 +504,7 @@ typedef enum pt_irq_type_e {
PT_IRQ_TYPE_ISA,
PT_IRQ_TYPE_MSI,
PT_IRQ_TYPE_MSI_TRANSLATE,
+ PT_IRQ_TYPE_SPI, /* ARM: valid range 32-1019 */
} pt_irq_type_t;
struct xen_domctl_bind_pt_irq {
uint32_t machine_irq;
@@ -524,6 +525,9 @@ struct xen_domctl_bind_pt_irq {
uint32_t gflags;
uint64_aligned_t gtable;
} msi;
+ struct {
+ uint16_t spi;
+ } spi;
} u;
};
typedef struct xen_domctl_bind_pt_irq xen_domctl_bind_pt_irq_t;
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 4227093..c36e05f 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -445,6 +445,18 @@ static XSM_INLINE int xsm_unmap_domain_pirq(XSM_DEFAULT_ARG struct domain *d)
return xsm_default_action(action, current->domain, d);
}
+static XSM_INLINE int xsm_bind_pt_irq(XSM_DEFAULT_ARG struct domain *d, struct xen_domctl_bind_pt_irq *bind)
+{
+ XSM_ASSERT_ACTION(XSM_HOOK);
+ return xsm_default_action(action, current->domain, d);
+}
+
+static XSM_INLINE int xsm_unbind_pt_irq(XSM_DEFAULT_ARG struct domain *d, struct xen_domctl_bind_pt_irq *bind)
+{
+ XSM_ASSERT_ACTION(XSM_HOOK);
+ return xsm_default_action(action, current->domain, d);
+}
+
static XSM_INLINE int xsm_unmap_domain_irq(XSM_DEFAULT_ARG struct domain *d, int irq, void *data)
{
XSM_ASSERT_ACTION(XSM_HOOK);
@@ -631,18 +643,6 @@ static XSM_INLINE int xsm_priv_mapping(XSM_DEFAULT_ARG struct domain *d, struct
return xsm_default_action(action, d, t);
}
-static XSM_INLINE int xsm_bind_pt_irq(XSM_DEFAULT_ARG struct domain *d, struct xen_domctl_bind_pt_irq *bind)
-{
- XSM_ASSERT_ACTION(XSM_HOOK);
- return xsm_default_action(action, current->domain, d);
-}
-
-static XSM_INLINE int xsm_unbind_pt_irq(XSM_DEFAULT_ARG struct domain *d, struct xen_domctl_bind_pt_irq *bind)
-{
- XSM_ASSERT_ACTION(XSM_HOOK);
- return xsm_default_action(action, current->domain, d);
-}
-
static XSM_INLINE int xsm_ioport_permission(XSM_DEFAULT_ARG struct domain *d, uint32_t s, uint32_t e, uint8_t allow)
{
XSM_ASSERT_ACTION(XSM_HOOK);
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index c2049f3..b7446be 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -107,6 +107,8 @@ struct xsm_operations {
int (*map_domain_irq) (struct domain *d, int irq, void *data);
int (*unmap_domain_pirq) (struct domain *d);
int (*unmap_domain_irq) (struct domain *d, int irq, void *data);
+ int (*bind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq *bind);
+ int (*unbind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq *bind);
int (*irq_permission) (struct domain *d, int pirq, uint8_t allow);
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);
@@ -169,8 +171,6 @@ struct xsm_operations {
int (*mmuext_op) (struct domain *d, struct domain *f);
int (*update_va_mapping) (struct domain *d, struct domain *f, l1_pgentry_t pte);
int (*priv_mapping) (struct domain *d, struct domain *t);
- int (*bind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq *bind);
- int (*unbind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq *bind);
int (*ioport_permission) (struct domain *d, uint32_t s, uint32_t e, uint8_t allow);
int (*ioport_mapping) (struct domain *d, uint32_t s, uint32_t e, uint8_t allow);
#endif
@@ -419,6 +419,18 @@ static inline int xsm_unmap_domain_irq (xsm_default_t def, struct domain *d, int
return xsm_ops->unmap_domain_irq(d, irq, data);
}
+static inline int xsm_bind_pt_irq(xsm_default_t def, struct domain *d,
+ struct xen_domctl_bind_pt_irq *bind)
+{
+ return xsm_ops->bind_pt_irq(d, bind);
+}
+
+static inline int xsm_unbind_pt_irq(xsm_default_t def, struct domain *d,
+ struct xen_domctl_bind_pt_irq *bind)
+{
+ return xsm_ops->unbind_pt_irq(d, bind);
+}
+
static inline int xsm_irq_permission (xsm_default_t def, struct domain *d, int pirq, uint8_t allow)
{
return xsm_ops->irq_permission(d, pirq, allow);
@@ -643,18 +655,6 @@ static inline int xsm_priv_mapping(xsm_default_t def, struct domain *d, struct d
return xsm_ops->priv_mapping(d, t);
}
-static inline int xsm_bind_pt_irq(xsm_default_t def, struct domain *d,
- struct xen_domctl_bind_pt_irq *bind)
-{
- return xsm_ops->bind_pt_irq(d, bind);
-}
-
-static inline int xsm_unbind_pt_irq(xsm_default_t def, struct domain *d,
- struct xen_domctl_bind_pt_irq *bind)
-{
- return xsm_ops->unbind_pt_irq(d, bind);
-}
-
static inline int xsm_ioport_permission (xsm_default_t def, struct domain *d, uint32_t s, uint32_t e, uint8_t allow)
{
return xsm_ops->ioport_permission(d, s, e, allow);
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 25fca68..a3b8aab 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -81,6 +81,8 @@ void xsm_fixup_ops (struct xsm_operations *ops)
set_to_dummy_if_null(ops, map_domain_irq);
set_to_dummy_if_null(ops, unmap_domain_pirq);
set_to_dummy_if_null(ops, unmap_domain_irq);
+ set_to_dummy_if_null(ops, bind_pt_irq);
+ set_to_dummy_if_null(ops, unbind_pt_irq);
set_to_dummy_if_null(ops, irq_permission);
set_to_dummy_if_null(ops, iomem_permission);
set_to_dummy_if_null(ops, iomem_mapping);
@@ -140,8 +142,6 @@ void xsm_fixup_ops (struct xsm_operations *ops)
set_to_dummy_if_null(ops, mmuext_op);
set_to_dummy_if_null(ops, update_va_mapping);
set_to_dummy_if_null(ops, priv_mapping);
- set_to_dummy_if_null(ops, bind_pt_irq);
- set_to_dummy_if_null(ops, unbind_pt_irq);
set_to_dummy_if_null(ops, ioport_permission);
set_to_dummy_if_null(ops, ioport_mapping);
#endif
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index ab5141d..688ba2a 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -580,12 +580,14 @@ static int flask_domctl(struct domain *d, int cmd)
#ifdef HAS_MEM_ACCESS
case XEN_DOMCTL_vm_event_op:
#endif
+
+ /* These have individual XSM hooks (arch/../domctl.c) */
+ case XEN_DOMCTL_bind_pt_irq:
+ case XEN_DOMCTL_unbind_pt_irq:
#ifdef CONFIG_X86
/* These have individual XSM hooks (arch/x86/domctl.c) */
case XEN_DOMCTL_shadow_op:
case XEN_DOMCTL_ioport_permission:
- case XEN_DOMCTL_bind_pt_irq:
- case XEN_DOMCTL_unbind_pt_irq:
case XEN_DOMCTL_ioport_mapping:
/* These have individual XSM hooks (drivers/passthrough/iommu.c) */
case XEN_DOMCTL_get_device_group:
@@ -911,6 +913,36 @@ static int flask_unmap_domain_irq (struct domain *d, int irq, void *data)
return rc;
}
+static int flask_bind_pt_irq (struct domain *d, struct xen_domctl_bind_pt_irq *bind)
+{
+ u32 dsid, rsid;
+ int rc = -EPERM;
+ int irq;
+ struct avc_audit_data ad;
+
+ rc = current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__ADD);
+ if ( rc )
+ return rc;
+
+ irq = domain_pirq_to_irq(d, bind->machine_irq);
+
+ rc = get_irq_sid(irq, &rsid, &ad);
+ if ( rc )
+ return rc;
+
+ rc = avc_current_has_perm(rsid, SECCLASS_HVM, HVM__BIND_IRQ, &ad);
+ if ( rc )
+ return rc;
+
+ dsid = domain_sid(d);
+ return avc_has_perm(dsid, rsid, SECCLASS_RESOURCE, RESOURCE__USE, &ad);
+}
+
+static int flask_unbind_pt_irq (struct domain *d, struct xen_domctl_bind_pt_irq *bind)
+{
+ return current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__REMOVE);
+}
+
static int flask_irq_permission (struct domain *d, int pirq, uint8_t access)
{
/* the PIRQ number is not useful; real IRQ is checked during mapping */
@@ -1468,36 +1500,6 @@ static int flask_priv_mapping(struct domain *d, struct domain *t)
{
return domain_has_perm(d, t, SECCLASS_MMU, MMU__TARGET_HACK);
}
-
-static int flask_bind_pt_irq (struct domain *d, struct xen_domctl_bind_pt_irq *bind)
-{
- u32 dsid, rsid;
- int rc = -EPERM;
- int irq;
- struct avc_audit_data ad;
-
- rc = current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__ADD);
- if ( rc )
- return rc;
-
- irq = domain_pirq_to_irq(d, bind->machine_irq);
-
- rc = get_irq_sid(irq, &rsid, &ad);
- if ( rc )
- return rc;
-
- rc = avc_current_has_perm(rsid, SECCLASS_HVM, HVM__BIND_IRQ, &ad);
- if ( rc )
- return rc;
-
- dsid = domain_sid(d);
- return avc_has_perm(dsid, rsid, SECCLASS_RESOURCE, RESOURCE__USE, &ad);
-}
-
-static int flask_unbind_pt_irq (struct domain *d, struct xen_domctl_bind_pt_irq *bind)
-{
- return current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__REMOVE);
-}
#endif /* CONFIG_X86 */
long do_flask_op(XEN_GUEST_HANDLE_PARAM(xsm_op_t) u_flask_op);
@@ -1556,6 +1558,8 @@ static struct xsm_operations flask_ops = {
.map_domain_irq = flask_map_domain_irq,
.unmap_domain_pirq = flask_unmap_domain_pirq,
.unmap_domain_irq = flask_unmap_domain_irq,
+ .bind_pt_irq = flask_bind_pt_irq,
+ .unbind_pt_irq = flask_unbind_pt_irq,
.irq_permission = flask_irq_permission,
.iomem_permission = flask_iomem_permission,
.iomem_mapping = flask_iomem_mapping,
@@ -1616,8 +1620,6 @@ static struct xsm_operations flask_ops = {
.mmuext_op = flask_mmuext_op,
.update_va_mapping = flask_update_va_mapping,
.priv_mapping = flask_priv_mapping,
- .bind_pt_irq = flask_bind_pt_irq,
- .unbind_pt_irq = flask_unbind_pt_irq,
.ioport_permission = flask_ioport_permission,
.ioport_mapping = flask_ioport_mapping,
#endif
--
2.1.4
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v5 p2 05/19] xen: guestcopy: Provide an helper to safely copy string from guest
2015-04-09 15:09 [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough Julien Grall
` (3 preceding siblings ...)
2015-04-09 15:09 ` [PATCH v5 p2 04/19] xen/arm: Implement hypercall DOMCTL_{, un}bind_pt_pirq Julien Grall
@ 2015-04-09 15:09 ` Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 06/19] xen/dts: Provide an helper to get a DT node from a path provided by a guest Julien Grall
` (13 subsequent siblings)
18 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2015-04-09 15:09 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, ian.campbell, tim, Julien Grall, Ian Jackson,
stefano.stabellini, Jan Beulich, Daniel De Graaf
From: Julien Grall <julien.grall@linaro.org>
Flask code already provides a helper to copy a string from guest. In a later
patch, the new DT hypercalls will need a similar function.
To avoid code duplication, copy the flask helper (flask_copying_string) to
common code:
- Rename into safe_copy_string_from_guest
- Add comment to explain the extra +1
- Return the buffer directly and use the macros provided by
xen/err.h to return an error code if necessary.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>
---
Changes in v5:
- return a char* rather than a void*
- Use '\0' rather than 0
Changes in v4:
- Use -ENOBUFS rather than -ENOENT
- Fix coding style in comment
- Typoes in commit message
- Convert the new flask_copying_string (for DT) in
safe_copy_string_from_guest
- Add Ian and Daniel's ack
Changes in v3:
- Use macros of xen/err.h to return either the buffer or an
error code
- Reuse size_t instead of unsigned long
- Update comment and commit message
Changes in v2:
- Rename copy_string_from_guest into safe_copy_string_from_guest
- Update commit message and comment in the code
---
xen/common/Makefile | 1 +
xen/common/guestcopy.c | 31 ++++++++++++++++++++++++++
xen/include/xen/guest_access.h | 5 +++++
xen/xsm/flask/flask_op.c | 49 +++++++++++-------------------------------
4 files changed, 50 insertions(+), 36 deletions(-)
create mode 100644 xen/common/guestcopy.c
diff --git a/xen/common/Makefile b/xen/common/Makefile
index e5bd75b..74deac0 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -9,6 +9,7 @@ obj-y += event_2l.o
obj-y += event_channel.o
obj-y += event_fifo.o
obj-y += grant_table.o
+obj-y += guestcopy.o
obj-y += irq.o
obj-y += kernel.o
obj-y += keyhandler.o
diff --git a/xen/common/guestcopy.c b/xen/common/guestcopy.c
new file mode 100644
index 0000000..6ae1815
--- /dev/null
+++ b/xen/common/guestcopy.c
@@ -0,0 +1,31 @@
+#include <xen/config.h>
+#include <xen/lib.h>
+#include <xen/guest_access.h>
+#include <xen/err.h>
+
+/*
+ * The function copies a string from the guest and adds a NUL to
+ * make sure the string is correctly terminated.
+ */
+char *safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf,
+ size_t size, size_t max_size)
+{
+ char *tmp;
+
+ if ( size > max_size )
+ return ERR_PTR(-ENOBUFS);
+
+ /* Add an extra +1 to append \0 */
+ tmp = xmalloc_array(char, size + 1);
+ if ( !tmp )
+ return ERR_PTR(-ENOMEM);
+
+ if ( copy_from_guest(tmp, u_buf, size) )
+ {
+ xfree(tmp);
+ return ERR_PTR(-EFAULT);
+ }
+ tmp[size] = '\0';
+
+ return tmp;
+}
diff --git a/xen/include/xen/guest_access.h b/xen/include/xen/guest_access.h
index 373454e..09989df 100644
--- a/xen/include/xen/guest_access.h
+++ b/xen/include/xen/guest_access.h
@@ -8,6 +8,8 @@
#define __XEN_GUEST_ACCESS_H__
#include <asm/guest_access.h>
+#include <xen/types.h>
+#include <public/xen.h>
#define copy_to_guest(hnd, ptr, nr) \
copy_to_guest_offset(hnd, 0, ptr, nr)
@@ -27,4 +29,7 @@
#define __clear_guest(hnd, nr) \
__clear_guest_offset(hnd, 0, nr)
+char *safe_copy_string_from_guest(XEN_GUEST_HANDLE(char) u_buf,
+ size_t size, size_t max_size);
+
#endif /* __XEN_GUEST_ACCESS_H__ */
diff --git a/xen/xsm/flask/flask_op.c b/xen/xsm/flask/flask_op.c
index 47aacc1..802ffd4 100644
--- a/xen/xsm/flask/flask_op.c
+++ b/xen/xsm/flask/flask_op.c
@@ -12,6 +12,7 @@
#include <xen/event.h>
#include <xsm/xsm.h>
#include <xen/guest_access.h>
+#include <xen/err.h>
#include <public/xsm/flask_op.h>
@@ -93,29 +94,6 @@ static int domain_has_security(struct domain *d, u32 perms)
perms, NULL);
}
-static int flask_copyin_string(XEN_GUEST_HANDLE(char) u_buf, char **buf,
- size_t size, size_t max_size)
-{
- char *tmp;
-
- if ( size > max_size )
- return -ENOENT;
-
- tmp = xmalloc_array(char, size + 1);
- if ( !tmp )
- return -ENOMEM;
-
- if ( copy_from_guest(tmp, u_buf, size) )
- {
- xfree(tmp);
- return -EFAULT;
- }
- tmp[size] = 0;
-
- *buf = tmp;
- return 0;
-}
-
#endif /* COMPAT */
static int flask_security_user(struct xen_flask_userlist *arg)
@@ -129,9 +107,9 @@ static int flask_security_user(struct xen_flask_userlist *arg)
if ( rv )
return rv;
- rv = flask_copyin_string(arg->u.user, &user, arg->size, PAGE_SIZE);
- if ( rv )
- return rv;
+ user = safe_copy_string_from_guest(arg->u.user, arg->size, PAGE_SIZE);
+ if ( IS_ERR(user) )
+ return PTR_ERR(user);
rv = security_get_user_sids(arg->start_sid, user, &sids, &nsids);
if ( rv < 0 )
@@ -244,9 +222,9 @@ static int flask_security_context(struct xen_flask_sid_context *arg)
if ( rv )
return rv;
- rv = flask_copyin_string(arg->context, &buf, arg->size, PAGE_SIZE);
- if ( rv )
- return rv;
+ buf = safe_copy_string_from_guest(arg->context, arg->size, PAGE_SIZE);
+ if ( IS_ERR(buf) )
+ return PTR_ERR(buf);
rv = security_context_to_sid(buf, arg->size, &arg->sid);
if ( rv < 0 )
@@ -336,14 +314,13 @@ static int flask_security_setavc_threshold(struct xen_flask_setavc_threshold *ar
static int flask_security_resolve_bool(struct xen_flask_boolean *arg)
{
char *name;
- int rv;
if ( arg->bool_id != -1 )
return 0;
- rv = flask_copyin_string(arg->name, &name, arg->size, bool_maxstr);
- if ( rv )
- return rv;
+ name = safe_copy_string_from_guest(arg->name, arg->size, bool_maxstr);
+ if ( IS_ERR(name) )
+ return PTR_ERR(name);
arg->bool_id = security_find_bool(name);
arg->size = 0;
@@ -574,9 +551,9 @@ static int flask_devicetree_label(struct xen_flask_devicetree_label *arg)
if ( rv )
return rv;
- rv = flask_copyin_string(arg->path, &buf, arg->length, PAGE_SIZE);
- if ( rv )
- return rv;
+ buf = safe_copy_string_from_guest(arg->path, arg->length, PAGE_SIZE);
+ if ( IS_ERR(buf) )
+ return PTR_ERR(buf);
/* buf is consumed or freed by this function */
rv = security_devicetree_setlabel(buf, sid);
--
2.1.4
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v5 p2 06/19] xen/dts: Provide an helper to get a DT node from a path provided by a guest
2015-04-09 15:09 [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough Julien Grall
` (4 preceding siblings ...)
2015-04-09 15:09 ` [PATCH v5 p2 05/19] xen: guestcopy: Provide an helper to safely copy string from guest Julien Grall
@ 2015-04-09 15:09 ` Julien Grall
2015-04-16 14:57 ` Ian Campbell
2015-04-09 15:09 ` [PATCH v5 p2 07/19] xen/passthrough: Introduce iommu_construct Julien Grall
` (12 subsequent siblings)
18 siblings, 1 reply; 48+ messages in thread
From: Julien Grall @ 2015-04-09 15:09 UTC (permalink / raw)
To: xen-devel; +Cc: stefano.stabellini, Julien Grall, tim, ian.campbell
From: Julien Grall <julien.grall@linaro.org>
The maximum size of the copied string has been chosen based on the value
use by XSM in similar case.
Furthermore, Linux seems to allow path up to 4096 characters. Though
this could vary from one OS to another.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
Changes in v4:
- Drop DEVICE_TREE_MAX_PATHLEN
- Bump the value to PAGE_SIZE (i.e 4096). It's used in XSM and
this value seems sensible for Linux
- Clarify how the maximum size has been chosen
Changes in v3:
- Use the new prototype of safe_copy_string_from_guest
Changes in v2:
- guest_copy_string_from_guest has been renamed into
safe_copy_string_from_guest
---
xen/common/device_tree.c | 18 ++++++++++++++++++
xen/include/xen/device_tree.h | 14 ++++++++++++++
2 files changed, 32 insertions(+)
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 02cae91..31f169b 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -13,6 +13,7 @@
#include <xen/config.h>
#include <xen/types.h>
#include <xen/init.h>
+#include <xen/guest_access.h>
#include <xen/device_tree.h>
#include <xen/kernel.h>
#include <xen/lib.h>
@@ -23,6 +24,7 @@
#include <xen/cpumask.h>
#include <xen/ctype.h>
#include <asm/setup.h>
+#include <xen/err.h>
const void *device_tree_flattened;
dt_irq_xlate_func dt_irq_xlate;
@@ -277,6 +279,22 @@ struct dt_device_node *dt_find_node_by_path(const char *path)
return np;
}
+int dt_find_node_by_gpath(XEN_GUEST_HANDLE(char) u_path, uint32_t u_plen,
+ struct dt_device_node **node)
+{
+ char *path;
+
+ path = safe_copy_string_from_guest(u_path, u_plen, PAGE_SIZE);
+ if ( IS_ERR(path) )
+ return PTR_ERR(path);
+
+ *node = dt_find_node_by_path(path);
+
+ xfree(path);
+
+ return (*node == NULL) ? -ESRCH : 0;
+}
+
struct dt_device_node *dt_find_node_by_alias(const char *alias)
{
const struct dt_alias_prop *app;
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 57eb3ee..e187780 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -456,6 +456,20 @@ struct dt_device_node *dt_find_node_by_alias(const char *alias);
*/
struct dt_device_node *dt_find_node_by_path(const char *path);
+
+/**
+ * dt_find_node_by_gpath - Same as dt_find_node_by_path but retrieve the
+ * path from the guest
+ *
+ * @u_path: Xen Guest handle to the buffer containing the path
+ * @u_plen: Length of the buffer
+ * @node: TODO
+ *
+ * Return 0 if succeed otherwise -errno
+ */
+int dt_find_node_by_gpath(XEN_GUEST_HANDLE(char) u_path, uint32_t u_plen,
+ struct dt_device_node **node);
+
/**
* dt_get_parent - Get a node's parent if any
* @node: Node to get parent
--
2.1.4
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v5 p2 07/19] xen/passthrough: Introduce iommu_construct
2015-04-09 15:09 [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough Julien Grall
` (5 preceding siblings ...)
2015-04-09 15:09 ` [PATCH v5 p2 06/19] xen/dts: Provide an helper to get a DT node from a path provided by a guest Julien Grall
@ 2015-04-09 15:09 ` Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 08/19] xen/passthrough: arm: release the DT devices assigned to a guest earlier Julien Grall
` (11 subsequent siblings)
18 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2015-04-09 15:09 UTC (permalink / raw)
To: xen-devel
Cc: stefano.stabellini, Julien Grall, tim, ian.campbell, Jan Beulich
From: Julien Grall <julien.grall@linaro.org>
This new function will correctly initialize the IOMMU page table for the
current domain.
Also use it in iommu_assign_dt_device even though the current IOMMU
implementation on ARM shares P2M with the processor.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
Changes in v5:
- Limit the scope of rc
- Add Jan's and Ian's ack
Changes in v4:
- Move memory_type_changed in iommu_construct. Added by commit
06ed8cc "x86: avoid needless EPT table ajustment and cache
flush"
- And an ASSERT and a comment in iommu_assign_dt_device to
explain why the call is safe for DOM0
Changes in v3:
- The ASSERT in iommu_construct was redundant with the if ()
- Remove d->need_iommu = 1 in assign_device has it's already
done by iommu_construct.
- Simplify the code in the caller of iommu_construct
Changes in v2:
- Add missing Signed-off-by
- Rename iommu_buildup to iommu_construct
---
xen/drivers/passthrough/arm/iommu.c | 6 ++++++
xen/drivers/passthrough/device_tree.c | 12 ++++++++++++
xen/drivers/passthrough/iommu.c | 26 ++++++++++++++++++++++++++
xen/drivers/passthrough/pci.c | 22 ++++------------------
xen/include/xen/iommu.h | 2 ++
5 files changed, 50 insertions(+), 18 deletions(-)
diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
index 3007b99..9234657 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -68,3 +68,9 @@ void arch_iommu_domain_destroy(struct domain *d)
{
iommu_dt_domain_destroy(d);
}
+
+int arch_iommu_populate_page_table(struct domain *d)
+{
+ /* The IOMMU shares the p2m with the CPU */
+ return -ENOSYS;
+}
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 377d41d..4d82a09 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -41,6 +41,18 @@ int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev)
if ( !list_empty(&dev->domain_list) )
goto fail;
+ if ( need_iommu(d) <= 0 )
+ {
+ /*
+ * The hwdom is forced to use IOMMU for protecting assigned
+ * device. Therefore the IOMMU data is already set up.
+ */
+ ASSERT(!is_hardware_domain(d));
+ rc = iommu_construct(d);
+ if ( rc )
+ goto fail;
+ }
+
rc = hd->platform_ops->assign_device(d, 0, dt_to_dev(dev));
if ( rc )
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 92ea26f..ae42aae 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -187,6 +187,32 @@ void iommu_teardown(struct domain *d)
tasklet_schedule(&iommu_pt_cleanup_tasklet);
}
+int iommu_construct(struct domain *d)
+{
+ if ( need_iommu(d) > 0 )
+ return 0;
+
+ if ( !iommu_use_hap_pt(d) )
+ {
+ int rc;
+
+ rc = arch_iommu_populate_page_table(d);
+ if ( rc )
+ return rc;
+ }
+
+ d->need_iommu = 1;
+ /*
+ * There may be dirty cache lines when a device is assigned
+ * and before need_iommu(d) becoming true, this will cause
+ * memory_type_changed lose effect if memory type changes.
+ * Call memory_type_changed here to amend this.
+ */
+ memory_type_changed(d);
+
+ return 0;
+}
+
void iommu_domain_destroy(struct domain *d)
{
struct hvm_iommu *hd = domain_hvm_iommu(d);
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 9f3413c..af26423 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1358,25 +1358,11 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
if ( !spin_trylock(&pcidevs_lock) )
return -ERESTART;
- if ( need_iommu(d) <= 0 )
+ rc = iommu_construct(d);
+ if ( rc )
{
- if ( !iommu_use_hap_pt(d) )
- {
- rc = arch_iommu_populate_page_table(d);
- if ( rc )
- {
- spin_unlock(&pcidevs_lock);
- return rc;
- }
- }
- d->need_iommu = 1;
- /*
- * There may be dirty cache lines when a device is assigned
- * and before need_iommu(d) becoming true, this will cause
- * memory_type_changed lose effect if memory type changes.
- * Call memory_type_changed here to amend this.
- */
- memory_type_changed(d);
+ spin_unlock(&pcidevs_lock);
+ return rc;
}
pdev = pci_get_pdev_by_domain(hardware_domain, seg, bus, devfn);
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index bf4aff0..e9d2d5c 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -65,6 +65,8 @@ int arch_iommu_domain_init(struct domain *d);
int arch_iommu_populate_page_table(struct domain *d);
void arch_iommu_check_autotranslated_hwdom(struct domain *d);
+int iommu_construct(struct domain *d);
+
/* Function used internally, use iommu_domain_destroy */
void iommu_teardown(struct domain *d);
--
2.1.4
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v5 p2 08/19] xen/passthrough: arm: release the DT devices assigned to a guest earlier
2015-04-09 15:09 [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough Julien Grall
` (6 preceding siblings ...)
2015-04-09 15:09 ` [PATCH v5 p2 07/19] xen/passthrough: Introduce iommu_construct Julien Grall
@ 2015-04-09 15:09 ` Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 09/19] xen/passthrough: iommu_deassign_device_dt: By default reassign device to nobody Julien Grall
` (10 subsequent siblings)
18 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2015-04-09 15:09 UTC (permalink / raw)
To: xen-devel
Cc: ian.campbell, Julien Grall, tim, Robert VanVossen,
stefano.stabellini, Jan Beulich
From: Julien Grall <julien.grall@linaro.org>
The toolstack may not have deassigned every device used by a guest.
Therefore we have to go through the device list and remove them before
asking the IOMMU drivers to release memory for this domain.
This can be done by moving the call to the release function when we
relinquish the resources. The IOMMU part will be destroyed later when
the domain is freed.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Signed-off-by: Robert VanVossen <robert.vanvossen@dornerworks.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
Changes in v5:
- Add Ian's ack
Changes in v4:
- Typoes in commit message
- Add Jan's ack
- iommu_release_dt_devices was only release the first device by
mistake. Thanks for Robert VanVossen for spotting it.
Changes in v3:
- Patch added. Superseed the patch "xen/passthrough: call
arch_iommu_domain_destroy before calling iommu teardown" in
the previous patch series.
---
xen/arch/arm/domain.c | 4 ++++
xen/drivers/passthrough/arm/iommu.c | 1 -
xen/drivers/passthrough/device_tree.c | 7 ++++++-
xen/include/xen/iommu.h | 2 +-
4 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 180bccc..24b8938 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -795,6 +795,10 @@ int domain_relinquish_resources(struct domain *d)
switch ( d->arch.relmem )
{
case RELMEM_not_started:
+ ret = iommu_release_dt_devices(d);
+ if ( ret )
+ return ret;
+
d->arch.relmem = RELMEM_xen;
/* Fallthrough */
diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c
index 9234657..95b1abb 100644
--- a/xen/drivers/passthrough/arm/iommu.c
+++ b/xen/drivers/passthrough/arm/iommu.c
@@ -66,7 +66,6 @@ int arch_iommu_domain_init(struct domain *d)
void arch_iommu_domain_destroy(struct domain *d)
{
- iommu_dt_domain_destroy(d);
}
int arch_iommu_populate_page_table(struct domain *d)
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 4d82a09..05ab274 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -105,7 +105,7 @@ int iommu_dt_domain_init(struct domain *d)
return 0;
}
-void iommu_dt_domain_destroy(struct domain *d)
+int iommu_release_dt_devices(struct domain *d)
{
struct hvm_iommu *hd = domain_hvm_iommu(d);
struct dt_device_node *dev, *_dev;
@@ -115,7 +115,12 @@ void iommu_dt_domain_destroy(struct domain *d)
{
rc = iommu_deassign_dt_device(d, dev);
if ( rc )
+ {
dprintk(XENLOG_ERR, "Failed to deassign %s in domain %u\n",
dt_node_full_name(dev), d->domain_id);
+ return rc;
+ }
}
+
+ return 0;
}
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index e9d2d5c..d9c9ede 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -117,7 +117,7 @@ void iommu_read_msi_from_ire(struct msi_desc *msi_desc, struct msi_msg *msg);
int iommu_assign_dt_device(struct domain *d, struct dt_device_node *dev);
int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev);
int iommu_dt_domain_init(struct domain *d);
-void iommu_dt_domain_destroy(struct domain *d);
+int iommu_release_dt_devices(struct domain *d);
#endif /* HAS_DEVICE_TREE */
--
2.1.4
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v5 p2 09/19] xen/passthrough: iommu_deassign_device_dt: By default reassign device to nobody
2015-04-09 15:09 [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough Julien Grall
` (7 preceding siblings ...)
2015-04-09 15:09 ` [PATCH v5 p2 08/19] xen/passthrough: arm: release the DT devices assigned to a guest earlier Julien Grall
@ 2015-04-09 15:09 ` Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 10/19] xen/iommu: arm: Wire iommu DOMCTL for ARM Julien Grall
` (9 subsequent siblings)
18 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2015-04-09 15:09 UTC (permalink / raw)
To: xen-devel; +Cc: stefano.stabellini, Julien Grall, tim, ian.campbell
From: Julien Grall <julien.grall@linaro.org>
Currently, when the device is deassigned from a domain, we directly reassign
to DOM0.
As the device may not have been correctly reset, this may lead to corruption or
expose some part of DOM0 memory. Also, we may have no way to reset some
platform devices.
If Xen reassigns the device to "nobody", it may receive some global/context
fault because the transaction has failed (indeed the context has been
marked invalid). Unfortunately there is no simple way to quiesce a buggy
hardware. I think we could live with that for a first version of platform
device passthrough.
DOM0 will have to issue an hypercall to assign the device to itself if it
wants to use it.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Stefano Stabellini <stefano.stabellini@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
Note: This behavior is documented in a following patch which extend
DOMCT_*assign_device to support non-PCI passthrough.
Changes in v5:
- Add Ian's ack
Changes in v4:
- Add Stefano's ack
Changes in v3:
- Use the coding style of the new SMMU drivers
Changes in v2:
- Fix typoes in the commit message
- Update commit message
---
xen/drivers/passthrough/arm/smmu.c | 8 +++++++-
xen/drivers/passthrough/device_tree.c | 9 +++------
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 8a9b58b..65de50b 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2692,7 +2692,7 @@ static int arm_smmu_reassign_dev(struct domain *s, struct domain *t,
int ret = 0;
/* Don't allow remapping on other domain than hwdom */
- if (t != hardware_domain)
+ if (t && t != hardware_domain)
return -EPERM;
if (t == s)
@@ -2702,6 +2702,12 @@ static int arm_smmu_reassign_dev(struct domain *s, struct domain *t,
if (ret)
return ret;
+ if (t) {
+ ret = arm_smmu_assign_dev(t, devfn, dev);
+ if (ret)
+ return ret;
+ }
+
return 0;
}
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 05ab274..0ec4103 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -80,15 +80,12 @@ int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev)
spin_lock(&dtdevs_lock);
- rc = hd->platform_ops->reassign_device(d, hardware_domain,
- 0, dt_to_dev(dev));
+ rc = hd->platform_ops->reassign_device(d, NULL, 0, dt_to_dev(dev));
if ( rc )
goto fail;
- list_del(&dev->domain_list);
-
- dt_device_set_used_by(dev, hardware_domain->domain_id);
- list_add(&dev->domain_list, &domain_hvm_iommu(hardware_domain)->dt_devices);
+ list_del_init(&dev->domain_list);
+ dt_device_set_used_by(dev, DOMID_IO);
fail:
spin_unlock(&dtdevs_lock);
--
2.1.4
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v5 p2 10/19] xen/iommu: arm: Wire iommu DOMCTL for ARM
2015-04-09 15:09 [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough Julien Grall
` (8 preceding siblings ...)
2015-04-09 15:09 ` [PATCH v5 p2 09/19] xen/passthrough: iommu_deassign_device_dt: By default reassign device to nobody Julien Grall
@ 2015-04-09 15:09 ` Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 11/19] xen/xsm: Add helpers to check permission for device tree passthrough Julien Grall
` (8 subsequent siblings)
18 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2015-04-09 15:09 UTC (permalink / raw)
To: xen-devel
Cc: stefano.stabellini, Julien Grall, tim, ian.campbell, Stefano Stabellini
From: Julien Grall <julien.grall@linaro.org>
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
Changes in v4:
- Add Ian's ack
Changes in v3:
- Add Stefano's ack
Changes in v2:
- Don't move the call in common code.
---
xen/arch/arm/domctl.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
index 531bb13..fd1e610 100644
--- a/xen/arch/arm/domctl.c
+++ b/xen/arch/arm/domctl.c
@@ -108,7 +108,16 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
return 0;
}
default:
- return subarch_do_domctl(domctl, d, u_domctl);
+ {
+ int rc;
+
+ rc = subarch_do_domctl(domctl, d, u_domctl);
+
+ if ( rc == -ENOSYS )
+ rc = iommu_do_domctl(domctl, d, u_domctl);
+
+ return rc;
+ }
}
}
--
2.1.4
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v5 p2 11/19] xen/xsm: Add helpers to check permission for device tree passthrough
2015-04-09 15:09 [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough Julien Grall
` (9 preceding siblings ...)
2015-04-09 15:09 ` [PATCH v5 p2 10/19] xen/iommu: arm: Wire iommu DOMCTL for ARM Julien Grall
@ 2015-04-09 15:09 ` Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 12/19] xen/passthrough: Extend XEN_DOMCTL_*assign_device to support DT device Julien Grall
` (7 subsequent siblings)
18 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2015-04-09 15:09 UTC (permalink / raw)
To: xen-devel
Cc: stefano.stabellini, Daniel De Graaf, Julien Grall, tim, ian.campbell
From: Julien Grall <julien.grall@linaro.org>
This is a follow-up of commit 525ee49 "xsm: add device tree labeling
support" which add support for device tree labelling in flask.
Those helpers will be use latter when non-pci passthrough (i.e device
tree) will be added.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
Changes in v5:
- Add Ian and Daniel's ack
Changes in v4:
- Patch added
---
xen/include/xsm/dummy.h | 23 +++++++++++++
xen/include/xsm/xsm.h | 27 +++++++++++++++
xen/xsm/dummy.c | 6 ++++
xen/xsm/flask/avc.c | 3 ++
xen/xsm/flask/hooks.c | 69 ++++++++++++++++++++++++++++++++++++-
xen/xsm/flask/include/avc.h | 2 ++
xen/xsm/flask/policy/access_vectors | 2 +-
7 files changed, 130 insertions(+), 2 deletions(-)
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index c36e05f..05641f9 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -350,6 +350,29 @@ static XSM_INLINE int xsm_deassign_device(XSM_DEFAULT_ARG struct domain *d, uint
#endif /* HAS_PASSTHROUGH && HAS_PCI */
+#if defined(HAS_PASSTHROUGH) && defined(HAS_DEVICE_TREE)
+static XSM_INLINE int xsm_test_assign_dtdevice(XSM_DEFAULT_ARG const char *dtpath)
+{
+ XSM_ASSERT_ACTION(XSM_HOOK);
+ return xsm_default_action(action, current->domain, NULL);
+}
+
+static XSM_INLINE int xsm_assign_dtdevice(XSM_DEFAULT_ARG struct domain *d,
+ const char *dtpath)
+{
+ XSM_ASSERT_ACTION(XSM_HOOK);
+ return xsm_default_action(action, current->domain, d);
+}
+
+static XSM_INLINE int xsm_deassign_dtdevice(XSM_DEFAULT_ARG struct domain *d,
+ const char *dtpath)
+{
+ XSM_ASSERT_ACTION(XSM_HOOK);
+ return xsm_default_action(action, current->domain, d);
+}
+
+#endif /* HAS_PASSTHROUGH && HAS_DEVICE_TREE */
+
static XSM_INLINE int xsm_resource_plug_core(XSM_DEFAULT_VOID)
{
XSM_ASSERT_ACTION(XSM_HOOK);
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index b7446be..c830b47 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -121,6 +121,12 @@ struct xsm_operations {
int (*deassign_device) (struct domain *d, uint32_t machine_bdf);
#endif
+#if defined(HAS_PASSTHROUGH) && defined(HAS_DEVICE_TREE)
+ int (*test_assign_dtdevice) (const char *dtpath);
+ int (*assign_dtdevice) (struct domain *d, const char *dtpath);
+ int (*deassign_dtdevice) (struct domain *d, const char *dtpath);
+#endif
+
int (*resource_plug_core) (void);
int (*resource_unplug_core) (void);
int (*resource_plug_pci) (uint32_t machine_bdf);
@@ -473,6 +479,27 @@ static inline int xsm_deassign_device(xsm_default_t def, struct domain *d, uint3
}
#endif /* HAS_PASSTHROUGH && HAS_PCI) */
+#if defined(HAS_PASSTHROUGH) && defined(HAS_DEVICE_TREE)
+static inline int xsm_assign_dtdevice(xsm_default_t def, struct domain *d,
+ const char *dtpath)
+{
+ return xsm_ops->assign_dtdevice(d, dtpath);
+}
+
+static inline int xsm_test_assign_dtdevice(xsm_default_t def,
+ const char *dtpath)
+{
+ return xsm_ops->test_assign_dtdevice(dtpath);
+}
+
+static inline int xsm_deassign_dtdevice(xsm_default_t def, struct domain *d,
+ const char *dtpath)
+{
+ return xsm_ops->deassign_dtdevice(d, dtpath);
+}
+
+#endif /* HAS_PASSTHROUGH && HAS_DEVICE_TREE */
+
static inline int xsm_resource_plug_pci (xsm_default_t def, uint32_t machine_bdf)
{
return xsm_ops->resource_plug_pci(machine_bdf);
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index a3b8aab..ef856dc 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -96,6 +96,12 @@ void xsm_fixup_ops (struct xsm_operations *ops)
set_to_dummy_if_null(ops, deassign_device);
#endif
+#if defined(HAS_PASSTHROUGH) && defined(HAS_DEVICE_TREE)
+ set_to_dummy_if_null(ops, test_assign_dtdevice);
+ set_to_dummy_if_null(ops, assign_dtdevice);
+ set_to_dummy_if_null(ops, deassign_dtdevice);
+#endif
+
set_to_dummy_if_null(ops, resource_plug_core);
set_to_dummy_if_null(ops, resource_unplug_core);
set_to_dummy_if_null(ops, resource_plug_pci);
diff --git a/xen/xsm/flask/avc.c b/xen/xsm/flask/avc.c
index b1a4f8a..31bc702 100644
--- a/xen/xsm/flask/avc.c
+++ b/xen/xsm/flask/avc.c
@@ -600,6 +600,9 @@ void avc_audit(u32 ssid, u32 tsid, u16 tclass, u32 requested,
case AVC_AUDIT_DATA_MEMORY:
avc_printk(&buf, "pte=%#lx mfn=%#lx ", a->memory.pte, a->memory.mfn);
break;
+ case AVC_AUDIT_DATA_DTDEV:
+ avc_printk(&buf, "dtdevice=%s ", a->dtdev);
+ break;
}
avc_dump_query(&buf, ssid, tsid, tclass);
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 688ba2a..074eb81 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -589,7 +589,12 @@ static int flask_domctl(struct domain *d, int cmd)
case XEN_DOMCTL_shadow_op:
case XEN_DOMCTL_ioport_permission:
case XEN_DOMCTL_ioport_mapping:
- /* These have individual XSM hooks (drivers/passthrough/iommu.c) */
+#endif
+#ifdef HAS_PASSTHROUGH
+ /*
+ * These have individual XSM hooks
+ * (drivers/passthrough/{pci,device_tree.c)
+ */
case XEN_DOMCTL_get_device_group:
case XEN_DOMCTL_test_assign_device:
case XEN_DOMCTL_assign_device:
@@ -1231,6 +1236,62 @@ static int flask_deassign_device(struct domain *d, uint32_t machine_bdf)
}
#endif /* HAS_PASSTHROUGH && HAS_PCI */
+#if defined(HAS_PASSTHROUGH) && defined(HAS_DEVICE_TREE)
+static int flask_test_assign_dtdevice(const char *dtpath)
+{
+ u32 rsid;
+ int rc = -EPERM;
+
+ rc = security_devicetree_sid(dtpath, &rsid);
+ if ( rc )
+ return rc;
+
+ return avc_current_has_perm(rsid, SECCLASS_RESOURCE, RESOURCE__STAT_DEVICE,
+ NULL);
+}
+
+static int flask_assign_dtdevice(struct domain *d, const char *dtpath)
+{
+ u32 dsid, rsid;
+ int rc = -EPERM;
+ struct avc_audit_data ad;
+
+ rc = current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__ADD);
+ if ( rc )
+ return rc;
+
+ rc = security_devicetree_sid(dtpath, &rsid);
+ if ( rc )
+ return rc;
+
+ AVC_AUDIT_DATA_INIT(&ad, DTDEV);
+ ad.dtdev = dtpath;
+ rc = avc_current_has_perm(rsid, SECCLASS_RESOURCE, RESOURCE__ADD_DEVICE, &ad);
+ if ( rc )
+ return rc;
+
+ dsid = domain_sid(d);
+ return avc_has_perm(dsid, rsid, SECCLASS_RESOURCE, RESOURCE__USE, &ad);
+}
+
+static int flask_deassign_dtdevice(struct domain *d, const char *dtpath)
+{
+ u32 rsid;
+ int rc = -EPERM;
+
+ rc = current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__REMOVE);
+ if ( rc )
+ return rc;
+
+ rc = security_devicetree_sid(dtpath, &rsid);
+ if ( rc )
+ return rc;
+
+ return avc_current_has_perm(rsid, SECCLASS_RESOURCE, RESOURCE__REMOVE_DEVICE,
+ NULL);
+}
+#endif /* HAS_PASSTHROUGH && HAS_DEVICE_TREE */
+
#ifdef HAS_MEM_ACCESS
static int flask_vm_event_control(struct domain *d, int mode, int op)
{
@@ -1598,6 +1659,12 @@ static struct xsm_operations flask_ops = {
.deassign_device = flask_deassign_device,
#endif
+#if defined(HAS_PASSTHROUGH) && defined(HAS_DEVICE_TREE)
+ .test_assign_dtdevice = flask_test_assign_dtdevice,
+ .assign_dtdevice = flask_assign_dtdevice,
+ .deassign_dtdevice = flask_deassign_dtdevice,
+#endif
+
#ifdef HAS_MEM_ACCESS
.vm_event_control = flask_vm_event_control,
.vm_event_op = flask_vm_event_op,
diff --git a/xen/xsm/flask/include/avc.h b/xen/xsm/flask/include/avc.h
index c7a99fc..4283562 100644
--- a/xen/xsm/flask/include/avc.h
+++ b/xen/xsm/flask/include/avc.h
@@ -39,6 +39,7 @@ struct avc_audit_data {
#define AVC_AUDIT_DATA_IRQ 2
#define AVC_AUDIT_DATA_RANGE 3
#define AVC_AUDIT_DATA_MEMORY 4
+#define AVC_AUDIT_DATA_DTDEV 5
struct domain *sdom;
struct domain *tdom;
union {
@@ -52,6 +53,7 @@ struct avc_audit_data {
unsigned long pte;
unsigned long mfn;
} memory;
+ const char *dtdev;
};
};
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index 128250e..5a760c5 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -416,7 +416,7 @@ class resource
remove_iomem
# XEN_DOMCTL_get_device_group, XEN_DOMCTL_test_assign_device:
# source = domain making the hypercall
-# target = PCI device being queried
+# target = device being queried
stat_device
# XEN_DOMCTL_assign_device
add_device
--
2.1.4
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v5 p2 12/19] xen/passthrough: Extend XEN_DOMCTL_*assign_device to support DT device
2015-04-09 15:09 [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough Julien Grall
` (10 preceding siblings ...)
2015-04-09 15:09 ` [PATCH v5 p2 11/19] xen/xsm: Add helpers to check permission for device tree passthrough Julien Grall
@ 2015-04-09 15:09 ` Julien Grall
2015-04-16 15:11 ` Ian Campbell
2015-04-09 15:09 ` [PATCH v5 p2 13/19] tools/libxl: Create a per-arch function to map IRQ to a domain Julien Grall
` (6 subsequent siblings)
18 siblings, 1 reply; 48+ messages in thread
From: Julien Grall @ 2015-04-09 15:09 UTC (permalink / raw)
To: xen-devel
Cc: Wei Liu, ian.campbell, tim, Julien Grall, Ian Jackson,
stefano.stabellini, Jan Beulich
From: Julien Grall <julien.grall@linaro.org>
A device node is described by a path. It will be used to retrieved the
node in the device tree and assign the related device to the domain.
Only non-PCI protected by an IOMMU can be assigned to a guest.
Also document the behavior of XEN_DOMCTL_deassign_device in the public
headers which differ between non-PCI and PCI.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Jan Beulich <jbeulich@suse.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Changes in v5:
- Fix comment in public/domctl.h
- Remove unecessary comment in drivers/passthrough/device_tree.c
- Check d->is_dying before assigning a device (consistency with
PCI code)
- Invert the if in iommu.c in order to avoid extra return
- Add Jan's ack for non-ARM part
Changes in v4:
- Add XSM bits
- Return -ENODEV rather than -ENOSYS
- Move the if (...) into the ifdef (see iommu.c)
- Document the behavior of XEN_DOMCTL_deassign_device
- Use PCI_BUS and PCI_DEVFN2 when it's possible
- iommu_dt_device_is_assigned now returns 0 when the device is
not protected
Changes in v2:
- Use a different number for XEN_DOMCTL_assign_dt_device
---
tools/libxc/include/xenctrl.h | 10 ++++
tools/libxc/xc_domain.c | 95 ++++++++++++++++++++++++++++--
xen/drivers/passthrough/device_tree.c | 108 +++++++++++++++++++++++++++++++++-
xen/drivers/passthrough/iommu.c | 7 ++-
xen/drivers/passthrough/pci.c | 47 ++++++++++-----
xen/include/public/domctl.h | 24 +++++++-
xen/include/xen/iommu.h | 3 +
7 files changed, 269 insertions(+), 25 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index cc78ed6..a26d222 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2057,6 +2057,16 @@ int xc_deassign_device(xc_interface *xch,
uint32_t domid,
uint32_t machine_sbdf);
+int xc_assign_dt_device(xc_interface *xch,
+ uint32_t domid,
+ char *path);
+int xc_test_assign_dt_device(xc_interface *xch,
+ uint32_t domid,
+ char *path);
+int xc_deassign_dt_device(xc_interface *xch,
+ uint32_t domid,
+ char *path);
+
int xc_domain_memory_mapping(xc_interface *xch,
uint32_t domid,
unsigned long first_gfn,
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 676ec50..a6fcf14 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -1650,7 +1650,8 @@ int xc_assign_device(
domctl.cmd = XEN_DOMCTL_assign_device;
domctl.domain = domid;
- domctl.u.assign_device.machine_sbdf = machine_sbdf;
+ domctl.u.assign_device.dev = XEN_DOMCTL_DEV_PCI;
+ domctl.u.assign_device.u.pci.machine_sbdf = machine_sbdf;
return do_domctl(xch, &domctl);
}
@@ -1699,7 +1700,8 @@ int xc_test_assign_device(
domctl.cmd = XEN_DOMCTL_test_assign_device;
domctl.domain = domid;
- domctl.u.assign_device.machine_sbdf = machine_sbdf;
+ domctl.u.assign_device.dev = XEN_DOMCTL_DEV_PCI;
+ domctl.u.assign_device.u.pci.machine_sbdf = machine_sbdf;
return do_domctl(xch, &domctl);
}
@@ -1713,11 +1715,96 @@ int xc_deassign_device(
domctl.cmd = XEN_DOMCTL_deassign_device;
domctl.domain = domid;
- domctl.u.assign_device.machine_sbdf = machine_sbdf;
-
+ domctl.u.assign_device.dev = XEN_DOMCTL_DEV_PCI;
+ domctl.u.assign_device.u.pci.machine_sbdf = machine_sbdf;
+
return do_domctl(xch, &domctl);
}
+int xc_assign_dt_device(
+ xc_interface *xch,
+ uint32_t domid,
+ char *path)
+{
+ int rc;
+ size_t size = strlen(path);
+ DECLARE_DOMCTL;
+ DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+ if ( xc_hypercall_bounce_pre(xch, path) )
+ return -1;
+
+ domctl.cmd = XEN_DOMCTL_assign_device;
+ domctl.domain = (domid_t)domid;
+
+ domctl.u.assign_device.dev = XEN_DOMCTL_DEV_DT;
+ domctl.u.assign_device.u.dt.size = size;
+ set_xen_guest_handle(domctl.u.assign_device.u.dt.path, path);
+
+ rc = do_domctl(xch, &domctl);
+
+ xc_hypercall_bounce_post(xch, path);
+
+ return rc;
+}
+
+int xc_test_assign_dt_device(
+ xc_interface *xch,
+ uint32_t domid,
+ char *path)
+{
+ int rc;
+ size_t size = strlen(path);
+ DECLARE_DOMCTL;
+ DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+ if ( xc_hypercall_bounce_pre(xch, path) )
+ return -1;
+
+ domctl.cmd = XEN_DOMCTL_test_assign_device;
+ domctl.domain = (domid_t)domid;
+
+ domctl.u.assign_device.dev = XEN_DOMCTL_DEV_DT;
+ domctl.u.assign_device.u.dt.size = size;
+ set_xen_guest_handle(domctl.u.assign_device.u.dt.path, path);
+
+ rc = do_domctl(xch, &domctl);
+
+ xc_hypercall_bounce_post(xch, path);
+
+ return rc;
+}
+
+int xc_deassign_dt_device(
+ xc_interface *xch,
+ uint32_t domid,
+ char *path)
+{
+ int rc;
+ size_t size = strlen(path);
+ DECLARE_DOMCTL;
+ DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+ if ( xc_hypercall_bounce_pre(xch, path) )
+ return -1;
+
+ domctl.cmd = XEN_DOMCTL_deassign_device;
+ domctl.domain = (domid_t)domid;
+
+ domctl.u.assign_device.dev = XEN_DOMCTL_DEV_DT;
+ domctl.u.assign_device.u.dt.size = size;
+ set_xen_guest_handle(domctl.u.assign_device.u.dt.path, path);
+
+ rc = do_domctl(xch, &domctl);
+
+ xc_hypercall_bounce_post(xch, path);
+
+ return rc;
+}
+
+
+
+
int xc_domain_update_msi_irq(
xc_interface *xch,
uint32_t domid,
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 0ec4103..5d3842a 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -1,9 +1,6 @@
/*
* Code to passthrough a device tree node to a guest
*
- * TODO: This contains only the necessary code to protected device passed to
- * dom0. It will need some updates when device passthrough will is added.
- *
* Julien Grall <julien.grall@linaro.org>
* Copyright (c) 2014 Linaro Limited.
*
@@ -20,8 +17,10 @@
#include <xen/lib.h>
#include <xen/sched.h>
+#include <xen/guest_access.h>
#include <xen/iommu.h>
#include <xen/device_tree.h>
+#include <xsm/xsm.h>
static spinlock_t dtdevs_lock = SPIN_LOCK_UNLOCKED;
@@ -93,6 +92,20 @@ fail:
return rc;
}
+static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
+{
+ bool_t assigned = 0;
+
+ if ( !dt_device_is_protected(dev) )
+ return 0;
+
+ spin_lock(&dtdevs_lock);
+ assigned = !list_empty(&dev->domain_list);
+ spin_unlock(&dtdevs_lock);
+
+ return assigned;
+}
+
int iommu_dt_domain_init(struct domain *d)
{
struct hvm_iommu *hd = domain_hvm_iommu(d);
@@ -121,3 +134,92 @@ int iommu_release_dt_devices(struct domain *d)
return 0;
}
+
+int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
+ XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
+{
+ int ret;
+ struct dt_device_node *dev;
+
+ switch ( domctl->cmd )
+ {
+ case XEN_DOMCTL_assign_device:
+ ret = -ENODEV;
+ if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
+ break;
+
+ if ( unlikely(d->is_dying) )
+ {
+ ret = -EINVAL;
+ break;
+ }
+
+ ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
+ domctl->u.assign_device.u.dt.size,
+ &dev);
+ if ( ret )
+ break;
+
+ ret = xsm_assign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev));
+ if ( ret )
+ break;
+
+ ret = iommu_assign_dt_device(d, dev);
+
+ if ( ret )
+ printk(XENLOG_G_ERR "XEN_DOMCTL_assign_dt_device: assign \"%s\""
+ " to dom%u failed (%d)\n",
+ dt_node_full_name(dev), d->domain_id, ret);
+ break;
+
+ case XEN_DOMCTL_deassign_device:
+ ret = -ENODEV;
+ if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
+ break;
+
+ ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
+ domctl->u.assign_device.u.dt.size,
+ &dev);
+ if ( ret )
+ break;
+
+ ret = xsm_deassign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev));
+
+ ret = iommu_deassign_dt_device(d, dev);
+
+ if ( ret )
+ printk(XENLOG_G_ERR "XEN_DOMCTL_assign_dt_device: assign \"%s\""
+ " to dom%u failed (%d)\n",
+ dt_node_full_name(dev), d->domain_id, ret);
+ break;
+
+ case XEN_DOMCTL_test_assign_device:
+ ret = -ENODEV;
+ if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_DT )
+ break;
+
+ ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
+ domctl->u.assign_device.u.dt.size,
+ &dev);
+ if ( ret )
+ break;
+
+ ret = xsm_test_assign_dtdevice(XSM_HOOK, dt_node_full_name(dev));
+ if ( ret )
+ break;
+
+ if ( iommu_dt_device_is_assigned(dev) )
+ {
+ printk(XENLOG_G_ERR "%s already assigned.\n",
+ dt_node_full_name(dev));
+ ret = -EINVAL;
+ }
+ break;
+
+ default:
+ ret = -ENOSYS;
+ break;
+ }
+
+ return ret;
+}
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index ae42aae..06cb38f 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -335,7 +335,7 @@ int iommu_do_domctl(
struct xen_domctl *domctl, struct domain *d,
XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
{
- int ret = -ENOSYS;
+ int ret = -ENODEV;
if ( !iommu_enabled )
return -ENOSYS;
@@ -344,6 +344,11 @@ int iommu_do_domctl(
ret = iommu_do_pci_domctl(domctl, d, u_domctl);
#endif
+#ifdef HAS_DEVICE_TREE
+ if ( ret == -ENODEV )
+ ret = iommu_do_dt_domctl(domctl, d, u_domctl);
+#endif
+
return ret;
}
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index af26423..862e20f 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1500,6 +1500,7 @@ int iommu_do_pci_domctl(
u16 seg;
u8 bus, devfn;
int ret = 0;
+ uint32_t machine_sbdf;
switch ( domctl->cmd )
{
@@ -1513,8 +1514,8 @@ int iommu_do_pci_domctl(
break;
seg = domctl->u.get_device_group.machine_sbdf >> 16;
- bus = (domctl->u.get_device_group.machine_sbdf >> 8) & 0xff;
- devfn = domctl->u.get_device_group.machine_sbdf & 0xff;
+ bus = PCI_BUS(domctl->u.get_device_group.machine_sbdf);
+ devfn = PCI_DEVFN2(domctl->u.get_device_group.machine_sbdf);
max_sdevs = domctl->u.get_device_group.max_sdevs;
sdevs = domctl->u.get_device_group.sdev_array;
@@ -1536,13 +1537,19 @@ int iommu_do_pci_domctl(
break;
case XEN_DOMCTL_test_assign_device:
- ret = xsm_test_assign_device(XSM_HOOK, domctl->u.assign_device.machine_sbdf);
+ ret = -ENODEV;
+ if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )
+ break;
+
+ machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf;
+
+ ret = xsm_test_assign_device(XSM_HOOK, machine_sbdf);
if ( ret )
break;
- seg = domctl->u.assign_device.machine_sbdf >> 16;
- bus = (domctl->u.assign_device.machine_sbdf >> 8) & 0xff;
- devfn = domctl->u.assign_device.machine_sbdf & 0xff;
+ seg = machine_sbdf >> 16;
+ bus = PCI_BUS(machine_sbdf);
+ devfn = PCI_DEVFN2(machine_sbdf);
if ( device_assigned(seg, bus, devfn) )
{
@@ -1554,19 +1561,25 @@ int iommu_do_pci_domctl(
break;
case XEN_DOMCTL_assign_device:
+ ret = -ENODEV;
+ if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )
+ break;
+
if ( unlikely(d->is_dying) )
{
ret = -EINVAL;
break;
}
- ret = xsm_assign_device(XSM_HOOK, d, domctl->u.assign_device.machine_sbdf);
+ machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf;
+
+ ret = xsm_assign_device(XSM_HOOK, d, machine_sbdf);
if ( ret )
break;
- seg = domctl->u.assign_device.machine_sbdf >> 16;
- bus = (domctl->u.assign_device.machine_sbdf >> 8) & 0xff;
- devfn = domctl->u.assign_device.machine_sbdf & 0xff;
+ seg = machine_sbdf >> 16;
+ bus = PCI_BUS(machine_sbdf);
+ devfn = PCI_DEVFN2(machine_sbdf);
ret = device_assigned(seg, bus, devfn) ?:
assign_device(d, seg, bus, devfn);
@@ -1582,13 +1595,19 @@ int iommu_do_pci_domctl(
break;
case XEN_DOMCTL_deassign_device:
- ret = xsm_deassign_device(XSM_HOOK, d, domctl->u.assign_device.machine_sbdf);
+ ret = -ENODEV;
+ if ( domctl->u.assign_device.dev != XEN_DOMCTL_DEV_PCI )
+ break;
+
+ machine_sbdf = domctl->u.assign_device.u.pci.machine_sbdf;
+
+ ret = xsm_deassign_device(XSM_HOOK, d, machine_sbdf);
if ( ret )
break;
- seg = domctl->u.assign_device.machine_sbdf >> 16;
- bus = (domctl->u.assign_device.machine_sbdf >> 8) & 0xff;
- devfn = domctl->u.assign_device.machine_sbdf & 0xff;
+ seg = machine_sbdf >> 16;
+ bus = PCI_BUS(machine_sbdf);
+ devfn = PCI_DEVFN2(machine_sbdf);
spin_lock(&pcidevs_lock);
ret = deassign_device(d, seg, bus, devfn);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index d94c647..993944e 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -475,12 +475,30 @@ typedef struct xen_domctl_sendtrigger xen_domctl_sendtrigger_t;
DEFINE_XEN_GUEST_HANDLE(xen_domctl_sendtrigger_t);
-/* Assign PCI device to HVM guest. Sets up IOMMU structures. */
+/* Assign a device to a guest. Sets up IOMMU structures. */
/* XEN_DOMCTL_assign_device */
/* XEN_DOMCTL_test_assign_device */
-/* XEN_DOMCTL_deassign_device */
+/*
+ * XEN_DOMCTL_deassign_device: The behavior of this DOMCTL differs
+ * between the different type of device:
+ * - PCI device (XEN_DOMCTL_DEV_PCI) will be reassigned to DOM0
+ * - DT device (XEN_DOMCTL_DT_PCI) will left unassigned. DOM0
+ * will have to call XEN_DOMCTL_assign_device in order to use the
+ * device.
+ */
+#define XEN_DOMCTL_DEV_PCI 0
+#define XEN_DOMCTL_DEV_DT 1
struct xen_domctl_assign_device {
- uint32_t machine_sbdf; /* machine PCI ID of assigned device */
+ uint32_t dev; /* XEN_DOMCTL_DEV_* */
+ union {
+ struct {
+ uint32_t machine_sbdf; /* machine PCI ID of assigned device */
+ } pci;
+ struct {
+ uint32_t size; /* Length of the path */
+ XEN_GUEST_HANDLE_64(char) path; /* path to the device tree node */
+ } dt;
+ } u;
};
typedef struct xen_domctl_assign_device xen_domctl_assign_device_t;
DEFINE_XEN_GUEST_HANDLE(xen_domctl_assign_device_t);
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index d9c9ede..b30bf41 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -119,6 +119,9 @@ int iommu_deassign_dt_device(struct domain *d, struct dt_device_node *dev);
int iommu_dt_domain_init(struct domain *d);
int iommu_release_dt_devices(struct domain *d);
+int iommu_do_dt_domctl(struct xen_domctl *, struct domain *,
+ XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
+
#endif /* HAS_DEVICE_TREE */
struct page_info;
--
2.1.4
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v5 p2 13/19] tools/libxl: Create a per-arch function to map IRQ to a domain
2015-04-09 15:09 [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough Julien Grall
` (11 preceding siblings ...)
2015-04-09 15:09 ` [PATCH v5 p2 12/19] xen/passthrough: Extend XEN_DOMCTL_*assign_device to support DT device Julien Grall
@ 2015-04-09 15:09 ` Julien Grall
2015-04-16 15:12 ` Ian Campbell
2015-04-09 15:09 ` [PATCH v5 p2 14/19] tools/libxl: Check if fdt_{first, next}_subnode are present in libfdt Julien Grall
` (5 subsequent siblings)
18 siblings, 1 reply; 48+ messages in thread
From: Julien Grall @ 2015-04-09 15:09 UTC (permalink / raw)
To: xen-devel
Cc: Wei Liu, ian.campbell, tim, Julien Grall, Ian Jackson,
stefano.stabellini
From: Julien Grall <julien.grall@linaro.org>
ARM and x86 use a different hypercall to map an IRQ to a domain.
The hypercall to give IRQ permission to the domain has also been moved
to be an x86 specific function as ARM guest won't be able to manage the IRQ.
We may want to support it later.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Changes in v5:
- Use the new function xc_domain_bind_pt_spi_irq
- Fix typoes
Changes in v4:
- Patch added
---
tools/libxl/libxl_arch.h | 4 ++++
tools/libxl/libxl_arm.c | 5 +++++
tools/libxl/libxl_create.c | 6 ++----
tools/libxl/libxl_x86.c | 13 +++++++++++++
4 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
index cae64c0..77b1f2a 100644
--- a/tools/libxl/libxl_arch.h
+++ b/tools/libxl/libxl_arch.h
@@ -39,4 +39,8 @@ int libxl__arch_vnuma_build_vmemrange(libxl__gc *gc,
uint32_t domid,
libxl_domain_build_info *b_info,
libxl__domain_build_state *state);
+
+/* arch specific irq map function */
+int libxl__arch_domain_map_irq(libxl__gc *gc, uint32_t domid, int irq);
+
#endif
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 5a5cb3f..aa302fd 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -742,6 +742,11 @@ int libxl__arch_vnuma_build_vmemrange(libxl__gc *gc,
return libxl__vnuma_build_vmemrange_pv_generic(gc, domid, info, state);
}
+int libxl__arch_domain_map_irq(libxl__gc *gc, uint32_t domid, int irq)
+{
+ return xc_domain_bind_pt_spi_irq(CTX->xch, domid, irq);
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index e5a343f..15b464e 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1205,11 +1205,9 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
LOG(DEBUG, "dom%d irq %d", domid, irq);
- ret = irq >= 0 ? xc_physdev_map_pirq(CTX->xch, domid, irq, &irq)
+ ret = irq >= 0 ? libxl__arch_domain_map_irq(gc, domid, irq)
: -EOVERFLOW;
- if (!ret)
- ret = xc_domain_irq_permission(CTX->xch, domid, irq, 1);
- if (ret < 0) {
+ if (ret) {
LOGE(ERROR, "failed give dom%d access to irq %d", domid, irq);
ret = ERROR_FAIL;
goto error_out;
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index 5e9a8d2..ed2bd38 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -424,6 +424,19 @@ out:
return rc;
}
+int libxl__arch_domain_map_irq(libxl__gc *gc, uint32_t domid, int irq)
+{
+ int ret;
+
+ ret = xc_physdev_map_pirq(CTX->xch, domid, irq, &irq);
+ if (ret)
+ return ret;
+
+ ret = xc_domain_irq_permission(CTX->xch, domid, irq, 1);
+
+ return ret;
+}
+
/*
* Local variables:
* mode: C
--
2.1.4
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v5 p2 14/19] tools/libxl: Check if fdt_{first, next}_subnode are present in libfdt
2015-04-09 15:09 [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough Julien Grall
` (12 preceding siblings ...)
2015-04-09 15:09 ` [PATCH v5 p2 13/19] tools/libxl: Create a per-arch function to map IRQ to a domain Julien Grall
@ 2015-04-09 15:09 ` Julien Grall
2015-04-16 15:13 ` Ian Campbell
2015-04-09 15:09 ` [PATCH v5 p2 15/19] tools/(lib)xl: Add partial device tree support for ARM Julien Grall
` (4 subsequent siblings)
18 siblings, 1 reply; 48+ messages in thread
From: Julien Grall @ 2015-04-09 15:09 UTC (permalink / raw)
To: xen-devel
Cc: Wei Liu, ian.campbell, tim, Julien Grall, Ian Jackson,
stefano.stabellini
From: Julien Grall <julien.grall@linaro.org>
The functions fdt_{fisrt,next}_subnode may not be available because:
* It has been introduced in 2013 => Doesn't work on Wheezy
* The prototype exists but the functions are not exposed. Don't ask
why...
The later has been fixed recently in the dtc repo [1]
When the functions are not available, implement our own in order to use
them in a following patch.
[1] git://git.kernel.org/pub/scm/utils/dtc/dtc.git
commit a4b093f7366fdb429ca1781144d3985fa50d0fbb
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
tools/configure needs to be regenerate as this patch is modifying
tools/configure.ac
Changes in v5:
- Add Ian's Signed-off-by for the license part and update the
license
- Rename libxl_fdt to libxl_libfdt_compat.c
Changes in v4:
- Patch added
---
tools/config.h.in | 6 +++
tools/configure.ac | 5 +++
tools/libxl/Makefile | 2 +-
tools/libxl/libxl_libfdt_compat.c | 92 +++++++++++++++++++++++++++++++++++++++
4 files changed, 104 insertions(+), 1 deletion(-)
create mode 100644 tools/libxl/libxl_libfdt_compat.c
diff --git a/tools/config.h.in b/tools/config.h.in
index 2a0ae48..06f8b6c 100644
--- a/tools/config.h.in
+++ b/tools/config.h.in
@@ -3,6 +3,12 @@
/* Blktap2 enabled */
#undef HAVE_BLKTAP2
+/* Define to 1 if you have the `fdt_first_subnode' function. */
+#undef HAVE_FDT_FIRST_SUBNODE
+
+/* Define to 1 if you have the `fdt_next_subnode' function. */
+#undef HAVE_FDT_NEXT_SUBNODE
+
/* Define to 1 if you have the <inttypes.h> header file. */
#undef HAVE_INTTYPES_H
diff --git a/tools/configure.ac b/tools/configure.ac
index d31c2f3..cc13336 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -355,6 +355,11 @@ AC_SUBST(libiconv)
case "$host_cpu" in
arm*|aarch64)
AC_CHECK_LIB([fdt], [fdt_create], [], [AC_MSG_ERROR([Could not find libfdt])])
+
+# The functions fdt_{first,next}_subnode may not be available because:
+# * It has been introduced in 2013 => Doesn't work on Wheezy
+# * The prototype exists but the functions are not exposed. Don't ask why...
+AC_CHECK_FUNCS([fdt_first_subnode fdt_next_subnode])
esac
# Checks for header files.
diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
index 1b16598..2afb146 100644
--- a/tools/libxl/Makefile
+++ b/tools/libxl/Makefile
@@ -59,7 +59,7 @@ endif
LIBXL_OBJS-y += libxl_remus_device.o libxl_remus_disk_drbd.o
LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o libxl_psr.o
-LIBXL_OBJS-$(CONFIG_ARM) += libxl_nocpuid.o libxl_arm.o
+LIBXL_OBJS-$(CONFIG_ARM) += libxl_nocpuid.o libxl_arm.o libxl_libfdt_compat.o
ifeq ($(CONFIG_NetBSD),y)
LIBXL_OBJS-y += libxl_netbsd.o
diff --git a/tools/libxl/libxl_libfdt_compat.c b/tools/libxl/libxl_libfdt_compat.c
new file mode 100644
index 0000000..d0519f9
--- /dev/null
+++ b/tools/libxl/libxl_libfdt_compat.c
@@ -0,0 +1,92 @@
+/*
+ * Copyright (C) 2006 David Gibson, IBM Corporation.
+ *
+ * This file is part of libxl, and was originally taken from libfdt.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Lesser General Public License for more details.
+ *
+ * Additionally, this particular file is dual licensed. That is,
+ * alternatively, at your option:
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ * copyright notice, this list of conditions and the following
+ * disclaimer.
+ * 2. Redistributions in binary form must reproduce the above
+ * copyright notice, this list of conditions and the following
+ * disclaimer in the documentation and/or other materials
+ * provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
+ * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE,
+ * EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * Note that this applies only to this file, and other files with a
+ * similar notice. Also, note that when the same code is distributed
+ * along with the rest of libxl, you must comply with the terms of the
+ * LGPLv2.1 for the whole of libxl including this file.
+ *
+ * The intent is to permit, in particular, upstream libfdt to
+ * incorporate improvements to this file within upstream libfdt. At
+ * the time of writing, upstream libfdt is dual licensed: 2-clause BSD
+ * (as above) and GPLv2-or-later. The 2-clause BSD licence is
+ * compatible with both GPLv2-or-later and LGPLv2.1-only; this permits
+ * copying in both directions, and the optional licence upgrade to a
+ * copyleft licence by libdft upstream or the Xen Project,
+ * respectively.
+ */
+
+#include <libfdt.h>
+
+#ifndef HAVE_FDT_FIRST_SUBNODE
+int fdt_first_subnode(const void *fdt, int offset)
+{
+ int depth = 0;
+
+ offset = fdt_next_node(fdt, offset, &depth);
+ if (offset < 0 || depth != 1)
+ return -FDT_ERR_NOTFOUND;
+
+ return offset;
+}
+#endif
+
+#ifndef HAVE_FDT_NEXT_SUBNODE
+int fdt_next_subnode(const void *fdt, int offset)
+{
+ int depth = 1;
+
+ /*
+ * With respect to the parent, the depth of the next subnode will be
+ * the same as the last.
+ */
+ do {
+ offset = fdt_next_node(fdt, offset, &depth);
+ if (offset < 0 || depth < 1)
+ return -FDT_ERR_NOTFOUND;
+ } while (depth > 1);
+
+ return offset;
+}
+#endif
--
2.1.4
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v5 p2 15/19] tools/(lib)xl: Add partial device tree support for ARM
2015-04-09 15:09 [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough Julien Grall
` (13 preceding siblings ...)
2015-04-09 15:09 ` [PATCH v5 p2 14/19] tools/libxl: Check if fdt_{first, next}_subnode are present in libfdt Julien Grall
@ 2015-04-09 15:09 ` Julien Grall
2015-04-09 16:13 ` Ian Jackson
2015-04-16 15:17 ` Ian Campbell
2015-04-09 15:09 ` [PATCH v5 p2 16/19] tools/libxl: arm: Use an higher value for the GIC phandle Julien Grall
` (3 subsequent siblings)
18 siblings, 2 replies; 48+ messages in thread
From: Julien Grall @ 2015-04-09 15:09 UTC (permalink / raw)
To: xen-devel
Cc: Wei Liu, ian.campbell, tim, Julien Grall, Ian Jackson,
stefano.stabellini
From: Julien Grall <julien.grall@linaro.org>
Let the user to pass additional nodes to the guest device tree. For
this purpose, everything in the node /passthrough from the partial
device tree will be copied into the guest device tree.
The node /aliases will be also copied to allow the user to define
aliases which can be used by the guest kernel.
A simple partial device tree will look like:
/dts-v1/;
/ {
#address-cells = <2>;
#size-cells = <2>;
passthrough {
compatible = "simple-bus";
ranges;
#address-cells = <2>;
#size-cells = <2>;
/* List of your nodes */
}
};
Note that:
* The interrupt-parent property will be added by the toolstack in
the root node
* The properties compatible, ranges, #address-cells and #size-cells
in /passthrough are mandatory.
The helpers provided by the libfdt don't perform all the necessary
security check on a given device tree. Therefore, only trusted device
tree should be used.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
An example of the partial device tree, as long as how to passthrough
a non-pci device will be added to the tree in a follow-up patch.
A new LIBXL_HAVE_* will be added in the patch which add support for
non-PCI passthrough as both are tight.
Changes in v5:
- Add a warning in the IDL
- Remove the requirement to use only the version 17 of the FDT
format.
Changes in v4:
- Mark the option as unsafe
- The _fdt_* helpers has been moved in a separate patch/file.
Only the prototype is declared
- The partial DT is considered valid. Remove some security check
which make the code cleaner
- Typoes
Changes in v3:
- Patch added
---
docs/man/xl.cfg.pod.5 | 10 +++
tools/libxl/libxl_arm.c | 157 ++++++++++++++++++++++++++++++++++++++++++++
tools/libxl/libxl_types.idl | 4 ++
tools/libxl/xl_cmdimpl.c | 1 +
4 files changed, 172 insertions(+)
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index f936dfc..ad95a9a 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -460,6 +460,16 @@ not emulated.
Specify that this domain is a driver domain. This enables certain
features needed in order to run a driver domain.
+=item B<device_tree=PATH>
+
+Specify a partial device tree (compiled via the Device Tree Compiler).
+Everything under the node "/passthrough" will be copied into the guest
+device tree. For convenience, the node "/aliases" is also copied to allow
+the user to defined aliases which can be used by the guest kernel.
+
+Given the complexity of verifying the validity of a device tree, this
+option should only be used with trusted device tree.
+
=back
=head2 Devices
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index aa302fd..2ce7e23 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -542,6 +542,142 @@ out:
}
}
+static int check_partial_fdt(libxl__gc *gc, void *fdt, size_t size)
+{
+ int r;
+
+ if (fdt_magic(fdt) != FDT_MAGIC) {
+ LOG(ERROR, "Partial FDT is not a valid Flat Device Tree");
+ return ERROR_FAIL;
+ }
+
+ r = fdt_check_header(fdt);
+ if (r) {
+ LOG(ERROR, "Failed to check the partial FDT (%d)", r);
+ return ERROR_FAIL;
+ }
+
+ if (fdt_totalsize(fdt) > size) {
+ LOG(ERROR, "Partial FDT totalsize is too big");
+ return ERROR_FAIL;
+ }
+
+ return 0;
+}
+
+static int copy_properties(libxl__gc *gc, void *fdt, void *pfdt,
+ int nodeoff)
+{
+ int propoff, nameoff, r;
+ const struct fdt_property *prop;
+
+ for (propoff = fdt_first_property_offset(pfdt, nodeoff);
+ propoff >= 0;
+ propoff = fdt_next_property_offset(pfdt, propoff)) {
+
+ if (!(prop = fdt_get_property_by_offset(pfdt, propoff, NULL))) {
+ return -FDT_ERR_INTERNAL;
+ }
+
+ nameoff = fdt32_to_cpu(prop->nameoff);
+ r = fdt_property(fdt, fdt_string(pfdt, nameoff),
+ prop->data, fdt32_to_cpu(prop->len));
+ if (r) return r;
+ }
+
+ /* FDT_ERR_NOTFOUND => There is no more properties for this node */
+ return (propoff != -FDT_ERR_NOTFOUND)? propoff : 0;
+}
+
+/*
+ * These functions are defined by libfdt or libxl_fdt.c if it's not
+ * present on the former.
+ */
+int fdt_next_subnode(const void *fdt, int offset);
+int fdt_first_subnode(const void *fdt, int offset);
+
+/* Copy a node from the partial device tree to the guest device tree */
+static int copy_node(libxl__gc *gc, void *fdt, void *pfdt,
+ int nodeoff, int depth)
+{
+ int r;
+
+ r = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL));
+ if (r) return r;
+
+ r = copy_properties(gc, fdt, pfdt, nodeoff);
+ if (r) return r;
+
+ for (nodeoff = fdt_first_subnode(pfdt, nodeoff);
+ nodeoff >= 0;
+ nodeoff = fdt_next_subnode(pfdt, nodeoff)) {
+ r = copy_node(gc, fdt, pfdt, nodeoff, depth + 1);
+ if (r) return r;
+ }
+
+ if (nodeoff != -FDT_ERR_NOTFOUND)
+ return nodeoff;
+
+ r = fdt_end_node(fdt);
+ if (r) return r;
+
+ return 0;
+}
+
+static int copy_node_by_path(libxl__gc *gc, const char *path,
+ void *fdt, void *pfdt)
+{
+ int nodeoff, r;
+ const char *name = strrchr(path, '/');
+
+ if (!name)
+ return -FDT_ERR_INTERNAL;
+
+ name++;
+
+ /*
+ * The FDT function to look at a node doesn't take into account the
+ * unit (i.e anything after @) when search by name. Check if the
+ * name exactly matches.
+ */
+ nodeoff = fdt_path_offset(pfdt, path);
+ if (nodeoff < 0)
+ return nodeoff;
+
+ if (strcmp(fdt_get_name(pfdt, nodeoff, NULL), name))
+ return -FDT_ERR_NOTFOUND;
+
+ r = copy_node(gc, fdt, pfdt, nodeoff, 0);
+ if (r) return r;
+
+ return 0;
+}
+
+/*
+ * The partial device tree is not copied entirely. Only the relevant bits are
+ * copied to the guest device tree:
+ * - /passthrough node
+ * - /aliases node
+ */
+static int copy_partial_fdt(libxl__gc *gc, void *fdt, void *pfdt)
+{
+ int r;
+
+ r = copy_node_by_path(gc, "/passthrough", fdt, pfdt);
+ if (r < 0) {
+ LOG(ERROR, "Can't copy the node \"/passthrough\" from the partial FDT");
+ return r;
+ }
+
+ r = copy_node_by_path(gc, "/aliases", fdt, pfdt);
+ if (r < 0 && r != -FDT_ERR_NOTFOUND) {
+ LOG(ERROR, "Can't copy the node \"/aliases\" from the partial FDT");
+ return r;
+ }
+
+ return 0;
+}
+
#define FDT_MAX_SIZE (1<<20)
int libxl__arch_domain_init_hw_description(libxl__gc *gc,
@@ -550,8 +686,10 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc,
struct xc_dom_image *dom)
{
void *fdt = NULL;
+ void *pfdt = NULL;
int rc, res;
size_t fdt_size = 0;
+ int pfdt_size = 0;
const libxl_version_info *vers;
const struct arch_info *ainfo;
@@ -571,6 +709,22 @@ int libxl__arch_domain_init_hw_description(libxl__gc *gc,
vers->xen_version_major, vers->xen_version_minor);
LOG(DEBUG, " - vGIC version: %s", gicv_to_string(xc_config->gic_version));
+ if (info->device_tree) {
+ LOG(DEBUG, " - Partial device tree provided: %s", info->device_tree);
+
+ rc = libxl_read_file_contents(CTX, info->device_tree,
+ &pfdt, &pfdt_size);
+ if (rc) {
+ LOGEV(ERROR, rc, "failed to read the partial device file %s",
+ info->device_tree);
+ return ERROR_FAIL;
+ }
+ libxl__ptr_add(gc, pfdt);
+
+ if (check_partial_fdt(gc, pfdt, pfdt_size))
+ return ERROR_FAIL;
+ }
+
/*
* Call "call" handling FDT_ERR_*. Will either:
* - loop back to retry_resize
@@ -637,6 +791,9 @@ next_resize:
FDT( make_timer_node(gc, fdt, ainfo) );
FDT( make_hypervisor_node(gc, fdt, vers) );
+ if (pfdt)
+ FDT( copy_partial_fdt(gc, fdt, pfdt) );
+
FDT( fdt_end_node(fdt) );
FDT( fdt_finish(fdt) );
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 0866433..9aada5a 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -411,6 +411,10 @@ libxl_domain_build_info = Struct("domain_build_info",[
("kernel", string),
("cmdline", string),
("ramdisk", string),
+ # Given the complexity of verifying the validity of a device tree,
+ # libxl doesn't do any security check on it. It's the responsability
+ # of the caller to provide only trusted device tree.
+ ("device_tree", string),
("u", KeyedUnion(None, libxl_domain_type, "type",
[("hvm", Struct(None, [("firmware", string),
("bios", libxl_bios_type),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 394b55d..c2415ba 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1379,6 +1379,7 @@ static void parse_config_data(const char *config_source,
xlu_cfg_replace_string (config, "kernel", &b_info->kernel, 0);
xlu_cfg_replace_string (config, "ramdisk", &b_info->ramdisk, 0);
+ xlu_cfg_replace_string (config, "device_tree", &b_info->device_tree, 0);
b_info->cmdline = parse_cmdline(config);
xlu_cfg_get_defbool(config, "driver_domain", &c_info->driver_domain, 0);
--
2.1.4
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v5 p2 16/19] tools/libxl: arm: Use an higher value for the GIC phandle
2015-04-09 15:09 [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough Julien Grall
` (14 preceding siblings ...)
2015-04-09 15:09 ` [PATCH v5 p2 15/19] tools/(lib)xl: Add partial device tree support for ARM Julien Grall
@ 2015-04-09 15:09 ` Julien Grall
2015-04-09 16:17 ` Ian Jackson
2015-04-09 15:09 ` [PATCH v5 p2 17/19] libxl: Add support for Device Tree passthrough Julien Grall
` (2 subsequent siblings)
18 siblings, 1 reply; 48+ messages in thread
From: Julien Grall @ 2015-04-09 15:09 UTC (permalink / raw)
To: xen-devel
Cc: Wei Liu, ian.campbell, tim, Julien Grall, Ian Jackson,
stefano.stabellini
From: Julien Grall <julien.grall@linaro.org>
The partial device tree may contains phandle. The Device Tree Compiler
tends to allocate the phandle from 1.
Reserve the ID 65000 for the GIC phandle. I think we can safely assume
that the partial device tree will never contain a such ID.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
To allocate dynamically the phandle, we would need to fill in
post-hoc (like we do with e.g the initramfs location) the
#interrupt-parent in "/". That would also require some refactoring
in the code to pass the phandle every time.
Defer this solution to a follow-up in order as having 65000 would be
very unlikely.
Changes in v5:
- Add Ian's Ack.
Changes in v3:
- Patch added
---
tools/libxl/libxl_arm.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 2ce7e23..cf1379d 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -80,10 +80,11 @@ static struct arch_info {
{"xen-3.0-aarch64", "arm,armv8-timer", "arm,armv8" },
};
-enum {
- PHANDLE_NONE = 0,
- PHANDLE_GIC,
-};
+/*
+ * The device tree compiler (DTC) is allocating the phandle from 1 to
+ * onwards. Reserve a high value for the GIC phandle.
+ */
+#define PHANDLE_GIC (65000)
typedef uint32_t be32;
typedef be32 gic_interrupt[3];
--
2.1.4
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v5 p2 17/19] libxl: Add support for Device Tree passthrough
2015-04-09 15:09 [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough Julien Grall
` (15 preceding siblings ...)
2015-04-09 15:09 ` [PATCH v5 p2 16/19] tools/libxl: arm: Use an higher value for the GIC phandle Julien Grall
@ 2015-04-09 15:09 ` Julien Grall
2015-04-09 16:14 ` Ian Jackson
2015-04-16 15:19 ` Ian Campbell
2015-04-09 15:09 ` [PATCH v5 p2 18/19] xl: Add new option dtdev Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 19/19] docs/misc: arm: Add documentation about Device Tree passthrough Julien Grall
18 siblings, 2 replies; 48+ messages in thread
From: Julien Grall @ 2015-04-09 15:09 UTC (permalink / raw)
To: xen-devel
Cc: Wei Liu, ian.campbell, tim, Julien Grall, Ian Jackson,
stefano.stabellini
From: Julien Grall <julien.grall@linaro.org>
On ARM, every non-PCI device are described in the device tree. Each of
them can be found via a path.
This patch introduces a very basic support, only the IOMMU will be set
up correctly. The user will have to:
- Describe the device in the partial device tree
- Map manually MMIO/IRQ
This is a first approach, that will allow to have a basic Device Tree
passthrough support in Xen. This could be improved later.
Furthermore add LIBXL_HAVE_DEVICETREE_PASSTHROUGH to indicate we
support Device Tree passthrough and partial device tree (introduced by a
previous patch).
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Changes in v5:
- Replace some "non-PCI" to "Device Tree"
Changes in v4:
- Add LIBXL_HAVE_DEVICTREE_PASSTHROUGH to indicate we support
non-PCI passthrough. This is also used in order to indicate
partial device tree is supported
- Remove libxl_dtdev.c as it contains only a 2 lines functions
and call directly xc_* from libxl_create.c
- Introduce domcreate_attach_dtdev
Changes in v3:
- Dynamic allocation has been dropped
- Rework the commit message in accordance with the previous
item
Changes in v2:
- Get DT infos earlier
- Allocate/map IRQ in libxl__arch_domain_create rather than in
libxl__device_dt_add
- Use VIRQ rather than the PIRQ to construct the interrupts
properties of the device tree
- Correct cpumask in make_dtdev_node. We allow the interrupt to
be used on the 8 CPUs
- Fix LOGE when we map the MMIO region in the guest in
libxl__device_dt_add. The domain and the IRQ were inverted
- Calculate the number of SPIs to configure the VGIC
- xc_physdev_dtdev_* helpers has been renamed to xc_dtdev_*
- Rename libxl_device_dt to libxl_device_dtdev
---
tools/libxl/libxl.h | 6 ++++++
tools/libxl/libxl_create.c | 32 ++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 5 +++++
tools/libxl/libxl_types.idl | 5 +++++
4 files changed, 48 insertions(+)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 6bc75c5..029354d 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -179,6 +179,12 @@
#define LIBXL_HAVE_BUILDINFO_HVM_MMIO_HOLE_MEMKB 1
/*
+ * libxl_domain_build_info has device_tree and libxl_device_dtdev
+ * exists. This mean Device Tree passthrough is supported for ARM
+ */
+#define LIBXL_HAVE_DEVICETREE_PASSTHROUGH 1
+
+/*
* libxl ABI compatibility
*
* The only guarantee which libxl makes regarding ABI compatibility
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 15b464e..39c828b 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -751,6 +751,8 @@ static void domcreate_attach_vtpms(libxl__egc *egc, libxl__multidev *multidev,
int ret);
static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *aodevs,
int ret);
+static void domcreate_attach_dtdev(libxl__egc *egc,
+ libxl__domain_create_state *dcs);
static void domcreate_console_available(libxl__egc *egc,
libxl__domain_create_state *dcs);
@@ -1444,6 +1446,36 @@ static void domcreate_attach_pci(libxl__egc *egc, libxl__multidev *multidev,
}
}
+ domcreate_attach_dtdev(egc, dcs);
+ return;
+
+error_out:
+ assert(ret);
+ domcreate_complete(egc, dcs, ret);
+}
+
+static void domcreate_attach_dtdev(libxl__egc *egc,
+ libxl__domain_create_state *dcs)
+{
+ STATE_AO_GC(dcs->ao);
+ int i;
+ int ret;
+ int domid = dcs->guest_domid;
+
+ /* convenience aliases */
+ libxl_domain_config *const d_config = dcs->guest_config;
+
+ for (i = 0; i < d_config->num_dtdevs; i++) {
+ const libxl_device_dtdev *dtdev = &d_config->dtdevs[i];
+
+ LOG(DEBUG, "Assign device \"%s\" to dom%u", dtdev->path, domid);
+ ret = xc_assign_dt_device(CTX->xch, domid, dtdev->path);
+ if (ret < 0) {
+ LOG(ERROR, "xc_assign_dtdevice failed: %d\n", ret);
+ goto error_out;
+ }
+ }
+
domcreate_console_available(egc, dcs);
domcreate_complete(egc, dcs, 0);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 3aba221..5a7e791 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1199,6 +1199,11 @@ _hidden int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
libxl_device_pci *pcidev, int num);
_hidden int libxl__device_pci_destroy_all(libxl__gc *gc, uint32_t domid);
+/* from libxl_dtdev */
+
+_hidden int libxl__device_dt_add(libxl__gc *gc, uint32_t domid,
+ const libxl_device_dtdev *dtdev);
+
/*----- xswait: wait for a xenstore node to be suitable -----*/
typedef struct libxl__xswait_state libxl__xswait_state;
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 9aada5a..4182cbd 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -537,6 +537,10 @@ libxl_device_pci = Struct("device_pci", [
("seize", bool),
])
+libxl_device_dtdev = Struct("device_dtdev", [
+ ("path", string),
+ ])
+
libxl_device_vtpm = Struct("device_vtpm", [
("backend_domid", libxl_domid),
("backend_domname", string),
@@ -563,6 +567,7 @@ libxl_domain_config = Struct("domain_config", [
("disks", Array(libxl_device_disk, "num_disks")),
("nics", Array(libxl_device_nic, "num_nics")),
("pcidevs", Array(libxl_device_pci, "num_pcidevs")),
+ ("dtdevs", Array(libxl_device_dtdev, "num_dtdevs")),
("vfbs", Array(libxl_device_vfb, "num_vfbs")),
("vkbs", Array(libxl_device_vkb, "num_vkbs")),
("vtpms", Array(libxl_device_vtpm, "num_vtpms")),
--
2.1.4
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v5 p2 18/19] xl: Add new option dtdev
2015-04-09 15:09 [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough Julien Grall
` (16 preceding siblings ...)
2015-04-09 15:09 ` [PATCH v5 p2 17/19] libxl: Add support for Device Tree passthrough Julien Grall
@ 2015-04-09 15:09 ` Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 19/19] docs/misc: arm: Add documentation about Device Tree passthrough Julien Grall
18 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2015-04-09 15:09 UTC (permalink / raw)
To: xen-devel
Cc: Wei Liu, ian.campbell, tim, Julien Grall, Ian Jackson,
stefano.stabellini
From: Julien Grall <julien.grall@linaro.org>
The option "dtdev" will be used to passthrough a device described
in the device tree to a guest.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Changes in v5:
- Drop "non-PCI" in the commit message
- Add Ian's ack
Changes in v4:
- Typoes in the documentation
- Wrap the line in xl_cmdimpl.c
Changes in v2:
- libxl_device_dt has been rename to libxl_device_dtdev
- use xrealloc instead of realloc
---
docs/man/xl.cfg.pod.5 | 5 +++++
tools/libxl/xl_cmdimpl.c | 22 +++++++++++++++++++++-
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index ad95a9a..8357ebe 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -786,6 +786,11 @@ More information about Xen gfx_passthru feature is available
on the XenVGAPassthrough L<http://wiki.xen.org/wiki/XenVGAPassthrough>
wiki page.
+=item B<dtdev=[ "DTDEV_PATH", "DTDEV_PATH", ... ]>
+
+Specifies the host device tree nodes to passthrough to this guest. Each
+DTDEV_PATH is the absolute path in the device tree.
+
=item B<ioports=[ "IOPORT_RANGE", "IOPORT_RANGE", ... ]>
Allow guest to access specific legacy I/O ports. Each B<IOPORT_RANGE>
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c2415ba..ed82b6a 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1169,7 +1169,7 @@ static void parse_config_data(const char *config_source,
long l, vcpus = 0;
XLU_Config *config;
XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms;
- XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian;
+ XLU_ConfigList *channels, *ioports, *irqs, *iomem, *viridian, *dtdevs;
int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian;
int pci_power_mgmt = 0;
int pci_msitranslate = 0;
@@ -1941,6 +1941,26 @@ skip_vfb:
libxl_defbool_set(&b_info->u.pv.e820_host, true);
}
+ if (!xlu_cfg_get_list (config, "dtdev", &dtdevs, 0, 0)) {
+ d_config->num_dtdevs = 0;
+ d_config->dtdevs = NULL;
+ for (i = 0; (buf = xlu_cfg_get_listitem(dtdevs, i)) != NULL; i++) {
+ libxl_device_dtdev *dtdev;
+
+ d_config->dtdevs = xrealloc(d_config->dtdevs,
+ sizeof (libxl_device_dtdev) * (i + 1));
+ dtdev = d_config->dtdevs + d_config->num_dtdevs;
+ libxl_device_dtdev_init(dtdev);
+
+ dtdev->path = strdup(buf);
+ if (dtdev->path == NULL) {
+ fprintf(stderr, "unable to duplicate string for dtdevs\n");
+ exit(-1);
+ }
+ d_config->num_dtdevs++;
+ }
+ }
+
switch (xlu_cfg_get_list(config, "cpuid", &cpuids, 0, 1)) {
case 0:
{
--
2.1.4
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH v5 p2 19/19] docs/misc: arm: Add documentation about Device Tree passthrough
2015-04-09 15:09 [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough Julien Grall
` (17 preceding siblings ...)
2015-04-09 15:09 ` [PATCH v5 p2 18/19] xl: Add new option dtdev Julien Grall
@ 2015-04-09 15:09 ` Julien Grall
2015-04-16 15:20 ` Ian Campbell
18 siblings, 1 reply; 48+ messages in thread
From: Julien Grall @ 2015-04-09 15:09 UTC (permalink / raw)
To: xen-devel; +Cc: stefano.stabellini, Julien Grall, tim, ian.campbell
From: Julien Grall <julien.grall@linaro.org>
Note that the example is done on Midway whose SMMU driver is not
supported on Xen upstream.
Currently, I don't have other platform where I can test Device Tree
passthrough.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
Changes in v5:
- Drop references to "non-PCI" in favor of "Device Tree"
- Typoes and update the docs
Changes in v4:
- Patch added
---
docs/misc/arm/passthrough.txt | 61 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 61 insertions(+)
create mode 100644 docs/misc/arm/passthrough.txt
diff --git a/docs/misc/arm/passthrough.txt b/docs/misc/arm/passthrough.txt
new file mode 100644
index 0000000..93da99c
--- /dev/null
+++ b/docs/misc/arm/passthrough.txt
@@ -0,0 +1,61 @@
+Passthrough a device described in the Device Tree to a guest
+============================================================
+
+The example will use the secondary network card for the midway server.
+
+1) Mark the device to let Xen know the device will be used for passthrough.
+This is done in the device tree node describing the device by adding the
+property "xen,passthrough". The command to do it in U-Boot is:
+
+ fdt set /soc/ethernet@fff51000 xen,passthrough
+
+2) Create a partial device tree describing the device. The IRQ are mapped
+1:1 to the guest (i.e VIRQ == IRQ). For MMIO, you will have to find a hole
+in the guest memory layout (see xen/include/public/arch-arm.h, note that
+the layout is not stable and can change between versions of Xen).
+
+/dts-v1/;
+
+/ {
+ /* #*cells are here to keep DTC happy */
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ aliases {
+ net = &mac0;
+ };
+
+ passthrough {
+ compatible = "simple-bus";
+ ranges;
+ #address-cells = <2>;
+ #size-cells = <2>;
+ mac0: ethernet@10000000 {
+ compatible = "calxeda,hb-xgmac";
+ reg = <0 0x10000000 0 0x1000>;
+ interrupts = <0 80 4 0 81 4 0 82 4>;
+ };
+ };
+};
+
+Note:
+ * The interrupt-parent property will be added by the toolstack in the
+ root node;
+ * The following properies are mandatory with the /passthrough node:
+ - compatible: It should always contain "simple-bus"
+ - ranges
+ - #address-cells
+ - #size-cells
+
+3) Compile the partial guest device with dtc (Device Tree Compiler).
+For our purpose, the compiled file will be called guest-midway.dtb and
+placed in /root in DOM0.
+
+3) Add the following options in the guest configuration file:
+
+device_tree = "/root/guest-midway.dtb"
+dtdev = [ "/soc/ethernet@fff51000" ]
+irqs = [ 112, 113, 114 ]
+iomem = [ "0xfff51,1@0x10000" ]
+
+
--
2.1.4
^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 15/19] tools/(lib)xl: Add partial device tree support for ARM
2015-04-09 15:09 ` [PATCH v5 p2 15/19] tools/(lib)xl: Add partial device tree support for ARM Julien Grall
@ 2015-04-09 16:13 ` Ian Jackson
2015-04-28 13:30 ` Julien Grall
2015-04-16 15:17 ` Ian Campbell
1 sibling, 1 reply; 48+ messages in thread
From: Ian Jackson @ 2015-04-09 16:13 UTC (permalink / raw)
To: Julien Grall
Cc: Wei Liu, ian.campbell, Julien Grall, tim, stefano.stabellini, xen-devel
Julien Grall writes ("[PATCH v5 p2 15/19] tools/(lib)xl: Add partial device tree support for ARM"):
> Let the user to pass additional nodes to the guest device tree. For
> this purpose, everything in the node /passthrough from the partial
> device tree will be copied into the guest device tree.
Thanks for explaining to me in person what I was missing about the
semantics of DTs. As I said, you may wish to explain some of this in
the commit message or some of the in-tree documents, perhaps with
references. I will leave that up to you.
In any case,
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Although:
> + # Given the complexity of verifying the validity of a device tree,
> + # libxl doesn't do any security check on it. It's the responsability
Spelling mistake - should be "responsibility".
Thanks,
Ian.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 17/19] libxl: Add support for Device Tree passthrough
2015-04-09 15:09 ` [PATCH v5 p2 17/19] libxl: Add support for Device Tree passthrough Julien Grall
@ 2015-04-09 16:14 ` Ian Jackson
2015-04-16 15:19 ` Ian Campbell
1 sibling, 0 replies; 48+ messages in thread
From: Ian Jackson @ 2015-04-09 16:14 UTC (permalink / raw)
To: Julien Grall
Cc: Wei Liu, ian.campbell, Julien Grall, tim, stefano.stabellini, xen-devel
Julien Grall writes ("[PATCH v5 p2 17/19] libxl: Add support for Device Tree passthrough"):
> From: Julien Grall <julien.grall@linaro.org>
>
> On ARM, every non-PCI device are described in the device tree. Each of
> them can be found via a path.
>
> This patch introduces a very basic support, only the IOMMU will be set
> up correctly. The user will have to:
> - Describe the device in the partial device tree
> - Map manually MMIO/IRQ
>
> This is a first approach, that will allow to have a basic Device Tree
> passthrough support in Xen. This could be improved later.
>
> Furthermore add LIBXL_HAVE_DEVICETREE_PASSTHROUGH to indicate we
> support Device Tree passthrough and partial device tree (introduced by a
> previous patch).
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 16/19] tools/libxl: arm: Use an higher value for the GIC phandle
2015-04-09 15:09 ` [PATCH v5 p2 16/19] tools/libxl: arm: Use an higher value for the GIC phandle Julien Grall
@ 2015-04-09 16:17 ` Ian Jackson
2015-04-09 16:33 ` Julien Grall
0 siblings, 1 reply; 48+ messages in thread
From: Ian Jackson @ 2015-04-09 16:17 UTC (permalink / raw)
To: Julien Grall
Cc: Wei Liu, ian.campbell, Julien Grall, tim, stefano.stabellini, xen-devel
Julien Grall writes ("[PATCH v5 p2 16/19] tools/libxl: arm: Use an higher value for the GIC phandle"):
> The partial device tree may contains phandle. The Device Tree Compiler
> tends to allocate the phandle from 1.
I have to say I have no idea what a phandle is...
> Reserve the ID 65000 for the GIC phandle. I think we can safely assume
> that the partial device tree will never contain a such ID.
Do we control the DT compiler ? What if it should change its
phandle allocation algorithm ?
> +/*
> + * The device tree compiler (DTC) is allocating the phandle from 1 to
> + * onwards. Reserve a high value for the GIC phandle.
> + */
FYI this should read "The device tree compiler (DTC) allocates phandle
values frrom 1 onwards".
Thanks,
Ian.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 16/19] tools/libxl: arm: Use an higher value for the GIC phandle
2015-04-09 16:17 ` Ian Jackson
@ 2015-04-09 16:33 ` Julien Grall
2015-04-09 16:52 ` Ian Jackson
0 siblings, 1 reply; 48+ messages in thread
From: Julien Grall @ 2015-04-09 16:33 UTC (permalink / raw)
To: Ian Jackson, Julien Grall
Cc: Wei Liu, ian.campbell, Julien Grall, tim, stefano.stabellini, xen-devel
Hi Ian,
On 09/04/15 17:17, Ian Jackson wrote:
> Julien Grall writes ("[PATCH v5 p2 16/19] tools/libxl: arm: Use an higher value for the GIC phandle"):
>> The partial device tree may contains phandle. The Device Tree Compiler
>> tends to allocate the phandle from 1.
>
> I have to say I have no idea what a phandle is...
A phandle is a way to reference another node in the device tree.
Any node that can referenced defines a phandle property with a unique
unsigned 32 bit value.
>> Reserve the ID 65000 for the GIC phandle. I think we can safely assume
>> that the partial device tree will never contain a such ID.
>
> Do we control the DT compiler ? What if it should change its
> phandle allocation algorithm ?
We don't control the DT compiler. But the algorithm of the phandle will
unlikely change. FWIW, the compiler is very tiny, it's not GCC.
I only expect people using the partial device tree in very specific use
case. Generic use case is not even possible with the current status of
non-PCI (i.e device tree) passthrough. So people control their environment.
As I said later in patch, supporting dynamic allocation will require
some rework in the device tree creation for the guest.
So I was suggesting this solution as temporary in order to not block the
DT passthrough.
>> +/*
>> + * The device tree compiler (DTC) is allocating the phandle from 1 to
>> + * onwards. Reserve a high value for the GIC phandle.
>> + */
>
> FYI this should read "The device tree compiler (DTC) allocates phandle
> values frrom 1 onwards".
Thanks, I will update the comment.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 16/19] tools/libxl: arm: Use an higher value for the GIC phandle
2015-04-09 16:33 ` Julien Grall
@ 2015-04-09 16:52 ` Ian Jackson
2015-04-09 17:00 ` Julien Grall
0 siblings, 1 reply; 48+ messages in thread
From: Ian Jackson @ 2015-04-09 16:52 UTC (permalink / raw)
To: Julien Grall
Cc: Wei Liu, ian.campbell, Julien Grall, tim, Julien Grall,
stefano.stabellini, xen-devel
Julien Grall writes ("Re: [Xen-devel] [PATCH v5 p2 16/19] tools/libxl: arm: Use an higher value for the GIC phandle"):
> On 09/04/15 17:17, Ian Jackson wrote:
> > I have to say I have no idea what a phandle is...
>
> A phandle is a way to reference another node in the device tree.
> Any node that can referenced defines a phandle property with a unique
> unsigned 32 bit value.
Thanks for the explanation.
> >> Reserve the ID 65000 for the GIC phandle. I think we can safely assume
> >> that the partial device tree will never contain a such ID.
> >
> > Do we control the DT compiler ? What if it should change its
> > phandle allocation algorithm ?
>
> We don't control the DT compiler. But the algorithm of the phandle will
> unlikely change. FWIW, the compiler is very tiny, it's not GCC.
Right.
> I only expect people using the partial device tree in very specific use
> case. Generic use case is not even possible with the current status of
> non-PCI (i.e device tree) passthrough. So people control their environment.
>
> As I said later in patch, supporting dynamic allocation will require
> some rework in the device tree creation for the guest.
>
> So I was suggesting this solution as temporary in order to not block the
> DT passthrough.
What would happen if our assumption about the DT compiler were
violated ?
Ian.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 16/19] tools/libxl: arm: Use an higher value for the GIC phandle
2015-04-09 16:52 ` Ian Jackson
@ 2015-04-09 17:00 ` Julien Grall
2015-04-09 17:02 ` Julien Grall
2015-04-09 17:04 ` Ian Jackson
0 siblings, 2 replies; 48+ messages in thread
From: Julien Grall @ 2015-04-09 17:00 UTC (permalink / raw)
To: Ian Jackson
Cc: Wei Liu, ian.campbell, Julien Grall, tim, Julien Grall,
stefano.stabellini, xen-devel
On 09/04/15 17:52, Ian Jackson wrote:
> Julien Grall writes ("Re: [Xen-devel] [PATCH v5 p2 16/19] tools/libxl: arm: Use an higher value for the GIC phandle"):
>> On 09/04/15 17:17, Ian Jackson wrote:
>> I only expect people using the partial device tree in very specific use
>> case. Generic use case is not even possible with the current status of
>> non-PCI (i.e device tree) passthrough. So people control their environment.
>>
>> As I said later in patch, supporting dynamic allocation will require
>> some rework in the device tree creation for the guest.
>>
>> So I was suggesting this solution as temporary in order to not block the
>> DT passthrough.
>
> What would happen if our assumption about the DT compiler were
> violated ?
The phandle would be present in 2 different nodes of the DT. FYI, that
may also happen if a user use 2 times the same phandle in the partial DT.
The guest may retrieve the wrong node and warn/crash depending on the
implementation.
Although, it won't impact neither Xen nor the toolstack.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 16/19] tools/libxl: arm: Use an higher value for the GIC phandle
2015-04-09 17:00 ` Julien Grall
@ 2015-04-09 17:02 ` Julien Grall
2015-04-09 17:04 ` Ian Jackson
1 sibling, 0 replies; 48+ messages in thread
From: Julien Grall @ 2015-04-09 17:02 UTC (permalink / raw)
To: Ian Jackson
Cc: Wei Liu, ian.campbell, Julien Grall, tim, stefano.stabellini, xen-devel
On 09/04/15 18:00, Julien Grall wrote:
> On 09/04/15 17:52, Ian Jackson wrote:
>> Julien Grall writes ("Re: [Xen-devel] [PATCH v5 p2 16/19] tools/libxl: arm: Use an higher value for the GIC phandle"):
>>> On 09/04/15 17:17, Ian Jackson wrote:
>>> I only expect people using the partial device tree in very specific use
>>> case. Generic use case is not even possible with the current status of
>>> non-PCI (i.e device tree) passthrough. So people control their environment.
>>>
>>> As I said later in patch, supporting dynamic allocation will require
>>> some rework in the device tree creation for the guest.
>>>
>>> So I was suggesting this solution as temporary in order to not block the
>>> DT passthrough.
>>
>> What would happen if our assumption about the DT compiler were
>> violated ?
>
> The phandle would be present in 2 different nodes of the DT. FYI, that
> may also happen if a user use 2 times the same phandle in the partial DT.
>
> The guest may retrieve the wrong node and warn/crash depending on the
> implementation.
>
> Although, it won't impact neither Xen nor the toolstack.
(To be more complete) Because Xen and the toolstack doesn't made any
usage of the phandle. The toolstack only generates the device tree and
copy necessary node from the partial DT.
--
Julien Grall
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 16/19] tools/libxl: arm: Use an higher value for the GIC phandle
2015-04-09 17:00 ` Julien Grall
2015-04-09 17:02 ` Julien Grall
@ 2015-04-09 17:04 ` Ian Jackson
2015-04-13 12:07 ` Julien Grall
1 sibling, 1 reply; 48+ messages in thread
From: Ian Jackson @ 2015-04-09 17:04 UTC (permalink / raw)
To: Julien Grall
Cc: Wei Liu, ian.campbell, Julien Grall, tim, stefano.stabellini, xen-devel
Julien Grall writes ("Re: [Xen-devel] [PATCH v5 p2 16/19] tools/libxl: arm: Use an higher value for the GIC phandle"):
> On 09/04/15 17:52, Ian Jackson wrote:
> > What would happen if our assumption about the DT compiler were
> > violated ?
>
> The phandle would be present in 2 different nodes of the DT. FYI, that
> may also happen if a user use 2 times the same phandle in the partial DT.
So we are making an assumption about the DT compiler which we don't
check, and when the assumption is violated, we produce a corrupted DT
which produces undefined behaviour in the guest.
Can we do some kind of check ?
Ian.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 16/19] tools/libxl: arm: Use an higher value for the GIC phandle
2015-04-09 17:04 ` Ian Jackson
@ 2015-04-13 12:07 ` Julien Grall
0 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2015-04-13 12:07 UTC (permalink / raw)
To: Ian Jackson, Julien Grall
Cc: Wei Liu, ian.campbell, Julien Grall, tim, stefano.stabellini, xen-devel
Hi Ian,
On 09/04/15 18:04, Ian Jackson wrote:
> Julien Grall writes ("Re: [Xen-devel] [PATCH v5 p2 16/19] tools/libxl: arm: Use an higher value for the GIC phandle"):
>> On 09/04/15 17:52, Ian Jackson wrote:
>>> What would happen if our assumption about the DT compiler were
>>> violated ?
>>
>> The phandle would be present in 2 different nodes of the DT. FYI, that
>> may also happen if a user use 2 times the same phandle in the partial DT.
>
> So we are making an assumption about the DT compiler which we don't
> check, and when the assumption is violated, we produce a corrupted DT
> which produces undefined behaviour in the guest.
>
> Can we do some kind of check ?
As discussed IRL, I will add a note in the documentation about this
restriction.
This will be removed when the phandle will be generated dynamically.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 01/19] xen/arm: Let the toolstack configure the number of SPIs
2015-04-09 15:09 ` [PATCH v5 p2 01/19] xen/arm: Let the toolstack configure the number of SPIs Julien Grall
@ 2015-04-16 14:39 ` Ian Campbell
2015-04-16 15:10 ` Julien Grall
0 siblings, 1 reply; 48+ messages in thread
From: Ian Campbell @ 2015-04-16 14:39 UTC (permalink / raw)
To: Julien Grall
Cc: Wei Liu, Ian Jackson, Julien Grall, tim, stefano.stabellini,
Jan Beulich, xen-devel
On Thu, 2015-04-09 at 16:09 +0100, Julien Grall wrote:
> From: Julien Grall <julien.grall@linaro.org>
>
> Each domain may have a different number of IRQs depending on the devices
> assigned to it.
> Rather re-using the number of IRQs used by the hardwared GIC, let the
^than
> toolstack specify the number of SPIs when the domain is created. This
> will avoid to waste memory.
"will avoid wasting memory."
> + /* Limit the number of virtual SPIs supported to (1020 - 32) = 988 */
> + if ( (nr_spis + NR_LOCAL_IRQS) > 1020 )
> + return -EINVAL;
If there's any chance this can be called by not-completely trusted code
(e.g. a disaggregated toolstack) then this if susceptible to an overflow
(sorry, I gave you this code in a previous rev).
I think you can just move the NR_LOCAL_IRQS to the other side of the
expression, i.e.
if ( nr_spis > 1020 - NR_LOCAL_IRQS )
With that and the grammar fixed:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Ian.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 04/19] xen/arm: Implement hypercall DOMCTL_{, un}bind_pt_pirq
2015-04-09 15:09 ` [PATCH v5 p2 04/19] xen/arm: Implement hypercall DOMCTL_{, un}bind_pt_pirq Julien Grall
@ 2015-04-16 14:55 ` Ian Campbell
2015-04-16 15:20 ` Julien Grall
2015-04-17 23:05 ` Daniel De Graaf
0 siblings, 2 replies; 48+ messages in thread
From: Ian Campbell @ 2015-04-16 14:55 UTC (permalink / raw)
To: Julien Grall, Daniel De Graaf
Cc: xen-devel, Julien Grall, tim, Jan Beulich, stefano.stabellini
On Thu, 2015-04-09 at 16:09 +0100, Julien Grall wrote:
> From: Julien Grall <julien.grall@linaro.org>
I've left the XSM related quotes untrimmed and CCd Daniel. I think it's
all code motion (making x86 specific things generic), so perhaps no ack
needed but an opportunity to nack instead ;-)
> On x86, an IRQ is assigned in 2 steps to an HVM guest:
> - The toolstack is calling PHYSDEVOP_map_pirq in order to create a
> guest PIRQ (IRQ bound to an event channel)
> - The emulator (QEMU) is calling DOMCTL_bind_pt_irq in order to
> bind the IRQ
>
> On ARM, there is no concept of PIRQ as the IRQ can be assigned to a
> virtual IRQ using the interrupt controller.
>
> It's not clear if we will need 2 different hypercalls on ARM to assign
> IRQ and, for now, only the toolstack will manage IRQ.
>
> In order to avoid re-using a fixed ABI hypercall (PHYSDEVOP_*) for a
> different purpose and allow us more time to figure out the right out,
> only DOMCTL_{,un}bind_pt_pirq is implemented on ARM.
>
> The DOMCTL is extended with a new type PT_IRQ_TYPE_SPI and only IRQ ==
> vIRQ (i.e machine_irq == spi) is supported.
>
> Concerning XSM, even if ARM is using one hypercall rather than 2, the
> resulting check is nearly the same.
>
> XSM PHYSDEVOP_map_pirq:
> 1) Check if the current domain can add resource to the domain
> 2) Check if the current domain has permission to add the IRQ
> 3) Check if the target domain has permission to use the IRQ
>
> XSM DOMCTL_bind_pirq_irq:
> 1) Check if the current domain can add resource to the domain
> 2) Check if the current domain has permission to bind the IRQ
> 3) Check if the target domain has permission to use the IRQ
>
> In order to keep the same XSM checks done by the 2 hypercalls on x86,
> call both xsm_map_domain_irq & xsm_bind_pt_irq in the ARM implementation.
I think this paragraph makes the previous bit obsolete?
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index 1f2c80c..676ec50 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -1773,15 +1773,16 @@ int xc_domain_unbind_msi_irq(
> }
>
> /* Pass-through: binds machine irq to guests irq */
> -int xc_domain_bind_pt_irq(
> - xc_interface *xch,
> - uint32_t domid,
> - uint8_t machine_irq,
> - uint8_t irq_type,
> - uint8_t bus,
> - uint8_t device,
> - uint8_t intx,
> - uint8_t isa_irq)
> +static int xc_domain_bind_pt_irq_int(
> + xc_interface *xch,
> + uint32_t domid,
> + uint8_t machine_irq,
> + uint8_t irq_type,
> + uint8_t bus,
> + uint8_t device,
> + uint8_t intx,
> + uint8_t isa_irq,
> + uint16_t spi)
Please either leave the indentation of the arguments as it is or fix it
properly, i.e. put xch on the same line as the prototype and align
everything else below it.
TBH I'd prefer the former even if it isn't strictly coding style since
it obscures the real change you made.
> @@ -1878,6 +1913,25 @@ int xc_domain_bind_pt_isa_irq(
> PT_IRQ_TYPE_ISA, 0, 0, 0, machine_irq));
> }
>
> +int xc_domain_bind_pt_spi_irq(
> + xc_interface *xch,
> + uint32_t domid,
> + uint16_t spi)
> +{
> + /* vSPI == SPI */
I wonder if to avoid API churn later we should accept both machine and
guest IRQ here and rely on the check that htey are the same in the
hypervisor to reject?
Fair enough we can change libxc interface if we want, but avoiding a
little churn later on seems reasonable and it makes it a better fit with
the existing interfaces.
> diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> index 6f30af7..531bb13 100644
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -10,6 +10,8 @@
> #include <xen/errno.h>
> #include <xen/sched.h>
> #include <xen/hypercall.h>
> +#include <xen/iocap.h>
> +#include <xsm/xsm.h>
> #include <public/domctl.h>
>
> long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
> @@ -30,6 +32,81 @@ long arch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>
> return p2m_cache_flush(d, s, e);
> }
> + case XEN_DOMCTL_bind_pt_irq:
> + {
> + int rc;
> + xen_domctl_bind_pt_irq_t *bind = &domctl->u.bind_pt_irq;
> + uint32_t irq = bind->u.spi.spi;
> + uint32_t virq = bind->machine_irq;
> +
> + /* We only support PT_IRQ_TYPE_SPI */
> + if ( bind->irq_type != PT_IRQ_TYPE_SPI )
> + return -EOPNOTSUPP;
> +
> + /*
> + * XXX: For now map the interrupt 1:1. Other support will require to
> + * modify domain_pirq_to_irq macro.
> + */
> + if ( irq != virq )
> + return -EINVAL;
> +
> + /*
> + * ARM doesn't require to separate IRQ assignation in 2
"ARM doesn't require separating assignment of IRQs into 2 hypercalls"
> + * hypercalls (PHYSDEVOP_map_pirq and DOMCTL_bind_pt_irq).
> + *
> + * Call xsm_map_domain_irq in order to keep the same XSM checks
> + * done by the 2 hypercalls
... for consistency with other architectures.
> + */
> + rc = xsm_map_domain_irq(XSM_HOOK, d, irq, NULL);
> + if ( rc )
> + return rc;
> +
> + rc = xsm_bind_pt_irq(XSM_HOOK, d, bind);
> + if ( rc )
> + return rc;
> +
> + if ( !irq_access_permitted(current->domain, irq) )
> + return -EPERM;
> +
> + if ( !vgic_reserve_virq(d, virq) )
> + return -EBUSY;
> +
> + rc = route_irq_to_guest(d, virq, irq, "routed IRQ");
> + if ( rc )
> + vgic_free_virq(d, virq);
> +
> + return rc;
> + }
> + case XEN_DOMCTL_unbind_pt_irq:
> + {
> + int rc;
> + xen_domctl_bind_pt_irq_t *bind = &domctl->u.bind_pt_irq;
> + uint32_t irq = bind->u.spi.spi;
> + uint32_t virq = bind->machine_irq;
> +
> + /* We only support PT_IRQ_TYPE_SPI */
> + if ( bind->irq_type != PT_IRQ_TYPE_SPI )
> + return -EOPNOTSUPP;
> +
> + /* For now map the interrupt 1:1 */
> + if ( irq != virq )
> + return -EINVAL;
The x86 version doesn't appear to consider irq_type or irq, only virq
(from ->machine_irq). That seems to be logical to me (if a little
underdocumented) and I think we should be consistent.
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index 4227093..c36e05f 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -445,6 +445,18 @@ static XSM_INLINE int xsm_unmap_domain_pirq(XSM_DEFAULT_ARG struct domain *d)
> return xsm_default_action(action, current->domain, d);
> }
>
> +static XSM_INLINE int xsm_bind_pt_irq(XSM_DEFAULT_ARG struct domain *d, struct xen_domctl_bind_pt_irq *bind)
> +{
> + XSM_ASSERT_ACTION(XSM_HOOK);
> + return xsm_default_action(action, current->domain, d);
> +}
> +
> +static XSM_INLINE int xsm_unbind_pt_irq(XSM_DEFAULT_ARG struct domain *d, struct xen_domctl_bind_pt_irq *bind)
> +{
> + XSM_ASSERT_ACTION(XSM_HOOK);
> + return xsm_default_action(action, current->domain, d);
> +}
> +
> static XSM_INLINE int xsm_unmap_domain_irq(XSM_DEFAULT_ARG struct domain *d, int irq, void *data)
> {
> XSM_ASSERT_ACTION(XSM_HOOK);
> @@ -631,18 +643,6 @@ static XSM_INLINE int xsm_priv_mapping(XSM_DEFAULT_ARG struct domain *d, struct
> return xsm_default_action(action, d, t);
> }
>
> -static XSM_INLINE int xsm_bind_pt_irq(XSM_DEFAULT_ARG struct domain *d, struct xen_domctl_bind_pt_irq *bind)
> -{
> - XSM_ASSERT_ACTION(XSM_HOOK);
> - return xsm_default_action(action, current->domain, d);
> -}
> -
> -static XSM_INLINE int xsm_unbind_pt_irq(XSM_DEFAULT_ARG struct domain *d, struct xen_domctl_bind_pt_irq *bind)
> -{
> - XSM_ASSERT_ACTION(XSM_HOOK);
> - return xsm_default_action(action, current->domain, d);
> -}
> -
> static XSM_INLINE int xsm_ioport_permission(XSM_DEFAULT_ARG struct domain *d, uint32_t s, uint32_t e, uint8_t allow)
> {
> XSM_ASSERT_ACTION(XSM_HOOK);
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index c2049f3..b7446be 100644
> --- a/xen/include/xsm/xsm.h
> +++ b/xen/include/xsm/xsm.h
> @@ -107,6 +107,8 @@ struct xsm_operations {
> int (*map_domain_irq) (struct domain *d, int irq, void *data);
> int (*unmap_domain_pirq) (struct domain *d);
> int (*unmap_domain_irq) (struct domain *d, int irq, void *data);
> + int (*bind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq *bind);
> + int (*unbind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq *bind);
> int (*irq_permission) (struct domain *d, int pirq, uint8_t allow);
> 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);
> @@ -169,8 +171,6 @@ struct xsm_operations {
> int (*mmuext_op) (struct domain *d, struct domain *f);
> int (*update_va_mapping) (struct domain *d, struct domain *f, l1_pgentry_t pte);
> int (*priv_mapping) (struct domain *d, struct domain *t);
> - int (*bind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq *bind);
> - int (*unbind_pt_irq) (struct domain *d, struct xen_domctl_bind_pt_irq *bind);
> int (*ioport_permission) (struct domain *d, uint32_t s, uint32_t e, uint8_t allow);
> int (*ioport_mapping) (struct domain *d, uint32_t s, uint32_t e, uint8_t allow);
> #endif
> @@ -419,6 +419,18 @@ static inline int xsm_unmap_domain_irq (xsm_default_t def, struct domain *d, int
> return xsm_ops->unmap_domain_irq(d, irq, data);
> }
>
> +static inline int xsm_bind_pt_irq(xsm_default_t def, struct domain *d,
> + struct xen_domctl_bind_pt_irq *bind)
> +{
> + return xsm_ops->bind_pt_irq(d, bind);
> +}
> +
> +static inline int xsm_unbind_pt_irq(xsm_default_t def, struct domain *d,
> + struct xen_domctl_bind_pt_irq *bind)
> +{
> + return xsm_ops->unbind_pt_irq(d, bind);
> +}
> +
> static inline int xsm_irq_permission (xsm_default_t def, struct domain *d, int pirq, uint8_t allow)
> {
> return xsm_ops->irq_permission(d, pirq, allow);
> @@ -643,18 +655,6 @@ static inline int xsm_priv_mapping(xsm_default_t def, struct domain *d, struct d
> return xsm_ops->priv_mapping(d, t);
> }
>
> -static inline int xsm_bind_pt_irq(xsm_default_t def, struct domain *d,
> - struct xen_domctl_bind_pt_irq *bind)
> -{
> - return xsm_ops->bind_pt_irq(d, bind);
> -}
> -
> -static inline int xsm_unbind_pt_irq(xsm_default_t def, struct domain *d,
> - struct xen_domctl_bind_pt_irq *bind)
> -{
> - return xsm_ops->unbind_pt_irq(d, bind);
> -}
> -
> static inline int xsm_ioport_permission (xsm_default_t def, struct domain *d, uint32_t s, uint32_t e, uint8_t allow)
> {
> return xsm_ops->ioport_permission(d, s, e, allow);
> diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
> index 25fca68..a3b8aab 100644
> --- a/xen/xsm/dummy.c
> +++ b/xen/xsm/dummy.c
> @@ -81,6 +81,8 @@ void xsm_fixup_ops (struct xsm_operations *ops)
> set_to_dummy_if_null(ops, map_domain_irq);
> set_to_dummy_if_null(ops, unmap_domain_pirq);
> set_to_dummy_if_null(ops, unmap_domain_irq);
> + set_to_dummy_if_null(ops, bind_pt_irq);
> + set_to_dummy_if_null(ops, unbind_pt_irq);
> set_to_dummy_if_null(ops, irq_permission);
> set_to_dummy_if_null(ops, iomem_permission);
> set_to_dummy_if_null(ops, iomem_mapping);
> @@ -140,8 +142,6 @@ void xsm_fixup_ops (struct xsm_operations *ops)
> set_to_dummy_if_null(ops, mmuext_op);
> set_to_dummy_if_null(ops, update_va_mapping);
> set_to_dummy_if_null(ops, priv_mapping);
> - set_to_dummy_if_null(ops, bind_pt_irq);
> - set_to_dummy_if_null(ops, unbind_pt_irq);
> set_to_dummy_if_null(ops, ioport_permission);
> set_to_dummy_if_null(ops, ioport_mapping);
> #endif
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index ab5141d..688ba2a 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -580,12 +580,14 @@ static int flask_domctl(struct domain *d, int cmd)
> #ifdef HAS_MEM_ACCESS
> case XEN_DOMCTL_vm_event_op:
> #endif
> +
> + /* These have individual XSM hooks (arch/../domctl.c) */
> + case XEN_DOMCTL_bind_pt_irq:
> + case XEN_DOMCTL_unbind_pt_irq:
> #ifdef CONFIG_X86
> /* These have individual XSM hooks (arch/x86/domctl.c) */
> case XEN_DOMCTL_shadow_op:
> case XEN_DOMCTL_ioport_permission:
> - case XEN_DOMCTL_bind_pt_irq:
> - case XEN_DOMCTL_unbind_pt_irq:
> case XEN_DOMCTL_ioport_mapping:
> /* These have individual XSM hooks (drivers/passthrough/iommu.c) */
> case XEN_DOMCTL_get_device_group:
> @@ -911,6 +913,36 @@ static int flask_unmap_domain_irq (struct domain *d, int irq, void *data)
> return rc;
> }
>
> +static int flask_bind_pt_irq (struct domain *d, struct xen_domctl_bind_pt_irq *bind)
> +{
> + u32 dsid, rsid;
> + int rc = -EPERM;
> + int irq;
> + struct avc_audit_data ad;
> +
> + rc = current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__ADD);
> + if ( rc )
> + return rc;
> +
> + irq = domain_pirq_to_irq(d, bind->machine_irq);
> +
> + rc = get_irq_sid(irq, &rsid, &ad);
> + if ( rc )
> + return rc;
> +
> + rc = avc_current_has_perm(rsid, SECCLASS_HVM, HVM__BIND_IRQ, &ad);
> + if ( rc )
> + return rc;
> +
> + dsid = domain_sid(d);
> + return avc_has_perm(dsid, rsid, SECCLASS_RESOURCE, RESOURCE__USE, &ad);
> +}
> +
> +static int flask_unbind_pt_irq (struct domain *d, struct xen_domctl_bind_pt_irq *bind)
> +{
> + return current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__REMOVE);
> +}
> +
> static int flask_irq_permission (struct domain *d, int pirq, uint8_t access)
> {
> /* the PIRQ number is not useful; real IRQ is checked during mapping */
> @@ -1468,36 +1500,6 @@ static int flask_priv_mapping(struct domain *d, struct domain *t)
> {
> return domain_has_perm(d, t, SECCLASS_MMU, MMU__TARGET_HACK);
> }
> -
> -static int flask_bind_pt_irq (struct domain *d, struct xen_domctl_bind_pt_irq *bind)
> -{
> - u32 dsid, rsid;
> - int rc = -EPERM;
> - int irq;
> - struct avc_audit_data ad;
> -
> - rc = current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__ADD);
> - if ( rc )
> - return rc;
> -
> - irq = domain_pirq_to_irq(d, bind->machine_irq);
> -
> - rc = get_irq_sid(irq, &rsid, &ad);
> - if ( rc )
> - return rc;
> -
> - rc = avc_current_has_perm(rsid, SECCLASS_HVM, HVM__BIND_IRQ, &ad);
> - if ( rc )
> - return rc;
> -
> - dsid = domain_sid(d);
> - return avc_has_perm(dsid, rsid, SECCLASS_RESOURCE, RESOURCE__USE, &ad);
> -}
> -
> -static int flask_unbind_pt_irq (struct domain *d, struct xen_domctl_bind_pt_irq *bind)
> -{
> - return current_has_perm(d, SECCLASS_RESOURCE, RESOURCE__REMOVE);
> -}
> #endif /* CONFIG_X86 */
>
> long do_flask_op(XEN_GUEST_HANDLE_PARAM(xsm_op_t) u_flask_op);
> @@ -1556,6 +1558,8 @@ static struct xsm_operations flask_ops = {
> .map_domain_irq = flask_map_domain_irq,
> .unmap_domain_pirq = flask_unmap_domain_pirq,
> .unmap_domain_irq = flask_unmap_domain_irq,
> + .bind_pt_irq = flask_bind_pt_irq,
> + .unbind_pt_irq = flask_unbind_pt_irq,
> .irq_permission = flask_irq_permission,
> .iomem_permission = flask_iomem_permission,
> .iomem_mapping = flask_iomem_mapping,
> @@ -1616,8 +1620,6 @@ static struct xsm_operations flask_ops = {
> .mmuext_op = flask_mmuext_op,
> .update_va_mapping = flask_update_va_mapping,
> .priv_mapping = flask_priv_mapping,
> - .bind_pt_irq = flask_bind_pt_irq,
> - .unbind_pt_irq = flask_unbind_pt_irq,
> .ioport_permission = flask_ioport_permission,
> .ioport_mapping = flask_ioport_mapping,
> #endif
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 06/19] xen/dts: Provide an helper to get a DT node from a path provided by a guest
2015-04-09 15:09 ` [PATCH v5 p2 06/19] xen/dts: Provide an helper to get a DT node from a path provided by a guest Julien Grall
@ 2015-04-16 14:57 ` Ian Campbell
0 siblings, 0 replies; 48+ messages in thread
From: Ian Campbell @ 2015-04-16 14:57 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, Julien Grall, tim, stefano.stabellini
On Thu, 2015-04-09 at 16:09 +0100, Julien Grall wrote:
> From: Julien Grall <julien.grall@linaro.org>
>
> The maximum size of the copied string has been chosen based on the value
> use by XSM in similar case.
>
> Furthermore, Linux seems to allow path up to 4096 characters. Though
> this could vary from one OS to another.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 01/19] xen/arm: Let the toolstack configure the number of SPIs
2015-04-16 14:39 ` Ian Campbell
@ 2015-04-16 15:10 ` Julien Grall
0 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2015-04-16 15:10 UTC (permalink / raw)
To: Ian Campbell, Julien Grall
Cc: Wei Liu, tim, Julien Grall, Ian Jackson, stefano.stabellini,
Jan Beulich, xen-devel
Hi Ian,
On 16/04/2015 15:39, Ian Campbell wrote:
> On Thu, 2015-04-09 at 16:09 +0100, Julien Grall wrote:
>> From: Julien Grall <julien.grall@linaro.org>
>>
>> Each domain may have a different number of IRQs depending on the devices
>> assigned to it.
>
>> Rather re-using the number of IRQs used by the hardwared GIC, let the
>
> ^than
>
>> toolstack specify the number of SPIs when the domain is created. This
>> will avoid to waste memory.
>
> "will avoid wasting memory."
>
>
>> + /* Limit the number of virtual SPIs supported to (1020 - 32) = 988 */
>> + if ( (nr_spis + NR_LOCAL_IRQS) > 1020 )
>> + return -EINVAL;
>
> If there's any chance this can be called by not-completely trusted code
> (e.g. a disaggregated toolstack) then this if susceptible to an overflow
> (sorry, I gave you this code in a previous rev).
Hmmm, right.
> I think you can just move the NR_LOCAL_IRQS to the other side of the
> expression, i.e.
> if ( nr_spis > 1020 - NR_LOCAL_IRQS )
I will do the change and adding a pair of parentheses for more clarity:
if ( nr_spis > (1020 - NR_LOCAL_IRQS) )
> With that and the grammar fixed:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Thanks,
--
Julien Grall
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 12/19] xen/passthrough: Extend XEN_DOMCTL_*assign_device to support DT device
2015-04-09 15:09 ` [PATCH v5 p2 12/19] xen/passthrough: Extend XEN_DOMCTL_*assign_device to support DT device Julien Grall
@ 2015-04-16 15:11 ` Ian Campbell
2015-04-16 15:21 ` Julien Grall
0 siblings, 1 reply; 48+ messages in thread
From: Ian Campbell @ 2015-04-16 15:11 UTC (permalink / raw)
To: Julien Grall
Cc: Wei Liu, tim, Julien Grall, Ian Jackson, stefano.stabellini,
Jan Beulich, xen-devel
On Thu, 2015-04-09 at 16:09 +0100, Julien Grall wrote:
> From: Julien Grall <julien.grall@linaro.org>
>
> A device node is described by a path. It will be used to retrieved the
"retrieve"
> node in the device tree and assign the related device to the domain.
>
> Only non-PCI protected by an IOMMU can be assigned to a guest.
I think a word is missing between PCI and protected, perhaps "devices"?
> Also document the behavior of XEN_DOMCTL_deassign_device in the public
> headers which differ between non-PCI and PCI.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
With the above fixed:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 13/19] tools/libxl: Create a per-arch function to map IRQ to a domain
2015-04-09 15:09 ` [PATCH v5 p2 13/19] tools/libxl: Create a per-arch function to map IRQ to a domain Julien Grall
@ 2015-04-16 15:12 ` Ian Campbell
2015-04-16 15:26 ` Julien Grall
0 siblings, 1 reply; 48+ messages in thread
From: Ian Campbell @ 2015-04-16 15:12 UTC (permalink / raw)
To: Julien Grall
Cc: Wei Liu, Ian Jackson, Julien Grall, tim, stefano.stabellini, xen-devel
On Thu, 2015-04-09 at 16:09 +0100, Julien Grall wrote:
> From: Julien Grall <julien.grall@linaro.org>
>
> ARM and x86 use a different hypercall to map an IRQ to a domain.
>
> The hypercall to give IRQ permission to the domain has also been moved
> to be an x86 specific function as ARM guest won't be able to manage the IRQ.
> We may want to support it later.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
(as I did v4 with the typoes fixed)
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 14/19] tools/libxl: Check if fdt_{first, next}_subnode are present in libfdt
2015-04-09 15:09 ` [PATCH v5 p2 14/19] tools/libxl: Check if fdt_{first, next}_subnode are present in libfdt Julien Grall
@ 2015-04-16 15:13 ` Ian Campbell
0 siblings, 0 replies; 48+ messages in thread
From: Ian Campbell @ 2015-04-16 15:13 UTC (permalink / raw)
To: Julien Grall
Cc: Wei Liu, Ian Jackson, Julien Grall, tim, stefano.stabellini, xen-devel
On Thu, 2015-04-09 at 16:09 +0100, Julien Grall wrote:
> From: Julien Grall <julien.grall@linaro.org>
>
> The functions fdt_{fisrt,next}_subnode may not be available because:
> * It has been introduced in 2013 => Doesn't work on Wheezy
> * The prototype exists but the functions are not exposed. Don't ask
> why...
>
> The later has been fixed recently in the dtc repo [1]
>
> When the functions are not available, implement our own in order to use
> them in a following patch.
>
> [1] git://git.kernel.org/pub/scm/utils/dtc/dtc.git
> commit a4b093f7366fdb429ca1781144d3985fa50d0fbb
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 15/19] tools/(lib)xl: Add partial device tree support for ARM
2015-04-09 15:09 ` [PATCH v5 p2 15/19] tools/(lib)xl: Add partial device tree support for ARM Julien Grall
2015-04-09 16:13 ` Ian Jackson
@ 2015-04-16 15:17 ` Ian Campbell
1 sibling, 0 replies; 48+ messages in thread
From: Ian Campbell @ 2015-04-16 15:17 UTC (permalink / raw)
To: Julien Grall
Cc: Wei Liu, Ian Jackson, Julien Grall, tim, stefano.stabellini, xen-devel
On Thu, 2015-04-09 at 16:09 +0100, Julien Grall wrote:
> From: Julien Grall <julien.grall@linaro.org>
>
> Let the user to pass additional nodes to the guest device tree. For
"Allow the user to pass" or "Let the user pass".
I prefer the first FWIW.
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
With one of the above:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 17/19] libxl: Add support for Device Tree passthrough
2015-04-09 15:09 ` [PATCH v5 p2 17/19] libxl: Add support for Device Tree passthrough Julien Grall
2015-04-09 16:14 ` Ian Jackson
@ 2015-04-16 15:19 ` Ian Campbell
1 sibling, 0 replies; 48+ messages in thread
From: Ian Campbell @ 2015-04-16 15:19 UTC (permalink / raw)
To: Julien Grall
Cc: Wei Liu, Ian Jackson, Julien Grall, tim, stefano.stabellini, xen-devel
On Thu, 2015-04-09 at 16:09 +0100, Julien Grall wrote:
> From: Julien Grall <julien.grall@linaro.org>
>
> On ARM, every non-PCI device are described in the device tree. Each of
> them can be found via a path.
>
> This patch introduces a very basic support, only the IOMMU will be set
> up correctly. The user will have to:
> - Describe the device in the partial device tree
> - Map manually MMIO/IRQ
>
> This is a first approach, that will allow to have a basic Device Tree
> passthrough support in Xen. This could be improved later.
>
> Furthermore add LIBXL_HAVE_DEVICETREE_PASSTHROUGH to indicate we
> support Device Tree passthrough and partial device tree (introduced by a
> previous patch).
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 19/19] docs/misc: arm: Add documentation about Device Tree passthrough
2015-04-09 15:09 ` [PATCH v5 p2 19/19] docs/misc: arm: Add documentation about Device Tree passthrough Julien Grall
@ 2015-04-16 15:20 ` Ian Campbell
0 siblings, 0 replies; 48+ messages in thread
From: Ian Campbell @ 2015-04-16 15:20 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, Julien Grall, tim, stefano.stabellini
On Thu, 2015-04-09 at 16:09 +0100, Julien Grall wrote:
> + root node;
> + * The following properies are mandatory with the /passthrough node:
"properties"
Otherwise: Acked-by: Ian Campbell <ian.campbell@citrix.com>
> + - compatible: It should always contain "simple-bus"
> + - ranges
> + - #address-cells
> + - #size-cells
> +
> +3) Compile the partial guest device with dtc (Device Tree Compiler).
> +For our purpose, the compiled file will be called guest-midway.dtb and
> +placed in /root in DOM0.
> +
> +3) Add the following options in the guest configuration file:
> +
> +device_tree = "/root/guest-midway.dtb"
> +dtdev = [ "/soc/ethernet@fff51000" ]
> +irqs = [ 112, 113, 114 ]
> +iomem = [ "0xfff51,1@0x10000" ]
> +
> +
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 04/19] xen/arm: Implement hypercall DOMCTL_{, un}bind_pt_pirq
2015-04-16 14:55 ` Ian Campbell
@ 2015-04-16 15:20 ` Julien Grall
2015-04-16 15:40 ` Ian Campbell
2015-04-17 23:05 ` Daniel De Graaf
1 sibling, 1 reply; 48+ messages in thread
From: Julien Grall @ 2015-04-16 15:20 UTC (permalink / raw)
To: Ian Campbell, Julien Grall, Daniel De Graaf
Cc: xen-devel, stefano.stabellini, Julien Grall, tim, Jan Beulich
Hi Ian,
On 16/04/2015 15:55, Ian Campbell wrote:
> On Thu, 2015-04-09 at 16:09 +0100, Julien Grall wrote:
>> From: Julien Grall <julien.grall@linaro.org>
>
> I've left the XSM related quotes untrimmed and CCd Daniel. I think it's
> all code motion (making x86 specific things generic), so perhaps no ack
> needed but an opportunity to nack instead ;-)
>
>> On x86, an IRQ is assigned in 2 steps to an HVM guest:
>> - The toolstack is calling PHYSDEVOP_map_pirq in order to create a
>> guest PIRQ (IRQ bound to an event channel)
>> - The emulator (QEMU) is calling DOMCTL_bind_pt_irq in order to
>> bind the IRQ
>>
>> On ARM, there is no concept of PIRQ as the IRQ can be assigned to a
>> virtual IRQ using the interrupt controller.
>>
>> It's not clear if we will need 2 different hypercalls on ARM to assign
>> IRQ and, for now, only the toolstack will manage IRQ.
>>
>> In order to avoid re-using a fixed ABI hypercall (PHYSDEVOP_*) for a
>> different purpose and allow us more time to figure out the right out,
>> only DOMCTL_{,un}bind_pt_pirq is implemented on ARM.
>>
>> The DOMCTL is extended with a new type PT_IRQ_TYPE_SPI and only IRQ ==
>> vIRQ (i.e machine_irq == spi) is supported.
>>
>> Concerning XSM, even if ARM is using one hypercall rather than 2, the
>> resulting check is nearly the same.
>>
>> XSM PHYSDEVOP_map_pirq:
>> 1) Check if the current domain can add resource to the domain
>> 2) Check if the current domain has permission to add the IRQ
>> 3) Check if the target domain has permission to use the IRQ
>>
>> XSM DOMCTL_bind_pirq_irq:
>> 1) Check if the current domain can add resource to the domain
>> 2) Check if the current domain has permission to bind the IRQ
>> 3) Check if the target domain has permission to use the IRQ
>>
>> In order to keep the same XSM checks done by the 2 hypercalls on x86,
>> call both xsm_map_domain_irq & xsm_bind_pt_irq in the ARM implementation.
>
> I think this paragraph makes the previous bit obsolete?
Do you mean: "Concerning XSM..." until and the 2 paragraphs for XSM
hypercalls? If so, yes.
>
>> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
>> index 1f2c80c..676ec50 100644
>> --- a/tools/libxc/xc_domain.c
>> +++ b/tools/libxc/xc_domain.c
>> @@ -1773,15 +1773,16 @@ int xc_domain_unbind_msi_irq(
>> }
>>
>> /* Pass-through: binds machine irq to guests irq */
>> -int xc_domain_bind_pt_irq(
>> - xc_interface *xch,
>> - uint32_t domid,
>> - uint8_t machine_irq,
>> - uint8_t irq_type,
>> - uint8_t bus,
>> - uint8_t device,
>> - uint8_t intx,
>> - uint8_t isa_irq)
>> +static int xc_domain_bind_pt_irq_int(
>> + xc_interface *xch,
>> + uint32_t domid,
>> + uint8_t machine_irq,
>> + uint8_t irq_type,
>> + uint8_t bus,
>> + uint8_t device,
>> + uint8_t intx,
>> + uint8_t isa_irq,
>> + uint16_t spi)
>
> Please either leave the indentation of the arguments as it is or fix it
> properly, i.e. put xch on the same line as the prototype and align
> everything else below it.
>
> TBH I'd prefer the former even if it isn't strictly coding style since
> it obscures the real change you made.
Ok. I will do it.
>> @@ -1878,6 +1913,25 @@ int xc_domain_bind_pt_isa_irq(
>> PT_IRQ_TYPE_ISA, 0, 0, 0, machine_irq));
>> }
>>
>> +int xc_domain_bind_pt_spi_irq(
>> + xc_interface *xch,
>> + uint32_t domid,
>> + uint16_t spi)
>> +{
>> + /* vSPI == SPI */
>
> I wonder if to avoid API churn later we should accept both machine and
> guest IRQ here and rely on the check that htey are the same in the
> hypervisor to reject?
>
> Fair enough we can change libxc interface if we want, but avoiding a
> little churn later on seems reasonable and it makes it a better fit with
> the existing interfaces.
what about the following prototype:
int xc_domain_bind_pt_spi_irq(
xc_interface *xch,
uint32_t domid,
uint16_t spi,
uint16_t vspi)
I didn't reuse the current name i.e (machine_irq) because I find the
name confusing.
[..]
>> + case XEN_DOMCTL_unbind_pt_irq:
>> + {
>> + int rc;
>> + xen_domctl_bind_pt_irq_t *bind = &domctl->u.bind_pt_irq;
>> + uint32_t irq = bind->u.spi.spi;
>> + uint32_t virq = bind->machine_irq;
>> +
>> + /* We only support PT_IRQ_TYPE_SPI */
>> + if ( bind->irq_type != PT_IRQ_TYPE_SPI )
>> + return -EOPNOTSUPP;
>> +
>> + /* For now map the interrupt 1:1 */
>> + if ( irq != virq )
>> + return -EINVAL;
>
> The x86 version doesn't appear to consider irq_type or irq, only virq
> (from ->machine_irq). That seems to be logical to me (if a little
> underdocumented) and I think we should be consistent.
On x86, the parameters are used later in pt_create_bind which take the
hypercall data in parameter.
The both check are required in order to avoid future issue if we
introduce new type and when we will support virq != irq.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 12/19] xen/passthrough: Extend XEN_DOMCTL_*assign_device to support DT device
2015-04-16 15:11 ` Ian Campbell
@ 2015-04-16 15:21 ` Julien Grall
0 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2015-04-16 15:21 UTC (permalink / raw)
To: Ian Campbell
Cc: Wei Liu, tim, Julien Grall, Ian Jackson, stefano.stabellini,
Jan Beulich, xen-devel
Hi Ian,
On 16/04/2015 16:11, Ian Campbell wrote:
> On Thu, 2015-04-09 at 16:09 +0100, Julien Grall wrote:
>> From: Julien Grall <julien.grall@linaro.org>
>>
>> A device node is described by a path. It will be used to retrieved the
>
> "retrieve"
>
>> node in the device tree and assign the related device to the domain.
>>
>> Only non-PCI protected by an IOMMU can be assigned to a guest.
>
> I think a word is missing between PCI and protected, perhaps "devices"?
Yes.
>
>> Also document the behavior of XEN_DOMCTL_deassign_device in the public
>> headers which differ between non-PCI and PCI.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>
> With the above fixed:
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Thanks,
--
Julien Grall
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 13/19] tools/libxl: Create a per-arch function to map IRQ to a domain
2015-04-16 15:12 ` Ian Campbell
@ 2015-04-16 15:26 ` Julien Grall
2015-04-16 15:42 ` Ian Campbell
0 siblings, 1 reply; 48+ messages in thread
From: Julien Grall @ 2015-04-16 15:26 UTC (permalink / raw)
To: Ian Campbell, Julien Grall
Cc: Wei Liu, tim, Julien Grall, Ian Jackson, stefano.stabellini, xen-devel
Hi Ian,
On 16/04/2015 16:12, Ian Campbell wrote:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> (as I did v4 with the typoes fixed)
I wasn't sure if I could keep your ack because the function used to map
the IRQ wasn't the same on v4.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 04/19] xen/arm: Implement hypercall DOMCTL_{, un}bind_pt_pirq
2015-04-16 15:20 ` Julien Grall
@ 2015-04-16 15:40 ` Ian Campbell
2015-04-17 6:02 ` Julien Grall
0 siblings, 1 reply; 48+ messages in thread
From: Ian Campbell @ 2015-04-16 15:40 UTC (permalink / raw)
To: Julien Grall
Cc: Julien Grall, tim, stefano.stabellini, Jan Beulich, xen-devel,
Daniel De Graaf
On Thu, 2015-04-16 at 16:20 +0100, Julien Grall wrote:
> >> Concerning XSM, even if ARM is using one hypercall rather than 2, the
> >> resulting check is nearly the same.
> >>
> >> XSM PHYSDEVOP_map_pirq:
> >> 1) Check if the current domain can add resource to the domain
> >> 2) Check if the current domain has permission to add the IRQ
> >> 3) Check if the target domain has permission to use the IRQ
> >>
> >> XSM DOMCTL_bind_pirq_irq:
> >> 1) Check if the current domain can add resource to the domain
> >> 2) Check if the current domain has permission to bind the IRQ
> >> 3) Check if the target domain has permission to use the IRQ
> >>
> >> In order to keep the same XSM checks done by the 2 hypercalls on x86,
> >> call both xsm_map_domain_irq & xsm_bind_pt_irq in the ARM implementation.
> >
> > I think this paragraph makes the previous bit obsolete?
>
> Do you mean: "Concerning XSM..." until and the 2 paragraphs for XSM
> hypercalls?
That's right.
> >> @@ -1878,6 +1913,25 @@ int xc_domain_bind_pt_isa_irq(
> >> PT_IRQ_TYPE_ISA, 0, 0, 0, machine_irq));
> >> }
> >>
> >> +int xc_domain_bind_pt_spi_irq(
> >> + xc_interface *xch,
> >> + uint32_t domid,
> >> + uint16_t spi)
> >> +{
> >> + /* vSPI == SPI */
> >
> > I wonder if to avoid API churn later we should accept both machine and
> > guest IRQ here and rely on the check that htey are the same in the
> > hypervisor to reject?
> >
> > Fair enough we can change libxc interface if we want, but avoiding a
> > little churn later on seems reasonable and it makes it a better fit with
> > the existing interfaces.
>
> what about the following prototype:
>
> int xc_domain_bind_pt_spi_irq(
> xc_interface *xch,
> uint32_t domid,
> uint16_t spi,
> uint16_t vspi)
>
> I didn't reuse the current name i.e (machine_irq) because I find the
> name confusing.
Sure. Although you hit machine_irq again at the next level in the stack
so I think it's rather moot.
>
> [..]
>
> >> + case XEN_DOMCTL_unbind_pt_irq:
> >> + {
> >> + int rc;
> >> + xen_domctl_bind_pt_irq_t *bind = &domctl->u.bind_pt_irq;
> >> + uint32_t irq = bind->u.spi.spi;
> >> + uint32_t virq = bind->machine_irq;
> >> +
> >> + /* We only support PT_IRQ_TYPE_SPI */
> >> + if ( bind->irq_type != PT_IRQ_TYPE_SPI )
> >> + return -EOPNOTSUPP;
> >> +
> >> + /* For now map the interrupt 1:1 */
> >> + if ( irq != virq )
> >> + return -EINVAL;
> >
> > The x86 version doesn't appear to consider irq_type or irq, only virq
> > (from ->machine_irq). That seems to be logical to me (if a little
> > underdocumented) and I think we should be consistent.
>
> On x86, the parameters are used later in pt_create_bind which take the
> hypercall data in parameter.
Ah yes, (although you mean pt_irq_destroy_bind I think?)
> The both check are required in order to avoid future issue if we
> introduce new type and when we will support virq != irq.
Shouldn't they be in pt_irq_destroy_bind then?
Ian.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 13/19] tools/libxl: Create a per-arch function to map IRQ to a domain
2015-04-16 15:26 ` Julien Grall
@ 2015-04-16 15:42 ` Ian Campbell
0 siblings, 0 replies; 48+ messages in thread
From: Ian Campbell @ 2015-04-16 15:42 UTC (permalink / raw)
To: Julien Grall
Cc: Wei Liu, tim, Julien Grall, Ian Jackson, stefano.stabellini, xen-devel
On Thu, 2015-04-16 at 16:26 +0100, Julien Grall wrote:
> Hi Ian,
>
> On 16/04/2015 16:12, Ian Campbell wrote:
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > (as I did v4 with the typoes fixed)
>
> I wasn't sure if I could keep your ack because the function used to map
> the IRQ wasn't the same on v4.
I wouldn't have minded, but no harm done.
Ian.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 04/19] xen/arm: Implement hypercall DOMCTL_{, un}bind_pt_pirq
2015-04-16 15:40 ` Ian Campbell
@ 2015-04-17 6:02 ` Julien Grall
2015-04-17 9:47 ` Ian Campbell
0 siblings, 1 reply; 48+ messages in thread
From: Julien Grall @ 2015-04-17 6:02 UTC (permalink / raw)
To: Ian Campbell, Julien Grall
Cc: Julien Grall, tim, stefano.stabellini, Jan Beulich, xen-devel,
Daniel De Graaf
Hi Ian,
On 16/04/2015 16:40, Ian Campbell wrote:
> On Thu, 2015-04-16 at 16:20 +0100, Julien Grall wrote:
>>>> Concerning XSM, even if ARM is using one hypercall rather than 2, the
>>>> resulting check is nearly the same.
>>>>
>>>> XSM PHYSDEVOP_map_pirq:
>>>> 1) Check if the current domain can add resource to the domain
>>>> 2) Check if the current domain has permission to add the IRQ
>>>> 3) Check if the target domain has permission to use the IRQ
>>>>
>>>> XSM DOMCTL_bind_pirq_irq:
>>>> 1) Check if the current domain can add resource to the domain
>>>> 2) Check if the current domain has permission to bind the IRQ
>>>> 3) Check if the target domain has permission to use the IRQ
>>>>
>>>> In order to keep the same XSM checks done by the 2 hypercalls on x86,
>>>> call both xsm_map_domain_irq & xsm_bind_pt_irq in the ARM implementation.
>>>
>>> I think this paragraph makes the previous bit obsolete?
>>
>> Do you mean: "Concerning XSM..." until and the 2 paragraphs for XSM
>> hypercalls?
>
> That's right.
Ok. I will drop it.
>>>> @@ -1878,6 +1913,25 @@ int xc_domain_bind_pt_isa_irq(
>>>> PT_IRQ_TYPE_ISA, 0, 0, 0, machine_irq));
>>>> }
>>>>
>>>> +int xc_domain_bind_pt_spi_irq(
>>>> + xc_interface *xch,
>>>> + uint32_t domid,
>>>> + uint16_t spi)
>>>> +{
>>>> + /* vSPI == SPI */
>>>
>>> I wonder if to avoid API churn later we should accept both machine and
>>> guest IRQ here and rely on the check that htey are the same in the
>>> hypervisor to reject?
>>>
>>> Fair enough we can change libxc interface if we want, but avoiding a
>>> little churn later on seems reasonable and it makes it a better fit with
>>> the existing interfaces.
>>
>> what about the following prototype:
>>
>> int xc_domain_bind_pt_spi_irq(
>> xc_interface *xch,
>> uint32_t domid,
>> uint16_t spi,
>> uint16_t vspi)
>>
>> I didn't reuse the current name i.e (machine_irq) because I find the
>> name confusing.
>
> Sure. Although you hit machine_irq again at the next level in the stack
> so I think it's rather moot.
>
>>
>> [..]
>>
>>>> + case XEN_DOMCTL_unbind_pt_irq:
>>>> + {
>>>> + int rc;
>>>> + xen_domctl_bind_pt_irq_t *bind = &domctl->u.bind_pt_irq;
>>>> + uint32_t irq = bind->u.spi.spi;
>>>> + uint32_t virq = bind->machine_irq;
>>>> +
>>>> + /* We only support PT_IRQ_TYPE_SPI */
>>>> + if ( bind->irq_type != PT_IRQ_TYPE_SPI )
>>>> + return -EOPNOTSUPP;
>>>> +
>>>> + /* For now map the interrupt 1:1 */
>>>> + if ( irq != virq )
>>>> + return -EINVAL;
>>>
>>> The x86 version doesn't appear to consider irq_type or irq, only virq
>>> (from ->machine_irq). That seems to be logical to me (if a little
>>> underdocumented) and I think we should be consistent.
>>
>> On x86, the parameters are used later in pt_create_bind which take the
>> hypercall data in parameter.
>
> Ah yes, (although you mean pt_irq_destroy_bind I think?)
No I mean pt_irq_create_bind. The function makes usage of machine_irq
and irq_type. Although, there is no clear switch case, just an if in the
code.
>> The both check are required in order to avoid future issue if we
>> introduce new type and when we will support virq != irq.
>
> Shouldn't they be in pt_irq_destroy_bind then?
I'm not following you. pt_irq_destroy_bind is an x86 specific function.
The check "virq != irq" is done in both DOMCTL_{,un}bind_irq on ARM even
though the one in unbind is not necessary useful.
Regards,
--
Julien Grall
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 04/19] xen/arm: Implement hypercall DOMCTL_{, un}bind_pt_pirq
2015-04-17 6:02 ` Julien Grall
@ 2015-04-17 9:47 ` Ian Campbell
0 siblings, 0 replies; 48+ messages in thread
From: Ian Campbell @ 2015-04-17 9:47 UTC (permalink / raw)
To: Julien Grall
Cc: Julien Grall, tim, stefano.stabellini, Jan Beulich, xen-devel,
Daniel De Graaf
On Fri, 2015-04-17 at 07:02 +0100, Julien Grall wrote:
> Hi Ian,
>
> On 16/04/2015 16:40, Ian Campbell wrote:
> > On Thu, 2015-04-16 at 16:20 +0100, Julien Grall wrote:
> >>>> Concerning XSM, even if ARM is using one hypercall rather than 2, the
> >>>> resulting check is nearly the same.
> >>>>
> >>>> XSM PHYSDEVOP_map_pirq:
> >>>> 1) Check if the current domain can add resource to the domain
> >>>> 2) Check if the current domain has permission to add the IRQ
> >>>> 3) Check if the target domain has permission to use the IRQ
> >>>>
> >>>> XSM DOMCTL_bind_pirq_irq:
> >>>> 1) Check if the current domain can add resource to the domain
> >>>> 2) Check if the current domain has permission to bind the IRQ
> >>>> 3) Check if the target domain has permission to use the IRQ
> >>>>
> >>>> In order to keep the same XSM checks done by the 2 hypercalls on x86,
> >>>> call both xsm_map_domain_irq & xsm_bind_pt_irq in the ARM implementation.
> >>>
> >>> I think this paragraph makes the previous bit obsolete?
> >>
> >> Do you mean: "Concerning XSM..." until and the 2 paragraphs for XSM
> >> hypercalls?
> >
> > That's right.
>
> Ok. I will drop it.
>
> >>>> @@ -1878,6 +1913,25 @@ int xc_domain_bind_pt_isa_irq(
> >>>> PT_IRQ_TYPE_ISA, 0, 0, 0, machine_irq));
> >>>> }
> >>>>
> >>>> +int xc_domain_bind_pt_spi_irq(
> >>>> + xc_interface *xch,
> >>>> + uint32_t domid,
> >>>> + uint16_t spi)
> >>>> +{
> >>>> + /* vSPI == SPI */
> >>>
> >>> I wonder if to avoid API churn later we should accept both machine and
> >>> guest IRQ here and rely on the check that htey are the same in the
> >>> hypervisor to reject?
> >>>
> >>> Fair enough we can change libxc interface if we want, but avoiding a
> >>> little churn later on seems reasonable and it makes it a better fit with
> >>> the existing interfaces.
> >>
> >> what about the following prototype:
> >>
> >> int xc_domain_bind_pt_spi_irq(
> >> xc_interface *xch,
> >> uint32_t domid,
> >> uint16_t spi,
> >> uint16_t vspi)
> >>
> >> I didn't reuse the current name i.e (machine_irq) because I find the
> >> name confusing.
> >
> > Sure. Although you hit machine_irq again at the next level in the stack
> > so I think it's rather moot.
> >
> >>
> >> [..]
> >>
> >>>> + case XEN_DOMCTL_unbind_pt_irq:
> >>>> + {
> >>>> + int rc;
> >>>> + xen_domctl_bind_pt_irq_t *bind = &domctl->u.bind_pt_irq;
> >>>> + uint32_t irq = bind->u.spi.spi;
> >>>> + uint32_t virq = bind->machine_irq;
> >>>> +
> >>>> + /* We only support PT_IRQ_TYPE_SPI */
> >>>> + if ( bind->irq_type != PT_IRQ_TYPE_SPI )
> >>>> + return -EOPNOTSUPP;
> >>>> +
> >>>> + /* For now map the interrupt 1:1 */
> >>>> + if ( irq != virq )
> >>>> + return -EINVAL;
> >>>
> >>> The x86 version doesn't appear to consider irq_type or irq, only virq
> >>> (from ->machine_irq). That seems to be logical to me (if a little
> >>> underdocumented) and I think we should be consistent.
> >>
> >> On x86, the parameters are used later in pt_create_bind which take the
> >> hypercall data in parameter.
> >
> > Ah yes, (although you mean pt_irq_destroy_bind I think?)
>
> No I mean pt_irq_create_bind.
My initial comment was talking specifically about
XEN_DOMCTL_unbind_pt_irq, which AFAICT does not call pt_irq_create_bind
(which is why I assumed you must mean unbind).
I agree that XEN_DOMCTL_bind_pt_irq should contain checks of all its
inputs, of course.
> The function makes usage of machine_irq
> and irq_type. Although, there is no clear switch case, just an if in the
> code.
>
> >> The both check are required in order to avoid future issue if we
> >> introduce new type and when we will support virq != irq.
> >
> > Shouldn't they be in pt_irq_destroy_bind then?
>
> I'm not following you. pt_irq_destroy_bind is an x86 specific function.
>
> The check "virq != irq" is done in both DOMCTL_{,un}bind_irq on ARM even
> though the one in unbind is not necessary useful.
This was exactly my point, on x86 XEN_DOMCTL_unbind_pt_irq didn't appear
to pay attention to anything other than the virq provided, I assumed
since it doesn't need any other information from the caller to do as it
has been asked.
But it seems like maybe I was wrong and pt_irq_destroy_bind (called from
XEN_DOMCTL_unbind_pt_irq on x86) does actually need other info (I'm not
sure why, it seems like it ought to know these things without being told
by the toolstack). In which case having your check for consistency with
x86 is probably tolerable.
Ian.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 04/19] xen/arm: Implement hypercall DOMCTL_{, un}bind_pt_pirq
2015-04-16 14:55 ` Ian Campbell
2015-04-16 15:20 ` Julien Grall
@ 2015-04-17 23:05 ` Daniel De Graaf
1 sibling, 0 replies; 48+ messages in thread
From: Daniel De Graaf @ 2015-04-17 23:05 UTC (permalink / raw)
To: Ian Campbell, Julien Grall
Cc: xen-devel, Julien Grall, tim, Jan Beulich, stefano.stabellini
On 04/16/2015 10:55 AM, Ian Campbell wrote:
> On Thu, 2015-04-09 at 16:09 +0100, Julien Grall wrote:
>> From: Julien Grall <julien.grall@linaro.org>
>
> I've left the XSM related quotes untrimmed and CCd Daniel. I think it's
> all code motion (making x86 specific things generic), so perhaps no ack
> needed but an opportunity to nack instead ;-)
This seems correct to me. My initial thought when looking at this problem
was that a distinct XSM hook for the ARM hypercall would be better, but
other than the minor overhead from doing the IRQ label lookups twice, there
is no real reason to prefer that method. This method has the advantage
of not making more architecture-specific hooks which are sometimes harder
to test/maintain.
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5 p2 15/19] tools/(lib)xl: Add partial device tree support for ARM
2015-04-09 16:13 ` Ian Jackson
@ 2015-04-28 13:30 ` Julien Grall
0 siblings, 0 replies; 48+ messages in thread
From: Julien Grall @ 2015-04-28 13:30 UTC (permalink / raw)
To: Ian Jackson, Julien Grall
Cc: Wei Liu, ian.campbell, Julien Grall, tim, stefano.stabellini, xen-devel
Hi Ian,
On 09/04/15 17:13, Ian Jackson wrote:
> Julien Grall writes ("[PATCH v5 p2 15/19] tools/(lib)xl: Add partial device tree support for ARM"):
>> Let the user to pass additional nodes to the guest device tree. For
>> this purpose, everything in the node /passthrough from the partial
>> device tree will be copied into the guest device tree.
>
> Thanks for explaining to me in person what I was missing about the
> semantics of DTs. As I said, you may wish to explain some of this in
> the commit message or some of the in-tree documents, perhaps with
> references. I will leave that up to you.
I will add a link to [1] in the in-tree documentation (patch #19).
[1] http://www.devicetree.org/Device_Tree_Usage#Basic_Data_Format
> In any case,
>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> Although:
>
>> + # Given the complexity of verifying the validity of a device tree,
>> + # libxl doesn't do any security check on it. It's the responsability
>
> Spelling mistake - should be "responsibility".
I will fix it.
Thanks,
--
Julien Grall
^ permalink raw reply [flat|nested] 48+ messages in thread
end of thread, other threads:[~2015-04-28 13:31 UTC | newest]
Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-09 15:09 [PATCH v5 p2 00/19] xen/arm Add support for non-PCI passthrough Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 01/19] xen/arm: Let the toolstack configure the number of SPIs Julien Grall
2015-04-16 14:39 ` Ian Campbell
2015-04-16 15:10 ` Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 02/19] xen/arm: vgic: Add spi_to_pending Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 03/19] xen/arm: Release IRQ routed to a domain when it's destroying Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 04/19] xen/arm: Implement hypercall DOMCTL_{, un}bind_pt_pirq Julien Grall
2015-04-16 14:55 ` Ian Campbell
2015-04-16 15:20 ` Julien Grall
2015-04-16 15:40 ` Ian Campbell
2015-04-17 6:02 ` Julien Grall
2015-04-17 9:47 ` Ian Campbell
2015-04-17 23:05 ` Daniel De Graaf
2015-04-09 15:09 ` [PATCH v5 p2 05/19] xen: guestcopy: Provide an helper to safely copy string from guest Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 06/19] xen/dts: Provide an helper to get a DT node from a path provided by a guest Julien Grall
2015-04-16 14:57 ` Ian Campbell
2015-04-09 15:09 ` [PATCH v5 p2 07/19] xen/passthrough: Introduce iommu_construct Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 08/19] xen/passthrough: arm: release the DT devices assigned to a guest earlier Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 09/19] xen/passthrough: iommu_deassign_device_dt: By default reassign device to nobody Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 10/19] xen/iommu: arm: Wire iommu DOMCTL for ARM Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 11/19] xen/xsm: Add helpers to check permission for device tree passthrough Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 12/19] xen/passthrough: Extend XEN_DOMCTL_*assign_device to support DT device Julien Grall
2015-04-16 15:11 ` Ian Campbell
2015-04-16 15:21 ` Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 13/19] tools/libxl: Create a per-arch function to map IRQ to a domain Julien Grall
2015-04-16 15:12 ` Ian Campbell
2015-04-16 15:26 ` Julien Grall
2015-04-16 15:42 ` Ian Campbell
2015-04-09 15:09 ` [PATCH v5 p2 14/19] tools/libxl: Check if fdt_{first, next}_subnode are present in libfdt Julien Grall
2015-04-16 15:13 ` Ian Campbell
2015-04-09 15:09 ` [PATCH v5 p2 15/19] tools/(lib)xl: Add partial device tree support for ARM Julien Grall
2015-04-09 16:13 ` Ian Jackson
2015-04-28 13:30 ` Julien Grall
2015-04-16 15:17 ` Ian Campbell
2015-04-09 15:09 ` [PATCH v5 p2 16/19] tools/libxl: arm: Use an higher value for the GIC phandle Julien Grall
2015-04-09 16:17 ` Ian Jackson
2015-04-09 16:33 ` Julien Grall
2015-04-09 16:52 ` Ian Jackson
2015-04-09 17:00 ` Julien Grall
2015-04-09 17:02 ` Julien Grall
2015-04-09 17:04 ` Ian Jackson
2015-04-13 12:07 ` Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 17/19] libxl: Add support for Device Tree passthrough Julien Grall
2015-04-09 16:14 ` Ian Jackson
2015-04-16 15:19 ` Ian Campbell
2015-04-09 15:09 ` [PATCH v5 p2 18/19] xl: Add new option dtdev Julien Grall
2015-04-09 15:09 ` [PATCH v5 p2 19/19] docs/misc: arm: Add documentation about Device Tree passthrough Julien Grall
2015-04-16 15:20 ` Ian Campbell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.