[PATCH 0/5] dom0less device assignment

[PATCH 0/5] 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 70739427f55d595ad1c575c47fef00c81881e9a2:

  pci: apply workaround for Intel errata HSE43 and BDF2/BDX2 (2018-12-04 14:04:54 +0100)

are available in the git repository at:

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

for you to fetch changes up to a62cd6de9c202db70dae1d761f4e575b5b67dafb:

  xen/arm: add dom0less device assignment info to docs (2018-12-05 09:22:23 -0800)

----------------------------------------------------------------
Stefano Stabellini (5):
      xen/arm: copy dtb fragment to guest dtb
      xen/arm: assign devices to boot domains
      xen/arm: handle "multiboot,dtb" compatible nodes
      xen/arm: use the physical number of gic lines for boot domains
      xen/arm: add dom0less device assignment info to docs

 docs/misc/arm/device-tree/booting.txt | 108 ++++++++++++++++++++++
 xen/arch/arm/bootfdt.c                |   6 +-
 xen/arch/arm/domain_build.c           | 166 +++++++++++++++++++++++++++++++++-
 xen/arch/arm/kernel.c                 |  12 ++-
 xen/arch/arm/setup.c                  |   1 +
 xen/include/asm-arm/kernel.h          |   2 +-
 xen/include/asm-arm/setup.h           |   1 +
 xen/include/xen/device_tree.h         |   2 +
 8 files changed, 291 insertions(+), 7 deletions(-)

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

[PATCH 1/5] 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,dtb" compatible node.

Copy the fragment to the guest dtb.

Add a dtb_bootmodule field to struct kernel_info to find the dtb
fragment for a guest.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
 xen/arch/arm/domain_build.c  | 77 ++++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/kernel.h |  2 +-
 2 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b0ec3f0..cc6b464 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>
@@ -1669,6 +1670,59 @@ static int __init make_vpl011_uart_node(const struct domain *d, void *fdt)
 }
 #endif
 
+static int __init copy_properties(void *fdt, void *pfdt, int nodeoff)
+{
+    int propoff, nameoff, r;
+    const struct fdt_property *prop;
+
+    for ( propoff = fdt_first_property_offset(pfdt, nodeoff);
+          propoff >= 0;
+          propoff = fdt_next_property_offset(pfdt, propoff) ) {
+
+        if ( !(prop = fdt_get_property_by_offset(pfdt, propoff, NULL)) )
+            return -FDT_ERR_INTERNAL;
+
+        nameoff = fdt32_to_cpu(prop->nameoff);
+        r = fdt_property(fdt, fdt_string(pfdt, nameoff),
+                         prop->data, fdt32_to_cpu(prop->len));
+        if ( r )
+            return r;
+    }
+
+    /* FDT_ERR_NOTFOUND => There is no more properties for this node */
+    return ( propoff != -FDT_ERR_NOTFOUND ) ? propoff : 0;
+}
+
+static int __init copy_node(void *fdt, void *pfdt, int nodeoff, int depth)
+{
+    int r;
+
+    r = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL));
+    if ( r )
+        return r;
+
+    r = copy_properties(fdt, pfdt, nodeoff);
+    if ( r )
+        return r;
+
+    for ( nodeoff = fdt_first_subnode(pfdt, nodeoff);
+          nodeoff >= 0;
+          nodeoff = fdt_next_subnode(pfdt, nodeoff) ) {
+        r = copy_node(fdt, pfdt, nodeoff, depth + 1);
+        if ( r )
+            return r;
+    }
+
+    if ( nodeoff != -FDT_ERR_NOTFOUND )
+        return nodeoff;
+
+    r = fdt_end_node(fdt);
+    if ( r )
+        return r;
+
+    return 0;
+}
+
 /*
  * 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.
@@ -1740,6 +1794,29 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
             goto err;
     }
 
+    if ( kinfo->dtb_bootmodule ) {
+        int nodeoff, res;
+        void *pfdt;
+
+        pfdt = ioremap_cache(kinfo->dtb_bootmodule->start,
+                             kinfo->dtb_bootmodule->size);
+        if ( pfdt == NULL )
+            return -EFAULT;
+
+        if ( fdt_magic(pfdt) != FDT_MAGIC )
+            return -EINVAL;
+
+        nodeoff = fdt_path_offset(pfdt, "/passthrough");
+        if (nodeoff < 0)
+            return nodeoff;
+
+        res = copy_node(kinfo->fdt, pfdt, nodeoff, 0);
+        if ( res )
+            return res;
+
+        iounmap(pfdt);
+    }
+
     ret = fdt_end_node(kinfo->fdt);
     if ( ret < 0 )
         goto err;
diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h
index 33f3e72..720dec4 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;
-- 
1.9.1


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

[PATCH 2/5] 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.

Device memory is only mapped 1:1. It is not possible to assign devices at
locations that conflict with the DomU memory map.

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

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
 xen/arch/arm/bootfdt.c        |  4 +-
 xen/arch/arm/domain_build.c   | 85 +++++++++++++++++++++++++++++++++++++++++++
 xen/include/xen/device_tree.h |  2 +
 3 files changed, 89 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 891b4b6..72cb8d6 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -55,8 +55,8 @@ 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);
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index cc6b464..d48f77e 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2094,6 +2094,88 @@ static int __init construct_domain(struct domain *d, struct kernel_info *kinfo)
     return 0;
 }
 
+static int __init scan_pt_node(const void *pfdt,
+                               int nodeoff, const char *name, int depth,
+                               u32 address_cells, u32 size_cells,
+                               void *data)
+{
+    int rc;
+    struct dt_device_node *node;
+    int len, i;
+    const struct fdt_property *prop;
+    struct kernel_info *kinfo = data;
+    struct domain *d = kinfo->d;
+    const __be32 *cell;
+
+    if ( depth > 2 )
+        return 0;
+
+    prop = fdt_get_property_namelen(pfdt, nodeoff, "reg", strlen("reg"), &len);
+    if ( prop != NULL )
+    {
+        paddr_t start, size;
+        cell = (const __be32 *)prop->data;
+        len = fdt32_to_cpu(prop->len) /
+              ((address_cells + size_cells) * sizeof (u32));
+
+        for ( i = 0; i < len; i++ )
+        {
+            device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
+
+            rc = guest_physmap_add_entry(d, _gfn(start >> PAGE_SHIFT),
+                                         _mfn(start >> PAGE_SHIFT),
+                                         get_order_from_bytes(size),
+                                         p2m_mmio_direct_dev);
+            if ( rc < 0 )
+            {
+                dprintk(XENLOG_ERR, "Failed to map %"PRIpaddr" to the guest\n", start);
+                return -EFAULT;
+            }
+        }
+    }
+
+    prop = fdt_get_property_namelen(pfdt, nodeoff, "path", strlen("path"), &len);
+    if ( prop != NULL ) {
+        node = dt_find_node_by_path((char *)&prop->data[0]);
+        if ( node != NULL )
+            rc = iommu_assign_dt_device(d, node);
+        else
+            dprintk(XENLOG_ERR, "Couldn't find node %s in host_dt!\n",
+                    (char *)&prop->data[0]);
+    }
+
+    prop = fdt_get_property_namelen(pfdt, nodeoff, "interrupts", strlen("interrupts"), &len);
+    if ( prop != NULL )
+    {
+        int pt_irq;
+        u32 *u = (u32*) &prop->data[0];
+
+        pt_irq = fdt32_to_cpu(*(u + 1)) + 32;
+
+        vgic_reserve_virq(d, pt_irq);
+        rc = route_irq_to_guest(d, pt_irq, pt_irq, "routed IRQ");
+        if ( rc < 0 )
+            return rc;
+    }
+    return 0;
+}
+
+static int __init domain_addign_devices(struct domain *d,
+                                        struct kernel_info *kinfo)
+{
+    void *pfdt;
+
+    pfdt = ioremap_cache(kinfo->dtb_bootmodule->start,
+            kinfo->dtb_bootmodule->size);
+    if ( pfdt == NULL )
+        return -EFAULT;
+
+    device_tree_for_each_node(pfdt, scan_pt_node, kinfo);
+
+    iounmap(pfdt);
+    return 0;
+}
+
 static int __init construct_domU(struct domain *d,
                                  const struct dt_device_node *node)
 {
@@ -2140,6 +2222,9 @@ static int __init construct_domU(struct domain *d,
     if ( kinfo.vpl011 )
         rc = domain_vpl011_init(d, NULL);
 
+    if ( kinfo.dtb_bootmodule )
+        rc = domain_addign_devices(d, &kinfo);
+
     return rc;
 }
 
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 7408a6c..356a422 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -161,6 +161,8 @@ extern const void *device_tree_flattened;
 int device_tree_for_each_node(const void *fdt,
                                      device_tree_node_func func,
                                      void *data);
+void device_tree_get_reg(const __be32 **cell, u32 address_cells,
+                         u32 size_cells, u64 *start, u64 *size);
 
 /**
  * dt_unflatten_host_device_tree - Unflatten the host device tree
-- 
1.9.1


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

[PATCH 3/5] xen/arm: handle "multiboot, dtb" compatible nodes

[Stefano Stabellini]

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

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
 xen/arch/arm/bootfdt.c      |  2 ++
 xen/arch/arm/kernel.c       | 12 +++++++++++-
 xen/arch/arm/setup.c        |  1 +
 xen/include/asm-arm/setup.h |  1 +
 4 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 72cb8d6..ad4fbac 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -205,6 +205,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,dtb") == 0 )
+        kind = BOOTMOD_DTB;
     else
         kind = BOOTMOD_UNKNOWN;
 
diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index d04a862..8918d75 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -426,7 +426,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 = 0, initrd_addr = 0, dtb_addr = 0, size;
     int rc;
 
     /* domain is NULL only for the hardware domain */
@@ -470,6 +470,16 @@ 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,dtb") )
+            {
+                u32 len;
+                const __be32 *val;
+
+                val = dt_get_property(node, "reg", &len);
+                dt_get_range(&val, node, &dtb_addr, &size);
+                info->dtb_bootmodule = boot_module_find_by_addr_and_kind(
+                        BOOTMOD_DTB, dtb_addr);
+            }
             else
                 continue;
         }
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index e83221a..b3de0a2 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -344,6 +344,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_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 48187e1..0ff0768 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_DTB,
     BOOTMOD_UNKNOWN
 }  bootmodule_kind;
 
-- 
1.9.1


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

[PATCH 4/5] xen/arm: use the physical number of gic lines for boot domains

[Stefano Stabellini]

We don't have a clear way to know how many virtual SPIs we need for the
boot domains. For simplicity, allocate as many as natively supported,
just like for dom0.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
 xen/arch/arm/domain_build.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d48f77e..529b674 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2239,7 +2239,7 @@ 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,
+            .arch.nr_spis = gic_number_lines() - 32,
             .flags = XEN_DOMCTL_CDF_hvm_guest | XEN_DOMCTL_CDF_hap,
             .max_vcpus = 1,
             .max_evtchn_port = -1,
@@ -2250,8 +2250,6 @@ 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;
         dt_property_read_u32(node, "cpus", &d_cfg.max_vcpus);
 
         d = domain_create(++max_init_domid, &d_cfg, false);
-- 
1.9.1


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

[PATCH 5/5] xen/arm: add dom0less device assignment info to docs

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
 docs/misc/arm/device-tree/booting.txt | 108 ++++++++++++++++++++++++++++++++++
 1 file changed, 108 insertions(+)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 317a9e9..f5aaf8f 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -226,3 +226,111 @@ 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,dtb"
+
+- reg
+
+    Specifies the physical address of the device tree binary fragment
+    RAM and its length.
+
+As an example:
+
+        module@0xc000000 {
+            compatible = "multiboot,dtb", "multiboot,module";
+            reg = <0x0 0xc000000 0xffffff>;
+        };
+
+The DTB fragment (loaded in memory at 0xc000000 in the example above)
+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.
+
+In addition, the following properties for each device node in the device
+tree fragment will be used for the device assignment setup:
+
+- reg
+
+  The reg property specifying the address and size of the device memory.
+  The device memory will be automatically mapped to the guest domain
+  with a 1:1 mapping (pseudo-physical address == physical address).
+
+- interrupts
+
+  The interrupts property specifies the interrupt of the device. They
+  are automatically routed to the guest domain with virtual irqs ==
+  physical irqs.
+
+- interrupt-parent
+
+  It contains a reference to the interrupt controller node. It should be
+  65000, corresponding to GUEST_PHANDLE_GIC.
+
+- path
+
+  A new string property named "path" holds the path in the host device
+  tree to the corresponding device node.
+
+The following is a real-world example of a device tree fragment for the
+network card on Xilinx MPSoC boards:
+
+/dts-v1/;
+
+/ {
+    #address-cells = <0x2>;
+    #size-cells = <0x1>;
+
+    passthrough {
+        compatible = "simple-bus";
+        ranges;
+        #address-cells = <0x2>;
+        #size-cells = <0x1>;
+
+        misc_clk {
+            #clock-cells = <0x0>;
+            clock-frequency = <0x7735940>;
+            compatible = "fixed-clock";
+            linux,phandle = <0x1>;
+            phandle = <0x1>;
+        };
+
+        ethernet@ff0e0000 {
+            compatible = "cdns,zynqmp-gem";
+            status = "okay";
+            interrupt-parent = <0xfde8>;
+            interrupts = <0x0 0x3f 0x4 0x0 0x3f 0x4>;
+            reg = <0x0 0xff0e0000 0x1000>;
+            clock-names = "pclk", "hclk", "tx_clk", "rx_clk";
+            #address-cells = <0x1>;
+            #size-cells = <0x0>;
+            clocks = <0x1 0x1 0x1 0x1>;
+            phy-mode = "rgmii-id";
+            xlnx,ptp-enet-clock = <0x0>;
+            local-mac-address = [00 0a 35 00 22 01];
+            phy-handle = <0x2>;
+            path = "/amba/ethernet@ff0e0000";
+
+            phy@c {
+                reg = <0xc>;
+                ti,rx-internal-delay = <0x8>;
+                ti,tx-internal-delay = <0xa>;
+                ti,fifo-depth = <0x1>;
+                ti,rxctrl-strap-worka;
+                linux,phandle = <0x2>;
+                phandle = <0x2>;
+            };
+        };
+    };
+};
-- 
1.9.1


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

Re: [PATCH 1/5] xen/arm: copy dtb fragment to guest dtb

[Julien Grall]

(+ Wei and Ian)

Hi Stefano,

On 12/5/18 5:28 PM, Stefano Stabellini wrote:
> Read the dtb fragment corresponding to a passthrough device from memory
> at the location referred to by the "multiboot,dtb" compatible node.
> 
> Copy the fragment to the guest dtb.
> 
> Add a dtb_bootmodule field to struct kernel_info to find the dtb
> fragment for a guest.

The code below is basically a copy from libxl, right? If so, this should 
be specified in the commit message.

Also, the license is different in libxl compare to the hypervisor 
(LGPLv2.1 vs GPLv2). So is there any issue to copy that code in the 
hypervisor?

> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
>   xen/arch/arm/domain_build.c  | 77 ++++++++++++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/kernel.h |  2 +-
>   2 files changed, 78 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index b0ec3f0..cc6b464 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>
> @@ -1669,6 +1670,59 @@ static int __init make_vpl011_uart_node(const struct domain *d, void *fdt)
>   }
>   #endif
>   
> +static int __init copy_properties(void *fdt, void *pfdt, int nodeoff)
> +{
> +    int propoff, nameoff, r;
> +    const struct fdt_property *prop;
> +
> +    for ( propoff = fdt_first_property_offset(pfdt, nodeoff);
> +          propoff >= 0;
> +          propoff = fdt_next_property_offset(pfdt, propoff) ) {

Coding style:

for ( ... )
{

> +
> +        if ( !(prop = fdt_get_property_by_offset(pfdt, propoff, NULL)) )
> +            return -FDT_ERR_INTERNAL;
> +
> +        nameoff = fdt32_to_cpu(prop->nameoff);
> +        r = fdt_property(fdt, fdt_string(pfdt, nameoff),
> +                         prop->data, fdt32_to_cpu(prop->len));
> +        if ( r )
> +            return r;
> +    }
> +
> +    /* FDT_ERR_NOTFOUND => There is no more properties for this node */
> +    return ( propoff != -FDT_ERR_NOTFOUND ) ? propoff : 0;
> +}
> +
> +static int __init copy_node(void *fdt, void *pfdt, int nodeoff, int depth)
> +{
> +    int r;
> +
> +    r = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL));
> +    if ( r )
> +        return r;
> +
> +    r = copy_properties(fdt, pfdt, nodeoff);
> +    if ( r )
> +        return r;
> +
> +    for ( nodeoff = fdt_first_subnode(pfdt, nodeoff);
> +          nodeoff >= 0;
> +          nodeoff = fdt_next_subnode(pfdt, nodeoff) ) {

Same here.

> +        r = copy_node(fdt, pfdt, nodeoff, depth + 1);
> +        if ( r )
> +            return r;
> +    }
> +
> +    if ( nodeoff != -FDT_ERR_NOTFOUND )
> +        return nodeoff;
> +
> +    r = fdt_end_node(fdt);
> +    if ( r )
> +        return r;
> +
> +    return 0;
> +}
> +
>   /*
>    * 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.
> @@ -1740,6 +1794,29 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>               goto err;
>       }
>   
> +    if ( kinfo->dtb_bootmodule ) {
> +        int nodeoff, res;
> +        void *pfdt;
> +
> +        pfdt = ioremap_cache(kinfo->dtb_bootmodule->start,
> +                             kinfo->dtb_bootmodule->size);

This suggests the DTB fragment should be cacheable. Can this be written 
in the documentation?

> +        if ( pfdt == NULL )
> +            return -EFAULT;
> +
> +        if ( fdt_magic(pfdt) != FDT_MAGIC )
> +            return -EINVAL;
> +
> +        nodeoff = fdt_path_offset(pfdt, "/passthrough");
> +        if (nodeoff < 0)
> +            return nodeoff;
> +
> +        res = copy_node(kinfo->fdt, pfdt, nodeoff, 0);
> +        if ( res )
> +            return res;

I would also copy /aliases as it may be used by the users to refer 
easily a node.

> +
> +        iounmap(pfdt);
> +    }
> +
>       ret = fdt_end_node(kinfo->fdt);
>       if ( ret < 0 )
>           goto err;
> diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h
> index 33f3e72..720dec4 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: [PATCH 2/5] xen/arm: assign devices to boot domains

[Julien Grall]

Hi Stefano,

On 12/5/18 5:28 PM, 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.
> 
> Device memory is only mapped 1:1. It is not possible to assign devices at
> locations that conflict with the DomU memory map.
I think 1:1 mapped is a pretty bad idea. You limit yourself a lot as the 
user does not control neither the HW nor the guest memory map.

So you need to provide a way for the user to specify the mapping.

> 
> The iommu is setup by passing the to the device to assign on the host

NIT: "the node to..."

> device tree. The path is specified in the device tree fragment as the
> "path" string property.

Path is too generic and may clash in the future with other bindings. 
Please add "xen," in front.

> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
>   xen/arch/arm/bootfdt.c        |  4 +-
>   xen/arch/arm/domain_build.c   | 85 +++++++++++++++++++++++++++++++++++++++++++
>   xen/include/xen/device_tree.h |  2 +
>   3 files changed, 89 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 891b4b6..72cb8d6 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -55,8 +55,8 @@ 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);
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index cc6b464..d48f77e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2094,6 +2094,88 @@ static int __init construct_domain(struct domain *d, struct kernel_info *kinfo)
>       return 0;
>   }
>   
> +static int __init scan_pt_node(const void *pfdt,
> +                               int nodeoff, const char *name, int depth,
> +                               u32 address_cells, u32 size_cells,
> +                               void *data)

Is it really necessary to parse the device-tree twice?

> +{
> +    int rc;
> +    struct dt_device_node *node;
> +    int len, i;
> +    const struct fdt_property *prop;
> +    struct kernel_info *kinfo = data;
> +    struct domain *d = kinfo->d;
> +    const __be32 *cell;
> +
> +    if ( depth > 2 )
> +        return 0;

Why do you limit yourself to depth 2? It is possible to have nested node 
describing memory.

> +
> +    prop = fdt_get_property_namelen(pfdt, nodeoff, "reg", strlen("reg"), &len);
> +    if ( prop != NULL )
> +    {
> +        paddr_t start, size;
> +        cell = (const __be32 *)prop->data;
> +        len = fdt32_to_cpu(prop->len) /
> +              ((address_cells + size_cells) * sizeof (u32));
> +
> +        for ( i = 0; i < len; i++ )
> +        {
> +            device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);

Here you assume the value in regs correspond to host physical address. 
This may not be the case if a device is under a bus.

> +
> +            rc = guest_physmap_add_entry(d, _gfn(start >> PAGE_SHIFT),

Please use gaddr_to_gfn and ...

> +                                         _mfn(start >> PAGE_SHIFT),

... maddr_to_mfn

> +                                         get_order_from_bytes(size),
> +                                         p2m_mmio_direct_dev);

I think the restriction on the memory attributes should be documented in 
patch #5.

> +            if ( rc < 0 )
> +            {
> +                dprintk(XENLOG_ERR, "Failed to map %"PRIpaddr" to the guest\n", start);
> +                return -EFAULT;
> +            }
> +        }
> +    }
> +
> +    prop = fdt_get_property_namelen(pfdt, nodeoff, "path", strlen("path"), &len);
> +    if ( prop != NULL ) {
> +        node = dt_find_node_by_path((char *)&prop->data[0]);

What's wrong with giving directly prop->data?

> +        if ( node != NULL )
> +            rc = iommu_assign_dt_device(d, node);
> +        else
> +            dprintk(XENLOG_ERR, "Couldn't find node %s in host_dt!\n",
> +                    (char *)&prop->data[0]);

Same here.

> +    }
> +
> +    prop = fdt_get_property_namelen(pfdt, nodeoff, "interrupts", strlen("interrupts"), &len);
> +    if ( prop != NULL )
> +    {
> +        int pt_irq;
> +        u32 *u = (u32*) &prop->data[0];

Same here for &prop->data[0]. But this stores a fdt_32 and not u32.

> +
> +        pt_irq = fdt32_to_cpu(*(u + 1)) + 32;
> +
> +        vgic_reserve_virq(d, pt_irq);
> +        rc = route_irq_to_guest(d, pt_irq, pt_irq, "routed IRQ");
> +        if ( rc < 0 )
> +            return rc;

You are assuming the device can only generate one interrupt. 
Furthermore, this is assuming all the nodes in the fragment will be 
under the GIC controller. You may want to passthrough a interrupt 
controller (i.e GPIO) to the guest and the related device.

> +    }

NIT: newline here.

> +    return 0;
> +}
> +
> +static int __init domain_addign_devices(struct domain *d,

s/addign/adding/

> +                                        struct kernel_info *kinfo)
> +{
> +    void *pfdt;
> +
> +    pfdt = ioremap_cache(kinfo->dtb_bootmodule->start,
> +            kinfo->dtb_bootmodule->size);
> +    if ( pfdt == NULL )
> +        return -EFAULT;
> +
> +    device_tree_for_each_node(pfdt, scan_pt_node, kinfo);
> +
> +    iounmap(pfdt);
> +    return 0;
> +}
> +
>   static int __init construct_domU(struct domain *d,
>                                    const struct dt_device_node *node)
>   {
> @@ -2140,6 +2222,9 @@ static int __init construct_domU(struct domain *d,
>       if ( kinfo.vpl011 )
>           rc = domain_vpl011_init(d, NULL);
>   
> +    if ( kinfo.dtb_bootmodule )
> +        rc = domain_addign_devices(d, &kinfo);
> +
>       return rc;
>   }
>   
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 7408a6c..356a422 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -161,6 +161,8 @@ extern const void *device_tree_flattened;
>   int device_tree_for_each_node(const void *fdt,
>                                        device_tree_node_func func,
>                                        void *data);
> +void device_tree_get_reg(const __be32 **cell, u32 address_cells,
> +                         u32 size_cells, u64 *start, u64 *size);
>   
>   /**
>    * dt_unflatten_host_device_tree - Unflatten the host device tree
> 

Cheers,

-- 
Julien Grall

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

Re: [PATCH 3/5] xen/arm: handle "multiboot, dtb" compatible nodes

[Julien Grall]

Hi,

On 12/5/18 5:28 PM, Stefano Stabellini wrote:
> Detect "multiboot,dtb" compatible nodes. Add them to the bootmod array
> as BOOTMOD_DTB.  In kernel_probe, find the right BOOTMOD_DTB and store a
> pointer to it in dtb_bootmodule.

This is too close to the name BOOTMOD_FDT. So this will confuse more 
than one developer.

This probably want to be renamed to BOOTMOD_GUEST_DTB.

> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
>   xen/arch/arm/bootfdt.c      |  2 ++
>   xen/arch/arm/kernel.c       | 12 +++++++++++-
>   xen/arch/arm/setup.c        |  1 +
>   xen/include/asm-arm/setup.h |  1 +
>   4 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 72cb8d6..ad4fbac 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -205,6 +205,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,dtb") == 0 )

I would prefer an explicit name such as "multiboot,device-tree".

> +        kind = BOOTMOD_DTB;
>       else
>           kind = BOOTMOD_UNKNOWN;
>   
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index d04a862..8918d75 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -426,7 +426,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 = 0, initrd_addr = 0, dtb_addr = 0, size;
>       int rc;
>   
>       /* domain is NULL only for the hardware domain */
> @@ -470,6 +470,16 @@ 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,dtb") )
> +            {
> +                u32 len;
> +                const __be32 *val;
> +
> +                val = dt_get_property(node, "reg", &len);
> +                dt_get_range(&val, node, &dtb_addr, &size);
> +                info->dtb_bootmodule = boot_module_find_by_addr_and_kind(
> +                        BOOTMOD_DTB, dtb_addr);
> +            }
>               else
>                   continue;
>           }
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index e83221a..b3de0a2 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -344,6 +344,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_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 48187e1..0ff0768 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_DTB,
>       BOOTMOD_UNKNOWN
>   }  bootmodule_kind;
>   
> 

Cheers,

-- 
Julien Grall

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

Re: [PATCH 4/5] xen/arm: use the physical number of gic lines for boot domains

[Julien Grall]

Hi,

On 12/5/18 5:28 PM, Stefano Stabellini wrote:
> We don't have a clear way to know how many virtual SPIs we need for the
> boot domains. For simplicity, allocate as many as natively supported,
> just like for dom0.

This will potentially allocate a lot of unused interrupts and a waste of 
memory. So is it the correct solution?

For instance, we would request the user to provide the number of interrupts.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 5/5] xen/arm: add dom0less device assignment info to docs

[Julien Grall]

Hi Stefano,

On 12/5/18 5:28 PM, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
>   docs/misc/arm/device-tree/booting.txt | 108 ++++++++++++++++++++++++++++++++++
>   1 file changed, 108 insertions(+)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 317a9e9..f5aaf8f 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -226,3 +226,111 @@ 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,dtb"

I would prefer "multiboot,device-tree"

> +
> +- reg
> +
> +    Specifies the physical address of the device tree binary fragment
> +    RAM and its length.
> +
> +As an example:
> +
> +        module@0xc000000 {
> +            compatible = "multiboot,dtb", "multiboot,module";
> +            reg = <0x0 0xc000000 0xffffff>;
> +        };
> +
> +The DTB fragment (loaded in memory at 0xc000000 in the example above)
> +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.
> +
> +In addition, the following properties for each device node in the device
> +tree fragment will be used for the device assignment setup:
> +
> +- reg
> +
> +  The reg property specifying the address and size of the device memory.
> +  The device memory will be automatically mapped to the guest domain
> +  with a 1:1 mapping (pseudo-physical address == physical address).

As said in a previous patch, I don't think this is correct to impose 
1:1. The user is neither in control of the HW memory map nor the Guest 
memory map. So not many people are going to be able to use it without 
hacking Xen.

> +
> +- interrupts
> +
> +  The interrupts property specifies the interrupt of the device. They
> +  are automatically routed to the guest domain with virtual irqs ==
> +  physical irqs.
> +
> +- interrupt-parent
> +
> +  It contains a reference to the interrupt controller node. It should be
> +  65000, corresponding to GUEST_PHANDLE_GIC.

We managed to get away in the toolstack with this property. So why do 
you need it for the hypervisor? Furthermore, this would forbid to 
passthrough any other interrupt controller to the guest.

> +
> +- path
> +
> +  A new string property named "path" holds the path in the host device
> +  tree to the corresponding device node.

This name is too generic. Please prepend "xen,"

> +
> +The following is a real-world example of a device tree fragment for the
> +network card on Xilinx MPSoC boards:
> +
> +/dts-v1/;
> +
> +/ {
> +    #address-cells = <0x2>;
> +    #size-cells = <0x1>;
> +
> +    passthrough {
> +        compatible = "simple-bus";
> +        ranges;
> +        #address-cells = <0x2>;
> +        #size-cells = <0x1>;
> +
> +        misc_clk {
> +            #clock-cells = <0x0>;
> +            clock-frequency = <0x7735940>;
> +            compatible = "fixed-clock";
> +            linux,phandle = <0x1>;
> +            phandle = <0x1>;
> +        };
> +
> +        ethernet@ff0e0000 {
> +            compatible = "cdns,zynqmp-gem";
> +            status = "okay";
> +            interrupt-parent = <0xfde8>;
> +            interrupts = <0x0 0x3f 0x4 0x0 0x3f 0x4>;
> +            reg = <0x0 0xff0e0000 0x1000>;
> +            clock-names = "pclk", "hclk", "tx_clk", "rx_clk";
> +            #address-cells = <0x1>;
> +            #size-cells = <0x0>;
> +            clocks = <0x1 0x1 0x1 0x1>;
> +            phy-mode = "rgmii-id";
> +            xlnx,ptp-enet-clock = <0x0>;
> +            local-mac-address = [00 0a 35 00 22 01];
> +            phy-handle = <0x2>;
> +            path = "/amba/ethernet@ff0e0000";
> +
> +            phy@c {
> +                reg = <0xc>;
> +                ti,rx-internal-delay = <0x8>;
> +                ti,tx-internal-delay = <0xa>;
> +                ti,fifo-depth = <0x1>;
> +                ti,rxctrl-strap-worka;
> +                linux,phandle = <0x2>;
> +                phandle = <0x2>;
> +            };
> +        };
> +    };
> +};
> 

Cheers,

-- 
Julien Grall

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

Re: [PATCH 1/5] xen/arm: copy dtb fragment to guest dtb

[Wei Liu]

On Mon, Dec 24, 2018 at 11:17:57AM +0000, Julien Grall wrote:
> (+ Wei and Ian)
> 
> Hi Stefano,
> 
> On 12/5/18 5:28 PM, Stefano Stabellini wrote:
> > Read the dtb fragment corresponding to a passthrough device from memory
> > at the location referred to by the "multiboot,dtb" compatible node.
> > 
> > Copy the fragment to the guest dtb.
> > 
> > Add a dtb_bootmodule field to struct kernel_info to find the dtb
> > fragment for a guest.
> 
> The code below is basically a copy from libxl, right? If so, this should be
> specified in the commit message.
> 
> Also, the license is different in libxl compare to the hypervisor (LGPLv2.1
> vs GPLv2). So is there any issue to copy that code in the hypervisor?

https://www.gnu.org/licenses/gpl-faq.html#AllCompatibility

It appears to be okay -- I was looking at "I want to copy code under:
LGPL v2.1 only" and "I want to license my code under: GPLv2 only". The
resulting license for the copied code is GPLv2.

I have CC'ed Lars and committers@ for more opinions.

Wei.

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

Re: [PATCH 1/5] xen/arm: copy dtb fragment to guest dtb

[Stefano Stabellini]

On Mon, 24 Dec 2018, Julien Grall wrote:
> (+ Wei and Ian)
> 
> Hi Stefano,
> 
> On 12/5/18 5:28 PM, Stefano Stabellini wrote:
> > Read the dtb fragment corresponding to a passthrough device from memory
> > at the location referred to by the "multiboot,dtb" compatible node.
> > 
> > Copy the fragment to the guest dtb.
> > 
> > Add a dtb_bootmodule field to struct kernel_info to find the dtb
> > fragment for a guest.
> 
> The code below is basically a copy from libxl, right? If so, this should be
> specified in the commit message.
> 
> Also, the license is different in libxl compare to the hypervisor (LGPLv2.1 vs
> GPLv2). So is there any issue to copy that code in the hypervisor?

Wei confirmed that it is OK. I'll add a note about this in the commit
message.


> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> >   xen/arch/arm/domain_build.c  | 77
> > ++++++++++++++++++++++++++++++++++++++++++++
> >   xen/include/asm-arm/kernel.h |  2 +-
> >   2 files changed, 78 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index b0ec3f0..cc6b464 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>
> > @@ -1669,6 +1670,59 @@ static int __init make_vpl011_uart_node(const struct
> > domain *d, void *fdt)
> >   }
> >   #endif
> >   +static int __init copy_properties(void *fdt, void *pfdt, int nodeoff)
> > +{
> > +    int propoff, nameoff, r;
> > +    const struct fdt_property *prop;
> > +
> > +    for ( propoff = fdt_first_property_offset(pfdt, nodeoff);
> > +          propoff >= 0;
> > +          propoff = fdt_next_property_offset(pfdt, propoff) ) {
> 
> Coding style:
> 
> for ( ... )
> {

OK


> > +
> > +        if ( !(prop = fdt_get_property_by_offset(pfdt, propoff, NULL)) )
> > +            return -FDT_ERR_INTERNAL;
> > +
> > +        nameoff = fdt32_to_cpu(prop->nameoff);
> > +        r = fdt_property(fdt, fdt_string(pfdt, nameoff),
> > +                         prop->data, fdt32_to_cpu(prop->len));
> > +        if ( r )
> > +            return r;
> > +    }
> > +
> > +    /* FDT_ERR_NOTFOUND => There is no more properties for this node */
> > +    return ( propoff != -FDT_ERR_NOTFOUND ) ? propoff : 0;
> > +}
> > +
> > +static int __init copy_node(void *fdt, void *pfdt, int nodeoff, int depth)
> > +{
> > +    int r;
> > +
> > +    r = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL));
> > +    if ( r )
> > +        return r;
> > +
> > +    r = copy_properties(fdt, pfdt, nodeoff);
> > +    if ( r )
> > +        return r;
> > +
> > +    for ( nodeoff = fdt_first_subnode(pfdt, nodeoff);
> > +          nodeoff >= 0;
> > +          nodeoff = fdt_next_subnode(pfdt, nodeoff) ) {
> 
> Same here.

OK


> > +        r = copy_node(fdt, pfdt, nodeoff, depth + 1);
> > +        if ( r )
> > +            return r;
> > +    }
> > +
> > +    if ( nodeoff != -FDT_ERR_NOTFOUND )
> > +        return nodeoff;
> > +
> > +    r = fdt_end_node(fdt);
> > +    if ( r )
> > +        return r;
> > +
> > +    return 0;
> > +}
> > +
> >   /*
> >    * 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.
> > @@ -1740,6 +1794,29 @@ static int __init prepare_dtb_domU(struct domain *d,
> > struct kernel_info *kinfo)
> >               goto err;
> >       }
> >   +    if ( kinfo->dtb_bootmodule ) {
> > +        int nodeoff, res;
> > +        void *pfdt;
> > +
> > +        pfdt = ioremap_cache(kinfo->dtb_bootmodule->start,
> > +                             kinfo->dtb_bootmodule->size);
> 
> This suggests the DTB fragment should be cacheable. Can this be written in the
> documentation?

OK


> > +        if ( pfdt == NULL )
> > +            return -EFAULT;
> > +
> > +        if ( fdt_magic(pfdt) != FDT_MAGIC )
> > +            return -EINVAL;
> > +
> > +        nodeoff = fdt_path_offset(pfdt, "/passthrough");
> > +        if (nodeoff < 0)
> > +            return nodeoff;
> > +
> > +        res = copy_node(kinfo->fdt, pfdt, nodeoff, 0);
> > +        if ( res )
> > +            return res;
> 
> I would also copy /aliases as it may be used by the users to refer easily a
> node.

OK


> > +
> > +        iounmap(pfdt);
> > +    }
> > +
> >       ret = fdt_end_node(kinfo->fdt);
> >       if ( ret < 0 )
> >           goto err;
> > diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h
> > index 33f3e72..720dec4 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: [PATCH 3/5] xen/arm: handle "multiboot, dtb" compatible nodes

[Stefano Stabellini]

On Mon, 24 Dec 2018, Julien Grall wrote:
> Hi,
> 
> On 12/5/18 5:28 PM, Stefano Stabellini wrote:
> > Detect "multiboot,dtb" compatible nodes. Add them to the bootmod array
> > as BOOTMOD_DTB.  In kernel_probe, find the right BOOTMOD_DTB and store a
> > pointer to it in dtb_bootmodule.
> 
> This is too close to the name BOOTMOD_FDT. So this will confuse more than one
> developer.
> 
> This probably want to be renamed to BOOTMOD_GUEST_DTB.

OK


> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> >   xen/arch/arm/bootfdt.c      |  2 ++
> >   xen/arch/arm/kernel.c       | 12 +++++++++++-
> >   xen/arch/arm/setup.c        |  1 +
> >   xen/include/asm-arm/setup.h |  1 +
> >   4 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index 72cb8d6..ad4fbac 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -205,6 +205,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,dtb") == 0 )
> 
> I would prefer an explicit name such as "multiboot,device-tree".

OK


> > +        kind = BOOTMOD_DTB;
> >       else
> >           kind = BOOTMOD_UNKNOWN;
> >   diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> > index d04a862..8918d75 100644
> > --- a/xen/arch/arm/kernel.c
> > +++ b/xen/arch/arm/kernel.c
> > @@ -426,7 +426,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 = 0, initrd_addr = 0, dtb_addr = 0, size;
> >       int rc;
> >         /* domain is NULL only for the hardware domain */
> > @@ -470,6 +470,16 @@ 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,dtb") )
> > +            {
> > +                u32 len;
> > +                const __be32 *val;
> > +
> > +                val = dt_get_property(node, "reg", &len);
> > +                dt_get_range(&val, node, &dtb_addr, &size);
> > +                info->dtb_bootmodule = boot_module_find_by_addr_and_kind(
> > +                        BOOTMOD_DTB, dtb_addr);
> > +            }
> >               else
> >                   continue;
> >           }
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index e83221a..b3de0a2 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -344,6 +344,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_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 48187e1..0ff0768 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_DTB,
> >       BOOTMOD_UNKNOWN
> >   }  bootmodule_kind;
> >   
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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

Re: [PATCH 4/5] xen/arm: use the physical number of gic lines for boot domains

[Stefano Stabellini]

On Mon, 24 Dec 2018, Julien Grall wrote:
> Hi,
> 
> On 12/5/18 5:28 PM, Stefano Stabellini wrote:
> > We don't have a clear way to know how many virtual SPIs we need for the
> > boot domains. For simplicity, allocate as many as natively supported,
> > just like for dom0.
> 
> This will potentially allocate a lot of unused interrupts and a waste of
> memory. So is it the correct solution?
> 
> For instance, we would request the user to provide the number of interrupts.

Unfortunately, this has to happen much earlier than when we parse user
supplied device tree options. We could make it an hypervisor command
line parameter but it would be nasty.

Given that this feature is aimed at partitioning scenarios where the
number of VMs is low, the increased memory overhead is not a big issue.

(If you recall we briefly discussed this topic at the last Linaro
Connect, this was the conclusion.)

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

Re: [PATCH 2/5] xen/arm: assign devices to boot domains

[Stefano Stabellini]

On Mon, 24 Dec 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 12/5/18 5:28 PM, 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.
> > 
> > Device memory is only mapped 1:1. It is not possible to assign devices at
> > locations that conflict with the DomU memory map.
> I think 1:1 mapped is a pretty bad idea. You limit yourself a lot as the user
> does not control neither the HW nor the guest memory map.
> 
> So you need to provide a way for the user to specify the mapping.

Yes, I think we want to introduce a separate xen,reg property that
includes a destination address as well. Something of the format:

  xen,reg = <source_addr source_size dest_addr>

More on this below.


> > 
> > The iommu is setup by passing the to the device to assign on the host
> 
> NIT: "the node to..."

I'll fix

 
> > device tree. The path is specified in the device tree fragment as the
> > "path" string property.
> 
> Path is too generic and may clash in the future with other bindings. Please
> add "xen," in front.

OK, I can rename


> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> >   xen/arch/arm/bootfdt.c        |  4 +-
> >   xen/arch/arm/domain_build.c   | 85
> > +++++++++++++++++++++++++++++++++++++++++++
> >   xen/include/xen/device_tree.h |  2 +
> >   3 files changed, 89 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index 891b4b6..72cb8d6 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -55,8 +55,8 @@ 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);
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index cc6b464..d48f77e 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2094,6 +2094,88 @@ static int __init construct_domain(struct domain *d,
> > struct kernel_info *kinfo)
> >       return 0;
> >   }
> >   +static int __init scan_pt_node(const void *pfdt,
> > +                               int nodeoff, const char *name, int depth,
> > +                               u32 address_cells, u32 size_cells,
> > +                               void *data)
> 
> Is it really necessary to parse the device-tree twice?

I don't think I understand this comment. The device tree fragment is not
scanned twice, just once I think. Am I missing something?


> > +{
> > +    int rc;
> > +    struct dt_device_node *node;
> > +    int len, i;
> > +    const struct fdt_property *prop;
> > +    struct kernel_info *kinfo = data;
> > +    struct domain *d = kinfo->d;
> > +    const __be32 *cell;
> > +
> > +    if ( depth > 2 )
> > +        return 0;
> 
> Why do you limit yourself to depth 2? It is possible to have nested node
> describing memory

That was because Xen needed a far better understanding of how reg works
to be able to parse nested nodes correctly. Specifically, see this
concrete example from ZynqMP:

        ethernet@ff0e0000 {
            compatible = "cdns,zynqmp-gem";
            status = "okay";
            interrupt-parent = <0xfde8>;
            interrupts = <0x0 0x3f 0x4 0x0 0x3f 0x4>;
            reg = <0x0 0xff0e0000 0x1000>;
            clock-names = "pclk", "hclk", "tx_clk", "rx_clk";
            #address-cells = <0x1>;
            #size-cells = <0x0>;
            clocks = <0x1 0x1 0x1 0x1>;
            phy-mode = "rgmii-id";
            xlnx,ptp-enet-clock = <0x0>;
            local-mac-address = [00 0a 35 00 22 01];
            phy-handle = <0x2>;
            xen,path = "/amba/ethernet@ff0e0000";

            phy@c {
                reg = <0xc>;
                ti,rx-internal-delay = <0x8>;
                ti,tx-internal-delay = <0xa>;
                ti,fifo-depth = <0x1>;
                ti,rxctrl-strap-worka;
                linux,phandle = <0x2>;
                phandle = <0x2>;
            };
        };

We want to map the first reg property under ethernet@ff0e0000, not the
reg property under phy@c, which it doesn't even refer to a memory
region. This problem goes away completely if we start using a new Xen
specific "xen,reg" property, instead of trying to figure out which
existing reg to map. With that, we can safely remove the artificial
depth <= 2 restriction.


> > +
> > +    prop = fdt_get_property_namelen(pfdt, nodeoff, "reg", strlen("reg"),
> > &len);
> > +    if ( prop != NULL )
> > +    {
> > +        paddr_t start, size;
> > +        cell = (const __be32 *)prop->data;
> > +        len = fdt32_to_cpu(prop->len) /
> > +              ((address_cells + size_cells) * sizeof (u32));
> > +
> > +        for ( i = 0; i < len; i++ )
> > +        {
> > +            device_tree_get_reg(&cell, address_cells, size_cells, &start,
> > &size);
> 
> Here you assume the value in regs correspond to host physical address. This
> may not be the case if a device is under a bus.

Yes, one more reason to move away from parsing the existing reg property
and use a new xen,reg property that gives directly the host physical
address to map to the right guest physical address (regardless of
busses). xen,reg would basically be equivalent to the iomem VM config
file option.


> > +
> > +            rc = guest_physmap_add_entry(d, _gfn(start >> PAGE_SHIFT),
> 
> Please use gaddr_to_gfn and ...

OK


> > +                                         _mfn(start >> PAGE_SHIFT),
> 
> ... maddr_to_mfn

OK


> > +                                         get_order_from_bytes(size),
> > +                                         p2m_mmio_direct_dev);
> 
> I think the restriction on the memory attributes should be documented in patch
> #5.

OK


> > +            if ( rc < 0 )
> > +            {
> > +                dprintk(XENLOG_ERR, "Failed to map %"PRIpaddr" to the
> > guest\n", start);
> > +                return -EFAULT;
> > +            }
> > +        }
> > +    }
> > +
> > +    prop = fdt_get_property_namelen(pfdt, nodeoff, "path", strlen("path"),
> > &len);
> > +    if ( prop != NULL ) {
> > +        node = dt_find_node_by_path((char *)&prop->data[0]);
> 
> What's wrong with giving directly prop->data?

Nothing wrong with it, just a matter of code style. I don't care about
this, I can change it.


> > +        if ( node != NULL )
> > +            rc = iommu_assign_dt_device(d, node);
> > +        else
> > +            dprintk(XENLOG_ERR, "Couldn't find node %s in host_dt!\n",
> > +                    (char *)&prop->data[0]);
> 
> Same here.

Same


> > +    }
> > +
> > +    prop = fdt_get_property_namelen(pfdt, nodeoff, "interrupts",
> > strlen("interrupts"), &len);
> > +    if ( prop != NULL )
> > +    {
> > +        int pt_irq;
> > +        u32 *u = (u32*) &prop->data[0];
> 
> Same here for &prop->data[0]. But this stores a fdt_32 and not u32.

OK


> > +
> > +        pt_irq = fdt32_to_cpu(*(u + 1)) + 32;
> > +
> > +        vgic_reserve_virq(d, pt_irq);
> > +        rc = route_irq_to_guest(d, pt_irq, pt_irq, "routed IRQ");
> > +        if ( rc < 0 )
> > +            return rc;
> 
> You are assuming the device can only generate one interrupt.

That's a mistake, thanks for spotting it, I'll fix it.


> Furthermore, this is assuming all the nodes in the fragment will be
> under the GIC controller.  You may want to passthrough a interrupt
> controller (i.e GPIO) to the guest and the related device.

I don't think that the non-GIC scenario is supported today. I don't
think it works even without dom0less. I wrote more about this as a reply
to the other email.


> > +    }
> 
> NIT: newline here.

OK


> 
> > +    return 0;
> > +}
> > +
> > +static int __init domain_addign_devices(struct domain *d,
> 
> s/addign/adding/

Damn! Thanks!


> > +                                        struct kernel_info *kinfo)
> > +{
> > +    void *pfdt;
> > +
> > +    pfdt = ioremap_cache(kinfo->dtb_bootmodule->start,
> > +            kinfo->dtb_bootmodule->size);
> > +    if ( pfdt == NULL )
> > +        return -EFAULT;
> > +
> > +    device_tree_for_each_node(pfdt, scan_pt_node, kinfo);
> > +
> > +    iounmap(pfdt);
> > +    return 0;
> > +}
> > +
> >   static int __init construct_domU(struct domain *d,
> >                                    const struct dt_device_node *node)
> >   {
> > @@ -2140,6 +2222,9 @@ static int __init construct_domU(struct domain *d,
> >       if ( kinfo.vpl011 )
> >           rc = domain_vpl011_init(d, NULL);
> >   +    if ( kinfo.dtb_bootmodule )
> > +        rc = domain_addign_devices(d, &kinfo);
> > +
> >       return rc;
> >   }
> >   diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> > index 7408a6c..356a422 100644
> > --- a/xen/include/xen/device_tree.h
> > +++ b/xen/include/xen/device_tree.h
> > @@ -161,6 +161,8 @@ extern const void *device_tree_flattened;
> >   int device_tree_for_each_node(const void *fdt,
> >                                        device_tree_node_func func,
> >                                        void *data);
> > +void device_tree_get_reg(const __be32 **cell, u32 address_cells,
> > +                         u32 size_cells, u64 *start, u64 *size);
> >     /**
> >    * dt_unflatten_host_device_tree - Unflatten the host device tree

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

Re: [PATCH 5/5] xen/arm: add dom0less device assignment info to docs

[Stefano Stabellini]

On Mon, 24 Dec 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 12/5/18 5:28 PM, Stefano Stabellini wrote:
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> >   docs/misc/arm/device-tree/booting.txt | 108
> > ++++++++++++++++++++++++++++++++++
> >   1 file changed, 108 insertions(+)
> > 
> > diff --git a/docs/misc/arm/device-tree/booting.txt
> > b/docs/misc/arm/device-tree/booting.txt
> > index 317a9e9..f5aaf8f 100644
> > --- a/docs/misc/arm/device-tree/booting.txt
> > +++ b/docs/misc/arm/device-tree/booting.txt
> > @@ -226,3 +226,111 @@ 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,dtb"
> 
> I would prefer "multiboot,device-tree"

I renamed it


> > +
> > +- reg
> > +
> > +    Specifies the physical address of the device tree binary fragment
> > +    RAM and its length.
> > +
> > +As an example:
> > +
> > +        module@0xc000000 {
> > +            compatible = "multiboot,dtb", "multiboot,module";
> > +            reg = <0x0 0xc000000 0xffffff>;
> > +        };
> > +
> > +The DTB fragment (loaded in memory at 0xc000000 in the example above)
> > +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.
> > +
> > +In addition, the following properties for each device node in the device
> > +tree fragment will be used for the device assignment setup:
> > +
> > +- reg
> > +
> > +  The reg property specifying the address and size of the device memory.
> > +  The device memory will be automatically mapped to the guest domain
> > +  with a 1:1 mapping (pseudo-physical address == physical address).
> 
> As said in a previous patch, I don't think this is correct to impose 1:1. The
> user is neither in control of the HW memory map nor the Guest memory map. So
> not many people are going to be able to use it without hacking Xen.

Yes, I'll fix this (and a couple of other issues) by introducing a new
"xen,reg" property, instead of trying to reuse the existing reg
property.


> > +
> > +- interrupts
> > +
> > +  The interrupts property specifies the interrupt of the device. They
> > +  are automatically routed to the guest domain with virtual irqs ==
> > +  physical irqs.
> > +
> > +- interrupt-parent
> > +
> > +  It contains a reference to the interrupt controller node. It should be
> > +  65000, corresponding to GUEST_PHANDLE_GIC.
> 
> We managed to get away in the toolstack with this property. So why do you need
> it for the hypervisor? Furthermore, this would forbid to passthrough any other
> interrupt controller to the guest.

The toolstack does use GUEST_PHANDLE_GIC today for passthrough
interrupts, see tools/libxl/libxl_arm.c:make_root_properties and
docs/misc/arm/passthrough.txt:

  * The interrupt-parent property will be added by the toolstack in the
    root node;

interrupt-parent always points to the guest GIC node, for virtual
interrupts such as the vuart, as well as physical passthrough
interrupts. Also see tools/libxl/libxl_arm.c:fdt_property_interrupts and
tools/libxl/libxl_arm.c:make_gicv2_node.

I don't think the scenario you are describing is supported today.


> > +
> > +- path
> > +
> > +  A new string property named "path" holds the path in the host device
> > +  tree to the corresponding device node.
> 
> This name is too generic. Please prepend "xen,"

OK, no prob


> > +
> > +The following is a real-world example of a device tree fragment for the
> > +network card on Xilinx MPSoC boards:
> > +
> > +/dts-v1/;
> > +
> > +/ {
> > +    #address-cells = <0x2>;
> > +    #size-cells = <0x1>;
> > +
> > +    passthrough {
> > +        compatible = "simple-bus";
> > +        ranges;
> > +        #address-cells = <0x2>;
> > +        #size-cells = <0x1>;
> > +
> > +        misc_clk {
> > +            #clock-cells = <0x0>;
> > +            clock-frequency = <0x7735940>;
> > +            compatible = "fixed-clock";
> > +            linux,phandle = <0x1>;
> > +            phandle = <0x1>;
> > +        };
> > +
> > +        ethernet@ff0e0000 {
> > +            compatible = "cdns,zynqmp-gem";
> > +            status = "okay";
> > +            interrupt-parent = <0xfde8>;
> > +            interrupts = <0x0 0x3f 0x4 0x0 0x3f 0x4>;
> > +            reg = <0x0 0xff0e0000 0x1000>;
> > +            clock-names = "pclk", "hclk", "tx_clk", "rx_clk";
> > +            #address-cells = <0x1>;
> > +            #size-cells = <0x0>;
> > +            clocks = <0x1 0x1 0x1 0x1>;
> > +            phy-mode = "rgmii-id";
> > +            xlnx,ptp-enet-clock = <0x0>;
> > +            local-mac-address = [00 0a 35 00 22 01];
> > +            phy-handle = <0x2>;
> > +            path = "/amba/ethernet@ff0e0000";
> > +
> > +            phy@c {
> > +                reg = <0xc>;
> > +                ti,rx-internal-delay = <0x8>;
> > +                ti,tx-internal-delay = <0xa>;
> > +                ti,fifo-depth = <0x1>;
> > +                ti,rxctrl-strap-worka;
> > +                linux,phandle = <0x2>;
> > +                phandle = <0x2>;
> > +            };
> > +        };
> > +    };
> > +};

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

Re: [PATCH 5/5] xen/arm: add dom0less device assignment info to docs

[Stefano Stabellini]

On Thu, 3 Jan 2019, Stefano Stabellini wrote:
> On Mon, 24 Dec 2018, Julien Grall wrote:
> > Hi Stefano,
> > 
> > On 12/5/18 5:28 PM, Stefano Stabellini wrote:
> > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > > ---
> > >   docs/misc/arm/device-tree/booting.txt | 108
> > > ++++++++++++++++++++++++++++++++++
> > >   1 file changed, 108 insertions(+)
> > > 
> > > diff --git a/docs/misc/arm/device-tree/booting.txt
> > > b/docs/misc/arm/device-tree/booting.txt
> > > index 317a9e9..f5aaf8f 100644
> > > --- a/docs/misc/arm/device-tree/booting.txt
> > > +++ b/docs/misc/arm/device-tree/booting.txt
> > > @@ -226,3 +226,111 @@ 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,dtb"
> > 
> > I would prefer "multiboot,device-tree"
> 
> I renamed it
> 
> 
> > > +
> > > +- reg
> > > +
> > > +    Specifies the physical address of the device tree binary fragment
> > > +    RAM and its length.
> > > +
> > > +As an example:
> > > +
> > > +        module@0xc000000 {
> > > +            compatible = "multiboot,dtb", "multiboot,module";
> > > +            reg = <0x0 0xc000000 0xffffff>;
> > > +        };
> > > +
> > > +The DTB fragment (loaded in memory at 0xc000000 in the example above)
> > > +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.
> > > +
> > > +In addition, the following properties for each device node in the device
> > > +tree fragment will be used for the device assignment setup:
> > > +
> > > +- reg
> > > +
> > > +  The reg property specifying the address and size of the device memory.
> > > +  The device memory will be automatically mapped to the guest domain
> > > +  with a 1:1 mapping (pseudo-physical address == physical address).
> > 
> > As said in a previous patch, I don't think this is correct to impose 1:1. The
> > user is neither in control of the HW memory map nor the Guest memory map. So
> > not many people are going to be able to use it without hacking Xen.
> 
> Yes, I'll fix this (and a couple of other issues) by introducing a new
> "xen,reg" property, instead of trying to reuse the existing reg
> property.
> 
> 
> > > +
> > > +- interrupts
> > > +
> > > +  The interrupts property specifies the interrupt of the device. They
> > > +  are automatically routed to the guest domain with virtual irqs ==
> > > +  physical irqs.
> > > +
> > > +- interrupt-parent
> > > +
> > > +  It contains a reference to the interrupt controller node. It should be
> > > +  65000, corresponding to GUEST_PHANDLE_GIC.
> > 
> > We managed to get away in the toolstack with this property. So why do you need
> > it for the hypervisor? Furthermore, this would forbid to passthrough any other
> > interrupt controller to the guest.
> 
> The toolstack does use GUEST_PHANDLE_GIC today for passthrough
> interrupts, see tools/libxl/libxl_arm.c:make_root_properties and
> docs/misc/arm/passthrough.txt:
> 
>   * The interrupt-parent property will be added by the toolstack in the
>     root node;
> 
> interrupt-parent always points to the guest GIC node, for virtual
> interrupts such as the vuart, as well as physical passthrough
> interrupts. Also see tools/libxl/libxl_arm.c:fdt_property_interrupts and
> tools/libxl/libxl_arm.c:make_gicv2_node.
> 
> I don't think the scenario you are describing is supported today.

Given that I have accumulated a bunch of changes, I'll send v2 of the
series now. We can further discuss the interrupts issue there. I am
happy to make changes if required.

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

Re: [PATCH 2/5] xen/arm: assign devices to boot domains

[Julien Grall]

Hi,

On 1/3/19 10:07 PM, Stefano Stabellini wrote:
> On Mon, 24 Dec 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 12/5/18 5:28 PM, 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.
>>>
>>> Device memory is only mapped 1:1. It is not possible to assign devices at
>>> locations that conflict with the DomU memory map.
>> I think 1:1 mapped is a pretty bad idea. You limit yourself a lot as the user
>> does not control neither the HW nor the guest memory map.
>>
>> So you need to provide a way for the user to specify the mapping.
> 
> Yes, I think we want to introduce a separate xen,reg property that
> includes a destination address as well. Something of the format:
> 
>    xen,reg = <source_addr source_size dest_addr>
> 
> More on this below.
> 
> 
>>>
>>> The iommu is setup by passing the to the device to assign on the host
>>
>> NIT: "the node to..."
> 
> I'll fix
> 
>   
>>> device tree. The path is specified in the device tree fragment as the
>>> "path" string property.
>>
>> Path is too generic and may clash in the future with other bindings. Please
>> add "xen," in front.
> 
> OK, I can rename
> 
> 
>>>
>>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>> ---
>>>    xen/arch/arm/bootfdt.c        |  4 +-
>>>    xen/arch/arm/domain_build.c   | 85
>>> +++++++++++++++++++++++++++++++++++++++++++
>>>    xen/include/xen/device_tree.h |  2 +
>>>    3 files changed, 89 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
>>> index 891b4b6..72cb8d6 100644
>>> --- a/xen/arch/arm/bootfdt.c
>>> +++ b/xen/arch/arm/bootfdt.c
>>> @@ -55,8 +55,8 @@ 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);
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index cc6b464..d48f77e 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -2094,6 +2094,88 @@ static int __init construct_domain(struct domain *d,
>>> struct kernel_info *kinfo)
>>>        return 0;
>>>    }
>>>    +static int __init scan_pt_node(const void *pfdt,
>>> +                               int nodeoff, const char *name, int depth,
>>> +                               u32 address_cells, u32 size_cells,
>>> +                               void *data)
>>
>> Is it really necessary to parse the device-tree twice?
> 
> I don't think I understand this comment. The device tree fragment is not
> scanned twice, just once I think. Am I missing something?

The previous patch is going through the DT to copy the properties over. 
This patch introduce a new function to go over the DT to find the Device 
to attach. So you are parsing the DT twice. Is there any reason for that?

> 
> 
>>> +{
>>> +    int rc;
>>> +    struct dt_device_node *node;
>>> +    int len, i;
>>> +    const struct fdt_property *prop;
>>> +    struct kernel_info *kinfo = data;
>>> +    struct domain *d = kinfo->d;
>>> +    const __be32 *cell;
>>> +
>>> +    if ( depth > 2 )
>>> +        return 0;
>>
>> Why do you limit yourself to depth 2? It is possible to have nested node
>> describing memory
> 
> That was because Xen needed a far better understanding of how reg works
> to be able to parse nested nodes correctly.

Xen has already the knowledge for understanding reg (see 
common/device_tree.c). Although, I agree that it only works on unflatten 
device-tree today.

> Specifically, see this
> concrete example from ZynqMP:
> 
>          ethernet@ff0e0000 {
>              compatible = "cdns,zynqmp-gem";
>              status = "okay";
>              interrupt-parent = <0xfde8>;
>              interrupts = <0x0 0x3f 0x4 0x0 0x3f 0x4>;
>              reg = <0x0 0xff0e0000 0x1000>;
>              clock-names = "pclk", "hclk", "tx_clk", "rx_clk";
>              #address-cells = <0x1>;
>              #size-cells = <0x0>;
>              clocks = <0x1 0x1 0x1 0x1>;
>              phy-mode = "rgmii-id";
>              xlnx,ptp-enet-clock = <0x0>;
>              local-mac-address = [00 0a 35 00 22 01];
>              phy-handle = <0x2>;
>              xen,path = "/amba/ethernet@ff0e0000";
> 
>              phy@c {
>                  reg = <0xc>;
>                  ti,rx-internal-delay = <0x8>;
>                  ti,tx-internal-delay = <0xa>;
>                  ti,fifo-depth = <0x1>;
>                  ti,rxctrl-strap-worka;
>                  linux,phandle = <0x2>;
>                  phandle = <0x2>;
>              };
>          };
> 
> We want to map the first reg property under ethernet@ff0e0000, not the
> reg property under phy@c, which it doesn't even refer to a memory
> region. This problem goes away completely if we start using a new Xen
> specific "xen,reg" property, instead of trying to figure out which
> existing reg to map. With that, we can safely remove the artificial
> depth <= 2 restriction.

Your proposition for "xen,reg" is a bit different from what I was 
thinking. Ideally we only want the user to specify once the guest 
physical address (i.e only in reg). But, I understand that it may 
require more work.

I think I would be happy with this solution providing there are an 
action to investigate whether "range" can be used.

>>> +
>>> +    prop = fdt_get_property_namelen(pfdt, nodeoff, "reg", strlen("reg"),
>>> &len);
>>> +    if ( prop != NULL )
>>> +    {
>>> +        paddr_t start, size;
>>> +        cell = (const __be32 *)prop->data;
>>> +        len = fdt32_to_cpu(prop->len) /
>>> +              ((address_cells + size_cells) * sizeof (u32));
>>> +
>>> +        for ( i = 0; i < len; i++ )
>>> +        {
>>> +            device_tree_get_reg(&cell, address_cells, size_cells, &start,
>>> &size);
>>
>> Here you assume the value in regs correspond to host physical address. This
>> may not be the case if a device is under a bus.
> 
> Yes, one more reason to move away from parsing the existing reg property
> and use a new xen,reg property that gives directly the host physical
> address to map to the right guest physical address (regardless of
> busses). xen,reg would basically be equivalent to the iomem VM config
> file option.
> 
> 
>>> +
>>> +            rc = guest_physmap_add_entry(d, _gfn(start >> PAGE_SHIFT),
>>
>> Please use gaddr_to_gfn and ...
> 
> OK
> 
> 
>>> +                                         _mfn(start >> PAGE_SHIFT),
>>
>> ... maddr_to_mfn
> 
> OK
> 
> 
>>> +                                         get_order_from_bytes(size),
>>> +                                         p2m_mmio_direct_dev);
>>
>> I think the restriction on the memory attributes should be documented in patch
>> #5.
> 
> OK
> 
> 
>>> +            if ( rc < 0 )
>>> +            {
>>> +                dprintk(XENLOG_ERR, "Failed to map %"PRIpaddr" to the
>>> guest\n", start);
>>> +                return -EFAULT;
>>> +            }
>>> +        }
>>> +    }
>>> +
>>> +    prop = fdt_get_property_namelen(pfdt, nodeoff, "path", strlen("path"),
>>> &len);
>>> +    if ( prop != NULL ) {
>>> +        node = dt_find_node_by_path((char *)&prop->data[0]);
>>
>> What's wrong with giving directly prop->data?
> 
> Nothing wrong with it, just a matter of code style. I don't care about
> this, I can change it.

My issue is with the cast, not &prop->data[0] itself (though I dislike 
it ;)). Casts are dangerous and should be avoided as much as possible.

[...]

>>> +
>>> +        pt_irq = fdt32_to_cpu(*(u + 1)) + 32;
>>> +
>>> +        vgic_reserve_virq(d, pt_irq);
>>> +        rc = route_irq_to_guest(d, pt_irq, pt_irq, "routed IRQ");
>>> +        if ( rc < 0 )
>>> +            return rc;
>>
>> You are assuming the device can only generate one interrupt.
> 
> That's a mistake, thanks for spotting it, I'll fix it.
> 
> 
>> Furthermore, this is assuming all the nodes in the fragment will be
>> under the GIC controller.  You may want to passthrough a interrupt
>> controller (i.e GPIO) to the guest and the related device.
> 
> I don't think that the non-GIC scenario is supported today. I don't
> think it works even without dom0less. I wrote more about this as a reply
> to the other email.
I believe this works out-of-box with the guest. If we take the example 
of the GPIO controller, it may be behind the GIC. You only care to route 
those interrupts used by GPIO and MMIO associated. The rest can be 
describe normally in the DT.

Of course, this means that you need to passthrough all devices using the 
GPIO controller to that guest.

So I still think you need to check whether the interrupts belongs the 
GIC or not.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 4/5] xen/arm: use the physical number of gic lines for boot domains

[Julien Grall]

Hi Stefano,

On 1/3/19 7:07 PM, Stefano Stabellini wrote:
> On Mon, 24 Dec 2018, Julien Grall wrote:
>> Hi,
>>
>> On 12/5/18 5:28 PM, Stefano Stabellini wrote:
>>> We don't have a clear way to know how many virtual SPIs we need for the
>>> boot domains. For simplicity, allocate as many as natively supported,
>>> just like for dom0.
>>
>> This will potentially allocate a lot of unused interrupts and a waste of
>> memory. So is it the correct solution?
>>
>> For instance, we would request the user to provide the number of interrupts.
> 
> Unfortunately, this has to happen much earlier than when we parse user
> supplied device tree options. We could make it an hypervisor command
> line parameter but it would be nasty.

Why does this value needs to be in the supplied device-tree. Can't it be 
part of the node in the host DT?

> 
> Given that this feature is aimed at partitioning scenarios where the
> number of VMs is low, the increased memory overhead is not a big issue.

Please write it down in the commit message.


> 
> (If you recall we briefly discussed this topic at the last Linaro
> Connect, this was the conclusion.)
> 

Cheers,

-- 
Julien Grall

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

Re: [PATCH 5/5] xen/arm: add dom0less device assignment info to docs

[Julien Grall]

Hi,

On 1/3/19 10:07 PM, Stefano Stabellini wrote:
> On Mon, 24 Dec 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 12/5/18 5:28 PM, Stefano Stabellini wrote:
>>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>> ---
>>>    docs/misc/arm/device-tree/booting.txt | 108
>>> ++++++++++++++++++++++++++++++++++
>>>    1 file changed, 108 insertions(+)
>>>
>>> diff --git a/docs/misc/arm/device-tree/booting.txt
>>> b/docs/misc/arm/device-tree/booting.txt
>>> index 317a9e9..f5aaf8f 100644
>>> --- a/docs/misc/arm/device-tree/booting.txt
>>> +++ b/docs/misc/arm/device-tree/booting.txt
>>> @@ -226,3 +226,111 @@ 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,dtb"
>>
>> I would prefer "multiboot,device-tree"
> 
> I renamed it
> 
> 
>>> +
>>> +- reg
>>> +
>>> +    Specifies the physical address of the device tree binary fragment
>>> +    RAM and its length.
>>> +
>>> +As an example:
>>> +
>>> +        module@0xc000000 {
>>> +            compatible = "multiboot,dtb", "multiboot,module";
>>> +            reg = <0x0 0xc000000 0xffffff>;
>>> +        };
>>> +
>>> +The DTB fragment (loaded in memory at 0xc000000 in the example above)
>>> +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.
>>> +
>>> +In addition, the following properties for each device node in the device
>>> +tree fragment will be used for the device assignment setup:
>>> +
>>> +- reg
>>> +
>>> +  The reg property specifying the address and size of the device memory.
>>> +  The device memory will be automatically mapped to the guest domain
>>> +  with a 1:1 mapping (pseudo-physical address == physical address).
>>
>> As said in a previous patch, I don't think this is correct to impose 1:1. The
>> user is neither in control of the HW memory map nor the Guest memory map. So
>> not many people are going to be able to use it without hacking Xen.
> 
> Yes, I'll fix this (and a couple of other issues) by introducing a new
> "xen,reg" property, instead of trying to reuse the existing reg
> property.
> 
> 
>>> +
>>> +- interrupts
>>> +
>>> +  The interrupts property specifies the interrupt of the device. They
>>> +  are automatically routed to the guest domain with virtual irqs ==
>>> +  physical irqs.
>>> +
>>> +- interrupt-parent
>>> +
>>> +  It contains a reference to the interrupt controller node. It should be
>>> +  65000, corresponding to GUEST_PHANDLE_GIC.
>>
>> We managed to get away in the toolstack with this property. So why do you need
>> it for the hypervisor? Furthermore, this would forbid to passthrough any other
>> interrupt controller to the guest.
> 
> The toolstack does use GUEST_PHANDLE_GIC today for passthrough
> interrupts, see tools/libxl/libxl_arm.c:make_root_properties and
> docs/misc/arm/passthrough.txt:
> 
>    * The interrupt-parent property will be added by the toolstack in the
>      root node;

You misunderstood my point here. The toolstack is adding the property 
for the user. So why does you require the user to add this property for 
Dom0less case?

> 
> interrupt-parent always points to the guest GIC node, for virtual
> interrupts such as the vuart, as well as physical passthrough
> interrupts. Also see tools/libxl/libxl_arm.c:fdt_property_interrupts and
> tools/libxl/libxl_arm.c:make_gicv2_node.

The "interrupt-parent" in those nodes are not necessary because the root 
contain "interrupt-parent". The property is only useful if you are going 
to use a different controller than the default one. For instance if you 
are using a GPIO controller.

> 
> I don't think the scenario you are describing is supported today.

See my answer on patch #2. I can't see why it is not supported today.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 5/5] xen/arm: add dom0less device assignment info to docs

[Julien Grall]

Hi Stefano,

On 1/3/19 11:31 PM, Stefano Stabellini wrote:
> On Thu, 3 Jan 2019, Stefano Stabellini wrote:
>> On Mon, 24 Dec 2018, Julien Grall wrote:
>>> Hi Stefano,
>>>
>>> On 12/5/18 5:28 PM, Stefano Stabellini wrote:
>>>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>>> ---
>>>>    docs/misc/arm/device-tree/booting.txt | 108
>>>> ++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 108 insertions(+)
>>>>
>>>> diff --git a/docs/misc/arm/device-tree/booting.txt
>>>> b/docs/misc/arm/device-tree/booting.txt
>>>> index 317a9e9..f5aaf8f 100644
>>>> --- a/docs/misc/arm/device-tree/booting.txt
>>>> +++ b/docs/misc/arm/device-tree/booting.txt
>>>> @@ -226,3 +226,111 @@ 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,dtb"
>>>
>>> I would prefer "multiboot,device-tree"
>>
>> I renamed it
>>
>>
>>>> +
>>>> +- reg
>>>> +
>>>> +    Specifies the physical address of the device tree binary fragment
>>>> +    RAM and its length.
>>>> +
>>>> +As an example:
>>>> +
>>>> +        module@0xc000000 {
>>>> +            compatible = "multiboot,dtb", "multiboot,module";
>>>> +            reg = <0x0 0xc000000 0xffffff>;
>>>> +        };
>>>> +
>>>> +The DTB fragment (loaded in memory at 0xc000000 in the example above)
>>>> +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.
>>>> +
>>>> +In addition, the following properties for each device node in the device
>>>> +tree fragment will be used for the device assignment setup:
>>>> +
>>>> +- reg
>>>> +
>>>> +  The reg property specifying the address and size of the device memory.
>>>> +  The device memory will be automatically mapped to the guest domain
>>>> +  with a 1:1 mapping (pseudo-physical address == physical address).
>>>
>>> As said in a previous patch, I don't think this is correct to impose 1:1. The
>>> user is neither in control of the HW memory map nor the Guest memory map. So
>>> not many people are going to be able to use it without hacking Xen.
>>
>> Yes, I'll fix this (and a couple of other issues) by introducing a new
>> "xen,reg" property, instead of trying to reuse the existing reg
>> property.
>>
>>
>>>> +
>>>> +- interrupts
>>>> +
>>>> +  The interrupts property specifies the interrupt of the device. They
>>>> +  are automatically routed to the guest domain with virtual irqs ==
>>>> +  physical irqs.
>>>> +
>>>> +- interrupt-parent
>>>> +
>>>> +  It contains a reference to the interrupt controller node. It should be
>>>> +  65000, corresponding to GUEST_PHANDLE_GIC.
>>>
>>> We managed to get away in the toolstack with this property. So why do you need
>>> it for the hypervisor? Furthermore, this would forbid to passthrough any other
>>> interrupt controller to the guest.
>>
>> The toolstack does use GUEST_PHANDLE_GIC today for passthrough
>> interrupts, see tools/libxl/libxl_arm.c:make_root_properties and
>> docs/misc/arm/passthrough.txt:
>>
>>    * The interrupt-parent property will be added by the toolstack in the
>>      root node;
>>
>> interrupt-parent always points to the guest GIC node, for virtual
>> interrupts such as the vuart, as well as physical passthrough
>> interrupts. Also see tools/libxl/libxl_arm.c:fdt_property_interrupts and
>> tools/libxl/libxl_arm.c:make_gicv2_node.
>>
>> I don't think the scenario you are describing is supported today.
> 
> Given that I have accumulated a bunch of changes, I'll send v2 of the
> series now. We can further discuss the interrupts issue there. I am
> happy to make changes if required.

I have answered to some of them. I will skip v2 and wait for v3.

Cheers,

-- 
Julien Grall

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