* [XEN][RFC PATCH v2 01/12] device tree: Remove __init from function type
2021-11-09 7:02 [XEN][RFC PATCH v2 00/12] dynamic node programming using overlay dtbo Vikram Garhwal
@ 2021-11-09 7:02 ` Vikram Garhwal
2021-11-09 7:02 ` [XEN][RFC PATCH v2 02/12] xen: arm: Add CONFIG_OVERLAY_DTB Vikram Garhwal
` (10 subsequent siblings)
11 siblings, 0 replies; 28+ messages in thread
From: Vikram Garhwal @ 2021-11-09 7:02 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.
Move map_range_data declaration to domain_build.h.
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 | 146 ++++++++++++++++++++++++++++++++++++
xen/arch/arm/domain_build.c | 150 -------------------------------------
xen/common/device_tree.c | 18 ++---
xen/include/asm-arm/domain_build.h | 10 +++
xen/include/xen/device_tree.h | 5 ++
5 files changed, 170 insertions(+), 159 deletions(-)
diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 70cd6c1..c7cf2b9 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -21,6 +21,8 @@
#include <xen/errno.h>
#include <xen/init.h>
#include <xen/lib.h>
+#include <xen/iocap.h>
+#include <asm/domain_build.h>
extern const struct device_desc _sdevice[], _edevice[];
extern const struct acpi_device_desc _asdevice[], _aedevice[];
@@ -84,6 +86,150 @@ 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;
+ bool need_mapping = !dt_device_for_passthrough(dev);
+ 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 ( need_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 0167731..3e8f6da 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -51,12 +51,6 @@ static int __init parse_dom0_mem(const char *s)
}
custom_param("dom0_mem", parse_dom0_mem);
-struct map_range_data
-{
- struct domain *d;
- p2m_type_t p2mt;
-};
-
/* Override macros from asm/page.h to make them work with mfn_t */
#undef virt_to_mfn
#define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
@@ -1624,41 +1618,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)
@@ -1690,59 +1649,6 @@ static int __init map_dt_irq_to_domain(const struct dt_device_node *dev,
return 0;
}
-static 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;
- bool need_mapping = !dt_device_for_passthrough(dev);
- 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 ( need_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
@@ -1773,62 +1679,6 @@ static int __init map_device_children(struct domain *d,
}
/*
- * 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
* - Retrieve the IRQ configuration (i.e edge/level) from device tree
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 92ce59e..88f3f7e 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -1821,12 +1821,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;
@@ -2057,7 +2057,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"
@@ -2066,7 +2066,7 @@ 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,
+void unflatten_device_tree(const void *fdt,
struct dt_device_node **mynodes)
{
unsigned long start, mem, size;
@@ -2189,7 +2189,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/asm-arm/domain_build.h b/xen/include/asm-arm/domain_build.h
index 34ceddc..17449b1 100644
--- a/xen/include/asm-arm/domain_build.h
+++ b/xen/include/asm-arm/domain_build.h
@@ -4,10 +4,20 @@
#include <xen/sched.h>
#include <asm/kernel.h>
+struct map_range_data
+{
+ struct domain *d;
+ p2m_type_t p2mt;
+};
+
int map_irq_to_domain(struct domain *d, unsigned int irq,
bool need_mapping, const char *devname);
int make_chosen_node(const struct kernel_info *kinfo);
void evtchn_allocate(struct domain *d);
+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);
#ifndef CONFIG_ACPI
static inline int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 573e4a0..9fc63ab 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.7.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* [XEN][RFC PATCH v2 02/12] xen: arm: Add CONFIG_OVERLAY_DTB
2021-11-09 7:02 [XEN][RFC PATCH v2 00/12] dynamic node programming using overlay dtbo Vikram Garhwal
2021-11-09 7:02 ` [XEN][RFC PATCH v2 01/12] device tree: Remove __init from function type Vikram Garhwal
@ 2021-11-09 7:02 ` Vikram Garhwal
2021-12-22 14:02 ` Julien Grall
2021-11-09 7:02 ` [XEN][RFC PATCH v2 03/12] libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB Vikram Garhwal
` (9 subsequent siblings)
11 siblings, 1 reply; 28+ messages in thread
From: Vikram Garhwal @ 2021-11-09 7:02 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk,
Vikram Garhwal, Volodymyr Babchuk
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
xen/arch/arm/Kconfig | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index ecfa682..895f0ee 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -46,6 +46,14 @@ config HAS_ITS
bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED
depends on GICV3 && !NEW_VGIC
+config OVERLAY_DTB
+ depends on MPSOC_PLATFORM
+ bool "Enable DTB overlay"
+ default y
+ ---help---
+
+ Allows dynamic addition/removal of Xen device tree nodes using a dtbo.
+
config HVM
def_bool y
--
2.7.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [XEN][RFC PATCH v2 02/12] xen: arm: Add CONFIG_OVERLAY_DTB
2021-11-09 7:02 ` [XEN][RFC PATCH v2 02/12] xen: arm: Add CONFIG_OVERLAY_DTB Vikram Garhwal
@ 2021-12-22 14:02 ` Julien Grall
0 siblings, 0 replies; 28+ messages in thread
From: Julien Grall @ 2021-12-22 14:02 UTC (permalink / raw)
To: Vikram Garhwal, xen-devel
Cc: sstabellini, bertrand.marquis, volodymyr_babchuk
Hi,
Please add a short commit message explaning how this is going to be used.
On 09/11/2021 08:02, Vikram Garhwal wrote:
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
> xen/arch/arm/Kconfig | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index ecfa682..895f0ee 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -46,6 +46,14 @@ config HAS_ITS
> bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED
> depends on GICV3 && !NEW_VGIC
>
> +config OVERLAY_DTB
> + depends on MPSOC_PLATFORM
None of the code looks to be Xilinx specific and this will prevent one
to build a generic Xen and test your code. So can you remove the dependency?
> + bool "Enable DTB overlay"
> + default y
In general, for new feature, we want to disable them by default and gate
them with UNSUPPORTED.
They can move off UNSUPPORTED once they are ready enough to be supported
(and possibly security supported).
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread
* [XEN][RFC PATCH v2 03/12] libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB.
2021-11-09 7:02 [XEN][RFC PATCH v2 00/12] dynamic node programming using overlay dtbo Vikram Garhwal
2021-11-09 7:02 ` [XEN][RFC PATCH v2 01/12] device tree: Remove __init from function type Vikram Garhwal
2021-11-09 7:02 ` [XEN][RFC PATCH v2 02/12] xen: arm: Add CONFIG_OVERLAY_DTB Vikram Garhwal
@ 2021-11-09 7:02 ` Vikram Garhwal
2021-11-09 11:10 ` Jan Beulich
2021-12-22 14:03 ` Julien Grall
2021-11-09 7:02 ` [XEN][RFC PATCH v2 04/12] libfdt: Add fdt_ prefix to overlay_get_target() Vikram Garhwal
` (8 subsequent siblings)
11 siblings, 2 replies; 28+ messages in thread
From: Vikram Garhwal @ 2021-11-09 7:02 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.
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
xen/common/libfdt/Makefile | 3 +++
1 file changed, 3 insertions(+)
diff --git a/xen/common/libfdt/Makefile b/xen/common/libfdt/Makefile
index 6bd207c..f838f5f 100644
--- a/xen/common/libfdt/Makefile
+++ b/xen/common/libfdt/Makefile
@@ -1,7 +1,10 @@
include Makefile.libfdt
SECTIONS := text data $(SPECIAL_DATA_SECTIONS)
+
+ifneq ($(CONFIG_OVERLAY_DTB),y)
OBJCOPYFLAGS := $(foreach s,$(SECTIONS),--rename-section .$(s)=.init.$(s))
+endif
obj-y += libfdt.o
nocov-y += libfdt.o
--
2.7.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [XEN][RFC PATCH v2 03/12] libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB.
2021-11-09 7:02 ` [XEN][RFC PATCH v2 03/12] libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB Vikram Garhwal
@ 2021-11-09 11:10 ` Jan Beulich
2021-12-22 14:03 ` Julien Grall
1 sibling, 0 replies; 28+ messages in thread
From: Jan Beulich @ 2021-11-09 11:10 UTC (permalink / raw)
To: Vikram Garhwal
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk, xen-devel
On 09.11.2021 08:02, 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.
>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
> xen/common/libfdt/Makefile | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/xen/common/libfdt/Makefile b/xen/common/libfdt/Makefile
> index 6bd207c..f838f5f 100644
> --- a/xen/common/libfdt/Makefile
> +++ b/xen/common/libfdt/Makefile
> @@ -1,7 +1,10 @@
> include Makefile.libfdt
>
> SECTIONS := text data $(SPECIAL_DATA_SECTIONS)
> +
> +ifneq ($(CONFIG_OVERLAY_DTB),y)
> OBJCOPYFLAGS := $(foreach s,$(SECTIONS),--rename-section .$(s)=.init.$(s))
> +endif
I think you'd better suppress the objcopy step altogether in that case.
Jan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN][RFC PATCH v2 03/12] libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB.
2021-11-09 7:02 ` [XEN][RFC PATCH v2 03/12] libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB Vikram Garhwal
2021-11-09 11:10 ` Jan Beulich
@ 2021-12-22 14:03 ` Julien Grall
1 sibling, 0 replies; 28+ messages in thread
From: Julien Grall @ 2021-12-22 14:03 UTC (permalink / raw)
To: Vikram Garhwal, xen-devel
Cc: sstabellini, bertrand.marquis, volodymyr_babchuk
Hi Vikram,
On 09/11/2021 08:02, 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.
>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
> xen/common/libfdt/Makefile | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/xen/common/libfdt/Makefile b/xen/common/libfdt/Makefile
> index 6bd207c..f838f5f 100644
> --- a/xen/common/libfdt/Makefile
> +++ b/xen/common/libfdt/Makefile
> @@ -1,7 +1,10 @@
> include Makefile.libfdt
>
> SECTIONS := text data $(SPECIAL_DATA_SECTIONS)
> +
NIT: I would add a short comment explaining why we need to keep after init.
> +ifneq ($(CONFIG_OVERLAY_DTB),y)
> OBJCOPYFLAGS := $(foreach s,$(SECTIONS),--rename-section .$(s)=.init.$(s))
> +endif
Acked-by: Julien Grall <jgrall@amazon.com>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread
* [XEN][RFC PATCH v2 04/12] libfdt: Add fdt_ prefix to overlay_get_target()
2021-11-09 7:02 [XEN][RFC PATCH v2 00/12] dynamic node programming using overlay dtbo Vikram Garhwal
` (2 preceding siblings ...)
2021-11-09 7:02 ` [XEN][RFC PATCH v2 03/12] libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB Vikram Garhwal
@ 2021-11-09 7:02 ` Vikram Garhwal
2021-12-22 14:07 ` Julien Grall
2021-11-09 7:02 ` [XEN][RFC PATCH v2 05/12] device tree: Add _dt_find_node_by_path() to find nodes in device tree Vikram Garhwal
` (7 subsequent siblings)
11 siblings, 1 reply; 28+ messages in thread
From: Vikram Garhwal @ 2021-11-09 7:02 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk, Vikram Garhwal
Add fdt_ prefix to overlay_get_target() and remove static type. This is done to
get the target path for all the overlay nodes. This is useful to find which
nodes are to be added/removed in dt_host.
Also, sending this patch to dtc mailing list to avoid the divergence.
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
xen/common/libfdt/fdt_overlay.c | 12 ++++++------
xen/include/xen/libfdt/libfdt.h | 3 +++
2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/xen/common/libfdt/fdt_overlay.c b/xen/common/libfdt/fdt_overlay.c
index 7b95e2b..194f51b 100644
--- a/xen/common/libfdt/fdt_overlay.c
+++ b/xen/common/libfdt/fdt_overlay.c
@@ -42,13 +42,13 @@ static uint32_t overlay_get_target_phandle(const void *fdto, int fragment)
}
/**
- * overlay_get_target - retrieves the offset of a fragment's target
+ * fdt_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
+ * fdt_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)
*
@@ -56,7 +56,7 @@ static uint32_t overlay_get_target_phandle(const void *fdto, int fragment)
* 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 fdt_overlay_get_target(const void *fdt, const void *fdto,
int fragment, char const **pathp)
{
uint32_t phandle;
@@ -638,7 +638,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_get_target(fdt, fdto, fragment, NULL);
if (target < 0)
return target;
@@ -781,7 +781,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_get_target(fdt, fdto, fragment, &target_path);
if (ret < 0)
return ret;
target = ret;
@@ -803,7 +803,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_get_target(fdt, fdto, fragment, &target_path);
if (ret < 0)
return ret;
target = ret;
diff --git a/xen/include/xen/libfdt/libfdt.h b/xen/include/xen/libfdt/libfdt.h
index c71689e..1f549d0 100644
--- a/xen/include/xen/libfdt/libfdt.h
+++ b/xen/include/xen/libfdt/libfdt.h
@@ -2115,6 +2115,9 @@ int fdt_overlay_apply(void *fdt, void *fdto);
const char *fdt_strerror(int errval);
+int fdt_overlay_get_target(const void *fdt, const void *fdto, int fragment,
+ char const **pathp);
+
#ifdef __cplusplus
}
#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* [XEN][RFC PATCH v2 05/12] device tree: Add _dt_find_node_by_path() to find nodes in device tree
2021-11-09 7:02 [XEN][RFC PATCH v2 00/12] dynamic node programming using overlay dtbo Vikram Garhwal
` (3 preceding siblings ...)
2021-11-09 7:02 ` [XEN][RFC PATCH v2 04/12] libfdt: Add fdt_ prefix to overlay_get_target() Vikram Garhwal
@ 2021-11-09 7:02 ` Vikram Garhwal
2021-11-09 7:02 ` [XEN][RFC PATCH v2 06/12] xen/smmu: Add remove_device callback for smmu_iommu ops Vikram Garhwal
` (6 subsequent siblings)
11 siblings, 0 replies; 28+ messages in thread
From: Vikram Garhwal @ 2021-11-09 7:02 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 88f3f7e..26d2e28 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);
+}
+
void dt_print_node_names(struct dt_device_node *dt)
{
struct dt_device_node *np;
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 9fc63ab..5ba26a0 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.7.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* [XEN][RFC PATCH v2 06/12] xen/smmu: Add remove_device callback for smmu_iommu ops
2021-11-09 7:02 [XEN][RFC PATCH v2 00/12] dynamic node programming using overlay dtbo Vikram Garhwal
` (4 preceding siblings ...)
2021-11-09 7:02 ` [XEN][RFC PATCH v2 05/12] device tree: Add _dt_find_node_by_path() to find nodes in device tree Vikram Garhwal
@ 2021-11-09 7:02 ` Vikram Garhwal
2021-12-22 14:31 ` Julien Grall
2021-11-09 7:02 ` [XEN][RFC PATCH v2 07/12] " Vikram Garhwal
` (5 subsequent siblings)
11 siblings, 1 reply; 28+ messages in thread
From: Vikram Garhwal @ 2021-11-09 7:02 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 | 54 ++++++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index c9dfc4c..1a32e2c 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -816,6 +816,17 @@ 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))
+ 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 +864,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 +913,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;
@@ -2876,6 +2929,7 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
.init = arm_smmu_iommu_domain_init,
.hwdom_init = arm_smmu_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.7.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [XEN][RFC PATCH v2 06/12] xen/smmu: Add remove_device callback for smmu_iommu ops
2021-11-09 7:02 ` [XEN][RFC PATCH v2 06/12] xen/smmu: Add remove_device callback for smmu_iommu ops Vikram Garhwal
@ 2021-12-22 14:31 ` Julien Grall
0 siblings, 0 replies; 28+ messages in thread
From: Julien Grall @ 2021-12-22 14:31 UTC (permalink / raw)
To: Vikram Garhwal, xen-devel
Cc: sstabellini, bertrand.marquis, volodymyr_babchuk
Hi,
On 09/11/2021 08:02, Vikram Garhwal wrote:
> 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 | 54 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 54 insertions(+)
>
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index c9dfc4c..1a32e2c 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -816,6 +816,17 @@ 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))
Coding style: The inner () are not necessary.
Also, is this check only there for code hardening purpose? If yes, then
I would add an ASSERT_UNREACHABLE() in the if body to allow catching the
error in debug build.
> + 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 +864,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;
> + }
Even if the IOMMU subsystem check it, it would be good that the SMMU
driver also check the device is not currently used before removing it.
If it is, then we should return -EBUSY.
> +
> + ret = remove_smmu_master(smmu, master);
> +
> + if (ret)
> + return ret;
> +
> + dev_node->is_protected = false;
Coding style: The indentation looks off compare to the other line.
> +
> + kfree(master);
> + return 0;
> +}
> +
> static int register_smmu_master(struct arm_smmu_device *smmu,
> struct device *dev,
> struct of_phandle_args *masterspec)
> @@ -876,6 +913,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;
> @@ -2876,6 +2929,7 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
> .init = arm_smmu_iommu_domain_init,
> .hwdom_init = arm_smmu_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,
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread
* [XEN][RFC PATCH v2 07/12] xen/smmu: Add remove_device callback for smmu_iommu ops
2021-11-09 7:02 [XEN][RFC PATCH v2 00/12] dynamic node programming using overlay dtbo Vikram Garhwal
` (5 preceding siblings ...)
2021-11-09 7:02 ` [XEN][RFC PATCH v2 06/12] xen/smmu: Add remove_device callback for smmu_iommu ops Vikram Garhwal
@ 2021-11-09 7:02 ` Vikram Garhwal
2021-12-22 14:38 ` Julien Grall
2021-11-09 7:02 ` [XEN][RFC PATCH v2 08/12] xen/arm: Implement device tree node removal functionalities Vikram Garhwal
` (4 subsequent siblings)
11 siblings, 1 reply; 28+ messages in thread
From: Vikram Garhwal @ 2021-11-09 7:02 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk,
Vikram Garhwal, Jan Beulich, Paul Durrant
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/device_tree.c | 30 ++++++++++++++++++++++++++++++
xen/include/xen/iommu.h | 2 ++
2 files changed, 32 insertions(+)
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 98f2aa0..9d9eed8 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -127,6 +127,36 @@ 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;
+
+ if ( iommu_dt_device_is_assigned(np) )
+ return -EPERM;
+
+ /*
+ * The driver which supports generic IOMMU DT bindings must have
+ * these callback implemented.
+ */
+ if ( !ops->remove_device )
+ return -EOPNOTSUPP;
+
+ /*
+ * 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);
+
+ 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 6b2cdff..c4d5d12 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.7.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [XEN][RFC PATCH v2 07/12] xen/smmu: Add remove_device callback for smmu_iommu ops
2021-11-09 7:02 ` [XEN][RFC PATCH v2 07/12] " Vikram Garhwal
@ 2021-12-22 14:38 ` Julien Grall
0 siblings, 0 replies; 28+ messages in thread
From: Julien Grall @ 2021-12-22 14:38 UTC (permalink / raw)
To: Vikram Garhwal, xen-devel
Cc: sstabellini, bertrand.marquis, volodymyr_babchuk, Jan Beulich,
Paul Durrant
Hi,
On 09/11/2021 08:02, Vikram Garhwal wrote:
> 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
The commit title and message doesn't match the code.
>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
> xen/drivers/passthrough/device_tree.c | 30 ++++++++++++++++++++++++++++++
> xen/include/xen/iommu.h | 2 ++
> 2 files changed, 32 insertions(+)
>
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 98f2aa0..9d9eed8 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -127,6 +127,36 @@ 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;
> +
> + if ( iommu_dt_device_is_assigned(np) )
> + return -EPERM;
EPERM means the caller doesn't have the permission to request it.
However, dom0 will have the permission to remote the device. The problem
is the device is currently assigned to a domain. So it would be better
to return EBUSY.
Also, most of the function wants to be protected with dtdevs_lock to
prevent concurrent access to add/remove/assign/deassign.
I would create a version of iommu_dt_device_is_assigned() (maybe called
iommu_dt_device_is_assigned_locked()) that would do the same but with
the caller hold the lock.
> +
> + /*
> + * The driver which supports generic IOMMU DT bindings must have
> + * these callback implemented.
> + */
> + if ( !ops->remove_device )
> + return -EOPNOTSUPP;
> +
> + /*
> + * 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);
> +
> + 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 6b2cdff..c4d5d12 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));
>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread
* [XEN][RFC PATCH v2 08/12] xen/arm: Implement device tree node removal functionalities
2021-11-09 7:02 [XEN][RFC PATCH v2 00/12] dynamic node programming using overlay dtbo Vikram Garhwal
` (6 preceding siblings ...)
2021-11-09 7:02 ` [XEN][RFC PATCH v2 07/12] " Vikram Garhwal
@ 2021-11-09 7:02 ` Vikram Garhwal
2021-11-09 11:16 ` Jan Beulich
2021-12-22 16:13 ` Julien Grall
2021-11-09 7:02 ` [XEN][RFC PATCH v2 09/12] xen/arm: Implement device tree node addition functionalities Vikram Garhwal
` (3 subsequent siblings)
11 siblings, 2 replies; 28+ messages in thread
From: Vikram Garhwal @ 2021-11-09 7:02 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk,
Vikram Garhwal, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Wei Liu
Introduce sysctl XEN_SYSCTL_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 domus. If
even one of the node in dtbo is not available for removal i.e. either not
there in dt_host or currently used by any domain, in that case we don't
remove any node in the given dtbo.
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 node. 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/device_tree.c | 53 ++++++
xen/common/sysctl.c | 372 ++++++++++++++++++++++++++++++++++++++++++
xen/include/public/sysctl.h | 23 +++
xen/include/xen/device_tree.h | 4 +
4 files changed, 452 insertions(+)
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 26d2e28..19320e1 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -385,6 +385,59 @@ void dt_print_node_names(struct dt_device_node *dt)
return;
}
+#if defined (CONFIG_OVERLAY_DTB)
+int 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 ( np->name == device_node->name )
+ {
+ current_node->allnext = np->next;
+ return 0;
+ }
+
+ for ( np = parent_node->child; np->sibling != NULL; np = np->sibling )
+ {
+ current_node = np;
+ if ( np->sibling->name == device_node->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;
+}
+#endif
+
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/common/sysctl.c b/xen/common/sysctl.c
index f2dab72..fca47f5 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -28,6 +28,311 @@
#include <xen/livepatch.h>
#include <xen/coverage.h>
+#if defined (CONFIG_OVERLAY_DTB)
+#include <xen/list.h>
+#include <xen/libfdt/libfdt.h>
+#include <xen/xmalloc.h>
+#include <xen/device_tree.h>
+#include <asm/domain_build.h>
+#endif
+
+#if defined (CONFIG_OVERLAY_DTB)
+static LIST_HEAD(overlay_tracker);
+static DEFINE_SPINLOCK(overlay_lock);
+
+/*
+ * overlay_node_track describes information about added nodes through dtbo.
+ * @dt_host_new: Pointer to the updated dt_host_new unflattened 'updated fdt'.
+ * @node_fullname: Store the name of nodes.
+ * @entry: List pointer.
+ */
+struct overlay_track {
+ struct list_head entry;
+ struct dt_device_node *dt_host_new;
+ char **node_fullname;
+ uint8_t num_nodes;
+};
+
+/* Basic sanity check for the dtbo tool stack provided to Xen. */
+static int check_overlay_fdt(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 int overlay_node_count(void *fdto)
+{
+ 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__");
+
+ fdt_for_each_subnode(subnode, fdto, overlay)
+ {
+ num_overlay_nodes++;
+ }
+ }
+
+ return num_overlay_nodes;
+}
+
+/*
+ * overlay_get_node_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 void overlay_get_node_info(void *fdto, char ***node_full_path,
+ int num_overlay_nodes)
+{
+ int fragment;
+ int node_num = 0;
+
+ *node_full_path = xmalloc_bytes(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_get_target(device_tree_flattened, fdto, fragment,
+ &target_path);
+ overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
+
+ fdt_for_each_subnode(subnode, fdto, overlay)
+ {
+ const char *node_name = fdt_get_name(fdto, subnode, NULL);
+ int node_name_len = strlen(node_name);
+ int target_path_len = strlen(target_path);
+ int node_full_name_len = target_path_len + node_name_len + 2;
+
+ (*node_full_path)[node_num] = xmalloc_bytes(node_full_name_len);
+
+ memcpy((*node_full_path)[node_num], target_path, target_path_len);
+
+ (*node_full_path)[node_num][target_path_len] = '/';
+
+ memcpy((*node_full_path)[node_num] + target_path_len + 1, node_name,
+ node_name_len);
+
+ (*node_full_path)[node_num][node_full_name_len - 1] = '\0';
+
+ node_num++;
+ }
+ }
+}
+
+/*
+ * Checks if all the devices node listed are present in dt_host and used by any
+ * domain.
+ */
+static int check_nodes(char **full_dt_node_path, uint32_t num_nodes)
+{
+ int rc = 0;
+ unsigned int i;
+ struct dt_device_node *overlay_node;
+ uint32_t ret = 0;
+
+ for ( i = 0; i < num_nodes; i++ ) {
+ dt_dprintk("Finding node %s in the dt_host\n", full_dt_node_path[i]);
+
+ overlay_node = dt_find_node_by_path(full_dt_node_path[i]);
+
+ if ( overlay_node == NULL )
+ {
+ rc = -EINVAL;
+
+ printk(XENLOG_G_ERR "Device %s is not present in the tree."
+ " Removing nodes failed\n", full_dt_node_path[i]);
+ return rc;
+ }
+
+ ret = dt_device_used_by(overlay_node);
+
+ dt_dprintk("Checking if node %s is used by any domain\n",
+ full_dt_node_path[i]);
+
+ if ( ret != 0 && ret != DOMID_IO )
+ {
+ rc = -EINVAL;
+
+ printk(XENLOG_G_ERR "Device %s as it is being used by domain %d."
+ " Removing nodes failed\n", full_dt_node_path[i], ret);
+ return rc;
+ }
+ }
+
+ return rc;
+}
+
+/* Remove nodes from dt_host. */
+static int remove_nodes(char **full_dt_node_path, uint32_t num_nodes)
+{
+ struct domain *d = hardware_domain;
+ int rc = 0;
+ struct dt_device_node *overlay_node;
+ unsigned int naddr;
+ unsigned int i, j, nirq;
+ struct dt_raw_irq rirq;
+ u64 addr, size;
+
+ for ( j = 0; j < num_nodes; j++ ) {
+ dt_dprintk("Removing node: %s\n", full_dt_node_path[j]);
+
+ overlay_node = dt_find_node_by_path(full_dt_node_path[j]);
+
+ nirq = dt_number_of_irq(overlay_node);
+
+ /* Remove IRQ permission */
+ for ( i = 0; i < nirq; i++ )
+ {
+ rc = dt_device_get_raw_irq(overlay_node, i, &rirq);
+ if ( rc )
+ {
+ printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
+ i, dt_node_full_name(overlay_node));
+ return rc;
+ }
+
+ rc = platform_get_irq(overlay_node, i);
+ if ( rc < 0 )
+ {
+ printk(XENLOG_ERR "Unable to get irq %u for %s\n",
+ i, dt_node_full_name(overlay_node));
+ return rc;
+ }
+
+ 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 = 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_node_path,
+ uint32_t 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.
+ */
+ 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_node_path[i], entry->node_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_G_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 = check_nodes(full_dt_node_path, num_nodes);
+
+ if ( rc )
+ goto out;
+
+ rc = remove_nodes(full_dt_node_path, num_nodes);
+
+ if ( rc ) {
+ printk(XENLOG_G_ERR "Removing node failed\n");
+ goto out;
+ }
+
+ list_del(&entry->entry);
+ xfree(entry->node_fullname);
+ xfree(entry->dt_host_new);
+ xfree(entry);
+
+out:
+ spin_unlock(&overlay_lock);
+ return rc;
+}
+#endif
+
long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
{
long ret = 0;
@@ -476,6 +781,73 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
copyback = 1;
break;
+#if defined (CONFIG_OVERLAY_DTB)
+ case XEN_SYSCTL_overlay:
+ {
+ void *overlay_fdt;
+ char **node_full_path = NULL;
+ int num_overlay_nodes;
+
+ if ( op->u.overlay_dt.overlay_fdt_size > 0 )
+ overlay_fdt = xmalloc_bytes(op->u.overlay_dt.overlay_fdt_size);
+ else
+ {
+ ret = -EINVAL;
+ break;
+ }
+
+ if ( overlay_fdt == NULL )
+ {
+ ret = -ENOMEM;
+ break;
+ }
+
+ ret = copy_from_guest(overlay_fdt, op->u.overlay_dt.overlay_fdt,
+ op->u.overlay_dt.overlay_fdt_size);
+ if ( ret )
+ {
+ gprintk(XENLOG_ERR, "copy from guest failed\n");
+ xfree(overlay_fdt);
+
+ ret = -EFAULT;
+ break;
+ }
+
+ if ( op->u.overlay_dt.overlay_op == XEN_SYSCTL_DT_OVERLAY_ADD )
+ {
+ ret = handle_add_overlay_nodes(overlay_fdt,
+ op->u.overlay_dt.overlay_fdt_size);
+ } else if ( op->u.overlay_dt.overlay_op ==
+ XEN_SYSCTL_DT_OVERLAY_REMOVE )
+ {
+ ret = check_overlay_fdt(overlay_fdt,
+ op->u.overlay_dt.overlay_fdt_size);
+ if ( ret )
+ {
+ ret = -EFAULT;
+ break;
+ }
+
+ num_overlay_nodes = overlay_node_count(overlay_fdt);
+ if ( num_overlay_nodes == 0 )
+ {
+ ret = -ENOMEM;
+ break;
+ }
+
+ overlay_get_node_info(overlay_fdt, &node_full_path,
+ num_overlay_nodes);
+
+ ret = handle_remove_overlay_nodes(node_full_path,
+ num_overlay_nodes);
+ }
+
+ xfree(node_full_path);
+
+ 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 3e53681..6624724 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1065,6 +1065,25 @@ typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
#endif
+#if defined (CONFIG_OVERLAY_DTB)
+#define XEN_SYSCTL_DT_OVERLAY_ADD 1
+#define XEN_SYSCTL_DT_OVERLAY_REMOVE 2
+
+/*
+ * XEN_SYSCTL_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_overlay_dt {
+ XEN_GUEST_HANDLE_64(void) overlay_fdt;
+ uint32_t overlay_fdt_size; /* Overlay dtb size. */
+ uint8_t overlay_op; /* Add or remove. */
+};
+#endif
+
struct xen_sysctl {
uint32_t cmd;
#define XEN_SYSCTL_readconsole 1
@@ -1095,6 +1114,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_overlay 30
uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
union {
struct xen_sysctl_readconsole readconsole;
@@ -1125,6 +1145,9 @@ struct xen_sysctl {
#if defined(__i386__) || defined(__x86_64__)
struct xen_sysctl_cpu_policy cpu_policy;
#endif
+#if defined (CONFIG_OVERLAY_DTB)
+ struct xen_sysctl_overlay_dt overlay_dt;
+#endif
uint8_t pad[128];
} u;
};
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 5ba26a0..cf29cf5 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -553,6 +553,10 @@ int dt_find_node_by_gpath(XEN_GUEST_HANDLE(char) u_path, uint32_t u_plen,
*/
void dt_print_node_names(struct dt_device_node *dt);
+#if defined (CONFIG_OVERLAY_DTB)
+int overlay_remove_node(struct dt_device_node *device_node);
+#endif
+
/**
* dt_get_parent - Get a node's parent if any
* @node: Node to get parent
--
2.7.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [XEN][RFC PATCH v2 08/12] xen/arm: Implement device tree node removal functionalities
2021-11-09 7:02 ` [XEN][RFC PATCH v2 08/12] xen/arm: Implement device tree node removal functionalities Vikram Garhwal
@ 2021-11-09 11:16 ` Jan Beulich
2021-12-22 16:13 ` Julien Grall
1 sibling, 0 replies; 28+ messages in thread
From: Jan Beulich @ 2021-11-09 11:16 UTC (permalink / raw)
To: Vikram Garhwal
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk,
Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu, xen-devel
On 09.11.2021 08:02, Vikram Garhwal wrote:
> Introduce sysctl XEN_SYSCTL_overlay to remove device-tree nodes added using
> device tree overlay.
XEN_SYSCTL_overlay is too generic a name imo.
> @@ -476,6 +781,73 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> copyback = 1;
> break;
>
> +#if defined (CONFIG_OVERLAY_DTB)
> + case XEN_SYSCTL_overlay:
> + {
> + void *overlay_fdt;
> + char **node_full_path = NULL;
> + int num_overlay_nodes;
> +
> + if ( op->u.overlay_dt.overlay_fdt_size > 0 )
> + overlay_fdt = xmalloc_bytes(op->u.overlay_dt.overlay_fdt_size);
> + else
> + {
> + ret = -EINVAL;
> + break;
> + }
> +
> + if ( overlay_fdt == NULL )
> + {
> + ret = -ENOMEM;
> + break;
> + }
> +
> + ret = copy_from_guest(overlay_fdt, op->u.overlay_dt.overlay_fdt,
> + op->u.overlay_dt.overlay_fdt_size);
> + if ( ret )
> + {
> + gprintk(XENLOG_ERR, "copy from guest failed\n");
> + xfree(overlay_fdt);
> +
> + ret = -EFAULT;
> + break;
> + }
> +
> + if ( op->u.overlay_dt.overlay_op == XEN_SYSCTL_DT_OVERLAY_ADD )
> + {
> + ret = handle_add_overlay_nodes(overlay_fdt,
> + op->u.overlay_dt.overlay_fdt_size);
> + } else if ( op->u.overlay_dt.overlay_op ==
> + XEN_SYSCTL_DT_OVERLAY_REMOVE )
> + {
> + ret = check_overlay_fdt(overlay_fdt,
> + op->u.overlay_dt.overlay_fdt_size);
> + if ( ret )
> + {
> + ret = -EFAULT;
> + break;
> + }
> +
> + num_overlay_nodes = overlay_node_count(overlay_fdt);
> + if ( num_overlay_nodes == 0 )
> + {
> + ret = -ENOMEM;
> + break;
> + }
> +
> + overlay_get_node_info(overlay_fdt, &node_full_path,
> + num_overlay_nodes);
> +
> + ret = handle_remove_overlay_nodes(node_full_path,
> + num_overlay_nodes);
> + }
> +
> + xfree(node_full_path);
> +
> + break;
> + }
> +#endif
> +
> default:
> ret = arch_do_sysctl(op, u_sysctl);
Seeing this call is even in (patch) context - would you mind clarifying
why you don't add the new code to arch_do_sysctl() (perhaps by way of
further forwarding to a new dt_sysctl(), which could then live in a DT-
specific source file)?
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -1065,6 +1065,25 @@ typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
> DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
> #endif
>
> +#if defined (CONFIG_OVERLAY_DTB)
> +#define XEN_SYSCTL_DT_OVERLAY_ADD 1
> +#define XEN_SYSCTL_DT_OVERLAY_REMOVE 2
> +
> +/*
> + * XEN_SYSCTL_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_overlay_dt {
> + XEN_GUEST_HANDLE_64(void) overlay_fdt;
> + uint32_t overlay_fdt_size; /* Overlay dtb size. */
> + uint8_t overlay_op; /* Add or remove. */
> +};
Please make padding explicit and check that it's zero on input (so
it can later be assigned a purpose without needing to bump the
sysctl interface version).
> @@ -1125,6 +1145,9 @@ struct xen_sysctl {
> #if defined(__i386__) || defined(__x86_64__)
> struct xen_sysctl_cpu_policy cpu_policy;
> #endif
> +#if defined (CONFIG_OVERLAY_DTB)
> + struct xen_sysctl_overlay_dt overlay_dt;
> +#endif
No CONFIG_* dependencies in public headers, please.
Jan
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [XEN][RFC PATCH v2 08/12] xen/arm: Implement device tree node removal functionalities
2021-11-09 7:02 ` [XEN][RFC PATCH v2 08/12] xen/arm: Implement device tree node removal functionalities Vikram Garhwal
2021-11-09 11:16 ` Jan Beulich
@ 2021-12-22 16:13 ` Julien Grall
2021-12-24 0:45 ` Stefano Stabellini
1 sibling, 1 reply; 28+ messages in thread
From: Julien Grall @ 2021-12-22 16:13 UTC (permalink / raw)
To: Vikram Garhwal, xen-devel
Cc: sstabellini, bertrand.marquis, volodymyr_babchuk, Andrew Cooper,
George Dunlap, Ian Jackson, Jan Beulich, Wei Liu
Hi,
On 09/11/2021 08:02, Vikram Garhwal wrote:
> Introduce sysctl XEN_SYSCTL_overlay to remove device-tree nodes added using
I agree with Jan about the name being too generic. I will comment on
this a bit further down.
> 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 domus. If
> even one of the node in dtbo is not available for removal i.e. either not
> there in dt_host or currently used by any domain, in that case we don't
> remove any node in the given dtbo.
>
> 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 node. 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/device_tree.c | 53 ++++++
> xen/common/sysctl.c | 372 ++++++++++++++++++++++++++++++++++++++++++
> xen/include/public/sysctl.h | 23 +++
> xen/include/xen/device_tree.h | 4 +
> 4 files changed, 452 insertions(+)
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 26d2e28..19320e1 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -385,6 +385,59 @@ void dt_print_node_names(struct dt_device_node *dt)
> return;
> }
>
> +#if defined (CONFIG_OVERLAY_DTB)
> +int overlay_remove_node(struct dt_device_node *device_node)
This want to be prefixed with dt_* so it is clear which components it is
touching. But I think all the DT overlay code (including sysctl) should
be moved in a new file (maybe dt_overlay.c) to spreading the overlay
code and growing
the size of sysctl.c.
> +{
> + 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 ( np->name == device_node->name )
Why are you checking the equality between the fields name rather than
the node itself?
If it is intended, then I think this wants to be explained because often
people wants to check the names are equal (e.g str*cmp()) rather than
the pointer where the names are stored.
> + {
> + current_node->allnext = np->next;
> + return 0;
> + }
> +
> + for ( np = parent_node->child; np->sibling != NULL; np = np->sibling )
> + {
> + current_node = np;
> + if ( np->sibling->name == device_node->name )
Same question.
> + {
> + /* 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;
> +}
> +#endif
> +
> 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/common/sysctl.c b/xen/common/sysctl.c
> index f2dab72..fca47f5 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -28,6 +28,311 @@
> #include <xen/livepatch.h>
> #include <xen/coverage.h>
>
> +#if defined (CONFIG_OVERLAY_DTB)
This can be shortend to #ifdef CONFIG_<...>.
> +#include <xen/list.h>
> +#include <xen/libfdt/libfdt.h>
> +#include <xen/xmalloc.h>
> +#include <xen/device_tree.h>
> +#include <asm/domain_build.h>
> +#endif
> +
> +#if defined (CONFIG_OVERLAY_DTB)
Same here.
> +static LIST_HEAD(overlay_tracker);
> +static DEFINE_SPINLOCK(overlay_lock);
> +
> +/*
> + * overlay_node_track describes information about added nodes through dtbo.
> + * @dt_host_new: Pointer to the updated dt_host_new unflattened 'updated fdt'.
> + * @node_fullname: Store the name of nodes.
> + * @entry: List pointer.
> + */
> +struct overlay_track {
> + struct list_head entry;
> + struct dt_device_node *dt_host_new;
> + char **node_fullname;
> + uint8_t num_nodes;
Any reason to restrict to 256 nodes?
> +};
> +
> +/* Basic sanity check for the dtbo tool stack provided to Xen. */
> +static int check_overlay_fdt(void *overlay_fdt, uint32_t overlay_fdt_size)
This function doesn't modify overlay_fdt. So I think it should be const
if fdt_total_size() and fdt_check_header() allows it.
> +{
> + 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 int overlay_node_count(void *fdto)
> +{
> + int num_overlay_nodes = 0;
The name suggests this should be an unsiged int.
> + int fragment;
> +
> + fdt_for_each_subnode(fragment, fdto, 0)
> + {
> +
> + int subnode;
> + int overlay;
> +
> + overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
This may return < 0 if __overlay__ is not found. From my understanding,
fdt_for_each_subnode() would end up to start from 0.
So I think you want to add a check here.
> +
> + fdt_for_each_subnode(subnode, fdto, overlay)
> + {
> + num_overlay_nodes++;
> + }
> + }
> +
> + return num_overlay_nodes;
> +}
> +
> +/*
> + * overlay_get_node_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 void overlay_get_node_info(void *fdto, char ***node_full_path,
You will retrieve mutiple nodes. So I think the function wants to be
named 'overlay_get_nodes_info()'. Same for node_full_path.
Furthermore, you could drop one * if you return the list of paths. This
will also allow you to return an error when xmalloc fails (see below)
Lastly, AFAICT, fdto can be const.
> + int num_overlay_nodes)
This coud be unsigned int.
> +{
> + int fragment;
> + int node_num = 0;
This could be unsigned int.
> +
> + *node_full_path = xmalloc_bytes(num_overlay_nodes * sizeof(char *));
Memory allocation can fail.
> +
> + fdt_for_each_subnode(fragment, fdto, 0)
> + {
> + int target;
> + int overlay;
> + int subnode;
> + const char *target_path;
> +
> + target = fdt_overlay_get_target(device_tree_flattened, fdto, fragment,
> + &target_path);
fdt_overlay_get_target() can fail. Also, the indentation looks strange.
> + overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
fdt_subnode_offset can fail.
> +
> + fdt_for_each_subnode(subnode, fdto, overlay)
> + {
> + const char *node_name = fdt_get_name(fdto, subnode, NULL);
AFAIU, fdt_get_name() can return NULL;
> + int node_name_len = strlen(node_name);
fdt_get_name() can already provide the len for you. Can you re-use it?
> + int target_path_len = strlen(target_path);
> + int node_full_name_len = target_path_len + node_name_len + 2;
node_name_len, target_path_len, node_full_name_len can be unsigned.
Also, I would add a comment explain what '2' refers to.
> +
> + (*node_full_path)[node_num] = xmalloc_bytes(node_full_name_len);
xmalloc_bytes() can fail.
> +
> + memcpy((*node_full_path)[node_num], target_path, target_path_len);
> +
> + (*node_full_path)[node_num][target_path_len] = '/';
> +
> + memcpy((*node_full_path)[node_num] + target_path_len + 1, node_name,
> + node_name_len);
> +
> + (*node_full_path)[node_num][node_full_name_len - 1] = '\0';
> +
> + node_num++;
> + }
> + }
> +}
> +
> +/*
> + * Checks if all the devices node listed are present in dt_host and used by any
> + * domain.
> + */
Why do we need to handle all the nodes together? I think the code would
end up to be simpler if we were handling node by node.
> +static int check_nodes(char **full_dt_node_path, uint32_t num_nodes)
> +{
> + int rc = 0;
> + unsigned int i;
> + struct dt_device_node *overlay_node;
> + uint32_t ret = 0;
AFAICT, you are storing a domid here, so this wants to be a domid_t and
possible renamed to domid.
> +
> + for ( i = 0; i < num_nodes; i++ ) {
> + dt_dprintk("Finding node %s in the dt_host\n", full_dt_node_path[i]);
> +
> + overlay_node = dt_find_node_by_path(full_dt_node_path[i]);
> +
> + if ( overlay_node == NULL )
> + {
> + rc = -EINVAL;
> +
> + printk(XENLOG_G_ERR "Device %s is not present in the tree."
You seem to use a mix of XENLOG_G_ERR and XENLOG_ERR. However, it is not
entirely clear some messages are more critical than the other. Could you
clarify?
> + " Removing nodes failed\n", full_dt_node_path[i]);
Coding style: We don't split error message even if they are more than 80
lines. This helps grepping them in the log.
> + return rc;
> + }
> +
> + ret = dt_device_used_by(overlay_node);
> +
> + dt_dprintk("Checking if node %s is used by any domain\n",
> + full_dt_node_path[i]);
> +
> + if ( ret != 0 && ret != DOMID_IO )
In the commit message you wrote:
"The nodes get removed only if it is not used by any of dom0 or domus"
This implies that this should return -EINVAL for domid as well. Can you
make sure the two matches? (I am not sure which one you want).
Also, what does prevent the device to not be assigned for the check?
> + {
> + rc = -EINVAL;
NIT: This is a bit pointless to set rc when it is just used 2 lines
below in the return. The alternative is to replace the return with a break.
> +
> + printk(XENLOG_G_ERR "Device %s as it is being used by domain %d."
> + " Removing nodes failed\n", full_dt_node_path[i], ret);
Coding style: Same about the error message.
> + return rc;
> + }
> + }
> +
> + return rc;
> +}
> +
> +/* Remove nodes from dt_host. */
> +static int remove_nodes(char **full_dt_node_path, uint32_t num_nodes)
In the commit message, you said you wanted to remove all the nodes
together. But this function could possibly fail leaving some nodes
present or not.
As I wrote above, I think handling node by node would be fine. However,
we need to make sure that the remove operation can be called again to
clean-up the rest of the nodes.
> +{
> + struct domain *d = hardware_domain;
> + int rc = 0;
> + struct dt_device_node *overlay_node;
> + unsigned int naddr;
> + unsigned int i, j, nirq;
> + struct dt_raw_irq rirq;
> + u64 addr, size;
> +
> + for ( j = 0; j < num_nodes; j++ ) {
> + dt_dprintk("Removing node: %s\n", full_dt_node_path[j]);
> +
> + overlay_node = dt_find_node_by_path(full_dt_node_path[j]);
> +
> + nirq = dt_number_of_irq(overlay_node);
> +
> + /* Remove IRQ permission */
> + for ( i = 0; i < nirq; i++ )
> + {
> + rc = dt_device_get_raw_irq(overlay_node, i, &rirq);
> + if ( rc )
> + {
> + printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
> + i, dt_node_full_name(overlay_node));
> + return rc;
> + }
The interrupt may not be routed to the GIC, in which case we want to
skip them. So you want to check the controller is equal to
dt_interrupt_controller.
The code is also quite similar to handle_device_interrupts(). So I would
abstract the code to avoid duplication.
That said, I find a bit odd to have to parse the overlay again on remove
given you expect the same to be passed. I think it might be better to
use rangeset to keep track of the interrupts added with that specific
overlay.
This will reduce the possibility that remove can go wrong.
> +
> + rc = platform_get_irq(overlay_node, i);
> + if ( rc < 0 )
> + {
> + printk(XENLOG_ERR "Unable to get irq %u for %s\n",
> + i, dt_node_full_name(overlay_node));
> + return rc;
> + }
> +
> + rc = irq_deny_access(d, rc);
A few things:
1) IRQs can be assigned to another domain without the device being
assigned. So this want to be checked.
2) This is assuming the IRQ was not shared. I am not entirely sure
how to solve this one. Maybe a TODO and note in the documentation will
do for now. Stefano?
> + 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)));
Same remarks as for the interrupts here. Also, this remove the
permission but not the mappings in the P2M. Is it intended?
> + 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 = 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_node_path,
This is an array of nodes name, right? If so, please add a 's' to be
make it clear.
> + uint32_t num_nodes)
I think num_nodes can simply be unsigned int.
> +{
> + 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.
> + */
Technically, you can still pass a dtbo with the same node names but with
different properties inside.
I am not suggesting to check it, but I think it would be good to explain
the limitation (this will be important when we decide to
support/security support it).
> + 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++ ) {
Coding style:
for ( ... )
{
> + if( strcmp(full_dt_node_path[i], entry->node_fullname[i]) )
Coding style: Space after 'if'
> + {
> + /* 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 ) {
Coding style: The { should be on its own line.
> + rc = -EINVAL;
> +
> + printk(XENLOG_G_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 = check_nodes(full_dt_node_path, num_nodes);
> +
> + if ( rc )
> + goto out;
> +
> + rc = remove_nodes(full_dt_node_path, num_nodes);
> +
> + if ( rc ) {
> + printk(XENLOG_G_ERR "Removing node failed\n");
> + goto out;
> + }
> +
> + list_del(&entry->entry);
> + xfree(entry->node_fullname);
> + xfree(entry->dt_host_new);
> + xfree(entry);
> +
> +out:
> + spin_unlock(&overlay_lock);
> + return rc;
> +}
> +#endif
> +
> long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> {
> long ret = 0;
> @@ -476,6 +781,73 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> copyback = 1;
> break;
>
> +#if defined (CONFIG_OVERLAY_DTB)
> + case XEN_SYSCTL_overlay:
> + {
> + void *overlay_fdt;
> + char **node_full_path = NULL;
> + int num_overlay_nodes;
I think this can be unsigned int.
> +
> + if ( op->u.overlay_dt.overlay_fdt_size > 0 )
> + overlay_fdt = xmalloc_bytes(op->u.overlay_dt.overlay_fdt_size);
> + else
> + {
> + ret = -EINVAL;
> + break;
> + }
I would re-order the code so the failure case is first. I.e:
if ( op->... <= 0 )
{
ret = -EINVAL;
break;
}
overlay_fdt = xmalloc...
> +
> + if ( overlay_fdt == NULL )
> + {
> + ret = -ENOMEM;
> + break;
> + }
> +
> + ret = copy_from_guest(overlay_fdt, op->u.overlay_dt.overlay_fdt,
> + op->u.overlay_dt.overlay_fdt_size);
> + if ( ret )
> + {
> + gprintk(XENLOG_ERR, "copy from guest failed\n");
See my remark about the printk() above. I think we need to be consistent
in how we use it in the overlay code.
> + xfree(overlay_fdt);
> +
> + ret = -EFAULT;
> + break;
> + }
> +
> + if ( op->u.overlay_dt.overlay_op == XEN_SYSCTL_DT_OVERLAY_ADD )
The code would be easier to read if you use a switch. However...
> + {
> + ret = handle_add_overlay_nodes(overlay_fdt,
> + op->u.overlay_dt.overlay_fdt_size);
... this function doesn't exist. So this will break compilation.
> + } else if ( op->u.overlay_dt.overlay_op ==
> + XEN_SYSCTL_DT_OVERLAY_REMOVE )
> + {
> + ret = check_overlay_fdt(overlay_fdt,
> + op->u.overlay_dt.overlay_fdt_size);
> + if ( ret )
> + {
> + ret = -EFAULT;
> + break;
> + }
> +
> + num_overlay_nodes = overlay_node_count(overlay_fdt);
> + if ( num_overlay_nodes == 0 )
> + {
> + ret = -ENOMEM;
> + break;
> + }
> +
> + overlay_get_node_info(overlay_fdt, &node_full_path,
> + num_overlay_nodes);
> +
> + ret = handle_remove_overlay_nodes(node_full_path,
> + num_overlay_nodes);
> + }
> +
> + xfree(node_full_path);
> +
> + 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 3e53681..6624724 100644
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -1065,6 +1065,25 @@ typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
> DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
> #endif
>
> +#if defined (CONFIG_OVERLAY_DTB)
> +#define XEN_SYSCTL_DT_OVERLAY_ADD 1
I would add this one only when it is introduced.
> +#define XEN_SYSCTL_DT_OVERLAY_REMOVE 2
Here you are using XEN_SYSCTL_DT_OVERLAY...
> +
> +/*
> + * XEN_SYSCTL_overlay
... here overlay and...
> + * 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_overlay_dt {
... here overlay_dt. For the hypercall, it is important to keep the
naming consistent. In this case, I would use
* defines: XEN_SYSCTL_DT_OVERLAY_<...>
* subop: XEN_SYSCTL_dt_overlay
* structure: 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. */
> +};
> +#endif
> +
> struct xen_sysctl {
> uint32_t cmd;
> #define XEN_SYSCTL_readconsole 1
> @@ -1095,6 +1114,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_overlay 30
> uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
> union {
> struct xen_sysctl_readconsole readconsole;
> @@ -1125,6 +1145,9 @@ struct xen_sysctl {
> #if defined(__i386__) || defined(__x86_64__)
> struct xen_sysctl_cpu_policy cpu_policy;
> #endif
> +#if defined (CONFIG_OVERLAY_DTB)
> + struct xen_sysctl_overlay_dt overlay_dt;
> +#endif
> uint8_t pad[128];
> } u;
> };
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 5ba26a0..cf29cf5 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -553,6 +553,10 @@ int dt_find_node_by_gpath(XEN_GUEST_HANDLE(char) u_path, uint32_t u_plen,
> */
> void dt_print_node_names(struct dt_device_node *dt);
>
> +#if defined (CONFIG_OVERLAY_DTB)
> +int overlay_remove_node(struct dt_device_node *device_node);
> +#endif
> +
> /**
> * dt_get_parent - Get a node's parent if any
> * @node: Node to get parent
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [XEN][RFC PATCH v2 08/12] xen/arm: Implement device tree node removal functionalities
2021-12-22 16:13 ` Julien Grall
@ 2021-12-24 0:45 ` Stefano Stabellini
0 siblings, 0 replies; 28+ messages in thread
From: Stefano Stabellini @ 2021-12-24 0:45 UTC (permalink / raw)
To: Julien Grall
Cc: Vikram Garhwal, xen-devel, sstabellini, bertrand.marquis,
volodymyr_babchuk, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Wei Liu
On Wed, 22 Dec 2021, Julien Grall wrote:
> Hi,
>
> On 09/11/2021 08:02, Vikram Garhwal wrote:
> > Introduce sysctl XEN_SYSCTL_overlay to remove device-tree nodes added using
>
> I agree with Jan about the name being too generic. I will comment on this a
> bit further down.
>
> > 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 domus.
> > If
> > even one of the node in dtbo is not available for removal i.e. either
> > not
> > there in dt_host or currently used by any domain, in that case we don't
> > remove any node in the given dtbo.
> >
> > 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 node. 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/device_tree.c | 53 ++++++
> > xen/common/sysctl.c | 372
> > ++++++++++++++++++++++++++++++++++++++++++
> > xen/include/public/sysctl.h | 23 +++
> > xen/include/xen/device_tree.h | 4 +
> > 4 files changed, 452 insertions(+)
> >
> > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > index 26d2e28..19320e1 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -385,6 +385,59 @@ void dt_print_node_names(struct dt_device_node *dt)
> > return;
> > }
> > +#if defined (CONFIG_OVERLAY_DTB)
> > +int overlay_remove_node(struct dt_device_node *device_node)
>
> This want to be prefixed with dt_* so it is clear which components it is
> touching. But I think all the DT overlay code (including sysctl) should be
> moved in a new file (maybe dt_overlay.c) to spreading the overlay code and
> growing
> the size of sysctl.c.
>
> > +{
> > + 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 ( np->name == device_node->name )
>
> Why are you checking the equality between the fields name rather than the node
> itself?
>
> If it is intended, then I think this wants to be explained because often
> people wants to check the names are equal (e.g str*cmp()) rather than the
> pointer where the names are stored.
>
> > + {
> > + current_node->allnext = np->next;
> > + return 0;
> > + }
> > +
> > + for ( np = parent_node->child; np->sibling != NULL; np = np->sibling )
> > + {
> > + current_node = np;
> > + if ( np->sibling->name == device_node->name )
>
> Same question.
>
> > + {
> > + /* 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;
> > +}
> > +#endif
> > +
> > 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/common/sysctl.c b/xen/common/sysctl.c
> > index f2dab72..fca47f5 100644
> > --- a/xen/common/sysctl.c
> > +++ b/xen/common/sysctl.c
> > @@ -28,6 +28,311 @@
> > #include <xen/livepatch.h>
> > #include <xen/coverage.h>
> > +#if defined (CONFIG_OVERLAY_DTB)
>
> This can be shortend to #ifdef CONFIG_<...>.
>
> > +#include <xen/list.h>
> > +#include <xen/libfdt/libfdt.h>
> > +#include <xen/xmalloc.h>
> > +#include <xen/device_tree.h>
> > +#include <asm/domain_build.h>
> > +#endif
> > +
> > +#if defined (CONFIG_OVERLAY_DTB)
>
> Same here.
>
> > +static LIST_HEAD(overlay_tracker);
> > +static DEFINE_SPINLOCK(overlay_lock);
> > +
> > +/*
> > + * overlay_node_track describes information about added nodes through dtbo.
> > + * @dt_host_new: Pointer to the updated dt_host_new unflattened 'updated
> > fdt'.
> > + * @node_fullname: Store the name of nodes.
> > + * @entry: List pointer.
> > + */
> > +struct overlay_track {
> > + struct list_head entry;
> > + struct dt_device_node *dt_host_new;
> > + char **node_fullname;
> > + uint8_t num_nodes;
>
> Any reason to restrict to 256 nodes?
>
> > +};
> > +
> > +/* Basic sanity check for the dtbo tool stack provided to Xen. */
> > +static int check_overlay_fdt(void *overlay_fdt, uint32_t overlay_fdt_size)
> This function doesn't modify overlay_fdt. So I think it should be const if
> fdt_total_size() and fdt_check_header() allows it.
>
> > +{
> > + 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 int overlay_node_count(void *fdto)
> > +{
> > + int num_overlay_nodes = 0;
>
> The name suggests this should be an unsiged int.
>
> > + int fragment;
> > +
> > + fdt_for_each_subnode(fragment, fdto, 0)
> > + {
> > +
> > + int subnode;
> > + int overlay;
> > +
> > + overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
>
> This may return < 0 if __overlay__ is not found. From my understanding,
> fdt_for_each_subnode() would end up to start from 0.
>
> So I think you want to add a check here.
>
> > +
> > + fdt_for_each_subnode(subnode, fdto, overlay)
> > + {
> > + num_overlay_nodes++;
> > + }
> > + }
> > +
> > + return num_overlay_nodes;
> > +}
> > +
> > +/*
> > + * overlay_get_node_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 void overlay_get_node_info(void *fdto, char ***node_full_path,
>
> You will retrieve mutiple nodes. So I think the function wants to be named
> 'overlay_get_nodes_info()'. Same for node_full_path.
>
> Furthermore, you could drop one * if you return the list of paths. This will
> also allow you to return an error when xmalloc fails (see below)
>
> Lastly, AFAICT, fdto can be const.
>
> > + int num_overlay_nodes)
>
> This coud be unsigned int.
>
> > +{
> > + int fragment;
> > + int node_num = 0;
>
> This could be unsigned int.
>
> > +
> > + *node_full_path = xmalloc_bytes(num_overlay_nodes * sizeof(char *));
>
> Memory allocation can fail.
>
> > +
> > + fdt_for_each_subnode(fragment, fdto, 0)
> > + {
> > + int target;
> > + int overlay;
> > + int subnode;
> > + const char *target_path;
> > +
> > + target = fdt_overlay_get_target(device_tree_flattened, fdto,
> > fragment,
> > + &target_path);
>
> fdt_overlay_get_target() can fail. Also, the indentation looks strange.
>
> > + overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
>
> fdt_subnode_offset can fail.
>
> > +
> > + fdt_for_each_subnode(subnode, fdto, overlay)
> > + {
> > + const char *node_name = fdt_get_name(fdto, subnode, NULL);
>
> AFAIU, fdt_get_name() can return NULL;
>
> > + int node_name_len = strlen(node_name);
>
> fdt_get_name() can already provide the len for you. Can you re-use it?
>
>
>
> > + int target_path_len = strlen(target_path);
> > + int node_full_name_len = target_path_len + node_name_len + 2;
>
> node_name_len, target_path_len, node_full_name_len can be unsigned. Also, I
> would add a comment explain what '2' refers to.
>
> > +
> > + (*node_full_path)[node_num] =
> > xmalloc_bytes(node_full_name_len);
>
> xmalloc_bytes() can fail.
>
> > +
> > + memcpy((*node_full_path)[node_num], target_path,
> > target_path_len);
> > +
> > + (*node_full_path)[node_num][target_path_len] = '/';
> > +
> > + memcpy((*node_full_path)[node_num] + target_path_len + 1,
> > node_name,
> > + node_name_len);
> > +
> > + (*node_full_path)[node_num][node_full_name_len - 1] = '\0';
> > +
> > + node_num++;
> > + }
> > + }
> > +}
> > +
> > +/*
> > + * Checks if all the devices node listed are present in dt_host and used by
> > any
> > + * domain.
> > + */
>
> Why do we need to handle all the nodes together? I think the code would end up
> to be simpler if we were handling node by node.
>
> > +static int check_nodes(char **full_dt_node_path, uint32_t num_nodes)
> > +{
> > + int rc = 0;
> > + unsigned int i;
> > + struct dt_device_node *overlay_node;
> > + uint32_t ret = 0;
>
> AFAICT, you are storing a domid here, so this wants to be a domid_t and
> possible renamed to domid.
>
> > +
> > + for ( i = 0; i < num_nodes; i++ ) {
> > + dt_dprintk("Finding node %s in the dt_host\n",
> > full_dt_node_path[i]);
> > +
> > + overlay_node = dt_find_node_by_path(full_dt_node_path[i]);
> > +
> > + if ( overlay_node == NULL )
> > + {
> > + rc = -EINVAL;
> > +
> > + printk(XENLOG_G_ERR "Device %s is not present in the tree."
>
> You seem to use a mix of XENLOG_G_ERR and XENLOG_ERR. However, it is not
> entirely clear some messages are more critical than the other. Could you
> clarify?
>
> > + " Removing nodes failed\n", full_dt_node_path[i]);
>
> Coding style: We don't split error message even if they are more than 80
> lines. This helps grepping them in the log.
>
> > + return rc;
> > + }
> > +
> > + ret = dt_device_used_by(overlay_node);
> > +
> > + dt_dprintk("Checking if node %s is used by any domain\n",
> > + full_dt_node_path[i]);
> > +
> > + if ( ret != 0 && ret != DOMID_IO )
>
> In the commit message you wrote:
>
> "The nodes get removed only if it is not used by any of dom0 or domus"
>
> This implies that this should return -EINVAL for domid as well. Can you make
> sure the two matches? (I am not sure which one you want).
>
> Also, what does prevent the device to not be assigned for the check?
>
> > + {
> > + rc = -EINVAL;
>
> NIT: This is a bit pointless to set rc when it is just used 2 lines below in
> the return. The alternative is to replace the return with a break.
>
> > +
> > + printk(XENLOG_G_ERR "Device %s as it is being used by domain
> > %d."
> > + " Removing nodes failed\n", full_dt_node_path[i], ret);
>
> Coding style: Same about the error message.
>
> > + return rc;
> > + }
> > + }
> > +
> > + return rc;
> > +}
> > +
> > +/* Remove nodes from dt_host. */
> > +static int remove_nodes(char **full_dt_node_path, uint32_t num_nodes)
>
> In the commit message, you said you wanted to remove all the nodes together.
> But this function could possibly fail leaving some nodes present or not.
>
> As I wrote above, I think handling node by node would be fine. However, we
> need to make sure that the remove operation can be called again to clean-up
> the rest of the nodes.
>
> > +{
> > + struct domain *d = hardware_domain;
> > + int rc = 0;
> > + struct dt_device_node *overlay_node;
> > + unsigned int naddr;
> > + unsigned int i, j, nirq;
> > + struct dt_raw_irq rirq;
> > + u64 addr, size;
> > +
> > + for ( j = 0; j < num_nodes; j++ ) {
> > + dt_dprintk("Removing node: %s\n", full_dt_node_path[j]);
> > +
> > + overlay_node = dt_find_node_by_path(full_dt_node_path[j]);
> > +
> > + nirq = dt_number_of_irq(overlay_node);
> > +
> > + /* Remove IRQ permission */
> > + for ( i = 0; i < nirq; i++ )
> > + {
> > + rc = dt_device_get_raw_irq(overlay_node, i, &rirq);
> > + if ( rc )
> > + {
> > + printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
> > + i, dt_node_full_name(overlay_node));
> > + return rc;
> > + }
>
> The interrupt may not be routed to the GIC, in which case we want to skip
> them. So you want to check the controller is equal to dt_interrupt_controller.
>
> The code is also quite similar to handle_device_interrupts(). So I would
> abstract the code to avoid duplication.
>
> That said, I find a bit odd to have to parse the overlay again on remove given
> you expect the same to be passed. I think it might be better to use rangeset
> to keep track of the interrupts added with that specific overlay.
>
> This will reduce the possibility that remove can go wrong.
>
> > +
> > + rc = platform_get_irq(overlay_node, i);
> > + if ( rc < 0 )
> > + {
> > + printk(XENLOG_ERR "Unable to get irq %u for %s\n",
> > + i, dt_node_full_name(overlay_node));
> > + return rc;
> > + }
> > +
> > + rc = irq_deny_access(d, rc);
>
> A few things:
>
> 1) IRQs can be assigned to another domain without the device being assigned.
> So this want to be checked.
> 2) This is assuming the IRQ was not shared. I am not entirely sure how to
> solve this one. Maybe a TODO and note in the documentation will do for now.
> Stefano?
I think it is fine if we don't handle shared IRQs for now. A note in the
documentation and a TODO is OK.
^ permalink raw reply [flat|nested] 28+ messages in thread
* [XEN][RFC PATCH v2 09/12] xen/arm: Implement device tree node addition functionalities
2021-11-09 7:02 [XEN][RFC PATCH v2 00/12] dynamic node programming using overlay dtbo Vikram Garhwal
` (7 preceding siblings ...)
2021-11-09 7:02 ` [XEN][RFC PATCH v2 08/12] xen/arm: Implement device tree node removal functionalities Vikram Garhwal
@ 2021-11-09 7:02 ` Vikram Garhwal
2021-11-09 11:19 ` Jan Beulich
2021-11-09 7:02 ` [XEN][RFC PATCH v2 10/12] tools/libs/ctrl: Implement new xc interfaces for dt overlay Vikram Garhwal
` (2 subsequent siblings)
11 siblings, 1 reply; 28+ messages in thread
From: Vikram Garhwal @ 2021-11-09 7:02 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk,
Vikram Garhwal, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Wei Liu
Update sysctl XEN_SYSCTL_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/device_tree.c | 41 +++++++++
xen/common/sysctl.c | 199 ++++++++++++++++++++++++++++++++++++++++++
xen/include/xen/device_tree.h | 2 +
3 files changed, 242 insertions(+)
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 19320e1..5dff64c 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -386,6 +386,47 @@ void dt_print_node_names(struct dt_device_node *dt)
}
#if defined (CONFIG_OVERLAY_DTB)
+int 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. 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;
+}
+
int overlay_remove_node(struct dt_device_node *device_node)
{
struct dt_device_node *np;
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index fca47f5..38824b2 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -331,6 +331,205 @@ out:
spin_unlock(&overlay_lock);
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 **node_full_path = NULL;
+ void *fdt = NULL;
+ struct dt_device_node *dt_host_new;
+ struct domain *d = hardware_domain;
+ struct overlay_track *tr = NULL;
+ unsigned int naddr;
+ unsigned int i, j;
+ 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 )
+ 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 )
+ goto err;
+
+ /*
+ * overlay_get_node_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.
+ */
+ overlay_get_node_info(overlay_fdt, &node_full_path, num_overlay_nodes);
+
+ 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(node_full_path[j]);
+ if ( overlay_node != NULL )
+ {
+ printk(XENLOG_ERR "node %s exists in device tree\n",
+ node_full_path[j]);
+ rc = -EINVAL;
+ xfree(node_full_path);
+ 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", node_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, node_full_path[j]);
+ if ( overlay_node == NULL )
+ {
+ dt_dprintk("%s node not found\n", node_full_path[j]);
+ rc = -EFAULT;
+ goto remove_node;
+ }
+
+ /* Add the node to dt_host. */
+ rc = overlay_add_node(overlay_node, overlay_node->parent->full_name);
+ if ( rc )
+ {
+ /* Node not added in dt_host. */
+ goto remove_node;
+ }
+
+ /* Get the node from dt_host and add interrupt and IOMMUs. */
+ 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_G_ERR "Interrupt failed\n");
+ goto remove_node;
+ }
+
+ /* Add device to IOMMUs */
+ rc = iommu_add_dt_device(overlay_node);
+ if ( rc < 0 )
+ {
+ printk(XENLOG_G_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 and map MMIOs */
+ for ( i = 0; i < naddr; i++ )
+ {
+ struct map_range_data mr_data = { .d = d,
+ .p2mt = p2m_mmio_direct_c };
+ 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->node_fullname = node_full_path;
+ tr->num_nodes = num_overlay_nodes;
+
+ if ( tr->node_fullname == NULL )
+ {
+ rc = -ENOMEM;
+ goto remove_node;
+ }
+
+ INIT_LIST_HEAD(&tr->entry);
+ list_add_tail(&tr->entry, &overlay_tracker);
+
+err:
+ spin_unlock(&overlay_lock);
+ xfree(fdt);
+ return rc;
+
+/*
+ * Failure case. We need to remove the nodes, free tracker(if tr exists) and
+ * dt_host_new.
+ */
+remove_node:
+ xfree(fdt);
+ rc = check_nodes(node_full_path, j);
+
+ if ( rc ) {
+ spin_unlock(&overlay_lock);
+ return rc;
+ }
+
+ rc = remove_nodes(node_full_path, j);
+
+ if ( rc ) {
+ printk(XENLOG_G_ERR "Removing node failed\n");
+ spin_unlock(&overlay_lock);
+ return rc;
+ }
+
+ spin_unlock(&overlay_lock);
+
+ xfree(dt_host_new);
+
+ if ( tr )
+ xfree(tr);
+
+ if ( node_full_path )
+ xfree(node_full_path);
+
+ return rc;
+}
#endif
long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index cf29cf5..eafb269 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -554,6 +554,8 @@ int dt_find_node_by_gpath(XEN_GUEST_HANDLE(char) u_path, uint32_t u_plen,
void dt_print_node_names(struct dt_device_node *dt);
#if defined (CONFIG_OVERLAY_DTB)
+int overlay_add_node(struct dt_device_node *device_node,
+ const char *parent_node_path);
int overlay_remove_node(struct dt_device_node *device_node);
#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [XEN][RFC PATCH v2 09/12] xen/arm: Implement device tree node addition functionalities
2021-11-09 7:02 ` [XEN][RFC PATCH v2 09/12] xen/arm: Implement device tree node addition functionalities Vikram Garhwal
@ 2021-11-09 11:19 ` Jan Beulich
2021-11-09 17:55 ` Vikram Garhwal
0 siblings, 1 reply; 28+ messages in thread
From: Jan Beulich @ 2021-11-09 11:19 UTC (permalink / raw)
To: Vikram Garhwal
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk,
Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu, xen-devel
On 09.11.2021 08:02, Vikram Garhwal wrote:
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -331,6 +331,205 @@ out:
> spin_unlock(&overlay_lock);
> 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)
You adding a static function here without any caller got me puzzled.
First I thought you'd be introducing a build failure this was, but
it's really patch 8 which does by introducing a call to this function
without the function actually being there.
Jan
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN][RFC PATCH v2 09/12] xen/arm: Implement device tree node addition functionalities
2021-11-09 11:19 ` Jan Beulich
@ 2021-11-09 17:55 ` Vikram Garhwal
0 siblings, 0 replies; 28+ messages in thread
From: Vikram Garhwal @ 2021-11-09 17:55 UTC (permalink / raw)
To: Jan Beulich
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk,
Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu, xen-devel
[-- Attachment #1: Type: text/plain, Size: 1771 bytes --]
Hi Jan,
Thanks for reviewing the patches.
Yeah, I messed it up while breaking the code into multiple patches. For next v3 patch series, I will move the function call "handle_add_overlay_nodes()"(in patch 8/12) to 9/12 patch.
Thanks,
Vikram
________________________________
From: Jan Beulich <jbeulich@suse.com>
Sent: Tuesday, November 9, 2021 3:19 AM
To: Vikram Garhwal <fnuv@xilinx.com>
Cc: sstabellini@kernel.org <sstabellini@kernel.org>; julien@xen.org <julien@xen.org>; bertrand.marquis@arm.com <bertrand.marquis@arm.com>; volodymyr_babchuk@epam.com <volodymyr_babchuk@epam.com>; Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Ian Jackson <iwj@xenproject.org>; Wei Liu <wl@xen.org>; xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org>
Subject: Re: [XEN][RFC PATCH v2 09/12] xen/arm: Implement device tree node addition functionalities
On 09.11.2021 08:02, Vikram Garhwal wrote:
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -331,6 +331,205 @@ out:
> spin_unlock(&overlay_lock);
> 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)
You adding a static function here without any caller got me puzzled.
First I thought you'd be introducing a build failure this was, but
it's really patch 8 which does by introducing a call to this function
without the function actually being there.
Jan
[-- Attachment #2: Type: text/html, Size: 5099 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* [XEN][RFC PATCH v2 10/12] tools/libs/ctrl: Implement new xc interfaces for dt overlay
2021-11-09 7:02 [XEN][RFC PATCH v2 00/12] dynamic node programming using overlay dtbo Vikram Garhwal
` (8 preceding siblings ...)
2021-11-09 7:02 ` [XEN][RFC PATCH v2 09/12] xen/arm: Implement device tree node addition functionalities Vikram Garhwal
@ 2021-11-09 7:02 ` Vikram Garhwal
2021-11-11 16:54 ` Anthony PERARD
2021-11-09 7:02 ` [XEN][RFC PATCH v2 11/12] tools/libs/light: Implement new libxl functions for device tree overlay ops Vikram Garhwal
2021-11-09 7:02 ` [XEN][RFC PATCH v2 12/12] tools/xl: Add new xl command overlay Vikram Garhwal
11 siblings, 1 reply; 28+ messages in thread
From: Vikram Garhwal @ 2021-11-09 7:02 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk,
Vikram Garhwal, Ian Jackson, Wei Liu, 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 | 5 +++++
tools/libs/ctrl/Makefile | 1 +
tools/libs/ctrl/xc_overlay.c | 51 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 57 insertions(+)
create mode 100644 tools/libs/ctrl/xc_overlay.c
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 07b96e6..cfd7c5c 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2684,6 +2684,11 @@ 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);
+#if defined (CONFIG_OVERLAY_DTB)
+int xc_dt_overlay(xc_interface *xch, void *overlay_fdt, int overlay_fdt_size,
+ uint8_t overlayop);
+#endif
+
/* Compat shims */
#include "xenctrl_compat.h"
diff --git a/tools/libs/ctrl/Makefile b/tools/libs/ctrl/Makefile
index 519246b..a21a949 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-$(CONFIG_OVERLAY_DTB) += 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 0000000..77f9edc
--- /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 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_overlay;
+ sysctl.u.overlay_dt.overlay_op= op;
+ sysctl.u.overlay_dt.overlay_fdt_size = overlay_fdt_size;
+
+ set_xen_guest_handle(sysctl.u.overlay_dt.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.7.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [XEN][RFC PATCH v2 10/12] tools/libs/ctrl: Implement new xc interfaces for dt overlay
2021-11-09 7:02 ` [XEN][RFC PATCH v2 10/12] tools/libs/ctrl: Implement new xc interfaces for dt overlay Vikram Garhwal
@ 2021-11-11 16:54 ` Anthony PERARD
2021-11-11 19:46 ` Vikram Garhwal
0 siblings, 1 reply; 28+ messages in thread
From: Anthony PERARD @ 2021-11-11 16:54 UTC (permalink / raw)
To: Vikram Garhwal
Cc: xen-devel, sstabellini, julien, bertrand.marquis,
volodymyr_babchuk, Ian Jackson, Wei Liu, Juergen Gross
On Mon, Nov 08, 2021 at 11:02:25PM -0800, Vikram Garhwal wrote:
> 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 | 5 +++++
> tools/libs/ctrl/Makefile | 1 +
> tools/libs/ctrl/xc_overlay.c | 51 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 57 insertions(+)
> create mode 100644 tools/libs/ctrl/xc_overlay.c
>
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 07b96e6..cfd7c5c 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -2684,6 +2684,11 @@ 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);
>
> +#if defined (CONFIG_OVERLAY_DTB)
> +int xc_dt_overlay(xc_interface *xch, void *overlay_fdt, int overlay_fdt_size,
> + uint8_t overlayop);
> +#endif
> +
> /* Compat shims */
> #include "xenctrl_compat.h"
>
> diff --git a/tools/libs/ctrl/Makefile b/tools/libs/ctrl/Makefile
> index 519246b..a21a949 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-$(CONFIG_OVERLAY_DTB) += xc_overlay.c
So, this patch seems to introduce the use of CONFIG_OVERLAY_DTB, is
there a reason why the new functionality can't be always builtin?
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN][RFC PATCH v2 10/12] tools/libs/ctrl: Implement new xc interfaces for dt overlay
2021-11-11 16:54 ` Anthony PERARD
@ 2021-11-11 19:46 ` Vikram Garhwal
2021-11-12 14:21 ` Anthony PERARD
0 siblings, 1 reply; 28+ messages in thread
From: Vikram Garhwal @ 2021-11-11 19:46 UTC (permalink / raw)
To: Anthony PERARD
Cc: xen-devel, sstabellini, julien, bertrand.marquis,
volodymyr_babchuk, Ian Jackson, Wei Liu, Juergen Gross
On Thu, Nov 11, 2021 at 04:54:25PM +0000, Anthony PERARD wrote:
Hi Anthony,
> On Mon, Nov 08, 2021 at 11:02:25PM -0800, Vikram Garhwal wrote:
> > 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 | 5 +++++
> > tools/libs/ctrl/Makefile | 1 +
> > tools/libs/ctrl/xc_overlay.c | 51 ++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 57 insertions(+)
> > create mode 100644 tools/libs/ctrl/xc_overlay.c
> >
> > diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> > index 07b96e6..cfd7c5c 100644
> > --- a/tools/include/xenctrl.h
> > +++ b/tools/include/xenctrl.h
> > @@ -2684,6 +2684,11 @@ 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);
> >
> > +#if defined (CONFIG_OVERLAY_DTB)
> > +int xc_dt_overlay(xc_interface *xch, void *overlay_fdt, int overlay_fdt_size,
> > + uint8_t overlayop);
> > +#endif
> > +
> > /* Compat shims */
> > #include "xenctrl_compat.h"
> >
> > diff --git a/tools/libs/ctrl/Makefile b/tools/libs/ctrl/Makefile
> > index 519246b..a21a949 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-$(CONFIG_OVERLAY_DTB) += xc_overlay.c
>
> So, this patch seems to introduce the use of CONFIG_OVERLAY_DTB, is
> there a reason why the new functionality can't be always builtin?
>
Above, if you meant removing "CONFIG_OEVRLAY_DTB" then here is my answer:
This feature is supported by ARM based FPGA devices only so there were a few
comments on v1 series to keep the code inside a config only. Now, for the tool
side also I kept the CONFIG_OVERLAY_DTB to align the xen-tools with Xen.
Although, now i saw your comments on patch 10 regarding "always provide
libxl_dt_overlay() but which would return ENOSYS when libxl is built without
CONFIG_OVERLAY_DTB". That seems better approach here for all three xen-tool
patches.
Initially, i was not sure what to do here that's why i wrote a question in the
cover letter about this.
Also, do you know how to enable this config via menuconfig when building the Xen
tools? I know how to enable for Xen but not sure about tools.
Thanks for reviewing this.
Regards,
Vikram Garhwal
> Thanks,
>
> --
> Anthony PERARD
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [XEN][RFC PATCH v2 10/12] tools/libs/ctrl: Implement new xc interfaces for dt overlay
2021-11-11 19:46 ` Vikram Garhwal
@ 2021-11-12 14:21 ` Anthony PERARD
0 siblings, 0 replies; 28+ messages in thread
From: Anthony PERARD @ 2021-11-12 14:21 UTC (permalink / raw)
To: Vikram Garhwal
Cc: xen-devel, sstabellini, julien, bertrand.marquis,
volodymyr_babchuk, Ian Jackson, Wei Liu, Juergen Gross
On Thu, Nov 11, 2021 at 11:46:36AM -0800, Vikram Garhwal wrote:
> On Thu, Nov 11, 2021 at 04:54:25PM +0000, Anthony PERARD wrote:
> Hi Anthony,
> > On Mon, Nov 08, 2021 at 11:02:25PM -0800, Vikram Garhwal wrote:
> > > +SRCS-$(CONFIG_OVERLAY_DTB) += xc_overlay.c
> >
> > So, this patch seems to introduce the use of CONFIG_OVERLAY_DTB, is
> > there a reason why the new functionality can't be always builtin?
> >
> Above, if you meant removing "CONFIG_OEVRLAY_DTB" then here is my answer:
> This feature is supported by ARM based FPGA devices only so there were a few
> comments on v1 series to keep the code inside a config only. Now, for the tool
> side also I kept the CONFIG_OVERLAY_DTB to align the xen-tools with Xen.
>
> Although, now i saw your comments on patch 10 regarding "always provide
> libxl_dt_overlay() but which would return ENOSYS when libxl is built without
> CONFIG_OVERLAY_DTB". That seems better approach here for all three xen-tool
> patches.
>
> Initially, i was not sure what to do here that's why i wrote a question in the
> cover letter about this.
>
> Also, do you know how to enable this config via menuconfig when building the Xen
> tools? I know how to enable for Xen but not sure about tools.
It isn't possible to use the configuration of the hypervisor to build
the tool. We use autoconf (configure.ac, ...) to configure the tools but
I don't think in this case that having CONFIG_OVERLAY_DTB for the tools
is the right thing to do. In the tools, we don't really have a way to
select functionalities available in the different libraries, or it is
mostly based on the architecture or operating system or available
system libraries.
Contrary to the hypervisor side, the added code in the libraries is
mostly glue which calls the hypervisor, so their isn't really a need to
avoid building it. If the functionality isn't available in the
hypervisor, it should return an error and the library can deal with that
error.
You might want to limit the build to Arm, but I don't know if that
"overlay dtb" thing is really Arm specific (even though device trees are
mostly use on Arm I guess).
--
Anthony PERARD
^ permalink raw reply [flat|nested] 28+ messages in thread
* [XEN][RFC PATCH v2 11/12] tools/libs/light: Implement new libxl functions for device tree overlay ops
2021-11-09 7:02 [XEN][RFC PATCH v2 00/12] dynamic node programming using overlay dtbo Vikram Garhwal
` (9 preceding siblings ...)
2021-11-09 7:02 ` [XEN][RFC PATCH v2 10/12] tools/libs/ctrl: Implement new xc interfaces for dt overlay Vikram Garhwal
@ 2021-11-09 7:02 ` Vikram Garhwal
2021-11-11 16:44 ` Anthony PERARD
2021-11-09 7:02 ` [XEN][RFC PATCH v2 12/12] tools/xl: Add new xl command overlay Vikram Garhwal
11 siblings, 1 reply; 28+ messages in thread
From: Vikram Garhwal @ 2021-11-09 7:02 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk,
Vikram Garhwal, Ian Jackson, Wei Liu, Anthony PERARD,
Juergen Gross
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
tools/include/libxl.h | 5 ++++
tools/libs/light/Makefile | 3 ++
tools/libs/light/libxl_overlay.c | 65 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 73 insertions(+)
create mode 100644 tools/libs/light/libxl_overlay.c
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index 2e8679d..3dcb3e7 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -2406,6 +2406,11 @@ 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);
+#if defined (CONFIG_OVERLAY_DTB)
+int libxl_dt_overlay(libxl_ctx *ctx, void *overlay,
+ int overlay_size, uint8_t op);
+#endif
+
/*
* 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 194bc5f..0fffa93 100644
--- a/tools/libs/light/Makefile
+++ b/tools/libs/light/Makefile
@@ -117,6 +117,9 @@ SRCS-y += libxl_genid.c
SRCS-y += _libxl_types.c
SRCS-y += libxl_flask.c
SRCS-y += _libxl_types_internal.c
+ifeq ($(CONFIG_OVERLAY_DTB),y)
+SRCS-y += libxl_overlay.o
+endif
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 0000000..d965aee
--- /dev/null
+++ b/tools/libs/light/libxl_overlay.c
@@ -0,0 +1,65 @@
+/*
+ * 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 op)
+{
+ int rc = 0;
+ GC_INIT(ctx);
+
+ if (check_overlay_fdt(gc, overlay_dt, overlay_dt_size)) {
+ LOG(ERROR, "Overlay DTB check failed\n");
+ 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, op);
+
+ if (rc)
+ LOG(ERROR, "%s: Adding/Removing overlay dtb failed.\n", __func__);
+
+ return rc;
+}
+
--
2.7.4
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [XEN][RFC PATCH v2 11/12] tools/libs/light: Implement new libxl functions for device tree overlay ops
2021-11-09 7:02 ` [XEN][RFC PATCH v2 11/12] tools/libs/light: Implement new libxl functions for device tree overlay ops Vikram Garhwal
@ 2021-11-11 16:44 ` Anthony PERARD
0 siblings, 0 replies; 28+ messages in thread
From: Anthony PERARD @ 2021-11-11 16:44 UTC (permalink / raw)
To: Vikram Garhwal
Cc: xen-devel, sstabellini, julien, bertrand.marquis,
volodymyr_babchuk, Ian Jackson, Wei Liu, Juergen Gross
On Mon, Nov 08, 2021 at 11:02:26PM -0800, Vikram Garhwal wrote:
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
> tools/include/libxl.h | 5 ++++
> tools/libs/light/Makefile | 3 ++
> tools/libs/light/libxl_overlay.c | 65 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 73 insertions(+)
> create mode 100644 tools/libs/light/libxl_overlay.c
>
> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> index 2e8679d..3dcb3e7 100644
> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
> @@ -2406,6 +2406,11 @@ 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);
>
> +#if defined (CONFIG_OVERLAY_DTB)
> +int libxl_dt_overlay(libxl_ctx *ctx, void *overlay,
> + int overlay_size, uint8_t op);
> +#endif
> +
This won't work. "libxl.h" is a public header to be installed, so
CONFIG_OVERLAY_DTB won't mean anything to other application making use
of libxl.
Instead, can you always provide libxl_dt_overlay() but which would
return ENOSYS when libxl is built without CONFIG_OVERLAY_DTB?
> /*
> * 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 194bc5f..0fffa93 100644
> --- a/tools/libs/light/Makefile
> +++ b/tools/libs/light/Makefile
> @@ -117,6 +117,9 @@ SRCS-y += libxl_genid.c
> SRCS-y += _libxl_types.c
> SRCS-y += libxl_flask.c
> SRCS-y += _libxl_types_internal.c
> +ifeq ($(CONFIG_OVERLAY_DTB),y)
> +SRCS-y += libxl_overlay.o
FYI, the "-y" is so that you can do
SRCS-$(CONFIG_OVERLAY_DTB) += libxl_overlay.o
;-)
> diff --git a/tools/libs/light/libxl_overlay.c b/tools/libs/light/libxl_overlay.c
> new file mode 100644
> index 0000000..d965aee
> --- /dev/null
> +++ b/tools/libs/light/libxl_overlay.c
> @@ -0,0 +1,65 @@
> +/*
> + * 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)
Does "fdt" can have a better type than a plain pointer?
There is a "size" provided, is it the size of the blob pointed by "fdt"?
If so, can the size been the first thing to be check? Surely there is a
minimum size for *fdt.
> +{
> + 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 op)
What is "op"? What the function is supposed to do beside calling libxc?
> +{
> + int rc = 0;
> + GC_INIT(ctx);
The function should have "GC_FREE;" before returning.
> +
> + if (check_overlay_fdt(gc, overlay_dt, overlay_dt_size)) {
> + LOG(ERROR, "Overlay DTB check failed\n");
> + return ERROR_FAIL;
> + } else
> + LOG(DEBUG, "Overlay DTB check passed\n");
FYI, there is no need for "\n" when using LOG().
> +
> + /* We don't need to do xc_interface_open here. */
> + rc = xc_dt_overlay(ctx->xch, overlay_dt, overlay_dt_size, op);
"rc" is reserved for libxl error code, xc_* return value should be
stored in "r" instead.
> +
> + if (rc)
> + LOG(ERROR, "%s: Adding/Removing overlay dtb failed.\n", __func__);
> +
> + return rc;
> +}
> +
Thanks,
--
Anthony PERARD
^ permalink raw reply [flat|nested] 28+ messages in thread
* [XEN][RFC PATCH v2 12/12] tools/xl: Add new xl command overlay
2021-11-09 7:02 [XEN][RFC PATCH v2 00/12] dynamic node programming using overlay dtbo Vikram Garhwal
` (10 preceding siblings ...)
2021-11-09 7:02 ` [XEN][RFC PATCH v2 11/12] tools/libs/light: Implement new libxl functions for device tree overlay ops Vikram Garhwal
@ 2021-11-09 7:02 ` Vikram Garhwal
11 siblings, 0 replies; 28+ messages in thread
From: Vikram Garhwal @ 2021-11-09 7:02 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, bertrand.marquis, volodymyr_babchuk,
Vikram Garhwal, Ian Jackson, Wei Liu, Anthony PERARD
Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
tools/xl/xl.h | 8 ++++++++
tools/xl/xl_cmdtable.c | 8 ++++++++
tools/xl/xl_vmcontrol.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 63 insertions(+)
diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index 7e23f30..2ee75a0 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -98,6 +98,11 @@ struct save_file_header {
#define SAVEFILE_BYTEORDER_VALUE ((uint32_t)0x01020304UL)
+#if defined (CONFIG_OVERLAY_DTB)
+#define XL_DT_OVERLAY_ADD 1
+#define XL_DT_OVERLAY_REMOVE 2
+#endif
+
void save_domain_core_begin(uint32_t domid,
int preserve_domid,
const char *override_config_file,
@@ -140,6 +145,9 @@ 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);
+#if defined (CONFIG_OVERLAY_DTB)
+int main_dt_overlay(int argc, char **argv);
+#endif
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 661323d..dd43920 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -20,6 +20,14 @@
#include "xl.h"
const struct cmd_spec cmd_table[] = {
+#if defined (CONFIG_OVERLAY_DTB)
+ { "overlay",
+ &main_dt_overlay, 1, 1,
+ "Add/Remove a device tree overlay",
+ "add/remove <.dtbo>"
+ "-h print this help\n"
+ },
+#endif
{ "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 435155a..88c8e66 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -1262,6 +1262,53 @@ int main_create(int argc, char **argv)
return 0;
}
+#if defined (CONFIG_OVERLAY_DTB)
+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;
+}
+#endif
/*
* Local variables:
* mode: C
--
2.7.4
^ permalink raw reply related [flat|nested] 28+ messages in thread