* [XEN][RFC PATCH v4 00/16] dynamic node programming using overlay dtbo
@ 2022-12-07 6:15 Vikram Garhwal
2022-12-07 6:15 ` [XEN][RFC PATCH v4 01/16] xen/arm/device: Remove __init from function type Vikram Garhwal
` (5 more replies)
0 siblings, 6 replies; 15+ messages in thread
From: Vikram Garhwal @ 2022-12-07 6:15 UTC (permalink / raw)
To: xen-devel; +Cc: sstabellini, julien, vikram.garhwal
Hi,
This RFC patch series is for introducing dynamic programming i.e. add/remove the
devices during run time. Using "xl dt_overlay" a device can be added/removed
with dtbo.
For adding a node using dynamic programming:
1. flatten device tree overlay node will be added to a fdt
2. Updated fdt will be unflattened to a new dt_host_new
3. Extract the newly added node information from dt_host_new
4. Add the added node under correct parent in original dt_host.
3. Map/Permit interrupt and iomem region as required.
For removing a node:
1. Find the node with given path.
2. Check if the node is used by any of domus. Removes the node only when
it's not used by any domain.
3. Removes IRQ permissions and MMIO access.
5. Find the node in dt_host and delete the device node entry from dt_host.
6. Free the overlay_tracker entry which means free dt_host_new also(created
in adding node step).
Change Log:
v3 -> v4:
Add support for adding node's children.
Add rwlock to dt_host functions.
Corrected fdt size issue when applying overlay into it.
Add memory allocation fail handling for unflatten_device_tree().
Changed xl overlay to xl dt_overlay.
Correct commit messages.
Addressed code issue from v3 review.
v2 -> v3:
Moved overlay functionalities to dt_overlay.c file.
Renamed XEN_SYSCTL_overlay to XEN_SYSCTL_dt_overlay.
Add dt_* prefix to overlay_add/remove_nodes.
Added dtdevs_lock to protect iommu_add_dt_device().
For iommu, moved spin_lock to caller.
Address code issue from v2 review.
v1 -> v2:
Add support for multiple node addition/removal using dtbo.
Replaced fpga-add and fpga-remove with one hypercall overlay_op.
Moved common domain_build.c function to device.c
Add OVERLAY_DTB configuration.
Renamed overlay_get_target() to fdt_overlay_get_target().
Split remove_device patch into two patches.
Moved overlay_add/remove code to sysctl and changed it from domctl to sysctl.
Added all overlay code under CONFIG_OVERLAY_DTB
Renamed all tool domains fpga function to overlay
Addressed code issues from v1 review.
Regards,
Vikram
Vikram Garhwal (16):
xen/arm/device: Remove __init from function type
xen/arm: Add CONFIG_OVERLAY_DTB
libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB.
libfdt: overlay: change overlay_get_target()
xen/device-tree: Add _dt_find_node_by_path() to find nodes in device
tree
xen/smmu: Add remove_device callback for smmu_iommu ops
xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller
xen/iommu: protect iommu_add_dt_device() with dtdevs_lock
xen/iommu: Introduce iommu_remove_dt_device()
asm/smp.h: move cpu related function to asm/cpu.h
common/device_tree: Add rwlock for dt_host
xen/arm: Implement device tree node removal functionalities
xen/arm: Implement device tree node addition functionalities
tools/libs/ctrl: Implement new xc interfaces for dt overlay
tools/libs/light: Implement new libxl functions for device tree
overlay ops
tools/xl: Add new xl command overlay for device tree overlay support
tools/include/libxl.h | 8 +
tools/include/xenctrl.h | 3 +
tools/libs/ctrl/Makefile.common | 1 +
tools/libs/ctrl/xc_dt_overlay.c | 51 ++
tools/libs/light/Makefile | 3 +
tools/libs/light/libxl_dt_overlay.c | 72 +++
tools/xl/xl.h | 1 +
tools/xl/xl_cmdtable.c | 6 +
tools/xl/xl_vmcontrol.c | 48 ++
xen/arch/arm/Kconfig | 5 +
xen/arch/arm/device.c | 145 +++++
xen/arch/arm/domain_build.c | 142 -----
xen/arch/arm/efi/efi-boot.h | 1 +
xen/arch/arm/include/asm/cpu.h | 35 +
xen/arch/arm/include/asm/domain.h | 1 +
xen/arch/arm/include/asm/psci.h | 1 +
xen/arch/arm/include/asm/setup.h | 3 +
xen/arch/arm/include/asm/smp.h | 24 -
xen/common/Makefile | 1 +
xen/common/device_tree.c | 59 +-
xen/common/dt_overlay.c | 876 ++++++++++++++++++++++++++
xen/common/libfdt/Makefile | 4 +
xen/common/libfdt/fdt_overlay.c | 29 +-
xen/common/libfdt/version.lds | 1 +
xen/common/sysctl.c | 5 +
xen/drivers/passthrough/arm/smmu.c | 56 ++
xen/drivers/passthrough/device_tree.c | 70 +-
xen/include/public/sysctl.h | 19 +
xen/include/xen/cpu.h | 4 +
xen/include/xen/device_tree.h | 27 +-
xen/include/xen/dt_overlay.h | 55 ++
xen/include/xen/iommu.h | 2 +
xen/include/xen/libfdt/libfdt.h | 18 +
xen/include/xen/softirq.h | 4 +
34 files changed, 1573 insertions(+), 207 deletions(-)
create mode 100644 tools/libs/ctrl/xc_dt_overlay.c
create mode 100644 tools/libs/light/libxl_dt_overlay.c
create mode 100644 xen/arch/arm/include/asm/cpu.h
create mode 100644 xen/common/dt_overlay.c
create mode 100644 xen/include/xen/dt_overlay.h
--
2.17.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [XEN][RFC PATCH v4 01/16] xen/arm/device: Remove __init from function type
2022-12-07 6:15 [XEN][RFC PATCH v4 00/16] dynamic node programming using overlay dtbo Vikram Garhwal
@ 2022-12-07 6:15 ` Vikram Garhwal
2023-01-20 12:39 ` Michal Orzel
2022-12-07 6:15 ` [XEN][RFC PATCH v4 02/16] xen/arm: Add CONFIG_OVERLAY_DTB Vikram Garhwal
` (4 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Vikram Garhwal @ 2022-12-07 6:15 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, vikram.garhwal, Bertrand Marquis, 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.
unflatten_device_tree(): Add handling of memory allocation failure.
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 <vikram.garhwal@amd.com>
---
xen/arch/arm/device.c | 145 +++++++++++++++++++++++++++++++
xen/arch/arm/domain_build.c | 142 ------------------------------
xen/arch/arm/include/asm/setup.h | 3 +
xen/common/device_tree.c | 27 +++---
xen/include/xen/device_tree.h | 5 ++
5 files changed, 170 insertions(+), 152 deletions(-)
diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 70cd6c1a19..d299c04e62 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -21,6 +21,9 @@
#include <xen/errno.h>
#include <xen/init.h>
#include <xen/lib.h>
+#include <xen/iocap.h>
+#include <asm/domain_build.h>
+#include <asm/setup.h>
extern const struct device_desc _sdevice[], _edevice[];
extern const struct acpi_device_desc _asdevice[], _aedevice[];
@@ -84,6 +87,148 @@ enum device_class device_get_class(const struct dt_device_node *dev)
return DEVICE_UNKNOWN;
}
+int map_irq_to_domain(struct domain *d, unsigned int irq,
+ bool need_mapping, const char *devname)
+{
+ int res;
+
+ res = irq_permit_access(d, irq);
+ if ( res )
+ {
+ printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n",
+ d->domain_id, irq);
+ return res;
+ }
+
+ if ( need_mapping )
+ {
+ /*
+ * Checking the return of vgic_reserve_virq is not
+ * necessary. It should not fail except when we try to map
+ * the IRQ twice. This can legitimately happen if the IRQ is shared
+ */
+ vgic_reserve_virq(d, irq);
+
+ res = route_irq_to_guest(d, irq, irq, devname);
+ if ( res < 0 )
+ {
+ printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
+ irq, d->domain_id);
+ return res;
+ }
+ }
+
+ dt_dprintk(" - IRQ: %u\n", irq);
+ return 0;
+}
+
+int map_range_to_domain(const struct dt_device_node *dev,
+ u64 addr, u64 len, void *data)
+{
+ struct map_range_data *mr_data = data;
+ struct domain *d = mr_data->d;
+ int res;
+
+ /*
+ * reserved-memory regions are RAM carved out for a special purpose.
+ * They are not MMIO and therefore a domain should not be able to
+ * manage them via the IOMEM interface.
+ */
+ if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/",
+ strlen("/reserved-memory/")) != 0 )
+ {
+ res = iomem_permit_access(d, paddr_to_pfn(addr),
+ paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
+ if ( res )
+ {
+ printk(XENLOG_ERR "Unable to permit to dom%d access to"
+ " 0x%"PRIx64" - 0x%"PRIx64"\n",
+ d->domain_id,
+ addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
+ return res;
+ }
+ }
+
+ if ( !mr_data->skip_mapping )
+ {
+ res = map_regions_p2mt(d,
+ gaddr_to_gfn(addr),
+ PFN_UP(len),
+ maddr_to_mfn(addr),
+ mr_data->p2mt);
+
+ if ( res < 0 )
+ {
+ printk(XENLOG_ERR "Unable to map 0x%"PRIx64
+ " - 0x%"PRIx64" in domain %d\n",
+ addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
+ d->domain_id);
+ return res;
+ }
+ }
+
+ dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
+ addr, addr + len, mr_data->p2mt);
+
+ return 0;
+}
+
+/*
+ * 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 4fb5c20b13..acde8e714e 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2229,41 +2229,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)
@@ -2295,57 +2260,6 @@ static int __init map_dt_irq_to_domain(const struct dt_device_node *dev,
return 0;
}
-int __init map_range_to_domain(const struct dt_device_node *dev,
- u64 addr, u64 len, void *data)
-{
- struct map_range_data *mr_data = data;
- struct domain *d = mr_data->d;
- int res;
-
- /*
- * reserved-memory regions are RAM carved out for a special purpose.
- * They are not MMIO and therefore a domain should not be able to
- * manage them via the IOMEM interface.
- */
- if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/",
- strlen("/reserved-memory/")) != 0 )
- {
- res = iomem_permit_access(d, paddr_to_pfn(addr),
- paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
- if ( res )
- {
- printk(XENLOG_ERR "Unable to permit to dom%d access to"
- " 0x%"PRIx64" - 0x%"PRIx64"\n",
- d->domain_id,
- addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
- return res;
- }
- }
-
- if ( !mr_data->skip_mapping )
- {
- res = map_regions_p2mt(d,
- gaddr_to_gfn(addr),
- PFN_UP(len),
- maddr_to_mfn(addr),
- mr_data->p2mt);
-
- if ( res < 0 )
- {
- printk(XENLOG_ERR "Unable to map 0x%"PRIx64
- " - 0x%"PRIx64" in domain %d\n",
- addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
- d->domain_id);
- return res;
- }
- }
-
- dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
- addr, addr + len, mr_data->p2mt);
-
- return 0;
-}
-
/*
* For a node which describes a discoverable bus (such as a PCI bus)
* then we may need to perform additional mappings in order to make
@@ -2373,62 +2287,6 @@ static int __init map_device_children(const struct dt_device_node *dev,
return 0;
}
-/*
- * handle_device_interrupts retrieves the interrupts configuration from
- * a device tree node and maps those interrupts to the target domain.
- *
- * Returns:
- * < 0 error
- * 0 success
- */
-static int __init handle_device_interrupts(struct domain *d,
- struct dt_device_node *dev,
- bool need_mapping)
-{
- unsigned int i, nirq;
- int res;
- struct dt_raw_irq rirq;
-
- nirq = dt_number_of_irq(dev);
-
- /* Give permission and map IRQs */
- for ( i = 0; i < nirq; i++ )
- {
- res = dt_device_get_raw_irq(dev, i, &rirq);
- if ( res )
- {
- printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
- i, dt_node_full_name(dev));
- return res;
- }
-
- /*
- * Don't map IRQ that have no physical meaning
- * ie: IRQ whose controller is not the GIC
- */
- if ( rirq.controller != dt_interrupt_controller )
- {
- dt_dprintk("irq %u not connected to primary controller. Connected to %s\n",
- i, dt_node_full_name(rirq.controller));
- continue;
- }
-
- res = platform_get_irq(dev, i);
- if ( res < 0 )
- {
- printk(XENLOG_ERR "Unable to get irq %u for %s\n",
- i, dt_node_full_name(dev));
- return res;
- }
-
- res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev));
- if ( res )
- return res;
- }
-
- return 0;
-}
-
/*
* For a given device node:
* - Give permission to the guest to manage IRQ and MMIO range
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index fdbf68aadc..ec050848aa 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -163,6 +163,9 @@ void device_tree_get_reg(const __be32 **cell, u32 address_cells,
u32 device_tree_get_u32(const void *fdt, int node,
const char *prop_name, u32 dflt);
+int handle_device_interrupts(struct domain *d, struct dt_device_node *dev,
+ bool need_mapping);
+
int map_range_to_domain(const struct dt_device_node *dev,
u64 addr, u64 len, void *data);
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 6c9712ab7b..6518eff9b0 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -1811,12 +1811,12 @@ int dt_count_phandle_with_args(const struct dt_device_node *np,
* @allnextpp: pointer to ->allnext from last allocated device_node
* @fpsize: Size of the node path up at the current depth.
*/
-static unsigned long __init unflatten_dt_node(const void *fdt,
- unsigned long mem,
- unsigned long *p,
- struct dt_device_node *dad,
- struct dt_device_node ***allnextpp,
- unsigned long fpsize)
+static unsigned long unflatten_dt_node(const void *fdt,
+ unsigned long mem,
+ unsigned long *p,
+ struct dt_device_node *dad,
+ struct dt_device_node ***allnextpp,
+ unsigned long fpsize)
{
struct dt_device_node *np;
struct dt_property *pp, **prev_pp = NULL;
@@ -2047,7 +2047,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
}
/**
- * __unflatten_device_tree - create tree of device_nodes from flat blob
+ * unflatten_device_tree - create tree of device_nodes from flat blob
*
* unflattens a device-tree, creating the
* tree of struct device_node. It also fills the "name" and "type"
@@ -2056,8 +2056,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,
- struct dt_device_node **mynodes)
+int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes)
{
unsigned long start, mem, size;
struct dt_device_node **allnextp = mynodes;
@@ -2079,6 +2078,12 @@ static void __init __unflatten_device_tree(const void *fdt,
/* Allocate memory for the expanded device tree */
mem = (unsigned long)_xmalloc (size + 4, __alignof__(struct dt_device_node));
+ if ( mem == 0 )
+ {
+ printk(XENLOG_ERR "Cannot allocate memory for unflatten device tree\n");
+ return -ENOMEM;
+ }
+
((__be32 *)mem)[size / 4] = cpu_to_be32(0xdeadbeef);
dt_dprintk(" unflattening %lx...\n", mem);
@@ -2095,6 +2100,8 @@ static void __init __unflatten_device_tree(const void *fdt,
*allnextp = NULL;
dt_dprintk(" <- unflatten_device_tree()\n");
+
+ return 0;
}
static void dt_alias_add(struct dt_alias_prop *ap,
@@ -2179,7 +2186,7 @@ dt_find_interrupt_controller(const struct dt_device_match *matches)
void __init dt_unflatten_host_device_tree(void)
{
- __unflatten_device_tree(device_tree_flattened, &dt_host);
+ unflatten_device_tree(device_tree_flattened, &dt_host);
dt_alias_scan();
}
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index a28937d12a..bde46d7120 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -181,6 +181,11 @@ int device_tree_for_each_node(const void *fdt, int node,
*/
void dt_unflatten_host_device_tree(void);
+/**
+ * unflatten any device tree.
+ */
+int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes);
+
/**
* IRQ translation callback
* TODO: For the moment we assume that we only have ONE
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [XEN][RFC PATCH v4 02/16] xen/arm: Add CONFIG_OVERLAY_DTB
2022-12-07 6:15 [XEN][RFC PATCH v4 00/16] dynamic node programming using overlay dtbo Vikram Garhwal
2022-12-07 6:15 ` [XEN][RFC PATCH v4 01/16] xen/arm/device: Remove __init from function type Vikram Garhwal
@ 2022-12-07 6:15 ` Vikram Garhwal
2023-01-20 12:41 ` Michal Orzel
2023-02-10 19:01 ` Julien Grall
2022-12-07 6:15 ` [XEN][RFC PATCH v4 03/16] libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB Vikram Garhwal
` (3 subsequent siblings)
5 siblings, 2 replies; 15+ messages in thread
From: Vikram Garhwal @ 2022-12-07 6:15 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, vikram.garhwal, Bertrand Marquis, Volodymyr Babchuk
Introduce a config option where the user can enable support for adding/removing
device tree nodes using a device tree binary overlay.
Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
---
xen/arch/arm/Kconfig | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 1fe5faf847..ae2ebf1697 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -52,6 +52,11 @@ config HAS_ITS
bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED
depends on GICV3 && !NEW_VGIC
+config OVERLAY_DTB
+ bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED
+ help
+ Dynamic addition/removal of Xen device tree nodes using a dtbo.
+
config HVM
def_bool y
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [XEN][RFC PATCH v4 03/16] libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB.
2022-12-07 6:15 [XEN][RFC PATCH v4 00/16] dynamic node programming using overlay dtbo Vikram Garhwal
2022-12-07 6:15 ` [XEN][RFC PATCH v4 01/16] xen/arm/device: Remove __init from function type Vikram Garhwal
2022-12-07 6:15 ` [XEN][RFC PATCH v4 02/16] xen/arm: Add CONFIG_OVERLAY_DTB Vikram Garhwal
@ 2022-12-07 6:15 ` Vikram Garhwal
2023-02-10 19:02 ` Julien Grall
2022-12-07 6:15 ` [XEN][RFC PATCH v4 04/16] libfdt: overlay: change overlay_get_target() Vikram Garhwal
` (2 subsequent siblings)
5 siblings, 1 reply; 15+ messages in thread
From: Vikram Garhwal @ 2022-12-07 6:15 UTC (permalink / raw)
To: xen-devel; +Cc: sstabellini, julien, vikram.garhwal
This is done to access fdt library function which are required for adding device
tree overlay nodes for dynamic programming of nodes.
Acked-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
---
xen/common/libfdt/Makefile | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/xen/common/libfdt/Makefile b/xen/common/libfdt/Makefile
index 75aaefa2e3..d50487aa6e 100644
--- a/xen/common/libfdt/Makefile
+++ b/xen/common/libfdt/Makefile
@@ -1,7 +1,11 @@
include $(src)/Makefile.libfdt
SECTIONS := text data $(SPECIAL_DATA_SECTIONS)
+
+# For CONFIG_OVERLAY_DTB, libfdt functionalities will be needed during runtime.
+ifneq ($(CONFIG_OVERLAY_DTB),y)
OBJCOPYFLAGS := $(foreach s,$(SECTIONS),--rename-section .$(s)=.init.$(s))
+endif
obj-y += libfdt.o
nocov-y += libfdt.o
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [XEN][RFC PATCH v4 04/16] libfdt: overlay: change overlay_get_target()
2022-12-07 6:15 [XEN][RFC PATCH v4 00/16] dynamic node programming using overlay dtbo Vikram Garhwal
` (2 preceding siblings ...)
2022-12-07 6:15 ` [XEN][RFC PATCH v4 03/16] libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB Vikram Garhwal
@ 2022-12-07 6:15 ` Vikram Garhwal
2023-01-20 12:44 ` Michal Orzel
2022-12-07 6:15 ` [XEN][RFC PATCH v4 05/16] xen/device-tree: Add _dt_find_node_by_path() to find nodes in device tree Vikram Garhwal
2022-12-07 6:15 ` [XEN][RFC PATCH v4 06/16] xen/smmu: Add remove_device callback for smmu_iommu ops Vikram Garhwal
5 siblings, 1 reply; 15+ messages in thread
From: Vikram Garhwal @ 2022-12-07 6:15 UTC (permalink / raw)
To: xen-devel; +Cc: sstabellini, julien, vikram.garhwal, David Gibson
Rename overlay_get_target() to fdt_overlay_target_offset() and remove static
function type.
This is done to get the target path for the overlay nodes which is very useful
in many cases. For example, Xen hypervisor needs it when applying overlays
because Xen needs to do further processing of the overlay nodes, e.g. mapping of
resources(IRQs and IOMMUs) to other VMs, creation of SMMU pagetables, etc.
Origin: https://github.com/dgibson/dtc 45f3d1a095dd
Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
Message-Id: <1637204036-382159-2-git-send-email-fnu.vikram@xilinx.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
xen/common/libfdt/fdt_overlay.c | 29 +++++++----------------------
xen/common/libfdt/version.lds | 1 +
xen/include/xen/libfdt/libfdt.h | 18 ++++++++++++++++++
3 files changed, 26 insertions(+), 22 deletions(-)
diff --git a/xen/common/libfdt/fdt_overlay.c b/xen/common/libfdt/fdt_overlay.c
index 7b95e2b639..acf0c4c2a6 100644
--- a/xen/common/libfdt/fdt_overlay.c
+++ b/xen/common/libfdt/fdt_overlay.c
@@ -41,37 +41,22 @@ static uint32_t overlay_get_target_phandle(const void *fdto, int fragment)
return fdt32_to_cpu(*val);
}
-/**
- * overlay_get_target - retrieves the offset of a fragment's target
- * @fdt: Base device tree blob
- * @fdto: Device tree overlay blob
- * @fragment: node offset of the fragment in the overlay
- * @pathp: pointer which receives the path of the target (or NULL)
- *
- * overlay_get_target() retrieves the target offset in the base
- * device tree of a fragment, no matter how the actual targeting is
- * done (through a phandle or a path)
- *
- * returns:
- * the targeted node offset in the base device tree
- * Negative error code on error
- */
-static int overlay_get_target(const void *fdt, const void *fdto,
- int fragment, char const **pathp)
+int fdt_overlay_target_offset(const void *fdt, const void *fdto,
+ int fragment_offset, char const **pathp)
{
uint32_t phandle;
const char *path = NULL;
int path_len = 0, ret;
/* Try first to do a phandle based lookup */
- phandle = overlay_get_target_phandle(fdto, fragment);
+ phandle = overlay_get_target_phandle(fdto, fragment_offset);
if (phandle == (uint32_t)-1)
return -FDT_ERR_BADPHANDLE;
/* no phandle, try path */
if (!phandle) {
/* And then a path based lookup */
- path = fdt_getprop(fdto, fragment, "target-path", &path_len);
+ path = fdt_getprop(fdto, fragment_offset, "target-path", &path_len);
if (path)
ret = fdt_path_offset(fdt, path);
else
@@ -638,7 +623,7 @@ static int overlay_merge(void *fdt, void *fdto)
if (overlay < 0)
return overlay;
- target = overlay_get_target(fdt, fdto, fragment, NULL);
+ target = fdt_overlay_target_offset(fdt, fdto, fragment, NULL);
if (target < 0)
return target;
@@ -781,7 +766,7 @@ static int overlay_symbol_update(void *fdt, void *fdto)
return -FDT_ERR_BADOVERLAY;
/* get the target of the fragment */
- ret = overlay_get_target(fdt, fdto, fragment, &target_path);
+ ret = fdt_overlay_target_offset(fdt, fdto, fragment, &target_path);
if (ret < 0)
return ret;
target = ret;
@@ -803,7 +788,7 @@ static int overlay_symbol_update(void *fdt, void *fdto)
if (!target_path) {
/* again in case setprop_placeholder changed it */
- ret = overlay_get_target(fdt, fdto, fragment, &target_path);
+ ret = fdt_overlay_target_offset(fdt, fdto, fragment, &target_path);
if (ret < 0)
return ret;
target = ret;
diff --git a/xen/common/libfdt/version.lds b/xen/common/libfdt/version.lds
index 7ab85f1d9d..cbce5d4a8b 100644
--- a/xen/common/libfdt/version.lds
+++ b/xen/common/libfdt/version.lds
@@ -77,6 +77,7 @@ LIBFDT_1.2 {
fdt_appendprop_addrrange;
fdt_setprop_inplace_namelen_partial;
fdt_create_with_flags;
+ fdt_overlay_target_offset;
local:
*;
};
diff --git a/xen/include/xen/libfdt/libfdt.h b/xen/include/xen/libfdt/libfdt.h
index c71689e2be..fabddbee8c 100644
--- a/xen/include/xen/libfdt/libfdt.h
+++ b/xen/include/xen/libfdt/libfdt.h
@@ -2109,6 +2109,24 @@ int fdt_del_node(void *fdt, int nodeoffset);
*/
int fdt_overlay_apply(void *fdt, void *fdto);
+/**
+ * fdt_overlay_target_offset - retrieves the offset of a fragment's target
+ * @fdt: Base device tree blob
+ * @fdto: Device tree overlay blob
+ * @fragment_offset: node offset of the fragment in the overlay
+ * @pathp: pointer which receives the path of the target (or NULL)
+ *
+ * fdt_overlay_target_offset() retrieves the target offset in the base
+ * device tree of a fragment, no matter how the actual targeting is
+ * done (through a phandle or a path)
+ *
+ * returns:
+ * the targeted node offset in the base device tree
+ * Negative error code on error
+ */
+int fdt_overlay_target_offset(const void *fdt, const void *fdto,
+ int fragment_offset, char const **pathp);
+
/**********************************************************************/
/* Debugging / informational functions */
/**********************************************************************/
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [XEN][RFC PATCH v4 05/16] xen/device-tree: Add _dt_find_node_by_path() to find nodes in device tree
2022-12-07 6:15 [XEN][RFC PATCH v4 00/16] dynamic node programming using overlay dtbo Vikram Garhwal
` (3 preceding siblings ...)
2022-12-07 6:15 ` [XEN][RFC PATCH v4 04/16] libfdt: overlay: change overlay_get_target() Vikram Garhwal
@ 2022-12-07 6:15 ` Vikram Garhwal
2023-01-20 12:50 ` Michal Orzel
2022-12-07 6:15 ` [XEN][RFC PATCH v4 06/16] xen/smmu: Add remove_device callback for smmu_iommu ops Vikram Garhwal
5 siblings, 1 reply; 15+ messages in thread
From: Vikram Garhwal @ 2022-12-07 6:15 UTC (permalink / raw)
To: xen-devel; +Cc: sstabellini, julien, vikram.garhwal
Add _dt_find_by_path() to find a matching node with path for a dt_device_node.
Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
---
xen/common/device_tree.c | 5 +++--
xen/include/xen/device_tree.h | 16 ++++++++++++++--
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 6518eff9b0..acf26a411d 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -358,11 +358,12 @@ 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 *device_tree_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;
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index bde46d7120..51e251b0b4 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -537,13 +537,25 @@ struct dt_device_node *dt_find_node_by_type(struct dt_device_node *from,
struct dt_device_node *dt_find_node_by_alias(const char *alias);
/**
- * dt_find_node_by_path - Find a node matching a full DT path
+ * device_tree_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(const char *path);
+struct dt_device_node *device_tree_find_node_by_path(struct dt_device_node *dt,
+ const char *path);
+/**
+ * dt_find_node_by_path - Find a node matching a full DT path
+ * @path: The full path to match
+ *
+ * Returns a node pointer.
+ */
+static inline struct dt_device_node *dt_find_node_by_path(const char *path)
+{
+ return device_tree_find_node_by_path(dt_host, path);
+}
/**
* dt_find_node_by_gpath - Same as dt_find_node_by_path but retrieve the
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [XEN][RFC PATCH v4 06/16] xen/smmu: Add remove_device callback for smmu_iommu ops
2022-12-07 6:15 [XEN][RFC PATCH v4 00/16] dynamic node programming using overlay dtbo Vikram Garhwal
` (4 preceding siblings ...)
2022-12-07 6:15 ` [XEN][RFC PATCH v4 05/16] xen/device-tree: Add _dt_find_node_by_path() to find nodes in device tree Vikram Garhwal
@ 2022-12-07 6:15 ` Vikram Garhwal
5 siblings, 0 replies; 15+ messages in thread
From: Vikram Garhwal @ 2022-12-07 6:15 UTC (permalink / raw)
To: xen-devel
Cc: sstabellini, julien, vikram.garhwal, Rahul Singh,
Bertrand Marquis, 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 <vikram.garhwal@amd.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
---
xen/drivers/passthrough/arm/smmu.c | 56 ++++++++++++++++++++++++++++++
1 file changed, 56 insertions(+)
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 0a514821b3..14e15f1bc6 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -816,6 +816,19 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
return 0;
}
+static int remove_smmu_master(struct arm_smmu_device *smmu,
+ struct arm_smmu_master *master)
+{
+ if (!smmu->masters.rb_node) {
+ ASSERT_UNREACHABLE();
+ return -ENOENT;
+ }
+
+ rb_erase(&master->node, &smmu->masters);
+
+ return 0;
+}
+
static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
struct device *dev,
struct iommu_fwspec *fwspec)
@@ -853,6 +866,32 @@ static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
return insert_smmu_master(smmu, master);
}
+static int arm_smmu_dt_remove_device_legacy(struct arm_smmu_device *smmu,
+ struct device *dev)
+{
+ struct arm_smmu_master *master;
+ struct device_node *dev_node = dev_get_dev_node(dev);
+ int ret;
+
+ master = find_smmu_master(smmu, dev_node);
+ if (master == NULL) {
+ dev_err(dev,
+ "No registrations found for master device %s\n",
+ dev_node->name);
+ return -EINVAL;
+ }
+
+ ret = remove_smmu_master(smmu, master);
+
+ if (ret)
+ return ret;
+
+ dev_node->is_protected = false;
+
+ kfree(master);
+ return 0;
+}
+
static int register_smmu_master(struct arm_smmu_device *smmu,
struct device *dev,
struct of_phandle_args *masterspec)
@@ -876,6 +915,22 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
fwspec);
}
+static int arm_smmu_dt_remove_device_generic(u8 devfn, struct device *dev)
+{
+ struct arm_smmu_device *smmu;
+ struct iommu_fwspec *fwspec;
+
+ fwspec = dev_iommu_fwspec_get(dev);
+ if (fwspec == NULL)
+ return -ENXIO;
+
+ smmu = find_smmu(fwspec->iommu_dev);
+ if (smmu == NULL)
+ return -ENXIO;
+
+ return arm_smmu_dt_remove_device_legacy(smmu, dev);
+}
+
static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev)
{
struct arm_smmu_device *smmu;
@@ -2858,6 +2913,7 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
.init = arm_smmu_iommu_domain_init,
.hwdom_init = arch_iommu_hwdom_init,
.add_device = arm_smmu_dt_add_device_generic,
+ .remove_device = arm_smmu_dt_remove_device_generic,
.teardown = arm_smmu_iommu_domain_teardown,
.iotlb_flush = arm_smmu_iotlb_flush,
.assign_device = arm_smmu_assign_dev,
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [XEN][RFC PATCH v4 01/16] xen/arm/device: Remove __init from function type
2022-12-07 6:15 ` [XEN][RFC PATCH v4 01/16] xen/arm/device: Remove __init from function type Vikram Garhwal
@ 2023-01-20 12:39 ` Michal Orzel
2023-02-09 20:11 ` Vikram Garhwal
0 siblings, 1 reply; 15+ messages in thread
From: Michal Orzel @ 2023-01-20 12:39 UTC (permalink / raw)
To: Vikram Garhwal, xen-devel
Cc: sstabellini, julien, Bertrand Marquis, Volodymyr Babchuk
Hi Vikram,
On 07/12/2022 07:15, Vikram Garhwal wrote:
>
>
> Change function type of following function to access during runtime:
> 1. map_irq_to_domain()
> 2. handle_device_interrupt()
> 3. map_range_to_domain()
> 4. unflatten_dt_node()
> 5. unflatten_device_tree()
If you do not want to do this at first use, then this should be a separate patch.
>
> Move map_irq_to_domain(), handle_device_interrupt() and map_range_to_domain() to
> device.c.
This should be a separate patch (without removing __init) to make the comparison easier.
>
> unflatten_device_tree(): Add handling of memory allocation failure.
Apart from that you also renamed __unflatten_device_tree to unflatten_device_tree
and you did not mention it.
>
> 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 <vikram.garhwal@amd.com>
> ---
> xen/arch/arm/device.c | 145 +++++++++++++++++++++++++++++++
> xen/arch/arm/domain_build.c | 142 ------------------------------
> xen/arch/arm/include/asm/setup.h | 3 +
> xen/common/device_tree.c | 27 +++---
> xen/include/xen/device_tree.h | 5 ++
> 5 files changed, 170 insertions(+), 152 deletions(-)
>
> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> index 70cd6c1a19..d299c04e62 100644
> --- a/xen/arch/arm/device.c
> +++ b/xen/arch/arm/device.c
> @@ -21,6 +21,9 @@
> #include <xen/errno.h>
> #include <xen/init.h>
> #include <xen/lib.h>
> +#include <xen/iocap.h>
> +#include <asm/domain_build.h>
> +#include <asm/setup.h>
>
> extern const struct device_desc _sdevice[], _edevice[];
> extern const struct acpi_device_desc _asdevice[], _aedevice[];
> @@ -84,6 +87,148 @@ enum device_class device_get_class(const struct dt_device_node *dev)
> return DEVICE_UNKNOWN;
> }
>
> +int map_irq_to_domain(struct domain *d, unsigned int irq,
> + bool need_mapping, const char *devname)
> +{
> + int res;
> +
> + res = irq_permit_access(d, irq);
> + if ( res )
> + {
> + printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n",
> + d->domain_id, irq);
> + return res;
> + }
> +
> + if ( need_mapping )
> + {
> + /*
> + * Checking the return of vgic_reserve_virq is not
> + * necessary. It should not fail except when we try to map
> + * the IRQ twice. This can legitimately happen if the IRQ is shared
> + */
> + vgic_reserve_virq(d, irq);
> +
> + res = route_irq_to_guest(d, irq, irq, devname);
> + if ( res < 0 )
> + {
> + printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
> + irq, d->domain_id);
> + return res;
> + }
> + }
> +
> + dt_dprintk(" - IRQ: %u\n", irq);
> + return 0;
> +}
> +
> +int map_range_to_domain(const struct dt_device_node *dev,
> + u64 addr, u64 len, void *data)
> +{
> + struct map_range_data *mr_data = data;
> + struct domain *d = mr_data->d;
> + int res;
> +
> + /*
> + * reserved-memory regions are RAM carved out for a special purpose.
> + * They are not MMIO and therefore a domain should not be able to
> + * manage them via the IOMEM interface.
> + */
> + if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/",
> + strlen("/reserved-memory/")) != 0 )
> + {
> + res = iomem_permit_access(d, paddr_to_pfn(addr),
> + paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> + if ( res )
> + {
> + printk(XENLOG_ERR "Unable to permit to dom%d access to"
> + " 0x%"PRIx64" - 0x%"PRIx64"\n",
> + d->domain_id,
> + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
> + return res;
> + }
> + }
> +
> + if ( !mr_data->skip_mapping )
> + {
> + res = map_regions_p2mt(d,
> + gaddr_to_gfn(addr),
> + PFN_UP(len),
> + maddr_to_mfn(addr),
> + mr_data->p2mt);
> +
> + if ( res < 0 )
> + {
> + printk(XENLOG_ERR "Unable to map 0x%"PRIx64
> + " - 0x%"PRIx64" in domain %d\n",
> + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
> + d->domain_id);
> + return res;
> + }
> + }
> +
> + dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
> + addr, addr + len, mr_data->p2mt);
> +
> + return 0;
> +}
> +
> +/*
> + * 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 4fb5c20b13..acde8e714e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2229,41 +2229,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;
> -}
If you move map_irq_to_domain from domain_build.c to device.c, then the prototype needs to also
be moved from domain_build.h to setup.h
> -
> static int __init map_dt_irq_to_domain(const struct dt_device_node *dev,
> const struct dt_irq *dt_irq,
> void *data)
> @@ -2295,57 +2260,6 @@ static int __init map_dt_irq_to_domain(const struct dt_device_node *dev,
> return 0;
> }
>
> -int __init map_range_to_domain(const struct dt_device_node *dev,
> - u64 addr, u64 len, void *data)
> -{
> - struct map_range_data *mr_data = data;
> - struct domain *d = mr_data->d;
> - int res;
> -
> - /*
> - * reserved-memory regions are RAM carved out for a special purpose.
> - * They are not MMIO and therefore a domain should not be able to
> - * manage them via the IOMEM interface.
> - */
> - if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/",
> - strlen("/reserved-memory/")) != 0 )
> - {
> - res = iomem_permit_access(d, paddr_to_pfn(addr),
> - paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> - if ( res )
> - {
> - printk(XENLOG_ERR "Unable to permit to dom%d access to"
> - " 0x%"PRIx64" - 0x%"PRIx64"\n",
> - d->domain_id,
> - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
> - return res;
> - }
> - }
> -
> - if ( !mr_data->skip_mapping )
> - {
> - res = map_regions_p2mt(d,
> - gaddr_to_gfn(addr),
> - PFN_UP(len),
> - maddr_to_mfn(addr),
> - mr_data->p2mt);
> -
> - if ( res < 0 )
> - {
> - printk(XENLOG_ERR "Unable to map 0x%"PRIx64
> - " - 0x%"PRIx64" in domain %d\n",
> - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
> - d->domain_id);
> - return res;
> - }
> - }
> -
> - dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
> - addr, addr + len, mr_data->p2mt);
> -
> - return 0;
> -}
> -
> /*
> * For a node which describes a discoverable bus (such as a PCI bus)
> * then we may need to perform additional mappings in order to make
> @@ -2373,62 +2287,6 @@ static int __init map_device_children(const struct dt_device_node *dev,
> return 0;
> }
>
> -/*
> - * handle_device_interrupts retrieves the interrupts configuration from
> - * a device tree node and maps those interrupts to the target domain.
> - *
> - * Returns:
> - * < 0 error
> - * 0 success
> - */
> -static int __init handle_device_interrupts(struct domain *d,
> - struct dt_device_node *dev,
> - bool need_mapping)
> -{
> - unsigned int i, nirq;
> - int res;
> - struct dt_raw_irq rirq;
> -
> - nirq = dt_number_of_irq(dev);
> -
> - /* Give permission and map IRQs */
> - for ( i = 0; i < nirq; i++ )
> - {
> - res = dt_device_get_raw_irq(dev, i, &rirq);
> - if ( res )
> - {
> - printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
> - i, dt_node_full_name(dev));
> - return res;
> - }
> -
> - /*
> - * Don't map IRQ that have no physical meaning
> - * ie: IRQ whose controller is not the GIC
> - */
> - if ( rirq.controller != dt_interrupt_controller )
> - {
> - dt_dprintk("irq %u not connected to primary controller. Connected to %s\n",
> - i, dt_node_full_name(rirq.controller));
> - continue;
> - }
> -
> - res = platform_get_irq(dev, i);
> - if ( res < 0 )
> - {
> - printk(XENLOG_ERR "Unable to get irq %u for %s\n",
> - i, dt_node_full_name(dev));
> - return res;
> - }
> -
> - res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev));
> - if ( res )
> - return res;
> - }
> -
> - return 0;
> -}
> -
> /*
> * For a given device node:
> * - Give permission to the guest to manage IRQ and MMIO range
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index fdbf68aadc..ec050848aa 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -163,6 +163,9 @@ void device_tree_get_reg(const __be32 **cell, u32 address_cells,
> u32 device_tree_get_u32(const void *fdt, int node,
> const char *prop_name, u32 dflt);
>
> +int handle_device_interrupts(struct domain *d, struct dt_device_node *dev,
> + bool need_mapping);
> +
> int map_range_to_domain(const struct dt_device_node *dev,
> u64 addr, u64 len, void *data);
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 6c9712ab7b..6518eff9b0 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -1811,12 +1811,12 @@ int dt_count_phandle_with_args(const struct dt_device_node *np,
> * @allnextpp: pointer to ->allnext from last allocated device_node
> * @fpsize: Size of the node path up at the current depth.
> */
> -static unsigned long __init unflatten_dt_node(const void *fdt,
> - unsigned long mem,
> - unsigned long *p,
> - struct dt_device_node *dad,
> - struct dt_device_node ***allnextpp,
> - unsigned long fpsize)
> +static unsigned long unflatten_dt_node(const void *fdt,
> + unsigned long mem,
> + unsigned long *p,
> + struct dt_device_node *dad,
> + struct dt_device_node ***allnextpp,
> + unsigned long fpsize)
> {
> struct dt_device_node *np;
> struct dt_property *pp, **prev_pp = NULL;
> @@ -2047,7 +2047,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
> }
>
> /**
> - * __unflatten_device_tree - create tree of device_nodes from flat blob
> + * unflatten_device_tree - create tree of device_nodes from flat blob
> *
> * unflattens a device-tree, creating the
> * tree of struct device_node. It also fills the "name" and "type"
> @@ -2056,8 +2056,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,
> - struct dt_device_node **mynodes)
> +int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes)
> {
> unsigned long start, mem, size;
> struct dt_device_node **allnextp = mynodes;
> @@ -2079,6 +2078,12 @@ static void __init __unflatten_device_tree(const void *fdt,
> /* Allocate memory for the expanded device tree */
> mem = (unsigned long)_xmalloc (size + 4, __alignof__(struct dt_device_node));
>
> + if ( mem == 0 )
NIT: !mem would be preffered.
> + {
> + printk(XENLOG_ERR "Cannot allocate memory for unflatten device tree\n");
> + return -ENOMEM;
What is the point of modifying the function to return a value if ...
> + }
> +
> ((__be32 *)mem)[size / 4] = cpu_to_be32(0xdeadbeef);
>
> dt_dprintk(" unflattening %lx...\n", mem);
> @@ -2095,6 +2100,8 @@ static void __init __unflatten_device_tree(const void *fdt,
> *allnextp = NULL;
>
> dt_dprintk(" <- unflatten_device_tree()\n");
> +
> + return 0;
> }
>
> static void dt_alias_add(struct dt_alias_prop *ap,
> @@ -2179,7 +2186,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);
... you do not check it anyway?
> dt_alias_scan();
> }
>
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index a28937d12a..bde46d7120 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -181,6 +181,11 @@ int device_tree_for_each_node(const void *fdt, int node,
> */
> void dt_unflatten_host_device_tree(void);
>
> +/**
> + * unflatten any device tree.
> + */
> +int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes);
> +
> /**
> * IRQ translation callback
> * TODO: For the moment we assume that we only have ONE
> --
> 2.17.1
>
>
~Michal
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [XEN][RFC PATCH v4 02/16] xen/arm: Add CONFIG_OVERLAY_DTB
2022-12-07 6:15 ` [XEN][RFC PATCH v4 02/16] xen/arm: Add CONFIG_OVERLAY_DTB Vikram Garhwal
@ 2023-01-20 12:41 ` Michal Orzel
2023-02-10 19:01 ` Julien Grall
1 sibling, 0 replies; 15+ messages in thread
From: Michal Orzel @ 2023-01-20 12:41 UTC (permalink / raw)
To: Vikram Garhwal, xen-devel
Cc: sstabellini, julien, Bertrand Marquis, Volodymyr Babchuk
Hi Vikram,
On 07/12/2022 07:15, Vikram Garhwal wrote:
>
>
> Introduce a config option where the user can enable support for adding/removing
> device tree nodes using a device tree binary overlay.
>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> ---
> xen/arch/arm/Kconfig | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 1fe5faf847..ae2ebf1697 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -52,6 +52,11 @@ config HAS_ITS
> bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED
> depends on GICV3 && !NEW_VGIC
>
> +config OVERLAY_DTB
> + bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED
> + help
> + Dynamic addition/removal of Xen device tree nodes using a dtbo.
help text should be indented by a tab and 2 spaces.
> +
> config HVM
> def_bool y
>
> --
> 2.17.1
>
>
~Michal
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [XEN][RFC PATCH v4 04/16] libfdt: overlay: change overlay_get_target()
2022-12-07 6:15 ` [XEN][RFC PATCH v4 04/16] libfdt: overlay: change overlay_get_target() Vikram Garhwal
@ 2023-01-20 12:44 ` Michal Orzel
0 siblings, 0 replies; 15+ messages in thread
From: Michal Orzel @ 2023-01-20 12:44 UTC (permalink / raw)
To: Vikram Garhwal, xen-devel; +Cc: sstabellini, julien, David Gibson
Hi Vikram,
On 07/12/2022 07:15, Vikram Garhwal wrote:
>
>
> Rename overlay_get_target() to fdt_overlay_target_offset() and remove static
> function type.
>
> This is done to get the target path for the overlay nodes which is very useful
> in many cases. For example, Xen hypervisor needs it when applying overlays
> because Xen needs to do further processing of the overlay nodes, e.g. mapping of
> resources(IRQs and IOMMUs) to other VMs, creation of SMMU pagetables, etc.
>
> Origin: https://github.com/dgibson/dtc 45f3d1a095dd
You should move the Origin tag after all the tags coming from the original patch.
As per "sending-patches.pandoc:
"All tags **above** the `Origin:` tag are from the original patch (which
should all be kept), while tags **after** `Origin:` are related to the
normal Xen patch process as described here."
>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> Message-Id: <1637204036-382159-2-git-send-email-fnu.vikram@xilinx.com>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
~Michal
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [XEN][RFC PATCH v4 05/16] xen/device-tree: Add _dt_find_node_by_path() to find nodes in device tree
2022-12-07 6:15 ` [XEN][RFC PATCH v4 05/16] xen/device-tree: Add _dt_find_node_by_path() to find nodes in device tree Vikram Garhwal
@ 2023-01-20 12:50 ` Michal Orzel
0 siblings, 0 replies; 15+ messages in thread
From: Michal Orzel @ 2023-01-20 12:50 UTC (permalink / raw)
To: Vikram Garhwal, xen-devel; +Cc: sstabellini, julien
Hi Vikram,
On 07/12/2022 07:15, Vikram Garhwal wrote:
>
>
> Add _dt_find_by_path() to find a matching node with path for a dt_device_node.
Here and in commit title you say _dt_find_by_path but you introduce device_tree_find_node_by_path.
Also, it would be beneficial to state why such change is needed.
>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> ---
> xen/common/device_tree.c | 5 +++--
> xen/include/xen/device_tree.h | 16 ++++++++++++++--
> 2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 6518eff9b0..acf26a411d 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -358,11 +358,12 @@ 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 *device_tree_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;
>
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index bde46d7120..51e251b0b4 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -537,13 +537,25 @@ struct dt_device_node *dt_find_node_by_type(struct dt_device_node *from,
> struct dt_device_node *dt_find_node_by_alias(const char *alias);
>
> /**
> - * dt_find_node_by_path - Find a node matching a full DT path
> + * device_tree_find_node_by_path - Find a node matching a full DT path
This description and ...
> + * @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(const char *path);
> +struct dt_device_node *device_tree_find_node_by_path(struct dt_device_node *dt,
> + const char *path);
>
> +/**
> + * dt_find_node_by_path - Find a node matching a full DT path
... this are identical. I think you should describe the difference.
The function names are also very similar and can be confused but I won't oppose it.
I will leave the decision to maintainers.
> + * @path: The full path to match
> + *
> + * Returns a node pointer.
> + */
> +static inline struct dt_device_node *dt_find_node_by_path(const char *path)
> +{
> + return device_tree_find_node_by_path(dt_host, path);
> +}
>
> /**
> * dt_find_node_by_gpath - Same as dt_find_node_by_path but retrieve the
> --
> 2.17.1
>
>
~Michal
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [XEN][RFC PATCH v4 01/16] xen/arm/device: Remove __init from function type
2023-01-20 12:39 ` Michal Orzel
@ 2023-02-09 20:11 ` Vikram Garhwal
2023-02-10 19:00 ` Julien Grall
0 siblings, 1 reply; 15+ messages in thread
From: Vikram Garhwal @ 2023-02-09 20:11 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: sstabellini, julien, Bertrand Marquis, Volodymyr Babchuk
Hi Michal,
Thanks for reviewing this. Please see my comments below.
On 1/20/23 4:39 AM, Michal Orzel wrote:
> Hi Vikram,
>
> On 07/12/2022 07:15, Vikram Garhwal wrote:
>>
>> Change function type of following function to access during runtime:
>> 1. map_irq_to_domain()
>> 2. handle_device_interrupt()
>> 3. map_range_to_domain()
>> 4. unflatten_dt_node()
>> 5. unflatten_device_tree()
> If you do not want to do this at first use, then this should be a separate patch.
>> Move map_irq_to_domain(), handle_device_interrupt() and map_range_to_domain() to
>> device.c.
> This should be a separate patch (without removing __init) to make the comparison easier.
Okay
>
>> unflatten_device_tree(): Add handling of memory allocation failure.
> Apart from that you also renamed __unflatten_device_tree to unflatten_device_tree
> and you did not mention it.
I will add this in commit.
>
>> 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 <vikram.garhwal@amd.com>
>> ---
>> xen/arch/arm/device.c | 145 +++++++++++++++++++++++++++++++
>> xen/arch/arm/domain_build.c | 142 ------------------------------
>> xen/arch/arm/include/asm/setup.h | 3 +
>> xen/common/device_tree.c | 27 +++---
>> xen/include/xen/device_tree.h | 5 ++
>> 5 files changed, 170 insertions(+), 152 deletions(-)
>>
>> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
>> index 70cd6c1a19..d299c04e62 100644
>> --- a/xen/arch/arm/device.c
>> +++ b/xen/arch/arm/device.c
>> @@ -21,6 +21,9 @@
>> #include <xen/errno.h>
>> #include <xen/init.h>
>> #include <xen/lib.h>
>> +#include <xen/iocap.h>
>> +#include <asm/domain_build.h>
>> +#include <asm/setup.h>
>>
>> extern const struct device_desc _sdevice[], _edevice[];
>> extern const struct acpi_device_desc _asdevice[], _aedevice[];
>> @@ -84,6 +87,148 @@ enum device_class device_get_class(const struct dt_device_node *dev)
>> return DEVICE_UNKNOWN;
>> }
>>
>> +int map_irq_to_domain(struct domain *d, unsigned int irq,
>> + bool need_mapping, const char *devname)
>> +{
>> + int res;
>> +
>> + res = irq_permit_access(d, irq);
>> + if ( res )
>> + {
>> + printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n",
>> + d->domain_id, irq);
>> + return res;
>> + }
>> +
>> + if ( need_mapping )
>> + {
>> + /*
>> + * Checking the return of vgic_reserve_virq is not
>> + * necessary. It should not fail except when we try to map
>> + * the IRQ twice. This can legitimately happen if the IRQ is shared
>> + */
>> + vgic_reserve_virq(d, irq);
>> +
>> + res = route_irq_to_guest(d, irq, irq, devname);
>> + if ( res < 0 )
>> + {
>> + printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
>> + irq, d->domain_id);
>> + return res;
>> + }
>> + }
>> +
>> + dt_dprintk(" - IRQ: %u\n", irq);
>> + return 0;
>> +}
>> +
>> +int map_range_to_domain(const struct dt_device_node *dev,
>> + u64 addr, u64 len, void *data)
>> +{
>> + struct map_range_data *mr_data = data;
>> + struct domain *d = mr_data->d;
>> + int res;
>> +
>> + /*
>> + * reserved-memory regions are RAM carved out for a special purpose.
>> + * They are not MMIO and therefore a domain should not be able to
>> + * manage them via the IOMEM interface.
>> + */
>> + if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/",
>> + strlen("/reserved-memory/")) != 0 )
>> + {
>> + res = iomem_permit_access(d, paddr_to_pfn(addr),
>> + paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
>> + if ( res )
>> + {
>> + printk(XENLOG_ERR "Unable to permit to dom%d access to"
>> + " 0x%"PRIx64" - 0x%"PRIx64"\n",
>> + d->domain_id,
>> + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
>> + return res;
>> + }
>> + }
>> +
>> + if ( !mr_data->skip_mapping )
>> + {
>> + res = map_regions_p2mt(d,
>> + gaddr_to_gfn(addr),
>> + PFN_UP(len),
>> + maddr_to_mfn(addr),
>> + mr_data->p2mt);
>> +
>> + if ( res < 0 )
>> + {
>> + printk(XENLOG_ERR "Unable to map 0x%"PRIx64
>> + " - 0x%"PRIx64" in domain %d\n",
>> + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
>> + d->domain_id);
>> + return res;
>> + }
>> + }
>> +
>> + dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
>> + addr, addr + len, mr_data->p2mt);
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * 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 4fb5c20b13..acde8e714e 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -2229,41 +2229,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;
>> -}
> If you move map_irq_to_domain from domain_build.c to device.c, then the prototype needs to also
> be moved from domain_build.h to setup.h
>
>> -
>> static int __init map_dt_irq_to_domain(const struct dt_device_node *dev,
>> const struct dt_irq *dt_irq,
>> void *data)
>> @@ -2295,57 +2260,6 @@ static int __init map_dt_irq_to_domain(const struct dt_device_node *dev,
>> return 0;
>> }
>>
>> -int __init map_range_to_domain(const struct dt_device_node *dev,
>> - u64 addr, u64 len, void *data)
>> -{
>> - struct map_range_data *mr_data = data;
>> - struct domain *d = mr_data->d;
>> - int res;
>> -
>> - /*
>> - * reserved-memory regions are RAM carved out for a special purpose.
>> - * They are not MMIO and therefore a domain should not be able to
>> - * manage them via the IOMEM interface.
>> - */
>> - if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/",
>> - strlen("/reserved-memory/")) != 0 )
>> - {
>> - res = iomem_permit_access(d, paddr_to_pfn(addr),
>> - paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
>> - if ( res )
>> - {
>> - printk(XENLOG_ERR "Unable to permit to dom%d access to"
>> - " 0x%"PRIx64" - 0x%"PRIx64"\n",
>> - d->domain_id,
>> - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
>> - return res;
>> - }
>> - }
>> -
>> - if ( !mr_data->skip_mapping )
>> - {
>> - res = map_regions_p2mt(d,
>> - gaddr_to_gfn(addr),
>> - PFN_UP(len),
>> - maddr_to_mfn(addr),
>> - mr_data->p2mt);
>> -
>> - if ( res < 0 )
>> - {
>> - printk(XENLOG_ERR "Unable to map 0x%"PRIx64
>> - " - 0x%"PRIx64" in domain %d\n",
>> - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
>> - d->domain_id);
>> - return res;
>> - }
>> - }
>> -
>> - dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
>> - addr, addr + len, mr_data->p2mt);
>> -
>> - return 0;
>> -}
>> -
>> /*
>> * For a node which describes a discoverable bus (such as a PCI bus)
>> * then we may need to perform additional mappings in order to make
>> @@ -2373,62 +2287,6 @@ static int __init map_device_children(const struct dt_device_node *dev,
>> return 0;
>> }
>>
>> -/*
>> - * handle_device_interrupts retrieves the interrupts configuration from
>> - * a device tree node and maps those interrupts to the target domain.
>> - *
>> - * Returns:
>> - * < 0 error
>> - * 0 success
>> - */
>> -static int __init handle_device_interrupts(struct domain *d,
>> - struct dt_device_node *dev,
>> - bool need_mapping)
>> -{
>> - unsigned int i, nirq;
>> - int res;
>> - struct dt_raw_irq rirq;
>> -
>> - nirq = dt_number_of_irq(dev);
>> -
>> - /* Give permission and map IRQs */
>> - for ( i = 0; i < nirq; i++ )
>> - {
>> - res = dt_device_get_raw_irq(dev, i, &rirq);
>> - if ( res )
>> - {
>> - printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
>> - i, dt_node_full_name(dev));
>> - return res;
>> - }
>> -
>> - /*
>> - * Don't map IRQ that have no physical meaning
>> - * ie: IRQ whose controller is not the GIC
>> - */
>> - if ( rirq.controller != dt_interrupt_controller )
>> - {
>> - dt_dprintk("irq %u not connected to primary controller. Connected to %s\n",
>> - i, dt_node_full_name(rirq.controller));
>> - continue;
>> - }
>> -
>> - res = platform_get_irq(dev, i);
>> - if ( res < 0 )
>> - {
>> - printk(XENLOG_ERR "Unable to get irq %u for %s\n",
>> - i, dt_node_full_name(dev));
>> - return res;
>> - }
>> -
>> - res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev));
>> - if ( res )
>> - return res;
>> - }
>> -
>> - return 0;
>> -}
>> -
>> /*
>> * For a given device node:
>> * - Give permission to the guest to manage IRQ and MMIO range
>> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
>> index fdbf68aadc..ec050848aa 100644
>> --- a/xen/arch/arm/include/asm/setup.h
>> +++ b/xen/arch/arm/include/asm/setup.h
>> @@ -163,6 +163,9 @@ void device_tree_get_reg(const __be32 **cell, u32 address_cells,
>> u32 device_tree_get_u32(const void *fdt, int node,
>> const char *prop_name, u32 dflt);
>>
>> +int handle_device_interrupts(struct domain *d, struct dt_device_node *dev,
>> + bool need_mapping);
>> +
>> int map_range_to_domain(const struct dt_device_node *dev,
>> u64 addr, u64 len, void *data);
>>
>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>> index 6c9712ab7b..6518eff9b0 100644
>> --- a/xen/common/device_tree.c
>> +++ b/xen/common/device_tree.c
>> @@ -1811,12 +1811,12 @@ int dt_count_phandle_with_args(const struct dt_device_node *np,
>> * @allnextpp: pointer to ->allnext from last allocated device_node
>> * @fpsize: Size of the node path up at the current depth.
>> */
>> -static unsigned long __init unflatten_dt_node(const void *fdt,
>> - unsigned long mem,
>> - unsigned long *p,
>> - struct dt_device_node *dad,
>> - struct dt_device_node ***allnextpp,
>> - unsigned long fpsize)
>> +static unsigned long unflatten_dt_node(const void *fdt,
>> + unsigned long mem,
>> + unsigned long *p,
>> + struct dt_device_node *dad,
>> + struct dt_device_node ***allnextpp,
>> + unsigned long fpsize)
>> {
>> struct dt_device_node *np;
>> struct dt_property *pp, **prev_pp = NULL;
>> @@ -2047,7 +2047,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt,
>> }
>>
>> /**
>> - * __unflatten_device_tree - create tree of device_nodes from flat blob
>> + * unflatten_device_tree - create tree of device_nodes from flat blob
>> *
>> * unflattens a device-tree, creating the
>> * tree of struct device_node. It also fills the "name" and "type"
>> @@ -2056,8 +2056,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,
>> - struct dt_device_node **mynodes)
>> +int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes)
>> {
>> unsigned long start, mem, size;
>> struct dt_device_node **allnextp = mynodes;
>> @@ -2079,6 +2078,12 @@ static void __init __unflatten_device_tree(const void *fdt,
>> /* Allocate memory for the expanded device tree */
>> mem = (unsigned long)_xmalloc (size + 4, __alignof__(struct dt_device_node));
>>
>> + if ( mem == 0 )
> NIT: !mem would be preffered.
>
>> + {
>> + printk(XENLOG_ERR "Cannot allocate memory for unflatten device tree\n");
>> + return -ENOMEM;
> What is the point of modifying the function to return a value if ...
>> + }
>> +
>> ((__be32 *)mem)[size / 4] = cpu_to_be32(0xdeadbeef);
>>
>> dt_dprintk(" unflattening %lx...\n", mem);
>> @@ -2095,6 +2100,8 @@ static void __init __unflatten_device_tree(const void *fdt,
>> *allnextp = NULL;
>>
>> dt_dprintk(" <- unflatten_device_tree()\n");
>> +
>> + return 0;
>> }
>>
>> static void dt_alias_add(struct dt_alias_prop *ap,
>> @@ -2179,7 +2186,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);
> ... you do not check it anyway?
So, here we kept it same but unflatten_device_tree() return is checked
in dt_overlay.c. Return of this function will be useful in dt_overlay as
we can run out of mem during runtime. Perhaps I will add another comment
about the reasoning for adding this.
>
>> dt_alias_scan();
>> }
>>
>> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
>> index a28937d12a..bde46d7120 100644
>> --- a/xen/include/xen/device_tree.h
>> +++ b/xen/include/xen/device_tree.h
>> @@ -181,6 +181,11 @@ int device_tree_for_each_node(const void *fdt, int node,
>> */
>> void dt_unflatten_host_device_tree(void);
>>
>> +/**
>> + * unflatten any device tree.
>> + */
>> +int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes);
>> +
>> /**
>> * IRQ translation callback
>> * TODO: For the moment we assume that we only have ONE
>> --
>> 2.17.1
>>
>>
> ~Michal
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [XEN][RFC PATCH v4 01/16] xen/arm/device: Remove __init from function type
2023-02-09 20:11 ` Vikram Garhwal
@ 2023-02-10 19:00 ` Julien Grall
0 siblings, 0 replies; 15+ messages in thread
From: Julien Grall @ 2023-02-10 19:00 UTC (permalink / raw)
To: Vikram Garhwal, Michal Orzel, xen-devel
Cc: sstabellini, Bertrand Marquis, Volodymyr Babchuk
On 09/02/2023 20:11, Vikram Garhwal wrote:
> Hi Michal,
>
> Thanks for reviewing this. Please see my comments below.
>
> On 1/20/23 4:39 AM, Michal Orzel wrote:
>> Hi Vikram,
>>
>> On 07/12/2022 07:15, Vikram Garhwal wrote:
>>>
>>> Change function type of following function to access during runtime:
>>> 1. map_irq_to_domain()
>>> 2. handle_device_interrupt()
>>> 3. map_range_to_domain()
>>> 4. unflatten_dt_node()
>>> 5. unflatten_device_tree()
>> If you do not want to do this at first use, then this should be a
>> separate patch.
>>> Move map_irq_to_domain(), handle_device_interrupt() and
>>> map_range_to_domain() to
>>> device.c.
>> This should be a separate patch (without removing __init) to make the
>> comparison easier.
> Okay
>>
>>> unflatten_device_tree(): Add handling of memory allocation failure.
>> Apart from that you also renamed __unflatten_device_tree to
>> unflatten_device_tree
>> and you did not mention it.
> I will add this in commit.
>>
>>> 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 <vikram.garhwal@amd.com>
>>> ---
>>> xen/arch/arm/device.c | 145 +++++++++++++++++++++++++++++++
>>> xen/arch/arm/domain_build.c | 142 ------------------------------
>>> xen/arch/arm/include/asm/setup.h | 3 +
>>> xen/common/device_tree.c | 27 +++---
>>> xen/include/xen/device_tree.h | 5 ++
>>> 5 files changed, 170 insertions(+), 152 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
>>> index 70cd6c1a19..d299c04e62 100644
>>> --- a/xen/arch/arm/device.c
>>> +++ b/xen/arch/arm/device.c
>>> @@ -21,6 +21,9 @@
>>> #include <xen/errno.h>
>>> #include <xen/init.h>
>>> #include <xen/lib.h>
>>> +#include <xen/iocap.h>
>>> +#include <asm/domain_build.h>
>>> +#include <asm/setup.h>
>>>
>>> extern const struct device_desc _sdevice[], _edevice[];
>>> extern const struct acpi_device_desc _asdevice[], _aedevice[];
>>> @@ -84,6 +87,148 @@ enum device_class device_get_class(const struct
>>> dt_device_node *dev)
>>> return DEVICE_UNKNOWN;
>>> }
>>>
>>> +int map_irq_to_domain(struct domain *d, unsigned int irq,
>>> + bool need_mapping, const char *devname)
>>> +{
>>> + int res;
>>> +
>>> + res = irq_permit_access(d, irq);
>>> + if ( res )
>>> + {
>>> + printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ
>>> %u\n",
>>> + d->domain_id, irq);
>>> + return res;
>>> + }
>>> +
>>> + if ( need_mapping )
>>> + {
>>> + /*
>>> + * Checking the return of vgic_reserve_virq is not
>>> + * necessary. It should not fail except when we try to map
>>> + * the IRQ twice. This can legitimately happen if the IRQ is
>>> shared
>>> + */
>>> + vgic_reserve_virq(d, irq);
>>> +
>>> + res = route_irq_to_guest(d, irq, irq, devname);
>>> + if ( res < 0 )
>>> + {
>>> + printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n",
>>> + irq, d->domain_id);
>>> + return res;
>>> + }
>>> + }
>>> +
>>> + dt_dprintk(" - IRQ: %u\n", irq);
>>> + return 0;
>>> +}
>>> +
>>> +int map_range_to_domain(const struct dt_device_node *dev,
>>> + u64 addr, u64 len, void *data)
>>> +{
>>> + struct map_range_data *mr_data = data;
>>> + struct domain *d = mr_data->d;
>>> + int res;
>>> +
>>> + /*
>>> + * reserved-memory regions are RAM carved out for a special
>>> purpose.
>>> + * They are not MMIO and therefore a domain should not be able to
>>> + * manage them via the IOMEM interface.
>>> + */
>>> + if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/",
>>> + strlen("/reserved-memory/")) != 0 )
>>> + {
>>> + res = iomem_permit_access(d, paddr_to_pfn(addr),
>>> + paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
>>> + if ( res )
>>> + {
>>> + printk(XENLOG_ERR "Unable to permit to dom%d access to"
>>> + " 0x%"PRIx64" - 0x%"PRIx64"\n",
>>> + d->domain_id,
>>> + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
>>> + return res;
>>> + }
>>> + }
>>> +
>>> + if ( !mr_data->skip_mapping )
>>> + {
>>> + res = map_regions_p2mt(d,
>>> + gaddr_to_gfn(addr),
>>> + PFN_UP(len),
>>> + maddr_to_mfn(addr),
>>> + mr_data->p2mt);
>>> +
>>> + if ( res < 0 )
>>> + {
>>> + printk(XENLOG_ERR "Unable to map 0x%"PRIx64
>>> + " - 0x%"PRIx64" in domain %d\n",
>>> + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
>>> + d->domain_id);
>>> + return res;
>>> + }
>>> + }
>>> +
>>> + dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
>>> + addr, addr + len, mr_data->p2mt);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/*
>>> + * 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 4fb5c20b13..acde8e714e 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -2229,41 +2229,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;
>>> -}
>> If you move map_irq_to_domain from domain_build.c to device.c, then
>> the prototype needs to also
>> be moved from domain_build.h to setup.h
>>
>>> -
>>> static int __init map_dt_irq_to_domain(const struct dt_device_node
>>> *dev,
>>> const struct dt_irq *dt_irq,
>>> void *data)
>>> @@ -2295,57 +2260,6 @@ static int __init map_dt_irq_to_domain(const
>>> struct dt_device_node *dev,
>>> return 0;
>>> }
>>>
>>> -int __init map_range_to_domain(const struct dt_device_node *dev,
>>> - u64 addr, u64 len, void *data)
>>> -{
>>> - struct map_range_data *mr_data = data;
>>> - struct domain *d = mr_data->d;
>>> - int res;
>>> -
>>> - /*
>>> - * reserved-memory regions are RAM carved out for a special
>>> purpose.
>>> - * They are not MMIO and therefore a domain should not be able to
>>> - * manage them via the IOMEM interface.
>>> - */
>>> - if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/",
>>> - strlen("/reserved-memory/")) != 0 )
>>> - {
>>> - res = iomem_permit_access(d, paddr_to_pfn(addr),
>>> - paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
>>> - if ( res )
>>> - {
>>> - printk(XENLOG_ERR "Unable to permit to dom%d access to"
>>> - " 0x%"PRIx64" - 0x%"PRIx64"\n",
>>> - d->domain_id,
>>> - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
>>> - return res;
>>> - }
>>> - }
>>> -
>>> - if ( !mr_data->skip_mapping )
>>> - {
>>> - res = map_regions_p2mt(d,
>>> - gaddr_to_gfn(addr),
>>> - PFN_UP(len),
>>> - maddr_to_mfn(addr),
>>> - mr_data->p2mt);
>>> -
>>> - if ( res < 0 )
>>> - {
>>> - printk(XENLOG_ERR "Unable to map 0x%"PRIx64
>>> - " - 0x%"PRIx64" in domain %d\n",
>>> - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1,
>>> - d->domain_id);
>>> - return res;
>>> - }
>>> - }
>>> -
>>> - dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
>>> - addr, addr + len, mr_data->p2mt);
>>> -
>>> - return 0;
>>> -}
>>> -
>>> /*
>>> * For a node which describes a discoverable bus (such as a PCI bus)
>>> * then we may need to perform additional mappings in order to make
>>> @@ -2373,62 +2287,6 @@ static int __init map_device_children(const
>>> struct dt_device_node *dev,
>>> return 0;
>>> }
>>>
>>> -/*
>>> - * handle_device_interrupts retrieves the interrupts configuration from
>>> - * a device tree node and maps those interrupts to the target domain.
>>> - *
>>> - * Returns:
>>> - * < 0 error
>>> - * 0 success
>>> - */
>>> -static int __init handle_device_interrupts(struct domain *d,
>>> - struct dt_device_node *dev,
>>> - bool need_mapping)
>>> -{
>>> - unsigned int i, nirq;
>>> - int res;
>>> - struct dt_raw_irq rirq;
>>> -
>>> - nirq = dt_number_of_irq(dev);
>>> -
>>> - /* Give permission and map IRQs */
>>> - for ( i = 0; i < nirq; i++ )
>>> - {
>>> - res = dt_device_get_raw_irq(dev, i, &rirq);
>>> - if ( res )
>>> - {
>>> - printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n",
>>> - i, dt_node_full_name(dev));
>>> - return res;
>>> - }
>>> -
>>> - /*
>>> - * Don't map IRQ that have no physical meaning
>>> - * ie: IRQ whose controller is not the GIC
>>> - */
>>> - if ( rirq.controller != dt_interrupt_controller )
>>> - {
>>> - dt_dprintk("irq %u not connected to primary controller.
>>> Connected to %s\n",
>>> - i, dt_node_full_name(rirq.controller));
>>> - continue;
>>> - }
>>> -
>>> - res = platform_get_irq(dev, i);
>>> - if ( res < 0 )
>>> - {
>>> - printk(XENLOG_ERR "Unable to get irq %u for %s\n",
>>> - i, dt_node_full_name(dev));
>>> - return res;
>>> - }
>>> -
>>> - res = map_irq_to_domain(d, res, need_mapping,
>>> dt_node_name(dev));
>>> - if ( res )
>>> - return res;
>>> - }
>>> -
>>> - return 0;
>>> -}
>>> -
>>> /*
>>> * For a given device node:
>>> * - Give permission to the guest to manage IRQ and MMIO range
>>> diff --git a/xen/arch/arm/include/asm/setup.h
>>> b/xen/arch/arm/include/asm/setup.h
>>> index fdbf68aadc..ec050848aa 100644
>>> --- a/xen/arch/arm/include/asm/setup.h
>>> +++ b/xen/arch/arm/include/asm/setup.h
>>> @@ -163,6 +163,9 @@ void device_tree_get_reg(const __be32 **cell, u32
>>> address_cells,
>>> u32 device_tree_get_u32(const void *fdt, int node,
>>> const char *prop_name, u32 dflt);
>>>
>>> +int handle_device_interrupts(struct domain *d, struct dt_device_node
>>> *dev,
>>> + bool need_mapping);
>>> +
>>> int map_range_to_domain(const struct dt_device_node *dev,
>>> u64 addr, u64 len, void *data);
>>>
>>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
>>> index 6c9712ab7b..6518eff9b0 100644
>>> --- a/xen/common/device_tree.c
>>> +++ b/xen/common/device_tree.c
>>> @@ -1811,12 +1811,12 @@ int dt_count_phandle_with_args(const struct
>>> dt_device_node *np,
>>> * @allnextpp: pointer to ->allnext from last allocated device_node
>>> * @fpsize: Size of the node path up at the current depth.
>>> */
>>> -static unsigned long __init unflatten_dt_node(const void *fdt,
>>> - unsigned long mem,
>>> - unsigned long *p,
>>> - struct dt_device_node
>>> *dad,
>>> - struct dt_device_node
>>> ***allnextpp,
>>> - unsigned long fpsize)
>>> +static unsigned long unflatten_dt_node(const void *fdt,
>>> + unsigned long mem,
>>> + unsigned long *p,
>>> + struct dt_device_node *dad,
>>> + struct dt_device_node
>>> ***allnextpp,
>>> + unsigned long fpsize)
>>> {
>>> struct dt_device_node *np;
>>> struct dt_property *pp, **prev_pp = NULL;
>>> @@ -2047,7 +2047,7 @@ static unsigned long __init
>>> unflatten_dt_node(const void *fdt,
>>> }
>>>
>>> /**
>>> - * __unflatten_device_tree - create tree of device_nodes from flat blob
>>> + * unflatten_device_tree - create tree of device_nodes from flat blob
>>> *
>>> * unflattens a device-tree, creating the
>>> * tree of struct device_node. It also fills the "name" and "type"
>>> @@ -2056,8 +2056,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,
>>> - struct dt_device_node
>>> **mynodes)
>>> +int unflatten_device_tree(const void *fdt, struct dt_device_node
>>> **mynodes)
>>> {
>>> unsigned long start, mem, size;
>>> struct dt_device_node **allnextp = mynodes;
>>> @@ -2079,6 +2078,12 @@ static void __init
>>> __unflatten_device_tree(const void *fdt,
>>> /* Allocate memory for the expanded device tree */
>>> mem = (unsigned long)_xmalloc (size + 4, __alignof__(struct
>>> dt_device_node));
>>>
>>> + if ( mem == 0 )
>> NIT: !mem would be preffered.
>>
>>> + {
>>> + printk(XENLOG_ERR "Cannot allocate memory for unflatten
>>> device tree\n");
>>> + return -ENOMEM;
>> What is the point of modifying the function to return a value if ...
>>> + }
>>> +
>>> ((__be32 *)mem)[size / 4] = cpu_to_be32(0xdeadbeef);
>>>
>>> dt_dprintk(" unflattening %lx...\n", mem);
>>> @@ -2095,6 +2100,8 @@ static void __init
>>> __unflatten_device_tree(const void *fdt,
>>> *allnextp = NULL;
>>>
>>> dt_dprintk(" <- unflatten_device_tree()\n");
>>> +
>>> + return 0;
>>> }
>>>
>>> static void dt_alias_add(struct dt_alias_prop *ap,
>>> @@ -2179,7 +2186,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);
>> ... you do not check it anyway?
> So, here we kept it same but unflatten_device_tree() return is checked
> in dt_overlay.c. Return of this function will be useful in dt_overlay as
> we can run out of mem during runtime. Perhaps I will add another comment
> about the reasoning for adding this.
It is a good practice to check the error return. In this case, I would
call panic() when there is an error.
With that said, the hardening of __unflatten_device_tree() deserve its
own patch.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [XEN][RFC PATCH v4 02/16] xen/arm: Add CONFIG_OVERLAY_DTB
2022-12-07 6:15 ` [XEN][RFC PATCH v4 02/16] xen/arm: Add CONFIG_OVERLAY_DTB Vikram Garhwal
2023-01-20 12:41 ` Michal Orzel
@ 2023-02-10 19:01 ` Julien Grall
1 sibling, 0 replies; 15+ messages in thread
From: Julien Grall @ 2023-02-10 19:01 UTC (permalink / raw)
To: Vikram Garhwal, xen-devel
Cc: sstabellini, Bertrand Marquis, Volodymyr Babchuk
Hi,
On 07/12/2022 06:15, Vikram Garhwal wrote:
> Introduce a config option where the user can enable support for adding/removing
> device tree nodes using a device tree binary overlay.
>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
> ---
> xen/arch/arm/Kconfig | 5 +++++
You also want to update SUPPORT.md.
> 1 file changed, 5 insertions(+)
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 1fe5faf847..ae2ebf1697 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -52,6 +52,11 @@ config HAS_ITS
> bool "GICv3 ITS MSI controller support (UNSUPPORTED)" if UNSUPPORTED
> depends on GICV3 && !NEW_VGIC
>
> +config OVERLAY_DTB
> + bool "DTB overlay support (UNSUPPORTED)" if UNSUPPORTED
> + help
> + Dynamic addition/removal of Xen device tree nodes using a dtbo.
> +
> config HVM
> def_bool y
>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [XEN][RFC PATCH v4 03/16] libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB.
2022-12-07 6:15 ` [XEN][RFC PATCH v4 03/16] libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB Vikram Garhwal
@ 2023-02-10 19:02 ` Julien Grall
0 siblings, 0 replies; 15+ messages in thread
From: Julien Grall @ 2023-02-10 19:02 UTC (permalink / raw)
To: Vikram Garhwal, xen-devel; +Cc: sstabellini
Hi,
On 07/12/2022 06:15, Vikram Garhwal wrote:
> This is done to access fdt library function which are required for adding device
> tree overlay nodes for dynamic programming of nodes.
>
> Acked-by: Julien Grall <jgrall@amazon.com>
> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
The tags are usually ordered chronologically: you first wrote the patch
and then I acked. So please switch the two tags.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2023-02-10 19:02 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-07 6:15 [XEN][RFC PATCH v4 00/16] dynamic node programming using overlay dtbo Vikram Garhwal
2022-12-07 6:15 ` [XEN][RFC PATCH v4 01/16] xen/arm/device: Remove __init from function type Vikram Garhwal
2023-01-20 12:39 ` Michal Orzel
2023-02-09 20:11 ` Vikram Garhwal
2023-02-10 19:00 ` Julien Grall
2022-12-07 6:15 ` [XEN][RFC PATCH v4 02/16] xen/arm: Add CONFIG_OVERLAY_DTB Vikram Garhwal
2023-01-20 12:41 ` Michal Orzel
2023-02-10 19:01 ` Julien Grall
2022-12-07 6:15 ` [XEN][RFC PATCH v4 03/16] libfdt: Keep fdt functions after init for CONFIG_OVERLAY_DTB Vikram Garhwal
2023-02-10 19:02 ` Julien Grall
2022-12-07 6:15 ` [XEN][RFC PATCH v4 04/16] libfdt: overlay: change overlay_get_target() Vikram Garhwal
2023-01-20 12:44 ` Michal Orzel
2022-12-07 6:15 ` [XEN][RFC PATCH v4 05/16] xen/device-tree: Add _dt_find_node_by_path() to find nodes in device tree Vikram Garhwal
2023-01-20 12:50 ` Michal Orzel
2022-12-07 6:15 ` [XEN][RFC PATCH v4 06/16] xen/smmu: Add remove_device callback for smmu_iommu ops Vikram Garhwal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.