* [XEN][RFC PATCH v3 01/14] xen/arm/device: Remove __init from function type
2022-03-08 19:46 [XEN][RFC PATCH v3 00/14] dynamic node programming using overlay dtbo Vikram Garhwal
@ 2022-03-08 19:46 ` Vikram Garhwal
2022-03-14 12:31 ` Luca Fancellu
2022-03-08 19:46 ` [XEN][RFC PATCH v3 02/14] xen/arm: Add CONFIG_OVERLAY_DTB Vikram Garhwal
` (12 subsequent siblings)
13 siblings, 1 reply; 42+ messages in thread
From: Vikram Garhwal @ 2022-03-08 19:46 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk,
Vikram Garhwal, Volodymyr Babchuk
Change function type of following function to access during runtime:
1. map_irq_to_domain()
2. handle_device_interrupt()
3. map_range_to_domain()
4. unflatten_dt_node()
5. unflatten_device_tree()
Move map_irq_to_domain(), handle_device_interrupt() and map_range_to_domain() to
device.c.
These changes are done to support the dynamic programming of a nodes where an
overlay node will be added to fdt and unflattened node will be added to dt_host.
Furthermore, IRQ and mmio mapping will be done for the added node.
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
xen/arch/arm/device.c | 136 +++++++++++++++++++++++++++++
xen/arch/arm/domain_build.c | 142 -------------------------------
xen/arch/arm/include/asm/setup.h | 3 +
xen/common/device_tree.c | 20 ++---
xen/include/xen/device_tree.h | 5 ++
5 files changed, 154 insertions(+), 152 deletions(-)
diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 70cd6c1a19..0dfd33b33e 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -21,6 +21,9 @@
#include <xen/errno.h>
#include <xen/init.h>
#include <xen/lib.h>
+#include <xen/iocap.h>
+#include <asm/domain_build.h>
+#include <asm/setup.h>
extern const struct device_desc _sdevice[], _edevice[];
extern const struct acpi_device_desc _asdevice[], _aedevice[];
@@ -84,6 +87,139 @@ enum device_class device_get_class(const struct dt_device_node *dev)
return DEVICE_UNKNOWN;
}
+int map_irq_to_domain(struct domain *d, unsigned int irq,
+ bool need_mapping, const char *devname)
+{
+ int res;
+
+ res = irq_permit_access(d, irq);
+ if ( res )
+ {
+ printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n",
+ d->domain_id, irq);
+ return res;
+ }
+
+ if ( need_mapping )
+ {
+ /*
+ * Checking the return of vgic_reserve_virq is not
+ * necessary. It should not fail except when we try to map
+ * the IRQ twice. This can legitimately happen if the IRQ is shared
+ */
+ vgic_reserve_virq(d, irq);
+
+ res = route_irq_to_guest(d, irq, irq, devname);
+ if ( res < 0 )
+ {
+ printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
+ irq, d->domain_id);
+ return res;
+ }
+ }
+
+ dt_dprintk(" - IRQ: %u\n", irq);
+ return 0;
+}
+
+int map_range_to_domain(const struct dt_device_node *dev,
+ u64 addr, u64 len, void *data)
+{
+ struct map_range_data *mr_data = data;
+ struct domain *d = mr_data->d;
+ int res;
+
+ res = iomem_permit_access(d, paddr_to_pfn(addr),
+ paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
+ if ( res )
+ {
+ printk(XENLOG_ERR "Unable to permit to dom%d access to"
+ " 0x%"PRIx64" - 0x%"PRIx64"\n",
+ d->domain_id,
+ addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
+ return res;
+ }
+
+ if ( !mr_data->skip_mapping )
+ {
+ res = map_regions_p2mt(d,
+ gaddr_to_gfn(addr),
+ PFN_UP(len),
+ maddr_to_mfn(addr),
+ mr_data->p2mt);
+
+ if ( res < 0 )
+ {
+ printk(XENLOG_ERR "Unable to map 0x%"PRIx64
+ " - 0x%"PRIx64" in domain %d\n",
+ addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
+ d->domain_id);
+ return res;
+ }
+ }
+
+ dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
+ addr, addr + len, mr_data->p2mt);
+
+ return 0;
+}
+
+/*
+ * handle_device_interrupts retrieves the interrupts configuration from
+ * a device tree node and maps those interrupts to the target domain.
+ *
+ * Returns:
+ * < 0 error
+ * 0 success
+ */
+int handle_device_interrupts(struct domain *d,
+ struct dt_device_node *dev,
+ bool need_mapping)
+{
+ unsigned int i, nirq;
+ int res;
+ struct dt_raw_irq rirq;
+
+ nirq = dt_number_of_irq(dev);
+
+ /* Give permission and map IRQs */
+ for ( i = 0; i < nirq; i++ )
+ {
+ res = dt_device_get_raw_irq(dev, i, &rirq);
+ if ( res )
+ {
+ printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
+ i, dt_node_full_name(dev));
+ return res;
+ }
+
+ /*
+ * Don't map IRQ that have no physical meaning
+ * ie: IRQ whose controller is not the GIC
+ */
+ if ( rirq.controller != dt_interrupt_controller )
+ {
+ dt_dprintk("irq %u not connected to primary controller. Connected to %s\n",
+ i, dt_node_full_name(rirq.controller));
+ continue;
+ }
+
+ res = platform_get_irq(dev, i);
+ if ( res < 0 )
+ {
+ printk(XENLOG_ERR "Unable to get irq %u for %s\n",
+ i, dt_node_full_name(dev));
+ return res;
+ }
+
+ res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev));
+ if ( res )
+ return res;
+ }
+
+ return 0;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 8be01678de..b06770a2af 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1794,41 +1794,6 @@ int __init make_chosen_node(const struct kernel_info *kinfo)
return res;
}
-int __init map_irq_to_domain(struct domain *d, unsigned int irq,
- bool need_mapping, const char *devname)
-{
- int res;
-
- res = irq_permit_access(d, irq);
- if ( res )
- {
- printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n",
- d->domain_id, irq);
- return res;
- }
-
- if ( need_mapping )
- {
- /*
- * Checking the return of vgic_reserve_virq is not
- * necessary. It should not fail except when we try to map
- * the IRQ twice. This can legitimately happen if the IRQ is shared
- */
- vgic_reserve_virq(d, irq);
-
- res = route_irq_to_guest(d, irq, irq, devname);
- if ( res < 0 )
- {
- printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
- irq, d->domain_id);
- return res;
- }
- }
-
- dt_dprintk(" - IRQ: %u\n", irq);
- return 0;
-}
-
static int __init map_dt_irq_to_domain(const struct dt_device_node *dev,
const struct dt_irq *dt_irq,
void *data)
@@ -1860,57 +1825,6 @@ static int __init map_dt_irq_to_domain(const struct dt_device_node *dev,
return 0;
}
-int __init map_range_to_domain(const struct dt_device_node *dev,
- u64 addr, u64 len, void *data)
-{
- struct map_range_data *mr_data = data;
- struct domain *d = mr_data->d;
- int res;
-
- /*
- * reserved-memory regions are RAM carved out for a special purpose.
- * They are not MMIO and therefore a domain should not be able to
- * manage them via the IOMEM interface.
- */
- if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/",
- strlen("/reserved-memory/")) != 0 )
- {
- res = iomem_permit_access(d, paddr_to_pfn(addr),
- paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
- if ( res )
- {
- printk(XENLOG_ERR "Unable to permit to dom%d access to"
- " 0x%"PRIx64" - 0x%"PRIx64"\n",
- d->domain_id,
- addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
- return res;
- }
- }
-
- if ( !mr_data->skip_mapping )
- {
- res = map_regions_p2mt(d,
- gaddr_to_gfn(addr),
- PFN_UP(len),
- maddr_to_mfn(addr),
- mr_data->p2mt);
-
- if ( res < 0 )
- {
- printk(XENLOG_ERR "Unable to map 0x%"PRIx64
- " - 0x%"PRIx64" in domain %d\n",
- addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
- d->domain_id);
- return res;
- }
- }
-
- dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
- addr, addr + len, mr_data->p2mt);
-
- return 0;
-}
-
/*
* For a node which describes a discoverable bus (such as a PCI bus)
* then we may need to perform additional mappings in order to make
@@ -1938,62 +1852,6 @@ static int __init map_device_children(const struct dt_device_node *dev,
return 0;
}
-/*
- * handle_device_interrupts retrieves the interrupts configuration from
- * a device tree node and maps those interrupts to the target domain.
- *
- * Returns:
- * < 0 error
- * 0 success
- */
-static int __init handle_device_interrupts(struct domain *d,
- struct dt_device_node *dev,
- bool need_mapping)
-{
- unsigned int i, nirq;
- int res;
- struct dt_raw_irq rirq;
-
- nirq = dt_number_of_irq(dev);
-
- /* Give permission and map IRQs */
- for ( i = 0; i < nirq; i++ )
- {
- res = dt_device_get_raw_irq(dev, i, &rirq);
- if ( res )
- {
- printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
- i, dt_node_full_name(dev));
- return res;
- }
-
- /*
- * Don't map IRQ that have no physical meaning
- * ie: IRQ whose controller is not the GIC
- */
- if ( rirq.controller != dt_interrupt_controller )
- {
- dt_dprintk("irq %u not connected to primary controller. Connected to %s\n",
- i, dt_node_full_name(rirq.controller));
- continue;
- }
-
- res = platform_get_irq(dev, i);
- if ( res < 0 )
- {
- printk(XENLOG_ERR "Unable to get irq %u for %s\n",
- i, dt_node_full_name(dev));
- return res;
- }
-
- res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev));
- if ( res )
- return res;
- }
-
- return 0;
-}
-
/*
* For a given device node:
* - Give permission to the guest to manage IRQ and MMIO range
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 7a1e1d6798..8a26f1845c 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -134,6 +134,9 @@ void device_tree_get_reg(const __be32 **cell, u32 address_cells,
u32 device_tree_get_u32(const void *fdt, int node,
const char *prop_name, u32 dflt);
+int handle_device_interrupts(struct domain *d, struct dt_device_node *dev,
+ bool need_mapping);
+
int map_range_to_domain(const struct dt_device_node *dev,
u64 addr, u64 len, void *data);
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 4aae281e89..f43d66a501 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -1811,12 +1811,12 @@ int dt_count_phandle_with_args(const struct dt_device_node *np,
* @allnextpp: pointer to ->allnext from last allocated device_node
* @fpsize: Size of the node path up at the current depth.
*/
-static unsigned long __init unflatten_dt_node(const void *fdt,
- unsigned long mem,
- unsigned long *p,
- struct dt_device_node *dad,
- struct dt_device_node ***allnextpp,
- unsigned long fpsize)
+static unsigned long unflatten_dt_node(const void *fdt,
+ unsigned long mem,
+ unsigned long *p,
+ struct dt_device_node *dad,
+ struct dt_device_node ***allnextpp,
+ unsigned long fpsize)
{
struct dt_device_node *np;
struct dt_property *pp, **prev_pp = NULL;
@@ -2047,7 +2047,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
}
/**
- * __unflatten_device_tree - create tree of device_nodes from flat blob
+ * unflatten_device_tree - create tree of device_nodes from flat blob
*
* unflattens a device-tree, creating the
* tree of struct device_node. It also fills the "name" and "type"
@@ -2056,8 +2056,8 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
* @fdt: The fdt to expand
* @mynodes: The device_node tree created by the call
*/
-static void __init __unflatten_device_tree(const void *fdt,
- struct dt_device_node **mynodes)
+void unflatten_device_tree(const void *fdt,
+ struct dt_device_node **mynodes)
{
unsigned long start, mem, size;
struct dt_device_node **allnextp = mynodes;
@@ -2179,7 +2179,7 @@ dt_find_interrupt_controller(const struct dt_device_match *matches)
void __init dt_unflatten_host_device_tree(void)
{
- __unflatten_device_tree(device_tree_flattened, &dt_host);
+ unflatten_device_tree(device_tree_flattened, &dt_host);
dt_alias_scan();
}
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index fd6cd00b43..06d7866c10 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -177,6 +177,11 @@ int device_tree_for_each_node(const void *fdt, int node,
*/
void dt_unflatten_host_device_tree(void);
+/*
+ * unflatten any device tree.
+ */
+void unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes);
+
/**
* IRQ translation callback
* TODO: For the moment we assume that we only have ONE
--
2.17.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 01/14] xen/arm/device: Remove __init from function type
2022-03-08 19:46 ` [XEN][RFC PATCH v3 01/14] xen/arm/device: Remove __init from function type Vikram Garhwal
@ 2022-03-14 12:31 ` Luca Fancellu
2022-03-15 22:29 ` Vikram Garhwal
2022-03-15 22:42 ` Vikram Garhwal
0 siblings, 2 replies; 42+ messages in thread
From: Luca Fancellu @ 2022-03-14 12:31 UTC (permalink / raw)
To: Vikram Garhwal
Cc: Xen-devel, sstabellini, julien, Bertrand Marquis, volodymyr_babchuk
> On 8 Mar 2022, at 19:46, Vikram Garhwal <fnu.vikram@xilinx.com> wrote:
>
> Change function type of following function to access during runtime:
> 1. map_irq_to_domain()
> 2. handle_device_interrupt()
> 3. map_range_to_domain()
> 4. unflatten_dt_node()
> 5. unflatten_device_tree()
>
> Move map_irq_to_domain(), handle_device_interrupt() and map_range_to_domain() to
> device.c.
>
> These changes are done to support the dynamic programming of a nodes where an
> overlay node will be added to fdt and unflattened node will be added to dt_host.
> Furthermore, IRQ and mmio mapping will be done for the added node.
>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
> xen/arch/arm/device.c | 136 +++++++++++++++++++++++++++++
> xen/arch/arm/domain_build.c | 142 -------------------------------
> xen/arch/arm/include/asm/setup.h | 3 +
> xen/common/device_tree.c | 20 ++---
> xen/include/xen/device_tree.h | 5 ++
> 5 files changed, 154 insertions(+), 152 deletions(-)
>
> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> index 70cd6c1a19..0dfd33b33e 100644
> --- a/xen/arch/arm/device.c
> +++ b/xen/arch/arm/device.c
> @@ -21,6 +21,9 @@
> #include <xen/errno.h>
> #include <xen/init.h>
> #include <xen/lib.h>
> +#include <xen/iocap.h>
> +#include <asm/domain_build.h>
> +#include <asm/setup.h>
>
> extern const struct device_desc _sdevice[], _edevice[];
> extern const struct acpi_device_desc _asdevice[], _aedevice[];
> @@ -84,6 +87,139 @@ enum device_class device_get_class(const struct dt_device_node *dev)
> return DEVICE_UNKNOWN;
> }
>
> +int map_irq_to_domain(struct domain *d, unsigned int irq,
> + bool need_mapping, const char *devname)
> +{
> + int res;
> +
> + res = irq_permit_access(d, irq);
> + if ( res )
> + {
> + printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n",
> + d->domain_id, irq);
> + return res;
> + }
> +
> + if ( need_mapping )
> + {
> + /*
> + * Checking the return of vgic_reserve_virq is not
> + * necessary. It should not fail except when we try to map
> + * the IRQ twice. This can legitimately happen if the IRQ is shared
> + */
> + vgic_reserve_virq(d, irq);
> +
> + res = route_irq_to_guest(d, irq, irq, devname);
> + if ( res < 0 )
> + {
> + printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
> + irq, d->domain_id);
> + return res;
> + }
> + }
> +
> + dt_dprintk(" - IRQ: %u\n", irq);
> + return 0;
> +}
> +
> +int map_range_to_domain(const struct dt_device_node *dev,
> + u64 addr, u64 len, void *data)
> +{
> + struct map_range_data *mr_data = data;
> + struct domain *d = mr_data->d;
> + int res;
> +
> + res = iomem_permit_access(d, paddr_to_pfn(addr),
> + paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
Hi Vikram,
Why the if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/",
strlen("/reserved-memory/")) != 0 ) was dropped?
> + if ( res )
> + {
> + printk(XENLOG_ERR "Unable to permit to dom%d access to"
> + " 0x%"PRIx64" - 0x%"PRIx64"\n",
> + d->domain_id,
> + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
> + return res;
> + }
> +
> + if ( !mr_data->skip_mapping )
> + {
> + res = map_regions_p2mt(d,
> + gaddr_to_gfn(addr),
> + PFN_UP(len),
> + maddr_to_mfn(addr),
> + mr_data->p2mt);
> +
> + if ( res < 0 )
> + {
> + printk(XENLOG_ERR "Unable to map 0x%"PRIx64
> + " - 0x%"PRIx64" in domain %d\n",
> + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
> + d->domain_id);
> + return res;
> + }
> + }
> +
> + dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
> + addr, addr + len, mr_data->p2mt);
> +
> + return 0;
> +}
> +
> +/*
> + * handle_device_interrupts retrieves the interrupts configuration from
> + * a device tree node and maps those interrupts to the target domain.
> + *
> + * Returns:
> + * < 0 error
> + * 0 success
> + */
> +int handle_device_interrupts(struct domain *d,
> + struct dt_device_node *dev,
> + bool need_mapping)
> +{
> + unsigned int i, nirq;
> + int res;
> + struct dt_raw_irq rirq;
> +
> + nirq = dt_number_of_irq(dev);
> +
> + /* Give permission and map IRQs */
> + for ( i = 0; i < nirq; i++ )
> + {
> + res = dt_device_get_raw_irq(dev, i, &rirq);
> + if ( res )
> + {
> + printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
> + i, dt_node_full_name(dev));
> + return res;
> + }
> +
> + /*
> + * Don't map IRQ that have no physical meaning
> + * ie: IRQ whose controller is not the GIC
> + */
> + if ( rirq.controller != dt_interrupt_controller )
> + {
> + dt_dprintk("irq %u not connected to primary controller. Connected to %s\n",
> + i, dt_node_full_name(rirq.controller));
> + continue;
> + }
> +
> + res = platform_get_irq(dev, i);
> + if ( res < 0 )
> + {
> + printk(XENLOG_ERR "Unable to get irq %u for %s\n",
> + i, dt_node_full_name(dev));
> + return res;
> + }
> +
> + res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev));
> + if ( res )
> + return res;
> + }
> +
> + return 0;
> +}
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 8be01678de..b06770a2af 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1794,41 +1794,6 @@ int __init make_chosen_node(const struct kernel_info *kinfo)
> return res;
> }
>
> -int __init map_irq_to_domain(struct domain *d, unsigned int irq,
> - bool need_mapping, const char *devname)
> -{
> - int res;
> -
> - res = irq_permit_access(d, irq);
> - if ( res )
> - {
> - printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n",
> - d->domain_id, irq);
> - return res;
> - }
> -
> - if ( need_mapping )
> - {
> - /*
> - * Checking the return of vgic_reserve_virq is not
> - * necessary. It should not fail except when we try to map
> - * the IRQ twice. This can legitimately happen if the IRQ is shared
> - */
> - vgic_reserve_virq(d, irq);
> -
> - res = route_irq_to_guest(d, irq, irq, devname);
> - if ( res < 0 )
> - {
> - printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
> - irq, d->domain_id);
> - return res;
> - }
> - }
> -
> - dt_dprintk(" - IRQ: %u\n", irq);
> - return 0;
> -}
> -
> static int __init map_dt_irq_to_domain(const struct dt_device_node *dev,
> const struct dt_irq *dt_irq,
> void *data)
> @@ -1860,57 +1825,6 @@ static int __init map_dt_irq_to_domain(const struct dt_device_node *dev,
> return 0;
> }
>
> -int __init map_range_to_domain(const struct dt_device_node *dev,
> - u64 addr, u64 len, void *data)
> -{
> - struct map_range_data *mr_data = data;
> - struct domain *d = mr_data->d;
> - int res;
> -
> - /*
> - * reserved-memory regions are RAM carved out for a special purpose.
> - * They are not MMIO and therefore a domain should not be able to
> - * manage them via the IOMEM interface.
> - */
> - if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/",
> - strlen("/reserved-memory/")) != 0 )
> - {
> - res = iomem_permit_access(d, paddr_to_pfn(addr),
> - paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> - if ( res )
> - {
> - printk(XENLOG_ERR "Unable to permit to dom%d access to"
> - " 0x%"PRIx64" - 0x%"PRIx64"\n",
> - d->domain_id,
> - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
> - return res;
> - }
> - }
> -
> - if ( !mr_data->skip_mapping )
> - {
> - res = map_regions_p2mt(d,
> - gaddr_to_gfn(addr),
> - PFN_UP(len),
> - maddr_to_mfn(addr),
> - mr_data->p2mt);
> -
> - if ( res < 0 )
> - {
> - printk(XENLOG_ERR "Unable to map 0x%"PRIx64
> - " - 0x%"PRIx64" in domain %d\n",
> - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
> - d->domain_id);
> - return res;
> - }
> - }
> -
> - dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
> - addr, addr + len, mr_data->p2mt);
> -
> - return 0;
> -}
> -
> /*
> * For a node which describes a discoverable bus (such as a PCI bus)
> * then we may need to perform additional mappings in order to make
> @@ -1938,62 +1852,6 @@ static int __init map_device_children(const struct dt_device_node *dev,
> return 0;
> }
>
> -/*
> - * handle_device_interrupts retrieves the interrupts configuration from
> - * a device tree node and maps those interrupts to the target domain.
> - *
> - * Returns:
> - * < 0 error
> - * 0 success
> - */
> -static int __init handle_device_interrupts(struct domain *d,
> - struct dt_device_node *dev,
> - bool need_mapping)
> -{
> - unsigned int i, nirq;
> - int res;
> - struct dt_raw_irq rirq;
> -
> - nirq = dt_number_of_irq(dev);
> -
> - /* Give permission and map IRQs */
> - for ( i = 0; i < nirq; i++ )
> - {
> - res = dt_device_get_raw_irq(dev, i, &rirq);
> - if ( res )
> - {
> - printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
> - i, dt_node_full_name(dev));
> - return res;
> - }
> -
> - /*
> - * Don't map IRQ that have no physical meaning
> - * ie: IRQ whose controller is not the GIC
> - */
> - if ( rirq.controller != dt_interrupt_controller )
> - {
> - dt_dprintk("irq %u not connected to primary controller. Connected to %s\n",
> - i, dt_node_full_name(rirq.controller));
> - continue;
> - }
> -
> - res = platform_get_irq(dev, i);
> - if ( res < 0 )
> - {
> - printk(XENLOG_ERR "Unable to get irq %u for %s\n",
> - i, dt_node_full_name(dev));
> - return res;
> - }
> -
> - res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev));
> - if ( res )
> - return res;
> - }
> -
> - return 0;
> -}
> -
> /*
> * For a given device node:
> * - Give permission to the guest to manage IRQ and MMIO range
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index 7a1e1d6798..8a26f1845c 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -134,6 +134,9 @@ void device_tree_get_reg(const __be32 **cell, u32 address_cells,
> u32 device_tree_get_u32(const void *fdt, int node,
> const char *prop_name, u32 dflt);
>
> +int handle_device_interrupts(struct domain *d, struct dt_device_node *dev,
> + bool need_mapping);
> +
> int map_range_to_domain(const struct dt_device_node *dev,
> u64 addr, u64 len, void *data);
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 4aae281e89..f43d66a501 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -1811,12 +1811,12 @@ int dt_count_phandle_with_args(const struct dt_device_node *np,
> * @allnextpp: pointer to ->allnext from last allocated device_node
> * @fpsize: Size of the node path up at the current depth.
> */
> -static unsigned long __init unflatten_dt_node(const void *fdt,
> - unsigned long mem,
> - unsigned long *p,
> - struct dt_device_node *dad,
> - struct dt_device_node ***allnextpp,
> - unsigned long fpsize)
> +static unsigned long unflatten_dt_node(const void *fdt,
> + unsigned long mem,
> + unsigned long *p,
> + struct dt_device_node *dad,
> + struct dt_device_node ***allnextpp,
> + unsigned long fpsize)
> {
> struct dt_device_node *np;
> struct dt_property *pp, **prev_pp = NULL;
> @@ -2047,7 +2047,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
> }
>
> /**
> - * __unflatten_device_tree - create tree of device_nodes from flat blob
> + * unflatten_device_tree - create tree of device_nodes from flat blob
> *
> * unflattens a device-tree, creating the
> * tree of struct device_node. It also fills the "name" and "type"
> @@ -2056,8 +2056,8 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
> * @fdt: The fdt to expand
> * @mynodes: The device_node tree created by the call
> */
> -static void __init __unflatten_device_tree(const void *fdt,
> - struct dt_device_node **mynodes)
> +void unflatten_device_tree(const void *fdt,
> + struct dt_device_node **mynodes)
> {
> unsigned long start, mem, size;
> struct dt_device_node **allnextp = mynodes;
> @@ -2179,7 +2179,7 @@ dt_find_interrupt_controller(const struct dt_device_match *matches)
>
> void __init dt_unflatten_host_device_tree(void)
> {
> - __unflatten_device_tree(device_tree_flattened, &dt_host);
> + unflatten_device_tree(device_tree_flattened, &dt_host);
> dt_alias_scan();
> }
>
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index fd6cd00b43..06d7866c10 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -177,6 +177,11 @@ int device_tree_for_each_node(const void *fdt, int node,
> */
> void dt_unflatten_host_device_tree(void);
>
> +/*
> + * unflatten any device tree.
> + */
> +void unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes);
> +
> /**
> * IRQ translation callback
> * TODO: For the moment we assume that we only have ONE
NIT: there are some minor code style issues, like indentation that could be fixed
Cheers,
Luca
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 01/14] xen/arm/device: Remove __init from function type
2022-03-14 12:31 ` Luca Fancellu
@ 2022-03-15 22:29 ` Vikram Garhwal
2022-03-15 22:42 ` Vikram Garhwal
1 sibling, 0 replies; 42+ messages in thread
From: Vikram Garhwal @ 2022-03-15 22:29 UTC (permalink / raw)
To: Luca Fancellu
Cc: Vikram Garhwal, Xen-devel, sstabellini, julien, Bertrand Marquis,
volodymyr_babchuk
On Mon, Mar 14, 2022 at 12:31:06PM +0000, Luca Fancellu wrote:
>
>
> > On 8 Mar 2022, at 19:46, Vikram Garhwal <fnu.vikram@xilinx.com> wrote:
> >
> > Change function type of following function to access during runtime:
> > 1. map_irq_to_domain()
> > 2. handle_device_interrupt()
> > 3. map_range_to_domain()
> > 4. unflatten_dt_node()
> > 5. unflatten_device_tree()
> >
> > Move map_irq_to_domain(), handle_device_interrupt() and map_range_to_domain() to
> > device.c.
> >
> > These changes are done to support the dynamic programming of a nodes where an
> > overlay node will be added to fdt and unflattened node will be added to dt_host.
> > Furthermore, IRQ and mmio mapping will be done for the added node.
> >
> > Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> > ---
> > xen/arch/arm/device.c | 136 +++++++++++++++++++++++++++++
> > xen/arch/arm/domain_build.c | 142 -------------------------------
> > xen/arch/arm/include/asm/setup.h | 3 +
> > xen/common/device_tree.c | 20 ++---
> > xen/include/xen/device_tree.h | 5 ++
> > 5 files changed, 154 insertions(+), 152 deletions(-)
> >
> > diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> > index 70cd6c1a19..0dfd33b33e 100644
> > --- a/xen/arch/arm/device.c
> > +++ b/xen/arch/arm/device.c
> > @@ -21,6 +21,9 @@
> > #include <xen/errno.h>
> > #include <xen/init.h>
> > #include <xen/lib.h>
> > +#include <xen/iocap.h>
> > +#include <asm/domain_build.h>
> > +#include <asm/setup.h>
> >
> > extern const struct device_desc _sdevice[], _edevice[];
> > extern const struct acpi_device_desc _asdevice[], _aedevice[];
> > @@ -84,6 +87,139 @@ enum device_class device_get_class(const struct dt_device_node *dev)
> > return DEVICE_UNKNOWN;
> > }
> >
> > +int map_irq_to_domain(struct domain *d, unsigned int irq,
> > + bool need_mapping, const char *devname)
> > +{
> > + int res;
> > +
> > + res = irq_permit_access(d, irq);
> > + if ( res )
> > + {
> > + printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n",
> > + d->domain_id, irq);
> > + return res;
> > + }
> > +
> > + if ( need_mapping )
> > + {
> > + /*
> > + * Checking the return of vgic_reserve_virq is not
> > + * necessary. It should not fail except when we try to map
> > + * the IRQ twice. This can legitimately happen if the IRQ is shared
> > + */
> > + vgic_reserve_virq(d, irq);
> > +
> > + res = route_irq_to_guest(d, irq, irq, devname);
> > + if ( res < 0 )
> > + {
> > + printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
> > + irq, d->domain_id);
> > + return res;
> > + }
> > + }
> > +
> > + dt_dprintk(" - IRQ: %u\n", irq);
> > + return 0;
> > +}
> > +
> > +int map_range_to_domain(const struct dt_device_node *dev,
> > + u64 addr, u64 len, void *data)
> > +{
> > + struct map_range_data *mr_data = data;
> > + struct domain *d = mr_data->d;
> > + int res;
> > +
> > + res = iomem_permit_access(d, paddr_to_pfn(addr),
> > + paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
>
> Hi Vikram,
>
> Why the if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/",
> strlen("/reserved-memory/")) != 0 ) was dropped?
>
Hi Luca,
Thanks for spotting this. Yeah, I messed this while porting the patches from
internal to upstream Xen.
Will address this in v4!
>
> > + if ( res )
> > + {
> > + printk(XENLOG_ERR "Unable to permit to dom%d access to"
> > + " 0x%"PRIx64" - 0x%"PRIx64"\n",
> > + d->domain_id,
> > + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
> > + return res;
> > + }
> > +
> > + if ( !mr_data->skip_mapping )
> > + {
> > + res = map_regions_p2mt(d,
> > + gaddr_to_gfn(addr),
> > + PFN_UP(len),
> > + maddr_to_mfn(addr),
> > + mr_data->p2mt);
> > +
> > + if ( res < 0 )
> > + {
> > + printk(XENLOG_ERR "Unable to map 0x%"PRIx64
> > + " - 0x%"PRIx64" in domain %d\n",
> > + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
> > + d->domain_id);
> > + return res;
> > + }
> > + }
> > +
> > + dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
> > + addr, addr + len, mr_data->p2mt);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * handle_device_interrupts retrieves the interrupts configuration from
> > + * a device tree node and maps those interrupts to the target domain.
> > + *
> > + * Returns:
> > + * < 0 error
> > + * 0 success
> > + */
> > +int handle_device_interrupts(struct domain *d,
> > + struct dt_device_node *dev,
> > + bool need_mapping)
> > +{
> > + unsigned int i, nirq;
> > + int res;
> > + struct dt_raw_irq rirq;
> > +
> > + nirq = dt_number_of_irq(dev);
> > +
> > + /* Give permission and map IRQs */
> > + for ( i = 0; i < nirq; i++ )
> > + {
> > + res = dt_device_get_raw_irq(dev, i, &rirq);
> > + if ( res )
> > + {
> > + printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
> > + i, dt_node_full_name(dev));
> > + return res;
> > + }
> > +
> > + /*
> > + * Don't map IRQ that have no physical meaning
> > + * ie: IRQ whose controller is not the GIC
> > + */
> > + if ( rirq.controller != dt_interrupt_controller )
> > + {
> > + dt_dprintk("irq %u not connected to primary controller. Connected to %s\n",
> > + i, dt_node_full_name(rirq.controller));
> > + continue;
> > + }
> > +
> > + res = platform_get_irq(dev, i);
> > + if ( res < 0 )
> > + {
> > + printk(XENLOG_ERR "Unable to get irq %u for %s\n",
> > + i, dt_node_full_name(dev));
> > + return res;
> > + }
> > +
> > + res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev));
> > + if ( res )
> > + return res;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /*
> > * Local variables:
> > * mode: C
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 8be01678de..b06770a2af 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1794,41 +1794,6 @@ int __init make_chosen_node(const struct kernel_info *kinfo)
> > return res;
> > }
> >
> > -int __init map_irq_to_domain(struct domain *d, unsigned int irq,
> > - bool need_mapping, const char *devname)
> > -{
> > - int res;
> > -
> > - res = irq_permit_access(d, irq);
> > - if ( res )
> > - {
> > - printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n",
> > - d->domain_id, irq);
> > - return res;
> > - }
> > -
> > - if ( need_mapping )
> > - {
> > - /*
> > - * Checking the return of vgic_reserve_virq is not
> > - * necessary. It should not fail except when we try to map
> > - * the IRQ twice. This can legitimately happen if the IRQ is shared
> > - */
> > - vgic_reserve_virq(d, irq);
> > -
> > - res = route_irq_to_guest(d, irq, irq, devname);
> > - if ( res < 0 )
> > - {
> > - printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
> > - irq, d->domain_id);
> > - return res;
> > - }
> > - }
> > -
> > - dt_dprintk(" - IRQ: %u\n", irq);
> > - return 0;
> > -}
> > -
> > static int __init map_dt_irq_to_domain(const struct dt_device_node *dev,
> > const struct dt_irq *dt_irq,
> > void *data)
> > @@ -1860,57 +1825,6 @@ static int __init map_dt_irq_to_domain(const struct dt_device_node *dev,
> > return 0;
> > }
> >
> > -int __init map_range_to_domain(const struct dt_device_node *dev,
> > - u64 addr, u64 len, void *data)
> > -{
> > - struct map_range_data *mr_data = data;
> > - struct domain *d = mr_data->d;
> > - int res;
> > -
> > - /*
> > - * reserved-memory regions are RAM carved out for a special purpose.
> > - * They are not MMIO and therefore a domain should not be able to
> > - * manage them via the IOMEM interface.
> > - */
> > - if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/",
> > - strlen("/reserved-memory/")) != 0 )
> > - {
> > - res = iomem_permit_access(d, paddr_to_pfn(addr),
> > - paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> > - if ( res )
> > - {
> > - printk(XENLOG_ERR "Unable to permit to dom%d access to"
> > - " 0x%"PRIx64" - 0x%"PRIx64"\n",
> > - d->domain_id,
> > - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
> > - return res;
> > - }
> > - }
> > -
> > - if ( !mr_data->skip_mapping )
> > - {
> > - res = map_regions_p2mt(d,
> > - gaddr_to_gfn(addr),
> > - PFN_UP(len),
> > - maddr_to_mfn(addr),
> > - mr_data->p2mt);
> > -
> > - if ( res < 0 )
> > - {
> > - printk(XENLOG_ERR "Unable to map 0x%"PRIx64
> > - " - 0x%"PRIx64" in domain %d\n",
> > - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
> > - d->domain_id);
> > - return res;
> > - }
> > - }
> > -
> > - dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
> > - addr, addr + len, mr_data->p2mt);
> > -
> > - return 0;
> > -}
> > -
> > /*
> > * For a node which describes a discoverable bus (such as a PCI bus)
> > * then we may need to perform additional mappings in order to make
> > @@ -1938,62 +1852,6 @@ static int __init map_device_children(const struct dt_device_node *dev,
> > return 0;
> > }
> >
> > -/*
> > - * handle_device_interrupts retrieves the interrupts configuration from
> > - * a device tree node and maps those interrupts to the target domain.
> > - *
> > - * Returns:
> > - * < 0 error
> > - * 0 success
> > - */
> > -static int __init handle_device_interrupts(struct domain *d,
> > - struct dt_device_node *dev,
> > - bool need_mapping)
> > -{
> > - unsigned int i, nirq;
> > - int res;
> > - struct dt_raw_irq rirq;
> > -
> > - nirq = dt_number_of_irq(dev);
> > -
> > - /* Give permission and map IRQs */
> > - for ( i = 0; i < nirq; i++ )
> > - {
> > - res = dt_device_get_raw_irq(dev, i, &rirq);
> > - if ( res )
> > - {
> > - printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
> > - i, dt_node_full_name(dev));
> > - return res;
> > - }
> > -
> > - /*
> > - * Don't map IRQ that have no physical meaning
> > - * ie: IRQ whose controller is not the GIC
> > - */
> > - if ( rirq.controller != dt_interrupt_controller )
> > - {
> > - dt_dprintk("irq %u not connected to primary controller. Connected to %s\n",
> > - i, dt_node_full_name(rirq.controller));
> > - continue;
> > - }
> > -
> > - res = platform_get_irq(dev, i);
> > - if ( res < 0 )
> > - {
> > - printk(XENLOG_ERR "Unable to get irq %u for %s\n",
> > - i, dt_node_full_name(dev));
> > - return res;
> > - }
> > -
> > - res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev));
> > - if ( res )
> > - return res;
> > - }
> > -
> > - return 0;
> > -}
> > -
> > /*
> > * For a given device node:
> > * - Give permission to the guest to manage IRQ and MMIO range
> > diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> > index 7a1e1d6798..8a26f1845c 100644
> > --- a/xen/arch/arm/include/asm/setup.h
> > +++ b/xen/arch/arm/include/asm/setup.h
> > @@ -134,6 +134,9 @@ void device_tree_get_reg(const __be32 **cell, u32 address_cells,
> > u32 device_tree_get_u32(const void *fdt, int node,
> > const char *prop_name, u32 dflt);
> >
> > +int handle_device_interrupts(struct domain *d, struct dt_device_node *dev,
> > + bool need_mapping);
> > +
> > int map_range_to_domain(const struct dt_device_node *dev,
> > u64 addr, u64 len, void *data);
> >
> > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > index 4aae281e89..f43d66a501 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -1811,12 +1811,12 @@ int dt_count_phandle_with_args(const struct dt_device_node *np,
> > * @allnextpp: pointer to ->allnext from last allocated device_node
> > * @fpsize: Size of the node path up at the current depth.
> > */
> > -static unsigned long __init unflatten_dt_node(const void *fdt,
> > - unsigned long mem,
> > - unsigned long *p,
> > - struct dt_device_node *dad,
> > - struct dt_device_node ***allnextpp,
> > - unsigned long fpsize)
> > +static unsigned long unflatten_dt_node(const void *fdt,
> > + unsigned long mem,
> > + unsigned long *p,
> > + struct dt_device_node *dad,
> > + struct dt_device_node ***allnextpp,
> > + unsigned long fpsize)
> > {
> > struct dt_device_node *np;
> > struct dt_property *pp, **prev_pp = NULL;
> > @@ -2047,7 +2047,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
> > }
> >
> > /**
> > - * __unflatten_device_tree - create tree of device_nodes from flat blob
> > + * unflatten_device_tree - create tree of device_nodes from flat blob
> > *
> > * unflattens a device-tree, creating the
> > * tree of struct device_node. It also fills the "name" and "type"
> > @@ -2056,8 +2056,8 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
> > * @fdt: The fdt to expand
> > * @mynodes: The device_node tree created by the call
> > */
> > -static void __init __unflatten_device_tree(const void *fdt,
> > - struct dt_device_node **mynodes)
> > +void unflatten_device_tree(const void *fdt,
> > + struct dt_device_node **mynodes)
> > {
> > unsigned long start, mem, size;
> > struct dt_device_node **allnextp = mynodes;
> > @@ -2179,7 +2179,7 @@ dt_find_interrupt_controller(const struct dt_device_match *matches)
> >
> > void __init dt_unflatten_host_device_tree(void)
> > {
> > - __unflatten_device_tree(device_tree_flattened, &dt_host);
> > + unflatten_device_tree(device_tree_flattened, &dt_host);
> > dt_alias_scan();
> > }
> >
> > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> > index fd6cd00b43..06d7866c10 100644
> > --- a/xen/include/xen/device_tree.h
> > +++ b/xen/include/xen/device_tree.h
> > @@ -177,6 +177,11 @@ int device_tree_for_each_node(const void *fdt, int node,
> > */
> > void dt_unflatten_host_device_tree(void);
> >
> > +/*
> > + * unflatten any device tree.
> > + */
> > +void unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes);
> > +
> > /**
> > * IRQ translation callback
> > * TODO: For the moment we assume that we only have ONE
>
> NIT: there are some minor code style issues, like indentation that could be fixed
>
> Cheers,
> Luca
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 01/14] xen/arm/device: Remove __init from function type
2022-03-14 12:31 ` Luca Fancellu
2022-03-15 22:29 ` Vikram Garhwal
@ 2022-03-15 22:42 ` Vikram Garhwal
1 sibling, 0 replies; 42+ messages in thread
From: Vikram Garhwal @ 2022-03-15 22:42 UTC (permalink / raw)
To: Luca Fancellu
Cc: Vikram Garhwal, Xen-devel, sstabellini, julien, Bertrand Marquis,
volodymyr_babchuk
On Mon, Mar 14, 2022 at 12:31:06PM +0000, Luca Fancellu wrote:
>
>
> > On 8 Mar 2022, at 19:46, Vikram Garhwal <fnu.vikram@xilinx.com> wrote:
> >
> > Change function type of following function to access during runtime:
> > 1. map_irq_to_domain()
> > 2. handle_device_interrupt()
> > 3. map_range_to_domain()
> > 4. unflatten_dt_node()
> > 5. unflatten_device_tree()
> >
> > Move map_irq_to_domain(), handle_device_interrupt() and map_range_to_domain() to
> > device.c.
> >
> > These changes are done to support the dynamic programming of a nodes where an
> > overlay node will be added to fdt and unflattened node will be added to dt_host.
> > Furthermore, IRQ and mmio mapping will be done for the added node.
> >
> > Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> > ---
> > xen/arch/arm/device.c | 136 +++++++++++++++++++++++++++++
> > xen/arch/arm/domain_build.c | 142 -------------------------------
> > xen/arch/arm/include/asm/setup.h | 3 +
> > xen/common/device_tree.c | 20 ++---
> > xen/include/xen/device_tree.h | 5 ++
> > 5 files changed, 154 insertions(+), 152 deletions(-)
> >
> > diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> > index 70cd6c1a19..0dfd33b33e 100644
> > --- a/xen/arch/arm/device.c
> > +++ b/xen/arch/arm/device.c
> > @@ -21,6 +21,9 @@
> > #include <xen/errno.h>
> > #include <xen/init.h>
> > #include <xen/lib.h>
> > +#include <xen/iocap.h>
> > +#include <asm/domain_build.h>
> > +#include <asm/setup.h>
> >
> > extern const struct device_desc _sdevice[], _edevice[];
> > extern const struct acpi_device_desc _asdevice[], _aedevice[];
> > @@ -84,6 +87,139 @@ enum device_class device_get_class(const struct dt_device_node *dev)
> > return DEVICE_UNKNOWN;
> > }
> >
> > +int map_irq_to_domain(struct domain *d, unsigned int irq,
> > + bool need_mapping, const char *devname)
> > +{
> > + int res;
> > +
> > + res = irq_permit_access(d, irq);
> > + if ( res )
> > + {
> > + printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n",
> > + d->domain_id, irq);
> > + return res;
> > + }
> > +
> > + if ( need_mapping )
> > + {
> > + /*
> > + * Checking the return of vgic_reserve_virq is not
> > + * necessary. It should not fail except when we try to map
> > + * the IRQ twice. This can legitimately happen if the IRQ is shared
> > + */
> > + vgic_reserve_virq(d, irq);
> > +
> > + res = route_irq_to_guest(d, irq, irq, devname);
> > + if ( res < 0 )
> > + {
> > + printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
> > + irq, d->domain_id);
> > + return res;
> > + }
> > + }
> > +
> > + dt_dprintk(" - IRQ: %u\n", irq);
> > + return 0;
> > +}
> > +
> > +int map_range_to_domain(const struct dt_device_node *dev,
> > + u64 addr, u64 len, void *data)
> > +{
> > + struct map_range_data *mr_data = data;
> > + struct domain *d = mr_data->d;
> > + int res;
> > +
> > + res = iomem_permit_access(d, paddr_to_pfn(addr),
> > + paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
>
> Hi Vikram,
>
> Why the if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/",
> strlen("/reserved-memory/")) != 0 ) was dropped?
>
>
Hi Luca,
Thanks for spotting this. Yeah, I messed this while porting the patches from
internal to upstream Xen.
Will address this in v4!
@everyone, apologies for resending the same email. Previous one didn't make to
xen-devel due to change in my email ID.
Regards,
Vikram
> > + if ( res )
> > + {
> > + printk(XENLOG_ERR "Unable to permit to dom%d access to"
> > + " 0x%"PRIx64" - 0x%"PRIx64"\n",
> > + d->domain_id,
> > + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
> > + return res;
> > + }
> > +
> > + if ( !mr_data->skip_mapping )
> > + {
> > + res = map_regions_p2mt(d,
> > + gaddr_to_gfn(addr),
> > + PFN_UP(len),
> > + maddr_to_mfn(addr),
> > + mr_data->p2mt);
> > +
> > + if ( res < 0 )
> > + {
> > + printk(XENLOG_ERR "Unable to map 0x%"PRIx64
> > + " - 0x%"PRIx64" in domain %d\n",
> > + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
> > + d->domain_id);
> > + return res;
> > + }
> > + }
> > +
> > + dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
> > + addr, addr + len, mr_data->p2mt);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * handle_device_interrupts retrieves the interrupts configuration from
> > + * a device tree node and maps those interrupts to the target domain.
> > + *
> > + * Returns:
> > + * < 0 error
> > + * 0 success
> > + */
> > +int handle_device_interrupts(struct domain *d,
> > + struct dt_device_node *dev,
> > + bool need_mapping)
> > +{
> > + unsigned int i, nirq;
> > + int res;
> > + struct dt_raw_irq rirq;
> > +
> > + nirq = dt_number_of_irq(dev);
> > +
> > + /* Give permission and map IRQs */
> > + for ( i = 0; i < nirq; i++ )
> > + {
> > + res = dt_device_get_raw_irq(dev, i, &rirq);
> > + if ( res )
> > + {
> > + printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
> > + i, dt_node_full_name(dev));
> > + return res;
> > + }
> > +
> > + /*
> > + * Don't map IRQ that have no physical meaning
> > + * ie: IRQ whose controller is not the GIC
> > + */
> > + if ( rirq.controller != dt_interrupt_controller )
> > + {
> > + dt_dprintk("irq %u not connected to primary controller. Connected to %s\n",
> > + i, dt_node_full_name(rirq.controller));
> > + continue;
> > + }
> > +
> > + res = platform_get_irq(dev, i);
> > + if ( res < 0 )
> > + {
> > + printk(XENLOG_ERR "Unable to get irq %u for %s\n",
> > + i, dt_node_full_name(dev));
> > + return res;
> > + }
> > +
> > + res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev));
> > + if ( res )
> > + return res;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /*
> > * Local variables:
> > * mode: C
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 8be01678de..b06770a2af 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1794,41 +1794,6 @@ int __init make_chosen_node(const struct kernel_info *kinfo)
> > return res;
> > }
> >
> > -int __init map_irq_to_domain(struct domain *d, unsigned int irq,
> > - bool need_mapping, const char *devname)
> > -{
> > - int res;
> > -
> > - res = irq_permit_access(d, irq);
> > - if ( res )
> > - {
> > - printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n",
> > - d->domain_id, irq);
> > - return res;
> > - }
> > -
> > - if ( need_mapping )
> > - {
> > - /*
> > - * Checking the return of vgic_reserve_virq is not
> > - * necessary. It should not fail except when we try to map
> > - * the IRQ twice. This can legitimately happen if the IRQ is shared
> > - */
> > - vgic_reserve_virq(d, irq);
> > -
> > - res = route_irq_to_guest(d, irq, irq, devname);
> > - if ( res < 0 )
> > - {
> > - printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
> > - irq, d->domain_id);
> > - return res;
> > - }
> > - }
> > -
> > - dt_dprintk(" - IRQ: %u\n", irq);
> > - return 0;
> > -}
> > -
> > static int __init map_dt_irq_to_domain(const struct dt_device_node *dev,
> > const struct dt_irq *dt_irq,
> > void *data)
> > @@ -1860,57 +1825,6 @@ static int __init map_dt_irq_to_domain(const struct dt_device_node *dev,
> > return 0;
> > }
> >
> > -int __init map_range_to_domain(const struct dt_device_node *dev,
> > - u64 addr, u64 len, void *data)
> > -{
> > - struct map_range_data *mr_data = data;
> > - struct domain *d = mr_data->d;
> > - int res;
> > -
> > - /*
> > - * reserved-memory regions are RAM carved out for a special purpose.
> > - * They are not MMIO and therefore a domain should not be able to
> > - * manage them via the IOMEM interface.
> > - */
> > - if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/",
> > - strlen("/reserved-memory/")) != 0 )
> > - {
> > - res = iomem_permit_access(d, paddr_to_pfn(addr),
> > - paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> > - if ( res )
> > - {
> > - printk(XENLOG_ERR "Unable to permit to dom%d access to"
> > - " 0x%"PRIx64" - 0x%"PRIx64"\n",
> > - d->domain_id,
> > - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
> > - return res;
> > - }
> > - }
> > -
> > - if ( !mr_data->skip_mapping )
> > - {
> > - res = map_regions_p2mt(d,
> > - gaddr_to_gfn(addr),
> > - PFN_UP(len),
> > - maddr_to_mfn(addr),
> > - mr_data->p2mt);
> > -
> > - if ( res < 0 )
> > - {
> > - printk(XENLOG_ERR "Unable to map 0x%"PRIx64
> > - " - 0x%"PRIx64" in domain %d\n",
> > - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
> > - d->domain_id);
> > - return res;
> > - }
> > - }
> > -
> > - dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
> > - addr, addr + len, mr_data->p2mt);
> > -
> > - return 0;
> > -}
> > -
> > /*
> > * For a node which describes a discoverable bus (such as a PCI bus)
> > * then we may need to perform additional mappings in order to make
> > @@ -1938,62 +1852,6 @@ static int __init map_device_children(const struct dt_device_node *dev,
> > return 0;
> > }
> >
> > -/*
> > - * handle_device_interrupts retrieves the interrupts configuration from
> > - * a device tree node and maps those interrupts to the target domain.
> > - *
> > - * Returns:
> > - * < 0 error
> > - * 0 success
> > - */
> > -static int __init handle_device_interrupts(struct domain *d,
> > - struct dt_device_node *dev,
> > - bool need_mapping)
> > -{
> > - unsigned int i, nirq;
> > - int res;
> > - struct dt_raw_irq rirq;
> > -
> > - nirq = dt_number_of_irq(dev);
> > -
> > - /* Give permission and map IRQs */
> > - for ( i = 0; i < nirq; i++ )
> > - {
> > - res = dt_device_get_raw_irq(dev, i, &rirq);
> > - if ( res )
> > - {
> > - printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
> > - i, dt_node_full_name(dev));
> > - return res;
> > - }
> > -
> > - /*
> > - * Don't map IRQ that have no physical meaning
> > - * ie: IRQ whose controller is not the GIC
> > - */
> > - if ( rirq.controller != dt_interrupt_controller )
> > - {
> > - dt_dprintk("irq %u not connected to primary controller. Connected to %s\n",
> > - i, dt_node_full_name(rirq.controller));
> > - continue;
> > - }
> > -
> > - res = platform_get_irq(dev, i);
> > - if ( res < 0 )
> > - {
> > - printk(XENLOG_ERR "Unable to get irq %u for %s\n",
> > - i, dt_node_full_name(dev));
> > - return res;
> > - }
> > -
> > - res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev));
> > - if ( res )
> > - return res;
> > - }
> > -
> > - return 0;
> > -}
> > -
> > /*
> > * For a given device node:
> > * - Give permission to the guest to manage IRQ and MMIO range
> > diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> > index 7a1e1d6798..8a26f1845c 100644
> > --- a/xen/arch/arm/include/asm/setup.h
> > +++ b/xen/arch/arm/include/asm/setup.h
> > @@ -134,6 +134,9 @@ void device_tree_get_reg(const __be32 **cell, u32 address_cells,
> > u32 device_tree_get_u32(const void *fdt, int node,
> > const char *prop_name, u32 dflt);
> >
> > +int handle_device_interrupts(struct domain *d, struct dt_device_node *dev,
> > + bool need_mapping);
> > +
> > int map_range_to_domain(const struct dt_device_node *dev,
> > u64 addr, u64 len, void *data);
> >
> > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > index 4aae281e89..f43d66a501 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -1811,12 +1811,12 @@ int dt_count_phandle_with_args(const struct dt_device_node *np,
> > * @allnextpp: pointer to ->allnext from last allocated device_node
> > * @fpsize: Size of the node path up at the current depth.
> > */
> > -static unsigned long __init unflatten_dt_node(const void *fdt,
> > - unsigned long mem,
> > - unsigned long *p,
> > - struct dt_device_node *dad,
> > - struct dt_device_node ***allnextpp,
> > - unsigned long fpsize)
> > +static unsigned long unflatten_dt_node(const void *fdt,
> > + unsigned long mem,
> > + unsigned long *p,
> > + struct dt_device_node *dad,
> > + struct dt_device_node ***allnextpp,
> > + unsigned long fpsize)
> > {
> > struct dt_device_node *np;
> > struct dt_property *pp, **prev_pp = NULL;
> > @@ -2047,7 +2047,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
> > }
> >
> > /**
> > - * __unflatten_device_tree - create tree of device_nodes from flat blob
> > + * unflatten_device_tree - create tree of device_nodes from flat blob
> > *
> > * unflattens a device-tree, creating the
> > * tree of struct device_node. It also fills the "name" and "type"
> > @@ -2056,8 +2056,8 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
> > * @fdt: The fdt to expand
> > * @mynodes: The device_node tree created by the call
> > */
> > -static void __init __unflatten_device_tree(const void *fdt,
> > - struct dt_device_node **mynodes)
> > +void unflatten_device_tree(const void *fdt,
> > + struct dt_device_node **mynodes)
> > {
> > unsigned long start, mem, size;
> > struct dt_device_node **allnextp = mynodes;
> > @@ -2179,7 +2179,7 @@ dt_find_interrupt_controller(const struct dt_device_match *matches)
> >
> > void __init dt_unflatten_host_device_tree(void)
> > {
> > - __unflatten_device_tree(device_tree_flattened, &dt_host);
> > + unflatten_device_tree(device_tree_flattened, &dt_host);
> > dt_alias_scan();
> > }
> >
> > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> > index fd6cd00b43..06d7866c10 100644
> > --- a/xen/include/xen/device_tree.h
> > +++ b/xen/include/xen/device_tree.h
> > @@ -177,6 +177,11 @@ int device_tree_for_each_node(const void *fdt, int node,
> > */
> > void dt_unflatten_host_device_tree(void);
> >
> > +/*
> > + * unflatten any device tree.
> > + */
> > +void unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes);
> > +
> > /**
> > * IRQ translation callback
> > * TODO: For the moment we assume that we only have ONE
>
> NIT: there are some minor code style issues, like indentation that could be fixed
>
> Cheers,
> Luca
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [XEN][RFC PATCH v3 02/14] xen/arm: Add CONFIG_OVERLAY_DTB
2022-03-08 19:46 [XEN][RFC PATCH v3 00/14] dynamic node programming using overlay dtbo Vikram Garhwal
2022-03-08 19:46 ` [XEN][RFC PATCH v3 01/14] xen/arm/device: Remove __init from function type Vikram Garhwal
@ 2022-03-08 19:46 ` Vikram Garhwal
2022-03-14 12:42 ` Luca Fancellu
2022-03-08 19:46 ` [XEN][RFC PATCH v3 03/14] libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB Vikram Garhwal
` (11 subsequent siblings)
13 siblings, 1 reply; 42+ messages in thread
From: Vikram Garhwal @ 2022-03-08 19:46 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk,
Vikram Garhwal, Volodymyr Babchuk
Introduce a config option where the user can enable support for adding/removing
device tree nodes using a device tree binary overlay.
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
xen/arch/arm/Kconfig | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index ecfa6822e4..0159fbe27a 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -46,6 +46,12 @@ config HAS_ITS
bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED
depends on GICV3 && !NEW_VGIC
+config OVERLAY_DTB
+ bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED
+ ---help---
+
+ Dynamic addition/removal of Xen device tree nodes using a dtbo.
+
config HVM
def_bool y
--
2.17.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 02/14] xen/arm: Add CONFIG_OVERLAY_DTB
2022-03-08 19:46 ` [XEN][RFC PATCH v3 02/14] xen/arm: Add CONFIG_OVERLAY_DTB Vikram Garhwal
@ 2022-03-14 12:42 ` Luca Fancellu
0 siblings, 0 replies; 42+ messages in thread
From: Luca Fancellu @ 2022-03-14 12:42 UTC (permalink / raw)
To: Vikram Garhwal
Cc: Xen-devel, sstabellini, julien, Bertrand Marquis, volodymyr_babchuk
> On 8 Mar 2022, at 19:46, Vikram Garhwal <fnu.vikram@xilinx.com> wrote:
>
> Introduce a config option where the user can enable support for adding/removing
> device tree nodes using a device tree binary overlay.
>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
> xen/arch/arm/Kconfig | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index ecfa6822e4..0159fbe27a 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -46,6 +46,12 @@ config HAS_ITS
> bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED
> depends on GICV3 && !NEW_VGIC
>
> +config OVERLAY_DTB
> + bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED
> + ---help---
> +
> + Dynamic addition/removal of Xen device tree nodes using a dtbo.
> +
Many recents entries in this file uses a different style from this one, using “help” instead of
“—help—“ and omitting the blank line, I would continue to use the more recent style if no
one object with it
Cheers,
Luca
> config HVM
> def_bool y
>
> --
> 2.17.1
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [XEN][RFC PATCH v3 03/14] libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB.
2022-03-08 19:46 [XEN][RFC PATCH v3 00/14] dynamic node programming using overlay dtbo Vikram Garhwal
2022-03-08 19:46 ` [XEN][RFC PATCH v3 01/14] xen/arm/device: Remove __init from function type Vikram Garhwal
2022-03-08 19:46 ` [XEN][RFC PATCH v3 02/14] xen/arm: Add CONFIG_OVERLAY_DTB Vikram Garhwal
@ 2022-03-08 19:46 ` Vikram Garhwal
2022-05-17 17:36 ` Julien Grall
2022-03-08 19:46 ` [XEN][RFC PATCH v3 04/14] libfdt: overlay: change overlay_get_target() Vikram Garhwal
` (10 subsequent siblings)
13 siblings, 1 reply; 42+ messages in thread
From: Vikram Garhwal @ 2022-03-08 19:46 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk, Vikram Garhwal
This is done to access fdt library function which are required for adding device
tree overlay nodes for dynamic programming of nodes.
Acked-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
xen/common/libfdt/Makefile | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/xen/common/libfdt/Makefile b/xen/common/libfdt/Makefile
index 6708af12e5..86faa90f79 100644
--- a/xen/common/libfdt/Makefile
+++ b/xen/common/libfdt/Makefile
@@ -1,7 +1,11 @@
include $(src)/Makefile.libfdt
SECTIONS := text data $(SPECIAL_DATA_SECTIONS)
+
+# For CONFIG_OVERLAY_DTB, libfdt functionalities will be needed during runtime.
+ifneq ($(CONFIG_OVERLAY_DTB),y)
OBJCOPYFLAGS := $(foreach s,$(SECTIONS),--rename-section .$(s)=.init.$(s))
+endif
obj-y += libfdt.o
nocov-y += libfdt.o
--
2.17.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 03/14] libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB.
2022-03-08 19:46 ` [XEN][RFC PATCH v3 03/14] libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB Vikram Garhwal
@ 2022-05-17 17:36 ` Julien Grall
0 siblings, 0 replies; 42+ messages in thread
From: Julien Grall @ 2022-05-17 17:36 UTC (permalink / raw)
To: Vikram Garhwal, xen-devel
Cc: sstabellini, bertrand.marquis, volodymyr_babchuk
Hi Vikram,
On 08/03/2022 19:46, Vikram Garhwal wrote:
> This is done to access fdt library function which are required for adding device
> tree overlay nodes for dynamic programming of nodes.
>
> Acked-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
The tags usually are usually ordered chronogically. You first wrote the
patch and then I provided a review. So your Signed-off-by should come
first and then my Acked-by.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 42+ messages in thread
* [XEN][RFC PATCH v3 04/14] libfdt: overlay: change overlay_get_target()
2022-03-08 19:46 [XEN][RFC PATCH v3 00/14] dynamic node programming using overlay dtbo Vikram Garhwal
` (2 preceding siblings ...)
2022-03-08 19:46 ` [XEN][RFC PATCH v3 03/14] libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB Vikram Garhwal
@ 2022-03-08 19:46 ` Vikram Garhwal
2022-03-14 14:55 ` Luca Fancellu
2022-05-17 17:51 ` Julien Grall
2022-03-08 19:46 ` [XEN][RFC PATCH v3 05/14] xen/device-tree: Add _dt_find_node_by_path() to find nodes in device tree Vikram Garhwal
` (9 subsequent siblings)
13 siblings, 2 replies; 42+ messages in thread
From: Vikram Garhwal @ 2022-03-08 19:46 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk, Vikram Garhwal
Rename overlay_get_target() to fdt_overlay_target_offset() and remove static
function type.
This is done to get the target path for the overlay nodes which is very useful
in many cases. For example, Xen hypervisor needs it when applying overlays
because Xen needs to do further processing of the overlay nodes, e.g. mapping of
resources(IRQs and IOMMUs) to other VMs, creation of SMMU pagetables, etc.
This commit is also applied to git://github.com/dgibson/dtc:
commit: ad9cf6bde5b90d4c1e5a79a2803e98d6344c27d7.
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
xen/common/libfdt/fdt_overlay.c | 29 +++++++----------------------
xen/common/libfdt/version.lds | 1 +
xen/include/xen/libfdt/libfdt.h | 18 ++++++++++++++++++
3 files changed, 26 insertions(+), 22 deletions(-)
diff --git a/xen/common/libfdt/fdt_overlay.c b/xen/common/libfdt/fdt_overlay.c
index 7b95e2b639..acf0c4c2a6 100644
--- a/xen/common/libfdt/fdt_overlay.c
+++ b/xen/common/libfdt/fdt_overlay.c
@@ -41,37 +41,22 @@ static uint32_t overlay_get_target_phandle(const void *fdto, int fragment)
return fdt32_to_cpu(*val);
}
-/**
- * overlay_get_target - retrieves the offset of a fragment's target
- * @fdt: Base device tree blob
- * @fdto: Device tree overlay blob
- * @fragment: node offset of the fragment in the overlay
- * @pathp: pointer which receives the path of the target (or NULL)
- *
- * overlay_get_target() retrieves the target offset in the base
- * device tree of a fragment, no matter how the actual targeting is
- * done (through a phandle or a path)
- *
- * returns:
- * the targeted node offset in the base device tree
- * Negative error code on error
- */
-static int overlay_get_target(const void *fdt, const void *fdto,
- int fragment, char const **pathp)
+int fdt_overlay_target_offset(const void *fdt, const void *fdto,
+ int fragment_offset, char const **pathp)
{
uint32_t phandle;
const char *path = NULL;
int path_len = 0, ret;
/* Try first to do a phandle based lookup */
- phandle = overlay_get_target_phandle(fdto, fragment);
+ phandle = overlay_get_target_phandle(fdto, fragment_offset);
if (phandle == (uint32_t)-1)
return -FDT_ERR_BADPHANDLE;
/* no phandle, try path */
if (!phandle) {
/* And then a path based lookup */
- path = fdt_getprop(fdto, fragment, "target-path", &path_len);
+ path = fdt_getprop(fdto, fragment_offset, "target-path", &path_len);
if (path)
ret = fdt_path_offset(fdt, path);
else
@@ -638,7 +623,7 @@ static int overlay_merge(void *fdt, void *fdto)
if (overlay < 0)
return overlay;
- target = overlay_get_target(fdt, fdto, fragment, NULL);
+ target = fdt_overlay_target_offset(fdt, fdto, fragment, NULL);
if (target < 0)
return target;
@@ -781,7 +766,7 @@ static int overlay_symbol_update(void *fdt, void *fdto)
return -FDT_ERR_BADOVERLAY;
/* get the target of the fragment */
- ret = overlay_get_target(fdt, fdto, fragment, &target_path);
+ ret = fdt_overlay_target_offset(fdt, fdto, fragment, &target_path);
if (ret < 0)
return ret;
target = ret;
@@ -803,7 +788,7 @@ static int overlay_symbol_update(void *fdt, void *fdto)
if (!target_path) {
/* again in case setprop_placeholder changed it */
- ret = overlay_get_target(fdt, fdto, fragment, &target_path);
+ ret = fdt_overlay_target_offset(fdt, fdto, fragment, &target_path);
if (ret < 0)
return ret;
target = ret;
diff --git a/xen/common/libfdt/version.lds b/xen/common/libfdt/version.lds
index 7ab85f1d9d..cbce5d4a8b 100644
--- a/xen/common/libfdt/version.lds
+++ b/xen/common/libfdt/version.lds
@@ -77,6 +77,7 @@ LIBFDT_1.2 {
fdt_appendprop_addrrange;
fdt_setprop_inplace_namelen_partial;
fdt_create_with_flags;
+ fdt_overlay_target_offset;
local:
*;
};
diff --git a/xen/include/xen/libfdt/libfdt.h b/xen/include/xen/libfdt/libfdt.h
index c71689e2be..fabddbee8c 100644
--- a/xen/include/xen/libfdt/libfdt.h
+++ b/xen/include/xen/libfdt/libfdt.h
@@ -2109,6 +2109,24 @@ int fdt_del_node(void *fdt, int nodeoffset);
*/
int fdt_overlay_apply(void *fdt, void *fdto);
+/**
+ * fdt_overlay_target_offset - retrieves the offset of a fragment's target
+ * @fdt: Base device tree blob
+ * @fdto: Device tree overlay blob
+ * @fragment_offset: node offset of the fragment in the overlay
+ * @pathp: pointer which receives the path of the target (or NULL)
+ *
+ * fdt_overlay_target_offset() retrieves the target offset in the base
+ * device tree of a fragment, no matter how the actual targeting is
+ * done (through a phandle or a path)
+ *
+ * returns:
+ * the targeted node offset in the base device tree
+ * Negative error code on error
+ */
+int fdt_overlay_target_offset(const void *fdt, const void *fdto,
+ int fragment_offset, char const **pathp);
+
/**********************************************************************/
/* Debugging / informational functions */
/**********************************************************************/
--
2.17.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 04/14] libfdt: overlay: change overlay_get_target()
2022-03-08 19:46 ` [XEN][RFC PATCH v3 04/14] libfdt: overlay: change overlay_get_target() Vikram Garhwal
@ 2022-03-14 14:55 ` Luca Fancellu
2022-05-17 17:51 ` Julien Grall
1 sibling, 0 replies; 42+ messages in thread
From: Luca Fancellu @ 2022-03-14 14:55 UTC (permalink / raw)
To: Vikram Garhwal
Cc: Xen-devel, sstabellini, julien, Bertrand Marquis, volodymyr_babchuk
> On 8 Mar 2022, at 19:46, Vikram Garhwal <fnu.vikram@xilinx.com> wrote:
>
> Rename overlay_get_target() to fdt_overlay_target_offset() and remove static
> function type.
>
> This is done to get the target path for the overlay nodes which is very useful
> in many cases. For example, Xen hypervisor needs it when applying overlays
> because Xen needs to do further processing of the overlay nodes, e.g. mapping of
> resources(IRQs and IOMMUs) to other VMs, creation of SMMU pagetables, etc.
>
> This commit is also applied to git://github.com/dgibson/dtc:
> commit: ad9cf6bde5b90d4c1e5a79a2803e98d6344c27d7.
Hi Vikram,
please correct me if I’m wrong, I’ve cloned the repository and I found your commit with
this SHA: 45f3d1a095dd3440578d5c6313eba555a791f3fb
Cheers,
Luca
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [XEN][RFC PATCH v3 04/14] libfdt: overlay: change overlay_get_target()
2022-03-08 19:46 ` [XEN][RFC PATCH v3 04/14] libfdt: overlay: change overlay_get_target() Vikram Garhwal
2022-03-14 14:55 ` Luca Fancellu
@ 2022-05-17 17:51 ` Julien Grall
1 sibling, 0 replies; 42+ messages in thread
From: Julien Grall @ 2022-05-17 17:51 UTC (permalink / raw)
To: Vikram Garhwal, xen-devel
Cc: sstabellini, bertrand.marquis, volodymyr_babchuk
Hi Vikram,
On 08/03/2022 19:46, Vikram Garhwal wrote:
> Rename overlay_get_target() to fdt_overlay_target_offset() and remove static
> function type.
>
> This is done to get the target path for the overlay nodes which is very useful
> in many cases. For example, Xen hypervisor needs it when applying overlays
> because Xen needs to do further processing of the overlay nodes, e.g. mapping of
> resources(IRQs and IOMMUs) to other VMs, creation of SMMU pagetables, etc.
>
> This commit is also applied to git://github.com/dgibson/dtc:
NIT: I have tried to use this URL and git clone got stuck. Looking at
github, they don't seem to list the git:// version. So maybe use https://
> commit: ad9cf6bde5b90d4c1e5a79a2803e98d6344c27d7.
We have introduced a tag Origin to help tracking commit from other
project. The tag looks like:
Origin: <repository> <commit-id>
The commit-id is first the 12-characters of the commit ID.
Please also retain the tags:
Message-Id: <1637204036-382159-2-git-send-email-fnu.vikram@xilinx.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 42+ messages in thread
* [XEN][RFC PATCH v3 05/14] xen/device-tree: Add _dt_find_node_by_path() to find nodes in device tree
2022-03-08 19:46 [XEN][RFC PATCH v3 00/14] dynamic node programming using overlay dtbo Vikram Garhwal
` (3 preceding siblings ...)
2022-03-08 19:46 ` [XEN][RFC PATCH v3 04/14] libfdt: overlay: change overlay_get_target() Vikram Garhwal
@ 2022-03-08 19:46 ` Vikram Garhwal
2022-03-14 15:35 ` Luca Fancellu
2022-03-08 19:46 ` [XEN][RFC PATCH v3 06/14] xen/smmu: Add remove_device callback for smmu_iommu ops Vikram Garhwal
` (8 subsequent siblings)
13 siblings, 1 reply; 42+ messages in thread
From: Vikram Garhwal @ 2022-03-08 19:46 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk, Vikram Garhwal
Add _dt_find_by_path() to find a matching node with path for a dt_device_node.
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
xen/common/device_tree.c | 10 ++++++++--
xen/include/xen/device_tree.h | 9 +++++++++
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index f43d66a501..2e334f949e 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -358,17 +358,23 @@ struct dt_device_node *dt_find_node_by_type(struct dt_device_node *from,
return np;
}
-struct dt_device_node *dt_find_node_by_path(const char *path)
+struct dt_device_node *_dt_find_node_by_path(struct dt_device_node *dt,
+ const char *path)
{
struct dt_device_node *np;
- dt_for_each_device_node(dt_host, np)
+ dt_for_each_device_node(dt, np)
if ( np->full_name && (dt_node_cmp(np->full_name, path) == 0) )
break;
return np;
}
+struct dt_device_node *dt_find_node_by_path(const char *path)
+{
+ return _dt_find_node_by_path(dt_host, path);
+}
+
int dt_find_node_by_gpath(XEN_GUEST_HANDLE(char) u_path, uint32_t u_plen,
struct dt_device_node **node)
{
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 06d7866c10..9da32a851e 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -525,6 +525,15 @@ struct dt_device_node *dt_find_node_by_alias(const char *alias);
*/
struct dt_device_node *dt_find_node_by_path(const char *path);
+/**
+ * _dt_find_node_by_path - Find a node matching a full DT path
+ * @dt_node: The device tree to search
+ * @path: The full path to match
+ *
+ * Returns a node pointer.
+ */
+struct dt_device_node *_dt_find_node_by_path(struct dt_device_node *dt,
+ const char *path);
/**
* dt_find_node_by_gpath - Same as dt_find_node_by_path but retrieve the
--
2.17.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 05/14] xen/device-tree: Add _dt_find_node_by_path() to find nodes in device tree
2022-03-08 19:46 ` [XEN][RFC PATCH v3 05/14] xen/device-tree: Add _dt_find_node_by_path() to find nodes in device tree Vikram Garhwal
@ 2022-03-14 15:35 ` Luca Fancellu
0 siblings, 0 replies; 42+ messages in thread
From: Luca Fancellu @ 2022-03-14 15:35 UTC (permalink / raw)
To: Vikram Garhwal
Cc: Xen-devel, sstabellini, julien, Bertrand Marquis, volodymyr_babchuk
Hi Vikram,
> On 8 Mar 2022, at 19:46, Vikram Garhwal <fnu.vikram@xilinx.com> wrote:
>
> Add _dt_find_by_path() to find a matching node with path for a dt_device_node.
>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
> xen/common/device_tree.c | 10 ++++++++--
> xen/include/xen/device_tree.h | 9 +++++++++
> 2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index f43d66a501..2e334f949e 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -358,17 +358,23 @@ struct dt_device_node *dt_find_node_by_type(struct dt_device_node *from,
> return np;
> }
>
> -struct dt_device_node *dt_find_node_by_path(const char *path)
> +struct dt_device_node *_dt_find_node_by_path(struct dt_device_node *dt,
> + const char *path)
A function with a name starting with an underscore feels to me more an internal function rather than public,
I suggest to find another name, maybe device_tree_find_node_by_path?
> {
> struct dt_device_node *np;
>
> - dt_for_each_device_node(dt_host, np)
> + dt_for_each_device_node(dt, np)
> if ( np->full_name && (dt_node_cmp(np->full_name, path) == 0) )
> break;
>
> return np;
> }
>
> +struct dt_device_node *dt_find_node_by_path(const char *path)
> +{
> + return _dt_find_node_by_path(dt_host, path);
> +}
This is public but it’s not declared in device_tree.h. Maybe this can become a static inline defined in the header?
Cheers,
Luca
^ permalink raw reply [flat|nested] 42+ messages in thread
* [XEN][RFC PATCH v3 06/14] xen/smmu: Add remove_device callback for smmu_iommu ops
2022-03-08 19:46 [XEN][RFC PATCH v3 00/14] dynamic node programming using overlay dtbo Vikram Garhwal
` (4 preceding siblings ...)
2022-03-08 19:46 ` [XEN][RFC PATCH v3 05/14] xen/device-tree: Add _dt_find_node_by_path() to find nodes in device tree Vikram Garhwal
@ 2022-03-08 19:46 ` Vikram Garhwal
2022-03-14 15:45 ` Luca Fancellu
2022-03-08 19:46 ` [XEN][RFC PATCH v3 07/14] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller Vikram Garhwal
` (7 subsequent siblings)
13 siblings, 1 reply; 42+ messages in thread
From: Vikram Garhwal @ 2022-03-08 19:46 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk,
Vikram Garhwal, Volodymyr Babchuk
Add remove_device callback for removing the device entry from smmu-master using
following steps:
1. Find if SMMU master exists for the device node.
2. Remove the SMMU master
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
xen/drivers/passthrough/arm/smmu.c | 56 ++++++++++++++++++++++++++++++
1 file changed, 56 insertions(+)
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index b186c28dff..8b3b8b9850 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -816,6 +816,19 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
return 0;
}
+static int remove_smmu_master(struct arm_smmu_device *smmu,
+ struct arm_smmu_master *master)
+{
+ if (!smmu->masters.rb_node) {
+ ASSERT_UNREACHABLE();
+ return -ENOENT;
+ }
+
+ rb_erase(&master->node, &smmu->masters);
+
+ return 0;
+}
+
static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
struct device *dev,
struct iommu_fwspec *fwspec)
@@ -853,6 +866,32 @@ static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
return insert_smmu_master(smmu, master);
}
+static int arm_smmu_dt_remove_device_legacy(struct arm_smmu_device *smmu,
+ struct device *dev)
+{
+ struct arm_smmu_master *master;
+ struct device_node *dev_node = dev_get_dev_node(dev);
+ int ret;
+
+ master = find_smmu_master(smmu, dev_node);
+ if (master == NULL) {
+ dev_err(dev,
+ "No registrations found for master device %s\n",
+ dev_node->name);
+ return -EINVAL;
+ }
+
+ ret = remove_smmu_master(smmu, master);
+
+ if (ret)
+ return ret;
+
+ dev_node->is_protected = false;
+
+ kfree(master);
+ return 0;
+}
+
static int register_smmu_master(struct arm_smmu_device *smmu,
struct device *dev,
struct of_phandle_args *masterspec)
@@ -876,6 +915,22 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
fwspec);
}
+static int arm_smmu_dt_remove_device_generic(u8 devfn, struct device *dev)
+{
+ struct arm_smmu_device *smmu;
+ struct iommu_fwspec *fwspec;
+
+ fwspec = dev_iommu_fwspec_get(dev);
+ if (fwspec == NULL)
+ return -ENXIO;
+
+ smmu = find_smmu(fwspec->iommu_dev);
+ if (smmu == NULL)
+ return -ENXIO;
+
+ return arm_smmu_dt_remove_device_legacy(smmu, dev);
+}
+
static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev)
{
struct arm_smmu_device *smmu;
@@ -2861,6 +2916,7 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
.init = arm_smmu_iommu_domain_init,
.hwdom_init = arch_iommu_hwdom_init,
.add_device = arm_smmu_dt_add_device_generic,
+ .remove_device = arm_smmu_dt_remove_device_generic,
.teardown = arm_smmu_iommu_domain_teardown,
.iotlb_flush = arm_smmu_iotlb_flush,
.iotlb_flush_all = arm_smmu_iotlb_flush_all,
--
2.17.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* [XEN][RFC PATCH v3 07/14] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller
2022-03-08 19:46 [XEN][RFC PATCH v3 00/14] dynamic node programming using overlay dtbo Vikram Garhwal
` (5 preceding siblings ...)
2022-03-08 19:46 ` [XEN][RFC PATCH v3 06/14] xen/smmu: Add remove_device callback for smmu_iommu ops Vikram Garhwal
@ 2022-03-08 19:46 ` Vikram Garhwal
2022-03-14 15:58 ` Luca Fancellu
2022-05-17 18:19 ` Julien Grall
2022-03-08 19:46 ` [XEN][RFC PATCH v3 08/14] xen/iommu: protect iommu_add_dt_device() with dtdevs_lock Vikram Garhwal
` (6 subsequent siblings)
13 siblings, 2 replies; 42+ messages in thread
From: Vikram Garhwal @ 2022-03-08 19:46 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk, Vikram Garhwal
Rename iommu_dt_device_is_assigned() to iommu_dt_device_is_assigned_lock().
Moving spin_lock to caller was done to prevent the concurrent access to
iommu_dt_device_is_assigned while doing add/remove/assign/deassign.
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
xen/drivers/passthrough/device_tree.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 98f2aa0dad..b3b04f8e03 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -83,16 +83,14 @@ fail:
return rc;
}
-static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
+static bool_t iommu_dt_device_is_assigned_lock(const struct dt_device_node *dev)
{
bool_t assigned = 0;
if ( !dt_device_is_protected(dev) )
return 0;
- spin_lock(&dtdevs_lock);
assigned = !list_empty(&dev->domain_list);
- spin_unlock(&dtdevs_lock);
return assigned;
}
@@ -225,12 +223,17 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
{
- if ( iommu_dt_device_is_assigned(dev) )
+ spin_lock(&dtdevs_lock);
+
+ if ( iommu_dt_device_is_assigned_lock(dev) )
{
printk(XENLOG_G_ERR "%s already assigned.\n",
dt_node_full_name(dev));
ret = -EINVAL;
}
+
+ spin_unlock(&dtdevs_lock);
+
break;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 07/14] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller
2022-03-08 19:46 ` [XEN][RFC PATCH v3 07/14] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller Vikram Garhwal
@ 2022-03-14 15:58 ` Luca Fancellu
2022-05-17 18:19 ` Julien Grall
1 sibling, 0 replies; 42+ messages in thread
From: Luca Fancellu @ 2022-03-14 15:58 UTC (permalink / raw)
To: Vikram Garhwal
Cc: Xen-devel, sstabellini, julien, Bertrand Marquis, volodymyr_babchuk
> On 8 Mar 2022, at 19:46, Vikram Garhwal <fnu.vikram@xilinx.com> wrote:
>
> Rename iommu_dt_device_is_assigned() to iommu_dt_device_is_assigned_lock().
>
> Moving spin_lock to caller was done to prevent the concurrent access to
> iommu_dt_device_is_assigned while doing add/remove/assign/deassign.
>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
> xen/drivers/passthrough/device_tree.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 98f2aa0dad..b3b04f8e03 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -83,16 +83,14 @@ fail:
> return rc;
> }
>
> -static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
> +static bool_t iommu_dt_device_is_assigned_lock(const struct dt_device_node *dev)
> {
> bool_t assigned = 0;
>
You can add an ASSERT(spin_is_locked(&dtdevs_lock)); to be sure, however the name is pretty clear,
so for me with or without it:
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> if ( !dt_device_is_protected(dev) )
> return 0;
>
> - spin_lock(&dtdevs_lock);
> assigned = !list_empty(&dev->domain_list);
> - spin_unlock(&dtdevs_lock);
>
> return assigned;
> }
> @@ -225,12 +223,17 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>
> if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
> {
> - if ( iommu_dt_device_is_assigned(dev) )
> + spin_lock(&dtdevs_lock);
> +
> + if ( iommu_dt_device_is_assigned_lock(dev) )
> {
> printk(XENLOG_G_ERR "%s already assigned.\n",
> dt_node_full_name(dev));
> ret = -EINVAL;
> }
> +
> + spin_unlock(&dtdevs_lock);
> +
> break;
> }
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 07/14] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller
2022-03-08 19:46 ` [XEN][RFC PATCH v3 07/14] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller Vikram Garhwal
2022-03-14 15:58 ` Luca Fancellu
@ 2022-05-17 18:19 ` Julien Grall
1 sibling, 0 replies; 42+ messages in thread
From: Julien Grall @ 2022-05-17 18:19 UTC (permalink / raw)
To: Vikram Garhwal, xen-devel
Cc: sstabellini, bertrand.marquis, volodymyr_babchuk
Hi,
On 08/03/2022 19:46, Vikram Garhwal wrote:
> Rename iommu_dt_device_is_assigned() to iommu_dt_device_is_assigned_lock().
>
> Moving spin_lock to caller was done to prevent the concurrent access to
> iommu_dt_device_is_assigned while doing add/remove/assign/deassign.
>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
> xen/drivers/passthrough/device_tree.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 98f2aa0dad..b3b04f8e03 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -83,16 +83,14 @@ fail:
> return rc;
> }
>
> -static bool_t iommu_dt_device_is_assigned(const struct dt_device_node *dev)
> +static bool_t iommu_dt_device_is_assigned_lock(const struct dt_device_node *dev)
NIT: We tend to use "_locked" when a function should be called with the
lock taken.
> {
> bool_t assigned = 0;
>
> if ( !dt_device_is_protected(dev) )
> return 0;
>
> - spin_lock(&dtdevs_lock);
> assigned = !list_empty(&dev->domain_list);
> - spin_unlock(&dtdevs_lock);
>
> return assigned;
> }
> @@ -225,12 +223,17 @@ int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
>
> if ( domctl->cmd == XEN_DOMCTL_test_assign_device )
> {
> - if ( iommu_dt_device_is_assigned(dev) )
> + spin_lock(&dtdevs_lock);
Is this actually sufficient? IOW what will ensure that the "dev" doesn't
disappear between the time we look it up (see dt_find_node_by_gpath())
and we check the assignment?
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 42+ messages in thread
* [XEN][RFC PATCH v3 08/14] xen/iommu: protect iommu_add_dt_device() with dtdevs_lock
2022-03-08 19:46 [XEN][RFC PATCH v3 00/14] dynamic node programming using overlay dtbo Vikram Garhwal
` (6 preceding siblings ...)
2022-03-08 19:46 ` [XEN][RFC PATCH v3 07/14] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller Vikram Garhwal
@ 2022-03-08 19:46 ` Vikram Garhwal
2022-03-14 17:34 ` Luca Fancellu
2022-03-08 19:46 ` [XEN][RFC PATCH v3 09/14] xen/iommu: Introduce iommu_remove_dt_device() Vikram Garhwal
` (5 subsequent siblings)
13 siblings, 1 reply; 42+ messages in thread
From: Vikram Garhwal @ 2022-03-08 19:46 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk, Vikram Garhwal
Protect iommu_add_dt_device() with dtdevs_lock to prevent concurrent access add.
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
xen/drivers/passthrough/device_tree.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index b3b04f8e03..776809a8f2 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -145,6 +145,8 @@ int iommu_add_dt_device(struct dt_device_node *np)
if ( dev_iommu_fwspec_get(dev) )
return 0;
+ spin_lock(&dtdevs_lock);
+
/*
* According to the Documentation/devicetree/bindings/iommu/iommu.txt
* from Linux.
@@ -157,7 +159,10 @@ int iommu_add_dt_device(struct dt_device_node *np)
* these callback implemented.
*/
if ( !ops->add_device || !ops->dt_xlate )
- return -EINVAL;
+ {
+ rc = -EINVAL;
+ goto fail;
+ }
if ( !dt_device_is_available(iommu_spec.np) )
break;
@@ -188,6 +193,8 @@ int iommu_add_dt_device(struct dt_device_node *np)
if ( rc < 0 )
iommu_fwspec_free(dev);
+fail:
+ spin_unlock(&dtdevs_lock);
return rc;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 08/14] xen/iommu: protect iommu_add_dt_device() with dtdevs_lock
2022-03-08 19:46 ` [XEN][RFC PATCH v3 08/14] xen/iommu: protect iommu_add_dt_device() with dtdevs_lock Vikram Garhwal
@ 2022-03-14 17:34 ` Luca Fancellu
0 siblings, 0 replies; 42+ messages in thread
From: Luca Fancellu @ 2022-03-14 17:34 UTC (permalink / raw)
To: Vikram Garhwal
Cc: Xen-devel, sstabellini, julien, Bertrand Marquis, volodymyr_babchuk
> On 8 Mar 2022, at 19:46, Vikram Garhwal <fnu.vikram@xilinx.com> wrote:
>
> Protect iommu_add_dt_device() with dtdevs_lock to prevent concurrent access add.
>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> xen/drivers/passthrough/device_tree.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index b3b04f8e03..776809a8f2 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -145,6 +145,8 @@ int iommu_add_dt_device(struct dt_device_node *np)
> if ( dev_iommu_fwspec_get(dev) )
> return 0;
>
> + spin_lock(&dtdevs_lock);
> +
> /*
> * According to the Documentation/devicetree/bindings/iommu/iommu.txt
> * from Linux.
> @@ -157,7 +159,10 @@ int iommu_add_dt_device(struct dt_device_node *np)
> * these callback implemented.
> */
> if ( !ops->add_device || !ops->dt_xlate )
> - return -EINVAL;
> + {
> + rc = -EINVAL;
> + goto fail;
> + }
>
> if ( !dt_device_is_available(iommu_spec.np) )
> break;
> @@ -188,6 +193,8 @@ int iommu_add_dt_device(struct dt_device_node *np)
> if ( rc < 0 )
> iommu_fwspec_free(dev);
>
> +fail:
> + spin_unlock(&dtdevs_lock);
> return rc;
> }
>
>
^ permalink raw reply [flat|nested] 42+ messages in thread
* [XEN][RFC PATCH v3 09/14] xen/iommu: Introduce iommu_remove_dt_device()
2022-03-08 19:46 [XEN][RFC PATCH v3 00/14] dynamic node programming using overlay dtbo Vikram Garhwal
` (7 preceding siblings ...)
2022-03-08 19:46 ` [XEN][RFC PATCH v3 08/14] xen/iommu: protect iommu_add_dt_device() with dtdevs_lock Vikram Garhwal
@ 2022-03-08 19:46 ` Vikram Garhwal
2022-03-14 17:50 ` Luca Fancellu
2022-03-08 19:47 ` [XEN][RFC PATCH v3 10/14] xen/arm: Implement device tree node removal functionalities Vikram Garhwal
` (4 subsequent siblings)
13 siblings, 1 reply; 42+ messages in thread
From: Vikram Garhwal @ 2022-03-08 19:46 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk,
Vikram Garhwal, Jan Beulich, Paul Durrant
Remove master device from the IOMMU.
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
xen/drivers/passthrough/device_tree.c | 38 +++++++++++++++++++++++++++
xen/include/xen/iommu.h | 2 ++
2 files changed, 40 insertions(+)
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 776809a8f2..5cd0b20c77 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -125,6 +125,44 @@ int iommu_release_dt_devices(struct domain *d)
return 0;
}
+int iommu_remove_dt_device(struct dt_device_node *np)
+{
+ const struct iommu_ops *ops = iommu_get_ops();
+ struct device *dev = dt_to_dev(np);
+ int rc;
+
+ if ( !ops )
+ return -EOPNOTSUPP;
+
+ spin_lock(&dtdevs_lock);
+
+ if ( iommu_dt_device_is_assigned_lock(np) ) {
+ rc = -EBUSY;
+ goto fail;
+ }
+
+ /*
+ * The driver which supports generic IOMMU DT bindings must have
+ * these callback implemented.
+ */
+ if ( !ops->remove_device ) {
+ rc = -EOPNOTSUPP;
+ goto fail;
+ }
+
+ /*
+ * Remove master device from the IOMMU if latter is present and available.
+ */
+ rc = ops->remove_device(0, dev);
+
+ if ( rc == 0 )
+ iommu_fwspec_free(dev);
+
+fail:
+ spin_unlock(&dtdevs_lock);
+ return rc;
+}
+
int iommu_add_dt_device(struct dt_device_node *np)
{
const struct iommu_ops *ops = iommu_get_ops();
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index b18e7760a2..64871e5cb8 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -215,6 +215,8 @@ int iommu_release_dt_devices(struct domain *d);
*/
int iommu_add_dt_device(struct dt_device_node *np);
+int iommu_remove_dt_device(struct dt_device_node *np);
+
int iommu_do_dt_domctl(struct xen_domctl *, struct domain *,
XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
--
2.17.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 09/14] xen/iommu: Introduce iommu_remove_dt_device()
2022-03-08 19:46 ` [XEN][RFC PATCH v3 09/14] xen/iommu: Introduce iommu_remove_dt_device() Vikram Garhwal
@ 2022-03-14 17:50 ` Luca Fancellu
2022-12-07 5:21 ` Vikram Garhwal
0 siblings, 1 reply; 42+ messages in thread
From: Luca Fancellu @ 2022-03-14 17:50 UTC (permalink / raw)
To: Vikram Garhwal
Cc: Xen-devel, sstabellini, julien, Bertrand Marquis,
volodymyr_babchuk, Jan Beulich, Paul Durrant
> +int iommu_remove_dt_device(struct dt_device_node *np)
> +{
> + const struct iommu_ops *ops = iommu_get_ops();
> + struct device *dev = dt_to_dev(np);
> + int rc;
> +
> + if ( !ops )
> + return -EOPNOTSUPP;
Here we have that the counterpart iommu_add_dt_device returns EINVAL here and...
> +
> + spin_lock(&dtdevs_lock);
> +
> + if ( iommu_dt_device_is_assigned_lock(np) ) {
> + rc = -EBUSY;
> + goto fail;
> + }
> +
> + /*
> + * The driver which supports generic IOMMU DT bindings must have
> + * these callback implemented.
> + */
> + if ( !ops->remove_device ) {
> + rc = -EOPNOTSUPP;
… here (for !ops->add_device), so I’m wondering if there is a mistake.
> + goto fail;
> + }
> +
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 09/14] xen/iommu: Introduce iommu_remove_dt_device()
2022-03-14 17:50 ` Luca Fancellu
@ 2022-12-07 5:21 ` Vikram Garhwal
0 siblings, 0 replies; 42+ messages in thread
From: Vikram Garhwal @ 2022-12-07 5:21 UTC (permalink / raw)
To: Luca Fancellu, Vikram Garhwal
Cc: Xen-devel, sstabellini, julien, Bertrand Marquis,
volodymyr_babchuk, Jan Beulich, Paul Durrant
Hi Luca,
On 3/14/22 10:50 AM, Luca Fancellu wrote:
>> +int iommu_remove_dt_device(struct dt_device_node *np)
>> +{
>> + const struct iommu_ops *ops = iommu_get_ops();
>> + struct device *dev = dt_to_dev(np);
>> + int rc;
>> +
>> + if ( !ops )
>> + return -EOPNOTSUPP;
> Here we have that the counterpart iommu_add_dt_device returns EINVAL here and...
> I add EINVAL here in v1 but Julien suggested to change it ot EOPNOTSUPP.
>> +
>> + spin_lock(&dtdevs_lock);
>> +
>> + if ( iommu_dt_device_is_assigned_lock(np) ) {
>> + rc = -EBUSY;
>> + goto fail;
>> + }
>> +
>> + /*
>> + * The driver which supports generic IOMMU DT bindings must have
>> + * these callback implemented.
>> + */
>> + if ( !ops->remove_device ) {
>> + rc = -EOPNOTSUPP;
> … here (for !ops->add_device), so I’m wondering if there is a mistake.
>
>> + goto fail;
>> + }
>> +
^ permalink raw reply [flat|nested] 42+ messages in thread
* [XEN][RFC PATCH v3 10/14] xen/arm: Implement device tree node removal functionalities
2022-03-08 19:46 [XEN][RFC PATCH v3 00/14] dynamic node programming using overlay dtbo Vikram Garhwal
` (8 preceding siblings ...)
2022-03-08 19:46 ` [XEN][RFC PATCH v3 09/14] xen/iommu: Introduce iommu_remove_dt_device() Vikram Garhwal
@ 2022-03-08 19:47 ` Vikram Garhwal
2022-03-15 10:10 ` Luca Fancellu
` (2 more replies)
2022-03-08 19:47 ` [XEN][RFC PATCH v3 11/14] xen/arm: Implement device tree node addition functionalities Vikram Garhwal
` (3 subsequent siblings)
13 siblings, 3 replies; 42+ messages in thread
From: Vikram Garhwal @ 2022-03-08 19:47 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk,
Vikram Garhwal, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu
Introduce sysctl XEN_SYSCTL_dt_overlay to remove device-tree nodes added using
device tree overlay.
xl overlay remove file.dtbo:
Removes all the nodes in a given dtbo.
First, removes IRQ permissions and MMIO accesses. Next, it finds the nodes
in dt_host and delete the device node entries from dt_host.
The nodes get removed only if it is not used by any of dom0 or domio.
Also, added overlay_track struct to keep the track of added node through device
tree overlay. overlay_track has dt_host_new which is unflattened form of updated
fdt and name of overlay nodes. When a node is removed, we also free the memory
used by overlay_track for the particular overlay node.
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
xen/common/Makefile | 1 +
xen/common/dt_overlay.c | 447 +++++++++++++++++++++++++++++++++++
xen/common/sysctl.c | 10 +
xen/include/public/sysctl.h | 18 ++
xen/include/xen/dt_overlay.h | 47 ++++
5 files changed, 523 insertions(+)
create mode 100644 xen/common/dt_overlay.c
create mode 100644 xen/include/xen/dt_overlay.h
diff --git a/xen/common/Makefile b/xen/common/Makefile
index dc8d3a13f5..2eb5734f8e 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -54,6 +54,7 @@ obj-y += wait.o
obj-bin-y += warning.init.o
obj-$(CONFIG_XENOPROF) += xenoprof.o
obj-y += xmalloc_tlsf.o
+obj-$(CONFIG_OVERLAY_DTB) += dt_overlay.o
obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma lzo unlzo unlz4 unzstd earlycpio,$(n).init.o)
diff --git a/xen/common/dt_overlay.c b/xen/common/dt_overlay.c
new file mode 100644
index 0000000000..fcb31de495
--- /dev/null
+++ b/xen/common/dt_overlay.c
@@ -0,0 +1,447 @@
+/*
+ * xen/common/dt_overlay.c
+ *
+ * Device tree overlay support in Xen.
+ *
+ * Copyright (c) 2021 Xilinx Inc.
+ * Written by Vikram Garhwal <fnu.vikram@xilinx.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#include <xen/iocap.h>
+#include <xen/xmalloc.h>
+#include <asm/domain_build.h>
+#include <xen/dt_overlay.h>
+#include <xen/guest_access.h>
+
+static LIST_HEAD(overlay_tracker);
+static DEFINE_SPINLOCK(overlay_lock);
+
+static int dt_overlay_remove_node(struct dt_device_node *device_node)
+{
+ struct dt_device_node *np;
+ struct dt_device_node *parent_node;
+ struct dt_device_node *current_node;
+
+ parent_node = device_node->parent;
+
+ current_node = parent_node;
+
+ if ( parent_node == NULL )
+ {
+ dt_dprintk("%s's parent node not found\n", device_node->name);
+ return -EFAULT;
+ }
+
+ np = parent_node->child;
+
+ if ( np == NULL )
+ {
+ dt_dprintk("parent node %s's not found\n", parent_node->name);
+ return -EFAULT;
+ }
+
+ /* If node to be removed is only child node or first child. */
+ if ( !dt_node_cmp(np->full_name, device_node->full_name) )
+ {
+ current_node->allnext = np->allnext;
+
+ /* If node is first child but not the only child. */
+ if ( np->sibling != NULL )
+ current_node->child = np->sibling;
+ else
+ /* If node is only child. */
+ current_node->child = NULL;
+ return 0;
+ }
+
+ for ( np = parent_node->child; np->sibling != NULL; np = np->sibling )
+ {
+ current_node = np;
+ if ( !dt_node_cmp(np->sibling->full_name, device_node->full_name) )
+ {
+ /* Found the node. Now we remove it. */
+ current_node->allnext = np->allnext->allnext;
+
+ if ( np->sibling->sibling )
+ current_node->sibling = np->sibling->sibling;
+ else
+ current_node->sibling = NULL;
+
+ break;
+ }
+ }
+
+ return 0;
+}
+
+/* Basic sanity check for the dtbo tool stack provided to Xen. */
+static int check_overlay_fdt(const void *overlay_fdt, uint32_t overlay_fdt_size)
+{
+ if ( (fdt_totalsize(overlay_fdt) != overlay_fdt_size) ||
+ fdt_check_header(overlay_fdt) )
+ {
+ printk(XENLOG_ERR "The overlay FDT is not a valid Flat Device Tree\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static unsigned int overlay_node_count(void *fdto)
+{
+ unsigned int num_overlay_nodes = 0;
+ int fragment;
+
+ fdt_for_each_subnode(fragment, fdto, 0)
+ {
+
+ int subnode;
+ int overlay;
+
+ overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
+
+ /*
+ * Overlay value can be < 0. But fdt_for_each_subnode() loop checks for
+ * overlay >= 0. So, no need for a overlay>=0 check here.
+ */
+
+ fdt_for_each_subnode(subnode, fdto, overlay)
+ {
+ num_overlay_nodes++;
+ }
+ }
+
+ return num_overlay_nodes;
+}
+
+/*
+ * overlay_get_nodes_info will get the all node's full name with path. This is
+ * useful when checking node for duplication i.e. dtbo tries to add nodes which
+ * already exists in device tree.
+ */
+static int overlay_get_nodes_info(const void *fdto, char ***nodes_full_path,
+ unsigned int num_overlay_nodes)
+{
+ int fragment;
+ unsigned int node_num = 0;
+
+ *nodes_full_path = xmalloc_bytes(num_overlay_nodes * sizeof(char *));
+
+ if ( *nodes_full_path == NULL )
+ return -ENOMEM;
+ memset(*nodes_full_path, 0x0, num_overlay_nodes * sizeof(char *));
+
+ fdt_for_each_subnode(fragment, fdto, 0)
+ {
+ int target;
+ int overlay;
+ int subnode;
+ const char *target_path;
+
+ target = fdt_overlay_target_offset(device_tree_flattened, fdto,
+ fragment, &target_path);
+ if ( target < 0 )
+ return target;
+
+ overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
+
+ /*
+ * Overlay value can be < 0. But fdt_for_each_subnode() loop checks for
+ * overlay >= 0. So, no need for a overlay>=0 check here.
+ */
+ fdt_for_each_subnode(subnode, fdto, overlay)
+ {
+ const char *node_name = NULL;
+ unsigned int node_name_len = 0;
+ unsigned int target_path_len = strlen(target_path);
+ unsigned int node_full_name_len = 0;
+
+ node_name = fdt_get_name(fdto, subnode, &node_name_len);
+
+ if ( node_name == NULL )
+ return -EINVAL;
+
+ /*
+ * Magic number 2 is for adding '/'. This is done to keep the
+ * node_full_name in the correct full node name format.
+ */
+ node_full_name_len = target_path_len + node_name_len + 2;
+
+ (*nodes_full_path)[node_num] = xmalloc_bytes(node_full_name_len);
+
+ if ( (*nodes_full_path)[node_num] == NULL )
+ return -ENOMEM;
+
+ memcpy((*nodes_full_path)[node_num], target_path, target_path_len);
+
+ (*nodes_full_path)[node_num][target_path_len] = '/';
+
+ memcpy((*nodes_full_path)[node_num] + target_path_len + 1, node_name,
+ node_name_len);
+
+ (*nodes_full_path)[node_num][node_full_name_len - 1] = '\0';
+
+ node_num++;
+ }
+ }
+
+ return 0;
+}
+
+/* Remove nodes from dt_host. */
+static int remove_nodes(char **full_dt_node_path, int **nodes_irq,
+ int *node_num_irq, unsigned int num_nodes)
+{
+ struct domain *d = hardware_domain;
+ int rc = 0;
+ struct dt_device_node *overlay_node;
+ unsigned int naddr;
+ unsigned int i, j, nirq;
+ u64 addr, size;
+ domid_t domid = 0;
+
+ for ( j = 0; j < num_nodes; j++ )
+ {
+ dt_dprintk("Finding node %s in the dt_host\n", full_dt_node_path[j]);
+
+ overlay_node = dt_find_node_by_path(full_dt_node_path[j]);
+
+ if ( overlay_node == NULL )
+ {
+ printk(XENLOG_ERR "Device %s is not present in the tree. Removing nodes failed\n",
+ full_dt_node_path[j]);
+ return -EINVAL;
+ }
+
+ domid = dt_device_used_by(overlay_node);
+
+ dt_dprintk("Checking if node %s is used by any domain\n",
+ full_dt_node_path[j]);
+
+ /* Remove the node iff it's assigned to domain 0 or domain io. */
+ if ( domid != 0 && domid != DOMID_IO )
+ {
+ printk(XENLOG_ERR "Device %s as it is being used by domain %d. Removing nodes failed\n",
+ full_dt_node_path[j], domid);
+ return -EINVAL;
+ }
+
+ dt_dprintk("Removing node: %s\n", full_dt_node_path[j]);
+
+ nirq = node_num_irq[j];
+
+ /* Remove IRQ permission */
+ for ( i = 0; i < nirq; i++ )
+ {
+ rc = nodes_irq[j][i];
+ /*
+ * TODO: We don't handle shared IRQs for now. So, it is assumed that
+ * the IRQs was not shared with another domain.
+ */
+ rc = irq_deny_access(d, rc);
+ if ( rc )
+ {
+ printk(XENLOG_ERR "unable to revoke access for irq %u for %s\n",
+ i, dt_node_full_name(overlay_node));
+ return rc;
+ }
+ }
+
+ rc = iommu_remove_dt_device(overlay_node);
+ if ( rc != 0 && rc != -ENXIO )
+ return rc;
+
+ naddr = dt_number_of_address(overlay_node);
+
+ /* Remove mmio access. */
+ for ( i = 0; i < naddr; i++ )
+ {
+ rc = dt_device_get_address(overlay_node, i, &addr, &size);
+ if ( rc )
+ {
+ printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
+ i, dt_node_full_name(overlay_node));
+ return rc;
+ }
+
+ rc = iomem_deny_access(d, paddr_to_pfn(addr),
+ paddr_to_pfn(PAGE_ALIGN(addr + size - 1)));
+ if ( rc )
+ {
+ printk(XENLOG_ERR "Unable to remove dom%d access to"
+ " 0x%"PRIx64" - 0x%"PRIx64"\n",
+ d->domain_id,
+ addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
+ return rc;
+ }
+ }
+
+ rc = dt_overlay_remove_node(overlay_node);
+ if ( rc )
+ return rc;
+ }
+
+ return rc;
+}
+
+/*
+ * First finds the device node to remove. Check if the device is being used by
+ * any dom and finally remove it from dt_host. IOMMU is already being taken care
+ * while destroying the domain.
+ */
+static long handle_remove_overlay_nodes(char **full_dt_nodes_path,
+ unsigned int num_nodes)
+{
+ int rc = 0;
+ struct overlay_track *entry, *temp, *track;
+ bool found_entry = false;
+ unsigned int i;
+
+ spin_lock(&overlay_lock);
+
+ /*
+ * First check if dtbo is correct i.e. it should one of the dtbo which was
+ * used when dynamically adding the node.
+ * Limitation: Cases with same node names but different property are not
+ * supported currently. We are relying on user to provide the same dtbo
+ * as it was used when adding the nodes.
+ */
+ list_for_each_entry_safe( entry, temp, &overlay_tracker, entry )
+ {
+ /* Checking the num of nodes first. If not same skip to next entry. */
+ if ( num_nodes == entry->num_nodes )
+ {
+ for ( i = 0; i < num_nodes; i++ )
+ {
+ if ( strcmp(full_dt_nodes_path[i], entry->nodes_fullname[i]) )
+ {
+ /* Node name didn't match. Skip to next entry. */
+ break;
+ }
+ }
+
+ /* Found one tracker with all node name matching. */
+ track = entry;
+ found_entry = true;
+ break;
+ }
+ }
+
+ if ( found_entry == false )
+ {
+ rc = -EINVAL;
+
+ printk(XENLOG_ERR "Cannot find any matching tracker with input dtbo."
+ " Removing nodes is supported for only prior added dtbo. Please"
+ " provide a valid dtbo which was used to add the nodes.\n");
+ goto out;
+
+ }
+
+ rc = remove_nodes(full_dt_nodes_path, entry->nodes_irq, entry->node_num_irq,
+ num_nodes);
+
+ if ( rc )
+ {
+ printk(XENLOG_ERR "Removing node failed\n");
+ goto out;
+ }
+
+ list_del(&entry->entry);
+
+ for ( i = 0; i < entry->num_nodes && entry->nodes_fullname[i] != NULL; i++ )
+ {
+ xfree(entry->nodes_fullname[i]);
+ }
+ xfree(entry->nodes_fullname);
+ for ( i = 0; i < entry->num_nodes && entry->nodes_irq[i] != NULL; i++ )
+ {
+ xfree(entry->nodes_irq[i]);
+ }
+ xfree(entry->nodes_irq);
+ xfree(entry->node_num_irq);
+ xfree(entry->dt_host_new);
+ xfree(entry->fdt);
+ xfree(entry);
+
+out:
+ spin_unlock(&overlay_lock);
+ return rc;
+}
+
+long dt_sysctl(struct xen_sysctl *op)
+{
+ long ret = 0;
+ void *overlay_fdt;
+ char **nodes_full_path = NULL;
+ unsigned int num_overlay_nodes = 0;
+
+ if ( op->u.dt_overlay.overlay_fdt_size <= 0 )
+ return -EINVAL;
+
+ overlay_fdt = xmalloc_bytes(op->u.dt_overlay.overlay_fdt_size);
+
+ if ( overlay_fdt == NULL )
+ return -ENOMEM;
+
+ ret = copy_from_guest(overlay_fdt, op->u.dt_overlay.overlay_fdt,
+ op->u.dt_overlay.overlay_fdt_size);
+ if ( ret )
+ {
+ gprintk(XENLOG_ERR, "copy from guest failed\n");
+ xfree(overlay_fdt);
+
+ return -EFAULT;
+ }
+
+ switch ( op->u.dt_overlay.overlay_op )
+ {
+ case XEN_SYSCTL_DT_OVERLAY_REMOVE:
+ ret = check_overlay_fdt(overlay_fdt,
+ op->u.dt_overlay.overlay_fdt_size);
+ if ( ret )
+ {
+ ret = -EFAULT;
+ break;
+ }
+
+ num_overlay_nodes = overlay_node_count(overlay_fdt);
+ if ( num_overlay_nodes == 0 )
+ {
+ ret = -ENOMEM;
+ break;
+ }
+
+ ret = overlay_get_nodes_info(overlay_fdt, &nodes_full_path,
+ num_overlay_nodes);
+ if ( ret )
+ break;
+
+ ret = handle_remove_overlay_nodes(nodes_full_path,
+ num_overlay_nodes);
+ break;
+
+ default:
+ break;
+ }
+
+ if ( nodes_full_path != NULL )
+ {
+ int i;
+ for ( i = 0; i < num_overlay_nodes && nodes_full_path[i] != NULL; i++ )
+ {
+ xfree(nodes_full_path[i]);
+ }
+ xfree(nodes_full_path);
+ }
+
+ return ret;
+}
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index fc4a0b31d6..d685c07159 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -29,6 +29,10 @@
#include <xen/livepatch.h>
#include <xen/coverage.h>
+#ifdef CONFIG_OVERLAY_DTB
+#include <xen/dt_overlay.h>
+#endif
+
long cf_check do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
{
long ret = 0;
@@ -482,6 +486,12 @@ long cf_check do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
copyback = 1;
break;
+#ifdef CONFIG_OVERLAY_DTB
+ case XEN_SYSCTL_overlay:
+ ret = dt_sysctl(op);
+ break;
+#endif
+
default:
ret = arch_do_sysctl(op, u_sysctl);
copyback = 0;
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 55252e97f2..e256aeb7c6 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1069,6 +1069,22 @@ typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
#endif
+#define XEN_SYSCTL_DT_OVERLAY_REMOVE 2
+
+/*
+ * XEN_SYSCTL_dt_overlay
+ * Performs addition/removal of device tree nodes under parent node using dtbo.
+ * This does in three steps:
+ * - Adds/Removes the nodes from dt_host.
+ * - Adds/Removes IRQ permission for the nodes.
+ * - Adds/Removes MMIO accesses.
+ */
+struct xen_sysctl_dt_overlay {
+ XEN_GUEST_HANDLE_64(void) overlay_fdt;
+ uint32_t overlay_fdt_size; /* Overlay dtb size. */
+ uint8_t overlay_op; /* Add or remove. */
+};
+
struct xen_sysctl {
uint32_t cmd;
#define XEN_SYSCTL_readconsole 1
@@ -1099,6 +1115,7 @@ struct xen_sysctl {
#define XEN_SYSCTL_livepatch_op 27
/* #define XEN_SYSCTL_set_parameter 28 */
#define XEN_SYSCTL_get_cpu_policy 29
+#define XEN_SYSCTL_dt_overlay 30
uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
union {
struct xen_sysctl_readconsole readconsole;
@@ -1129,6 +1146,7 @@ struct xen_sysctl {
#if defined(__i386__) || defined(__x86_64__)
struct xen_sysctl_cpu_policy cpu_policy;
#endif
+ struct xen_sysctl_dt_overlay dt_overlay;
uint8_t pad[128];
} u;
};
diff --git a/xen/include/xen/dt_overlay.h b/xen/include/xen/dt_overlay.h
new file mode 100644
index 0000000000..525818b77c
--- /dev/null
+++ b/xen/include/xen/dt_overlay.h
@@ -0,0 +1,47 @@
+/*
+ * xen/common/dt_overlay.c
+ *
+ * Device tree overlay suppoert in Xen.
+ *
+ * Copyright (c) 2021 Xilinx Inc.
+ * Written by Vikram Garhwal <fnu.vikram@xilinx.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#ifndef __XEN_DT_SYSCTL_H__
+#define __XEN_DT_SYSCTL_H__
+
+#include <xen/list.h>
+#include <xen/libfdt/libfdt.h>
+#include <xen/device_tree.h>
+#include <public/sysctl.h>
+
+/*
+ * overlay_node_track describes information about added nodes through dtbo.
+ * @entry: List pointer.
+ * @dt_host_new: Pointer to the updated dt_host_new unflattened 'updated fdt'.
+ * @fdt: Stores the fdt.
+ * @nodes_fullname: Stores the full name of nodes.
+ * @nodes_irq: Stores the IRQ added from overlay dtb.
+ * @node_num_irq: Stores num of IRQ for each node in overlay dtb.
+ * @num_nodes: Stores total number of nodes in overlay dtb.
+ */
+struct overlay_track {
+ struct list_head entry;
+ struct dt_device_node *dt_host_new;
+ void *fdt;
+ char **nodes_fullname;
+ int **nodes_irq;
+ int *node_num_irq;
+ unsigned int num_nodes;
+};
+
+long dt_sysctl(struct xen_sysctl *op);
+#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 10/14] xen/arm: Implement device tree node removal functionalities
2022-03-08 19:47 ` [XEN][RFC PATCH v3 10/14] xen/arm: Implement device tree node removal functionalities Vikram Garhwal
@ 2022-03-15 10:10 ` Luca Fancellu
2022-05-18 18:31 ` Julien Grall
2022-05-19 8:13 ` Julien Grall
2 siblings, 0 replies; 42+ messages in thread
From: Luca Fancellu @ 2022-03-15 10:10 UTC (permalink / raw)
To: Vikram Garhwal
Cc: Xen-devel, Stefano Stabellini, julien, Bertrand Marquis,
volodymyr_babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu
>
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index dc8d3a13f5..2eb5734f8e 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -54,6 +54,7 @@ obj-y += wait.o
> obj-bin-y += warning.init.o
> obj-$(CONFIG_XENOPROF) += xenoprof.o
> obj-y += xmalloc_tlsf.o
> +obj-$(CONFIG_OVERLAY_DTB) += dt_overlay.o
I think the entries are in alphabetical order, this should be added after += domain.o
> +/*
> + * overlay_get_nodes_info will get the all node's full name with path. This is
> + * useful when checking node for duplication i.e. dtbo tries to add nodes which
> + * already exists in device tree.
> + */
> +static int overlay_get_nodes_info(const void *fdto, char ***nodes_full_path,
> + unsigned int num_overlay_nodes)
> +{
> + int fragment;
> + unsigned int node_num = 0;
> +
> + *nodes_full_path = xmalloc_bytes(num_overlay_nodes * sizeof(char *));
Here you can use xzalloc_bytes and...
> +
> + if ( *nodes_full_path == NULL )
> + return -ENOMEM;
> + memset(*nodes_full_path, 0x0, num_overlay_nodes * sizeof(char *));
Get rid of this memset
> +
> +/* Remove nodes from dt_host. */
> +static int remove_nodes(char **full_dt_node_path, int **nodes_irq,
> + int *node_num_irq, unsigned int num_nodes)
> +{
> + struct domain *d = hardware_domain;
> + int rc = 0;
> + struct dt_device_node *overlay_node;
> + unsigned int naddr;
> + unsigned int i, j, nirq;
> + u64 addr, size;
> + domid_t domid = 0;
> +
> + for ( j = 0; j < num_nodes; j++ )
> + {
> + dt_dprintk("Finding node %s in the dt_host\n", full_dt_node_path[j]);
> +
> + overlay_node = dt_find_node_by_path(full_dt_node_path[j]);
> +
> + if ( overlay_node == NULL )
> + {
> + printk(XENLOG_ERR "Device %s is not present in the tree. Removing nodes failed\n",
> + full_dt_node_path[j]);
> + return -EINVAL;
> + }
> +
> + domid = dt_device_used_by(overlay_node);
> +
> + dt_dprintk("Checking if node %s is used by any domain\n",
> + full_dt_node_path[j]);
> +
> + /* Remove the node iff it's assigned to domain 0 or domain io. */
> + if ( domid != 0 && domid != DOMID_IO )
> + {
> + printk(XENLOG_ERR "Device %s as it is being used by domain %d. Removing nodes failed\n",
> + full_dt_node_path[j], domid);
> + return -EINVAL;
> + }
> +
> + dt_dprintk("Removing node: %s\n", full_dt_node_path[j]);
> +
> + nirq = node_num_irq[j];
> +
> + /* Remove IRQ permission */
> + for ( i = 0; i < nirq; i++ )
> + {
> + rc = nodes_irq[j][i];
> + /*
> + * TODO: We don't handle shared IRQs for now. So, it is assumed that
> + * the IRQs was not shared with another domain.
> + */
> + rc = irq_deny_access(d, rc);
> + if ( rc )
> + {
> + printk(XENLOG_ERR "unable to revoke access for irq %u for %s\n",
> + i, dt_node_full_name(overlay_node));
> + return rc;
> + }
> + }
> +
> + rc = iommu_remove_dt_device(overlay_node);
> + if ( rc != 0 && rc != -ENXIO )
> + return rc;
> +
> + naddr = dt_number_of_address(overlay_node);
> +
> + /* Remove mmio access. */
> + for ( i = 0; i < naddr; i++ )
> + {
> + rc = dt_device_get_address(overlay_node, i, &addr, &size);
> + if ( rc )
> + {
> + printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
> + i, dt_node_full_name(overlay_node));
> + return rc;
> + }
> +
> + rc = iomem_deny_access(d, paddr_to_pfn(addr),
> + paddr_to_pfn(PAGE_ALIGN(addr + size - 1)));
> + if ( rc )
> + {
> + printk(XENLOG_ERR "Unable to remove dom%d access to"
> + " 0x%"PRIx64" - 0x%"PRIx64"\n",
> + d->domain_id,
> + addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
NIT: here in each line under XENLOG_ERR, there is an extra space, these lines
Could be aligned to XENLOG_ERR, just for code style purpose.
> + return rc;
> + }
> + }
> +
> + rc = dt_overlay_remove_node(overlay_node);
> + if ( rc )
> + return rc;
> + }
> +
> + return rc;
> +}
> +
>
> +long dt_sysctl(struct xen_sysctl *op)
> +{
> + long ret = 0;
> + void *overlay_fdt;
> + char **nodes_full_path = NULL;
> + unsigned int num_overlay_nodes = 0;
> +
> + if ( op->u.dt_overlay.overlay_fdt_size <= 0 )
> + return -EINVAL;
> +
> + overlay_fdt = xmalloc_bytes(op->u.dt_overlay.overlay_fdt_size);
> +
> + if ( overlay_fdt == NULL )
> + return -ENOMEM;
> +
> + ret = copy_from_guest(overlay_fdt, op->u.dt_overlay.overlay_fdt,
> + op->u.dt_overlay.overlay_fdt_size);
> + if ( ret )
> + {
> + gprintk(XENLOG_ERR, "copy from guest failed\n");
> + xfree(overlay_fdt);
> +
> + return -EFAULT;
> + }
> +
> + switch ( op->u.dt_overlay.overlay_op )
> + {
> + case XEN_SYSCTL_DT_OVERLAY_REMOVE:
> + ret = check_overlay_fdt(overlay_fdt,
> + op->u.dt_overlay.overlay_fdt_size);
> + if ( ret )
> + {
> + ret = -EFAULT;
> + break;
> + }
> +
> + num_overlay_nodes = overlay_node_count(overlay_fdt);
> + if ( num_overlay_nodes == 0 )
> + {
> + ret = -ENOMEM;
> + break;
> + }
> +
> + ret = overlay_get_nodes_info(overlay_fdt, &nodes_full_path,
> + num_overlay_nodes);
> + if ( ret )
> + break;
> +
> + ret = handle_remove_overlay_nodes(nodes_full_path,
> + num_overlay_nodes);
> + break;
> +
> + default:
> + break;
> + }
> +
> + if ( nodes_full_path != NULL )
> + {
> + int I;
unsigned int
> + for ( i = 0; i < num_overlay_nodes && nodes_full_path[i] != NULL; i++ )
> + {
> + xfree(nodes_full_path[i]);
> + }
> + xfree(nodes_full_path);
> + }
> +
> + return ret;
> +}
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index fc4a0b31d6..d685c07159 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -29,6 +29,10 @@
> #include <xen/livepatch.h>
> #include <xen/coverage.h>
>
> +#ifdef CONFIG_OVERLAY_DTB
> +#include <xen/dt_overlay.h>
> +#endif
Maybe this header can be included anyway, removing ifdefs, and...
> +
> long cf_check do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> {
> long ret = 0;
> @@ -482,6 +486,12 @@ long cf_check do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> copyback = 1;
> break;
>
> +#ifdef CONFIG_OVERLAY_DTB
> + case XEN_SYSCTL_overlay:
> + ret = dt_sysctl(op);
> + break;
> +#endif
Also here you can remove ifdefs and use the header to switch between the real implementation
or a static inline returning error if CONFIG_OVERLAY_DTB is not enabled, take a look in
livepatch_op(struct xen_sysctl_livepatch_op *op).
dt_sysctl can take struct xen_sysctl_dt_overlay* as input.
> +
> default:
> ret = arch_do_sysctl(op, u_sysctl);
> copyback = 0;
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index 55252e97f2..e256aeb7c6 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -1069,6 +1069,22 @@ typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
> DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
> #endif
>
> +#define XEN_SYSCTL_DT_OVERLAY_REMOVE 2
> +
> +/*
> + * XEN_SYSCTL_dt_overlay
> + * Performs addition/removal of device tree nodes under parent node using dtbo.
> + * This does in three steps:
> + * - Adds/Removes the nodes from dt_host.
> + * - Adds/Removes IRQ permission for the nodes.
> + * - Adds/Removes MMIO accesses.
> + */
> +struct xen_sysctl_dt_overlay {
> + XEN_GUEST_HANDLE_64(void) overlay_fdt;
> + uint32_t overlay_fdt_size; /* Overlay dtb size. */
> + uint8_t overlay_op; /* Add or remove. */
> +};
> +
> struct xen_sysctl {
> uint32_t cmd;
> #define XEN_SYSCTL_readconsole 1
> @@ -1099,6 +1115,7 @@ struct xen_sysctl {
> #define XEN_SYSCTL_livepatch_op 27
> /* #define XEN_SYSCTL_set_parameter 28 */
> #define XEN_SYSCTL_get_cpu_policy 29
> +#define XEN_SYSCTL_dt_overlay 30
> uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
> union {
> struct xen_sysctl_readconsole readconsole;
> @@ -1129,6 +1146,7 @@ struct xen_sysctl {
> #if defined(__i386__) || defined(__x86_64__)
> struct xen_sysctl_cpu_policy cpu_policy;
> #endif
> + struct xen_sysctl_dt_overlay dt_overlay;
Here I would need an opinion from someone more experienced, but I think when a change
is made in this structure, XEN_SYSCTL_INTERFACE_VERSION should be bumped?
> uint8_t pad[128];
> } u;
> };
> diff --git a/xen/include/xen/dt_overlay.h b/xen/include/xen/dt_overlay.h
> new file mode 100644
> index 0000000000..525818b77c
> --- /dev/null
> +++ b/xen/include/xen/dt_overlay.h
> @@ -0,0 +1,47 @@
> +/*
> + * xen/common/dt_overlay.c
Typo: dt_overlay.h
> + *
> + * Device tree overlay suppoert in Xen.
Typo: support
> + *
> + * Copyright (c) 2021 Xilinx Inc.
> + * Written by Vikram Garhwal <fnu.vikram@xilinx.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +#ifndef __XEN_DT_SYSCTL_H__
> +#define __XEN_DT_SYSCTL_H__
> +
> +#include <xen/list.h>
> +#include <xen/libfdt/libfdt.h>
> +#include <xen/device_tree.h>
> +#include <public/sysctl.h>
In case you decide to pass struct xen_sysctl_dt_overlay to dt_sysctl, you can remove
#include <public/sysctl.h> and use a forward declaration to struct xen_sysctl_dt_overlay
instead.
> +
> +/*
> + * overlay_node_track describes information about added nodes through dtbo.
> + * @entry: List pointer.
> + * @dt_host_new: Pointer to the updated dt_host_new unflattened 'updated fdt'.
> + * @fdt: Stores the fdt.
> + * @nodes_fullname: Stores the full name of nodes.
> + * @nodes_irq: Stores the IRQ added from overlay dtb.
> + * @node_num_irq: Stores num of IRQ for each node in overlay dtb.
> + * @num_nodes: Stores total number of nodes in overlay dtb.
> + */
> +struct overlay_track {
> + struct list_head entry;
> + struct dt_device_node *dt_host_new;
> + void *fdt;
> + char **nodes_fullname;
> + int **nodes_irq;
> + int *node_num_irq;
> + unsigned int num_nodes;
> +};
> +
> +long dt_sysctl(struct xen_sysctl *op);
> +#endif
>
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 10/14] xen/arm: Implement device tree node removal functionalities
2022-03-08 19:47 ` [XEN][RFC PATCH v3 10/14] xen/arm: Implement device tree node removal functionalities Vikram Garhwal
2022-03-15 10:10 ` Luca Fancellu
@ 2022-05-18 18:31 ` Julien Grall
2022-12-07 1:37 ` Vikram Garhwal
2022-05-19 8:13 ` Julien Grall
2 siblings, 1 reply; 42+ messages in thread
From: Julien Grall @ 2022-05-18 18:31 UTC (permalink / raw)
To: Vikram Garhwal, xen-devel
Cc: sstabellini, bertrand.marquis, volodymyr_babchuk, Andrew Cooper,
George Dunlap, Jan Beulich, Wei Liu
Hi Vikram,
On 08/03/2022 19:47, Vikram Garhwal wrote:
> Introduce sysctl XEN_SYSCTL_dt_overlay to remove device-tree nodes added using
> device tree overlay.
>
> xl overlay remove file.dtbo:
> Removes all the nodes in a given dtbo.
> First, removes IRQ permissions and MMIO accesses. Next, it finds the nodes
> in dt_host and delete the device node entries from dt_host.
>
> The nodes get removed only if it is not used by any of dom0 or domio.
>
> Also, added overlay_track struct to keep the track of added node through device
> tree overlay. overlay_track has dt_host_new which is unflattened form of updated
> fdt and name of overlay nodes. When a node is removed, we also free the memory
> used by overlay_track for the particular overlay node.
>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
> xen/common/Makefile | 1 +
> xen/common/dt_overlay.c | 447 +++++++++++++++++++++++++++++++++++
> xen/common/sysctl.c | 10 +
> xen/include/public/sysctl.h | 18 ++
> xen/include/xen/dt_overlay.h | 47 ++++
> 5 files changed, 523 insertions(+)
> create mode 100644 xen/common/dt_overlay.c
> create mode 100644 xen/include/xen/dt_overlay.h
>
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index dc8d3a13f5..2eb5734f8e 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -54,6 +54,7 @@ obj-y += wait.o
> obj-bin-y += warning.init.o
> obj-$(CONFIG_XENOPROF) += xenoprof.o
> obj-y += xmalloc_tlsf.o
> +obj-$(CONFIG_OVERLAY_DTB) += dt_overlay.o
>
> obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma lzo unlzo unlz4 unzstd earlycpio,$(n).init.o)
>
> diff --git a/xen/common/dt_overlay.c b/xen/common/dt_overlay.c
> new file mode 100644
> index 0000000000..fcb31de495
> --- /dev/null
> +++ b/xen/common/dt_overlay.c
> @@ -0,0 +1,447 @@
> +/*
> + * xen/common/dt_overlay.c
> + *
> + * Device tree overlay support in Xen.
> + *
> + * Copyright (c) 2021 Xilinx Inc.
> + * Written by Vikram Garhwal <fnu.vikram@xilinx.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +#include <xen/iocap.h>
> +#include <xen/xmalloc.h>
> +#include <asm/domain_build.h>
> +#include <xen/dt_overlay.h>
> +#include <xen/guest_access.h>
> +
> +static LIST_HEAD(overlay_tracker);
> +static DEFINE_SPINLOCK(overlay_lock);
> +
> +static int dt_overlay_remove_node(struct dt_device_node *device_node)
> +{
> + struct dt_device_node *np;
> + struct dt_device_node *parent_node;
> + struct dt_device_node *current_node;
> +
> + parent_node = device_node->parent;
> +
> + current_node = parent_node;
> +
> + if ( parent_node == NULL )
> + {
> + dt_dprintk("%s's parent node not found\n", device_node->name);
> + return -EFAULT;
> + }
> +
> + np = parent_node->child;
> +
> + if ( np == NULL )
> + {
> + dt_dprintk("parent node %s's not found\n", parent_node->name);
> + return -EFAULT;
> + }
> +
> + /* If node to be removed is only child node or first child. */
> + if ( !dt_node_cmp(np->full_name, device_node->full_name) )
> + {
> + current_node->allnext = np->allnext;
While reviewing the previous patches, I realized that we have nothing to
prevent someone to browse the device-tree while it is modified.
I am not sure this can be solved with just refcounting (like Linux
does). So maybe we need a read-write-lock. I am open to other
suggestions here.
> +
> + /* If node is first child but not the only child. */
> + if ( np->sibling != NULL )
> + current_node->child = np->sibling;
> + else
> + /* If node is only child. */
> + current_node->child = NULL;
Those 4 lines can be replaced with one line:
current_node->child = np->sibling;
> + return 0;
> + }
> +
> + for ( np = parent_node->child; np->sibling != NULL; np = np->sibling )
> + {
> + current_node = np;
> + if ( !dt_node_cmp(np->sibling->full_name, device_node->full_name) )
> + {
> + /* Found the node. Now we remove it. */
> + current_node->allnext = np->allnext->allnext;
I find this code quite confusing to read. AFAICT, 'np' and
'current_node' are exactly the same here. Why do you use different name
to access it?
> +
> + if ( np->sibling->sibling )
> + current_node->sibling = np->sibling->sibling;
> + else
> + current_node->sibling = NULL;
Same here. This could be replaced with:
current_node->child = nb->sibling->sibling;
> +
> + break;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/* Basic sanity check for the dtbo tool stack provided to Xen. */
> +static int check_overlay_fdt(const void *overlay_fdt, uint32_t overlay_fdt_size)
> +{
> + if ( (fdt_totalsize(overlay_fdt) != overlay_fdt_size) ||
> + fdt_check_header(overlay_fdt) )
> + {
> + printk(XENLOG_ERR "The overlay FDT is not a valid Flat Device Tree\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static unsigned int overlay_node_count(void *fdto)
> +{
> + unsigned int num_overlay_nodes = 0;
> + int fragment;
> +
> + fdt_for_each_subnode(fragment, fdto, 0)
> + {
> +
> + int subnode;
> + int overlay;
> +
> + overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> +
> + /*
> + * Overlay value can be < 0. But fdt_for_each_subnode() loop checks for
> + * overlay >= 0. So, no need for a overlay>=0 check here.
> + */
> +
> + fdt_for_each_subnode(subnode, fdto, overlay)
> + {
> + num_overlay_nodes++;
> + }
> + }
> +
> + return num_overlay_nodes;
> +}
> +
> +/*
> + * overlay_get_nodes_info will get the all node's full name with path. This is
> + * useful when checking node for duplication i.e. dtbo tries to add nodes which
> + * already exists in device tree.
> + */
AFAIU the code below will only retrieve one level of nodes. So if you have
foo {
bar {
}
}
Only foo will be part of the nodes_full_path. Is it correct?
> +static int overlay_get_nodes_info(const void *fdto, char ***nodes_full_path,
> + unsigned int num_overlay_nodes)
> +{
> + int fragment;
> + unsigned int node_num = 0;
> +
> + *nodes_full_path = xmalloc_bytes(num_overlay_nodes * sizeof(char *));
> +
> + if ( *nodes_full_path == NULL )
> + return -ENOMEM;
> + memset(*nodes_full_path, 0x0, num_overlay_nodes * sizeof(char *));
> +
> + fdt_for_each_subnode(fragment, fdto, 0)
> + {
> + int target;
> + int overlay;
> + int subnode;
> + const char *target_path;
> +
> + target = fdt_overlay_target_offset(device_tree_flattened, fdto,
> + fragment, &target_path);
> + if ( target < 0 )
> + return target;
> +
> + overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> +
> + /*
> + * Overlay value can be < 0. But fdt_for_each_subnode() loop checks for
> + * overlay >= 0. So, no need for a overlay>=0 check here.
> + */
> + fdt_for_each_subnode(subnode, fdto, overlay)
> + {
> + const char *node_name = NULL;
> + unsigned int node_name_len = 0;
> + unsigned int target_path_len = strlen(target_path);
> + unsigned int node_full_name_len = 0;
> +
> + node_name = fdt_get_name(fdto, subnode, &node_name_len);
> +
> + if ( node_name == NULL )
> + return -EINVAL;
> +
> + /*
> + * Magic number 2 is for adding '/'. This is done to keep the
> + * node_full_name in the correct full node name format.
> + */
> + node_full_name_len = target_path_len + node_name_len + 2;
> +
> + (*nodes_full_path)[node_num] = xmalloc_bytes(node_full_name_len);
> +
> + if ( (*nodes_full_path)[node_num] == NULL )
> + return -ENOMEM;
> +
> + memcpy((*nodes_full_path)[node_num], target_path, target_path_len);
> +
> + (*nodes_full_path)[node_num][target_path_len] = '/';
> +
> + memcpy((*nodes_full_path)[node_num] + target_path_len + 1, node_name,
> + node_name_len);
> +
> + (*nodes_full_path)[node_num][node_full_name_len - 1] = '\0';
> +
> + node_num++;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/* Remove nodes from dt_host. */
> +static int remove_nodes(char **full_dt_node_path, int **nodes_irq,
> + int *node_num_irq, unsigned int num_nodes)
Most of the information above are stored in overlay_track. So can we
pass a pointer to the overlay_track?
Also, I think most of the parameter (include overlay track) should not
be modified here. So please use const.
> +{
> + struct domain *d = hardware_domain;
> + int rc = 0;
> + struct dt_device_node *overlay_node;
> + unsigned int naddr;
> + unsigned int i, j, nirq;
> + u64 addr, size;
> + domid_t domid = 0;
> +
> + for ( j = 0; j < num_nodes; j++ )
> + {
> + dt_dprintk("Finding node %s in the dt_host\n", full_dt_node_path[j]);
> +
> + overlay_node = dt_find_node_by_path(full_dt_node_path[j]);
> +
> + if ( overlay_node == NULL )
This error (and some below) may happen because we partially removed the
DTBO but stopped because on error. So on the next run, it is possible
that "overlay_node" will be NULL and therefore you will not be able to
remove the node.
In your use-case, are you planning to ask the admin to reboot if you
can't remove a node?
> + {
> + printk(XENLOG_ERR "Device %s is not present in the tree. Removing nodes failed\n",
> + full_dt_node_path[j]);
> + return -EINVAL;
> + }
> +
> + domid = dt_device_used_by(overlay_node);
> +
> + dt_dprintk("Checking if node %s is used by any domain\n",
> + full_dt_node_path[j]);
> +
> + /* Remove the node iff it's assigned to domain 0 or domain io. */
> + if ( domid != 0 && domid != DOMID_IO )
I think I asked before, but I have seen no answer on that. What will
prevent the device to not be assigned after this check?
Also, in general, I think it would be helpful if you answer on the ML
questions. This would at least confirm that you have seen them and we
agree on what to do.
> + {
> + printk(XENLOG_ERR "Device %s as it is being used by domain %d. Removing nodes failed\n",
> + full_dt_node_path[j], domid);
> + return -EINVAL;
> + }
> +
> + dt_dprintk("Removing node: %s\n", full_dt_node_path[j]);
> +
> + nirq = node_num_irq[j];
> +
> + /* Remove IRQ permission */
> + for ( i = 0; i < nirq; i++ )
> + {
> + rc = nodes_irq[j][i];
> + /*
> + * TODO: We don't handle shared IRQs for now. So, it is assumed that
> + * the IRQs was not shared with another domain.
> + */
This is not what I meant in v2. Interrupts cannot be shared between
domain on Arm. However, interrupts can be shared between devices.
This is the latter part that needs a TODO.
In addition to that, as I wrote, an IRQ can be assigned to a *single*
domain without the device been assigned to that domain. So I think this
needs to be checked possibly by using the information stored in "desc"
to know where the IRQ was routed to.
> + rc = irq_deny_access(d, rc);
> + if ( rc )
> + {
> + printk(XENLOG_ERR "unable to revoke access for irq %u for %s\n",
> + i, dt_node_full_name(overlay_node));
> + return rc;
> + }
> + }
> +
> + rc = iommu_remove_dt_device(overlay_node);
> + if ( rc != 0 && rc != -ENXIO )
> + return rc;
> +
> + naddr = dt_number_of_address(overlay_node);
> +
> + /* Remove mmio access. */
> + for ( i = 0; i < naddr; i++ )
> + {
> + rc = dt_device_get_address(overlay_node, i, &addr, &size);
> + if ( rc )
> + {
> + printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
> + i, dt_node_full_name(overlay_node));
> + return rc;
> + }
> +
> + rc = iomem_deny_access(d, paddr_to_pfn(addr),
> + paddr_to_pfn(PAGE_ALIGN(addr + size - 1)));
I think you missed my comment here. Similar to the IRQs, you are asking
for trouble to parse the device-tree again. It would be better to store
the information using a rangeset (see include/xen/rangeset.h).
I also think the double array for the IRQs should be converted to a
rangeset as this would simplify the code.
Furthemore, you are removing the permission but not the mapping in the
P2M. Can you clarify why?
> + if ( rc )
> + {
> + printk(XENLOG_ERR "Unable to remove dom%d access to"
> + " 0x%"PRIx64" - 0x%"PRIx64"\n",
> + d->domain_id,
> + addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
> + return rc;
> + }
> + }
> +
> + rc = dt_overlay_remove_node(overlay_node);
> + if ( rc )
> + return rc;
> + }
> +
> + return rc;
> +}
[...]
> + * overlay_node_track describes information about added nodes through dtbo.
> + * @entry: List pointer.
> + * @dt_host_new: Pointer to the updated dt_host_new unflattened 'updated fdt'.
> + * @fdt: Stores the fdt.
> + * @nodes_fullname: Stores the full name of nodes.
> + * @nodes_irq: Stores the IRQ added from overlay dtb.
> + * @node_num_irq: Stores num of IRQ for each node in overlay dtb.
> + * @num_nodes: Stores total number of nodes in overlay dtb.
> + */
> +struct overlay_track {
> + struct list_head entry;
> + struct dt_device_node *dt_host_new;
> + void *fdt;
> + char **nodes_fullname;
Looking at the code, the main use for the fullname are to check the FDT
match and looking up in the DT.
In order to check the DT, you could use memcmp() to confirm both the
stored FDT and the one provided by the user match.
For the lookup, you could avoid it by storing a pointer to the root of
the new subtrees.
Please let me know if you disagree with this approach.
> + int **nodes_irq;
> + int *node_num_irq;
> + unsigned int num_nodes;
> +};
> +
> +long dt_sysctl(struct xen_sysctl *op);
> +#endif
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 10/14] xen/arm: Implement device tree node removal functionalities
2022-05-18 18:31 ` Julien Grall
@ 2022-12-07 1:37 ` Vikram Garhwal
2022-12-07 16:50 ` Julien Grall
0 siblings, 1 reply; 42+ messages in thread
From: Vikram Garhwal @ 2022-12-07 1:37 UTC (permalink / raw)
To: Julien Grall, Vikram Garhwal, xen-devel
Cc: sstabellini, bertrand.marquis, volodymyr_babchuk, Andrew Cooper,
George Dunlap, Jan Beulich, Wei Liu
Hi Julien & Luca,
Sorry for so long delay on the this patch series. I was stuck with
internal work and then had to go on a long leave.
I prepared v4 and will send it shortly. Addressed all the feedback. I
have few comments. Please see them inline below.
On 5/18/22 11:31 AM, Julien Grall wrote:
> CAUTION: This message has originated from an External Source. Please
> use proper judgment and caution when opening attachments, clicking
> links, or responding to this email.
>
>
> Hi Vikram,
>
> On 08/03/2022 19:47, Vikram Garhwal wrote:
>> Introduce sysctl XEN_SYSCTL_dt_overlay to remove device-tree nodes
>> added using
>> device tree overlay.
>>
>> xl overlay remove file.dtbo:
>> Removes all the nodes in a given dtbo.
>> First, removes IRQ permissions and MMIO accesses. Next, it finds
>> the nodes
>> in dt_host and delete the device node entries from dt_host.
>>
>> The nodes get removed only if it is not used by any of dom0 or
>> domio.
>>
>> Also, added overlay_track struct to keep the track of added node
>> through device
>> tree overlay. overlay_track has dt_host_new which is unflattened form
>> of updated
>> fdt and name of overlay nodes. When a node is removed, we also free
>> the memory
>> used by overlay_track for the particular overlay node.
>>
>> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
>> ---
>> xen/common/Makefile | 1 +
>> xen/common/dt_overlay.c | 447 +++++++++++++++++++++++++++++++++++
>> xen/common/sysctl.c | 10 +
>> xen/include/public/sysctl.h | 18 ++
>> xen/include/xen/dt_overlay.h | 47 ++++
>> 5 files changed, 523 insertions(+)
>> create mode 100644 xen/common/dt_overlay.c
>> create mode 100644 xen/include/xen/dt_overlay.h
>>
>> diff --git a/xen/common/Makefile b/xen/common/Makefile
>> index dc8d3a13f5..2eb5734f8e 100644
>> --- a/xen/common/Makefile
>> +++ b/xen/common/Makefile
>> @@ -54,6 +54,7 @@ obj-y += wait.o
>> obj-bin-y += warning.init.o
>> obj-$(CONFIG_XENOPROF) += xenoprof.o
>> obj-y += xmalloc_tlsf.o
>> +obj-$(CONFIG_OVERLAY_DTB) += dt_overlay.o
>>
>> obj-bin-$(CONFIG_X86) += $(foreach n,decompress bunzip2 unxz unlzma
>> lzo unlzo unlz4 unzstd earlycpio,$(n).init.o)
>>
>> diff --git a/xen/common/dt_overlay.c b/xen/common/dt_overlay.c
>> new file mode 100644
>> index 0000000000..fcb31de495
>> --- /dev/null
>> +++ b/xen/common/dt_overlay.c
>> @@ -0,0 +1,447 @@
>> +/*
>> + * xen/common/dt_overlay.c
>> + *
>> + * Device tree overlay support in Xen.
>> + *
>> + * Copyright (c) 2021 Xilinx Inc.
>> + * Written by Vikram Garhwal <fnu.vikram@xilinx.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms and conditions of the GNU General Public
>> + * License, version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +#include <xen/iocap.h>
>> +#include <xen/xmalloc.h>
>> +#include <asm/domain_build.h>
>> +#include <xen/dt_overlay.h>
>> +#include <xen/guest_access.h>
>> +
>> +static LIST_HEAD(overlay_tracker);
>> +static DEFINE_SPINLOCK(overlay_lock);
>> +
>> +static int dt_overlay_remove_node(struct dt_device_node *device_node)
>> +{
>> + struct dt_device_node *np;
>> + struct dt_device_node *parent_node;
>> + struct dt_device_node *current_node;
>> +
>> + parent_node = device_node->parent;
>> +
>> + current_node = parent_node;
>> +
>> + if ( parent_node == NULL )
>> + {
>> + dt_dprintk("%s's parent node not found\n", device_node->name);
>> + return -EFAULT;
>> + }
>> +
>> + np = parent_node->child;
>> +
>> + if ( np == NULL )
>> + {
>> + dt_dprintk("parent node %s's not found\n", parent_node->name);
>> + return -EFAULT;
>> + }
>> +
>> + /* If node to be removed is only child node or first child. */
>> + if ( !dt_node_cmp(np->full_name, device_node->full_name) )
>> + {
>> + current_node->allnext = np->allnext;
>
> While reviewing the previous patches, I realized that we have nothing to
> prevent someone to browse the device-tree while it is modified.
>
> I am not sure this can be solved with just refcounting (like Linux
> does). So maybe we need a read-write-lock. I am open to other
> suggestions here.
Added read_locks for all dt_host access funtions.
>> +
>> + /* If node is first child but not the only child. */
>> + if ( np->sibling != NULL )
>> + current_node->child = np->sibling;
>> + else
>> + /* If node is only child. */
>> + current_node->child = NULL;
>
> Those 4 lines can be replaced with one line:
>
> current_node->child = np->sibling;
>
>> + return 0;
>> + }
>> +
>> + for ( np = parent_node->child; np->sibling != NULL; np =
>> np->sibling )
>> + {
>> + current_node = np;
>> + if ( !dt_node_cmp(np->sibling->full_name,
>> device_node->full_name) )
>> + {
>> + /* Found the node. Now we remove it. */
>> + current_node->allnext = np->allnext->allnext;
>
> I find this code quite confusing to read. AFAICT, 'np' and
> 'current_node' are exactly the same here. Why do you use different name
> to access it?
>
>> +
>> + if ( np->sibling->sibling )
>> + current_node->sibling = np->sibling->sibling;
>> + else
>> + current_node->sibling = NULL;
>
> Same here. This could be replaced with:
>
> current_node->child = nb->sibling->sibling;
>
>> +
>> + break;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/* Basic sanity check for the dtbo tool stack provided to Xen. */
>> +static int check_overlay_fdt(const void *overlay_fdt, uint32_t
>> overlay_fdt_size)
>> +{
>> + if ( (fdt_totalsize(overlay_fdt) != overlay_fdt_size) ||
>> + fdt_check_header(overlay_fdt) )
>> + {
>> + printk(XENLOG_ERR "The overlay FDT is not a valid Flat
>> Device Tree\n");
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static unsigned int overlay_node_count(void *fdto)
>> +{
>> + unsigned int num_overlay_nodes = 0;
>> + int fragment;
>> +
>> + fdt_for_each_subnode(fragment, fdto, 0)
>> + {
>> +
>> + int subnode;
>> + int overlay;
>> +
>> + overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
>> +
>> + /*
>> + * Overlay value can be < 0. But fdt_for_each_subnode() loop
>> checks for
>> + * overlay >= 0. So, no need for a overlay>=0 check here.
>> + */
>> +
>> + fdt_for_each_subnode(subnode, fdto, overlay)
>> + {
>> + num_overlay_nodes++;
>> + }
>> + }
>> +
>> + return num_overlay_nodes;
>> +}
>> +
>> +/*
>> + * overlay_get_nodes_info will get the all node's full name with
>> path. This is
>> + * useful when checking node for duplication i.e. dtbo tries to add
>> nodes which
>> + * already exists in device tree.
>> + */
>
> AFAIU the code below will only retrieve one level of nodes. So if you
> have
>
> foo {
> bar {
> }
> }
>
> Only foo will be part of the nodes_full_path. Is it correct?
Added support for adding children nodes too.
>
>
>> +static int overlay_get_nodes_info(const void *fdto, char
>> ***nodes_full_path,
>> + unsigned int num_overlay_nodes)
>> +{
>> + int fragment;
>> + unsigned int node_num = 0;
>> +
>> + *nodes_full_path = xmalloc_bytes(num_overlay_nodes * sizeof(char
>> *));
>> +
>> + if ( *nodes_full_path == NULL )
>> + return -ENOMEM;
>> + memset(*nodes_full_path, 0x0, num_overlay_nodes * sizeof(char *));
>> +
>> + fdt_for_each_subnode(fragment, fdto, 0)
>> + {
>> + int target;
>> + int overlay;
>> + int subnode;
>> + const char *target_path;
>> +
>> + target = fdt_overlay_target_offset(device_tree_flattened, fdto,
>> + fragment, &target_path);
>> + if ( target < 0 )
>> + return target;
>> +
>> + overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
>> +
>> + /*
>> + * Overlay value can be < 0. But fdt_for_each_subnode() loop
>> checks for
>> + * overlay >= 0. So, no need for a overlay>=0 check here.
>> + */
>> + fdt_for_each_subnode(subnode, fdto, overlay)
>> + {
>> + const char *node_name = NULL;
>> + unsigned int node_name_len = 0;
>> + unsigned int target_path_len = strlen(target_path);
>> + unsigned int node_full_name_len = 0;
>> +
>> + node_name = fdt_get_name(fdto, subnode, &node_name_len);
>> +
>> + if ( node_name == NULL )
>> + return -EINVAL;
>> +
>> + /*
>> + * Magic number 2 is for adding '/'. This is done to
>> keep the
>> + * node_full_name in the correct full node name format.
>> + */
>> + node_full_name_len = target_path_len + node_name_len + 2;
>> +
>> + (*nodes_full_path)[node_num] =
>> xmalloc_bytes(node_full_name_len);
>> +
>> + if ( (*nodes_full_path)[node_num] == NULL )
>> + return -ENOMEM;
>> +
>> + memcpy((*nodes_full_path)[node_num], target_path,
>> target_path_len);
>> +
>> + (*nodes_full_path)[node_num][target_path_len] = '/';
>> +
>> + memcpy((*nodes_full_path)[node_num] + target_path_len +
>> 1, node_name,
>> + node_name_len);
>> +
>> + (*nodes_full_path)[node_num][node_full_name_len - 1] =
>> '\0';
>> +
>> + node_num++;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/* Remove nodes from dt_host. */
>> +static int remove_nodes(char **full_dt_node_path, int **nodes_irq,
>> + int *node_num_irq, unsigned int num_nodes)
>
> Most of the information above are stored in overlay_track. So can we
> pass a pointer to the overlay_track?
>
> Also, I think most of the parameter (include overlay track) should not
> be modified here. So please use const.
>
>> +{
>> + struct domain *d = hardware_domain;
>> + int rc = 0;
>> + struct dt_device_node *overlay_node;
>> + unsigned int naddr;
>> + unsigned int i, j, nirq;
>> + u64 addr, size;
>> + domid_t domid = 0;
>> +
>> + for ( j = 0; j < num_nodes; j++ )
>> + {
>> + dt_dprintk("Finding node %s in the dt_host\n",
>> full_dt_node_path[j]);
>> +
>> + overlay_node = dt_find_node_by_path(full_dt_node_path[j]);
>> +
>> + if ( overlay_node == NULL )
>
> This error (and some below) may happen because we partially removed the
> DTBO but stopped because on error. So on the next run, it is possible
> that "overlay_node" will be NULL and therefore you will not be able to
> remove the node.
>
> In your use-case, are you planning to ask the admin to reboot if you
> can't remove a node?
Yeah. What is error case where it may happen?
We are checking if dtbo is same as it was added and also expect user to
remove the nodes only iff the nodes aren't
used by any domain.
>
>> + {
>> + printk(XENLOG_ERR "Device %s is not present in the tree.
>> Removing nodes failed\n",
>> + full_dt_node_path[j]);
>> + return -EINVAL;
>> + }
>> +
>> + domid = dt_device_used_by(overlay_node);
>> +
>> + dt_dprintk("Checking if node %s is used by any domain\n",
>> + full_dt_node_path[j]);
>> +
>> + /* Remove the node iff it's assigned to domain 0 or domain
>> io. */
>> + if ( domid != 0 && domid != DOMID_IO )
>
> I think I asked before, but I have seen no answer on that. What will
> prevent the device to not be assigned after this check?
So, here for removal we assume that user removed all on-going ops on the
dtbo nodes which they wants to remove.
>
>
> Also, in general, I think it would be helpful if you answer on the ML
> questions. This would at least confirm that you have seen them and we
> agree on what to do.
Will keep this in mind and reply.
>
>> + {
>> + printk(XENLOG_ERR "Device %s as it is being used by
>> domain %d. Removing nodes failed\n",
>> + full_dt_node_path[j], domid);
>> + return -EINVAL;
>> + }
>> +
>> + dt_dprintk("Removing node: %s\n", full_dt_node_path[j]);
>> +
>> + nirq = node_num_irq[j];
>> +
>> + /* Remove IRQ permission */
>> + for ( i = 0; i < nirq; i++ )
>> + {
>> + rc = nodes_irq[j][i];
>> + /*
>> + * TODO: We don't handle shared IRQs for now. So, it is
>> assumed that
>> + * the IRQs was not shared with another domain.
>> + */
>
> This is not what I meant in v2. Interrupts cannot be shared between
> domain on Arm. However, interrupts can be shared between devices.
>
> This is the latter part that needs a TODO.
>
> In addition to that, as I wrote, an IRQ can be assigned to a *single*
> domain without the device been assigned to that domain. So I think this
> needs to be checked possibly by using the information stored in "desc"
> to know where the IRQ was routed to.
>
>> + rc = irq_deny_access(d, rc);
>> + if ( rc )
>> + {
>> + printk(XENLOG_ERR "unable to revoke access for irq
>> %u for %s\n",
>> + i, dt_node_full_name(overlay_node));
>> + return rc;
>> + }
>> + }
>> +
>> + rc = iommu_remove_dt_device(overlay_node);
>> + if ( rc != 0 && rc != -ENXIO )
>> + return rc;
>> +
>> + naddr = dt_number_of_address(overlay_node);
>> +
>> + /* Remove mmio access. */
>> + for ( i = 0; i < naddr; i++ )
>> + {
>> + rc = dt_device_get_address(overlay_node, i, &addr, &size);
>> + if ( rc )
>> + {
>> + printk(XENLOG_ERR "Unable to retrieve address %u for
>> %s\n",
>> + i, dt_node_full_name(overlay_node));
>> + return rc;
>> + }
>> +
>> + rc = iomem_deny_access(d, paddr_to_pfn(addr),
>> + paddr_to_pfn(PAGE_ALIGN(addr +
>> size - 1)));
>
> I think you missed my comment here. Similar to the IRQs, you are asking
> for trouble to parse the device-tree again. It would be better to store
> the information using a rangeset (see include/xen/rangeset.h).
>
> I also think the double array for the IRQs should be converted to a
> rangeset as this would simplify the code.
>
Keeping rangeset will work if we only parse one-level nodes. But if
there are descendant nodes, then its looking complicated to get info
using rangeset. While adding, we have to add parent first and then it's
descendant. But while remove, we will need to remove descendants first
and the parent node lastly.
I am unable to comeup with easy way to use rangeset to store information
in this way for add and remove.
> Furthemore, you are removing the permission but not the mapping in the
> P2M. Can you clarify why?
We are not actually mapping the nodes here.
>
>
>> + if ( rc )
>> + {
>> + printk(XENLOG_ERR "Unable to remove dom%d access to"
>> + " 0x%"PRIx64" - 0x%"PRIx64"\n",
>> + d->domain_id,
>> + addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
>> + return rc;
>> + }
>> + }
>> +
>> + rc = dt_overlay_remove_node(overlay_node);
>> + if ( rc )
>> + return rc;
>> + }
>> +
>> + return rc;
>> +}
>
> [...]
>
>> + * overlay_node_track describes information about added nodes
>> through dtbo.
>> + * @entry: List pointer.
>> + * @dt_host_new: Pointer to the updated dt_host_new unflattened
>> 'updated fdt'.
>> + * @fdt: Stores the fdt.
>> + * @nodes_fullname: Stores the full name of nodes.
>> + * @nodes_irq: Stores the IRQ added from overlay dtb.
>> + * @node_num_irq: Stores num of IRQ for each node in overlay dtb.
>> + * @num_nodes: Stores total number of nodes in overlay dtb.
>> + */
>> +struct overlay_track {
>> + struct list_head entry;
>> + struct dt_device_node *dt_host_new;
>> + void *fdt;
>> + char **nodes_fullname;
>
> Looking at the code, the main use for the fullname are to check the FDT
> match and looking up in the DT.
>
> In order to check the DT, you could use memcmp() to confirm both the
> stored FDT and the one provided by the user match.
>
> For the lookup, you could avoid it by storing a pointer to the root of
> the new subtrees.
>
> Please let me know if you disagree with this approach.
>
If I understood correctly: just keeping the root of new overlay subtree
will not work for all case. It will work only if the nodes added are
adjacent to each other i.e. have the same parent then it will work as we
add all overlay nodes as the last child of their parent. But If two
subnodes have different parents, they may or may not be
nearby(node->allnext won't work) then we will issues removing the node
from this approach.
I did following small modification to your suggestion:
Keep FDT( do memcmp) for match and also keep address for all added nodes
at one-level( we can find children info if we know the top one-level
nodes. Please check overlay_node_count()). This will take 8bytes * num
of nodes in one-level space which is lot less space than keeping
nodes_fullname.
I will send v4 this week.
>> + int **nodes_irq;
>> + int *node_num_irq;
>> + unsigned int num_nodes;
>> +};
>> +
>> +long dt_sysctl(struct xen_sysctl *op);
>> +#endif
>
> Cheers,
>
> --
> Julien Grall
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 10/14] xen/arm: Implement device tree node removal functionalities
2022-12-07 1:37 ` Vikram Garhwal
@ 2022-12-07 16:50 ` Julien Grall
0 siblings, 0 replies; 42+ messages in thread
From: Julien Grall @ 2022-12-07 16:50 UTC (permalink / raw)
To: Vikram Garhwal, Vikram Garhwal, xen-devel
Cc: sstabellini, bertrand.marquis, volodymyr_babchuk, Andrew Cooper,
George Dunlap, Jan Beulich, Wei Liu
On 07/12/2022 01:37, Vikram Garhwal wrote:
>> In your use-case, are you planning to ask the admin to reboot if you
>> can't remove a node?
> Yeah. What is error case where it may happen?
The code below have many possible failures. I would suggest to test by
throwing an error on the second node and check what happen if you try to
remove again.
>
> We are checking if dtbo is same as it was added and also expect user to
> remove the nodes only iff the nodes aren't
>
> used by any domain.
>
>>
>>> + {
>>> + printk(XENLOG_ERR "Device %s is not present in the tree.
>>> Removing nodes failed\n",
>>> + full_dt_node_path[j]);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + domid = dt_device_used_by(overlay_node);
>>> +
>>> + dt_dprintk("Checking if node %s is used by any domain\n",
>>> + full_dt_node_path[j]);
>>> +
>>> + /* Remove the node iff it's assigned to domain 0 or domain
>>> io. */
>>> + if ( domid != 0 && domid != DOMID_IO )
>>
>> I think I asked before, but I have seen no answer on that. What will
>> prevent the device to not be assigned after this check?
>
> So, here for removal we assume that user removed all on-going ops on the
> dtbo nodes which they wants to remove.
Please don't make any assumption on what the user is doing even if it is
dom0. So the hypervisor code should be hardened.
>>> + {
>>> + printk(XENLOG_ERR "Device %s as it is being used by
>>> domain %d. Removing nodes failed\n",
>>> + full_dt_node_path[j], domid);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + dt_dprintk("Removing node: %s\n", full_dt_node_path[j]);
>>> +
>>> + nirq = node_num_irq[j];
>>> +
>>> + /* Remove IRQ permission */
>>> + for ( i = 0; i < nirq; i++ )
>>> + {
>>> + rc = nodes_irq[j][i];
>>> + /*
>>> + * TODO: We don't handle shared IRQs for now. So, it is
>>> assumed that
>>> + * the IRQs was not shared with another domain.
>>> + */
>>
>> This is not what I meant in v2. Interrupts cannot be shared between
>> domain on Arm. However, interrupts can be shared between devices.
>>
>> This is the latter part that needs a TODO.
>>
>> In addition to that, as I wrote, an IRQ can be assigned to a *single*
>> domain without the device been assigned to that domain. So I think this
>> needs to be checked possibly by using the information stored in "desc"
>> to know where the IRQ was routed to.
>>
>>> + rc = irq_deny_access(d, rc);
>>> + if ( rc )
>>> + {
>>> + printk(XENLOG_ERR "unable to revoke access for irq
>>> %u for %s\n",
>>> + i, dt_node_full_name(overlay_node));
>>> + return rc;
>>> + }
>>> + }
>>> +
>>> + rc = iommu_remove_dt_device(overlay_node);
>>> + if ( rc != 0 && rc != -ENXIO )
>>> + return rc;
>>> +
>>> + naddr = dt_number_of_address(overlay_node);
>>> +
>>> + /* Remove mmio access. */
>>> + for ( i = 0; i < naddr; i++ )
>>> + {
>>> + rc = dt_device_get_address(overlay_node, i, &addr, &size);
>>> + if ( rc )
>>> + {
>>> + printk(XENLOG_ERR "Unable to retrieve address %u for
>>> %s\n",
>>> + i, dt_node_full_name(overlay_node));
>>> + return rc;
>>> + }
>>> +
>>> + rc = iomem_deny_access(d, paddr_to_pfn(addr),
>>> + paddr_to_pfn(PAGE_ALIGN(addr +
>>> size - 1)));
>>
>> I think you missed my comment here. Similar to the IRQs, you are asking
>> for trouble to parse the device-tree again. It would be better to store
>> the information using a rangeset (see include/xen/rangeset.h).
>>
>> I also think the double array for the IRQs should be converted to a
>> rangeset as this would simplify the code.
>>
> Keeping rangeset will work if we only parse one-level nodes. But if
> there are descendant nodes, then its looking complicated to get info
> using rangeset. While adding, we have to add parent first and then it's
> descendant. But while remove, we will need to remove descendants first
> and the parent node lastly.
I am not sure I understand the problem here. If you use a rangeset, then
you don't need to go through every node one by one to apply/revert the
permissions. You could simply apply/revert all the permission in one call.
>> Furthemore, you are removing the permission but not the mapping in the
>> P2M. Can you clarify why?
> We are not actually mapping the nodes here.
The remove function should be the inverse of the add function. AFAICT,
you are calling mapping the P2M (see call to map_range_to_domain()). So
this removal function *have* to remove the entry from the P2Ms.
If you disagree with that, then please explain who is going to remove
the P2M mappings.
±>>
>>
>>> + if ( rc )
>>> + {
>>> + printk(XENLOG_ERR "Unable to remove dom%d access to"
>>> + " 0x%"PRIx64" - 0x%"PRIx64"\n",
>>> + d->domain_id,
>>> + addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
>>> + return rc;
>>> + }
>>> + }
>>> +
>>> + rc = dt_overlay_remove_node(overlay_node);
>>> + if ( rc )
>>> + return rc;
>>> + }
>>> +
>>> + return rc;
>>> +}
>>
>> [...]
>>
>>> + * overlay_node_track describes information about added nodes
>>> through dtbo.
>>> + * @entry: List pointer.
>>> + * @dt_host_new: Pointer to the updated dt_host_new unflattened
>>> 'updated fdt'.
>>> + * @fdt: Stores the fdt.
>>> + * @nodes_fullname: Stores the full name of nodes.
>>> + * @nodes_irq: Stores the IRQ added from overlay dtb.
>>> + * @node_num_irq: Stores num of IRQ for each node in overlay dtb.
>>> + * @num_nodes: Stores total number of nodes in overlay dtb.
>>> + */
>>> +struct overlay_track {
>>> + struct list_head entry;
>>> + struct dt_device_node *dt_host_new;
>>> + void *fdt;
>>> + char **nodes_fullname;
>>
>> Looking at the code, the main use for the fullname are to check the FDT
>> match and looking up in the DT.
>>
>> In order to check the DT, you could use memcmp() to confirm both the
>> stored FDT and the one provided by the user match.
>>
>> For the lookup, you could avoid it by storing a pointer to the root of
>> the new subtrees.
>>
>> Please let me know if you disagree with this approach.
>>
> If I understood correctly: just keeping the root of new overlay subtree
> will not work for all case. It will work only if the nodes added are
> adjacent to each other i.e. have the same parent then it will work as we
> add all overlay nodes as the last child of their parent. But If two
> subnodes have different parents, they may or may not be
> nearby(node->allnext won't work) then we will issues removing the node
> from this approach.
Thanks for the explanation.
>
> I did following small modification to your suggestion:
> Keep FDT( do memcmp) for match and also keep address for all added nodes
> at one-level( we can find children info if we know the top one-level
> nodes. Please check overlay_node_count()). This will take 8bytes * num
> of nodes in one-level space which is lot less space than keeping
> nodes_fullname.
This seems to match my thoughts after your explanation above. I will
have a look at the next version.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [XEN][RFC PATCH v3 10/14] xen/arm: Implement device tree node removal functionalities
2022-03-08 19:47 ` [XEN][RFC PATCH v3 10/14] xen/arm: Implement device tree node removal functionalities Vikram Garhwal
2022-03-15 10:10 ` Luca Fancellu
2022-05-18 18:31 ` Julien Grall
@ 2022-05-19 8:13 ` Julien Grall
2 siblings, 0 replies; 42+ messages in thread
From: Julien Grall @ 2022-05-19 8:13 UTC (permalink / raw)
To: Vikram Garhwal, xen-devel
Cc: sstabellini, bertrand.marquis, volodymyr_babchuk, Andrew Cooper,
George Dunlap, Jan Beulich, Wei Liu
Hi Vikram,
A few more comments that I spotted after reviewing the next patch.
On 08/03/2022 19:47, Vikram Garhwal wrote:
> Introduce sysctl XEN_SYSCTL_dt_overlay to remove device-tree nodes added using
> device tree overlay.
>
> xl overlay remove file.dtbo:
> Removes all the nodes in a given dtbo.
> First, removes IRQ permissions and MMIO accesses. Next, it finds the nodes
> in dt_host and delete the device node entries from dt_host.
>
> The nodes get removed only if it is not used by any of dom0 or domio.
Can overlay be nested (let say B nest in A)? If yes, how do you deal
with the case the A is removed before B?
[...]
> +long dt_sysctl(struct xen_sysctl *op)
> +{
> + long ret = 0;
> + void *overlay_fdt;
> + char **nodes_full_path = NULL;
> + unsigned int num_overlay_nodes = 0;
> +
> + if ( op->u.dt_overlay.overlay_fdt_size <= 0 )
From my understanding, FDT are typically limited to 2MB. At minimum, we
should check the overlay is not bigger than that (to avoid arbirtrary
allocation size). I would possibly consider to limit to lower than that
(i.e 500KB) if there is no need to have larger and to reduce the amount
memory consumption by the overlay code.
> + return -EINVAL;
> +
> + overlay_fdt = xmalloc_bytes(op->u.dt_overlay.overlay_fdt_size);
> +
> + if ( overlay_fdt == NULL )
> + return -ENOMEM;
> +
> + ret = copy_from_guest(overlay_fdt, op->u.dt_overlay.overlay_fdt,
> + op->u.dt_overlay.overlay_fdt_size);
> + if ( ret )
> + {
> + gprintk(XENLOG_ERR, "copy from guest failed\n");
> + xfree(overlay_fdt);
You free overlay_fdt, but not in the other paths.
> +
> + return -EFAULT;
> + }
> +
> + switch ( op->u.dt_overlay.overlay_op )
> + {
> + case XEN_SYSCTL_DT_OVERLAY_REMOVE:
> + ret = check_overlay_fdt(overlay_fdt,
> + op->u.dt_overlay.overlay_fdt_size);
> + if ( ret )
> + {
> + ret = -EFAULT;
> + break;
> + }
> +
> + num_overlay_nodes = overlay_node_count(overlay_fdt);
> + if ( num_overlay_nodes == 0 )
> + {
> + ret = -ENOMEM;
> + break;
> + }
> +
> + ret = overlay_get_nodes_info(overlay_fdt, &nodes_full_path,
> + num_overlay_nodes);
> + if ( ret )
> + break;
> +
> + ret = handle_remove_overlay_nodes(nodes_full_path,
> + num_overlay_nodes);
> + break;
> +
> + default:
> + break;
> + }
> +
> + if ( nodes_full_path != NULL )
> + {
> + int i;
> + for ( i = 0; i < num_overlay_nodes && nodes_full_path[i] != NULL; i++ )
> + {
> + xfree(nodes_full_path[i]);
> + }
> + xfree(nodes_full_path);
> + }
AFAICT, nodes_full_path is not going to be used by the subop to add an
overlay. So I would consider to move this within the case or (even
better) create a function handling the subop (like you did for add) so
we don't end up with a large switch.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 42+ messages in thread
* [XEN][RFC PATCH v3 11/14] xen/arm: Implement device tree node addition functionalities
2022-03-08 19:46 [XEN][RFC PATCH v3 00/14] dynamic node programming using overlay dtbo Vikram Garhwal
` (9 preceding siblings ...)
2022-03-08 19:47 ` [XEN][RFC PATCH v3 10/14] xen/arm: Implement device tree node removal functionalities Vikram Garhwal
@ 2022-03-08 19:47 ` Vikram Garhwal
2022-03-15 10:40 ` Luca Fancellu
2022-05-18 19:03 ` Julien Grall
2022-03-08 19:47 ` [XEN][RFC PATCH v3 12/14] tools/libs/ctrl: Implement new xc interfaces for dt overlay Vikram Garhwal
` (2 subsequent siblings)
13 siblings, 2 replies; 42+ messages in thread
From: Vikram Garhwal @ 2022-03-08 19:47 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk,
Vikram Garhwal, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu
Update sysctl XEN_SYSCTL_dt_overlay to enable support for dtbo nodes addition
using device tree overlay.
xl overlay add file.dtbo:
Each time overlay nodes are added using .dtbo, a new fdt(memcpy of
device_tree_flattened) is created and updated with overlay nodes. This
updated fdt is further unflattened to a dt_host_new. Next, it checks if any
of the overlay nodes already exists in the dt_host. If overlay nodes doesn't
exist then find the overlay nodes in dt_host_new, find the overlay node's
parent in dt_host and add the nodes as child under their parent in the
dt_host. The node is attached as the last node under target parent.
Finally, add IRQs, add device to IOMMUs, set permissions and map MMIO for the
overlay node.
When a node is added using overlay, a new entry is allocated in the
overlay_track to keep the track of memory allocation due to addition of overlay
node. This is helpful for freeing the memory allocated when a device tree node
is removed.
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
xen/common/dt_overlay.c | 324 ++++++++++++++++++++++++++++++++++++
xen/include/public/sysctl.h | 1 +
2 files changed, 325 insertions(+)
diff --git a/xen/common/dt_overlay.c b/xen/common/dt_overlay.c
index fcb31de495..01aed62d74 100644
--- a/xen/common/dt_overlay.c
+++ b/xen/common/dt_overlay.c
@@ -82,6 +82,64 @@ static int dt_overlay_remove_node(struct dt_device_node *device_node)
return 0;
}
+static int dt_overlay_add_node(struct dt_device_node *device_node,
+ const char *parent_node_path)
+{
+ struct dt_device_node *parent_node;
+ struct dt_device_node *np;
+ struct dt_device_node *next_node;
+ struct dt_device_node *new_node;
+
+ parent_node = dt_find_node_by_path(parent_node_path);
+
+ new_node = device_node;
+
+ if ( new_node == NULL )
+ return -EINVAL;
+
+ if ( parent_node == NULL )
+ {
+ dt_dprintk("Node not found. Partial dtb will not be added");
+ return -EINVAL;
+ }
+
+ /*
+ * If node is found. We can attach the device_node as a child of the
+ * parent node.
+ */
+
+ /* If parent has no child. */
+ if ( parent_node->child == NULL )
+ {
+ next_node = parent_node->allnext;
+ new_node->parent = parent_node;
+ parent_node->allnext = new_node;
+ parent_node->child = new_node;
+ /* Now plug next_node at the end of device_node. */
+ new_node->allnext = next_node;
+ } else {
+ /* If parent has at least one child node. */
+
+ /*
+ * Iterate to the last child node of parent.
+ */
+ for ( np = parent_node->child; np->sibling != NULL; np = np->sibling )
+ {
+ }
+
+ next_node = np->allnext;
+ new_node->parent = parent_node;
+ np->sibling = new_node;
+ np->allnext = new_node;
+ /* Now plug next_node at the end of device_node. */
+ new_node->sibling = next_node;
+ new_node->allnext = next_node;
+ np->sibling->sibling = NULL;
+ }
+
+ return 0;
+}
+
/* Basic sanity check for the dtbo tool stack provided to Xen. */
static int check_overlay_fdt(const void *overlay_fdt, uint32_t overlay_fdt_size)
{
@@ -377,6 +435,267 @@ out:
return rc;
}
+/*
+ * Adds device tree nodes under target node.
+ * We use dt_host_new to unflatten the updated device_tree_flattened. This is
+ * done to avoid the removal of device_tree generation, iomem regions mapping to
+ * hardware domain done by handle_node().
+ */
+static long handle_add_overlay_nodes(void *overlay_fdt,
+ uint32_t overlay_fdt_size)
+{
+ int rc = 0;
+ struct dt_device_node *overlay_node;
+ char **nodes_full_path = NULL;
+ int **nodes_irq = NULL;
+ int *node_num_irq = NULL;
+ void *fdt = NULL;
+ struct dt_device_node *dt_host_new = NULL;
+ struct domain *d = hardware_domain;
+ struct overlay_track *tr = NULL;
+ unsigned int naddr;
+ unsigned int num_irq;
+ unsigned int i, j, k;
+ unsigned int num_overlay_nodes;
+ u64 addr, size;
+
+ fdt = xmalloc_bytes(fdt_totalsize(device_tree_flattened));
+ if ( fdt == NULL )
+ return -ENOMEM;
+
+ num_overlay_nodes = overlay_node_count(overlay_fdt);
+ if ( num_overlay_nodes == 0 )
+ {
+ xfree(fdt);
+ return -ENOMEM;
+ }
+
+ spin_lock(&overlay_lock);
+
+ memcpy(fdt, device_tree_flattened, fdt_totalsize(device_tree_flattened));
+
+ rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
+ if ( rc )
+ {
+ xfree(fdt);
+ return rc;
+ }
+
+ /*
+ * overlay_get_nodes_info is called to get the node information from dtbo.
+ * This is done before fdt_overlay_apply() because the overlay apply will
+ * erase the magic of overlay_fdt.
+ */
+ rc = overlay_get_nodes_info(overlay_fdt, &nodes_full_path,
+ num_overlay_nodes);
+ if ( rc )
+ {
+ printk(XENLOG_ERR "Getting nodes information failed with error %d\n",
+ rc);
+ goto err;
+ }
+
+ nodes_irq = xmalloc_bytes(num_overlay_nodes * sizeof(int *));
+
+ if ( nodes_irq == NULL )
+ {
+ rc = -ENOMEM;
+ goto err;
+ }
+ memset(nodes_irq, 0x0, num_overlay_nodes * sizeof(int *));
+
+ node_num_irq = xmalloc_bytes(num_overlay_nodes * sizeof(int));
+ if ( node_num_irq == NULL )
+ {
+ rc = -ENOMEM;
+ goto err;
+ }
+ memset(node_num_irq, 0x0, num_overlay_nodes * sizeof(int));
+
+ rc = fdt_overlay_apply(fdt, overlay_fdt);
+ if ( rc )
+ {
+ printk(XENLOG_ERR "Adding overlay node failed with error %d\n", rc);
+ goto err;
+ }
+
+ for ( j = 0; j < num_overlay_nodes; j++ )
+ {
+ /* Check if any of the node already exists in dt_host. */
+ overlay_node = dt_find_node_by_path(nodes_full_path[j]);
+ if ( overlay_node != NULL )
+ {
+ printk(XENLOG_ERR "node %s exists in device tree\n",
+ nodes_full_path[j]);
+ rc = -EINVAL;
+ goto err;
+ }
+ }
+
+ /* Unflatten the fdt into a new dt_host. */
+ unflatten_device_tree(fdt, &dt_host_new);
+
+ for ( j = 0; j < num_overlay_nodes; j++ )
+ {
+ dt_dprintk("Adding node: %s\n", nodes_full_path[j]);
+
+ /* Find the newly added node in dt_host_new by it's full path. */
+ overlay_node = _dt_find_node_by_path(dt_host_new, nodes_full_path[j]);
+ if ( overlay_node == NULL )
+ {
+ dt_dprintk("%s node not found\n", nodes_full_path[j]);
+ rc = -EFAULT;
+ goto remove_node;
+ }
+
+ /* Add the node to dt_host. */
+ rc = dt_overlay_add_node(overlay_node, overlay_node->parent->full_name);
+ if ( rc )
+ {
+ /* Node not added in dt_host. */
+ goto remove_node;
+ }
+
+ overlay_node = dt_find_node_by_path(overlay_node->full_name);
+ if ( overlay_node == NULL )
+ {
+ /* Sanity check. But code will never come here. */
+ printk(XENLOG_ERR "Cannot find %s node under updated dt_host\n",
+ overlay_node->name);
+ goto remove_node;
+ }
+
+ /* First let's handle the interrupts. */
+ rc = handle_device_interrupts(d, overlay_node, false);
+ if ( rc )
+ {
+ printk(XENLOG_ERR "Interrupt failed\n");
+ goto remove_node;
+ }
+
+ /* Store IRQs for each node. */
+ num_irq = dt_number_of_irq(overlay_node);
+ node_num_irq[j] = num_irq;
+ nodes_irq[j] = xmalloc_bytes(num_irq * sizeof(int));
+ if ( nodes_irq[j] == NULL )
+ {
+ rc = -ENOMEM;
+ goto remove_node;
+ }
+
+ for ( k = 0; k < num_irq; k++ )
+ {
+ nodes_irq[j][k] = platform_get_irq(overlay_node, k);
+ }
+
+ /* Add device to IOMMUs */
+ rc = iommu_add_dt_device(overlay_node);
+ if ( rc < 0 )
+ {
+ printk(XENLOG_ERR "Failed to add %s to the IOMMU\n",
+ dt_node_full_name(overlay_node));
+ goto remove_node;
+ }
+
+ /* Set permissions. */
+ naddr = dt_number_of_address(overlay_node);
+
+ dt_dprintk("%s passthrough = %d naddr = %u\n",
+ dt_node_full_name(overlay_node), false, naddr);
+
+ /* Give permission for map MMIOs */
+ for ( i = 0; i < naddr; i++ )
+ {
+ struct map_range_data mr_data = { .d = d,
+ .p2mt = p2m_mmio_direct_c,
+ .skip_mapping = true };
+ rc = dt_device_get_address(overlay_node, i, &addr, &size);
+ if ( rc )
+ {
+ printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
+ i, dt_node_full_name(overlay_node));
+ goto remove_node;
+ }
+
+ rc = map_range_to_domain(overlay_node, addr, size, &mr_data);
+ if ( rc )
+ goto remove_node;
+ }
+ }
+
+ /* This will happen if everything above goes right. */
+ tr = xzalloc(struct overlay_track);
+ if ( tr == NULL )
+ {
+ rc = -ENOMEM;
+ goto remove_node;
+ }
+
+ tr->dt_host_new = dt_host_new;
+ tr->fdt = fdt;
+ tr->nodes_fullname = nodes_full_path;
+ tr->num_nodes = num_overlay_nodes;
+ tr->nodes_irq = nodes_irq;
+ tr->node_num_irq = node_num_irq;
+
+ if ( tr->nodes_fullname == NULL )
+ {
+ rc = -ENOMEM;
+ goto remove_node;
+ }
+
+ INIT_LIST_HEAD(&tr->entry);
+ list_add_tail(&tr->entry, &overlay_tracker);
+
+ spin_unlock(&overlay_lock);
+ return rc;
+
+/*
+ * Failure case. We need to remove the nodes, free tracker(if tr exists) and
+ * dt_host_new.
+ */
+remove_node:
+ rc = remove_nodes(nodes_full_path, nodes_irq, node_num_irq, j);
+
+ if ( rc )
+ {
+ printk(XENLOG_ERR "Removing node failed\n");
+ spin_unlock(&overlay_lock);
+ return rc;
+ }
+
+err:
+ spin_unlock(&overlay_lock);
+
+ xfree(dt_host_new);
+ xfree(fdt);
+
+ if ( nodes_full_path != NULL )
+ {
+ for ( i = 0; i < num_overlay_nodes && nodes_full_path[i] != NULL; i++ )
+ {
+ xfree(nodes_full_path[i]);
+ }
+ xfree(nodes_full_path);
+ }
+
+ if ( nodes_irq != NULL )
+ {
+ for ( i = 0; i < num_overlay_nodes && nodes_irq[i] != NULL; i++ )
+ {
+ xfree(nodes_irq[i]);
+ }
+ xfree(nodes_irq);
+ }
+
+ if ( node_num_irq )
+ xfree(node_num_irq);
+
+ xfree(tr);
+
+ return rc;
+}
+
long dt_sysctl(struct xen_sysctl *op)
{
long ret = 0;
@@ -404,6 +723,11 @@ long dt_sysctl(struct xen_sysctl *op)
switch ( op->u.dt_overlay.overlay_op )
{
+ case XEN_SYSCTL_DT_OVERLAY_ADD:
+ ret = handle_add_overlay_nodes(overlay_fdt,
+ op->u.dt_overlay.overlay_fdt_size);
+ break;
+
case XEN_SYSCTL_DT_OVERLAY_REMOVE:
ret = check_overlay_fdt(overlay_fdt,
op->u.dt_overlay.overlay_fdt_size);
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index e256aeb7c6..bb3ef44989 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1069,6 +1069,7 @@ typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
#endif
+#define XEN_SYSCTL_DT_OVERLAY_ADD 1
#define XEN_SYSCTL_DT_OVERLAY_REMOVE 2
/*
--
2.17.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 11/14] xen/arm: Implement device tree node addition functionalities
2022-03-08 19:47 ` [XEN][RFC PATCH v3 11/14] xen/arm: Implement device tree node addition functionalities Vikram Garhwal
@ 2022-03-15 10:40 ` Luca Fancellu
2022-05-18 19:03 ` Julien Grall
1 sibling, 0 replies; 42+ messages in thread
From: Luca Fancellu @ 2022-03-15 10:40 UTC (permalink / raw)
To: Vikram Garhwal
Cc: Xen-devel, Stefano Stabellini, julien, Bertrand Marquis,
volodymyr_babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
Wei Liu
>
> +static int dt_overlay_add_node(struct dt_device_node *device_node,
> + const char *parent_node_path)
Const should be indented at the same level of struct
> +/*
> + * Adds device tree nodes under target node.
> + * We use dt_host_new to unflatten the updated device_tree_flattened. This is
> + * done to avoid the removal of device_tree generation, iomem regions mapping to
> + * hardware domain done by handle_node().
> + */
> +static long handle_add_overlay_nodes(void *overlay_fdt,
> + uint32_t overlay_fdt_size)
> +{
> + int rc = 0;
> + struct dt_device_node *overlay_node;
> + char **nodes_full_path = NULL;
> + int **nodes_irq = NULL;
> + int *node_num_irq = NULL;
> + void *fdt = NULL;
> + struct dt_device_node *dt_host_new = NULL;
> + struct domain *d = hardware_domain;
> + struct overlay_track *tr = NULL;
> + unsigned int naddr;
> + unsigned int num_irq;
> + unsigned int i, j, k;
> + unsigned int num_overlay_nodes;
All unsigned int can stay in the same line
> + u64 addr, size;
> +
> + fdt = xmalloc_bytes(fdt_totalsize(device_tree_flattened));
> + if ( fdt == NULL )
> + return -ENOMEM;
> +
> + num_overlay_nodes = overlay_node_count(overlay_fdt);
> + if ( num_overlay_nodes == 0 )
> + {
> + xfree(fdt);
> + return -ENOMEM;
> + }
> +
> + spin_lock(&overlay_lock);
> +
> + memcpy(fdt, device_tree_flattened, fdt_totalsize(device_tree_flattened));
> +
> + rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
> + if ( rc )
> + {
> + xfree(fdt);
> + return rc;
> + }
> +
> + /*
> + * overlay_get_nodes_info is called to get the node information from dtbo.
> + * This is done before fdt_overlay_apply() because the overlay apply will
> + * erase the magic of overlay_fdt.
> + */
> + rc = overlay_get_nodes_info(overlay_fdt, &nodes_full_path,
> + num_overlay_nodes);
> + if ( rc )
> + {
> + printk(XENLOG_ERR "Getting nodes information failed with error %d\n",
> + rc);
> + goto err;
> + }
> +
> + nodes_irq = xmalloc_bytes(num_overlay_nodes * sizeof(int *));
You can use xzalloc_bytes and remove the memset below here and...
> +
> + if ( nodes_irq == NULL )
> + {
> + rc = -ENOMEM;
> + goto err;
> + }
> + memset(nodes_irq, 0x0, num_overlay_nodes * sizeof(int *));
> +
> + node_num_irq = xmalloc_bytes(num_overlay_nodes * sizeof(int));
Here
> + if ( node_num_irq == NULL )
> + {
> + rc = -ENOMEM;
> + goto err;
> + }
> + memset(node_num_irq, 0x0, num_overlay_nodes * sizeof(int));
> +
> + rc = fdt_overlay_apply(fdt, overlay_fdt);
> + if ( rc )
> + {
> + printk(XENLOG_ERR "Adding overlay node failed with error %d\n", rc);
> + goto err;
> + }
> +
> + […]
> +err:
> + spin_unlock(&overlay_lock);
> +
> + xfree(dt_host_new);
> + xfree(fdt);
> +
> + if ( nodes_full_path != NULL )
> + {
> + for ( i = 0; i < num_overlay_nodes && nodes_full_path[i] != NULL; i++ )
> + {
> + xfree(nodes_full_path[i]);
> + }
> + xfree(nodes_full_path);
> + }
> +
> + if ( nodes_irq != NULL )
> + {
> + for ( i = 0; i < num_overlay_nodes && nodes_irq[i] != NULL; i++ )
> + {
> + xfree(nodes_irq[i]);
> + }
> + xfree(nodes_irq);
> + }
I see you use this operation quite a bit in this module, perhaps you can create a function to
do that
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 11/14] xen/arm: Implement device tree node addition functionalities
2022-03-08 19:47 ` [XEN][RFC PATCH v3 11/14] xen/arm: Implement device tree node addition functionalities Vikram Garhwal
2022-03-15 10:40 ` Luca Fancellu
@ 2022-05-18 19:03 ` Julien Grall
1 sibling, 0 replies; 42+ messages in thread
From: Julien Grall @ 2022-05-18 19:03 UTC (permalink / raw)
To: Vikram Garhwal, xen-devel
Cc: sstabellini, bertrand.marquis, volodymyr_babchuk, Andrew Cooper,
George Dunlap, Jan Beulich, Wei Liu
Hi Vikram,
On 08/03/2022 19:47, Vikram Garhwal wrote:
> Update sysctl XEN_SYSCTL_dt_overlay to enable support for dtbo nodes addition
> using device tree overlay.
>
> xl overlay add file.dtbo:
> Each time overlay nodes are added using .dtbo, a new fdt(memcpy of
> device_tree_flattened) is created and updated with overlay nodes. This
> updated fdt is further unflattened to a dt_host_new. Next, it checks if any
> of the overlay nodes already exists in the dt_host. If overlay nodes doesn't
> exist then find the overlay nodes in dt_host_new, find the overlay node's
> parent in dt_host and add the nodes as child under their parent in the
> dt_host. The node is attached as the last node under target parent.
>
> Finally, add IRQs, add device to IOMMUs, set permissions and map MMIO for the
> overlay node.
>
> When a node is added using overlay, a new entry is allocated in the
> overlay_track to keep the track of memory allocation due to addition of overlay
> node. This is helpful for freeing the memory allocated when a device tree node
> is removed.
>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
> xen/common/dt_overlay.c | 324 ++++++++++++++++++++++++++++++++++++
> xen/include/public/sysctl.h | 1 +
> 2 files changed, 325 insertions(+)
>
> diff --git a/xen/common/dt_overlay.c b/xen/common/dt_overlay.c
> index fcb31de495..01aed62d74 100644
> --- a/xen/common/dt_overlay.c
> +++ b/xen/common/dt_overlay.c
> @@ -82,6 +82,64 @@ static int dt_overlay_remove_node(struct dt_device_node *device_node)
> return 0;
> }
>
> +static int dt_overlay_add_node(struct dt_device_node *device_node,
> + const char *parent_node_path)
> +{
> + struct dt_device_node *parent_node;
> + struct dt_device_node *np;
> + struct dt_device_node *next_node;
> + struct dt_device_node *new_node;
> +
> + parent_node = dt_find_node_by_path(parent_node_path);
> +
> + new_node = device_node;
You only use device_node to set new_node. So the local variable is a bit
pointless. I don't mind whether you want to rename the parameter
new_node or keep the existing name.
> +
> + if ( new_node == NULL )
OOI, how could it be NULL?
> + return -EINVAL;
> +
> + if ( parent_node == NULL )
> + {
> + dt_dprintk("Node not found. Partial dtb will not be added");
> + return -EINVAL;
> + }
> +
> + /*
> + * If node is found. We can attach the device_node as a child of the
Below you use new_node rather than device_node
> + * parent node.
> + */
> +
> + /* If parent has no child. */
> + if ( parent_node->child == NULL )
> + {
> + next_node = parent_node->allnext;
> + new_node->parent = parent_node;
> + parent_node->allnext = new_node;
> + parent_node->child = new_node;
> + /* Now plug next_node at the end of device_node. */
Same.
> + new_node->allnext = next_node;
> + } else {
Coding style:
The } and { should be on there own line. I.e:
}
else
{
> + /* If parent has at least one child node. */
> +
> + /*
> + * Iterate to the last child node of parent.
> + */
> + for ( np = parent_node->child; np->sibling != NULL; np = np->sibling )
> + {
> + }
> +
> + next_node = np->allnext;
> + new_node->parent = parent_node;
> + np->sibling = new_node;
> + np->allnext = new_node;
> + /* Now plug next_node at the end of device_node. */
Same remark about device node.
> + new_node->sibling = next_node;
> + new_node->allnext = next_node;
> + np->sibling->sibling = NULL;
> + }
> +
> + return 0;
> +}
> +
> /* Basic sanity check for the dtbo tool stack provided to Xen. */
> static int check_overlay_fdt(const void *overlay_fdt, uint32_t overlay_fdt_size)
> {
> @@ -377,6 +435,267 @@ out:
> return rc;
> }
>
> +/*
> + * Adds device tree nodes under target node.
> + * We use dt_host_new to unflatten the updated device_tree_flattened. This is
> + * done to avoid the removal of device_tree generation, iomem regions mapping to
> + * hardware domain done by handle_node().
> + */
> +static long handle_add_overlay_nodes(void *overlay_fdt,
> + uint32_t overlay_fdt_size)
> +{
> + int rc = 0;
> + struct dt_device_node *overlay_node;
> + char **nodes_full_path = NULL;
> + int **nodes_irq = NULL;
> + int *node_num_irq = NULL;
> + void *fdt = NULL;
> + struct dt_device_node *dt_host_new = NULL;
> + struct domain *d = hardware_domain;
> + struct overlay_track *tr = NULL;
> + unsigned int naddr;
> + unsigned int num_irq;
> + unsigned int i, j, k;
> + unsigned int num_overlay_nodes;
> + u64 addr, size;
> +
> + fdt = xmalloc_bytes(fdt_totalsize(device_tree_flattened));
> + if ( fdt == NULL )
> + return -ENOMEM;
> +
> + num_overlay_nodes = overlay_node_count(overlay_fdt);
> + if ( num_overlay_nodes == 0 )
> + {
> + xfree(fdt);
> + return -ENOMEM;
> + }
> +
> + spin_lock(&overlay_lock);
> +
> + memcpy(fdt, device_tree_flattened, fdt_totalsize(device_tree_flattened));
> +
> + rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size);
AFAICT, this doesn't need to be checked within the lock. So it would be
better to move it before grabbing the lock and the allocation.
> + if ( rc )
> + {
> + xfree(fdt);
> + return rc;
> + }
> +
> + /*
> + * overlay_get_nodes_info is called to get the node information from dtbo.
> + * This is done before fdt_overlay_apply() because the overlay apply will
> + * erase the magic of overlay_fdt.
> + */
> + rc = overlay_get_nodes_info(overlay_fdt, &nodes_full_path,
> + num_overlay_nodes);
> + if ( rc )
> + {
> + printk(XENLOG_ERR "Getting nodes information failed with error %d\n",
> + rc);
> + goto err;
> + }
> +
> + nodes_irq = xmalloc_bytes(num_overlay_nodes * sizeof(int *));
> +
> + if ( nodes_irq == NULL )
> + {
> + rc = -ENOMEM;
> + goto err;
> + }
> + memset(nodes_irq, 0x0, num_overlay_nodes * sizeof(int *));
> +
> + node_num_irq = xmalloc_bytes(num_overlay_nodes * sizeof(int));
> + if ( node_num_irq == NULL )
> + {
> + rc = -ENOMEM;
> + goto err;
> + }
> + memset(node_num_irq, 0x0, num_overlay_nodes * sizeof(int));
> +
> + rc = fdt_overlay_apply(fdt, overlay_fdt);
I am a bit puzzled how this work. Above, you allocated 'fdt' with the
size of the current device-tree. So there might not be enough space in
the current fdt to apply the overlay. Can you clarify it?
> + if ( rc )
> + {
> + printk(XENLOG_ERR "Adding overlay node failed with error %d\n", rc);
> + goto err;
> + }
> +
> + for ( j = 0; j < num_overlay_nodes; j++ )
> + {
> + /* Check if any of the node already exists in dt_host. */
> + overlay_node = dt_find_node_by_path(nodes_full_path[j]);
> + if ( overlay_node != NULL )
> + {
> + printk(XENLOG_ERR "node %s exists in device tree\n",
> + nodes_full_path[j]);
> + rc = -EINVAL;
> + goto err;
> + }
> + }
> +
> + /* Unflatten the fdt into a new dt_host. */
> + unflatten_device_tree(fdt, &dt_host_new);
As I mentionned before, unflatten_device_tree() may fail to allocate
memory. So it needs to be updated to propogate any error in order to use
it in an hypercall.
> +
> + for ( j = 0; j < num_overlay_nodes; j++ )
> + {
> + dt_dprintk("Adding node: %s\n", nodes_full_path[j]);
> +
> + /* Find the newly added node in dt_host_new by it's full path. */
> + overlay_node = _dt_find_node_by_path(dt_host_new, nodes_full_path[j]);
> + if ( overlay_node == NULL )
> + {
> + dt_dprintk("%s node not found\n", nodes_full_path[j]);
> + rc = -EFAULT;
> + goto remove_node;
> + }
> +
> + /* Add the node to dt_host. */
> + rc = dt_overlay_add_node(overlay_node, overlay_node->parent->full_name);
> + if ( rc )
> + {
> + /* Node not added in dt_host. */
> + goto remove_node;
> + }
> +
> + overlay_node = dt_find_node_by_path(overlay_node->full_name);
> + if ( overlay_node == NULL )
> + {
> + /* Sanity check. But code will never come here. */
Please add ASSERT_UNREACHABLE just to make clear this is not expected.
> + printk(XENLOG_ERR "Cannot find %s node under updated dt_host\n",
> + overlay_node->name);
> + goto remove_node;
> + }
> +
> + /* First let's handle the interrupts. */
> + rc = handle_device_interrupts(d, overlay_node, false);
> + if ( rc )
> + {
> + printk(XENLOG_ERR "Interrupt failed\n");
> + goto remove_node;
> + }
> +
> + /* Store IRQs for each node. */
> + num_irq = dt_number_of_irq(overlay_node);
> + node_num_irq[j] = num_irq;
> + nodes_irq[j] = xmalloc_bytes(num_irq * sizeof(int));
> + if ( nodes_irq[j] == NULL )
> + {
> + rc = -ENOMEM;
> + goto remove_node;
> + }
> +
> + for ( k = 0; k < num_irq; k++ )
> + {
> + nodes_irq[j][k] = platform_get_irq(overlay_node, k);
platform_get_irq() can fail.
> + }
> +
> + /* Add device to IOMMUs */
> + rc = iommu_add_dt_device(overlay_node);
> + if ( rc < 0 )
> + {
> + printk(XENLOG_ERR "Failed to add %s to the IOMMU\n",
> + dt_node_full_name(overlay_node));
> + goto remove_node;
> + }
> +
> + /* Set permissions. */
> + naddr = dt_number_of_address(overlay_node);
> +
> + dt_dprintk("%s passthrough = %d naddr = %u\n",
> + dt_node_full_name(overlay_node), false, naddr);
> +
> + /* Give permission for map MMIOs */
> + for ( i = 0; i < naddr; i++ )
> + {
> + struct map_range_data mr_data = { .d = d,
> + .p2mt = p2m_mmio_direct_c,
> + .skip_mapping = true };
> + rc = dt_device_get_address(overlay_node, i, &addr, &size);
> + if ( rc )
> + {
> + printk(XENLOG_ERR "Unable to retrieve address %u for %s\n",
> + i, dt_node_full_name(overlay_node));
> + goto remove_node;
> + }
> +
> + rc = map_range_to_domain(overlay_node, addr, size, &mr_data);
> + if ( rc )
> + goto remove_node;
> + }
> + }
> +
> + /* This will happen if everything above goes right. */
> + tr = xzalloc(struct overlay_track);
I think it would be best to allocate the overlay structure first. So 1)
you don't have to undo everything because of an allocation failure and
2) you can remove a lot of local variables and use "tr-><field>" directly.
> + if ( tr == NULL )
> + {
> + rc = -ENOMEM;
> + goto remove_node;
> + }
> +
> + tr->dt_host_new = dt_host_new;
> + tr->fdt = fdt;
> + tr->nodes_fullname = nodes_full_path;
> + tr->num_nodes = num_overlay_nodes;
> + tr->nodes_irq = nodes_irq;
> + tr->node_num_irq = node_num_irq;
> +
> + if ( tr->nodes_fullname == NULL )
> + {
> + rc = -ENOMEM;
> + goto remove_node;
> + }
> +
> + INIT_LIST_HEAD(&tr->entry);
> + list_add_tail(&tr->entry, &overlay_tracker);
> +
> + spin_unlock(&overlay_lock);
> + return rc;
> +
> +/*
> + * Failure case. We need to remove the nodes, free tracker(if tr exists) and
> + * dt_host_new.
> + */
> +remove_node:
> + rc = remove_nodes(nodes_full_path, nodes_irq, node_num_irq, j);
Looking at the implement of remove_nodes(), it would not be able to cope
if half of it is initialized. So it an error would be returned (or
potentially crash as you set node_num_irq[X] before allocating the
memory) and...
> +
> + if ( rc )
> + {
> + printk(XENLOG_ERR "Removing node failed\n");
> + spin_unlock(&overlay_lock);
> + return rc;
... no memory would be freed. It is not clear to me whether the memory
leak is on purpose. If it is, then I think it should be written down in
a comment.
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index e256aeb7c6..bb3ef44989 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -1069,6 +1069,7 @@ typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
> DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
> #endif
>
> +#define XEN_SYSCTL_DT_OVERLAY_ADD 1
I find a bit odd that the documentation is added before the number. I
think it might make sense to either define the two sysctl in the
previous patch or in a separate one. I don't mind either way.
> #define XEN_SYSCTL_DT_OVERLAY_REMOVE 2
>
> /*
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 42+ messages in thread
* [XEN][RFC PATCH v3 12/14] tools/libs/ctrl: Implement new xc interfaces for dt overlay
2022-03-08 19:46 [XEN][RFC PATCH v3 00/14] dynamic node programming using overlay dtbo Vikram Garhwal
` (10 preceding siblings ...)
2022-03-08 19:47 ` [XEN][RFC PATCH v3 11/14] xen/arm: Implement device tree node addition functionalities Vikram Garhwal
@ 2022-03-08 19:47 ` Vikram Garhwal
2022-03-15 10:49 ` Luca Fancellu
2022-03-17 15:47 ` Anthony PERARD
2022-03-08 19:47 ` [XEN][RFC PATCH v3 13/14] tools/libs/light: Implement new libxl functions for device tree overlay ops Vikram Garhwal
2022-03-08 19:47 ` [XEN][RFC PATCH v3 14/14] tools/xl: Add new xl command overlay for device tree overlay support Vikram Garhwal
13 siblings, 2 replies; 42+ messages in thread
From: Vikram Garhwal @ 2022-03-08 19:47 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk,
Vikram Garhwal, Wei Liu, Anthony PERARD, Juergen Gross
xc_dt_overlay() sends the device tree binary overlay, size of .dtbo and overlay
operation type i.e. add or remove to xen.
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
tools/include/xenctrl.h | 3 +++
tools/libs/ctrl/Makefile | 1 +
tools/libs/ctrl/xc_overlay.c | 51 ++++++++++++++++++++++++++++++++++++
3 files changed, 55 insertions(+)
create mode 100644 tools/libs/ctrl/xc_overlay.c
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 95bd5eca67..b7552d0d9c 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2629,6 +2629,9 @@ int xc_livepatch_replace(xc_interface *xch, char *name, uint32_t timeout, uint32
int xc_domain_cacheflush(xc_interface *xch, uint32_t domid,
xen_pfn_t start_pfn, xen_pfn_t nr_pfns);
+int xc_dt_overlay(xc_interface *xch, void *overlay_fdt, int overlay_fdt_size,
+ uint8_t overlay_op);
+
/* Compat shims */
#include "xenctrl_compat.h"
diff --git a/tools/libs/ctrl/Makefile b/tools/libs/ctrl/Makefile
index ef7362327f..848a8737c7 100644
--- a/tools/libs/ctrl/Makefile
+++ b/tools/libs/ctrl/Makefile
@@ -3,6 +3,7 @@ include $(XEN_ROOT)/tools/Rules.mk
SRCS-y += xc_altp2m.c
SRCS-y += xc_cpupool.c
+SRCS-y += xc_overlay.c
SRCS-y += xc_domain.c
SRCS-y += xc_evtchn.c
SRCS-y += xc_gnttab.c
diff --git a/tools/libs/ctrl/xc_overlay.c b/tools/libs/ctrl/xc_overlay.c
new file mode 100644
index 0000000000..8fe780d75a
--- /dev/null
+++ b/tools/libs/ctrl/xc_overlay.c
@@ -0,0 +1,51 @@
+/*
+ *
+ * Overlay control functions.
+ * Copyright (C) 2021 Xilinx Inc.
+ * Author Vikram Garhwal <fnu.vikram@xilinx.com>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation;
+ * version 2.1 of the License.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "xc_bitops.h"
+#include "xc_private.h"
+#include <xen/hvm/hvm_op.h>
+#include <libfdt.h>
+
+int xc_dt_overlay(xc_interface *xch, void *overlay_fdt, int overlay_fdt_size,
+ uint8_t overlay_op)
+{
+ int err;
+ DECLARE_SYSCTL;
+
+ DECLARE_HYPERCALL_BOUNCE(overlay_fdt, overlay_fdt_size,
+ XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+ if ( (err = xc_hypercall_bounce_pre(xch, overlay_fdt)) )
+ goto err;
+
+ sysctl.cmd = XEN_SYSCTL_dt_overlay;
+ sysctl.u.dt_overlay.overlay_op = overlay_op;
+ sysctl.u.dt_overlay.overlay_fdt_size = overlay_fdt_size;
+
+ set_xen_guest_handle(sysctl.u.dt_overlay.overlay_fdt, overlay_fdt);
+
+ if ( (err = do_sysctl(xch, &sysctl)) != 0 )
+ PERROR("%s failed\n", __func__);
+
+err:
+ xc_hypercall_bounce_post(xch, overlay_fdt);
+
+ return err;
+}
--
2.17.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 12/14] tools/libs/ctrl: Implement new xc interfaces for dt overlay
2022-03-08 19:47 ` [XEN][RFC PATCH v3 12/14] tools/libs/ctrl: Implement new xc interfaces for dt overlay Vikram Garhwal
@ 2022-03-15 10:49 ` Luca Fancellu
2022-03-17 15:47 ` Anthony PERARD
1 sibling, 0 replies; 42+ messages in thread
From: Luca Fancellu @ 2022-03-15 10:49 UTC (permalink / raw)
To: Vikram Garhwal
Cc: Xen-devel, sstabellini, julien, Bertrand Marquis,
volodymyr_babchuk, Wei Liu, Anthony PERARD, Juergen Gross
>
> diff --git a/tools/libs/ctrl/Makefile b/tools/libs/ctrl/Makefile
> index ef7362327f..848a8737c7 100644
> --- a/tools/libs/ctrl/Makefile
> +++ b/tools/libs/ctrl/Makefile
> @@ -3,6 +3,7 @@ include $(XEN_ROOT)/tools/Rules.mk
>
> SRCS-y += xc_altp2m.c
> SRCS-y += xc_cpupool.c
> +SRCS-y += xc_overlay.c
I think these entries are in alphabetical order
> SRCS-y += xc_domain.c
> SRCS-y += xc_evtchn.c
> SRCS-y += xc_gnttab.c
> diff --git a/tools/libs/ctrl/xc_overlay.c b/tools/libs/ctrl/xc_overlay.c
> new file mode 100644
> index 0000000000..8fe780d75a
> --- /dev/null
> +++ b/tools/libs/ctrl/xc_overlay.c
> @@ -0,0 +1,51 @@
> +/*
> + *
This blank line can be removed
> + * Overlay control functions.
> + * Copyright (C) 2021 Xilinx Inc.
> + * Author Vikram Garhwal <fnu.vikram@xilinx.com>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation;
> + * version 2.1 of the License.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "xc_bitops.h"
> +#include "xc_private.h"
> +#include <xen/hvm/hvm_op.h>
> +#include <libfdt.h>
> +
> +int xc_dt_overlay(xc_interface *xch, void *overlay_fdt, int overlay_fdt_size,
> + uint8_t overlay_op)
> +{
> + int err;
> + DECLARE_SYSCTL;
> +
> + DECLARE_HYPERCALL_BOUNCE(overlay_fdt, overlay_fdt_size,
> + XC_HYPERCALL_BUFFER_BOUNCE_IN);
XC_HYPERCALL_BUFFER_BOUNCE_IN can stay at the same level of overlay_fdt
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 12/14] tools/libs/ctrl: Implement new xc interfaces for dt overlay
2022-03-08 19:47 ` [XEN][RFC PATCH v3 12/14] tools/libs/ctrl: Implement new xc interfaces for dt overlay Vikram Garhwal
2022-03-15 10:49 ` Luca Fancellu
@ 2022-03-17 15:47 ` Anthony PERARD
1 sibling, 0 replies; 42+ messages in thread
From: Anthony PERARD @ 2022-03-17 15:47 UTC (permalink / raw)
To: Vikram Garhwal
Cc: xen-devel, sstabellini, julien, bertrand.marquis,
volodymyr_babchuk, Wei Liu, Juergen Gross
Hi Vikram,
On Tue, Mar 08, 2022 at 11:47:02AM -0800, Vikram Garhwal wrote:
> diff --git a/tools/libs/ctrl/xc_overlay.c b/tools/libs/ctrl/xc_overlay.c
> new file mode 100644
> index 0000000000..8fe780d75a
> --- /dev/null
> +++ b/tools/libs/ctrl/xc_overlay.c
Could rename this new file? I don't think using "overlay" alone is going
to be helpful to figure out what it is about. Renaming it
"xc_dt_overlay.c" would already be better.
> @@ -0,0 +1,51 @@
> +/*
> + *
> + * Overlay control functions.
Maybe "Device Tree overlay functions" would be better. I'm not sure that
"control" is useful in the file description.
> + * Copyright (C) 2021 Xilinx Inc.
> + * Author Vikram Garhwal <fnu.vikram@xilinx.com>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation;
> + * version 2.1 of the License.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "xc_bitops.h"
> +#include "xc_private.h"
> +#include <xen/hvm/hvm_op.h>
> +#include <libfdt.h>
> +
> +int xc_dt_overlay(xc_interface *xch, void *overlay_fdt, int overlay_fdt_size,
> + uint8_t overlay_op)
Shouldn't the function prototype match the types from the sysctl
structure? There is "int" vs "uint32_t" for "overlay_fdt_size".
> +{
> + int err;
> + DECLARE_SYSCTL;
> +
> + DECLARE_HYPERCALL_BOUNCE(overlay_fdt, overlay_fdt_size,
> + XC_HYPERCALL_BUFFER_BOUNCE_IN);
> +
> + if ( (err = xc_hypercall_bounce_pre(xch, overlay_fdt)) )
> + goto err;
> +
> + sysctl.cmd = XEN_SYSCTL_dt_overlay;
> + sysctl.u.dt_overlay.overlay_op = overlay_op;
> + sysctl.u.dt_overlay.overlay_fdt_size = overlay_fdt_size;
> +
> + set_xen_guest_handle(sysctl.u.dt_overlay.overlay_fdt, overlay_fdt);
> +
> + if ( (err = do_sysctl(xch, &sysctl)) != 0 )
> + PERROR("%s failed\n", __func__);
The \n should be remove from the message. perror already adds a newline,
and it also adds information about the error after the message so a
newline in the middle will make it harder to read the error.
> +
> +err:
> + xc_hypercall_bounce_post(xch, overlay_fdt);
> +
> + return err;
> +}
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 42+ messages in thread
* [XEN][RFC PATCH v3 13/14] tools/libs/light: Implement new libxl functions for device tree overlay ops
2022-03-08 19:46 [XEN][RFC PATCH v3 00/14] dynamic node programming using overlay dtbo Vikram Garhwal
` (11 preceding siblings ...)
2022-03-08 19:47 ` [XEN][RFC PATCH v3 12/14] tools/libs/ctrl: Implement new xc interfaces for dt overlay Vikram Garhwal
@ 2022-03-08 19:47 ` Vikram Garhwal
2022-03-15 10:58 ` Luca Fancellu
2022-03-17 17:45 ` Anthony PERARD
2022-03-08 19:47 ` [XEN][RFC PATCH v3 14/14] tools/xl: Add new xl command overlay for device tree overlay support Vikram Garhwal
13 siblings, 2 replies; 42+ messages in thread
From: Vikram Garhwal @ 2022-03-08 19:47 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk,
Vikram Garhwal, Wei Liu, Anthony PERARD, Juergen Gross
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
tools/include/libxl.h | 3 ++
tools/libs/light/Makefile | 1 +
tools/libs/light/libxl_overlay.c | 67 ++++++++++++++++++++++++++++++++
3 files changed, 71 insertions(+)
create mode 100644 tools/libs/light/libxl_overlay.c
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index 51a9b6cfac..b31e17c2ce 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -2419,6 +2419,9 @@ libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid,
int *num);
void libxl_device_pci_list_free(libxl_device_pci* list, int num);
+int libxl_dt_overlay(libxl_ctx *ctx, void *overlay,
+ int overlay_size, uint8_t overlay_op);
+
/*
* Turns the current process into a backend device service daemon
* for a driver domain.
diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
index 453bea0067..405115c13c 100644
--- a/tools/libs/light/Makefile
+++ b/tools/libs/light/Makefile
@@ -116,6 +116,7 @@ SRCS-y += libxl_genid.c
SRCS-y += _libxl_types.c
SRCS-y += libxl_flask.c
SRCS-y += _libxl_types_internal.c
+SRCS-y += libxl_overlay.o
ifeq ($(CONFIG_LIBNL),y)
CFLAGS_LIBXL += $(LIBNL3_CFLAGS)
diff --git a/tools/libs/light/libxl_overlay.c b/tools/libs/light/libxl_overlay.c
new file mode 100644
index 0000000000..e370e8cac8
--- /dev/null
+++ b/tools/libs/light/libxl_overlay.c
@@ -0,0 +1,67 @@
+/*
+ * Copyright (C) 2021 Xilinx Inc.
+ * Author Vikram Garhwal <fnu.vikram@xilinx.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published
+ * by the Free Software Foundation; version 2.1 only. with the special
+ * exception on linking described in file LICENSE.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Lesser General Public License for more details.
+ */
+
+#include "libxl_osdeps.h" /* must come before any other headers */
+#include "libxl_internal.h"
+#include <libfdt.h>
+#include <xenguest.h>
+#include <xenctrl.h>
+
+static int check_overlay_fdt(libxl__gc *gc, void *fdt, size_t size)
+{
+ int r;
+
+ if (fdt_magic(fdt) != FDT_MAGIC) {
+ LOG(ERROR, "Overlay FDT is not a valid Flat Device Tree");
+ return ERROR_FAIL;
+ }
+
+ r = fdt_check_header(fdt);
+ if (r) {
+ LOG(ERROR, "Failed to check the overlay FDT (%d)", r);
+ return ERROR_FAIL;
+ }
+
+ if (fdt_totalsize(fdt) > size) {
+ LOG(ERROR, "Overlay FDT totalsize is too big");
+ return ERROR_FAIL;
+ }
+
+ return 0;
+}
+
+int libxl_dt_overlay(libxl_ctx *ctx, void *overlay_dt, int overlay_dt_size,
+ uint8_t overlay_op)
+{
+ int rc = 0;
+ GC_INIT(ctx);
+
+ if (check_overlay_fdt(gc, overlay_dt, overlay_dt_size)) {
+ LOG(ERROR, "Overlay DTB check failed\n");
+ GC_FREE;
+ return ERROR_FAIL;
+ } else
+ LOG(DEBUG, "Overlay DTB check passed\n");
+
+ /* We don't need to do xc_interface_open here. */
+ rc = xc_dt_overlay(ctx->xch, overlay_dt, overlay_dt_size, overlay_op);
+
+ if (rc)
+ LOG(ERROR, "%s: Adding/Removing overlay dtb failed.\n", __func__);
+
+ GC_FREE;
+ return rc;
+}
+
--
2.17.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 13/14] tools/libs/light: Implement new libxl functions for device tree overlay ops
2022-03-08 19:47 ` [XEN][RFC PATCH v3 13/14] tools/libs/light: Implement new libxl functions for device tree overlay ops Vikram Garhwal
@ 2022-03-15 10:58 ` Luca Fancellu
2022-03-17 17:45 ` Anthony PERARD
1 sibling, 0 replies; 42+ messages in thread
From: Luca Fancellu @ 2022-03-15 10:58 UTC (permalink / raw)
To: Vikram Garhwal
Cc: Xen-devel, sstabellini, julien, Bertrand Marquis,
volodymyr_babchuk, Wei Liu, Anthony PERARD, Juergen Gross
> On 8 Mar 2022, at 19:47, Vikram Garhwal <fnu.vikram@xilinx.com> wrote:
>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
I don’t know if an empty commit message is ok here, will leave to the maintainer
the choice, for me the code looks ok
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [XEN][RFC PATCH v3 13/14] tools/libs/light: Implement new libxl functions for device tree overlay ops
2022-03-08 19:47 ` [XEN][RFC PATCH v3 13/14] tools/libs/light: Implement new libxl functions for device tree overlay ops Vikram Garhwal
2022-03-15 10:58 ` Luca Fancellu
@ 2022-03-17 17:45 ` Anthony PERARD
1 sibling, 0 replies; 42+ messages in thread
From: Anthony PERARD @ 2022-03-17 17:45 UTC (permalink / raw)
To: Vikram Garhwal
Cc: xen-devel, sstabellini, julien, bertrand.marquis,
volodymyr_babchuk, Wei Liu, Juergen Gross
On Tue, Mar 08, 2022 at 11:47:03AM -0800, Vikram Garhwal wrote:
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
> tools/include/libxl.h | 3 ++
> tools/libs/light/Makefile | 1 +
> tools/libs/light/libxl_overlay.c | 67 ++++++++++++++++++++++++++++++++
> 3 files changed, 71 insertions(+)
> create mode 100644 tools/libs/light/libxl_overlay.c
>
> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> index 51a9b6cfac..b31e17c2ce 100644
> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
> @@ -2419,6 +2419,9 @@ libxl_device_pci *libxl_device_pci_list(libxl_ctx *ctx, uint32_t domid,
> int *num);
> void libxl_device_pci_list_free(libxl_device_pci* list, int num);
>
> +int libxl_dt_overlay(libxl_ctx *ctx, void *overlay,
> + int overlay_size, uint8_t overlay_op);
> +
As you are making changes to libxl's API, you are going to need to add a
"#define LIBXL_HAVE_*" in "include/libxl.h" about the availability of
the new function.
> /*
> * Turns the current process into a backend device service daemon
> * for a driver domain.
> diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
> index 453bea0067..405115c13c 100644
> --- a/tools/libs/light/Makefile
> +++ b/tools/libs/light/Makefile
> @@ -116,6 +116,7 @@ SRCS-y += libxl_genid.c
> SRCS-y += _libxl_types.c
> SRCS-y += libxl_flask.c
> SRCS-y += _libxl_types_internal.c
> +SRCS-y += libxl_overlay.o
Building this new file unconditionally is an issue at the moment.
"./configure" doesn't check if libfdt is on the system unless we happen
to build for Arm. And libfdt is mandatory when building for Arm.
Could you build this new source file on Arm only? With a comment why.
"SRCS-$(CONFIG_ARM) +="
Alternatively, you could try to rework configure to always check for
libfdt, but I doubt that device tree overlay are going to be useful on
x86 at the moment.
Then, libxl_dt_overlay() will need a stub that just return an error when
building on system that don't have libfdt.
>
> ifeq ($(CONFIG_LIBNL),y)
> CFLAGS_LIBXL += $(LIBNL3_CFLAGS)
> diff --git a/tools/libs/light/libxl_overlay.c b/tools/libs/light/libxl_overlay.c
> new file mode 100644
> index 0000000000..e370e8cac8
> --- /dev/null
> +++ b/tools/libs/light/libxl_overlay.c
Could you rename this new file "libxl_dt_overlay.c"? There could be
other kind of "overlay".
> @@ -0,0 +1,67 @@
> +/*
> + * Copyright (C) 2021 Xilinx Inc.
> + * Author Vikram Garhwal <fnu.vikram@xilinx.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU Lesser General Public License for more details.
> + */
> +
> +#include "libxl_osdeps.h" /* must come before any other headers */
> +#include "libxl_internal.h"
> +#include <libfdt.h>
> +#include <xenguest.h>
> +#include <xenctrl.h>
> +
> +static int check_overlay_fdt(libxl__gc *gc, void *fdt, size_t size)
> +{
> + int r;
> +
> + if (fdt_magic(fdt) != FDT_MAGIC) {
> + LOG(ERROR, "Overlay FDT is not a valid Flat Device Tree");
> + return ERROR_FAIL;
> + }
> +
> + r = fdt_check_header(fdt);
> + if (r) {
> + LOG(ERROR, "Failed to check the overlay FDT (%d)", r);
> + return ERROR_FAIL;
> + }
> +
> + if (fdt_totalsize(fdt) > size) {
> + LOG(ERROR, "Overlay FDT totalsize is too big");
> + return ERROR_FAIL;
> + }
> +
> + return 0;
> +}
> +
> +int libxl_dt_overlay(libxl_ctx *ctx, void *overlay_dt, int overlay_dt_size,
I wonder whether the type of "overlay_dt_size" should be something else.
check_overlay_fdt() takes a "size_t", but the hypercall takes
"uint32_t".
> + uint8_t overlay_op)
> +{
> + int rc = 0;
By CODING_STYLE, "rc" shouldn't be set when declared.
> + GC_INIT(ctx);
> +
> + if (check_overlay_fdt(gc, overlay_dt, overlay_dt_size)) {
> + LOG(ERROR, "Overlay DTB check failed\n");
No need for \n, it's already added by LOG().
> + GC_FREE;
> + return ERROR_FAIL;
Instead of writing GC_FREE more than once, could you set "rc" then "goto
out;"?
> + } else
> + LOG(DEBUG, "Overlay DTB check passed\n");
This needs to be in a {} block because the true side of the "if" uses
braces.
> +
> + /* We don't need to do xc_interface_open here. */
That comment isn't very useful, as it is a fact of using "ctx".
> + rc = xc_dt_overlay(ctx->xch, overlay_dt, overlay_dt_size, overlay_op);
Instead of "rc", could you store the result of xc_dt_overlay() in "r"?
"rc" is reserved for libxl error code. Also, the return value of
libxl_dt_overlay() can't be the return value of xc_dt_overlay().
There's some better explanation in the CODING_STYLE file, if needed.
> + if (rc)
> + LOG(ERROR, "%s: Adding/Removing overlay dtb failed.\n", __func__);
> +
> + GC_FREE;
> + return rc;
> +}
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 42+ messages in thread
* [XEN][RFC PATCH v3 14/14] tools/xl: Add new xl command overlay for device tree overlay support
2022-03-08 19:46 [XEN][RFC PATCH v3 00/14] dynamic node programming using overlay dtbo Vikram Garhwal
` (12 preceding siblings ...)
2022-03-08 19:47 ` [XEN][RFC PATCH v3 13/14] tools/libs/light: Implement new libxl functions for device tree overlay ops Vikram Garhwal
@ 2022-03-08 19:47 ` Vikram Garhwal
2022-03-15 12:11 ` Luca Fancellu
2022-03-17 18:18 ` Anthony PERARD
13 siblings, 2 replies; 42+ messages in thread
From: Vikram Garhwal @ 2022-03-08 19:47 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk,
Vikram Garhwal, Wei Liu, Anthony PERARD
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
tools/xl/xl.h | 4 ++++
tools/xl/xl_cmdtable.c | 6 ++++++
tools/xl/xl_vmcontrol.c | 45 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 55 insertions(+)
diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index c5c4bedbdd..604fd5bb94 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -97,6 +97,9 @@ struct save_file_header {
#define SAVEFILE_BYTEORDER_VALUE ((uint32_t)0x01020304UL)
+#define XL_DT_OVERLAY_ADD 1
+#define XL_DT_OVERLAY_REMOVE 2
+
void save_domain_core_begin(uint32_t domid,
int preserve_domid,
const char *override_config_file,
@@ -139,6 +142,7 @@ int main_shutdown(int argc, char **argv);
int main_reboot(int argc, char **argv);
int main_list(int argc, char **argv);
int main_vm_list(int argc, char **argv);
+int main_dt_overlay(int argc, char **argv);
int main_create(int argc, char **argv);
int main_config_update(int argc, char **argv);
int main_button_press(int argc, char **argv);
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 661323d488..5812d19db8 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -20,6 +20,12 @@
#include "xl.h"
const struct cmd_spec cmd_table[] = {
+ { "overlay",
+ &main_dt_overlay, 1, 1,
+ "Add/Remove a device tree overlay",
+ "add/remove <.dtbo>"
+ "-h print this help\n"
+ },
{ "create",
&main_create, 1, 1,
"Create a domain from config file <filename>",
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 435155a033..76b969dc33 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -1262,6 +1262,51 @@ int main_create(int argc, char **argv)
return 0;
}
+int main_dt_overlay(int argc, char **argv)
+{
+ const char *overlay_ops = argv[1];
+ const char *overlay_config_file = argv[2];
+ void *overlay_dtb = NULL;
+ int rc;
+ uint8_t op;
+ int overlay_dtb_size = 0;
+
+ if (overlay_ops == NULL) {
+ fprintf(stderr, "No overlay operation mode provided\n");
+ return ERROR_FAIL;
+ }
+
+ if (strcmp(overlay_ops, "add") == 0)
+ op = XL_DT_OVERLAY_ADD;
+ else if (strcmp(overlay_ops, "remove") == 0)
+ op = XL_DT_OVERLAY_REMOVE;
+ else {
+ fprintf(stderr, "Invalid dt overlay operation\n");
+ return ERROR_FAIL;
+ }
+
+ if (overlay_config_file) {
+ rc = libxl_read_file_contents(ctx, overlay_config_file,
+ &overlay_dtb, &overlay_dtb_size);
+
+ if (rc) {
+ fprintf(stderr, "failed to read the overlay device tree file %s\n",
+ overlay_config_file);
+ free(overlay_dtb);
+ return ERROR_FAIL;
+ }
+ } else {
+ fprintf(stderr, "overlay dtbo file not provided\n");
+ return ERROR_FAIL;
+ }
+
+ rc = libxl_dt_overlay(ctx, overlay_dtb, overlay_dtb_size, op);
+ if (rc)
+ fprintf(stderr, "Overlay operation failed\n");
+
+ free(overlay_dtb);
+ return rc;
+}
/*
* Local variables:
* mode: C
--
2.17.1
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 14/14] tools/xl: Add new xl command overlay for device tree overlay support
2022-03-08 19:47 ` [XEN][RFC PATCH v3 14/14] tools/xl: Add new xl command overlay for device tree overlay support Vikram Garhwal
@ 2022-03-15 12:11 ` Luca Fancellu
2022-03-17 18:18 ` Anthony PERARD
1 sibling, 0 replies; 42+ messages in thread
From: Luca Fancellu @ 2022-03-15 12:11 UTC (permalink / raw)
To: Vikram Garhwal
Cc: Xen-devel, sstabellini, julien, Bertrand Marquis,
volodymyr_babchuk, Wei Liu, Anthony PERARD
> On 8 Mar 2022, at 19:47, Vikram Garhwal <fnu.vikram@xilinx.com> wrote:
>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
> tools/xl/xl.h | 4 ++++
> tools/xl/xl_cmdtable.c | 6 ++++++
> tools/xl/xl_vmcontrol.c | 45 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 55 insertions(+)
>
> diff --git a/tools/xl/xl.h b/tools/xl/xl.h
> index c5c4bedbdd..604fd5bb94 100644
> --- a/tools/xl/xl.h
> +++ b/tools/xl/xl.h
> @@ -97,6 +97,9 @@ struct save_file_header {
>
> #define SAVEFILE_BYTEORDER_VALUE ((uint32_t)0x01020304UL)
>
> +#define XL_DT_OVERLAY_ADD 1
> +#define XL_DT_OVERLAY_REMOVE 2
Maybe you can get rid of them and ...
> +
> void save_domain_core_begin(uint32_t domid,
> int preserve_domid,
> const char *override_config_file,
> @@ -139,6 +142,7 @@ int main_shutdown(int argc, char **argv);
> int main_reboot(int argc, char **argv);
> int main_list(int argc, char **argv);
> int main_vm_list(int argc, char **argv);
> +int main_dt_overlay(int argc, char **argv);
> int main_create(int argc, char **argv);
> int main_config_update(int argc, char **argv);
> int main_button_press(int argc, char **argv);
> diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
> index 661323d488..5812d19db8 100644
> --- a/tools/xl/xl_cmdtable.c
> +++ b/tools/xl/xl_cmdtable.c
> @@ -20,6 +20,12 @@
> #include "xl.h"
>
> const struct cmd_spec cmd_table[] = {
> + { "overlay",
> + &main_dt_overlay, 1, 1,
> + "Add/Remove a device tree overlay",
> + "add/remove <.dtbo>"
> + "-h print this help\n"
> + },
> { "create",
> &main_create, 1, 1,
> "Create a domain from config file <filename>",
> diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
> index 435155a033..76b969dc33 100644
> --- a/tools/xl/xl_vmcontrol.c
> +++ b/tools/xl/xl_vmcontrol.c
> @@ -1262,6 +1262,51 @@ int main_create(int argc, char **argv)
> return 0;
> }
>
> +int main_dt_overlay(int argc, char **argv)
> +{
> + const char *overlay_ops = argv[1];
> + const char *overlay_config_file = argv[2];
> + void *overlay_dtb = NULL;
> + int rc;
> + uint8_t op;
> + int overlay_dtb_size = 0;
> +
> + if (overlay_ops == NULL) {
> + fprintf(stderr, "No overlay operation mode provided\n");
> + return ERROR_FAIL;
> + }
> +
> + if (strcmp(overlay_ops, "add") == 0)
> + op = XL_DT_OVERLAY_ADD;
> + else if (strcmp(overlay_ops, "remove") == 0)
> + op = XL_DT_OVERLAY_REMOVE;
Use them there, maybe defining const int <name> = <value>
Since these values are only used there
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [XEN][RFC PATCH v3 14/14] tools/xl: Add new xl command overlay for device tree overlay support
2022-03-08 19:47 ` [XEN][RFC PATCH v3 14/14] tools/xl: Add new xl command overlay for device tree overlay support Vikram Garhwal
2022-03-15 12:11 ` Luca Fancellu
@ 2022-03-17 18:18 ` Anthony PERARD
1 sibling, 0 replies; 42+ messages in thread
From: Anthony PERARD @ 2022-03-17 18:18 UTC (permalink / raw)
To: Vikram Garhwal
Cc: xen-devel, sstabellini, julien, bertrand.marquis,
volodymyr_babchuk, Wei Liu
On Tue, Mar 08, 2022 at 11:47:04AM -0800, Vikram Garhwal wrote:
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
> tools/xl/xl.h | 4 ++++
> tools/xl/xl_cmdtable.c | 6 ++++++
> tools/xl/xl_vmcontrol.c | 45 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 55 insertions(+)
>
> diff --git a/tools/xl/xl.h b/tools/xl/xl.h
> index c5c4bedbdd..604fd5bb94 100644
> --- a/tools/xl/xl.h
> +++ b/tools/xl/xl.h
> @@ -97,6 +97,9 @@ struct save_file_header {
>
> #define SAVEFILE_BYTEORDER_VALUE ((uint32_t)0x01020304UL)
>
> +#define XL_DT_OVERLAY_ADD 1
> +#define XL_DT_OVERLAY_REMOVE 2
These value would need to be part of libxl's API, rather than been
defined here. Could you create a new enum with both operation in
"libxl_types.idl", then have the libxl function convert them to the
hypercall operation? (So to be done in the previous patch.)
> void save_domain_core_begin(uint32_t domid,
> int preserve_domid,
> const char *override_config_file,
> @@ -139,6 +142,7 @@ int main_shutdown(int argc, char **argv);
> int main_reboot(int argc, char **argv);
> int main_list(int argc, char **argv);
> int main_vm_list(int argc, char **argv);
> +int main_dt_overlay(int argc, char **argv);
> int main_create(int argc, char **argv);
> int main_config_update(int argc, char **argv);
> int main_button_press(int argc, char **argv);
> diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
> index 661323d488..5812d19db8 100644
> --- a/tools/xl/xl_cmdtable.c
> +++ b/tools/xl/xl_cmdtable.c
> @@ -20,6 +20,12 @@
> #include "xl.h"
>
> const struct cmd_spec cmd_table[] = {
> + { "overlay",
> + &main_dt_overlay, 1, 1,
I think the first "1" needs to be a "0", because it doesn't seems that
the command can do a "dry-run".
> + "Add/Remove a device tree overlay",
> + "add/remove <.dtbo>"
> + "-h print this help\n"
> + },
I don't think "overlay" is a good name for the command. Maybe
"dt-overlay" ? But we seem to mostly have "*-add" "*-remove" command (or
attach/detach), so maybe two new commands would be better:
"dt-overlay-add" and "dt-overlay-remove" rather than using a subcommand
for add/remove.
Also, could you add this/those commands later in "cmd_table"? I'd rather
keep "create" first when running `xl help`. So maybe at the end, or
some other place that kind of make sens?
> { "create",
> &main_create, 1, 1,
> "Create a domain from config file <filename>",
> diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
> index 435155a033..76b969dc33 100644
> --- a/tools/xl/xl_vmcontrol.c
> +++ b/tools/xl/xl_vmcontrol.c
> @@ -1262,6 +1262,51 @@ int main_create(int argc, char **argv)
> return 0;
> }
>
> +int main_dt_overlay(int argc, char **argv)
> +{
> + const char *overlay_ops = argv[1];
> + const char *overlay_config_file = argv[2];
> + void *overlay_dtb = NULL;
> + int rc;
> + uint8_t op;
> + int overlay_dtb_size = 0;
> +
> + if (overlay_ops == NULL) {
> + fprintf(stderr, "No overlay operation mode provided\n");
> + return ERROR_FAIL;
> + }
> +
> + if (strcmp(overlay_ops, "add") == 0)
> + op = XL_DT_OVERLAY_ADD;
> + else if (strcmp(overlay_ops, "remove") == 0)
> + op = XL_DT_OVERLAY_REMOVE;
> + else {
> + fprintf(stderr, "Invalid dt overlay operation\n");
> + return ERROR_FAIL;
> + }
> +
> + if (overlay_config_file) {
> + rc = libxl_read_file_contents(ctx, overlay_config_file,
> + &overlay_dtb, &overlay_dtb_size);
> +
> + if (rc) {
> + fprintf(stderr, "failed to read the overlay device tree file %s\n",
> + overlay_config_file);
> + free(overlay_dtb);
> + return ERROR_FAIL;
> + }
> + } else {
> + fprintf(stderr, "overlay dtbo file not provided\n");
Instead of making out of bound access to "argv", you could check that
"argc" have the expected value, and if not, print the help of the
command. If you look at main_save() for example, there is: "if(argc is
wrong value){help("save");} which would print the help for the
command.
> + return ERROR_FAIL;
> + }
> +
> + rc = libxl_dt_overlay(ctx, overlay_dtb, overlay_dtb_size, op);
> + if (rc)
> + fprintf(stderr, "Overlay operation failed\n");
I'm not sure that this error message would be useful, as libxl should
already have printed something.
> +
> + free(overlay_dtb);
> + return rc;
> +}
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 42+ messages in thread