* [PATCH 1/5] xen/arm: copy dtb fragment to guest dtb
2018-12-05 17:28 [PATCH 0/5] dom0less device assignment Stefano Stabellini
@ 2018-12-05 17:28 ` Stefano Stabellini
2018-12-24 11:17 ` Julien Grall
2018-12-05 17:28 ` [PATCH 2/5] xen/arm: assign devices to boot domains Stefano Stabellini
` (3 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: Stefano Stabellini @ 2018-12-05 17:28 UTC (permalink / raw)
To: julien.grall
Cc: Stefano Stabellini, Achin.Gupta, sstabellini, andrii_anisov, xen-devel
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 1/5] xen/arm: copy dtb fragment to guest dtb
2018-12-05 17:28 ` [PATCH 1/5] xen/arm: copy dtb fragment to guest dtb Stefano Stabellini
@ 2018-12-24 11:17 ` Julien Grall
2019-01-02 12:22 ` Wei Liu
2019-01-02 23:25 ` Stefano Stabellini
0 siblings, 2 replies; 22+ messages in thread
From: Julien Grall @ 2018-12-24 11:17 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, andrii_anisov, Achin.Gupta, Ian Jackson,
xen-devel, Wei Liu
(+ 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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/5] xen/arm: copy dtb fragment to guest dtb
2018-12-24 11:17 ` Julien Grall
@ 2019-01-02 12:22 ` Wei Liu
2019-01-02 23:25 ` Stefano Stabellini
1 sibling, 0 replies; 22+ messages in thread
From: Wei Liu @ 2019-01-02 12:22 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Stefano Stabellini, andrii_anisov,
Lars Kurth, Achin.Gupta, Ian Jackson, xen-devel, Committers,
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/5] xen/arm: copy dtb fragment to guest dtb
2018-12-24 11:17 ` Julien Grall
2019-01-02 12:22 ` Wei Liu
@ 2019-01-02 23:25 ` Stefano Stabellini
1 sibling, 0 replies; 22+ messages in thread
From: Stefano Stabellini @ 2019-01-02 23:25 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Stefano Stabellini, andrii_anisov,
Achin.Gupta, Ian Jackson, xen-devel, Wei Liu
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/5] xen/arm: assign devices to boot domains
2018-12-05 17:28 [PATCH 0/5] dom0less device assignment Stefano Stabellini
2018-12-05 17:28 ` [PATCH 1/5] xen/arm: copy dtb fragment to guest dtb Stefano Stabellini
@ 2018-12-05 17:28 ` Stefano Stabellini
2018-12-24 13:55 ` Julien Grall
2018-12-05 17:28 ` [PATCH 3/5] xen/arm: handle "multiboot, dtb" compatible nodes Stefano Stabellini
` (2 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: Stefano Stabellini @ 2018-12-05 17:28 UTC (permalink / raw)
To: julien.grall
Cc: Stefano Stabellini, Achin.Gupta, sstabellini, andrii_anisov, xen-devel
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 2/5] xen/arm: assign devices to boot domains
2018-12-05 17:28 ` [PATCH 2/5] xen/arm: assign devices to boot domains Stefano Stabellini
@ 2018-12-24 13:55 ` Julien Grall
2019-01-03 22:07 ` Stefano Stabellini
0 siblings, 1 reply; 22+ messages in thread
From: Julien Grall @ 2018-12-24 13:55 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Achin.Gupta, andrii_anisov, xen-devel
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 2/5] xen/arm: assign devices to boot domains
2018-12-24 13:55 ` Julien Grall
@ 2019-01-03 22:07 ` Stefano Stabellini
2019-01-15 12:50 ` Julien Grall
0 siblings, 1 reply; 22+ messages in thread
From: Stefano Stabellini @ 2019-01-03 22:07 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Achin.Gupta, Stefano Stabellini,
andrii_anisov, xen-devel
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 2/5] xen/arm: assign devices to boot domains
2019-01-03 22:07 ` Stefano Stabellini
@ 2019-01-15 12:50 ` Julien Grall
0 siblings, 0 replies; 22+ messages in thread
From: Julien Grall @ 2019-01-15 12:50 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Achin.Gupta, andrii_anisov, xen-devel
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 3/5] xen/arm: handle "multiboot, dtb" compatible nodes
2018-12-05 17:28 [PATCH 0/5] dom0less device assignment Stefano Stabellini
2018-12-05 17:28 ` [PATCH 1/5] xen/arm: copy dtb fragment to guest dtb Stefano Stabellini
2018-12-05 17:28 ` [PATCH 2/5] xen/arm: assign devices to boot domains Stefano Stabellini
@ 2018-12-05 17:28 ` Stefano Stabellini
2018-12-24 13:58 ` Julien Grall
2018-12-05 17:28 ` [PATCH 4/5] xen/arm: use the physical number of gic lines for boot domains Stefano Stabellini
2018-12-05 17:28 ` [PATCH 5/5] xen/arm: add dom0less device assignment info to docs Stefano Stabellini
4 siblings, 1 reply; 22+ messages in thread
From: Stefano Stabellini @ 2018-12-05 17:28 UTC (permalink / raw)
To: julien.grall
Cc: Stefano Stabellini, Achin.Gupta, sstabellini, andrii_anisov, xen-devel
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 3/5] xen/arm: handle "multiboot, dtb" compatible nodes
2018-12-05 17:28 ` [PATCH 3/5] xen/arm: handle "multiboot, dtb" compatible nodes Stefano Stabellini
@ 2018-12-24 13:58 ` Julien Grall
2019-01-02 23:28 ` Stefano Stabellini
0 siblings, 1 reply; 22+ messages in thread
From: Julien Grall @ 2018-12-24 13:58 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Achin.Gupta, andrii_anisov, xen-devel
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 3/5] xen/arm: handle "multiboot, dtb" compatible nodes
2018-12-24 13:58 ` Julien Grall
@ 2019-01-02 23:28 ` Stefano Stabellini
0 siblings, 0 replies; 22+ messages in thread
From: Stefano Stabellini @ 2019-01-02 23:28 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Achin.Gupta, Stefano Stabellini,
andrii_anisov, xen-devel
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 4/5] xen/arm: use the physical number of gic lines for boot domains
2018-12-05 17:28 [PATCH 0/5] dom0less device assignment Stefano Stabellini
` (2 preceding siblings ...)
2018-12-05 17:28 ` [PATCH 3/5] xen/arm: handle "multiboot, dtb" compatible nodes Stefano Stabellini
@ 2018-12-05 17:28 ` Stefano Stabellini
2018-12-24 14:01 ` Julien Grall
2018-12-05 17:28 ` [PATCH 5/5] xen/arm: add dom0less device assignment info to docs Stefano Stabellini
4 siblings, 1 reply; 22+ messages in thread
From: Stefano Stabellini @ 2018-12-05 17:28 UTC (permalink / raw)
To: julien.grall
Cc: Stefano Stabellini, Achin.Gupta, sstabellini, andrii_anisov, xen-devel
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 4/5] xen/arm: use the physical number of gic lines for boot domains
2018-12-05 17:28 ` [PATCH 4/5] xen/arm: use the physical number of gic lines for boot domains Stefano Stabellini
@ 2018-12-24 14:01 ` Julien Grall
2019-01-03 19:07 ` Stefano Stabellini
0 siblings, 1 reply; 22+ messages in thread
From: Julien Grall @ 2018-12-24 14:01 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Achin.Gupta, andrii_anisov, xen-devel
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/5] xen/arm: use the physical number of gic lines for boot domains
2018-12-24 14:01 ` Julien Grall
@ 2019-01-03 19:07 ` Stefano Stabellini
2019-01-15 12:56 ` Julien Grall
0 siblings, 1 reply; 22+ messages in thread
From: Stefano Stabellini @ 2019-01-03 19:07 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Achin.Gupta, Stefano Stabellini,
andrii_anisov, xen-devel
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/5] xen/arm: use the physical number of gic lines for boot domains
2019-01-03 19:07 ` Stefano Stabellini
@ 2019-01-15 12:56 ` Julien Grall
0 siblings, 0 replies; 22+ messages in thread
From: Julien Grall @ 2019-01-15 12:56 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Achin.Gupta, andrii_anisov, xen-devel
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 5/5] xen/arm: add dom0less device assignment info to docs
2018-12-05 17:28 [PATCH 0/5] dom0less device assignment Stefano Stabellini
` (3 preceding siblings ...)
2018-12-05 17:28 ` [PATCH 4/5] xen/arm: use the physical number of gic lines for boot domains Stefano Stabellini
@ 2018-12-05 17:28 ` Stefano Stabellini
2018-12-24 14:06 ` Julien Grall
4 siblings, 1 reply; 22+ messages in thread
From: Stefano Stabellini @ 2018-12-05 17:28 UTC (permalink / raw)
To: julien.grall
Cc: Stefano Stabellini, Achin.Gupta, sstabellini, andrii_anisov, xen-devel
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 5/5] xen/arm: add dom0less device assignment info to docs
2018-12-05 17:28 ` [PATCH 5/5] xen/arm: add dom0less device assignment info to docs Stefano Stabellini
@ 2018-12-24 14:06 ` Julien Grall
2019-01-03 22:07 ` Stefano Stabellini
0 siblings, 1 reply; 22+ messages in thread
From: Julien Grall @ 2018-12-24 14:06 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Achin.Gupta, andrii_anisov, xen-devel
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 5/5] xen/arm: add dom0less device assignment info to docs
2018-12-24 14:06 ` Julien Grall
@ 2019-01-03 22:07 ` Stefano Stabellini
2019-01-03 23:31 ` Stefano Stabellini
2019-01-15 13:02 ` Julien Grall
0 siblings, 2 replies; 22+ messages in thread
From: Stefano Stabellini @ 2019-01-03 22:07 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, Achin.Gupta, Stefano Stabellini,
andrii_anisov, xen-devel
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 5/5] xen/arm: add dom0less device assignment info to docs
2019-01-03 22:07 ` Stefano Stabellini
@ 2019-01-03 23:31 ` Stefano Stabellini
2019-01-15 13:03 ` Julien Grall
2019-01-15 13:02 ` Julien Grall
1 sibling, 1 reply; 22+ messages in thread
From: Stefano Stabellini @ 2019-01-03 23:31 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Julien Grall, andrii_anisov, Achin.Gupta, xen-devel
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 5/5] xen/arm: add dom0less device assignment info to docs
2019-01-03 23:31 ` Stefano Stabellini
@ 2019-01-15 13:03 ` Julien Grall
0 siblings, 0 replies; 22+ messages in thread
From: Julien Grall @ 2019-01-15 13:03 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Achin.Gupta, andrii_anisov, xen-devel
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/5] xen/arm: add dom0less device assignment info to docs
2019-01-03 22:07 ` Stefano Stabellini
2019-01-03 23:31 ` Stefano Stabellini
@ 2019-01-15 13:02 ` Julien Grall
1 sibling, 0 replies; 22+ messages in thread
From: Julien Grall @ 2019-01-15 13:02 UTC (permalink / raw)
To: Stefano Stabellini
Cc: Stefano Stabellini, Achin.Gupta, andrii_anisov, xen-devel
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
^ permalink raw reply [flat|nested] 22+ messages in thread