Re: [Xen-devel] [PATCH v6 5/8] xen/arm: assign devices to boot domains
Hi Stefano,
On 30/09/2019 22:12, Stefano Stabellini wrote:
On Sun, 29 Sep 2019, Julien Grall wrote:
On 9/27/19 12:11 AM, Stefano Stabellini wrote:
+return 0;
+}
+/*
+ * xen,path specifies the corresponding node in the host DT.
+ * Both interrupt mappings and IOMMU settings are based on it,
+ * as they are done based on the corresponding host DT node.
+ */
+else if ( dt_prop_cmp("xen,path", name) == 0 )
+{
+node = dt_find_node_by_path(prop->data);
+if ( node == NULL )
+{
+printk(XENLOG_ERR "Couldn't find node %s in host_dt!\n",
+ (char *)prop->data);
+return -EINVAL;
+}
+
+res = iommu_assign_dt_device(kinfo->d, node);
+if ( res < 0 )
+return res; > +
+res = handle_device_interrupts(kinfo->d, node, true);
+if ( res < 0 )
+return res;
Same here.
You are talking about return values, right? Not code style to be fixed
-- I cannot see anything wrong with the code style.
I honestly can't remember why I wrote "same here". Please ignore this one and I
will have a look at the next version.
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v6 5/8] xen/arm: assign devices to boot domains
On Sun, 29 Sep 2019, Julien Grall wrote:
> Hi,
>
> On 9/27/19 12:11 AM, Stefano Stabellini wrote:
> > Scan the user provided dtb fragment at boot. For each device node, map
> > memory to guests, and route interrupts and setup the iommu.
> >
> > The memory region to remap is specified by the "xen,reg" property.
> >
> > The iommu is setup by passing the node of the device to assign on the
> > host device tree. The path is specified in the device tree fragment as
> > the "xen,path" string property.
> >
> > The interrupts are remapped based on the information from the
> > corresponding node on the host device tree. Call
> > handle_device_interrupts to remap interrupts. Interrupts related device
> > tree properties are copied from the device tree fragment, same as all
> > the other properties.
> >
> > Also set add the new flag XEN_DOMCTL_CDF_iommu so that dom0less domU
> > can use the IOMMU if a partial dtb is specified.
> >
> > Signed-off-by: Stefano Stabellini
> > ---
> > Changes in v6:
> > - turn dprintks into printks
> > - return error on page alignment check failure
> > - set XEN_DOMCTL_CDF_iommu if partial dtb is specified
> >
> > Changes in v5:
> > - use local variable for name
> > - use map_regions_p2mt
> > - add warning for not page aligned addresses/sizes
> > - introduce handle_passthrough_prop
> >
> > Changes in v4:
> > - use unsigned
> > - improve commit message
> > - code style
> > - use dt_prop_cmp
> > - use device_tree_get_reg
> > - don't copy over xen,reg and xen,path
> > - don't create special interrupt properties for domU: copy them from the
> >fragment
> > - in-code comment
> >
> > Changes in v3:
> > - improve commit message
> > - remove superfluous cast
> > - merge code with the copy code
> > - add interrup-parent
> > - demove depth > 2 check
> > - reuse code from handle_device_interrupts
> > - copy interrupts from host dt
> >
> > Changes in v2:
> > - rename "path" to "xen,path"
> > - grammar fix
> > - use gaddr_to_gfn and maddr_to_mfn
> > - remove depth <= 2 limitation in scanning the dtb fragment
> > - introduce and parse xen,reg
> > - code style
> > - support more than one interrupt per device
> > - specify only the GIC is supported
> > ---
> > xen/arch/arm/domain_build.c | 104 ++--
> > 1 file changed, 101 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 08d6d238e3..a461816345 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1699,6 +1699,88 @@ static int __init make_vpl011_uart_node(struct
> > kernel_info *kinfo)
> > }
> > #endif
> > +/*
> > + * Scan device tree properties for passthrough specific information.
> > + * Returns -ENOENT when no passthrough properties are found
>
> Such things only work if you control the return value. Looking at the code,
> you propagate the value from iommu_assign_dt_device(). While today we return
> -EINVAL when the device is not protected, it may make sense to return -ENOENT
> (after all the device is not behind an IOMMU and therefore technically not
> found).
>
> You would end up to present the property when it should not. So it would be
> better to consider returning a positive value when the property needs to be
> copied.
Yes, I understand your point and it is valid. I'll make the change. I'll
return "1" and also change this comment on top of the function.
> > + * < 0 on error
> > + * 0 on success
> > + */
> > +static int __init handle_passthrough_prop(struct kernel_info *kinfo,
> > + const struct fdt_property *prop,
> > + const char *name,
> > + uint32_t address_cells, uint32_t
> > size_cells)
> > +{
> > +const __be32 *cell;
> > +unsigned int i, len;
> > +struct dt_device_node *node;
> > +int res;
> > +
> > +/* xen,reg specifies where to map the MMIO region */
> > +if ( dt_prop_cmp("xen,reg", name) == 0 )
> > +{
> > +paddr_t mstart, size, gstart;
>
> NIT: Newline between declaration and code.
>
> > +cell = (const __be32 *)prop->data;
> > +len = fdt32_to_cpu(prop->len) /
> > +((address_cells * 2 + size_cells) * sizeof(uint32_t));
>
> NIT: The indentation looks incorrect. I missed this one on the previous
> review.
>
> > +
> > +for ( i = 0; i < len; i++ )
> > +{
> > +device_tree_get_reg(, address_cells, size_cells,
> > +, );
>
> Same here. It might be worth checking your editor configuration to avoid such
> things happening in the future...
Yes, my editor doesn't do Xen alignment right. I have fixed these three
instances.
> > +gstart = dt_next_cell(address_cells, );
> > +
> > +if ( gstart & ~PAGE_MASK || mstart & ~PAGE_MASK || size &
> > ~PAGE_MASK )
> > +{
> > +
Re: [Xen-devel] [PATCH v6 5/8] xen/arm: assign devices to boot domains
Hi,
On 9/27/19 12:11 AM, Stefano Stabellini wrote:
Scan the user provided dtb fragment at boot. For each device node, map
memory to guests, and route interrupts and setup the iommu.
The memory region to remap is specified by the "xen,reg" property.
The iommu is setup by passing the node of the device to assign on the
host device tree. The path is specified in the device tree fragment as
the "xen,path" string property.
The interrupts are remapped based on the information from the
corresponding node on the host device tree. Call
handle_device_interrupts to remap interrupts. Interrupts related device
tree properties are copied from the device tree fragment, same as all
the other properties.
Also set add the new flag XEN_DOMCTL_CDF_iommu so that dom0less domU
can use the IOMMU if a partial dtb is specified.
Signed-off-by: Stefano Stabellini
---
Changes in v6:
- turn dprintks into printks
- return error on page alignment check failure
- set XEN_DOMCTL_CDF_iommu if partial dtb is specified
Changes in v5:
- use local variable for name
- use map_regions_p2mt
- add warning for not page aligned addresses/sizes
- introduce handle_passthrough_prop
Changes in v4:
- use unsigned
- improve commit message
- code style
- use dt_prop_cmp
- use device_tree_get_reg
- don't copy over xen,reg and xen,path
- don't create special interrupt properties for domU: copy them from the
fragment
- in-code comment
Changes in v3:
- improve commit message
- remove superfluous cast
- merge code with the copy code
- add interrup-parent
- demove depth > 2 check
- reuse code from handle_device_interrupts
- copy interrupts from host dt
Changes in v2:
- rename "path" to "xen,path"
- grammar fix
- use gaddr_to_gfn and maddr_to_mfn
- remove depth <= 2 limitation in scanning the dtb fragment
- introduce and parse xen,reg
- code style
- support more than one interrupt per device
- specify only the GIC is supported
---
xen/arch/arm/domain_build.c | 104 ++--
1 file changed, 101 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 08d6d238e3..a461816345 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1699,6 +1699,88 @@ static int __init make_vpl011_uart_node(struct
kernel_info *kinfo)
}
#endif
+/*
+ * Scan device tree properties for passthrough specific information.
+ * Returns -ENOENT when no passthrough properties are found
Such things only work if you control the return value. Looking at the
code, you propagate the value from iommu_assign_dt_device(). While today
we return -EINVAL when the device is not protected, it may make sense to
return -ENOENT (after all the device is not behind an IOMMU and
therefore technically not found).
You would end up to present the property when it should not. So it would
be better to consider returning a positive value when the property needs
to be copied.
+ * < 0 on error
+ * 0 on success
+ */
+static int __init handle_passthrough_prop(struct kernel_info *kinfo,
+ const struct fdt_property *prop,
+ const char *name,
+ uint32_t address_cells, uint32_t
size_cells)
+{
+const __be32 *cell;
+unsigned int i, len;
+struct dt_device_node *node;
+int res;
+
+/* xen,reg specifies where to map the MMIO region */
+if ( dt_prop_cmp("xen,reg", name) == 0 )
+{
+paddr_t mstart, size, gstart;
NIT: Newline between declaration and code.
+cell = (const __be32 *)prop->data;
+len = fdt32_to_cpu(prop->len) /
+((address_cells * 2 + size_cells) * sizeof(uint32_t));
NIT: The indentation looks incorrect. I missed this one on the previous
review.
+
+for ( i = 0; i < len; i++ )
+{
+device_tree_get_reg(, address_cells, size_cells,
+, );
Same here. It might be worth checking your editor configuration to avoid
such things happening in the future...
+gstart = dt_next_cell(address_cells, );
+
+if ( gstart & ~PAGE_MASK || mstart & ~PAGE_MASK || size &
~PAGE_MASK )
+{
+printk(XENLOG_ERR
+ "DomU passthrough config has not page aligned
addresses/sizes\n");
+return -EINVAL;
+}
+
+res = map_regions_p2mt(kinfo->d,
+ gaddr_to_gfn(gstart),
+ PFN_DOWN(size),
+ maddr_to_mfn(mstart),
+ p2m_mmio_direct_dev);
+if ( res < 0 )
+{
+printk(XENLOG_ERR
+ "Failed to map %"PRIpaddr" to the guest
at%"PRIpaddr"\n",
+ mstart, gstart);
+return -EFAULT;
+}
+}
+
+return 0;
+}
+
[Xen-devel] [PATCH v6 5/8] xen/arm: assign devices to boot domains
Scan the user provided dtb fragment at boot. For each device node, map
memory to guests, and route interrupts and setup the iommu.
The memory region to remap is specified by the "xen,reg" property.
The iommu is setup by passing the node of the device to assign on the
host device tree. The path is specified in the device tree fragment as
the "xen,path" string property.
The interrupts are remapped based on the information from the
corresponding node on the host device tree. Call
handle_device_interrupts to remap interrupts. Interrupts related device
tree properties are copied from the device tree fragment, same as all
the other properties.
Also set add the new flag XEN_DOMCTL_CDF_iommu so that dom0less domU
can use the IOMMU if a partial dtb is specified.
Signed-off-by: Stefano Stabellini
---
Changes in v6:
- turn dprintks into printks
- return error on page alignment check failure
- set XEN_DOMCTL_CDF_iommu if partial dtb is specified
Changes in v5:
- use local variable for name
- use map_regions_p2mt
- add warning for not page aligned addresses/sizes
- introduce handle_passthrough_prop
Changes in v4:
- use unsigned
- improve commit message
- code style
- use dt_prop_cmp
- use device_tree_get_reg
- don't copy over xen,reg and xen,path
- don't create special interrupt properties for domU: copy them from the
fragment
- in-code comment
Changes in v3:
- improve commit message
- remove superfluous cast
- merge code with the copy code
- add interrup-parent
- demove depth > 2 check
- reuse code from handle_device_interrupts
- copy interrupts from host dt
Changes in v2:
- rename "path" to "xen,path"
- grammar fix
- use gaddr_to_gfn and maddr_to_mfn
- remove depth <= 2 limitation in scanning the dtb fragment
- introduce and parse xen,reg
- code style
- support more than one interrupt per device
- specify only the GIC is supported
---
xen/arch/arm/domain_build.c | 104 ++--
1 file changed, 101 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 08d6d238e3..a461816345 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1699,6 +1699,88 @@ static int __init make_vpl011_uart_node(struct
kernel_info *kinfo)
}
#endif
+/*
+ * Scan device tree properties for passthrough specific information.
+ * Returns -ENOENT when no passthrough properties are found
+ * < 0 on error
+ * 0 on success
+ */
+static int __init handle_passthrough_prop(struct kernel_info *kinfo,
+ const struct fdt_property *prop,
+ const char *name,
+ uint32_t address_cells, uint32_t
size_cells)
+{
+const __be32 *cell;
+unsigned int i, len;
+struct dt_device_node *node;
+int res;
+
+/* xen,reg specifies where to map the MMIO region */
+if ( dt_prop_cmp("xen,reg", name) == 0 )
+{
+paddr_t mstart, size, gstart;
+cell = (const __be32 *)prop->data;
+len = fdt32_to_cpu(prop->len) /
+((address_cells * 2 + size_cells) * sizeof(uint32_t));
+
+for ( i = 0; i < len; i++ )
+{
+device_tree_get_reg(, address_cells, size_cells,
+, );
+gstart = dt_next_cell(address_cells, );
+
+if ( gstart & ~PAGE_MASK || mstart & ~PAGE_MASK || size &
~PAGE_MASK )
+{
+printk(XENLOG_ERR
+ "DomU passthrough config has not page aligned
addresses/sizes\n");
+return -EINVAL;
+}
+
+res = map_regions_p2mt(kinfo->d,
+ gaddr_to_gfn(gstart),
+ PFN_DOWN(size),
+ maddr_to_mfn(mstart),
+ p2m_mmio_direct_dev);
+if ( res < 0 )
+{
+printk(XENLOG_ERR
+ "Failed to map %"PRIpaddr" to the guest
at%"PRIpaddr"\n",
+ mstart, gstart);
+return -EFAULT;
+}
+}
+
+return 0;
+}
+/*
+ * xen,path specifies the corresponding node in the host DT.
+ * Both interrupt mappings and IOMMU settings are based on it,
+ * as they are done based on the corresponding host DT node.
+ */
+else if ( dt_prop_cmp("xen,path", name) == 0 )
+{
+node = dt_find_node_by_path(prop->data);
+if ( node == NULL )
+{
+printk(XENLOG_ERR "Couldn't find node %s in host_dt!\n",
+ (char *)prop->data);
+return -EINVAL;
+}
+
+res = iommu_assign_dt_device(kinfo->d, node);
+if ( res < 0 )
+return res;
+
+res = handle_device_interrupts(kinfo->d, node, true);
+if ( res < 0 )
+return res;
+
+return 0;
+}
+
+return -ENOENT;
+}
+
static
