| Message ID | 20230502233650.20121-17-vikram.garhwal@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dynamic node programming using overlay dtbo | expand |
On 03/05/2023 01:36, Vikram Garhwal wrote: > Update sysctl XEN_SYSCTL_dt_overlay to enable support for dtbo nodes addition > using device tree overlay. > > xl dt-overlay add file.dtbo: > Each time overlay nodes are added using .dtbo, a new fdt(memcpy of > device_tree_flattened) is created and updated with overlay nodes. This > updated fdt is further unflattened to a dt_host_new. Next, it checks if any > of the overlay nodes already exists in the dt_host. If overlay nodes doesn't > exist then find the overlay nodes in dt_host_new, find the overlay node's > parent in dt_host and add the nodes as child under their parent in the > dt_host. The node is attached as the last node under target parent. > > Finally, add IRQs, add device to IOMMUs, set permissions and map MMIO for the > overlay node. > > When a node is added using overlay, a new entry is allocated in the > overlay_track to keep the track of memory allocation due to addition of overlay > node. This is helpful for freeing the memory allocated when a device tree node > is removed. > > The main purpose of this to address first part of dynamic programming i.e. > making xen aware of new device tree node which means updating the dt_host with > overlay node information. Here we are adding/removing node from dt_host, and > checking/setting IOMMU and IRQ permission but never mapping them to any domain. > Right now, mapping/Un-mapping will happen only when a new domU is > created/destroyed using "xl create". > > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> > --- > xen/common/dt-overlay.c | 510 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 510 insertions(+) > > diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c > index b89cceab84..09ea46111b 100644 > --- a/xen/common/dt-overlay.c > +++ b/xen/common/dt-overlay.c > @@ -33,6 +33,25 @@ static struct dt_device_node * > return child_node; > } > > +/* > + * Returns next node to the input node. If node has children then return > + * last descendant's next node. > +*/ > +static struct dt_device_node * > +dt_find_next_node(struct dt_device_node *dt, const struct dt_device_node *node) > +{ > + struct dt_device_node *np; > + > + dt_for_each_device_node(dt, np) > + if ( np == node ) > + break; > + > + if ( np->child ) > + np = find_last_descendants_node(np); > + > + return np->allnext; > +} > + > static int dt_overlay_remove_node(struct dt_device_node *device_node) > { > struct dt_device_node *np; > @@ -106,6 +125,76 @@ static int dt_overlay_remove_node(struct dt_device_node *device_node) > return 0; > } > > +static int dt_overlay_add_node(struct dt_device_node *device_node, > + const char *parent_node_path) > +{ > + struct dt_device_node *parent_node; > + struct dt_device_node *next_node; > + > + parent_node = dt_find_node_by_path(parent_node_path); > + > + if ( parent_node == NULL ) > + { > + dt_dprintk("Parent node %s not found. Overlay node will not be added\n", > + parent_node_path); > + return -EINVAL; > + } > + > + /* If parent has no child. */ > + if ( parent_node->child == NULL ) > + { > + next_node = parent_node->allnext; > + device_node->parent = parent_node; > + parent_node->allnext = device_node; > + parent_node->child = device_node; > + } > + else > + { > + struct dt_device_node *np; > + /* If parent has at least one child node. incorrect comment style, should be: /* * */ > + * Iterate to the last child node of parent. > + */ > + for ( np = parent_node->child; np->sibling != NULL; np = np->sibling ); > + > + /* Iterate over all child nodes of np node. */ > + if ( np->child ) > + { > + struct dt_device_node *np_last_descendant; > + > + np_last_descendant = find_last_descendants_node(np); > + > + next_node = np_last_descendant->allnext; > + np_last_descendant->allnext = device_node; > + } > + else > + { > + next_node = np->allnext; > + np->allnext = device_node; > + } > + > + device_node->parent = parent_node; > + np->sibling = device_node; > + np->sibling->sibling = NULL; > + } > + > + /* Iterate over all child nodes of device_node to add children too. */ > + if ( device_node->child ) > + { > + struct dt_device_node *device_node_last_descendant; > + > + device_node_last_descendant = find_last_descendants_node(device_node); empty line > + /* Plug next_node at the end of last children of device_node. */ > + device_node_last_descendant->allnext = next_node; > + } > + else > + { > + /* Now plug next_node at the end of device_node. */ > + device_node->allnext = next_node; > + } > + > + return 0; > +} > + > /* Basic sanity check for the dtbo tool stack provided to Xen. */ > static int check_overlay_fdt(const void *overlay_fdt, uint32_t overlay_fdt_size) > { > @@ -145,6 +234,82 @@ static unsigned int overlay_node_count(const void *overlay_fdt) > return num_overlay_nodes; > } > > +/* > + * overlay_get_nodes_info gets full name with path for all the nodes which > + * are in one level of __overlay__ tag. This is useful when checking node for > + * duplication i.e. dtbo tries to add nodes which already exists in device tree. > + */ > +static int overlay_get_nodes_info(const void *fdto, char ***nodes_full_path, > + unsigned int num_overlay_nodes) > +{ > + int fragment; > + > + *nodes_full_path = xzalloc_bytes(num_overlay_nodes * sizeof(char *)); You are not freeing memory in this function in case of an error and rely on a caller to free the memory allocated by this function. Wouldn't it be better to allocate this from handle_add_overlay_nodes()? > + > + if ( *nodes_full_path == NULL ) > + return -ENOMEM; > + > + fdt_for_each_subnode(fragment, fdto, 0) > + { > + int target; > + int overlay; > + int subnode; > + const char *target_path; > + > + target = fdt_overlay_target_offset(device_tree_flattened, fdto, > + fragment, &target_path); > + if ( target < 0 ) > + return target; > + > + if ( target_path == NULL ) > + return -EINVAL; > + > + overlay = fdt_subnode_offset(fdto, fragment, "__overlay__"); > + > + /* > + * overlay value can be < 0. But fdt_for_each_subnode() loop checks for > + * overlay >= 0. So, no need for a overlay>=0 check here. > + */ > + fdt_for_each_subnode(subnode, fdto, overlay) > + { > + const char *node_name = NULL; > + int node_name_len; > + unsigned int target_path_len = strlen(target_path); > + unsigned int node_full_name_len; > + unsigned int node_num = 0; How this is supposed to work given that on each iteration you are setting this to 0 and at the end of the iteration you increment it? It will then always be 0. > + > + node_name = fdt_get_name(fdto, subnode, &node_name_len); > + > + if ( node_name == NULL ) > + return node_name_len; > + > + /* > + * Magic number 2 is for adding '/' and '\0'. This is done to keep > + * the node_full_path in the correct full node name format. > + */ > + node_full_name_len = target_path_len + node_name_len + 2; > + > + (*nodes_full_path)[node_num] = xmalloc_bytes(node_full_name_len); > + > + if ( (*nodes_full_path)[node_num] == NULL ) > + return -ENOMEM; > + > + memcpy((*nodes_full_path)[node_num], target_path, target_path_len); > + > + (*nodes_full_path)[node_num][target_path_len] = '/'; > + > + memcpy((*nodes_full_path)[node_num] + target_path_len + 1, > + node_name, node_name_len); > + > + (*nodes_full_path)[node_num][node_full_name_len - 1] = '\0'; > + > + node_num++; > + } > + } > + > + return 0; > +} > + > static int handle_remove_irq_iommu(struct dt_device_node *device_node) > { > int rc = 0; > @@ -371,6 +536,344 @@ out: > return rc; > } > > +/* > + * Handles IRQ and IOMMU mapping for the overlay_node and all descendants of the > + * overlay_node. > + */ > +static int handle_add_irq_iommu(struct domain *d, If you only allow adding nodes to hwdom, what's the point of this parameter? This is not consitent with function that does removing. > + struct dt_device_node *overlay_node) > +{ > + int rc; > + unsigned int naddr, i, len; > + struct dt_device_node *np; > + > + /* First let's handle the interrupts. */ > + rc = handle_device_interrupts(d, overlay_node, false); Similarily to your comment about skip_mapping, it would be welcome to explain why false here. > + if ( rc < 0 ) > + { > + printk(XENLOG_ERR "Failed to retrieve interrupts configuration\n"); > + return rc; > + } > + > + /* Check if iommu property exists. */ > + if ( dt_get_property(overlay_node, "iommus", &len) ) > + { > + /* Add device to IOMMUs. */ > + rc = iommu_add_dt_device(overlay_node); > + if ( rc < 0 ) > + { > + printk(XENLOG_ERR "Failed to add %s to the IOMMU\n", > + dt_node_full_name(overlay_node)); > + return rc; > + } > + } > + > + /* Set permissions. */ > + naddr = dt_number_of_address(overlay_node); > + > + dt_dprintk("%s naddr = %u\n", dt_node_full_name(overlay_node), naddr); > + > + /* Give permission to map MMIOs */ > + for ( i = 0; i < naddr; i++ ) > + { > + uint64_t addr, size; Make sure to check Ayan's 32-bit paddr series status as it impacts parts of your code. > + > + /* > + * For now, we skip_mapping which means it will only permit iomem access > + * to hardware_domain using iomem_permit_access() but will never map as > + * map_range_p2mt() will not be called. > + */ > + struct map_range_data mr_data = { .d = d, > + .p2mt = p2m_mmio_direct_c, > + .skip_mapping = true > + }; > + > + rc = dt_device_get_address(overlay_node, i, &addr, &size); > + if ( rc ) > + { > + printk(XENLOG_ERR "Unable to retrieve address %u for %s\n", > + i, dt_node_full_name(overlay_node)); > + return rc; > + } > + > + rc = map_range_to_domain(overlay_node, addr, size, &mr_data); > + if ( rc ) > + return rc; > + } > + > + /* Map IRQ and IOMMU for overlay_node's children. */ > + for ( np = overlay_node->child; np != NULL; np = np->sibling ) > + { > + rc = handle_add_irq_iommu(d, np); > + if ( rc ) > + { > + printk(XENLOG_ERR "Adding IRQ and IOMMU failed\n"); > + return rc; > + } > + } > + > + return rc; > +} > + > +/* > + * Adds device tree nodes under target node. > + * We use tr->dt_host_new to unflatten the updated device_tree_flattened. This > + * is done to avoid the removal of device_tree generation, iomem regions mapping > + * to hardware domain done by handle_node(). > + */ > +static long handle_add_overlay_nodes(void *overlay_fdt, > + uint32_t overlay_fdt_size) > +{ > + int rc, j, i; > + struct dt_device_node *overlay_node; > + struct overlay_track *tr = NULL; > + char **nodes_full_path = NULL; > + unsigned int new_fdt_size; > + > + tr = xzalloc(struct overlay_track); > + if ( tr == NULL ) > + return -ENOMEM; > + > + new_fdt_size = fdt_totalsize(device_tree_flattened) + > + fdt_totalsize(overlay_fdt); > + > + tr->fdt = xzalloc_bytes(new_fdt_size); > + if ( tr->fdt == NULL ) > + { > + xfree(tr); > + return -ENOMEM; > + } > + > + tr->num_nodes = overlay_node_count(overlay_fdt); > + if ( tr->num_nodes == 0 ) > + { > + xfree(tr->fdt); > + xfree(tr); > + return -ENOMEM; > + } > + > + tr->nodes_address = xzalloc_bytes(tr->num_nodes * sizeof(unsigned long)); > + if ( tr->nodes_address == NULL ) > + { > + xfree(tr->fdt); > + xfree(tr); > + return -ENOMEM; > + } > + > + rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size); > + if ( rc ) > + { > + xfree(tr->nodes_address); > + xfree(tr->fdt); > + xfree(tr); > + return rc; > + } > + > + /* > + * Keep a copy of overlay_fdt as fdt_overlay_apply will change the input > + * overlay's content(magic) when applying overlay. > + */ > + tr->overlay_fdt = xzalloc_bytes(overlay_fdt_size); > + if ( tr->overlay_fdt == NULL ) > + { > + xfree(tr->nodes_address); > + xfree(tr->fdt); > + xfree(tr); > + return -ENOMEM; > + } > + > + memcpy(tr->overlay_fdt, overlay_fdt, overlay_fdt_size); > + > + spin_lock(&overlay_lock); > + > + memcpy(tr->fdt, device_tree_flattened, > + fdt_totalsize(device_tree_flattened)); > + > + /* Open tr->fdt with more space to accommodate the overlay_fdt. */ > + rc = fdt_open_into(tr->fdt, tr->fdt, new_fdt_size); > + if ( rc ) > + { > + printk(XENLOG_ERR "Increasing tr->fdt size failed with error %d\n", User does not know what tr->fdt is and it's better to be more user friendly. > + rc); > + goto err; > + } > + > + /* > + * overlay_get_nodes_info is called to get the node information from dtbo. > + * This is done before fdt_overlay_apply() because the overlay apply will > + * erase the magic of overlay_fdt. > + */ > + rc = overlay_get_nodes_info(overlay_fdt, &nodes_full_path, > + tr->num_nodes); > + if ( rc ) > + { > + printk(XENLOG_ERR "Getting nodes information failed with error %d\n", > + rc); > + goto err; > + } > + > + rc = fdt_overlay_apply(tr->fdt, overlay_fdt); > + if ( rc ) > + { > + printk(XENLOG_ERR "Adding overlay node failed with error %d\n", rc); > + goto err; > + } > + > + /* > + * Check if any of the node already exists in dt_host. If node already exits > + * we can return here as this overlay_fdt is not suitable for overlay ops. > + */ > + for ( j = 0; j < tr->num_nodes; j++ ) > + { > + overlay_node = dt_find_node_by_path(nodes_full_path[j]); > + if ( overlay_node != NULL ) > + { > + printk(XENLOG_ERR "node %s exists in device tree\n", > + nodes_full_path[j]); > + rc = -EINVAL; > + goto err; > + } > + } > + > + /* Unflatten the tr->fdt into a new dt_host. */ > + rc = unflatten_device_tree(tr->fdt, &tr->dt_host_new); > + if ( rc ) > + { > + printk(XENLOG_ERR "unflatten_device_tree failed with error %d\n", rc); > + goto err; > + } > + > + for ( j = 0; j < tr->num_nodes; j++ ) > + { > + struct dt_device_node *prev_node, *next_node; > + > + dt_dprintk("Adding node: %s\n", nodes_full_path[j]); > + > + /* Find the newly added node in tr->dt_host_new by it's full path. */ > + overlay_node = device_tree_find_node_by_path(tr->dt_host_new, > + nodes_full_path[j]); > + if ( overlay_node == NULL ) > + { > + /* Sanity check. But code will never come here. */ > + ASSERT_UNREACHABLE(); > + goto remove_node; > + } > + > + /* > + * Find previous and next node to overlay_node in dt_host_new. We will > + * need these nodes to fix the dt_host_new mapping. When overlay_node is > + * take out of dt_host_new tree and added to dt_host, link between > + * previous node and next_node is broken. We will need to refresh > + * dt_host_new with correct linking for any other overlay nodes > + * extraction in future. > + */ > + dt_for_each_device_node(tr->dt_host_new, prev_node) > + if ( prev_node->allnext == overlay_node ) > + break; > + > + next_node = dt_find_next_node(tr->dt_host_new, overlay_node); > + > + read_lock(&dt_host->lock); > + > + /* Add the node to dt_host. */ > + rc = dt_overlay_add_node(overlay_node, overlay_node->parent->full_name); > + if ( rc ) > + { > + read_unlock(&dt_host->lock); > + > + /* Node not added in dt_host. */ > + goto remove_node; > + } > + > + read_unlock(&dt_host->lock); > + > + prev_node->allnext = next_node; > + > + overlay_node = dt_find_node_by_path(overlay_node->full_name); > + if ( overlay_node == NULL ) > + { > + /* Sanity check. But code will never come here. */ > + ASSERT_UNREACHABLE(); > + goto remove_node; > + } > + > + rc = handle_add_irq_iommu(hardware_domain, overlay_node); > + if ( rc ) > + { > + printk(XENLOG_ERR "Adding IRQ and IOMMU failed\n"); > + return rc; This is definitely wrong as you will return with spinlock held and memory not freed. > + } > + > + /* Keep overlay_node address in tracker. */ > + tr->nodes_address[j] = (unsigned long)overlay_node; > + } > + > + INIT_LIST_HEAD(&tr->entry); > + list_add_tail(&tr->entry, &overlay_tracker); > + > + spin_unlock(&overlay_lock); > + > + if ( nodes_full_path != NULL ) > + { > + for ( i = 0; i < tr->num_nodes && nodes_full_path[i] != NULL; > + i++ ) > + { > + xfree(nodes_full_path[i]); > + } > + xfree(nodes_full_path); > + } This block of code is identical to the one at the end so some cleanup is necessary. > + > + return rc; > + > +/* > + * Failure case. We need to remove the nodes, free tracker(if tr exists) and > + * tr->dt_host_new. > + */ > +remove_node: > + tr->num_nodes = j; > + rc = remove_nodes(tr); > + > + if ( rc ) > + { > + /* > + * User needs to provide right overlay. Incorrect node information > + * example parent node doesn't exist in dt_host etc can cause memory > + * leaks as removing_nodes() will fail and this means nodes memory is > + * not freed from tracker. Which may cause memory leaks. Ideally, these > + * device tree related mistakes will be caught by fdt_overlay_apply() > + * but given that we don't manage that code keeping this warning message > + * is better here. > + */ > + printk(XENLOG_ERR "Removing node failed.\n"); > + spin_unlock(&overlay_lock); > + return rc; > + } > + > +err: > + spin_unlock(&overlay_lock); > + > + if ( tr->dt_host_new ) > + xfree(tr->dt_host_new); > + > + xfree(tr->overlay_fdt); > + xfree(tr->nodes_address); > + xfree(tr->fdt); > + > + if ( nodes_full_path != NULL ) > + { > + for ( i = 0; i < tr->num_nodes && nodes_full_path[i] != NULL; > + i++ ) > + { > + xfree(nodes_full_path[i]); > + } > + xfree(nodes_full_path); > + } > + > + xfree(tr); > + > + return rc; > +} > + > long dt_sysctl(struct xen_sysctl_dt_overlay *op) > { > long ret; > @@ -395,6 +898,13 @@ long dt_sysctl(struct xen_sysctl_dt_overlay *op) > > switch ( op->overlay_op ) > { > + case XEN_SYSCTL_DT_OVERLAY_ADD: > + ret = handle_add_overlay_nodes(overlay_fdt, op->overlay_fdt_size); > + if ( ret ) > + xfree(overlay_fdt); See my comment about xfree in previous patch > + > + break; > + > case XEN_SYSCTL_DT_OVERLAY_REMOVE: > ret = handle_remove_overlay_nodes(overlay_fdt, op->overlay_fdt_size); > xfree(overlay_fdt); ~Michal
Hi Michal, On 5/10/23 3:18 AM, Michal Orzel wrote: > > On 03/05/2023 01:36, Vikram Garhwal wrote: >> Update sysctl XEN_SYSCTL_dt_overlay to enable support for dtbo nodes addition >> using device tree overlay. >> >> xl dt-overlay add file.dtbo: >> Each time overlay nodes are added using .dtbo, a new fdt(memcpy of >> device_tree_flattened) is created and updated with overlay nodes. This >> updated fdt is further unflattened to a dt_host_new. Next, it checks if any >> of the overlay nodes already exists in the dt_host. If overlay nodes doesn't >> exist then find the overlay nodes in dt_host_new, find the overlay node's >> parent in dt_host and add the nodes as child under their parent in the >> dt_host. The node is attached as the last node under target parent. >> >> Finally, add IRQs, add device to IOMMUs, set permissions and map MMIO for the >> overlay node. >> >> When a node is added using overlay, a new entry is allocated in the >> overlay_track to keep the track of memory allocation due to addition of overlay >> node. This is helpful for freeing the memory allocated when a device tree node >> is removed. >> >> The main purpose of this to address first part of dynamic programming i.e. >> making xen aware of new device tree node which means updating the dt_host with >> overlay node information. Here we are adding/removing node from dt_host, and >> checking/setting IOMMU and IRQ permission but never mapping them to any domain. >> Right now, mapping/Un-mapping will happen only when a new domU is >> created/destroyed using "xl create". >> >> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> >> --- >> xen/common/dt-overlay.c | 510 ++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 510 insertions(+) >> >> diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c >> index b89cceab84..09ea46111b 100644 >> --- a/xen/common/dt-overlay.c >> +++ b/xen/common/dt-overlay.c >> @@ -33,6 +33,25 @@ static struct dt_device_node * >> return child_node; >> } >> >> +/* >> + * Returns next node to the input node. If node has children then return >> + * last descendant's next node. >> +*/ >> +static struct dt_device_node * >> +dt_find_next_node(struct dt_device_node *dt, const struct dt_device_node *node) >> +{ >> + struct dt_device_node *np; >> + >> + dt_for_each_device_node(dt, np) >> + if ( np == node ) >> + break; >> + >> + if ( np->child ) >> + np = find_last_descendants_node(np); >> + >> + return np->allnext; >> +} >> + >> static int dt_overlay_remove_node(struct dt_device_node *device_node) >> { >> struct dt_device_node *np; >> @@ -106,6 +125,76 @@ static int dt_overlay_remove_node(struct dt_device_node *device_node) >> return 0; >> } >> >> +static int dt_overlay_add_node(struct dt_device_node *device_node, >> + const char *parent_node_path) >> +{ >> + struct dt_device_node *parent_node; >> + struct dt_device_node *next_node; >> + >> + parent_node = dt_find_node_by_path(parent_node_path); >> + >> + if ( parent_node == NULL ) >> + { >> + dt_dprintk("Parent node %s not found. Overlay node will not be added\n", >> + parent_node_path); >> + return -EINVAL; >> + } >> + >> + /* If parent has no child. */ >> + if ( parent_node->child == NULL ) >> + { >> + next_node = parent_node->allnext; >> + device_node->parent = parent_node; >> + parent_node->allnext = device_node; >> + parent_node->child = device_node; >> + } >> + else >> + { >> + struct dt_device_node *np; >> + /* If parent has at least one child node. > incorrect comment style, should be: > /* > * > */ Changed this in v7. > >> + * Iterate to the last child node of parent. >> + */ >> + for ( np = parent_node->child; np->sibling != NULL; np = np->sibling ); >> + >> + /* Iterate over all child nodes of np node. */ >> + if ( np->child ) >> + { >> + struct dt_device_node *np_last_descendant; >> + >> + np_last_descendant = find_last_descendants_node(np); >> + >> + next_node = np_last_descendant->allnext; >> + np_last_descendant->allnext = device_node; >> + } >> + else >> + { >> + next_node = np->allnext; >> + np->allnext = device_node; >> + } >> + >> + device_node->parent = parent_node; >> + np->sibling = device_node; >> + np->sibling->sibling = NULL; >> + } >> + >> + /* Iterate over all child nodes of device_node to add children too. */ >> + if ( device_node->child ) >> + { >> + struct dt_device_node *device_node_last_descendant; >> + >> + device_node_last_descendant = find_last_descendants_node(device_node); > empty line > >> + /* Plug next_node at the end of last children of device_node. */ >> + device_node_last_descendant->allnext = next_node; >> + } >> + else >> + { >> + /* Now plug next_node at the end of device_node. */ >> + device_node->allnext = next_node; >> + } >> + >> + return 0; >> +} >> + >> /* Basic sanity check for the dtbo tool stack provided to Xen. */ >> static int check_overlay_fdt(const void *overlay_fdt, uint32_t overlay_fdt_size) >> { >> @@ -145,6 +234,82 @@ static unsigned int overlay_node_count(const void *overlay_fdt) >> return num_overlay_nodes; >> } >> >> +/* >> + * overlay_get_nodes_info gets full name with path for all the nodes which >> + * are in one level of __overlay__ tag. This is useful when checking node for >> + * duplication i.e. dtbo tries to add nodes which already exists in device tree. >> + */ >> +static int overlay_get_nodes_info(const void *fdto, char ***nodes_full_path, >> + unsigned int num_overlay_nodes) >> +{ >> + int fragment; >> + >> + *nodes_full_path = xzalloc_bytes(num_overlay_nodes * sizeof(char *)); > You are not freeing memory in this function in case of an error and rely on a caller > to free the memory allocated by this function. Wouldn't it be better to allocate this > from handle_add_overlay_nodes()? Changed this in v7. > >> + >> + if ( *nodes_full_path == NULL ) >> + return -ENOMEM; >> + >> + fdt_for_each_subnode(fragment, fdto, 0) >> + { >> + int target; >> + int overlay; >> + int subnode; >> + const char *target_path; >> + >> + target = fdt_overlay_target_offset(device_tree_flattened, fdto, >> + fragment, &target_path); >> + if ( target < 0 ) >> + return target; >> + >> + if ( target_path == NULL ) >> + return -EINVAL; >> + >> + overlay = fdt_subnode_offset(fdto, fragment, "__overlay__"); >> + >> + /* >> + * overlay value can be < 0. But fdt_for_each_subnode() loop checks for >> + * overlay >= 0. So, no need for a overlay>=0 check here. >> + */ >> + fdt_for_each_subnode(subnode, fdto, overlay) >> + { >> + const char *node_name = NULL; >> + int node_name_len; >> + unsigned int target_path_len = strlen(target_path); >> + unsigned int node_full_name_len; >> + unsigned int node_num = 0; > How this is supposed to work given that on each iteration you are setting this to 0 > and at the end of the iteration you increment it? It will then always be 0. Yeah, this will work only for one node. I moved it here because of a comment in v4 but it's wrong. Corrected it now. Thanks for catching this! >> + >> + node_name = fdt_get_name(fdto, subnode, &node_name_len); >> + >> + if ( node_name == NULL ) >> + return node_name_len; >> + >> + /* >> + * Magic number 2 is for adding '/' and '\0'. This is done to keep >> + * the node_full_path in the correct full node name format. >> + */ >> + node_full_name_len = target_path_len + node_name_len + 2; >> + >> + (*nodes_full_path)[node_num] = xmalloc_bytes(node_full_name_len); >> + >> + if ( (*nodes_full_path)[node_num] == NULL ) >> + return -ENOMEM; >> + >> + memcpy((*nodes_full_path)[node_num], target_path, target_path_len); >> + >> + (*nodes_full_path)[node_num][target_path_len] = '/'; >> + >> + memcpy((*nodes_full_path)[node_num] + target_path_len + 1, >> + node_name, node_name_len); >> + >> + (*nodes_full_path)[node_num][node_full_name_len - 1] = '\0'; >> + >> + node_num++; >> + } >> + } >> + >> + return 0; >> +} >> + >> static int handle_remove_irq_iommu(struct dt_device_node *device_node) >> { >> int rc = 0; >> @@ -371,6 +536,344 @@ out: >> return rc; >> } >> >> +/* >> + * Handles IRQ and IOMMU mapping for the overlay_node and all descendants of the >> + * overlay_node. >> + */ >> +static int handle_add_irq_iommu(struct domain *d, > If you only allow adding nodes to hwdom, what's the point of this parameter? > This is not consitent with function that does removing. Remove domain *d part for this series. This was something added for assigning the node to a running domain but while splitting the series into two part(check 00/19 patch) i didn't remove it. Removed it in all the places. > >> + struct dt_device_node *overlay_node) >> +{ >> + int rc; >> + unsigned int naddr, i, len; >> + struct dt_device_node *np; >> + >> + /* First let's handle the interrupts. */ >> + rc = handle_device_interrupts(d, overlay_node, false); > Similarily to your comment about skip_mapping, it would be welcome to explain why false here. Done! > >> + if ( rc < 0 ) >> + { >> + printk(XENLOG_ERR "Failed to retrieve interrupts configuration\n"); >> + return rc; >> + } >> + >> + /* Check if iommu property exists. */ >> + if ( dt_get_property(overlay_node, "iommus", &len) ) >> + { >> + /* Add device to IOMMUs. */ >> + rc = iommu_add_dt_device(overlay_node); >> + if ( rc < 0 ) >> + { >> + printk(XENLOG_ERR "Failed to add %s to the IOMMU\n", >> + dt_node_full_name(overlay_node)); >> + return rc; >> + } >> + } >> + >> + /* Set permissions. */ >> + naddr = dt_number_of_address(overlay_node); >> + >> + dt_dprintk("%s naddr = %u\n", dt_node_full_name(overlay_node), naddr); >> + >> + /* Give permission to map MMIOs */ >> + for ( i = 0; i < naddr; i++ ) >> + { >> + uint64_t addr, size; > Make sure to check Ayan's 32-bit paddr series status as it impacts parts of your code. So, In staging, I cannot see any change for dt_device_get_address() which we using in these patches. That function still uses uint64_t addr instead of paddr_t. Did i misread your comment? > >> + >> + /* >> + * For now, we skip_mapping which means it will only permit iomem access >> + * to hardware_domain using iomem_permit_access() but will never map as >> + * map_range_p2mt() will not be called. >> + */ >> + struct map_range_data mr_data = { .d = d, >> + .p2mt = p2m_mmio_direct_c, >> + .skip_mapping = true >> + }; >> + >> + rc = dt_device_get_address(overlay_node, i, &addr, &size); >> + if ( rc ) >> + { >> + printk(XENLOG_ERR "Unable to retrieve address %u for %s\n", >> + i, dt_node_full_name(overlay_node)); >> + return rc; >> + } >> + >> + rc = map_range_to_domain(overlay_node, addr, size, &mr_data); >> + if ( rc ) >> + return rc; >> + } >> + >> + /* Map IRQ and IOMMU for overlay_node's children. */ >> + for ( np = overlay_node->child; np != NULL; np = np->sibling ) >> + { >> + rc = handle_add_irq_iommu(d, np); >> + if ( rc ) >> + { >> + printk(XENLOG_ERR "Adding IRQ and IOMMU failed\n"); >> + return rc; >> + } >> + } >> + >> + return rc; >> +} >> + >> +/* >> + * Adds device tree nodes under target node. >> + * We use tr->dt_host_new to unflatten the updated device_tree_flattened. This >> + * is done to avoid the removal of device_tree generation, iomem regions mapping >> + * to hardware domain done by handle_node(). >> + */ >> +static long handle_add_overlay_nodes(void *overlay_fdt, >> + uint32_t overlay_fdt_size) >> +{ >> + int rc, j, i; >> + struct dt_device_node *overlay_node; >> + struct overlay_track *tr = NULL; >> + char **nodes_full_path = NULL; >> + unsigned int new_fdt_size; >> + >> + tr = xzalloc(struct overlay_track); >> + if ( tr == NULL ) >> + return -ENOMEM; >> + >> + new_fdt_size = fdt_totalsize(device_tree_flattened) + >> + fdt_totalsize(overlay_fdt); >> + >> + tr->fdt = xzalloc_bytes(new_fdt_size); >> + if ( tr->fdt == NULL ) >> + { >> + xfree(tr); >> + return -ENOMEM; >> + } >> + >> + tr->num_nodes = overlay_node_count(overlay_fdt); >> + if ( tr->num_nodes == 0 ) >> + { >> + xfree(tr->fdt); >> + xfree(tr); >> + return -ENOMEM; >> + } >> + >> + tr->nodes_address = xzalloc_bytes(tr->num_nodes * sizeof(unsigned long)); >> + if ( tr->nodes_address == NULL ) >> + { >> + xfree(tr->fdt); >> + xfree(tr); >> + return -ENOMEM; >> + } >> + >> + rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size); >> + if ( rc ) >> + { >> + xfree(tr->nodes_address); >> + xfree(tr->fdt); >> + xfree(tr); >> + return rc; >> + } >> + >> + /* >> + * Keep a copy of overlay_fdt as fdt_overlay_apply will change the input >> + * overlay's content(magic) when applying overlay. >> + */ >> + tr->overlay_fdt = xzalloc_bytes(overlay_fdt_size); >> + if ( tr->overlay_fdt == NULL ) >> + { >> + xfree(tr->nodes_address); >> + xfree(tr->fdt); >> + xfree(tr); >> + return -ENOMEM; >> + } >> + >> + memcpy(tr->overlay_fdt, overlay_fdt, overlay_fdt_size); >> + >> + spin_lock(&overlay_lock); >> + >> + memcpy(tr->fdt, device_tree_flattened, >> + fdt_totalsize(device_tree_flattened)); >> + >> + /* Open tr->fdt with more space to accommodate the overlay_fdt. */ >> + rc = fdt_open_into(tr->fdt, tr->fdt, new_fdt_size); >> + if ( rc ) >> + { >> + printk(XENLOG_ERR "Increasing tr->fdt size failed with error %d\n", > User does not know what tr->fdt is and it's better to be more user friendly. Done > >> + rc); >> + goto err; >> + } >> + >> + /* >> + * overlay_get_nodes_info is called to get the node information from dtbo. >> + * This is done before fdt_overlay_apply() because the overlay apply will >> + * erase the magic of overlay_fdt. >> + */ >> + rc = overlay_get_nodes_info(overlay_fdt, &nodes_full_path, >> + tr->num_nodes); >> + if ( rc ) >> + { >> + printk(XENLOG_ERR "Getting nodes information failed with error %d\n", >> + rc); >> + goto err; >> + } >> + >> + rc = fdt_overlay_apply(tr->fdt, overlay_fdt); >> + if ( rc ) >> + { >> + printk(XENLOG_ERR "Adding overlay node failed with error %d\n", rc); >> + goto err; >> + } >> + >> + /* >> + * Check if any of the node already exists in dt_host. If node already exits >> + * we can return here as this overlay_fdt is not suitable for overlay ops. >> + */ >> + for ( j = 0; j < tr->num_nodes; j++ ) >> + { >> + overlay_node = dt_find_node_by_path(nodes_full_path[j]); >> + if ( overlay_node != NULL ) >> + { >> + printk(XENLOG_ERR "node %s exists in device tree\n", >> + nodes_full_path[j]); >> + rc = -EINVAL; >> + goto err; >> + } >> + } >> + >> + /* Unflatten the tr->fdt into a new dt_host. */ >> + rc = unflatten_device_tree(tr->fdt, &tr->dt_host_new); >> + if ( rc ) >> + { >> + printk(XENLOG_ERR "unflatten_device_tree failed with error %d\n", rc); >> + goto err; >> + } >> + >> + for ( j = 0; j < tr->num_nodes; j++ ) >> + { >> + struct dt_device_node *prev_node, *next_node; >> + >> + dt_dprintk("Adding node: %s\n", nodes_full_path[j]); >> + >> + /* Find the newly added node in tr->dt_host_new by it's full path. */ >> + overlay_node = device_tree_find_node_by_path(tr->dt_host_new, >> + nodes_full_path[j]); >> + if ( overlay_node == NULL ) >> + { >> + /* Sanity check. But code will never come here. */ >> + ASSERT_UNREACHABLE(); >> + goto remove_node; >> + } >> + >> + /* >> + * Find previous and next node to overlay_node in dt_host_new. We will >> + * need these nodes to fix the dt_host_new mapping. When overlay_node is >> + * take out of dt_host_new tree and added to dt_host, link between >> + * previous node and next_node is broken. We will need to refresh >> + * dt_host_new with correct linking for any other overlay nodes >> + * extraction in future. >> + */ >> + dt_for_each_device_node(tr->dt_host_new, prev_node) >> + if ( prev_node->allnext == overlay_node ) >> + break; >> + >> + next_node = dt_find_next_node(tr->dt_host_new, overlay_node); >> + >> + read_lock(&dt_host->lock); >> + >> + /* Add the node to dt_host. */ >> + rc = dt_overlay_add_node(overlay_node, overlay_node->parent->full_name); >> + if ( rc ) >> + { >> + read_unlock(&dt_host->lock); >> + >> + /* Node not added in dt_host. */ >> + goto remove_node; >> + } >> + >> + read_unlock(&dt_host->lock); >> + >> + prev_node->allnext = next_node; >> + >> + overlay_node = dt_find_node_by_path(overlay_node->full_name); >> + if ( overlay_node == NULL ) >> + { >> + /* Sanity check. But code will never come here. */ >> + ASSERT_UNREACHABLE(); >> + goto remove_node; >> + } >> + >> + rc = handle_add_irq_iommu(hardware_domain, overlay_node); >> + if ( rc ) >> + { >> + printk(XENLOG_ERR "Adding IRQ and IOMMU failed\n"); >> + return rc; > This is definitely wrong as you will return with spinlock held and memory not freed. Fixed it. And also split this function into two parts for better read > >> + } >> + >> + /* Keep overlay_node address in tracker. */ >> + tr->nodes_address[j] = (unsigned long)overlay_node; >> + } >> + >> + INIT_LIST_HEAD(&tr->entry); >> + list_add_tail(&tr->entry, &overlay_tracker); >> + >> + spin_unlock(&overlay_lock); >> + >> + if ( nodes_full_path != NULL ) >> + { >> + for ( i = 0; i < tr->num_nodes && nodes_full_path[i] != NULL; >> + i++ ) >> + { >> + xfree(nodes_full_path[i]); >> + } >> + xfree(nodes_full_path); >> + } > This block of code is identical to the one at the end so some cleanup is necessary. Created a common function for this as this will happen in three places. I missed freeing memory in remove_node failure cases. > >> + >> + return rc; >> + >> +/* >> + * Failure case. We need to remove the nodes, free tracker(if tr exists) and >> + * tr->dt_host_new. >> + */ >> +remove_node: >> + tr->num_nodes = j; >> + rc = remove_nodes(tr); >> + >> + if ( rc ) >> + { >> + /* >> + * User needs to provide right overlay. Incorrect node information >> + * example parent node doesn't exist in dt_host etc can cause memory >> + * leaks as removing_nodes() will fail and this means nodes memory is >> + * not freed from tracker. Which may cause memory leaks. Ideally, these >> + * device tree related mistakes will be caught by fdt_overlay_apply() >> + * but given that we don't manage that code keeping this warning message >> + * is better here. >> + */ >> + printk(XENLOG_ERR "Removing node failed.\n"); >> + spin_unlock(&overlay_lock); >> + return rc; >> + } >> + >> +err: >> + spin_unlock(&overlay_lock); >> + >> + if ( tr->dt_host_new ) >> + xfree(tr->dt_host_new); >> + >> + xfree(tr->overlay_fdt); >> + xfree(tr->nodes_address); >> + xfree(tr->fdt); >> + >> + if ( nodes_full_path != NULL ) >> + { >> + for ( i = 0; i < tr->num_nodes && nodes_full_path[i] != NULL; >> + i++ ) >> + { >> + xfree(nodes_full_path[i]); >> + } >> + xfree(nodes_full_path); >> + } >> + >> + xfree(tr); >> + >> + return rc; >> +} >> + >> long dt_sysctl(struct xen_sysctl_dt_overlay *op) >> { >> long ret; >> @@ -395,6 +898,13 @@ long dt_sysctl(struct xen_sysctl_dt_overlay *op) >> >> switch ( op->overlay_op ) >> { >> + case XEN_SYSCTL_DT_OVERLAY_ADD: >> + ret = handle_add_overlay_nodes(overlay_fdt, op->overlay_fdt_size); >> + if ( ret ) >> + xfree(overlay_fdt); > See my comment about xfree in previous patch I am sending v7 with fix for 15/19 patches also. Please review that. > >> + >> + break; >> + >> case XEN_SYSCTL_DT_OVERLAY_REMOVE: >> ret = handle_remove_overlay_nodes(overlay_fdt, op->overlay_fdt_size); >> xfree(overlay_fdt); > ~Michal
diff --git a/xen/common/dt-overlay.c b/xen/common/dt-overlay.c index b89cceab84..09ea46111b 100644 --- a/xen/common/dt-overlay.c +++ b/xen/common/dt-overlay.c @@ -33,6 +33,25 @@ static struct dt_device_node * return child_node; } +/* + * Returns next node to the input node. If node has children then return + * last descendant's next node. +*/ +static struct dt_device_node * +dt_find_next_node(struct dt_device_node *dt, const struct dt_device_node *node) +{ + struct dt_device_node *np; + + dt_for_each_device_node(dt, np) + if ( np == node ) + break; + + if ( np->child ) + np = find_last_descendants_node(np); + + return np->allnext; +} + static int dt_overlay_remove_node(struct dt_device_node *device_node) { struct dt_device_node *np; @@ -106,6 +125,76 @@ static int dt_overlay_remove_node(struct dt_device_node *device_node) return 0; } +static int dt_overlay_add_node(struct dt_device_node *device_node, + const char *parent_node_path) +{ + struct dt_device_node *parent_node; + struct dt_device_node *next_node; + + parent_node = dt_find_node_by_path(parent_node_path); + + if ( parent_node == NULL ) + { + dt_dprintk("Parent node %s not found. Overlay node will not be added\n", + parent_node_path); + return -EINVAL; + } + + /* If parent has no child. */ + if ( parent_node->child == NULL ) + { + next_node = parent_node->allnext; + device_node->parent = parent_node; + parent_node->allnext = device_node; + parent_node->child = device_node; + } + else + { + struct dt_device_node *np; + /* If parent has at least one child node. + * Iterate to the last child node of parent. + */ + for ( np = parent_node->child; np->sibling != NULL; np = np->sibling ); + + /* Iterate over all child nodes of np node. */ + if ( np->child ) + { + struct dt_device_node *np_last_descendant; + + np_last_descendant = find_last_descendants_node(np); + + next_node = np_last_descendant->allnext; + np_last_descendant->allnext = device_node; + } + else + { + next_node = np->allnext; + np->allnext = device_node; + } + + device_node->parent = parent_node; + np->sibling = device_node; + np->sibling->sibling = NULL; + } + + /* Iterate over all child nodes of device_node to add children too. */ + if ( device_node->child ) + { + struct dt_device_node *device_node_last_descendant; + + device_node_last_descendant = find_last_descendants_node(device_node); + /* Plug next_node at the end of last children of device_node. */ + device_node_last_descendant->allnext = next_node; + } + else + { + /* Now plug next_node at the end of device_node. */ + device_node->allnext = next_node; + } + + return 0; +} + /* Basic sanity check for the dtbo tool stack provided to Xen. */ static int check_overlay_fdt(const void *overlay_fdt, uint32_t overlay_fdt_size) { @@ -145,6 +234,82 @@ static unsigned int overlay_node_count(const void *overlay_fdt) return num_overlay_nodes; } +/* + * overlay_get_nodes_info gets full name with path for all the nodes which + * are in one level of __overlay__ tag. This is useful when checking node for + * duplication i.e. dtbo tries to add nodes which already exists in device tree. + */ +static int overlay_get_nodes_info(const void *fdto, char ***nodes_full_path, + unsigned int num_overlay_nodes) +{ + int fragment; + + *nodes_full_path = xzalloc_bytes(num_overlay_nodes * sizeof(char *)); + + if ( *nodes_full_path == NULL ) + return -ENOMEM; + + fdt_for_each_subnode(fragment, fdto, 0) + { + int target; + int overlay; + int subnode; + const char *target_path; + + target = fdt_overlay_target_offset(device_tree_flattened, fdto, + fragment, &target_path); + if ( target < 0 ) + return target; + + if ( target_path == NULL ) + return -EINVAL; + + overlay = fdt_subnode_offset(fdto, fragment, "__overlay__"); + + /* + * overlay value can be < 0. But fdt_for_each_subnode() loop checks for + * overlay >= 0. So, no need for a overlay>=0 check here. + */ + fdt_for_each_subnode(subnode, fdto, overlay) + { + const char *node_name = NULL; + int node_name_len; + unsigned int target_path_len = strlen(target_path); + unsigned int node_full_name_len; + unsigned int node_num = 0; + + node_name = fdt_get_name(fdto, subnode, &node_name_len); + + if ( node_name == NULL ) + return node_name_len; + + /* + * Magic number 2 is for adding '/' and '\0'. This is done to keep + * the node_full_path in the correct full node name format. + */ + node_full_name_len = target_path_len + node_name_len + 2; + + (*nodes_full_path)[node_num] = xmalloc_bytes(node_full_name_len); + + if ( (*nodes_full_path)[node_num] == NULL ) + return -ENOMEM; + + memcpy((*nodes_full_path)[node_num], target_path, target_path_len); + + (*nodes_full_path)[node_num][target_path_len] = '/'; + + memcpy((*nodes_full_path)[node_num] + target_path_len + 1, + node_name, node_name_len); + + (*nodes_full_path)[node_num][node_full_name_len - 1] = '\0'; + + node_num++; + } + } + + return 0; +} + static int handle_remove_irq_iommu(struct dt_device_node *device_node) { int rc = 0; @@ -371,6 +536,344 @@ out: return rc; } +/* + * Handles IRQ and IOMMU mapping for the overlay_node and all descendants of the + * overlay_node. + */ +static int handle_add_irq_iommu(struct domain *d, + struct dt_device_node *overlay_node) +{ + int rc; + unsigned int naddr, i, len; + struct dt_device_node *np; + + /* First let's handle the interrupts. */ + rc = handle_device_interrupts(d, overlay_node, false); + if ( rc < 0 ) + { + printk(XENLOG_ERR "Failed to retrieve interrupts configuration\n"); + return rc; + } + + /* Check if iommu property exists. */ + if ( dt_get_property(overlay_node, "iommus", &len) ) + { + /* Add device to IOMMUs. */ + rc = iommu_add_dt_device(overlay_node); + if ( rc < 0 ) + { + printk(XENLOG_ERR "Failed to add %s to the IOMMU\n", + dt_node_full_name(overlay_node)); + return rc; + } + } + + /* Set permissions. */ + naddr = dt_number_of_address(overlay_node); + + dt_dprintk("%s naddr = %u\n", dt_node_full_name(overlay_node), naddr); + + /* Give permission to map MMIOs */ + for ( i = 0; i < naddr; i++ ) + { + uint64_t addr, size; + + /* + * For now, we skip_mapping which means it will only permit iomem access + * to hardware_domain using iomem_permit_access() but will never map as + * map_range_p2mt() will not be called. + */ + struct map_range_data mr_data = { .d = d, + .p2mt = p2m_mmio_direct_c, + .skip_mapping = true + }; + + rc = dt_device_get_address(overlay_node, i, &addr, &size); + if ( rc ) + { + printk(XENLOG_ERR "Unable to retrieve address %u for %s\n", + i, dt_node_full_name(overlay_node)); + return rc; + } + + rc = map_range_to_domain(overlay_node, addr, size, &mr_data); + if ( rc ) + return rc; + } + + /* Map IRQ and IOMMU for overlay_node's children. */ + for ( np = overlay_node->child; np != NULL; np = np->sibling ) + { + rc = handle_add_irq_iommu(d, np); + if ( rc ) + { + printk(XENLOG_ERR "Adding IRQ and IOMMU failed\n"); + return rc; + } + } + + return rc; +} + +/* + * Adds device tree nodes under target node. + * We use tr->dt_host_new to unflatten the updated device_tree_flattened. This + * is done to avoid the removal of device_tree generation, iomem regions mapping + * to hardware domain done by handle_node(). + */ +static long handle_add_overlay_nodes(void *overlay_fdt, + uint32_t overlay_fdt_size) +{ + int rc, j, i; + struct dt_device_node *overlay_node; + struct overlay_track *tr = NULL; + char **nodes_full_path = NULL; + unsigned int new_fdt_size; + + tr = xzalloc(struct overlay_track); + if ( tr == NULL ) + return -ENOMEM; + + new_fdt_size = fdt_totalsize(device_tree_flattened) + + fdt_totalsize(overlay_fdt); + + tr->fdt = xzalloc_bytes(new_fdt_size); + if ( tr->fdt == NULL ) + { + xfree(tr); + return -ENOMEM; + } + + tr->num_nodes = overlay_node_count(overlay_fdt); + if ( tr->num_nodes == 0 ) + { + xfree(tr->fdt); + xfree(tr); + return -ENOMEM; + } + + tr->nodes_address = xzalloc_bytes(tr->num_nodes * sizeof(unsigned long)); + if ( tr->nodes_address == NULL ) + { + xfree(tr->fdt); + xfree(tr); + return -ENOMEM; + } + + rc = check_overlay_fdt(overlay_fdt, overlay_fdt_size); + if ( rc ) + { + xfree(tr->nodes_address); + xfree(tr->fdt); + xfree(tr); + return rc; + } + + /* + * Keep a copy of overlay_fdt as fdt_overlay_apply will change the input + * overlay's content(magic) when applying overlay. + */ + tr->overlay_fdt = xzalloc_bytes(overlay_fdt_size); + if ( tr->overlay_fdt == NULL ) + { + xfree(tr->nodes_address); + xfree(tr->fdt); + xfree(tr); + return -ENOMEM; + } + + memcpy(tr->overlay_fdt, overlay_fdt, overlay_fdt_size); + + spin_lock(&overlay_lock); + + memcpy(tr->fdt, device_tree_flattened, + fdt_totalsize(device_tree_flattened)); + + /* Open tr->fdt with more space to accommodate the overlay_fdt. */ + rc = fdt_open_into(tr->fdt, tr->fdt, new_fdt_size); + if ( rc ) + { + printk(XENLOG_ERR "Increasing tr->fdt size failed with error %d\n", + rc); + goto err; + } + + /* + * overlay_get_nodes_info is called to get the node information from dtbo. + * This is done before fdt_overlay_apply() because the overlay apply will + * erase the magic of overlay_fdt. + */ + rc = overlay_get_nodes_info(overlay_fdt, &nodes_full_path, + tr->num_nodes); + if ( rc ) + { + printk(XENLOG_ERR "Getting nodes information failed with error %d\n", + rc); + goto err; + } + + rc = fdt_overlay_apply(tr->fdt, overlay_fdt); + if ( rc ) + { + printk(XENLOG_ERR "Adding overlay node failed with error %d\n", rc); + goto err; + } + + /* + * Check if any of the node already exists in dt_host. If node already exits + * we can return here as this overlay_fdt is not suitable for overlay ops. + */ + for ( j = 0; j < tr->num_nodes; j++ ) + { + overlay_node = dt_find_node_by_path(nodes_full_path[j]); + if ( overlay_node != NULL ) + { + printk(XENLOG_ERR "node %s exists in device tree\n", + nodes_full_path[j]); + rc = -EINVAL; + goto err; + } + } + + /* Unflatten the tr->fdt into a new dt_host. */ + rc = unflatten_device_tree(tr->fdt, &tr->dt_host_new); + if ( rc ) + { + printk(XENLOG_ERR "unflatten_device_tree failed with error %d\n", rc); + goto err; + } + + for ( j = 0; j < tr->num_nodes; j++ ) + { + struct dt_device_node *prev_node, *next_node; + + dt_dprintk("Adding node: %s\n", nodes_full_path[j]); + + /* Find the newly added node in tr->dt_host_new by it's full path. */ + overlay_node = device_tree_find_node_by_path(tr->dt_host_new, + nodes_full_path[j]); + if ( overlay_node == NULL ) + { + /* Sanity check. But code will never come here. */ + ASSERT_UNREACHABLE(); + goto remove_node; + } + + /* + * Find previous and next node to overlay_node in dt_host_new. We will + * need these nodes to fix the dt_host_new mapping. When overlay_node is + * take out of dt_host_new tree and added to dt_host, link between + * previous node and next_node is broken. We will need to refresh + * dt_host_new with correct linking for any other overlay nodes + * extraction in future. + */ + dt_for_each_device_node(tr->dt_host_new, prev_node) + if ( prev_node->allnext == overlay_node ) + break; + + next_node = dt_find_next_node(tr->dt_host_new, overlay_node); + + read_lock(&dt_host->lock); + + /* Add the node to dt_host. */ + rc = dt_overlay_add_node(overlay_node, overlay_node->parent->full_name); + if ( rc ) + { + read_unlock(&dt_host->lock); + + /* Node not added in dt_host. */ + goto remove_node; + } + + read_unlock(&dt_host->lock); + + prev_node->allnext = next_node; + + overlay_node = dt_find_node_by_path(overlay_node->full_name); + if ( overlay_node == NULL ) + { + /* Sanity check. But code will never come here. */ + ASSERT_UNREACHABLE(); + goto remove_node; + } + + rc = handle_add_irq_iommu(hardware_domain, overlay_node); + if ( rc ) + { + printk(XENLOG_ERR "Adding IRQ and IOMMU failed\n"); + return rc; + } + + /* Keep overlay_node address in tracker. */ + tr->nodes_address[j] = (unsigned long)overlay_node; + } + + INIT_LIST_HEAD(&tr->entry); + list_add_tail(&tr->entry, &overlay_tracker); + + spin_unlock(&overlay_lock); + + if ( nodes_full_path != NULL ) + { + for ( i = 0; i < tr->num_nodes && nodes_full_path[i] != NULL; + i++ ) + { + xfree(nodes_full_path[i]); + } + xfree(nodes_full_path); + } + + return rc; + +/* + * Failure case. We need to remove the nodes, free tracker(if tr exists) and + * tr->dt_host_new. + */ +remove_node: + tr->num_nodes = j; + rc = remove_nodes(tr); + + if ( rc ) + { + /* + * User needs to provide right overlay. Incorrect node information + * example parent node doesn't exist in dt_host etc can cause memory + * leaks as removing_nodes() will fail and this means nodes memory is + * not freed from tracker. Which may cause memory leaks. Ideally, these + * device tree related mistakes will be caught by fdt_overlay_apply() + * but given that we don't manage that code keeping this warning message + * is better here. + */ + printk(XENLOG_ERR "Removing node failed.\n"); + spin_unlock(&overlay_lock); + return rc; + } + +err: + spin_unlock(&overlay_lock); + + if ( tr->dt_host_new ) + xfree(tr->dt_host_new); + + xfree(tr->overlay_fdt); + xfree(tr->nodes_address); + xfree(tr->fdt); + + if ( nodes_full_path != NULL ) + { + for ( i = 0; i < tr->num_nodes && nodes_full_path[i] != NULL; + i++ ) + { + xfree(nodes_full_path[i]); + } + xfree(nodes_full_path); + } + + xfree(tr); + + return rc; +} + long dt_sysctl(struct xen_sysctl_dt_overlay *op) { long ret; @@ -395,6 +898,13 @@ long dt_sysctl(struct xen_sysctl_dt_overlay *op) switch ( op->overlay_op ) { + case XEN_SYSCTL_DT_OVERLAY_ADD: + ret = handle_add_overlay_nodes(overlay_fdt, op->overlay_fdt_size); + if ( ret ) + xfree(overlay_fdt); + + break; + case XEN_SYSCTL_DT_OVERLAY_REMOVE: ret = handle_remove_overlay_nodes(overlay_fdt, op->overlay_fdt_size); xfree(overlay_fdt);
Update sysctl XEN_SYSCTL_dt_overlay to enable support for dtbo nodes addition using device tree overlay. xl dt-overlay add file.dtbo: Each time overlay nodes are added using .dtbo, a new fdt(memcpy of device_tree_flattened) is created and updated with overlay nodes. This updated fdt is further unflattened to a dt_host_new. Next, it checks if any of the overlay nodes already exists in the dt_host. If overlay nodes doesn't exist then find the overlay nodes in dt_host_new, find the overlay node's parent in dt_host and add the nodes as child under their parent in the dt_host. The node is attached as the last node under target parent. Finally, add IRQs, add device to IOMMUs, set permissions and map MMIO for the overlay node. When a node is added using overlay, a new entry is allocated in the overlay_track to keep the track of memory allocation due to addition of overlay node. This is helpful for freeing the memory allocated when a device tree node is removed. The main purpose of this to address first part of dynamic programming i.e. making xen aware of new device tree node which means updating the dt_host with overlay node information. Here we are adding/removing node from dt_host, and checking/setting IOMMU and IRQ permission but never mapping them to any domain. Right now, mapping/Un-mapping will happen only when a new domU is created/destroyed using "xl create". Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> --- xen/common/dt-overlay.c | 510 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 510 insertions(+)