[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [XEN][RFC PATCH v4 01/16] xen/arm/device: Remove __init from function type
- To: Michal Orzel <michal.orzel@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
- From: Vikram Garhwal <vikram.garhwal@xxxxxxx>
- Date: Thu, 9 Feb 2023 12:11:36 -0800
- Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none
- Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=OFGY06zPTNqnKGYkvDoBvSMwGEqziDiz8aErmM1+Bp0=; b=BOdDbXb5uTJy4XfACMclP6oGXkrhqDzz1NSXF/aTEZre4LM8IvFO+WfBFx48hzxnBtkJ5Vk+osNznKWL4DiRYjpQ9DOAvqZ8cOZ91bWSY0l3UfDZvjuF59QO0ufgiz0D6srP9krwVDBRl6/hvhChoPngIZN7+xUdjlE0Mkt1ARjtFCi/PEXmBjUTlxJcGUgaisDaPqddjjjIEFFCsNHpx9b/0EiS43vHb3xJOPgnU8C3kTRFJTARZzhQvkdtA0kvEpoGu5sDLb84w/JD0L7QBADvnrFwCmB4/L7LcpMtEFowQi+dotFWMO/HYL/os9FySD3DYp9q2HX51MYl9w3d0Q==
- Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=KVJNTnU7QoMO6jp/TvQ5JkVjtT9iu/Fr66agRDuf87I89iO4BO1ROeh7Cx0L8uYTbkPCSI1J39Fqcjlm/T5fgg59h3GIV6+qXXRDLex9sa6bYwWstHWfgFm1WTHIub8f9UYKoFg0s0F9jo3td1LI79ka4QwlGKd1pd3FKhK1mRWSBYfdmGM2On2nNov+Kh06JxVTKisPko0Jgq9yGo5SdjC/EtY98FQRrQaMc/6TzxHgbQTyGiCHShHSY5ojlGeJ+O/cPEGYnrKgu/yg7+KOf4et5Qzz35OCUWTvZEU1/oam3ekTSE5t2TjC2v+nbOQIDnDYSD90/9wDyzMwuLEmLg==
- Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amd.com;
- Cc: sstabellini@xxxxxxxxxx, julien@xxxxxxx, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
- Delivery-date: Thu, 09 Feb 2023 20:11:58 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
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@xxxxxxx>
---
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
|
