[Xen-devel] [PATCH v5 0/8] dom0less device assignment

[Xen-devel] [PATCH v5 0/8] dom0less device assignment

[Stefano Stabellini]

Hi all,

This small patch series adds device assignment support to Dom0less.
The last patch is the documentation.

Cheers,

Stefano


The following changes since commit f93abf0315efef861270c25d83c8047fd6a54ec4:

  xen: sched: Fix Arm build after commit f855dd9625 (2019-09-24 18:58:55 +0100)

are available in the Git repository at:

  http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git 

for you to fetch changes up to 66b8868b2c88117be43ff916ba7ba1743176cf0a:

  xen/arm: add dom0-less device assignment info to docs (2019-09-25 11:47:04 -0700)

----------------------------------------------------------------
Stefano Stabellini (8):
      xen/arm: introduce handle_device_interrupts
      xen/arm: export device_tree_get_reg and device_tree_get_u32
      xen/arm: introduce kinfo->guest_phandle_gic
      xen/arm: copy dtb fragment to guest dtb
      xen/arm: assign devices to boot domains
      xen/arm: handle "multiboot,device-tree" compatible nodes
      xen/arm: introduce nr_spis
      xen/arm: add dom0-less device assignment info to docs

 docs/misc/arm/device-tree/booting.txt |  44 +++-
 docs/misc/arm/passthrough.txt         | 101 +++++++++
 xen/arch/arm/bootfdt.c                |  10 +-
 xen/arch/arm/domain_build.c           | 384 +++++++++++++++++++++++++++++-----
 xen/arch/arm/kernel.c                 |  14 +-
 xen/arch/arm/setup.c                  |   1 +
 xen/include/asm-arm/kernel.h          |   5 +-
 xen/include/asm-arm/setup.h           |   7 +
 8 files changed, 510 insertions(+), 56 deletions(-)

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

[Xen-devel] [PATCH v5 1/8] xen/arm: introduce handle_device_interrupts

[Stefano Stabellini]

Move the interrupt handling code out of handle_device to a new function
so that it can be reused for dom0less VMs (it will be used in later
patches).

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
Acked-by: Julien Grall <julien.grall@arm.com>
---
Changes in v4:
- rename handle_interrupts to handle_device_interrupts
- improve in-code comment
- remove return 1 if mapping is done
- use unsigned

Changes in v3:
- add patch

The diff is hard to read but I just moved the interrupts related code
from handle_devices to a new function handle_device_interrupts, and very
little else.
---
 xen/arch/arm/domain_build.c | 80 +++++++++++++++++++++++--------------
 1 file changed, 51 insertions(+), 29 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index a0fee1ef13..21985628f0 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1239,41 +1239,22 @@ static int __init map_device_children(struct domain *d,
 }
 
 /*
- * For a given device node:
- *  - Give permission to the guest to manage IRQ and MMIO range
- *  - Retrieve the IRQ configuration (i.e edge/level) from device tree
- * When the device is not marked for guest passthrough:
- *  - Assign the device to the guest if it's protected by an IOMMU
- *  - Map the IRQs and iomem regions to DOM0
+ * handle_device_interrupts retrieves the interrupts configuration from
+ * a device tree node and maps those interrupts to the target domain.
+ *
+ * Returns:
+ *   < 0 error
+ *   0   success
  */
-static int __init handle_device(struct domain *d, struct dt_device_node *dev,
-                                p2m_type_t p2mt)
+static int __init handle_device_interrupts(struct domain *d,
+                                           struct dt_device_node *dev,
+                                           bool need_mapping)
 {
-    unsigned int nirq;
-    unsigned int naddr;
-    unsigned int i;
+    unsigned int i, nirq;
     int res;
     struct dt_raw_irq rirq;
-    u64 addr, size;
-    bool need_mapping = !dt_device_for_passthrough(dev);
 
     nirq = dt_number_of_irq(dev);
-    naddr = dt_number_of_address(dev);
-
-    dt_dprintk("%s passthrough = %d nirq = %d naddr = %u\n",
-               dt_node_full_name(dev), need_mapping, nirq, naddr);
-
-    if ( dt_device_is_protected(dev) && need_mapping )
-    {
-        dt_dprintk("%s setup iommu\n", dt_node_full_name(dev));
-        res = iommu_assign_dt_device(d, dev);
-        if ( res )
-        {
-            printk(XENLOG_ERR "Failed to setup the IOMMU for %s\n",
-                   dt_node_full_name(dev));
-            return res;
-        }
-    }
 
     /* Give permission and map IRQs */
     for ( i = 0; i < nirq; i++ )
@@ -1310,6 +1291,47 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev,
             return res;
     }
 
+    return 0;
+}
+
+/*
+ * For a given device node:
+ *  - Give permission to the guest to manage IRQ and MMIO range
+ *  - Retrieve the IRQ configuration (i.e edge/level) from device tree
+ * When the device is not marked for guest passthrough:
+ *  - Assign the device to the guest if it's protected by an IOMMU
+ *  - Map the IRQs and iomem regions to DOM0
+ */
+static int __init handle_device(struct domain *d, struct dt_device_node *dev,
+                                p2m_type_t p2mt)
+{
+    unsigned int naddr;
+    unsigned int i;
+    int res;
+    u64 addr, size;
+    bool need_mapping = !dt_device_for_passthrough(dev);
+
+    naddr = dt_number_of_address(dev);
+
+    dt_dprintk("%s passthrough = %d naddr = %u\n",
+               dt_node_full_name(dev), need_mapping, naddr);
+
+    if ( dt_device_is_protected(dev) && need_mapping )
+    {
+        dt_dprintk("%s setup iommu\n", dt_node_full_name(dev));
+        res = iommu_assign_dt_device(d, dev);
+        if ( res )
+        {
+            printk(XENLOG_ERR "Failed to setup the IOMMU for %s\n",
+                   dt_node_full_name(dev));
+            return res;
+        }
+    }
+
+    res = handle_device_interrupts(d, dev, need_mapping);
+    if ( res < 0 )
+        return res;
+
     /* Give permission and map MMIOs */
     for ( i = 0; i < naddr; i++ )
     {
-- 
2.17.1


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

[Xen-devel] [PATCH v5 2/8] xen/arm: export device_tree_get_reg and device_tree_get_u32

[Stefano Stabellini]

They'll be used in later patches.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>

---
Changes in v5:
- move declarations to xen/include/asm-arm/setup.h

Changes in v4:
- new patch
---
 xen/arch/arm/bootfdt.c      | 8 ++++----
 xen/include/asm-arm/setup.h | 6 ++++++
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 623173bc7f..a7810abb15 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -55,15 +55,15 @@ static bool __init device_tree_node_compatible(const void *fdt, int node,
     return false;
 }
 
-static void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
-                                       u32 size_cells, u64 *start, u64 *size)
+void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
+                                u32 size_cells, u64 *start, u64 *size)
 {
     *start = dt_next_cell(address_cells, cell);
     *size = dt_next_cell(size_cells, cell);
 }
 
-static u32 __init device_tree_get_u32(const void *fdt, int node,
-                                      const char *prop_name, u32 dflt)
+u32 __init device_tree_get_u32(const void *fdt, int node,
+                               const char *prop_name, u32 dflt)
 {
     const struct fdt_property *prop;
 
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index efcba545c2..fa0a8721b2 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -115,6 +115,12 @@ const char *boot_module_kind_as_string(bootmodule_kind kind);
 extern uint32_t hyp_traps_vector[];
 void init_traps(void);
 
+void device_tree_get_reg(const __be32 **cell, u32 address_cells,
+                         u32 size_cells, u64 *start, u64 *size);
+
+u32 device_tree_get_u32(const void *fdt, int node,
+                        const char *prop_name, u32 dflt);
+
 #endif
 /*
  * Local variables:
-- 
2.17.1


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

[Xen-devel] [PATCH v5 3/8] xen/arm: introduce kinfo->guest_phandle_gic

[Stefano Stabellini]

Instead of always hard-coding the GIC phandle (GUEST_PHANDLE_GIC), store
it in a variable under kinfo. This way it can be dynamically chosen per
domain. Remove the fdt pointer argument to the make_*_domU_node
functions and oass a struct kernel_info * instead. The fdt pointer can
be accessed from kinfo->fdt. Remove the struct domain *d parameter to
the make_*_domU_node functions because it becomes unused.

Initialize guest_phandle_gic to GUEST_PHANDLE_GIC at the beginning of
prepare_dtb_domU. Later patches will change the value of
guest_phandle_gic depending on user provided information.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>

---
Changes in v5:
- improve commit message
Changes in v4:
- new patch
---
 xen/arch/arm/domain_build.c  | 36 +++++++++++++++++++++---------------
 xen/include/asm-arm/kernel.h |  3 +++
 2 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 21985628f0..32f85cd959 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -627,7 +627,8 @@ static int __init fdt_property_interrupts(const struct kernel_info *kinfo,
 {
     int res;
     uint32_t phandle = is_hardware_domain(kinfo->d) ?
-                       dt_interrupt_controller->phandle : GUEST_PHANDLE_GIC;
+                       dt_interrupt_controller->phandle :
+                       kinfo->guest_phandle_gic;
 
     res = fdt_property(kinfo->fdt, "interrupts",
                        intr, sizeof(intr[0]) * num_irq);
@@ -1537,8 +1538,9 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
     return res;
 }
 
-static int __init make_gicv2_domU_node(const struct domain *d, void *fdt)
+static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
 {
+    void *fdt = kinfo->fdt;
     int res = 0;
     __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2];
     __be32 *cells;
@@ -1573,11 +1575,11 @@ static int __init make_gicv2_domU_node(const struct domain *d, void *fdt)
     if (res)
         return res;
 
-    res = fdt_property_cell(fdt, "linux,phandle", GUEST_PHANDLE_GIC);
+    res = fdt_property_cell(fdt, "linux,phandle", kinfo->guest_phandle_gic);
     if (res)
         return res;
 
-    res = fdt_property_cell(fdt, "phandle", GUEST_PHANDLE_GIC);
+    res = fdt_property_cell(fdt, "phandle", kinfo->guest_phandle_gic);
     if (res)
         return res;
 
@@ -1586,8 +1588,9 @@ static int __init make_gicv2_domU_node(const struct domain *d, void *fdt)
     return res;
 }
 
-static int __init make_gicv3_domU_node(const struct domain *d, void *fdt)
+static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
 {
+    void *fdt = kinfo->fdt;
     int res = 0;
     __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2];
     __be32 *cells;
@@ -1622,11 +1625,11 @@ static int __init make_gicv3_domU_node(const struct domain *d, void *fdt)
     if (res)
         return res;
 
-    res = fdt_property_cell(fdt, "linux,phandle", GUEST_PHANDLE_GIC);
+    res = fdt_property_cell(fdt, "linux,phandle", kinfo->guest_phandle_gic);
     if (res)
         return res;
 
-    res = fdt_property_cell(fdt, "phandle", GUEST_PHANDLE_GIC);
+    res = fdt_property_cell(fdt, "phandle", kinfo->guest_phandle_gic);
     if (res)
         return res;
 
@@ -1635,22 +1638,23 @@ static int __init make_gicv3_domU_node(const struct domain *d, void *fdt)
     return res;
 }
 
-static int __init make_gic_domU_node(const struct domain *d, void *fdt)
+static int __init make_gic_domU_node(struct kernel_info *kinfo)
 {
-    switch ( d->arch.vgic.version )
+    switch ( kinfo->d->arch.vgic.version )
     {
     case GIC_V3:
-        return make_gicv3_domU_node(d, fdt);
+        return make_gicv3_domU_node(kinfo);
     case GIC_V2:
-        return make_gicv2_domU_node(d, fdt);
+        return make_gicv2_domU_node(kinfo);
     default:
         panic("Unsupported GIC version\n");
     }
 }
 
 #ifdef CONFIG_SBSA_VUART_CONSOLE
-static int __init make_vpl011_uart_node(const struct domain *d, void *fdt)
+static int __init make_vpl011_uart_node(struct kernel_info *kinfo)
 {
+    void *fdt = kinfo->fdt;
     int res;
     gic_interrupt_t intr;
     __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS];
@@ -1681,7 +1685,7 @@ static int __init make_vpl011_uart_node(const struct domain *d, void *fdt)
         return res;
 
     res = fdt_property_cell(fdt, "interrupt-parent",
-                            GUEST_PHANDLE_GIC);
+                            kinfo->guest_phandle_gic);
     if ( res )
         return res;
 
@@ -1706,6 +1710,8 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
     int addrcells, sizecells;
     int ret;
 
+    kinfo->guest_phandle_gic = GUEST_PHANDLE_GIC;
+
     addrcells = GUEST_ROOT_ADDRESS_CELLS;
     sizecells = GUEST_ROOT_SIZE_CELLS;
 
@@ -1749,7 +1755,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
     if ( ret )
         goto err;
 
-    ret = make_gic_domU_node(d, kinfo->fdt);
+    ret = make_gic_domU_node(kinfo);
     if ( ret )
         goto err;
 
@@ -1761,7 +1767,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
     {
         ret = -EINVAL;
 #ifdef CONFIG_SBSA_VUART_CONSOLE
-        ret = make_vpl011_uart_node(d, kinfo->fdt);
+        ret = make_vpl011_uart_node(kinfo);
 #endif
         if ( ret )
             goto err;
diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h
index 33f3e72b11..760434369b 100644
--- a/xen/include/asm-arm/kernel.h
+++ b/xen/include/asm-arm/kernel.h
@@ -36,6 +36,9 @@ struct kernel_info {
     /* Enable pl011 emulation */
     bool vpl011;
 
+    /* GIC phandle */
+    uint32_t guest_phandle_gic;
+
     /* loader to use for this kernel */
     void (*load)(struct kernel_info *info);
     /* loader specific state */
-- 
2.17.1


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

[Xen-devel] [PATCH v5 4/8] xen/arm: copy dtb fragment to guest dtb

[Stefano Stabellini]

Read the dtb fragment corresponding to a passthrough device from memory
at the location referred to by the "multiboot,device-tree" compatible
node.

Add a new field named dtb_bootmodule to struct kernel_info to keep track
of the dtb fragment location.

Copy the fragment to the guest dtb (only /aliases and /passthrough).

Set kinfo->guest_phandle_gic based on the phandle of the special "/gic"
node in the device tree fragment. "/gic" is a dummy node in the dtb
fragment that represents the gic interrupt controller. Other properties
in the dtb fragment might refer to it (for instance interrupt-parent of
a device node). We reuse the phandle of "/gic" from the dtb fragment as
the phandle of the full GIC node that will be created for the guest
device tree. That way, when we copy properties from the device tree
fragment to the domU device tree the links remain unbroken.

Some of the code below is taken from tools/libxl/libxl_arm.c. Note that
it is OK to take LGPL 2.1 code and including it into a GPLv2 code base.
The result is GPLv2 code.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>

----
Changes in v5:
- code style
- in-code comment
- remove depth parameter from scan_pfdt_node
- for instead of loop in domain_handle_dtb_bootmodule
- move "gic" check to domain_handle_dtb_bootmodule
- add check_partial_fdt
- use DT_ROOT_NODE_ADDR/SIZE_CELLS_DEFAULT
- add scan_passthrough_prop parameter, set it to false for "/aliases"

Changes in v4:
- use recursion in the implementation
- rename handle_properties to handle_prop_pfdt
- rename scan_pt_node to scan_pfdt_node
- pass kinfo to handle_properties
- use uint32_t instead of u32
- rename r to res
- add "passthrough" and "aliases" check
- add a name == NULL check
- code style
- move DTB fragment scanning earlier, before DomU GIC node creation
- set guest_phandle_gic based on "/gic"
- in-code comment

Changes in v3:
- switch to using device_tree_for_each_node for the copy

Changes in v2:
- add a note about the code coming from libxl in the commit message
- copy /aliases
- code style
---
 xen/arch/arm/domain_build.c  | 156 +++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/kernel.h |   2 +-
 2 files changed, 157 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 32f85cd959..9d985d3bbe 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -14,6 +14,7 @@
 #include <xen/guest_access.h>
 #include <xen/iocap.h>
 #include <xen/acpi.h>
+#include <xen/vmap.h>
 #include <xen/warning.h>
 #include <acpi/actables.h>
 #include <asm/device.h>
@@ -1700,6 +1701,154 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo)
 }
 #endif
 
+static int __init handle_prop_pfdt(struct kernel_info *kinfo,
+                                   const void *pfdt, int nodeoff,
+                                   uint32_t address_cells, uint32_t size_cells,
+                                   bool scan_passthrough_prop)
+{
+    void *fdt = kinfo->fdt;
+    int propoff, nameoff, res;
+    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);
+        res = fdt_property(fdt, fdt_string(pfdt, nameoff),
+                           prop->data, fdt32_to_cpu(prop->len));
+        if ( res )
+            return res;
+    }
+
+    /* FDT_ERR_NOTFOUND => There is no more properties for this node */
+    return ( propoff != -FDT_ERR_NOTFOUND ) ? propoff : 0;
+}
+
+static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt,
+                                 int nodeoff,
+                                 uint32_t address_cells, uint32_t size_cells,
+                                 bool scan_passthrough_prop)
+{
+    int rc = 0;
+    void *fdt = kinfo->fdt;
+    int node_next;
+
+    rc = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL));
+    if ( rc )
+        return rc;
+
+    rc = handle_prop_pfdt(kinfo, pfdt, nodeoff, address_cells, size_cells,
+                          scan_passthrough_prop);
+    if ( rc )
+        return rc;
+
+    address_cells = device_tree_get_u32(pfdt, nodeoff, "#address-cells",
+                                        DT_ROOT_NODE_ADDR_CELLS_DEFAULT);
+    size_cells = device_tree_get_u32(pfdt, nodeoff, "#size-cells",
+                                     DT_ROOT_NODE_SIZE_CELLS_DEFAULT);
+
+    node_next = fdt_first_subnode(pfdt, nodeoff);
+    while ( node_next > 0 )
+    {
+        scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells,
+                       scan_passthrough_prop);
+        node_next = fdt_next_subnode(pfdt, node_next);
+    }
+
+    return fdt_end_node(fdt);
+}
+
+static int __init check_partial_fdt(void *pfdt, size_t size)
+{
+    int res;
+
+    if (fdt_magic(pfdt) != FDT_MAGIC) {
+        dprintk(XENLOG_ERR, "Partial FDT is not a valid Flat Device Tree");
+        return -EINVAL;
+    }
+
+    res = fdt_check_header(pfdt);
+    if (res) {
+        dprintk(XENLOG_ERR, "Failed to check the partial FDT (%d)", res);
+        return -EINVAL;
+    }
+
+    if (fdt_totalsize(pfdt) > size) {
+        dprintk(XENLOG_ERR, "Partial FDT totalsize is too big");
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
+static int __init domain_handle_dtb_bootmodule(struct domain *d,
+                                               struct kernel_info *kinfo)
+{
+    void *pfdt;
+    int res, node_next;
+
+    pfdt = ioremap_cache(kinfo->dtb_bootmodule->start,
+                         kinfo->dtb_bootmodule->size);
+    if ( pfdt == NULL )
+        return -EFAULT;
+
+    res = check_partial_fdt(pfdt, kinfo->dtb_bootmodule->size);
+    if ( res < 0 )
+        return res;
+
+    for ( node_next = fdt_first_subnode(pfdt, 0); 
+          node_next > 0;
+          node_next = fdt_next_subnode(pfdt, node_next) )
+    {
+        const char *name = fdt_get_name(pfdt, node_next, NULL);
+
+        if ( name == NULL )
+            continue;
+
+        /*
+         * Only scan /gic /aliases /passthrough, ignore the rest.
+         * They don't have to be parsed in order.
+         *
+         * Take the GIC phandle value from the special /gic node in the
+         * DTB fragment.
+         */
+        if ( dt_node_cmp(name, "gic") == 0 )
+        {
+            kinfo->guest_phandle_gic = fdt_get_phandle(pfdt, node_next);
+            continue;
+        }
+
+        if ( dt_node_cmp(name, "aliases") == 0 )
+        {
+            res = scan_pfdt_node(kinfo, pfdt, node_next,
+                    DT_ROOT_NODE_ADDR_CELLS_DEFAULT,
+                    DT_ROOT_NODE_SIZE_CELLS_DEFAULT,
+                    false);
+            if ( res )
+                return res;
+            continue;
+        }
+        if ( dt_node_cmp(name, "passthrough") == 0 )
+        {
+            res = scan_pfdt_node(kinfo, pfdt, node_next,
+                    DT_ROOT_NODE_ADDR_CELLS_DEFAULT,
+                    DT_ROOT_NODE_SIZE_CELLS_DEFAULT,
+                    true);
+            if ( res )
+                return res;
+            continue;
+        }
+    }
+
+    iounmap(pfdt);
+
+    return res;
+}
+
 /*
  * The max size for DT is 2MB. However, the generated DT is small, 4KB
  * are enough for now, but we might have to increase it in the future.
@@ -1755,6 +1904,13 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
     if ( ret )
         goto err;
 
+    if ( kinfo->dtb_bootmodule )
+    {
+        ret = domain_handle_dtb_bootmodule(d, kinfo);
+        if ( ret )
+            return ret;
+    }
+
     ret = make_gic_domU_node(kinfo);
     if ( ret )
         goto err;
diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h
index 760434369b..7f5e659561 100644
--- a/xen/include/asm-arm/kernel.h
+++ b/xen/include/asm-arm/kernel.h
@@ -28,7 +28,7 @@ struct kernel_info {
     paddr_t gnttab_size;
 
     /* boot blob load addresses */
-    const struct bootmodule *kernel_bootmodule, *initrd_bootmodule;
+    const struct bootmodule *kernel_bootmodule, *initrd_bootmodule, *dtb_bootmodule;
     const char* cmdline;
     paddr_t dtb_paddr;
     paddr_t initrd_paddr;
-- 
2.17.1


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

[Xen-devel] [PATCH v5 5/8] xen/arm: assign devices to boot domains

[Stefano Stabellini]

Scan the user provided dtb fragment at boot. For each device node, map
memory to guests, and route interrupts and setup the iommu.

The memory region to remap is specified by the "xen,reg" property.

The iommu is setup by passing the node of the device to assign on the
host device tree. The path is specified in the device tree fragment as
the "xen,path" string property.

The interrupts are remapped based on the information from the
corresponding node on the host device tree. Call
handle_device_interrupts to remap interrupts. Interrupts related device
tree properties are copied from the device tree fragment, same as all
the other properties.

Also add the new flag XEN_DOMCTL_CDF_iommu to that dom0less domU can use
the IOMMU.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
Changes in v5:
- use local variable for name
- use map_regions_p2mt
- add warning for not page aligned addresses/sizes
- introduce handle_passthrough_prop

Changes in v4:
- use unsigned
- improve commit message
- code style
- use dt_prop_cmp
- use device_tree_get_reg
- don't copy over xen,reg and xen,path
- don't create special interrupt properties for domU: copy them from the
  fragment
- in-code comment

Changes in v3:
- improve commit message
- remove superfluous cast
- merge code with the copy code
- add interrup-parent
- demove depth > 2 check
- reuse code from handle_device_interrupts
- copy interrupts from host dt

Changes in v2:
- rename "path" to "xen,path"
- grammar fix
- use gaddr_to_gfn and maddr_to_mfn
- remove depth <= 2 limitation in scanning the dtb fragment
- introduce and parse xen,reg
- code style
- support more than one interrupt per device
- specify only the GIC is supported
---
 xen/arch/arm/domain_build.c | 101 ++++++++++++++++++++++++++++++++++--
 1 file changed, 97 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 9d985d3bbe..414893bc24 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1701,6 +1701,85 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo)
 }
 #endif
 
+/*
+ * Scan device tree properties for passthrough specific information.
+ * Returns -ENOENT when no passthrough properties are found
+ *         < 0 on error
+ *         0 on success
+ */
+static int __init handle_passthrough_prop(struct kernel_info *kinfo,
+                                          const struct fdt_property *prop,
+                                          const char *name,
+                                          uint32_t address_cells, uint32_t size_cells)
+{
+    const __be32 *cell;
+    unsigned int i, len;
+    struct dt_device_node *node;
+    int res;
+
+    /* xen,reg specifies where to map the MMIO region */
+    if ( dt_prop_cmp("xen,reg", name) == 0 )
+    {
+        paddr_t mstart, size, gstart;
+        cell = (const __be32 *)prop->data;
+        len = fdt32_to_cpu(prop->len) /
+            ((address_cells * 2 + size_cells) * sizeof(uint32_t));
+
+        for ( i = 0; i < len; i++ )
+        {
+            device_tree_get_reg(&cell, address_cells, size_cells,
+                    &mstart, &size);
+            gstart = dt_next_cell(address_cells, &cell);
+
+            if ( gstart & ~PAGE_MASK || mstart & ~PAGE_MASK || size & ~PAGE_MASK )
+                dprintk(XENLOG_WARNING,
+                        "DomU passthrough config has not page aligned addresses/sizes\n");
+
+            res = map_regions_p2mt(kinfo->d,
+                    gaddr_to_gfn(gstart),
+                    PFN_DOWN(size),
+                    maddr_to_mfn(mstart),
+                    p2m_mmio_direct_dev);
+            if ( res < 0 )
+            {
+                dprintk(XENLOG_ERR,
+                        "Failed to map %"PRIpaddr" to the guest at%"PRIpaddr"\n",
+                        mstart, gstart);
+                return -EFAULT;
+            }
+        }
+
+        return 0;
+    }
+    /*
+     * xen,path specifies the corresponding node in the host DT.
+     * Both interrupt mappings and IOMMU settings are based on it,
+     * as they are done based on the corresponding host DT node.
+     */
+    else if ( dt_prop_cmp("xen,path", name) == 0 )
+    {
+        node = dt_find_node_by_path(prop->data);
+        if ( node == NULL )
+        {
+            dprintk(XENLOG_ERR, "Couldn't find node %s in host_dt!\n",
+                    (char *)prop->data);
+            return -EINVAL;
+        }
+
+        res = iommu_assign_dt_device(kinfo->d, node);
+        if ( res < 0 )
+            return res;
+
+        res = handle_device_interrupts(kinfo->d, node, true);
+        if ( res < 0 )
+            return res;
+
+        return 0;
+    }
+
+    return -ENOENT;
+}
+
 static int __init handle_prop_pfdt(struct kernel_info *kinfo,
                                    const void *pfdt, int nodeoff,
                                    uint32_t address_cells, uint32_t size_cells,
@@ -1709,6 +1788,7 @@ static int __init handle_prop_pfdt(struct kernel_info *kinfo,
     void *fdt = kinfo->fdt;
     int propoff, nameoff, res;
     const struct fdt_property *prop;
+    const char *name;
 
     for ( propoff = fdt_first_property_offset(pfdt, nodeoff);
           propoff >= 0;
@@ -1717,11 +1797,23 @@ static int __init handle_prop_pfdt(struct kernel_info *kinfo,
         if ( !(prop = fdt_get_property_by_offset(pfdt, propoff, NULL)) )
             return -FDT_ERR_INTERNAL;
 
+        res = 0;
         nameoff = fdt32_to_cpu(prop->nameoff);
-        res = fdt_property(fdt, fdt_string(pfdt, nameoff),
-                           prop->data, fdt32_to_cpu(prop->len));
-        if ( res )
+        name = fdt_string(pfdt, nameoff);
+
+        if ( scan_passthrough_prop )
+            res = handle_passthrough_prop(kinfo, prop, name,
+                                          address_cells, size_cells);
+        if ( res < 0 && res != -ENOENT )
             return res;
+
+        /* copy all other properties */
+        if ( !scan_passthrough_prop || res == -ENOENT )
+        {
+            res = fdt_property(fdt, name, prop->data, fdt32_to_cpu(prop->len));
+            if ( res )
+                return res;
+        }
     }
 
     /* FDT_ERR_NOTFOUND => There is no more properties for this node */
@@ -2254,7 +2346,8 @@ void __init create_domUs(void)
         struct xen_domctl_createdomain d_cfg = {
             .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE,
             .arch.nr_spis = 0,
-            .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
+            .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
+                     XEN_DOMCTL_CDF_iommu,
             .max_evtchn_port = -1,
             .max_grant_frames = 64,
             .max_maptrack_frames = 1024,
-- 
2.17.1


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

[Xen-devel] [PATCH v5 6/8] xen/arm: handle "multiboot, device-tree" compatible nodes

[Stefano Stabellini]

Detect "multiboot,device-tree" compatible nodes. Add them to the bootmod
array as BOOTMOD_GUEST_DTB.  In kernel_probe, find the right
BOOTMOD_GUEST_DTB and store a pointer to it in dtb_bootmodule.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
Acked-by: Julien Grall <julien.grall@arm.com>
---
Changes in v4:
- use uint32_t
- remove useless 0 initialization
- add return value check

Changes in v2:
- rename BOOTMOD_DTB to BOOTMOD_GUEST_DTB
- rename multiboot,dtb to multiboot,device-tree
---
 xen/arch/arm/bootfdt.c      |  2 ++
 xen/arch/arm/kernel.c       | 14 +++++++++++++-
 xen/arch/arm/setup.c        |  1 +
 xen/include/asm-arm/setup.h |  1 +
 4 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index a7810abb15..08fb59f4e7 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -242,6 +242,8 @@ static void __init process_multiboot_node(const void *fdt, int node,
         kind = BOOTMOD_RAMDISK;
     else if ( fdt_node_check_compatible(fdt, node, "xen,xsm-policy") == 0 )
         kind = BOOTMOD_XSM;
+    else if ( fdt_node_check_compatible(fdt, node, "multiboot,device-tree") == 0 )
+        kind = BOOTMOD_GUEST_DTB;
     else
         kind = BOOTMOD_UNKNOWN;
 
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 389bef2afa..8eff074836 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -425,7 +425,7 @@ int __init kernel_probe(struct kernel_info *info,
     struct bootmodule *mod = NULL;
     struct bootcmdline *cmd = NULL;
     struct dt_device_node *node;
-    u64 kernel_addr, initrd_addr, size;
+    u64 kernel_addr, initrd_addr, dtb_addr, size;
     int rc;
 
     /* domain is NULL only for the hardware domain */
@@ -469,6 +469,18 @@ int __init kernel_probe(struct kernel_info *info,
                 info->initrd_bootmodule = boot_module_find_by_addr_and_kind(
                         BOOTMOD_RAMDISK, initrd_addr);
             }
+            else if ( dt_device_is_compatible(node, "multiboot,device-tree") )
+            {
+                uint32_t len;
+                const __be32 *val;
+
+                val = dt_get_property(node, "reg", &len);
+                if ( val == NULL )
+                    continue;
+                dt_get_range(&val, node, &dtb_addr, &size);
+                info->dtb_bootmodule = boot_module_find_by_addr_and_kind(
+                        BOOTMOD_GUEST_DTB, dtb_addr);
+            }
             else
                 continue;
         }
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index fca7e68cba..d764a4e9b2 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -369,6 +369,7 @@ const char * __init boot_module_kind_as_string(bootmodule_kind kind)
     case BOOTMOD_KERNEL:  return "Kernel";
     case BOOTMOD_RAMDISK: return "Ramdisk";
     case BOOTMOD_XSM:     return "XSM";
+    case BOOTMOD_GUEST_DTB:     return "DTB";
     case BOOTMOD_UNKNOWN: return "Unknown";
     default: BUG();
     }
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index fa0a8721b2..2f8f24e286 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -16,6 +16,7 @@ typedef enum {
     BOOTMOD_KERNEL,
     BOOTMOD_RAMDISK,
     BOOTMOD_XSM,
+    BOOTMOD_GUEST_DTB,
     BOOTMOD_UNKNOWN
 }  bootmodule_kind;
 
-- 
2.17.1


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

[Xen-devel] [PATCH v5 7/8] xen/arm: introduce nr_spis

[Stefano Stabellini]

We don't have a clear way to know how many virtual SPIs we need for the
dom0-less domains. Introduce a new option under xen,domain to specify
the number of SPIs to allocate for a domain.

The property is optional. When absent, we'll use the physical number of
GIC lines for dom0-less domains, just like for dom0.

Remove the old setting of nr_spis based on the presence of the vpl011.

The implication of this change is that without nr_spis dom0less domains
get the same amount of SPI allocated as dom0, regardless of how many
physical devices they have assigned, and regardless of whether they have
a virtual pl011 (which also needs an emulated SPI). For instance, we
could end up exposing 256 SPIs for each dom0less domain without a
nr_spis property. If we have 4 dom0less domains without nr_spis, it
would result in 80K of additional memory being used.

When nr_spis is present, the domain gets exactly nr_spis allocated SPIs.
If the number is too low, it might not be enough for the devices
assigned it to it. If the number is less than GUEST_VPL011_SPI, the
virtual pl011 won't work.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
Changes in v5:
- improve commit message
- allocate enough SPIs for vpl011

Changes in v4:
- improve commit message

Changes in v3:
- improve commit message
- introduce nr_spis
---
 xen/arch/arm/domain_build.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 414893bc24..bf4d960eb5 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2345,7 +2345,6 @@ void __init create_domUs(void)
         struct domain *d;
         struct xen_domctl_createdomain d_cfg = {
             .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE,
-            .arch.nr_spis = 0,
             .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
                      XEN_DOMCTL_CDF_iommu,
             .max_evtchn_port = -1,
@@ -2356,13 +2355,23 @@ void __init create_domUs(void)
         if ( !dt_device_is_compatible(node, "xen,domain") )
             continue;
 
-        if ( dt_property_read_bool(node, "vpl011") )
-            d_cfg.arch.nr_spis = GUEST_VPL011_SPI - 32 + 1;
-
         if ( !dt_property_read_u32(node, "cpus", &d_cfg.max_vcpus) )
             panic("Missing property 'cpus' for domain %s\n",
                   dt_node_name(node));
 
+        if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
+        {
+            d_cfg.arch.nr_spis = gic_number_lines() - 32;
+
+            /*
+             * vpl011 uses one emulated SPI. If vpl011 is requested, make
+             * sure that we allocate enough SPIs for it.
+             */
+            if ( dt_property_read_bool(node, "vpl011") )
+                d_cfg.arch.nr_spis = MAX(d_cfg.arch.nr_spis,
+                                         GUEST_VPL011_SPI - 32 + 1);
+        }
+
         d = domain_create(++max_init_domid, &d_cfg, false);
         if ( IS_ERR(d) )
             panic("Error creating domain %s\n", dt_node_name(node));
-- 
2.17.1


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

[Xen-devel] [PATCH v5 8/8] xen/arm: add dom0-less device assignment info to docs

[Stefano Stabellini]

Add info about the SPI used for the virtual pl011.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>

---
Changes in v5:
- improve wording

Changes in v4:
- fix spelling
- add "multiboot,module"
- improve commit message
- improve doc
- expand the nr_spis and vpl011 sections and include information about
  the vpl011 SPI
- move passthrough information to docs/misc/arm/passthrough.txt

Changes in v3:
- add nr_spis
- change description of interrupts and interrupt-parent

Changes in v2:
- device tree fragment loaded in cacheable memory
- rename multiboot,dtb to multiboot,device-tree
- rename "path" to "xen,path"
- add a note about device memory mapping
- introduce xen,reg
- specify only the GIC is supported
---
 docs/misc/arm/device-tree/booting.txt |  44 ++++++++++-
 docs/misc/arm/passthrough.txt         | 101 ++++++++++++++++++++++++++
 2 files changed, 144 insertions(+), 1 deletion(-)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 317a9e962a..9d34f9ab39 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -146,7 +146,18 @@ with the following properties:
 
 - vpl011
 
-    An empty property to enable/disable a virtual pl011 for the guest to use.
+    An empty property to enable/disable a virtual pl011 for the guest to
+    use. The virtual pl011 uses SPI number 0 (see GUEST_VPL011_SPI).
+    Please note that the SPI used for the virtual pl011 could clash with the
+    physical SPI of a physical device assigned to the guest.
+
+- nr_spis
+
+    Optional. A 32-bit integer specifying the number of SPIs (Shared
+    Peripheral Interrupts) to allocate for the domain. If nr_spis is
+    missing, the max number of SPIs supported by the physical GIC is
+    used. If both vpl011 and nr_spis are set, nr_spis should be at least
+    1 to account for the SPI used by the virtual pl011.
 
 - #address-cells and #size-cells
 
@@ -226,3 +237,34 @@ chosen {
         };
     };
 };
+
+
+Device Assignment
+=================
+
+Device Assignment (Passthrough) is supported by adding another module,
+alongside the kernel and ramdisk, with the device tree fragment
+corresponding to the device node to assign to the guest.
+
+The dtb sub-node should have the following properties:
+
+- compatible
+
+    "multiboot,device-tree" and "multiboot,module"
+
+- reg
+
+    Specifies the physical address of the device tree binary fragment
+    RAM and its length.
+
+As an example:
+
+        module@0xc000000 {
+            compatible = "multiboot,device-tree", "multiboot,module";
+            reg = <0x0 0xc000000 0xffffff>;
+        };
+
+The DTB fragment is loaded at 0xc000000 in the example above. It should
+follow the convention explained in docs/misc/arm/passthrough.txt. The
+DTB fragment will be added to the guest device tree, so that the guest
+kernel will be able to discover the device.
diff --git a/docs/misc/arm/passthrough.txt b/docs/misc/arm/passthrough.txt
index 0efbd122de..a67ada8eb8 100644
--- a/docs/misc/arm/passthrough.txt
+++ b/docs/misc/arm/passthrough.txt
@@ -80,6 +80,107 @@ SPI numbers start from 32, in this example 80 + 32 = 112.
 See man [xl.cfg] for the iomem format. The reg property is just a pair
 of address, then size numbers, each of them can occupy 1 or 2 cells.
 
+
+Dom0-less Device Passthrough
+============================
+
+The partial device tree for dom0-less guests should have the following
+properties for each node corresponding to a physical device to assign to
+the guest:
+
+- xen,reg
+
+  The xen,reg property is an array of:
+
+    <phys_addr size guest_addr>
+
+  They specify the physical address and size of the device memory
+  ranges together with the corresponding guest address to map them to.
+  The size of `phys_addr' and `guest_addr' is determined by
+  #address-cells, the size of `size' is determined by #size-cells, of
+  the partial device tree.
+  The memory will be mapped as device memory in the guest (Device-nGnRE).
+
+- xen,path
+
+  A string property representing the path in the host device tree to the
+  corresponding device node.
+
+In addition, a special /gic node is expected as a placeholder for the
+full GIC node that will be added by Xen for the guest. /gic can be
+referenced by other properties in the device tree fragment. For
+instance, it can be referenced by interrupt-parent under a device node.
+Xen will take care of replacing the "gic" placeholder node for a
+complete GIC node while retaining all the references correctly. The new
+GIC node created by Xen is a regular interrupt-controller@<unit> node.
+
+    gic: gic {
+        #interrupt-cells = <0x3>;
+        interrupt-controller;
+    };
+
+Note that the #interrupt-cells and interrupt-controller properties are
+not actually required, however, DTC expects them to be present if gic is
+referenced by interrupt-parent or similar.
+
+
+Example
+=======
+
+The following is a real-world example of a device tree fragment to
+assign a network card to a dom0-less guest on Xilinx Ultrascale+ MPSoC:
+
+/dts-v1/;
+
+/ {
+    #address-cells = <2>;
+    #size-cells = <1>;
+
+    gic: gic {
+        #interrupt-cells = <3>;
+        interrupt-controller;
+    };
+
+    passthrough {
+        compatible = "simple-bus";
+        ranges;
+        #address-cells = <2>;
+        #size-cells = <1>;
+
+        misc_clk: misc_clk {
+            #clock-cells = <0>;
+            clock-frequency = <0x7735940>;
+            compatible = "fixed-clock";
+        };
+
+        ethernet@ff0e0000 {
+            compatible = "cdns,zynqmp-gem";
+            status = "okay";
+            reg = <0x0 0xff0e0000 0x1000>;
+            clock-names = "pclk", "hclk", "tx_clk", "rx_clk";
+            #address-cells = <1>;
+            #size-cells = <0>;
+            clocks = <&misc_clk &misc_clk &misc_clk &misc_clk>;
+            phy-mode = "rgmii-id";
+            xlnx,ptp-enet-clock = <0>;
+            local-mac-address = [00 0a 35 00 22 01];
+            interrupt-parent = <&gic>;
+            interrupts = <0 63 0x4 0 63 0x4>;
+            xen,path = "/amba/ethernet@ff0e0000";
+            xen,reg = <0x0 0xff0e0000 0x1000 0x0 0xff0e0000>;
+
+            phy@c {
+                reg = <0xc>;
+                ti,rx-internal-delay = <0x8>;
+                ti,tx-internal-delay = <0xa>;
+                ti,fifo-depth = <0x1>;
+                ti,rxctrl-strap-worka;
+            };
+        };
+    };
+};
+
+
 [arm,gic.txt]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt
 [arm,gic-v3.txt]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
 [xl.cfg]: https://xenbits.xen.org/docs/unstable/man/xl.cfg.5.html
-- 
2.17.1


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

Re: [Xen-devel] [PATCH v5 2/8] xen/arm: export device_tree_get_reg and device_tree_get_u32

[Julien Grall]

Hi,

On 25/09/2019 19:49, Stefano Stabellini wrote:
> They'll be used in later patches.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

> 
> ---
> Changes in v5:
> - move declarations to xen/include/asm-arm/setup.h
> 
> Changes in v4:
> - new patch
> ---
>   xen/arch/arm/bootfdt.c      | 8 ++++----
>   xen/include/asm-arm/setup.h | 6 ++++++
>   2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 623173bc7f..a7810abb15 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -55,15 +55,15 @@ static bool __init device_tree_node_compatible(const void *fdt, int node,
>       return false;
>   }
>   
> -static void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
> -                                       u32 size_cells, u64 *start, u64 *size)
> +void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
> +                                u32 size_cells, u64 *start, u64 *size)
>   {
>       *start = dt_next_cell(address_cells, cell);
>       *size = dt_next_cell(size_cells, cell);
>   }
>   
> -static u32 __init device_tree_get_u32(const void *fdt, int node,
> -                                      const char *prop_name, u32 dflt)
> +u32 __init device_tree_get_u32(const void *fdt, int node,
> +                               const char *prop_name, u32 dflt)
>   {
>       const struct fdt_property *prop;
>   
> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> index efcba545c2..fa0a8721b2 100644
> --- a/xen/include/asm-arm/setup.h
> +++ b/xen/include/asm-arm/setup.h
> @@ -115,6 +115,12 @@ const char *boot_module_kind_as_string(bootmodule_kind kind);
>   extern uint32_t hyp_traps_vector[];
>   void init_traps(void);
>   
> +void device_tree_get_reg(const __be32 **cell, u32 address_cells,
> +                         u32 size_cells, u64 *start, u64 *size);
> +
> +u32 device_tree_get_u32(const void *fdt, int node,
> +                        const char *prop_name, u32 dflt);
> +
>   #endif
>   /*
>    * Local variables:
> 

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

Re: [Xen-devel] [PATCH v5 3/8] xen/arm: introduce kinfo->guest_phandle_gic

[Julien Grall]

Hi,

On 25/09/2019 19:49, Stefano Stabellini wrote:
> Instead of always hard-coding the GIC phandle (GUEST_PHANDLE_GIC), store
> it in a variable under kinfo. This way it can be dynamically chosen per
> domain. Remove the fdt pointer argument to the make_*_domU_node
> functions and oass a struct kernel_info * instead. The fdt pointer can
> be accessed from kinfo->fdt. Remove the struct domain *d parameter to
> the make_*_domU_node functions because it becomes unused.
> 
> Initialize guest_phandle_gic to GUEST_PHANDLE_GIC at the beginning of
> prepare_dtb_domU. Later patches will change the value of
> guest_phandle_gic depending on user provided information.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> 
> ---
> Changes in v5:
> - improve commit message
> Changes in v4:
> - new patch
> ---
>   xen/arch/arm/domain_build.c  | 36 +++++++++++++++++++++---------------
>   xen/include/asm-arm/kernel.h |  3 +++
>   2 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 21985628f0..32f85cd959 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -627,7 +627,8 @@ static int __init fdt_property_interrupts(const struct kernel_info *kinfo,
>   {
>       int res;
>       uint32_t phandle = is_hardware_domain(kinfo->d) ?
> -                       dt_interrupt_controller->phandle : GUEST_PHANDLE_GIC;

Sorry, I only realized it now. But is there any reason to not set 
guest_phandle_gic for the hwdom also?

[...]

> diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h
> index 33f3e72b11..760434369b 100644
> --- a/xen/include/asm-arm/kernel.h
> +++ b/xen/include/asm-arm/kernel.h
> @@ -36,6 +36,9 @@ struct kernel_info {
>       /* Enable pl011 emulation */
>       bool vpl011;
>   
> +    /* GIC phandle */
> +    uint32_t guest_phandle_gic;

This would also allow to drop the guest_ prefix.

> +
>       /* loader to use for this kernel */
>       void (*load)(struct kernel_info *info);
>       /* loader specific state */
> 

Cheers,

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

Re: [Xen-devel] [PATCH v5 4/8] xen/arm: copy dtb fragment to guest dtb

[Julien Grall]

On 25/09/2019 19:49, Stefano Stabellini wrote:
> Read the dtb fragment corresponding to a passthrough device from memory
> at the location referred to by the "multiboot,device-tree" compatible
> node.
> 
> Add a new field named dtb_bootmodule to struct kernel_info to keep track
> of the dtb fragment location.
> 
> Copy the fragment to the guest dtb (only /aliases and /passthrough).
> 
> Set kinfo->guest_phandle_gic based on the phandle of the special "/gic"
> node in the device tree fragment. "/gic" is a dummy node in the dtb
> fragment that represents the gic interrupt controller. Other properties
> in the dtb fragment might refer to it (for instance interrupt-parent of
> a device node). We reuse the phandle of "/gic" from the dtb fragment as
> the phandle of the full GIC node that will be created for the guest
> device tree. That way, when we copy properties from the device tree
> fragment to the domU device tree the links remain unbroken.
> 
> Some of the code below is taken from tools/libxl/libxl_arm.c. Note that
> it is OK to take LGPL 2.1 code and including it into a GPLv2 code base.
> The result is GPLv2 code.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> 
> ----
> Changes in v5:
> - code style
> - in-code comment
> - remove depth parameter from scan_pfdt_node
> - for instead of loop in domain_handle_dtb_bootmodule
> - move "gic" check to domain_handle_dtb_bootmodule
> - add check_partial_fdt
> - use DT_ROOT_NODE_ADDR/SIZE_CELLS_DEFAULT
> - add scan_passthrough_prop parameter, set it to false for "/aliases"
> 
> Changes in v4:
> - use recursion in the implementation
> - rename handle_properties to handle_prop_pfdt
> - rename scan_pt_node to scan_pfdt_node
> - pass kinfo to handle_properties
> - use uint32_t instead of u32
> - rename r to res
> - add "passthrough" and "aliases" check
> - add a name == NULL check
> - code style
> - move DTB fragment scanning earlier, before DomU GIC node creation
> - set guest_phandle_gic based on "/gic"
> - in-code comment
> 
> Changes in v3:
> - switch to using device_tree_for_each_node for the copy
> 
> Changes in v2:
> - add a note about the code coming from libxl in the commit message
> - copy /aliases
> - code style
> ---
>   xen/arch/arm/domain_build.c  | 156 +++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/kernel.h |   2 +-
>   2 files changed, 157 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 32f85cd959..9d985d3bbe 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -14,6 +14,7 @@
>   #include <xen/guest_access.h>
>   #include <xen/iocap.h>
>   #include <xen/acpi.h>
> +#include <xen/vmap.h>
>   #include <xen/warning.h>
>   #include <acpi/actables.h>
>   #include <asm/device.h>
> @@ -1700,6 +1701,154 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo)
>   }
>   #endif
>   
> +static int __init handle_prop_pfdt(struct kernel_info *kinfo,
> +                                   const void *pfdt, int nodeoff,
> +                                   uint32_t address_cells, uint32_t size_cells,
> +                                   bool scan_passthrough_prop)

Well, the introduce of scan_passthrough_prop makes little sense as you 
are not using until a follow-up patch. I am ok if you introduce it here, 
but this should at least be mentioned in the commit message.

> +{
> +    void *fdt = kinfo->fdt;
> +    int propoff, nameoff, res;
> +    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);
> +        res = fdt_property(fdt, fdt_string(pfdt, nameoff),
> +                           prop->data, fdt32_to_cpu(prop->len));
> +        if ( res )
> +            return res;
> +    }
> +
> +    /* FDT_ERR_NOTFOUND => There is no more properties for this node */
> +    return ( propoff != -FDT_ERR_NOTFOUND ) ? propoff : 0;
> +}
> +
> +static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt,
> +                                 int nodeoff,
> +                                 uint32_t address_cells, uint32_t size_cells,
> +                                 bool scan_passthrough_prop)
> +{
> +    int rc = 0;
> +    void *fdt = kinfo->fdt;
> +    int node_next;
> +
> +    rc = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL));
> +    if ( rc )
> +        return rc;
> +
> +    rc = handle_prop_pfdt(kinfo, pfdt, nodeoff, address_cells, size_cells,
> +                          scan_passthrough_prop);
> +    if ( rc )
> +        return rc;
> +
> +    address_cells = device_tree_get_u32(pfdt, nodeoff, "#address-cells",
> +                                        DT_ROOT_NODE_ADDR_CELLS_DEFAULT);
> +    size_cells = device_tree_get_u32(pfdt, nodeoff, "#size-cells",
> +                                     DT_ROOT_NODE_SIZE_CELLS_DEFAULT);
> +
> +    node_next = fdt_first_subnode(pfdt, nodeoff);
> +    while ( node_next > 0 )
> +    {
> +        scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells,
> +                       scan_passthrough_prop);
> +        node_next = fdt_next_subnode(pfdt, node_next);
> +    }
> +
> +    return fdt_end_node(fdt);
> +}
> +
> +static int __init check_partial_fdt(void *pfdt, size_t size)
> +{
> +    int res;
> +
> +    if (fdt_magic(pfdt) != FDT_MAGIC) {

Coding style:

if ( ... )
{

> +        dprintk(XENLOG_ERR, "Partial FDT is not a valid Flat Device Tree");
> +        return -EINVAL;
> +    }
> +
> +    res = fdt_check_header(pfdt);
> +    if (res) {

Same here.

> +        dprintk(XENLOG_ERR, "Failed to check the partial FDT (%d)", res);
> +        return -EINVAL;
> +    }
> +
> +    if (fdt_totalsize(pfdt) > size) {

Same here.

> +        dprintk(XENLOG_ERR, "Partial FDT totalsize is too big");
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +static int __init domain_handle_dtb_bootmodule(struct domain *d,
> +                                               struct kernel_info *kinfo)
> +{
> +    void *pfdt;
> +    int res, node_next;
> +
> +    pfdt = ioremap_cache(kinfo->dtb_bootmodule->start,
> +                         kinfo->dtb_bootmodule->size);
> +    if ( pfdt == NULL )
> +        return -EFAULT;
> +
> +    res = check_partial_fdt(pfdt, kinfo->dtb_bootmodule->size);
> +    if ( res < 0 )
> +        return res;
> +
> +    for ( node_next = fdt_first_subnode(pfdt, 0);
> +          node_next > 0;
> +          node_next = fdt_next_subnode(pfdt, node_next) )
> +    {
> +        const char *name = fdt_get_name(pfdt, node_next, NULL);
> +
> +        if ( name == NULL )
> +            continue;
> +
> +        /*
> +         * Only scan /gic /aliases /passthrough, ignore the rest.
> +         * They don't have to be parsed in order.
> +         *
> +         * Take the GIC phandle value from the special /gic node in the
> +         * DTB fragment.
> +         */
> +        if ( dt_node_cmp(name, "gic") == 0 )
> +        {
> +            kinfo->guest_phandle_gic = fdt_get_phandle(pfdt, node_next);
> +            continue;
> +        }
> +
> +        if ( dt_node_cmp(name, "aliases") == 0 )
> +        {
> +            res = scan_pfdt_node(kinfo, pfdt, node_next,
> +                    DT_ROOT_NODE_ADDR_CELLS_DEFAULT,
> +                    DT_ROOT_NODE_SIZE_CELLS_DEFAULT,
> +                    false);

NIT: This is not aligned correctly.

> +            if ( res )
> +                return res;
> +            continue;
> +        }
> +        if ( dt_node_cmp(name, "passthrough") == 0 )
> +        {
> +            res = scan_pfdt_node(kinfo, pfdt, node_next,
> +                    DT_ROOT_NODE_ADDR_CELLS_DEFAULT,
> +                    DT_ROOT_NODE_SIZE_CELLS_DEFAULT,
> +                    true);
Same here.

> +            if ( res )
> +                return res;
> +            continue;
> +        }
> +    }
> +
> +    iounmap(pfdt);
> +
> +    return res;
> +}
> +
>   /*
>    * The max size for DT is 2MB. However, the generated DT is small, 4KB
>    * are enough for now, but we might have to increase it in the future.
> @@ -1755,6 +1904,13 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>       if ( ret )
>           goto err;
>   

I would add a comment here mentioning that the code below always need to 
be called before the rest of the DT is generated. This is because you 
depend on the value of the field guest_phandle_gic.

> +    if ( kinfo->dtb_bootmodule )
> +    {
> +        ret = domain_handle_dtb_bootmodule(d, kinfo);
> +        if ( ret )
> +            return ret;
> +    }
> +
>       ret = make_gic_domU_node(kinfo);
>       if ( ret )
>           goto err;
> diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h
> index 760434369b..7f5e659561 100644
> --- a/xen/include/asm-arm/kernel.h
> +++ b/xen/include/asm-arm/kernel.h
> @@ -28,7 +28,7 @@ struct kernel_info {
>       paddr_t gnttab_size;
>   
>       /* boot blob load addresses */
> -    const struct bootmodule *kernel_bootmodule, *initrd_bootmodule;
> +    const struct bootmodule *kernel_bootmodule, *initrd_bootmodule, *dtb_bootmodule;
>       const char* cmdline;
>       paddr_t dtb_paddr;
>       paddr_t initrd_paddr;
> 

Cheers,

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

Re: [Xen-devel] [PATCH v5 5/8] xen/arm: assign devices to boot domains

[Julien Grall]

Hi Stefano,

On 25/09/2019 19:49, Stefano Stabellini wrote:
> Scan the user provided dtb fragment at boot. For each device node, map
> memory to guests, and route interrupts and setup the iommu.
> 
> The memory region to remap is specified by the "xen,reg" property.
> 
> The iommu is setup by passing the node of the device to assign on the
> host device tree. The path is specified in the device tree fragment as
> the "xen,path" string property.
> 
> The interrupts are remapped based on the information from the
> corresponding node on the host device tree. Call
> handle_device_interrupts to remap interrupts. Interrupts related device
> tree properties are copied from the device tree fragment, same as all
> the other properties.
> 
> Also add the new flag XEN_DOMCTL_CDF_iommu to that dom0less domU can use
> the IOMMU.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
> Changes in v5:
> - use local variable for name
> - use map_regions_p2mt
> - add warning for not page aligned addresses/sizes
> - introduce handle_passthrough_prop
> 
> Changes in v4:
> - use unsigned
> - improve commit message
> - code style
> - use dt_prop_cmp
> - use device_tree_get_reg
> - don't copy over xen,reg and xen,path
> - don't create special interrupt properties for domU: copy them from the
>    fragment
> - in-code comment
> 
> Changes in v3:
> - improve commit message
> - remove superfluous cast
> - merge code with the copy code
> - add interrup-parent
> - demove depth > 2 check
> - reuse code from handle_device_interrupts
> - copy interrupts from host dt
> 
> Changes in v2:
> - rename "path" to "xen,path"
> - grammar fix
> - use gaddr_to_gfn and maddr_to_mfn
> - remove depth <= 2 limitation in scanning the dtb fragment
> - introduce and parse xen,reg
> - code style
> - support more than one interrupt per device
> - specify only the GIC is supported
> ---
>   xen/arch/arm/domain_build.c | 101 ++++++++++++++++++++++++++++++++++--
>   1 file changed, 97 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 9d985d3bbe..414893bc24 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1701,6 +1701,85 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo)
>   }
>   #endif
>   
> +/*
> + * Scan device tree properties for passthrough specific information.
> + * Returns -ENOENT when no passthrough properties are found
> + *         < 0 on error
> + *         0 on success
> + */
> +static int __init handle_passthrough_prop(struct kernel_info *kinfo,
> +                                          const struct fdt_property *prop,
> +                                          const char *name,
> +                                          uint32_t address_cells, uint32_t size_cells)
> +{
> +    const __be32 *cell;
> +    unsigned int i, len;
> +    struct dt_device_node *node;
> +    int res;
> +
> +    /* xen,reg specifies where to map the MMIO region */
> +    if ( dt_prop_cmp("xen,reg", name) == 0 )
> +    {
> +        paddr_t mstart, size, gstart;
> +        cell = (const __be32 *)prop->data;
> +        len = fdt32_to_cpu(prop->len) /
> +            ((address_cells * 2 + size_cells) * sizeof(uint32_t));
> +
> +        for ( i = 0; i < len; i++ )
> +        {
> +            device_tree_get_reg(&cell, address_cells, size_cells,
> +                    &mstart, &size);
> +            gstart = dt_next_cell(address_cells, &cell);
> +
> +            if ( gstart & ~PAGE_MASK || mstart & ~PAGE_MASK || size & ~PAGE_MASK )
> +                dprintk(XENLOG_WARNING,
> +                        "DomU passthrough config has not page aligned addresses/sizes\n");

I don't think this is wise to continue, the more this is a printk that 
can only happen in debug build. So someone using a release build may not 
notice the error.

So I think this wants to be a printk(XENLOG_ERR,...) and also return an 
error.

> +
> +            res = map_regions_p2mt(kinfo->d,
> +                    gaddr_to_gfn(gstart),
> +                    PFN_DOWN(size),
> +                    maddr_to_mfn(mstart),
> +                    p2m_mmio_direct_dev);

Coding style.

> +            if ( res < 0 )
> +            {
> +                dprintk(XENLOG_ERR,
> +                        "Failed to map %"PRIpaddr" to the guest at%"PRIpaddr"\n",
> +                        mstart, gstart);

Similarly, this wants to be a printk.

> +                return -EFAULT;
> +            }
> +        }
> +
> +        return 0;
> +    }
> +    /*
> +     * xen,path specifies the corresponding node in the host DT.
> +     * Both interrupt mappings and IOMMU settings are based on it,
> +     * as they are done based on the corresponding host DT node.
> +     */
> +    else if ( dt_prop_cmp("xen,path", name) == 0 )
> +    {
> +        node = dt_find_node_by_path(prop->data);
> +        if ( node == NULL )
> +        {
> +            dprintk(XENLOG_ERR, "Couldn't find node %s in host_dt!\n",
> +                    (char *)prop->data);

Same here.

> +            return -EINVAL;
> +        }
> +
> +        res = iommu_assign_dt_device(kinfo->d, node);
> +        if ( res < 0 )
> +            return res;
> +
> +        res = handle_device_interrupts(kinfo->d, node, true);
> +        if ( res < 0 )
> +            return res;
> +
> +        return 0;
> +    }
> +
> +    return -ENOENT;
> +}
> +
>   static int __init handle_prop_pfdt(struct kernel_info *kinfo,
>                                      const void *pfdt, int nodeoff,
>                                      uint32_t address_cells, uint32_t size_cells,
> @@ -1709,6 +1788,7 @@ static int __init handle_prop_pfdt(struct kernel_info *kinfo,
>       void *fdt = kinfo->fdt;
>       int propoff, nameoff, res;
>       const struct fdt_property *prop;
> +    const char *name;
>   
>       for ( propoff = fdt_first_property_offset(pfdt, nodeoff);
>             propoff >= 0;
> @@ -1717,11 +1797,23 @@ static int __init handle_prop_pfdt(struct kernel_info *kinfo,
>           if ( !(prop = fdt_get_property_by_offset(pfdt, propoff, NULL)) )
>               return -FDT_ERR_INTERNAL;
>   
> +        res = 0;
>           nameoff = fdt32_to_cpu(prop->nameoff);
> -        res = fdt_property(fdt, fdt_string(pfdt, nameoff),
> -                           prop->data, fdt32_to_cpu(prop->len));
> -        if ( res )
> +        name = fdt_string(pfdt, nameoff);
> +
> +        if ( scan_passthrough_prop )
> +            res = handle_passthrough_prop(kinfo, prop, name,
> +                                          address_cells, size_cells);
> +        if ( res < 0 && res != -ENOENT )
>               return res;
> +
> +        /* copy all other properties */
> +        if ( !scan_passthrough_prop || res == -ENOENT )
> +        {
> +            res = fdt_property(fdt, name, prop->data, fdt32_to_cpu(prop->len));
> +            if ( res )
> +                return res;
> +        }
>       }
>   
>       /* FDT_ERR_NOTFOUND => There is no more properties for this node */
> @@ -2254,7 +2346,8 @@ void __init create_domUs(void)
>           struct xen_domctl_createdomain d_cfg = {
>               .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE,
>               .arch.nr_spis = 0,
> -            .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
> +            .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
> +                     XEN_DOMCTL_CDF_iommu,

This will break dom0less on platform without an IOMMU because setting 
CDF_iommu for them will be invalid.

I also don't think this is wise to always enable the IOMMU. This should 
only be done if there is a partial device-tree present (most likely it 
means passthrough will be used).

>               .max_evtchn_port = -1,
>               .max_grant_frames = 64,
>               .max_maptrack_frames = 1024,
> 

Cheers,

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

Re: [Xen-devel] [PATCH v5 7/8] xen/arm: introduce nr_spis

[Julien Grall]

Hi,

On 25/09/2019 19:49, Stefano Stabellini wrote:
> We don't have a clear way to know how many virtual SPIs we need for the
> dom0-less domains. Introduce a new option under xen,domain to specify
> the number of SPIs to allocate for a domain.
> 
> The property is optional. When absent, we'll use the physical number of
> GIC lines for dom0-less domains, just like for dom0.

Based on the code below, this is not correct when using vpl011.

> 
> Remove the old setting of nr_spis based on the presence of the vpl011.
> 
> The implication of this change is that without nr_spis dom0less domains
> get the same amount of SPI allocated as dom0, regardless of how many
> physical devices they have assigned, and regardless of whether they have
> a virtual pl011 (which also needs an emulated SPI). For instance, we
> could end up exposing 256 SPIs for each dom0less domain without a
> nr_spis property. If we have 4 dom0less domains without nr_spis, it
> would result in 80K of additional memory being used.

I don't understand what you are trying to imply with your example. Ok, 
this tell you how much memory you are going to waste... but this does 
still not explain why the nr_spis are increased in the default case.

> 
> When nr_spis is present, the domain gets exactly nr_spis allocated SPIs.
> If the number is too low, it might not be enough for the devices
> assigned it to it. If the number is less than GUEST_VPL011_SPI, the
> virtual pl011 won't work.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
> Changes in v5:
> - improve commit message
> - allocate enough SPIs for vpl011
> 
> Changes in v4:
> - improve commit message
> 
> Changes in v3:
> - improve commit message
> - introduce nr_spis
> ---
>   xen/arch/arm/domain_build.c | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 414893bc24..bf4d960eb5 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2345,7 +2345,6 @@ void __init create_domUs(void)
>           struct domain *d;
>           struct xen_domctl_createdomain d_cfg = {
>               .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE,
> -            .arch.nr_spis = 0,
>               .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
>                        XEN_DOMCTL_CDF_iommu,
>               .max_evtchn_port = -1,
> @@ -2356,13 +2355,23 @@ void __init create_domUs(void)
>           if ( !dt_device_is_compatible(node, "xen,domain") )
>               continue;
>   
> -        if ( dt_property_read_bool(node, "vpl011") )
> -            d_cfg.arch.nr_spis = GUEST_VPL011_SPI - 32 + 1;
> -
>           if ( !dt_property_read_u32(node, "cpus", &d_cfg.max_vcpus) )
>               panic("Missing property 'cpus' for domain %s\n",
>                     dt_node_name(node));
>   
> +        if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
> +        {
> +            d_cfg.arch.nr_spis = gic_number_lines() - 32;
> +
> +            /*
> +             * vpl011 uses one emulated SPI. If vpl011 is requested, make
> +             * sure that we allocate enough SPIs for it.
> +             */
> +            if ( dt_property_read_bool(node, "vpl011") )
> +                d_cfg.arch.nr_spis = MAX(d_cfg.arch.nr_spis,
> +                                         GUEST_VPL011_SPI - 32 + 1);
> +        }
> +
>           d = domain_create(++max_init_domid, &d_cfg, false);
>           if ( IS_ERR(d) )
>               panic("Error creating domain %s\n", dt_node_name(node));
> 

Cheers,

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

Re: [Xen-devel] [PATCH v5 8/8] xen/arm: add dom0-less device assignment info to docs

[Julien Grall]

Hi,

On 25/09/2019 19:49, Stefano Stabellini wrote:
> +- nr_spis
> +
> +    Optional. A 32-bit integer specifying the number of SPIs (Shared
> +    Peripheral Interrupts) to allocate for the domain. If nr_spis is
> +    missing, the max number of SPIs supported by the physical GIC is
> +    used.

This is not correct anymore when vpl011 is set.

With that fixed:

Acked-by: Julien Grall <julien.grall@arm.com.

Cheers,

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

Re: [Xen-devel] [PATCH v5 7/8] xen/arm: introduce nr_spis

[Stefano Stabellini]

On Wed, 25 Sep 2019, Julien Grall wrote:
> Hi,
> 
> On 25/09/2019 19:49, Stefano Stabellini wrote:
> > We don't have a clear way to know how many virtual SPIs we need for the
> > dom0-less domains. Introduce a new option under xen,domain to specify
> > the number of SPIs to allocate for a domain.
> > 
> > The property is optional. When absent, we'll use the physical number of
> > GIC lines for dom0-less domains, just like for dom0.
> 
> Based on the code below, this is not correct when using vpl011.

I'll write:

The property is optional. When absent, we'll use the physical number of
GIC lines for dom0-less domains, or GUEST_VPL011_SPI+1 if vpl011 is
requested, whichever is greater.


> > 
> > Remove the old setting of nr_spis based on the presence of the vpl011.
> > 
> > The implication of this change is that without nr_spis dom0less domains
> > get the same amount of SPI allocated as dom0, regardless of how many
> > physical devices they have assigned, and regardless of whether they have
> > a virtual pl011 (which also needs an emulated SPI). For instance, we
> > could end up exposing 256 SPIs for each dom0less domain without a
> > nr_spis property. If we have 4 dom0less domains without nr_spis, it
> > would result in 80K of additional memory being used.
> 
> I don't understand what you are trying to imply with your example. Ok, 
> this tell you how much memory you are going to waste... but this does 
> still not explain why the nr_spis are increased in the default case.

I misunderstood what you wanted me to add to the commit message. I'll
remove the example and instead write:

The implication of this change is that without nr_spis dom0less domains
get the same amount of SPI allocated as dom0, regardless of how many
physical devices they have assigned, and regardless of whether they have
a virtual pl011 (which also needs an emulated SPI). This is done because
the SPIs allocation needs to be done before parsing any passthrough
information, so we have to account for any potential physical SPI
assigned to the domain.


Is this better?



> > When nr_spis is present, the domain gets exactly nr_spis allocated SPIs.
> > If the number is too low, it might not be enough for the devices
> > assigned it to it. If the number is less than GUEST_VPL011_SPI, the
> > virtual pl011 won't work.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> > ---
> > Changes in v5:
> > - improve commit message
> > - allocate enough SPIs for vpl011
> > 
> > Changes in v4:
> > - improve commit message
> > 
> > Changes in v3:
> > - improve commit message
> > - introduce nr_spis
> > ---
> >   xen/arch/arm/domain_build.c | 17 +++++++++++++----
> >   1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 414893bc24..bf4d960eb5 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2345,7 +2345,6 @@ void __init create_domUs(void)
> >           struct domain *d;
> >           struct xen_domctl_createdomain d_cfg = {
> >               .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE,
> > -            .arch.nr_spis = 0,
> >               .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
> >                        XEN_DOMCTL_CDF_iommu,
> >               .max_evtchn_port = -1,
> > @@ -2356,13 +2355,23 @@ void __init create_domUs(void)
> >           if ( !dt_device_is_compatible(node, "xen,domain") )
> >               continue;
> >   
> > -        if ( dt_property_read_bool(node, "vpl011") )
> > -            d_cfg.arch.nr_spis = GUEST_VPL011_SPI - 32 + 1;
> > -
> >           if ( !dt_property_read_u32(node, "cpus", &d_cfg.max_vcpus) )
> >               panic("Missing property 'cpus' for domain %s\n",
> >                     dt_node_name(node));
> >   
> > +        if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
> > +        {
> > +            d_cfg.arch.nr_spis = gic_number_lines() - 32;
> > +
> > +            /*
> > +             * vpl011 uses one emulated SPI. If vpl011 is requested, make
> > +             * sure that we allocate enough SPIs for it.
> > +             */
> > +            if ( dt_property_read_bool(node, "vpl011") )
> > +                d_cfg.arch.nr_spis = MAX(d_cfg.arch.nr_spis,
> > +                                         GUEST_VPL011_SPI - 32 + 1);
> > +        }
> > +
> >           d = domain_create(++max_init_domid, &d_cfg, false);
> >           if ( IS_ERR(d) )
> >               panic("Error creating domain %s\n", dt_node_name(node));
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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

Re: [Xen-devel] [PATCH v5 3/8] xen/arm: introduce kinfo->guest_phandle_gic

[Stefano Stabellini]

On Wed, 25 Sep 2019, Julien Grall wrote:
> Hi,
> 
> On 25/09/2019 19:49, Stefano Stabellini wrote:
> > Instead of always hard-coding the GIC phandle (GUEST_PHANDLE_GIC), store
> > it in a variable under kinfo. This way it can be dynamically chosen per
> > domain. Remove the fdt pointer argument to the make_*_domU_node
> > functions and oass a struct kernel_info * instead. The fdt pointer can
> > be accessed from kinfo->fdt. Remove the struct domain *d parameter to
> > the make_*_domU_node functions because it becomes unused.
> > 
> > Initialize guest_phandle_gic to GUEST_PHANDLE_GIC at the beginning of
> > prepare_dtb_domU. Later patches will change the value of
> > guest_phandle_gic depending on user provided information.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > 
> > ---
> > Changes in v5:
> > - improve commit message
> > Changes in v4:
> > - new patch
> > ---
> >   xen/arch/arm/domain_build.c  | 36 +++++++++++++++++++++---------------
> >   xen/include/asm-arm/kernel.h |  3 +++
> >   2 files changed, 24 insertions(+), 15 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 21985628f0..32f85cd959 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -627,7 +627,8 @@ static int __init fdt_property_interrupts(const struct kernel_info *kinfo,
> >   {
> >       int res;
> >       uint32_t phandle = is_hardware_domain(kinfo->d) ?
> > -                       dt_interrupt_controller->phandle : GUEST_PHANDLE_GIC;
> 
> Sorry, I only realized it now. But is there any reason to not set 
> guest_phandle_gic for the hwdom also?

Yes, I can do that for dom0 too


> > diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h
> > index 33f3e72b11..760434369b 100644
> > --- a/xen/include/asm-arm/kernel.h
> > +++ b/xen/include/asm-arm/kernel.h
> > @@ -36,6 +36,9 @@ struct kernel_info {
> >       /* Enable pl011 emulation */
> >       bool vpl011;
> >   
> > +    /* GIC phandle */
> > +    uint32_t guest_phandle_gic;
> 
> This would also allow to drop the guest_ prefix.
> 
> > +
> >       /* loader to use for this kernel */
> >       void (*load)(struct kernel_info *info);
> >       /* loader specific state */
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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

Re: [Xen-devel] [PATCH v5 4/8] xen/arm: copy dtb fragment to guest dtb

[Stefano Stabellini]

On Wed, 25 Sep 2019, Julien Grall wrote:
> On 25/09/2019 19:49, Stefano Stabellini wrote:
> > Read the dtb fragment corresponding to a passthrough device from memory
> > at the location referred to by the "multiboot,device-tree" compatible
> > node.
> > 
> > Add a new field named dtb_bootmodule to struct kernel_info to keep track
> > of the dtb fragment location.
> > 
> > Copy the fragment to the guest dtb (only /aliases and /passthrough).
> > 
> > Set kinfo->guest_phandle_gic based on the phandle of the special "/gic"
> > node in the device tree fragment. "/gic" is a dummy node in the dtb
> > fragment that represents the gic interrupt controller. Other properties
> > in the dtb fragment might refer to it (for instance interrupt-parent of
> > a device node). We reuse the phandle of "/gic" from the dtb fragment as
> > the phandle of the full GIC node that will be created for the guest
> > device tree. That way, when we copy properties from the device tree
> > fragment to the domU device tree the links remain unbroken.
> > 
> > Some of the code below is taken from tools/libxl/libxl_arm.c. Note that
> > it is OK to take LGPL 2.1 code and including it into a GPLv2 code base.
> > The result is GPLv2 code.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > 
> > ----
> > Changes in v5:
> > - code style
> > - in-code comment
> > - remove depth parameter from scan_pfdt_node
> > - for instead of loop in domain_handle_dtb_bootmodule
> > - move "gic" check to domain_handle_dtb_bootmodule
> > - add check_partial_fdt
> > - use DT_ROOT_NODE_ADDR/SIZE_CELLS_DEFAULT
> > - add scan_passthrough_prop parameter, set it to false for "/aliases"
> > 
> > Changes in v4:
> > - use recursion in the implementation
> > - rename handle_properties to handle_prop_pfdt
> > - rename scan_pt_node to scan_pfdt_node
> > - pass kinfo to handle_properties
> > - use uint32_t instead of u32
> > - rename r to res
> > - add "passthrough" and "aliases" check
> > - add a name == NULL check
> > - code style
> > - move DTB fragment scanning earlier, before DomU GIC node creation
> > - set guest_phandle_gic based on "/gic"
> > - in-code comment
> > 
> > Changes in v3:
> > - switch to using device_tree_for_each_node for the copy
> > 
> > Changes in v2:
> > - add a note about the code coming from libxl in the commit message
> > - copy /aliases
> > - code style
> > ---
> >   xen/arch/arm/domain_build.c  | 156 +++++++++++++++++++++++++++++++++++
> >   xen/include/asm-arm/kernel.h |   2 +-
> >   2 files changed, 157 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 32f85cd959..9d985d3bbe 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -14,6 +14,7 @@
> >   #include <xen/guest_access.h>
> >   #include <xen/iocap.h>
> >   #include <xen/acpi.h>
> > +#include <xen/vmap.h>
> >   #include <xen/warning.h>
> >   #include <acpi/actables.h>
> >   #include <asm/device.h>
> > @@ -1700,6 +1701,154 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo)
> >   }
> >   #endif
> >   
> > +static int __init handle_prop_pfdt(struct kernel_info *kinfo,
> > +                                   const void *pfdt, int nodeoff,
> > +                                   uint32_t address_cells, uint32_t size_cells,
> > +                                   bool scan_passthrough_prop)
> 
> Well, the introduce of scan_passthrough_prop makes little sense as you 
> are not using until a follow-up patch. I am ok if you introduce it here, 
> but this should at least be mentioned in the commit message.

I'll mention it


> > +{
> > +    void *fdt = kinfo->fdt;
> > +    int propoff, nameoff, res;
> > +    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);
> > +        res = fdt_property(fdt, fdt_string(pfdt, nameoff),
> > +                           prop->data, fdt32_to_cpu(prop->len));
> > +        if ( res )
> > +            return res;
> > +    }
> > +
> > +    /* FDT_ERR_NOTFOUND => There is no more properties for this node */
> > +    return ( propoff != -FDT_ERR_NOTFOUND ) ? propoff : 0;
> > +}
> > +
> > +static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt,
> > +                                 int nodeoff,
> > +                                 uint32_t address_cells, uint32_t size_cells,
> > +                                 bool scan_passthrough_prop)
> > +{
> > +    int rc = 0;
> > +    void *fdt = kinfo->fdt;
> > +    int node_next;
> > +
> > +    rc = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL));
> > +    if ( rc )
> > +        return rc;
> > +
> > +    rc = handle_prop_pfdt(kinfo, pfdt, nodeoff, address_cells, size_cells,
> > +                          scan_passthrough_prop);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    address_cells = device_tree_get_u32(pfdt, nodeoff, "#address-cells",
> > +                                        DT_ROOT_NODE_ADDR_CELLS_DEFAULT);
> > +    size_cells = device_tree_get_u32(pfdt, nodeoff, "#size-cells",
> > +                                     DT_ROOT_NODE_SIZE_CELLS_DEFAULT);
> > +
> > +    node_next = fdt_first_subnode(pfdt, nodeoff);
> > +    while ( node_next > 0 )
> > +    {
> > +        scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells,
> > +                       scan_passthrough_prop);
> > +        node_next = fdt_next_subnode(pfdt, node_next);
> > +    }
> > +
> > +    return fdt_end_node(fdt);
> > +}
> > +
> > +static int __init check_partial_fdt(void *pfdt, size_t size)
> > +{
> > +    int res;
> > +
> > +    if (fdt_magic(pfdt) != FDT_MAGIC) {
> 
> Coding style:
> 
> if ( ... )
> {
> 
> > +        dprintk(XENLOG_ERR, "Partial FDT is not a valid Flat Device Tree");
> > +        return -EINVAL;
> > +    }
> > +
> > +    res = fdt_check_header(pfdt);
> > +    if (res) {
> 
> Same here.
> 
> > +        dprintk(XENLOG_ERR, "Failed to check the partial FDT (%d)", res);
> > +        return -EINVAL;
> > +    }
> > +
> > +    if (fdt_totalsize(pfdt) > size) {
> 
> Same here.

I'll fix the coding style issues

> 
> > +        dprintk(XENLOG_ERR, "Partial FDT totalsize is too big");
> > +        return -EINVAL;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static int __init domain_handle_dtb_bootmodule(struct domain *d,
> > +                                               struct kernel_info *kinfo)
> > +{
> > +    void *pfdt;
> > +    int res, node_next;
> > +
> > +    pfdt = ioremap_cache(kinfo->dtb_bootmodule->start,
> > +                         kinfo->dtb_bootmodule->size);
> > +    if ( pfdt == NULL )
> > +        return -EFAULT;
> > +
> > +    res = check_partial_fdt(pfdt, kinfo->dtb_bootmodule->size);
> > +    if ( res < 0 )
> > +        return res;
> > +
> > +    for ( node_next = fdt_first_subnode(pfdt, 0);
> > +          node_next > 0;
> > +          node_next = fdt_next_subnode(pfdt, node_next) )
> > +    {
> > +        const char *name = fdt_get_name(pfdt, node_next, NULL);
> > +
> > +        if ( name == NULL )
> > +            continue;
> > +
> > +        /*
> > +         * Only scan /gic /aliases /passthrough, ignore the rest.
> > +         * They don't have to be parsed in order.
> > +         *
> > +         * Take the GIC phandle value from the special /gic node in the
> > +         * DTB fragment.
> > +         */
> > +        if ( dt_node_cmp(name, "gic") == 0 )
> > +        {
> > +            kinfo->guest_phandle_gic = fdt_get_phandle(pfdt, node_next);
> > +            continue;
> > +        }
> > +
> > +        if ( dt_node_cmp(name, "aliases") == 0 )
> > +        {
> > +            res = scan_pfdt_node(kinfo, pfdt, node_next,
> > +                    DT_ROOT_NODE_ADDR_CELLS_DEFAULT,
> > +                    DT_ROOT_NODE_SIZE_CELLS_DEFAULT,
> > +                    false);
> 
> NIT: This is not aligned correctly.
> 
> > +            if ( res )
> > +                return res;
> > +            continue;
> > +        }
> > +        if ( dt_node_cmp(name, "passthrough") == 0 )
> > +        {
> > +            res = scan_pfdt_node(kinfo, pfdt, node_next,
> > +                    DT_ROOT_NODE_ADDR_CELLS_DEFAULT,
> > +                    DT_ROOT_NODE_SIZE_CELLS_DEFAULT,
> > +                    true);
> Same here.
> 
> > +            if ( res )
> > +                return res;
> > +            continue;
> > +        }
> > +    }
> > +
> > +    iounmap(pfdt);
> > +
> > +    return res;
> > +}
> > +
> >   /*
> >    * The max size for DT is 2MB. However, the generated DT is small, 4KB
> >    * are enough for now, but we might have to increase it in the future.
> > @@ -1755,6 +1904,13 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
> >       if ( ret )
> >           goto err;
> >   
> 
> I would add a comment here mentioning that the code below always need to 
> be called before the rest of the DT is generated. This is because you 
> depend on the value of the field guest_phandle_gic.

OK


> > +    if ( kinfo->dtb_bootmodule )
> > +    {
> > +        ret = domain_handle_dtb_bootmodule(d, kinfo);
> > +        if ( ret )
> > +            return ret;
> > +    }
> > +
> >       ret = make_gic_domU_node(kinfo);
> >       if ( ret )
> >           goto err;
> > diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h
> > index 760434369b..7f5e659561 100644
> > --- a/xen/include/asm-arm/kernel.h
> > +++ b/xen/include/asm-arm/kernel.h
> > @@ -28,7 +28,7 @@ struct kernel_info {
> >       paddr_t gnttab_size;
> >   
> >       /* boot blob load addresses */
> > -    const struct bootmodule *kernel_bootmodule, *initrd_bootmodule;
> > +    const struct bootmodule *kernel_bootmodule, *initrd_bootmodule, *dtb_bootmodule;
> >       const char* cmdline;
> >       paddr_t dtb_paddr;
> >       paddr_t initrd_paddr;
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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

Re: [Xen-devel] [PATCH v5 5/8] xen/arm: assign devices to boot domains

[Stefano Stabellini]

On Wed, 25 Sep 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 25/09/2019 19:49, Stefano Stabellini wrote:
> > Scan the user provided dtb fragment at boot. For each device node, map
> > memory to guests, and route interrupts and setup the iommu.
> > 
> > The memory region to remap is specified by the "xen,reg" property.
> > 
> > The iommu is setup by passing the node of the device to assign on the
> > host device tree. The path is specified in the device tree fragment as
> > the "xen,path" string property.
> > 
> > The interrupts are remapped based on the information from the
> > corresponding node on the host device tree. Call
> > handle_device_interrupts to remap interrupts. Interrupts related device
> > tree properties are copied from the device tree fragment, same as all
> > the other properties.
> > 
> > Also add the new flag XEN_DOMCTL_CDF_iommu to that dom0less domU can use
> > the IOMMU.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> > Changes in v5:
> > - use local variable for name
> > - use map_regions_p2mt
> > - add warning for not page aligned addresses/sizes
> > - introduce handle_passthrough_prop
> > 
> > Changes in v4:
> > - use unsigned
> > - improve commit message
> > - code style
> > - use dt_prop_cmp
> > - use device_tree_get_reg
> > - don't copy over xen,reg and xen,path
> > - don't create special interrupt properties for domU: copy them from the
> >    fragment
> > - in-code comment
> > 
> > Changes in v3:
> > - improve commit message
> > - remove superfluous cast
> > - merge code with the copy code
> > - add interrup-parent
> > - demove depth > 2 check
> > - reuse code from handle_device_interrupts
> > - copy interrupts from host dt
> > 
> > Changes in v2:
> > - rename "path" to "xen,path"
> > - grammar fix
> > - use gaddr_to_gfn and maddr_to_mfn
> > - remove depth <= 2 limitation in scanning the dtb fragment
> > - introduce and parse xen,reg
> > - code style
> > - support more than one interrupt per device
> > - specify only the GIC is supported
> > ---
> >   xen/arch/arm/domain_build.c | 101 ++++++++++++++++++++++++++++++++++--
> >   1 file changed, 97 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 9d985d3bbe..414893bc24 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1701,6 +1701,85 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo)
> >   }
> >   #endif
> >   
> > +/*
> > + * Scan device tree properties for passthrough specific information.
> > + * Returns -ENOENT when no passthrough properties are found
> > + *         < 0 on error
> > + *         0 on success
> > + */
> > +static int __init handle_passthrough_prop(struct kernel_info *kinfo,
> > +                                          const struct fdt_property *prop,
> > +                                          const char *name,
> > +                                          uint32_t address_cells, uint32_t size_cells)
> > +{
> > +    const __be32 *cell;
> > +    unsigned int i, len;
> > +    struct dt_device_node *node;
> > +    int res;
> > +
> > +    /* xen,reg specifies where to map the MMIO region */
> > +    if ( dt_prop_cmp("xen,reg", name) == 0 )
> > +    {
> > +        paddr_t mstart, size, gstart;
> > +        cell = (const __be32 *)prop->data;
> > +        len = fdt32_to_cpu(prop->len) /
> > +            ((address_cells * 2 + size_cells) * sizeof(uint32_t));
> > +
> > +        for ( i = 0; i < len; i++ )
> > +        {
> > +            device_tree_get_reg(&cell, address_cells, size_cells,
> > +                    &mstart, &size);
> > +            gstart = dt_next_cell(address_cells, &cell);
> > +
> > +            if ( gstart & ~PAGE_MASK || mstart & ~PAGE_MASK || size & ~PAGE_MASK )
> > +                dprintk(XENLOG_WARNING,
> > +                        "DomU passthrough config has not page aligned addresses/sizes\n");
> 
> I don't think this is wise to continue, the more this is a printk that 
> can only happen in debug build. So someone using a release build may not 
> notice the error.
> 
> So I think this wants to be a printk(XENLOG_ERR,...) and also return an 
> error.

I'll fix the code style issues, use printk instead of dprintk and return
error here.


> > +
> > +            res = map_regions_p2mt(kinfo->d,
> > +                    gaddr_to_gfn(gstart),
> > +                    PFN_DOWN(size),
> > +                    maddr_to_mfn(mstart),
> > +                    p2m_mmio_direct_dev);
> 
> Coding style.
> 
> > +            if ( res < 0 )
> > +            {
> > +                dprintk(XENLOG_ERR,
> > +                        "Failed to map %"PRIpaddr" to the guest at%"PRIpaddr"\n",
> > +                        mstart, gstart);
> 
> Similarly, this wants to be a printk.
> 
> > +                return -EFAULT;
> > +            }
> > +        }
> > +
> > +        return 0;
> > +    }
> > +    /*
> > +     * xen,path specifies the corresponding node in the host DT.
> > +     * Both interrupt mappings and IOMMU settings are based on it,
> > +     * as they are done based on the corresponding host DT node.
> > +     */
> > +    else if ( dt_prop_cmp("xen,path", name) == 0 )
> > +    {
> > +        node = dt_find_node_by_path(prop->data);
> > +        if ( node == NULL )
> > +        {
> > +            dprintk(XENLOG_ERR, "Couldn't find node %s in host_dt!\n",
> > +                    (char *)prop->data);
> 
> Same here.
> 
> > +            return -EINVAL;
> > +        }
> > +
> > +        res = iommu_assign_dt_device(kinfo->d, node);
> > +        if ( res < 0 )
> > +            return res;
> > +
> > +        res = handle_device_interrupts(kinfo->d, node, true);
> > +        if ( res < 0 )
> > +            return res;
> > +
> > +        return 0;
> > +    }
> > +
> > +    return -ENOENT;
> > +}
> > +
> >   static int __init handle_prop_pfdt(struct kernel_info *kinfo,
> >                                      const void *pfdt, int nodeoff,
> >                                      uint32_t address_cells, uint32_t size_cells,
> > @@ -1709,6 +1788,7 @@ static int __init handle_prop_pfdt(struct kernel_info *kinfo,
> >       void *fdt = kinfo->fdt;
> >       int propoff, nameoff, res;
> >       const struct fdt_property *prop;
> > +    const char *name;
> >   
> >       for ( propoff = fdt_first_property_offset(pfdt, nodeoff);
> >             propoff >= 0;
> > @@ -1717,11 +1797,23 @@ static int __init handle_prop_pfdt(struct kernel_info *kinfo,
> >           if ( !(prop = fdt_get_property_by_offset(pfdt, propoff, NULL)) )
> >               return -FDT_ERR_INTERNAL;
> >   
> > +        res = 0;
> >           nameoff = fdt32_to_cpu(prop->nameoff);
> > -        res = fdt_property(fdt, fdt_string(pfdt, nameoff),
> > -                           prop->data, fdt32_to_cpu(prop->len));
> > -        if ( res )
> > +        name = fdt_string(pfdt, nameoff);
> > +
> > +        if ( scan_passthrough_prop )
> > +            res = handle_passthrough_prop(kinfo, prop, name,
> > +                                          address_cells, size_cells);
> > +        if ( res < 0 && res != -ENOENT )
> >               return res;
> > +
> > +        /* copy all other properties */
> > +        if ( !scan_passthrough_prop || res == -ENOENT )
> > +        {
> > +            res = fdt_property(fdt, name, prop->data, fdt32_to_cpu(prop->len));
> > +            if ( res )
> > +                return res;
> > +        }
> >       }
> >   
> >       /* FDT_ERR_NOTFOUND => There is no more properties for this node */
> > @@ -2254,7 +2346,8 @@ void __init create_domUs(void)
> >           struct xen_domctl_createdomain d_cfg = {
> >               .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE,
> >               .arch.nr_spis = 0,
> > -            .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
> > +            .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
> > +                     XEN_DOMCTL_CDF_iommu,
> 
> This will break dom0less on platform without an IOMMU because setting 
> CDF_iommu for them will be invalid.
> 
> I also don't think this is wise to always enable the IOMMU. This should 
> only be done if there is a partial device-tree present (most likely it 
> means passthrough will be used).

That's interesting, good idea, I'll do that

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

Re: [Xen-devel] [PATCH v5 7/8] xen/arm: introduce nr_spis

[Julien Grall]

Hi Stefano,

On 9/26/19 2:25 AM, Stefano Stabellini wrote:
> On Wed, 25 Sep 2019, Julien Grall wrote:
>> Hi,
>>
>> On 25/09/2019 19:49, Stefano Stabellini wrote:
>>> We don't have a clear way to know how many virtual SPIs we need for the
>>> dom0-less domains. Introduce a new option under xen,domain to specify
>>> the number of SPIs to allocate for a domain.
>>>
>>> The property is optional. When absent, we'll use the physical number of
>>> GIC lines for dom0-less domains, just like for dom0.
>>
>> Based on the code below, this is not correct when using vpl011.
> 
> I'll write:
> 
> The property is optional. When absent, we'll use the physical number of
> GIC lines for dom0-less domains, or GUEST_VPL011_SPI+1 if vpl011 is
> requested, whichever is greater.

Sounds good to me.

> 
> 
>>>
>>> Remove the old setting of nr_spis based on the presence of the vpl011.
>>>
>>> The implication of this change is that without nr_spis dom0less domains
>>> get the same amount of SPI allocated as dom0, regardless of how many
>>> physical devices they have assigned, and regardless of whether they have
>>> a virtual pl011 (which also needs an emulated SPI). For instance, we
>>> could end up exposing 256 SPIs for each dom0less domain without a
>>> nr_spis property. If we have 4 dom0less domains without nr_spis, it
>>> would result in 80K of additional memory being used.
>>
>> I don't understand what you are trying to imply with your example. Ok,
>> this tell you how much memory you are going to waste... but this does
>> still not explain why the nr_spis are increased in the default case.
> 
> I misunderstood what you wanted me to add to the commit message.

Sorry for the confusion, my main point is you can't really say this is 
low footprint as this is very subjective. Personally, I feel it is a lot 
because if you take the example, this is roughly 8% of the current size 
of Xen (in default config).

> I'll remove the example and instead write:
> 
> The implication of this change is that without nr_spis dom0less domains
> get the same amount of SPI allocated as dom0, regardless of how many
> physical devices they have assigned, and regardless of whether they have
> a virtual pl011 (which also needs an emulated SPI). This is done because
> the SPIs allocation needs to be done before parsing any passthrough
> information, so we have to account for any potential physical SPI
> assigned to the domain.
> 
> 
> Is this better?

Yes, thank you.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v5 5/8] xen/arm: assign devices to boot domains

[Oleksandr]

On 26.09.19 00:12, Julien Grall wrote:
> Hi Stefano,


Hi Stefano, Julien


>
> On 25/09/2019 19:49, Stefano Stabellini wrote:
>> Scan the user provided dtb fragment at boot. For each device node, map
>> memory to guests, and route interrupts and setup the iommu.
>>
>> The memory region to remap is specified by the "xen,reg" property.
>>
>> The iommu is setup by passing the node of the device to assign on the
>> host device tree. The path is specified in the device tree fragment as
>> the "xen,path" string property.
>>
>> The interrupts are remapped based on the information from the
>> corresponding node on the host device tree. Call
>> handle_device_interrupts to remap interrupts. Interrupts related device
>> tree properties are copied from the device tree fragment, same as all
>> the other properties.
>>
>> Also add the new flag XEN_DOMCTL_CDF_iommu to that dom0less domU can use
>> the IOMMU.
>>
>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>> ---
>> Changes in v5:
>> - use local variable for name
>> - use map_regions_p2mt
>> - add warning for not page aligned addresses/sizes
>> - introduce handle_passthrough_prop
>>
>> Changes in v4:
>> - use unsigned
>> - improve commit message
>> - code style
>> - use dt_prop_cmp
>> - use device_tree_get_reg
>> - don't copy over xen,reg and xen,path
>> - don't create special interrupt properties for domU: copy them from the
>>     fragment
>> - in-code comment
>>
>> Changes in v3:
>> - improve commit message
>> - remove superfluous cast
>> - merge code with the copy code
>> - add interrup-parent
>> - demove depth > 2 check
>> - reuse code from handle_device_interrupts
>> - copy interrupts from host dt
>>
>> Changes in v2:
>> - rename "path" to "xen,path"
>> - grammar fix
>> - use gaddr_to_gfn and maddr_to_mfn
>> - remove depth <= 2 limitation in scanning the dtb fragment
>> - introduce and parse xen,reg
>> - code style
>> - support more than one interrupt per device
>> - specify only the GIC is supported
>> ---
>>    xen/arch/arm/domain_build.c | 101 ++++++++++++++++++++++++++++++++++--
>>    1 file changed, 97 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 9d985d3bbe..414893bc24 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -1701,6 +1701,85 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo)
>>    }
>>    #endif
>>    
>> +/*
>> + * Scan device tree properties for passthrough specific information.
>> + * Returns -ENOENT when no passthrough properties are found
>> + *         < 0 on error
>> + *         0 on success
>> + */
>> +static int __init handle_passthrough_prop(struct kernel_info *kinfo,
>> +                                          const struct fdt_property *prop,
>> +                                          const char *name,
>> +                                          uint32_t address_cells, uint32_t size_cells)
>> +{
>> +    const __be32 *cell;
>> +    unsigned int i, len;
>> +    struct dt_device_node *node;
>> +    int res;
>> +
>> +    /* xen,reg specifies where to map the MMIO region */
>> +    if ( dt_prop_cmp("xen,reg", name) == 0 )
>> +    {
>> +        paddr_t mstart, size, gstart;
>> +        cell = (const __be32 *)prop->data;
>> +        len = fdt32_to_cpu(prop->len) /
>> +            ((address_cells * 2 + size_cells) * sizeof(uint32_t));
>> +
>> +        for ( i = 0; i < len; i++ )
>> +        {
>> +            device_tree_get_reg(&cell, address_cells, size_cells,
>> +                    &mstart, &size);
>> +            gstart = dt_next_cell(address_cells, &cell);
>> +
>> +            if ( gstart & ~PAGE_MASK || mstart & ~PAGE_MASK || size & ~PAGE_MASK )
>> +                dprintk(XENLOG_WARNING,
>> +                        "DomU passthrough config has not page aligned addresses/sizes\n");
> I don't think this is wise to continue, the more this is a printk that
> can only happen in debug build. So someone using a release build may not
> notice the error.
>
> So I think this wants to be a printk(XENLOG_ERR,...) and also return an
> error.
>
>> +
>> +            res = map_regions_p2mt(kinfo->d,
>> +                    gaddr_to_gfn(gstart),
>> +                    PFN_DOWN(size),
>> +                    maddr_to_mfn(mstart),
>> +                    p2m_mmio_direct_dev);
> Coding style.
>
>> +            if ( res < 0 )
>> +            {
>> +                dprintk(XENLOG_ERR,
>> +                        "Failed to map %"PRIpaddr" to the guest at%"PRIpaddr"\n",
>> +                        mstart, gstart);
> Similarly, this wants to be a printk.
>
>> +                return -EFAULT;
>> +            }
>> +        }
>> +
>> +        return 0;
>> +    }
>> +    /*
>> +     * xen,path specifies the corresponding node in the host DT.
>> +     * Both interrupt mappings and IOMMU settings are based on it,
>> +     * as they are done based on the corresponding host DT node.
>> +     */
>> +    else if ( dt_prop_cmp("xen,path", name) == 0 )
>> +    {
>> +        node = dt_find_node_by_path(prop->data);
>> +        if ( node == NULL )
>> +        {
>> +            dprintk(XENLOG_ERR, "Couldn't find node %s in host_dt!\n",
>> +                    (char *)prop->data);
> Same here.
>
>> +            return -EINVAL;
>> +        }

I have to admit that I don't know about dom0less feature enough ...


But, shouldn't we check if the device is behind the IOMMU and try to add 
it (iommu_add_dt_device) before assigning it (this is needed for drivers 
which support generic IOMMU DT bindings in the first place).

[please take a look at 
https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg02714.html 
if so]

Julien, what do you think?


>> +
>> +        res = iommu_assign_dt_device(kinfo->d, node);
>> +        if ( res < 0 )
>> +            return res;
>> +
>> +        res = handle_device_interrupts(kinfo->d, node, true);
>> +        if ( res < 0 )
>> +            return res;
>> +
>> +        return 0;
>> +    }
>> +
>> +    return -ENOENT;
>> +}
>> +
>>    static int __init handle_prop_pfdt(struct kernel_info *kinfo,
>>                                       const void *pfdt, int nodeoff,
>>                                       uint32_t address_cells, uint32_t size_cells,
>> @@ -1709,6 +1788,7 @@ static int __init handle_prop_pfdt(struct kernel_info *kinfo,
>>        void *fdt = kinfo->fdt;
>>        int propoff, nameoff, res;
>>        const struct fdt_property *prop;
>> +    const char *name;
>>    
>>        for ( propoff = fdt_first_property_offset(pfdt, nodeoff);
>>              propoff >= 0;
>> @@ -1717,11 +1797,23 @@ static int __init handle_prop_pfdt(struct kernel_info *kinfo,
>>            if ( !(prop = fdt_get_property_by_offset(pfdt, propoff, NULL)) )
>>                return -FDT_ERR_INTERNAL;
>>    
>> +        res = 0;
>>            nameoff = fdt32_to_cpu(prop->nameoff);
>> -        res = fdt_property(fdt, fdt_string(pfdt, nameoff),
>> -                           prop->data, fdt32_to_cpu(prop->len));
>> -        if ( res )
>> +        name = fdt_string(pfdt, nameoff);
>> +
>> +        if ( scan_passthrough_prop )
>> +            res = handle_passthrough_prop(kinfo, prop, name,
>> +                                          address_cells, size_cells);
>> +        if ( res < 0 && res != -ENOENT )
>>                return res;
>> +
>> +        /* copy all other properties */
>> +        if ( !scan_passthrough_prop || res == -ENOENT )
>> +        {
>> +            res = fdt_property(fdt, name, prop->data, fdt32_to_cpu(prop->len));
>> +            if ( res )
>> +                return res;
>> +        }
>>        }
>>    
>>        /* FDT_ERR_NOTFOUND => There is no more properties for this node */
>> @@ -2254,7 +2346,8 @@ void __init create_domUs(void)
>>            struct xen_domctl_createdomain d_cfg = {
>>                .arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE,
>>                .arch.nr_spis = 0,
>> -            .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
>> +            .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
>> +                     XEN_DOMCTL_CDF_iommu,
> This will break dom0less on platform without an IOMMU because setting
> CDF_iommu for them will be invalid.
>
> I also don't think this is wise to always enable the IOMMU. This should
> only be done if there is a partial device-tree present (most likely it
> means passthrough will be used).
>
>>                .max_evtchn_port = -1,
>>                .max_grant_frames = 64,
>>                .max_maptrack_frames = 1024,
>>
> Cheers,
>
-- 
Regards,

Oleksandr Tyshchenko


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

Re: [Xen-devel] [PATCH v5 5/8] xen/arm: assign devices to boot domains

[Julien Grall]

Hi,

On 27/09/2019 15:40, Oleksandr wrote:
> 
> On 26.09.19 00:12, Julien Grall wrote:
>> On 25/09/2019 19:49, Stefano Stabellini wrote:
>>> Scan the user provided dtb fragment at boot. For each device node, map
>>> memory to guests, and route interrupts and setup the iommu.
>>>
>>> The memory region to remap is specified by the "xen,reg" property.
>>>
>>> The iommu is setup by passing the node of the device to assign on the
>>> host device tree. The path is specified in the device tree fragment as
>>> the "xen,path" string property.
>>>
>>> The interrupts are remapped based on the information from the
>>> corresponding node on the host device tree. Call
>>> handle_device_interrupts to remap interrupts. Interrupts related device
>>> tree properties are copied from the device tree fragment, same as all
>>> the other properties.
>>>
>>> Also add the new flag XEN_DOMCTL_CDF_iommu to that dom0less domU can use
>>> the IOMMU.
>>>
>>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>> ---
>>> Changes in v5:
>>> - use local variable for name
>>> - use map_regions_p2mt
>>> - add warning for not page aligned addresses/sizes
>>> - introduce handle_passthrough_prop
>>>
>>> Changes in v4:
>>> - use unsigned
>>> - improve commit message
>>> - code style
>>> - use dt_prop_cmp
>>> - use device_tree_get_reg
>>> - don't copy over xen,reg and xen,path
>>> - don't create special interrupt properties for domU: copy them from the
>>>     fragment
>>> - in-code comment
>>>
>>> Changes in v3:
>>> - improve commit message
>>> - remove superfluous cast
>>> - merge code with the copy code
>>> - add interrup-parent
>>> - demove depth > 2 check
>>> - reuse code from handle_device_interrupts
>>> - copy interrupts from host dt
>>>
>>> Changes in v2:
>>> - rename "path" to "xen,path"
>>> - grammar fix
>>> - use gaddr_to_gfn and maddr_to_mfn
>>> - remove depth <= 2 limitation in scanning the dtb fragment
>>> - introduce and parse xen,reg
>>> - code style
>>> - support more than one interrupt per device
>>> - specify only the GIC is supported
>>> ---
>>>    xen/arch/arm/domain_build.c | 101 
>>> ++++++++++++++++++++++++++++++++++--
>>>    1 file changed, 97 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 9d985d3bbe..414893bc24 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -1701,6 +1701,85 @@ static int __init make_vpl011_uart_node(struct 
>>> kernel_info *kinfo)
>>>    }
>>>    #endif
>>> +/*
>>> + * Scan device tree properties for passthrough specific information.
>>> + * Returns -ENOENT when no passthrough properties are found
>>> + *         < 0 on error
>>> + *         0 on success
>>> + */
>>> +static int __init handle_passthrough_prop(struct kernel_info *kinfo,
>>> +                                          const struct fdt_property 
>>> *prop,
>>> +                                          const char *name,
>>> +                                          uint32_t address_cells, 
>>> uint32_t size_cells)
>>> +{
>>> +    const __be32 *cell;
>>> +    unsigned int i, len;
>>> +    struct dt_device_node *node;
>>> +    int res;
>>> +
>>> +    /* xen,reg specifies where to map the MMIO region */
>>> +    if ( dt_prop_cmp("xen,reg", name) == 0 )
>>> +    {
>>> +        paddr_t mstart, size, gstart;
>>> +        cell = (const __be32 *)prop->data;
>>> +        len = fdt32_to_cpu(prop->len) /
>>> +            ((address_cells * 2 + size_cells) * sizeof(uint32_t));
>>> +
>>> +        for ( i = 0; i < len; i++ )
>>> +        {
>>> +            device_tree_get_reg(&cell, address_cells, size_cells,
>>> +                    &mstart, &size);
>>> +            gstart = dt_next_cell(address_cells, &cell);
>>> +
>>> +            if ( gstart & ~PAGE_MASK || mstart & ~PAGE_MASK || size 
>>> & ~PAGE_MASK )
>>> +                dprintk(XENLOG_WARNING,
>>> +                        "DomU passthrough config has not page 
>>> aligned addresses/sizes\n");
>> I don't think this is wise to continue, the more this is a printk that
>> can only happen in debug build. So someone using a release build may not
>> notice the error.
>>
>> So I think this wants to be a printk(XENLOG_ERR,...) and also return an
>> error.
>>
>>> +
>>> +            res = map_regions_p2mt(kinfo->d,
>>> +                    gaddr_to_gfn(gstart),
>>> +                    PFN_DOWN(size),
>>> +                    maddr_to_mfn(mstart),
>>> +                    p2m_mmio_direct_dev);
>> Coding style.
>>
>>> +            if ( res < 0 )
>>> +            {
>>> +                dprintk(XENLOG_ERR,
>>> +                        "Failed to map %"PRIpaddr" to the guest 
>>> at%"PRIpaddr"\n",
>>> +                        mstart, gstart);
>> Similarly, this wants to be a printk.
>>
>>> +                return -EFAULT;
>>> +            }
>>> +        }
>>> +
>>> +        return 0;
>>> +    }
>>> +    /*
>>> +     * xen,path specifies the corresponding node in the host DT.
>>> +     * Both interrupt mappings and IOMMU settings are based on it,
>>> +     * as they are done based on the corresponding host DT node.
>>> +     */
>>> +    else if ( dt_prop_cmp("xen,path", name) == 0 )
>>> +    {
>>> +        node = dt_find_node_by_path(prop->data);
>>> +        if ( node == NULL )
>>> +        {
>>> +            dprintk(XENLOG_ERR, "Couldn't find node %s in host_dt!\n",
>>> +                    (char *)prop->data);
>> Same here.
>>
>>> +            return -EINVAL;
>>> +        }
> 
> I have to admit that I don't know about dom0less feature enough ...
> 
> 
> But, shouldn't we check if the device is behind the IOMMU and try to add 
> it (iommu_add_dt_device) before assigning it (this is needed for drivers 
> which support generic IOMMU DT bindings in the first place).
> 
> [please take a look at 
> https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg02714.html 
> if so]
> 
> Julien, what do you think?

Yes you are right.

@Stefano, this is a recently merged feature. Without it, you will not be 
able to use passthrough with dom0less guest when the IOMMU (such as 
IPMMU) is using the generic DT bindings.

Cheers,

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

Re: [Xen-devel] [PATCH v5 5/8] xen/arm: assign devices to boot domains

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 7615 bytes --]

On Fri, 27 Sep 2019, Julien Grall wrote:
> Hi,
> 
> On 27/09/2019 15:40, Oleksandr wrote:
> > 
> > On 26.09.19 00:12, Julien Grall wrote:
> >> On 25/09/2019 19:49, Stefano Stabellini wrote:
> >>> Scan the user provided dtb fragment at boot. For each device node, map
> >>> memory to guests, and route interrupts and setup the iommu.
> >>>
> >>> The memory region to remap is specified by the "xen,reg" property.
> >>>
> >>> The iommu is setup by passing the node of the device to assign on the
> >>> host device tree. The path is specified in the device tree fragment as
> >>> the "xen,path" string property.
> >>>
> >>> The interrupts are remapped based on the information from the
> >>> corresponding node on the host device tree. Call
> >>> handle_device_interrupts to remap interrupts. Interrupts related device
> >>> tree properties are copied from the device tree fragment, same as all
> >>> the other properties.
> >>>
> >>> Also add the new flag XEN_DOMCTL_CDF_iommu to that dom0less domU can use
> >>> the IOMMU.
> >>>
> >>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> >>> ---
> >>> Changes in v5:
> >>> - use local variable for name
> >>> - use map_regions_p2mt
> >>> - add warning for not page aligned addresses/sizes
> >>> - introduce handle_passthrough_prop
> >>>
> >>> Changes in v4:
> >>> - use unsigned
> >>> - improve commit message
> >>> - code style
> >>> - use dt_prop_cmp
> >>> - use device_tree_get_reg
> >>> - don't copy over xen,reg and xen,path
> >>> - don't create special interrupt properties for domU: copy them from the
> >>>     fragment
> >>> - in-code comment
> >>>
> >>> Changes in v3:
> >>> - improve commit message
> >>> - remove superfluous cast
> >>> - merge code with the copy code
> >>> - add interrup-parent
> >>> - demove depth > 2 check
> >>> - reuse code from handle_device_interrupts
> >>> - copy interrupts from host dt
> >>>
> >>> Changes in v2:
> >>> - rename "path" to "xen,path"
> >>> - grammar fix
> >>> - use gaddr_to_gfn and maddr_to_mfn
> >>> - remove depth <= 2 limitation in scanning the dtb fragment
> >>> - introduce and parse xen,reg
> >>> - code style
> >>> - support more than one interrupt per device
> >>> - specify only the GIC is supported
> >>> ---
> >>>    xen/arch/arm/domain_build.c | 101 
> >>> ++++++++++++++++++++++++++++++++++--
> >>>    1 file changed, 97 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> >>> index 9d985d3bbe..414893bc24 100644
> >>> --- a/xen/arch/arm/domain_build.c
> >>> +++ b/xen/arch/arm/domain_build.c
> >>> @@ -1701,6 +1701,85 @@ static int __init make_vpl011_uart_node(struct 
> >>> kernel_info *kinfo)
> >>>    }
> >>>    #endif
> >>> +/*
> >>> + * Scan device tree properties for passthrough specific information.
> >>> + * Returns -ENOENT when no passthrough properties are found
> >>> + *         < 0 on error
> >>> + *         0 on success
> >>> + */
> >>> +static int __init handle_passthrough_prop(struct kernel_info *kinfo,
> >>> +                                          const struct fdt_property 
> >>> *prop,
> >>> +                                          const char *name,
> >>> +                                          uint32_t address_cells, 
> >>> uint32_t size_cells)
> >>> +{
> >>> +    const __be32 *cell;
> >>> +    unsigned int i, len;
> >>> +    struct dt_device_node *node;
> >>> +    int res;
> >>> +
> >>> +    /* xen,reg specifies where to map the MMIO region */
> >>> +    if ( dt_prop_cmp("xen,reg", name) == 0 )
> >>> +    {
> >>> +        paddr_t mstart, size, gstart;
> >>> +        cell = (const __be32 *)prop->data;
> >>> +        len = fdt32_to_cpu(prop->len) /
> >>> +            ((address_cells * 2 + size_cells) * sizeof(uint32_t));
> >>> +
> >>> +        for ( i = 0; i < len; i++ )
> >>> +        {
> >>> +            device_tree_get_reg(&cell, address_cells, size_cells,
> >>> +                    &mstart, &size);
> >>> +            gstart = dt_next_cell(address_cells, &cell);
> >>> +
> >>> +            if ( gstart & ~PAGE_MASK || mstart & ~PAGE_MASK || size 
> >>> & ~PAGE_MASK )
> >>> +                dprintk(XENLOG_WARNING,
> >>> +                        "DomU passthrough config has not page 
> >>> aligned addresses/sizes\n");
> >> I don't think this is wise to continue, the more this is a printk that
> >> can only happen in debug build. So someone using a release build may not
> >> notice the error.
> >>
> >> So I think this wants to be a printk(XENLOG_ERR,...) and also return an
> >> error.
> >>
> >>> +
> >>> +            res = map_regions_p2mt(kinfo->d,
> >>> +                    gaddr_to_gfn(gstart),
> >>> +                    PFN_DOWN(size),
> >>> +                    maddr_to_mfn(mstart),
> >>> +                    p2m_mmio_direct_dev);
> >> Coding style.
> >>
> >>> +            if ( res < 0 )
> >>> +            {
> >>> +                dprintk(XENLOG_ERR,
> >>> +                        "Failed to map %"PRIpaddr" to the guest 
> >>> at%"PRIpaddr"\n",
> >>> +                        mstart, gstart);
> >> Similarly, this wants to be a printk.
> >>
> >>> +                return -EFAULT;
> >>> +            }
> >>> +        }
> >>> +
> >>> +        return 0;
> >>> +    }
> >>> +    /*
> >>> +     * xen,path specifies the corresponding node in the host DT.
> >>> +     * Both interrupt mappings and IOMMU settings are based on it,
> >>> +     * as they are done based on the corresponding host DT node.
> >>> +     */
> >>> +    else if ( dt_prop_cmp("xen,path", name) == 0 )
> >>> +    {
> >>> +        node = dt_find_node_by_path(prop->data);
> >>> +        if ( node == NULL )
> >>> +        {
> >>> +            dprintk(XENLOG_ERR, "Couldn't find node %s in host_dt!\n",
> >>> +                    (char *)prop->data);
> >> Same here.
> >>
> >>> +            return -EINVAL;
> >>> +        }
> > 
> > I have to admit that I don't know about dom0less feature enough ...
> > 
> > 
> > But, shouldn't we check if the device is behind the IOMMU and try to add 
> > it (iommu_add_dt_device) before assigning it (this is needed for drivers 
> > which support generic IOMMU DT bindings in the first place).
> > 
> > [please take a look at 
> > https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg02714.html 
> > if so]
> > 
> > Julien, what do you think?
> 
> Yes you are right.
> 
> @Stefano, this is a recently merged feature. Without it, you will not be 
> able to use passthrough with dom0less guest when the IOMMU (such as 
> IPMMU) is using the generic DT bindings.

Just double-checking but it should be only a matter of the following,
right?

+        res = iommu_add_dt_device(node);
+        if ( res < 0 )
+            return res;
+
+        if ( dt_device_is_protected(node) )
+        {
+            res = iommu_assign_dt_device(kinfo->d, node);
+            if ( res < 0 )
+                return res;
+        }
+

(I am asking because I couldn't quite test it due to the error with
mmu-masters I mentioned in the other email.)

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

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

Re: [Xen-devel] [PATCH v5 5/8] xen/arm: assign devices to boot domains

[Oleksandr]

On 28.09.19 02:28, Stefano Stabellini wrote:

Hi Stefano

> On Fri, 27 Sep 2019, Julien Grall wrote:
>> Hi,
>>
>> On 27/09/2019 15:40, Oleksandr wrote:
>>> On 26.09.19 00:12, Julien Grall wrote:
>>>> On 25/09/2019 19:49, Stefano Stabellini wrote:
>>>>> Scan the user provided dtb fragment at boot. For each device node, map
>>>>> memory to guests, and route interrupts and setup the iommu.
>>>>>
>>>>> The memory region to remap is specified by the "xen,reg" property.
>>>>>
>>>>> The iommu is setup by passing the node of the device to assign on the
>>>>> host device tree. The path is specified in the device tree fragment as
>>>>> the "xen,path" string property.
>>>>>
>>>>> The interrupts are remapped based on the information from the
>>>>> corresponding node on the host device tree. Call
>>>>> handle_device_interrupts to remap interrupts. Interrupts related device
>>>>> tree properties are copied from the device tree fragment, same as all
>>>>> the other properties.
>>>>>
>>>>> Also add the new flag XEN_DOMCTL_CDF_iommu to that dom0less domU can use
>>>>> the IOMMU.
>>>>>
>>>>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>>>> ---
>>>>> Changes in v5:
>>>>> - use local variable for name
>>>>> - use map_regions_p2mt
>>>>> - add warning for not page aligned addresses/sizes
>>>>> - introduce handle_passthrough_prop
>>>>>
>>>>> Changes in v4:
>>>>> - use unsigned
>>>>> - improve commit message
>>>>> - code style
>>>>> - use dt_prop_cmp
>>>>> - use device_tree_get_reg
>>>>> - don't copy over xen,reg and xen,path
>>>>> - don't create special interrupt properties for domU: copy them from the
>>>>>      fragment
>>>>> - in-code comment
>>>>>
>>>>> Changes in v3:
>>>>> - improve commit message
>>>>> - remove superfluous cast
>>>>> - merge code with the copy code
>>>>> - add interrup-parent
>>>>> - demove depth > 2 check
>>>>> - reuse code from handle_device_interrupts
>>>>> - copy interrupts from host dt
>>>>>
>>>>> Changes in v2:
>>>>> - rename "path" to "xen,path"
>>>>> - grammar fix
>>>>> - use gaddr_to_gfn and maddr_to_mfn
>>>>> - remove depth <= 2 limitation in scanning the dtb fragment
>>>>> - introduce and parse xen,reg
>>>>> - code style
>>>>> - support more than one interrupt per device
>>>>> - specify only the GIC is supported
>>>>> ---
>>>>>     xen/arch/arm/domain_build.c | 101
>>>>> ++++++++++++++++++++++++++++++++++--
>>>>>     1 file changed, 97 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>>>> index 9d985d3bbe..414893bc24 100644
>>>>> --- a/xen/arch/arm/domain_build.c
>>>>> +++ b/xen/arch/arm/domain_build.c
>>>>> @@ -1701,6 +1701,85 @@ static int __init make_vpl011_uart_node(struct
>>>>> kernel_info *kinfo)
>>>>>     }
>>>>>     #endif
>>>>> +/*
>>>>> + * Scan device tree properties for passthrough specific information.
>>>>> + * Returns -ENOENT when no passthrough properties are found
>>>>> + *         < 0 on error
>>>>> + *         0 on success
>>>>> + */
>>>>> +static int __init handle_passthrough_prop(struct kernel_info *kinfo,
>>>>> +                                          const struct fdt_property
>>>>> *prop,
>>>>> +                                          const char *name,
>>>>> +                                          uint32_t address_cells,
>>>>> uint32_t size_cells)
>>>>> +{
>>>>> +    const __be32 *cell;
>>>>> +    unsigned int i, len;
>>>>> +    struct dt_device_node *node;
>>>>> +    int res;
>>>>> +
>>>>> +    /* xen,reg specifies where to map the MMIO region */
>>>>> +    if ( dt_prop_cmp("xen,reg", name) == 0 )
>>>>> +    {
>>>>> +        paddr_t mstart, size, gstart;
>>>>> +        cell = (const __be32 *)prop->data;
>>>>> +        len = fdt32_to_cpu(prop->len) /
>>>>> +            ((address_cells * 2 + size_cells) * sizeof(uint32_t));
>>>>> +
>>>>> +        for ( i = 0; i < len; i++ )
>>>>> +        {
>>>>> +            device_tree_get_reg(&cell, address_cells, size_cells,
>>>>> +                    &mstart, &size);
>>>>> +            gstart = dt_next_cell(address_cells, &cell);
>>>>> +
>>>>> +            if ( gstart & ~PAGE_MASK || mstart & ~PAGE_MASK || size
>>>>> & ~PAGE_MASK )
>>>>> +                dprintk(XENLOG_WARNING,
>>>>> +                        "DomU passthrough config has not page
>>>>> aligned addresses/sizes\n");
>>>> I don't think this is wise to continue, the more this is a printk that
>>>> can only happen in debug build. So someone using a release build may not
>>>> notice the error.
>>>>
>>>> So I think this wants to be a printk(XENLOG_ERR,...) and also return an
>>>> error.
>>>>
>>>>> +
>>>>> +            res = map_regions_p2mt(kinfo->d,
>>>>> +                    gaddr_to_gfn(gstart),
>>>>> +                    PFN_DOWN(size),
>>>>> +                    maddr_to_mfn(mstart),
>>>>> +                    p2m_mmio_direct_dev);
>>>> Coding style.
>>>>
>>>>> +            if ( res < 0 )
>>>>> +            {
>>>>> +                dprintk(XENLOG_ERR,
>>>>> +                        "Failed to map %"PRIpaddr" to the guest
>>>>> at%"PRIpaddr"\n",
>>>>> +                        mstart, gstart);
>>>> Similarly, this wants to be a printk.
>>>>
>>>>> +                return -EFAULT;
>>>>> +            }
>>>>> +        }
>>>>> +
>>>>> +        return 0;
>>>>> +    }
>>>>> +    /*
>>>>> +     * xen,path specifies the corresponding node in the host DT.
>>>>> +     * Both interrupt mappings and IOMMU settings are based on it,
>>>>> +     * as they are done based on the corresponding host DT node.
>>>>> +     */
>>>>> +    else if ( dt_prop_cmp("xen,path", name) == 0 )
>>>>> +    {
>>>>> +        node = dt_find_node_by_path(prop->data);
>>>>> +        if ( node == NULL )
>>>>> +        {
>>>>> +            dprintk(XENLOG_ERR, "Couldn't find node %s in host_dt!\n",
>>>>> +                    (char *)prop->data);
>>>> Same here.
>>>>
>>>>> +            return -EINVAL;
>>>>> +        }
>>> I have to admit that I don't know about dom0less feature enough ...
>>>
>>>
>>> But, shouldn't we check if the device is behind the IOMMU and try to add
>>> it (iommu_add_dt_device) before assigning it (this is needed for drivers
>>> which support generic IOMMU DT bindings in the first place).
>>>
>>> [please take a look at
>>> https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg02714.html
>>> if so]
>>>
>>> Julien, what do you think?
>> Yes you are right.
>>
>> @Stefano, this is a recently merged feature. Without it, you will not be
>> able to use passthrough with dom0less guest when the IOMMU (such as
>> IPMMU) is using the generic DT bindings.
> Just double-checking but it should be only a matter of the following,
> right?
>
> +        res = iommu_add_dt_device(node);
> +        if ( res < 0 )
> +            return res;

I think, the change above is correct.


> +
> +        if ( dt_device_is_protected(node) )
> +        {
> +            res = iommu_assign_dt_device(kinfo->d, node);
> +            if ( res < 0 )
> +                return res;
> +        }
> +
>
> (I am asking because I couldn't quite test it due to the error with
> mmu-masters I mentioned in the other email.)
Regarding the check "if (dt_device_is_protected(node))" here. I think, 
it depends on the "xen,path" purpose.

1. If "xen,path" property is, let say, close to "dtdev" property in 
domain config file, where we describe master devices which are behind 
the IOMMU, so *must* be protected, then that check should be removed. 
Please see iommu_do_dt_domctl().

2. If "xen,path" property can also be used to describe devices which are 
not behind the IOMMU (so don't need to be protected), but just for the 
"interrupt mappings" purposes, then that check is correct and should remain.

-- 
Regards,

Oleksandr Tyshchenko


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

Re: [Xen-devel] [PATCH v5 5/8] xen/arm: assign devices to boot domains

[Julien Grall]

Hi Oleksandr,

On 30/09/2019 10:34, Oleksandr wrote:
> On 28.09.19 02:28, Stefano Stabellini wrote:
>>>> I have to admit that I don't know about dom0less feature enough ...
>>>>
>>>>
>>>> But, shouldn't we check if the device is behind the IOMMU and try to add
>>>> it (iommu_add_dt_device) before assigning it (this is needed for drivers
>>>> which support generic IOMMU DT bindings in the first place).
>>>>
>>>> [please take a look at
>>>> https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg02714.html
>>>> if so]
>>>>
>>>> Julien, what do you think?
>>> Yes you are right.
>>>
>>> @Stefano, this is a recently merged feature. Without it, you will not be
>>> able to use passthrough with dom0less guest when the IOMMU (such as
>>> IPMMU) is using the generic DT bindings.
>> Just double-checking but it should be only a matter of the following,
>> right?
>>
>> +        res = iommu_add_dt_device(node);
>> +        if ( res < 0 )
>> +            return res;
> 
> I think, the change above is correct.
> 
> 
>> +
>> +        if ( dt_device_is_protected(node) )
>> +        {
>> +            res = iommu_assign_dt_device(kinfo->d, node);
>> +            if ( res < 0 )
>> +                return res;
>> +        }
>> +
>>
>> (I am asking because I couldn't quite test it due to the error with
>> mmu-masters I mentioned in the other email.)
> Regarding the check "if (dt_device_is_protected(node))" here. I think, it 
> depends on the "xen,path" purpose.
> 
> 1. If "xen,path" property is, let say, close to "dtdev" property in domain 
> config file, where we describe master devices which are behind the IOMMU, so 
> *must* be protected, then that check should be removed. Please see 
> iommu_do_dt_domctl().
> 
> 2. If "xen,path" property can also be used to describe devices which are not 
> behind the IOMMU (so don't need to be protected), but just for the "interrupt 
> mappings" purposes, then that check is correct and should remain.

Some device may not be behind an IOMMU but still do DMA. We are not doing a 
favor to the user to continue the assignment as this could lead to at best to a 
non-working device (at worst a security issue).

Therefore I am against the solution 2).

However, this raises some questions why MMIOs are treated differently (i.e they 
don't need an IOMMU).

In the current setup, you would not be able to passthrough a non DMA-capable to 
a guest if they needs interrupts (e.g. an UART) but you would be if they don't 
use interrupts.

So I think we need a couple of more changes:
    1) Introduce an option to allow the user to ignore IOMMU issues (something 
like "xen,force-assign-without-iommu").
    2) "xen,reg" cannot be specified without "xen,path". This allows us to 
police the user DT.

Any opinions?

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v5 5/8] xen/arm: assign devices to boot domains

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 4307 bytes --]

On Mon, 30 Sep 2019, Julien Grall wrote:
> Hi Oleksandr,
> 
> On 30/09/2019 10:34, Oleksandr wrote:
> > On 28.09.19 02:28, Stefano Stabellini wrote:
> > > > > I have to admit that I don't know about dom0less feature enough ...
> > > > > 
> > > > > 
> > > > > But, shouldn't we check if the device is behind the IOMMU and try to
> > > > > add
> > > > > it (iommu_add_dt_device) before assigning it (this is needed for
> > > > > drivers
> > > > > which support generic IOMMU DT bindings in the first place).
> > > > > 
> > > > > [please take a look at
> > > > > https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg02714.html
> > > > > if so]
> > > > > 
> > > > > Julien, what do you think?
> > > > Yes you are right.
> > > > 
> > > > @Stefano, this is a recently merged feature. Without it, you will not be
> > > > able to use passthrough with dom0less guest when the IOMMU (such as
> > > > IPMMU) is using the generic DT bindings.
> > > Just double-checking but it should be only a matter of the following,
> > > right?
> > > 
> > > +        res = iommu_add_dt_device(node);
> > > +        if ( res < 0 )
> > > +            return res;
> > 
> > I think, the change above is correct.
> > 
> > 
> > > +
> > > +        if ( dt_device_is_protected(node) )
> > > +        {
> > > +            res = iommu_assign_dt_device(kinfo->d, node);
> > > +            if ( res < 0 )
> > > +                return res;
> > > +        }
> > > +
> > > 
> > > (I am asking because I couldn't quite test it due to the error with
> > > mmu-masters I mentioned in the other email.)
> > Regarding the check "if (dt_device_is_protected(node))" here. I think, it
> > depends on the "xen,path" purpose.
> > 
> > 1. If "xen,path" property is, let say, close to "dtdev" property in domain
> > config file, where we describe master devices which are behind the IOMMU, so
> > *must* be protected, then that check should be removed. Please see
> > iommu_do_dt_domctl().
> > 
> > 2. If "xen,path" property can also be used to describe devices which are not
> > behind the IOMMU (so don't need to be protected), but just for the
> > "interrupt mappings" purposes, then that check is correct and should remain.
> 
> Some device may not be behind an IOMMU but still do DMA. We are not doing a
> favor to the user to continue the assignment as this could lead to at best to
> a non-working device (at worst a security issue).
> 
> Therefore I am against the solution 2).

I agree. (And honestly, "xen,path" was introduced as an equivalent of
"dtdev" initially.)


> However, this raises some questions why MMIOs are treated differently (i.e
> they don't need an IOMMU).
> 
> In the current setup, you would not be able to passthrough a non DMA-capable
> to a guest if they needs interrupts (e.g. an UART) but you would be if they
> don't use interrupts.
> 
> So I think we need a couple of more changes:
>    1) Introduce an option to allow the user to ignore IOMMU issues (something
> like "xen,force-assign-without-iommu").
>    2) "xen,reg" cannot be specified without "xen,path". This allows us to
> police the user DT.

Interesting questions.

Something like "xen,force-assign-without-iommu" would be useful. The
upside of being able to assign a non-IOMMU-protected non-DMA-capable
device outweighs the downsides.

I am less sure about having to specify "xen,reg" together with
"xen,path". It is fairly common to have a control register MMIO region
page in FPGA that doesn't do any DMA and has no related interrupts. In
those cases, it is nice to be able to handle it by just having one
"xen,reg" property. But maybe if the user also passes
"xen,force-assign-without-iommu" then we could also ignore a missing
"xen,path".

In any case, my preference would be to keep the series as is for now,
and make these changes later. However, for the sake of moving things
forward quickly, I also implemented Julien's suggestions. So I'll send
two v7 updates to this series:

- v7a: the minimal changes version, without things discussed here except
       for removing the "if (dt_device_is_protected(node))" check
- v7b: a version with all the changes discussed here

Julien, I'll let you pick your favorite, hopefully one of them will be
to your liking.

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

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

Re: [Xen-devel] [PATCH v5 5/8] xen/arm: assign devices to boot domains

[Julien Grall]

Hi Stefano,

On 01/10/2019 00:24, Stefano Stabellini wrote:
> On Mon, 30 Sep 2019, Julien Grall wrote:
>> Hi Oleksandr,
>>
>> On 30/09/2019 10:34, Oleksandr wrote:
>>> On 28.09.19 02:28, Stefano Stabellini wrote:
>>>>>> I have to admit that I don't know about dom0less feature enough ...
>>>>>>
>>>>>>
>>>>>> But, shouldn't we check if the device is behind the IOMMU and try to
>>>>>> add
>>>>>> it (iommu_add_dt_device) before assigning it (this is needed for
>>>>>> drivers
>>>>>> which support generic IOMMU DT bindings in the first place).
>>>>>>
>>>>>> [please take a look at
>>>>>> https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg02714.html
>>>>>> if so]
>>>>>>
>>>>>> Julien, what do you think?
>>>>> Yes you are right.
>>>>>
>>>>> @Stefano, this is a recently merged feature. Without it, you will not be
>>>>> able to use passthrough with dom0less guest when the IOMMU (such as
>>>>> IPMMU) is using the generic DT bindings.
>>>> Just double-checking but it should be only a matter of the following,
>>>> right?
>>>>
>>>> +        res = iommu_add_dt_device(node);
>>>> +        if ( res < 0 )
>>>> +            return res;
>>>
>>> I think, the change above is correct.
>>>
>>>
>>>> +
>>>> +        if ( dt_device_is_protected(node) )
>>>> +        {
>>>> +            res = iommu_assign_dt_device(kinfo->d, node);
>>>> +            if ( res < 0 )
>>>> +                return res;
>>>> +        }
>>>> +
>>>>
>>>> (I am asking because I couldn't quite test it due to the error with
>>>> mmu-masters I mentioned in the other email.)
>>> Regarding the check "if (dt_device_is_protected(node))" here. I think, it
>>> depends on the "xen,path" purpose.
>>>
>>> 1. If "xen,path" property is, let say, close to "dtdev" property in domain
>>> config file, where we describe master devices which are behind the IOMMU, so
>>> *must* be protected, then that check should be removed. Please see
>>> iommu_do_dt_domctl().
>>>
>>> 2. If "xen,path" property can also be used to describe devices which are not
>>> behind the IOMMU (so don't need to be protected), but just for the
>>> "interrupt mappings" purposes, then that check is correct and should remain.
>>
>> Some device may not be behind an IOMMU but still do DMA. We are not doing a
>> favor to the user to continue the assignment as this could lead to at best to
>> a non-working device (at worst a security issue).
>>
>> Therefore I am against the solution 2).
> 
> I agree. (And honestly, "xen,path" was introduced as an equivalent of
> "dtdev" initially.)
> 
> 
>> However, this raises some questions why MMIOs are treated differently (i.e
>> they don't need an IOMMU).
>>
>> In the current setup, you would not be able to passthrough a non DMA-capable
>> to a guest if they needs interrupts (e.g. an UART) but you would be if they
>> don't use interrupts.
>>
>> So I think we need a couple of more changes:
>>     1) Introduce an option to allow the user to ignore IOMMU issues (something
>> like "xen,force-assign-without-iommu").
>>     2) "xen,reg" cannot be specified without "xen,path". This allows us to
>> police the user DT.
> 
> Interesting questions.
> 
> Something like "xen,force-assign-without-iommu" would be useful. The
> upside of being able to assign a non-IOMMU-protected non-DMA-capable
> device outweighs the downsides.
> 
> I am less sure about having to specify "xen,reg" together with
> "xen,path". It is fairly common to have a control register MMIO region
> page in FPGA that doesn't do any DMA and has no related interrupts. In
> those cases, it is nice to be able to handle it by just having one
> "xen,reg" property. But maybe if the user also passes
> "xen,force-assign-without-iommu" then we could also ignore a missing
> "xen,path".
> 
> In any case, my preference would be to keep the series as is for now,
> and make these changes later. 

Bindings are meant to be stable, so we would end up to have to create a new 
bindings to cater the solution discussed here. So I would rather avoid to take 
that approach.

> However, for the sake of moving things
> forward quickly, I also implemented Julien's suggestions. So I'll send
> two v7 updates to this series:
> 
> - v7a: the minimal changes version, without things discussed here except
>         for removing the "if (dt_device_is_protected(node))" check
> - v7b: a version with all the changes discussed here
> 
> Julien, I'll let you pick your favorite, hopefully one of them will be
> to your liking.

Thank you for suggesting the two versions. I will have a look at them.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [PATCH v5 5/8] xen/arm: assign devices to boot domains

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 5587 bytes --]

On Tue, 1 Oct 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 01/10/2019 00:24, Stefano Stabellini wrote:
> > On Mon, 30 Sep 2019, Julien Grall wrote:
> > > Hi Oleksandr,
> > > 
> > > On 30/09/2019 10:34, Oleksandr wrote:
> > > > On 28.09.19 02:28, Stefano Stabellini wrote:
> > > > > > > I have to admit that I don't know about dom0less feature enough
> > > > > > > ...
> > > > > > > 
> > > > > > > 
> > > > > > > But, shouldn't we check if the device is behind the IOMMU and try
> > > > > > > to
> > > > > > > add
> > > > > > > it (iommu_add_dt_device) before assigning it (this is needed for
> > > > > > > drivers
> > > > > > > which support generic IOMMU DT bindings in the first place).
> > > > > > > 
> > > > > > > [please take a look at
> > > > > > > https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg02714.html
> > > > > > > if so]
> > > > > > > 
> > > > > > > Julien, what do you think?
> > > > > > Yes you are right.
> > > > > > 
> > > > > > @Stefano, this is a recently merged feature. Without it, you will
> > > > > > not be
> > > > > > able to use passthrough with dom0less guest when the IOMMU (such as
> > > > > > IPMMU) is using the generic DT bindings.
> > > > > Just double-checking but it should be only a matter of the following,
> > > > > right?
> > > > > 
> > > > > +        res = iommu_add_dt_device(node);
> > > > > +        if ( res < 0 )
> > > > > +            return res;
> > > > 
> > > > I think, the change above is correct.
> > > > 
> > > > 
> > > > > +
> > > > > +        if ( dt_device_is_protected(node) )
> > > > > +        {
> > > > > +            res = iommu_assign_dt_device(kinfo->d, node);
> > > > > +            if ( res < 0 )
> > > > > +                return res;
> > > > > +        }
> > > > > +
> > > > > 
> > > > > (I am asking because I couldn't quite test it due to the error with
> > > > > mmu-masters I mentioned in the other email.)
> > > > Regarding the check "if (dt_device_is_protected(node))" here. I think,
> > > > it
> > > > depends on the "xen,path" purpose.
> > > > 
> > > > 1. If "xen,path" property is, let say, close to "dtdev" property in
> > > > domain
> > > > config file, where we describe master devices which are behind the
> > > > IOMMU, so
> > > > *must* be protected, then that check should be removed. Please see
> > > > iommu_do_dt_domctl().
> > > > 
> > > > 2. If "xen,path" property can also be used to describe devices which are
> > > > not
> > > > behind the IOMMU (so don't need to be protected), but just for the
> > > > "interrupt mappings" purposes, then that check is correct and should
> > > > remain.
> > > 
> > > Some device may not be behind an IOMMU but still do DMA. We are not doing
> > > a
> > > favor to the user to continue the assignment as this could lead to at best
> > > to
> > > a non-working device (at worst a security issue).
> > > 
> > > Therefore I am against the solution 2).
> > 
> > I agree. (And honestly, "xen,path" was introduced as an equivalent of
> > "dtdev" initially.)
> > 
> > 
> > > However, this raises some questions why MMIOs are treated differently (i.e
> > > they don't need an IOMMU).
> > > 
> > > In the current setup, you would not be able to passthrough a non
> > > DMA-capable
> > > to a guest if they needs interrupts (e.g. an UART) but you would be if
> > > they
> > > don't use interrupts.
> > > 
> > > So I think we need a couple of more changes:
> > >     1) Introduce an option to allow the user to ignore IOMMU issues
> > > (something
> > > like "xen,force-assign-without-iommu").
> > >     2) "xen,reg" cannot be specified without "xen,path". This allows us to
> > > police the user DT.
> > 
> > Interesting questions.
> > 
> > Something like "xen,force-assign-without-iommu" would be useful. The
> > upside of being able to assign a non-IOMMU-protected non-DMA-capable
> > device outweighs the downsides.
> > 
> > I am less sure about having to specify "xen,reg" together with
> > "xen,path". It is fairly common to have a control register MMIO region
> > page in FPGA that doesn't do any DMA and has no related interrupts. In
> > those cases, it is nice to be able to handle it by just having one
> > "xen,reg" property. But maybe if the user also passes
> > "xen,force-assign-without-iommu" then we could also ignore a missing
> > "xen,path".
> > 
> > In any case, my preference would be to keep the series as is for now,
> > and make these changes later. 
> 
> Bindings are meant to be stable, so we would end up to have to create a new
> bindings to cater the solution discussed here. So I would rather avoid to take
> that approach.

Adding a note here from our discussion on IRC. One idea would be to keep
the code as is (v7a) but to make sure the docs reflect that xen,reg and
xen,path are both required. That would be good. However, the docs
already imply it so I didn't actually need to make any changes in that
respect in v7a. In any case, I could certainly add a statement or two to
the docs if it helps.


> > However, for the sake of moving things
> > forward quickly, I also implemented Julien's suggestions. So I'll send
> > two v7 updates to this series:
> > 
> > - v7a: the minimal changes version, without things discussed here except
> >         for removing the "if (dt_device_is_protected(node))" check
> > - v7b: a version with all the changes discussed here
> > 
> > Julien, I'll let you pick your favorite, hopefully one of them will be
> > to your liking.
> 
> Thank you for suggesting the two versions. I will have a look at them.

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

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