| Message ID | 20190808231242.26424-2-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/6] xen/arm: introduce handle_interrupts | expand |
Hi Stefano, Stefano Stabellini writes: > Read the dtb fragment corresponding to a passthrough device from memory > at the location referred to by the "multiboot,dtb" compatible node. > > Copy the fragment to the guest dtb. > > Add a dtb_bootmodule field to struct kernel_info to find the dtb > fragment for a guest. > > Some of the code below is taken from tools/libxl/libxl_arm.c. Note that > it is OK to take LGPL 2.1 code and including it into a GPLv2 code base. > The result is GPLv2 code. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > ---- > Changes in v3: > - switch to using device_tree_for_each_node for the copy > > Changes in v2: > - add a note about the code coming from libxl in the commit message > - copy /aliases > - code style > --- > xen/arch/arm/domain_build.c | 103 +++++++++++++++++++++++++++++++++++ > xen/include/asm-arm/kernel.h | 2 +- > 2 files changed, 104 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 00ddb3b05d..70bcdc449d 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -14,6 +14,7 @@ > #include <xen/guest_access.h> > #include <xen/iocap.h> > #include <xen/acpi.h> > +#include <xen/vmap.h> > #include <xen/warning.h> > #include <acpi/actables.h> > #include <asm/device.h> > @@ -1706,6 +1707,102 @@ static int __init make_vpl011_uart_node(const struct domain *d, void *fdt) > } > #endif > > +static int __init handle_properties(struct domain *d, void *fdt, const void *pfdt, int nodeoff, > + u32 address_cells, u32 size_cells) nitpicking: "handle_properties" is somewhat non-descriptive in context of this file. From this name it is hard to tell that this function copies properties of FDT node. > +{ > + int propoff, nameoff, r; > + const struct fdt_property *prop; > + > + for ( propoff = fdt_first_property_offset(pfdt, nodeoff); > + propoff >= 0; > + propoff = fdt_next_property_offset(pfdt, propoff) ) > + { > + Do you really need this empty line? > + if ( !(prop = fdt_get_property_by_offset(pfdt, propoff, NULL)) ) > + return -FDT_ERR_INTERNAL; > + > + nameoff = fdt32_to_cpu(prop->nameoff); > + r = fdt_property(fdt, fdt_string(pfdt, nameoff), > + prop->data, fdt32_to_cpu(prop->len)); > + if ( r ) > + return r; > + } > + > + /* FDT_ERR_NOTFOUND => There is no more properties for this node */ > + return ( propoff != -FDT_ERR_NOTFOUND ) ? propoff : 0; > +} > + > +static int __init scan_pt_node(const void *pfdt, > + int nodeoff, const char *name, int depth, > + u32 address_cells, u32 size_cells, > + void *data) > +{ > + int rc; > + int i, num; > + struct kernel_info *kinfo = data; > + void *fdt = kinfo->fdt; > + int depth_next = depth; > + int node_next; > + > + /* no need to parse initial node */ > + if ( !depth ) > + return 0; > + > + rc = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL)); > + if ( rc ) > + return rc; > + > + rc = handle_properties(kinfo->d, fdt, pfdt, nodeoff, > + address_cells, size_cells); > + if ( rc ) > + return rc; > + > + node_next = fdt_next_node(pfdt, nodeoff, &depth_next); > + > + /* > + * If the next node is a sibling, then we need to call > + * fdt_end_node once. If the next node is one level up, we need to > + * call it twice: once for us and the second time for our parent. > + * Both these two conditions are expressed together by depth - > + * depth_next + 1. > + * > + * If we reached the end of the device tree fragment, then it is > + * easy: we need to call fdt_end_node once for every level of depth > + * to close all open nodes. > + */ > + if ( depth_next < 0 ) > + num = depth; > + else > + num = depth - depth_next + 1; > + > + for ( i = 0; i < num; i++ ) > + { > + rc = fdt_end_node(fdt); > + if ( rc ) > + return rc; > + } > + > + return 0; > +} > + > +static int __init domain_handle_dtb_bootmodule(struct domain *d, > + struct kernel_info *kinfo) > +{ > + void *pfdt; > + int res; > + > + pfdt = ioremap_cache(kinfo->dtb_bootmodule->start, > + kinfo->dtb_bootmodule->size); > + if ( pfdt == NULL ) > + return -EFAULT; > + > + res = device_tree_for_each_node(pfdt, scan_pt_node, kinfo); > + > + iounmap(pfdt); > + > + return res; > +} > + > /* > * The max size for DT is 2MB. However, the generated DT is small, 4KB > * are enough for now, but we might have to increase it in the future. > @@ -1777,6 +1874,12 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) > goto err; > } > > + if ( kinfo->dtb_bootmodule ) { > + ret = domain_handle_dtb_bootmodule(d, kinfo); > + if ( ret ) > + return ret; > + } > + > ret = fdt_end_node(kinfo->fdt); > if ( ret < 0 ) > goto err; > diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h > index 33f3e72b11..720dec4071 100644 > --- a/xen/include/asm-arm/kernel.h > +++ b/xen/include/asm-arm/kernel.h > @@ -28,7 +28,7 @@ struct kernel_info { > paddr_t gnttab_size; > > /* boot blob load addresses */ > - const struct bootmodule *kernel_bootmodule, *initrd_bootmodule; > + const struct bootmodule *kernel_bootmodule, *initrd_bootmodule, *dtb_bootmodule; > const char* cmdline; > paddr_t dtb_paddr; > paddr_t initrd_paddr; -- Volodymyr Babchuk at EPAM
Hi Stefano, On 8/9/19 12:12 AM, Stefano Stabellini wrote: > Read the dtb fragment corresponding to a passthrough device from memory > at the location referred to by the "multiboot,dtb" compatible node. > > Copy the fragment to the guest dtb. > > Add a dtb_bootmodule field to struct kernel_info to find the dtb > fragment for a guest. > > Some of the code below is taken from tools/libxl/libxl_arm.c. Note that > it is OK to take LGPL 2.1 code and including it into a GPLv2 code base. > The result is GPLv2 code. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > ---- > Changes in v3: > - switch to using device_tree_for_each_node for the copy > > Changes in v2: > - add a note about the code coming from libxl in the commit message > - copy /aliases > - code style > --- > xen/arch/arm/domain_build.c | 103 +++++++++++++++++++++++++++++++++++ > xen/include/asm-arm/kernel.h | 2 +- > 2 files changed, 104 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 00ddb3b05d..70bcdc449d 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -14,6 +14,7 @@ > #include <xen/guest_access.h> > #include <xen/iocap.h> > #include <xen/acpi.h> > +#include <xen/vmap.h> > #include <xen/warning.h> > #include <acpi/actables.h> > #include <asm/device.h> > @@ -1706,6 +1707,102 @@ static int __init make_vpl011_uart_node(const struct domain *d, void *fdt) > } > #endif > > +static int __init handle_properties(struct domain *d, void *fdt, const void *pfdt, int nodeoff, > + u32 address_cells, u32 size_cells) So in this file we already have a function call write_properties. How a user will know which one to use? It is also not entirely clear from there variable name what is the difference between fdt and pfdt. Also, no more u32 in the code please. This is now part of the CODING_STYLE. > +{ > + int propoff, nameoff, r; Please no single letter variable unless this is obvious to understand (such as i, j for iteration). This should be ret, rc or res. Note that somehow you are using the 3 of them in the same patches... I am not going to ask for consistency thought. > + const struct fdt_property *prop; > + > + for ( propoff = fdt_first_property_offset(pfdt, nodeoff); > + propoff >= 0; > + propoff = fdt_next_property_offset(pfdt, propoff) ) > + { > + > + if ( !(prop = fdt_get_property_by_offset(pfdt, propoff, NULL)) ) > + return -FDT_ERR_INTERNAL; > + > + nameoff = fdt32_to_cpu(prop->nameoff); > + r = fdt_property(fdt, fdt_string(pfdt, nameoff), > + prop->data, fdt32_to_cpu(prop->len)); > + if ( r ) > + return r; > + } > + > + /* FDT_ERR_NOTFOUND => There is no more properties for this node */ > + return ( propoff != -FDT_ERR_NOTFOUND ) ? propoff : 0; > +} > + > +static int __init scan_pt_node(const void *pfdt, > + int nodeoff, const char *name, int depth, > + u32 address_cells, u32 size_cells, Same here. > + void *data) > +{ > + int rc; > + int i, num; Anything that can't be negative, should be unsigned. > + struct kernel_info *kinfo = data; > + void *fdt = kinfo->fdt; > + int depth_next = depth; > + int node_next; > + > + /* no need to parse initial node */ > + if ( !depth ) > + return 0; So this is going to copy what ever is in the partial device-tree. In other word, the users can override pretty much anything that Xen created. I don't think this is what we want. Instead, we should only copy nodes under /passthrough and also the /aliases. > + > + rc = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL)); fdt_begin_node assume the second parameter will be non-NULL. What if fdt_get_name() returns NULL? > + if ( rc ) > + return rc; > + > + rc = handle_properties(kinfo->d, fdt, pfdt, nodeoff, > + address_cells, size_cells); > + if ( rc ) > + return rc; > + > + node_next = fdt_next_node(pfdt, nodeoff, &depth_next); What if node_next is invalid (i.e because it is negative)? > + > + /* > + * If the next node is a sibling, then we need to call > + * fdt_end_node once. If the next node is one level up, we need to > + * call it twice: once for us and the second time for our parent. > + * Both these two conditions are expressed together by depth - > + * depth_next + 1. > + * > + * If we reached the end of the device tree fragment, then it is > + * easy: we need to call fdt_end_node once for every level of depth > + * to close all open nodes. > + */ > + if ( depth_next < 0 ) > + num = depth; > + else > + num = depth - depth_next + 1; > + > + for ( i = 0; i < num; i++ ) > + { > + rc = fdt_end_node(fdt); > + if ( rc ) > + return rc; > + } All this code is a bit complicated because you decided to use dt_tree_for_each_node that is not really suitable here. It would be possible to use recursion as we did for the dom0 DT thanks to fdt_first_subnode, fdt_next_subnode. Something like: handle_node { fdt_begin_node(....); write_properties(...); node = fdt_first_subnode(...); while ( node > 0 ) { handle_node(node...); node = fdt_next_subnode(...); } fdt_end_node(....); } > + > + return 0; > +} > + > +static int __init domain_handle_dtb_bootmodule(struct domain *d, > + struct kernel_info *kinfo) > +{ > + void *pfdt; > + int res; > + > + pfdt = ioremap_cache(kinfo->dtb_bootmodule->start, > + kinfo->dtb_bootmodule->size); Indentation. > + if ( pfdt == NULL ) > + return -EFAULT; > + > + res = device_tree_for_each_node(pfdt, scan_pt_node, kinfo); > + > + iounmap(pfdt); > + > + return res; > +} > + > /* > * The max size for DT is 2MB. However, the generated DT is small, 4KB > * are enough for now, but we might have to increase it in the future. > @@ -1777,6 +1874,12 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) > goto err; > } > > + if ( kinfo->dtb_bootmodule ) { > + ret = domain_handle_dtb_bootmodule(d, kinfo); > + if ( ret ) > + return ret; > + } > + > ret = fdt_end_node(kinfo->fdt); > if ( ret < 0 ) > goto err; > diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h > index 33f3e72b11..720dec4071 100644 > --- a/xen/include/asm-arm/kernel.h > +++ b/xen/include/asm-arm/kernel.h > @@ -28,7 +28,7 @@ struct kernel_info { > paddr_t gnttab_size; > > /* boot blob load addresses */ > - const struct bootmodule *kernel_bootmodule, *initrd_bootmodule; > + const struct bootmodule *kernel_bootmodule, *initrd_bootmodule, *dtb_bootmodule; > const char* cmdline; > paddr_t dtb_paddr; > paddr_t initrd_paddr; > Cheers,
On Fri, 9 Aug 2019, Volodymyr Babchuk wrote: > Stefano Stabellini writes: > > > Read the dtb fragment corresponding to a passthrough device from memory > > at the location referred to by the "multiboot,dtb" compatible node. > > > > Copy the fragment to the guest dtb. > > > > Add a dtb_bootmodule field to struct kernel_info to find the dtb > > fragment for a guest. > > > > Some of the code below is taken from tools/libxl/libxl_arm.c. Note that > > it is OK to take LGPL 2.1 code and including it into a GPLv2 code base. > > The result is GPLv2 code. > > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > > > ---- > > Changes in v3: > > - switch to using device_tree_for_each_node for the copy > > > > Changes in v2: > > - add a note about the code coming from libxl in the commit message > > - copy /aliases > > - code style > > --- > > xen/arch/arm/domain_build.c | 103 +++++++++++++++++++++++++++++++++++ > > xen/include/asm-arm/kernel.h | 2 +- > > 2 files changed, 104 insertions(+), 1 deletion(-) > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > index 00ddb3b05d..70bcdc449d 100644 > > --- a/xen/arch/arm/domain_build.c > > +++ b/xen/arch/arm/domain_build.c > > @@ -14,6 +14,7 @@ > > #include <xen/guest_access.h> > > #include <xen/iocap.h> > > #include <xen/acpi.h> > > +#include <xen/vmap.h> > > #include <xen/warning.h> > > #include <acpi/actables.h> > > #include <asm/device.h> > > @@ -1706,6 +1707,102 @@ static int __init make_vpl011_uart_node(const struct domain *d, void *fdt) > > } > > #endif > > > > +static int __init handle_properties(struct domain *d, void *fdt, const void *pfdt, int nodeoff, > > + u32 address_cells, u32 size_cells) > nitpicking: "handle_properties" is somewhat non-descriptive in context > of this file. From this name it is hard to tell that this function > copies properties of FDT node. I'll rename it to handle_prop_pfdt: the important message to convey is that it is a function to handle the properties of the partial device tree. > > +{ > > + int propoff, nameoff, r; > > + const struct fdt_property *prop; > > + > > + for ( propoff = fdt_first_property_offset(pfdt, nodeoff); > > + propoff >= 0; > > + propoff = fdt_next_property_offset(pfdt, propoff) ) > > + { > > + > Do you really need this empty line? I'll remove it > > + if ( !(prop = fdt_get_property_by_offset(pfdt, propoff, NULL)) ) > > + return -FDT_ERR_INTERNAL; > > + > > + nameoff = fdt32_to_cpu(prop->nameoff); > > + r = fdt_property(fdt, fdt_string(pfdt, nameoff), > > + prop->data, fdt32_to_cpu(prop->len)); > > + if ( r ) > > + return r; > > + } > > + > > + /* FDT_ERR_NOTFOUND => There is no more properties for this node */ > > + return ( propoff != -FDT_ERR_NOTFOUND ) ? propoff : 0; > > +} > > + > > +static int __init scan_pt_node(const void *pfdt, > > + int nodeoff, const char *name, int depth, > > + u32 address_cells, u32 size_cells, > > + void *data) > > +{ > > + int rc; > > + int i, num; > > + struct kernel_info *kinfo = data; > > + void *fdt = kinfo->fdt; > > + int depth_next = depth; > > + int node_next; > > + > > + /* no need to parse initial node */ > > + if ( !depth ) > > + return 0; > > + > > + rc = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL)); > > + if ( rc ) > > + return rc; > > + > > + rc = handle_properties(kinfo->d, fdt, pfdt, nodeoff, > > + address_cells, size_cells); > > + if ( rc ) > > + return rc; > > + > > + node_next = fdt_next_node(pfdt, nodeoff, &depth_next); > > + > > + /* > > + * If the next node is a sibling, then we need to call > > + * fdt_end_node once. If the next node is one level up, we need to > > + * call it twice: once for us and the second time for our parent. > > + * Both these two conditions are expressed together by depth - > > + * depth_next + 1. > > + * > > + * If we reached the end of the device tree fragment, then it is > > + * easy: we need to call fdt_end_node once for every level of depth > > + * to close all open nodes. > > + */ > > + if ( depth_next < 0 ) > > + num = depth; > > + else > > + num = depth - depth_next + 1; > > + > > + for ( i = 0; i < num; i++ ) > > + { > > + rc = fdt_end_node(fdt); > > + if ( rc ) > > + return rc; > > + } > > + > > + return 0; > > +} > > + > > +static int __init domain_handle_dtb_bootmodule(struct domain *d, > > + struct kernel_info *kinfo) > > +{ > > + void *pfdt; > > + int res; > > + > > + pfdt = ioremap_cache(kinfo->dtb_bootmodule->start, > > + kinfo->dtb_bootmodule->size); > > + if ( pfdt == NULL ) > > + return -EFAULT; > > + > > + res = device_tree_for_each_node(pfdt, scan_pt_node, kinfo); > > + > > + iounmap(pfdt); > > + > > + return res; > > +} > > + > > /* > > * The max size for DT is 2MB. However, the generated DT is small, 4KB > > * are enough for now, but we might have to increase it in the future. > > @@ -1777,6 +1874,12 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) > > goto err; > > } > > > > + if ( kinfo->dtb_bootmodule ) { > > + ret = domain_handle_dtb_bootmodule(d, kinfo); > > + if ( ret ) > > + return ret; > > + } > > + > > ret = fdt_end_node(kinfo->fdt); > > if ( ret < 0 ) > > goto err; > > diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h > > index 33f3e72b11..720dec4071 100644 > > --- a/xen/include/asm-arm/kernel.h > > +++ b/xen/include/asm-arm/kernel.h > > @@ -28,7 +28,7 @@ struct kernel_info { > > paddr_t gnttab_size; > > > > /* boot blob load addresses */ > > - const struct bootmodule *kernel_bootmodule, *initrd_bootmodule; > > + const struct bootmodule *kernel_bootmodule, *initrd_bootmodule, *dtb_bootmodule; > > const char* cmdline; > > paddr_t dtb_paddr; > > paddr_t initrd_paddr; > > > -- > Volodymyr Babchuk at EPAM
On Fri, 9 Aug 2019, Julien Grall wrote: > On 8/9/19 12:12 AM, Stefano Stabellini wrote: > > Read the dtb fragment corresponding to a passthrough device from memory > > at the location referred to by the "multiboot,dtb" compatible node. > > > > Copy the fragment to the guest dtb. > > > > Add a dtb_bootmodule field to struct kernel_info to find the dtb > > fragment for a guest. > > > > Some of the code below is taken from tools/libxl/libxl_arm.c. Note that > > it is OK to take LGPL 2.1 code and including it into a GPLv2 code base. > > The result is GPLv2 code. > > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > > > ---- > > Changes in v3: > > - switch to using device_tree_for_each_node for the copy > > > > Changes in v2: > > - add a note about the code coming from libxl in the commit message > > - copy /aliases > > - code style > > --- > > xen/arch/arm/domain_build.c | 103 +++++++++++++++++++++++++++++++++++ > > xen/include/asm-arm/kernel.h | 2 +- > > 2 files changed, 104 insertions(+), 1 deletion(-) > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > index 00ddb3b05d..70bcdc449d 100644 > > --- a/xen/arch/arm/domain_build.c > > +++ b/xen/arch/arm/domain_build.c > > @@ -14,6 +14,7 @@ > > #include <xen/guest_access.h> > > #include <xen/iocap.h> > > #include <xen/acpi.h> > > +#include <xen/vmap.h> > > #include <xen/warning.h> > > #include <acpi/actables.h> > > #include <asm/device.h> > > @@ -1706,6 +1707,102 @@ static int __init make_vpl011_uart_node(const struct > > domain *d, void *fdt) > > } > > #endif > > +static int __init handle_properties(struct domain *d, void *fdt, const > > void *pfdt, int nodeoff, > > + u32 address_cells, u32 size_cells) > > So in this file we already have a function call write_properties. How a user > will know which one to use? I have renamed handle_properties to handle_prop_pfdt, to make it clear that this one is specific to partial device tree fragments. > It is also not entirely clear from there variable name what is the difference > between fdt and pfdt. I have clarified and reduced the list of parameters by passing a kinfo instead of domain and fdt separately. > Also, no more u32 in the code please. This is now part of the CODING_STYLE. Interesting, I haven't been following. Should I use uint32_t? What's the recommended practice for fixed sized integers now? > > +{ > > + int propoff, nameoff, r; > > Please no single letter variable unless this is obvious to understand (such as > i, j for iteration). This should be ret, rc or res. > > Note that somehow you are using the 3 of them in the same patches... I am not > going to ask for consistency thought. OK > > + const struct fdt_property *prop; > > + > > + for ( propoff = fdt_first_property_offset(pfdt, nodeoff); > > + propoff >= 0; > > + propoff = fdt_next_property_offset(pfdt, propoff) ) > > + { > > + > > + if ( !(prop = fdt_get_property_by_offset(pfdt, propoff, NULL)) ) > > + return -FDT_ERR_INTERNAL; > > + > > + nameoff = fdt32_to_cpu(prop->nameoff); > > + r = fdt_property(fdt, fdt_string(pfdt, nameoff), > > + prop->data, fdt32_to_cpu(prop->len)); > > + if ( r ) > > + return r; > > + } > > + > > + /* FDT_ERR_NOTFOUND => There is no more properties for this node */ > > + return ( propoff != -FDT_ERR_NOTFOUND ) ? propoff : 0; > > +} > > + > > +static int __init scan_pt_node(const void *pfdt, > > + int nodeoff, const char *name, int depth, > > + u32 address_cells, u32 size_cells, > > Same here. > > > + void *data) > > +{ > > + int rc; > > + int i, num; > > Anything that can't be negative, should be unsigned. OK > > + struct kernel_info *kinfo = data; > > + void *fdt = kinfo->fdt; > > + int depth_next = depth; > > + int node_next; > > + > > + /* no need to parse initial node */ > > + if ( !depth ) > > + return 0; > > So this is going to copy what ever is in the partial device-tree. In other > word, the users can override pretty much anything that Xen created. I don't > think this is what we want. > > Instead, we should only copy nodes under /passthrough and also the /aliases. Very good point! I'll fix it. > > + > > + rc = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL)); > > fdt_begin_node assume the second parameter will be non-NULL. What if > fdt_get_name() returns NULL? I'll add a check. > > + if ( rc ) > > + return rc; > > + > > + rc = handle_properties(kinfo->d, fdt, pfdt, nodeoff, > > + address_cells, size_cells); > > + if ( rc ) > > + return rc; > > + > > + node_next = fdt_next_node(pfdt, nodeoff, &depth_next); > > What if node_next is invalid (i.e because it is negative)? This goes away with the change described below. > > + > > + /* > > + * If the next node is a sibling, then we need to call > > + * fdt_end_node once. If the next node is one level up, we need to > > + * call it twice: once for us and the second time for our parent. > > + * Both these two conditions are expressed together by depth - > > + * depth_next + 1. > > + * > > + * If we reached the end of the device tree fragment, then it is > > + * easy: we need to call fdt_end_node once for every level of depth > > + * to close all open nodes. > > + */ > > + if ( depth_next < 0 ) > > + num = depth; > > + else > > + num = depth - depth_next + 1; > > + > > + for ( i = 0; i < num; i++ ) > > + { > > + rc = fdt_end_node(fdt); > > + if ( rc ) > > + return rc; > > + } > > All this code is a bit complicated because you decided to use > dt_tree_for_each_node that is not really suitable here. It would be possible > to use recursion as we did for the dom0 DT thanks to fdt_first_subnode, > fdt_next_subnode. > > Something like: > > handle_node > { > fdt_begin_node(....); > write_properties(...); > node = fdt_first_subnode(...); > > while ( node > 0 ) > { > handle_node(node...); > node = fdt_next_subnode(...); > } > fdt_end_node(....); > } Yes, I have followed your suggestion, and I have now a recursive implementation without any calls to dt_tree_for_each_node, very similar to the one you described here. > > + > > + return 0; > > +} > > + > > +static int __init domain_handle_dtb_bootmodule(struct domain *d, > > + struct kernel_info *kinfo) > > +{ > > + void *pfdt; > > + int res; > > + > > + pfdt = ioremap_cache(kinfo->dtb_bootmodule->start, > > + kinfo->dtb_bootmodule->size); > > Indentation. OK > > + if ( pfdt == NULL ) > > + return -EFAULT; > > + > > + res = device_tree_for_each_node(pfdt, scan_pt_node, kinfo); > > + > > + iounmap(pfdt); > > + > > + return res; > > +} > > + > > /* > > * The max size for DT is 2MB. However, the generated DT is small, 4KB > > * are enough for now, but we might have to increase it in the future. > > @@ -1777,6 +1874,12 @@ static int __init prepare_dtb_domU(struct domain *d, > > struct kernel_info *kinfo) > > goto err; > > } > > + if ( kinfo->dtb_bootmodule ) { > > + ret = domain_handle_dtb_bootmodule(d, kinfo); > > + if ( ret ) > > + return ret; > > + } > > + > > ret = fdt_end_node(kinfo->fdt); > > if ( ret < 0 ) > > goto err; > > diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h > > index 33f3e72b11..720dec4071 100644 > > --- a/xen/include/asm-arm/kernel.h > > +++ b/xen/include/asm-arm/kernel.h > > @@ -28,7 +28,7 @@ struct kernel_info { > > paddr_t gnttab_size; > > /* boot blob load addresses */ > > - const struct bootmodule *kernel_bootmodule, *initrd_bootmodule; > > + const struct bootmodule *kernel_bootmodule, *initrd_bootmodule, > > *dtb_bootmodule; > > const char* cmdline; > > paddr_t dtb_paddr; > > paddr_t initrd_paddr;
Hi, On 19/08/2019 23:47, Stefano Stabellini wrote: > On Fri, 9 Aug 2019, Julien Grall wrote: >> On 8/9/19 12:12 AM, Stefano Stabellini wrote: >> It is also not entirely clear from there variable name what is the difference >> between fdt and pfdt. > > I have clarified and reduced the list of parameters by passing a kinfo > instead of domain and fdt separately. > > >> Also, no more u32 in the code please. This is now part of the CODING_STYLE. > > Interesting, I haven't been following. Should I use uint32_t? What's the > recommended practice for fixed sized integers now? CODING_STYLE states: "Use basic C types and C standard mandated typedef-s where possible (and with preference in this order)." So this would be uintN_t. Cheers,
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 00ddb3b05d..70bcdc449d 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -14,6 +14,7 @@ #include <xen/guest_access.h> #include <xen/iocap.h> #include <xen/acpi.h> +#include <xen/vmap.h> #include <xen/warning.h> #include <acpi/actables.h> #include <asm/device.h> @@ -1706,6 +1707,102 @@ static int __init make_vpl011_uart_node(const struct domain *d, void *fdt) } #endif +static int __init handle_properties(struct domain *d, void *fdt, const void *pfdt, int nodeoff, + u32 address_cells, u32 size_cells) +{ + int propoff, nameoff, r; + const struct fdt_property *prop; + + for ( propoff = fdt_first_property_offset(pfdt, nodeoff); + propoff >= 0; + propoff = fdt_next_property_offset(pfdt, propoff) ) + { + + if ( !(prop = fdt_get_property_by_offset(pfdt, propoff, NULL)) ) + return -FDT_ERR_INTERNAL; + + nameoff = fdt32_to_cpu(prop->nameoff); + r = fdt_property(fdt, fdt_string(pfdt, nameoff), + prop->data, fdt32_to_cpu(prop->len)); + if ( r ) + return r; + } + + /* FDT_ERR_NOTFOUND => There is no more properties for this node */ + return ( propoff != -FDT_ERR_NOTFOUND ) ? propoff : 0; +} + +static int __init scan_pt_node(const void *pfdt, + int nodeoff, const char *name, int depth, + u32 address_cells, u32 size_cells, + void *data) +{ + int rc; + int i, num; + struct kernel_info *kinfo = data; + void *fdt = kinfo->fdt; + int depth_next = depth; + int node_next; + + /* no need to parse initial node */ + if ( !depth ) + return 0; + + rc = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL)); + if ( rc ) + return rc; + + rc = handle_properties(kinfo->d, fdt, pfdt, nodeoff, + address_cells, size_cells); + if ( rc ) + return rc; + + node_next = fdt_next_node(pfdt, nodeoff, &depth_next); + + /* + * If the next node is a sibling, then we need to call + * fdt_end_node once. If the next node is one level up, we need to + * call it twice: once for us and the second time for our parent. + * Both these two conditions are expressed together by depth - + * depth_next + 1. + * + * If we reached the end of the device tree fragment, then it is + * easy: we need to call fdt_end_node once for every level of depth + * to close all open nodes. + */ + if ( depth_next < 0 ) + num = depth; + else + num = depth - depth_next + 1; + + for ( i = 0; i < num; i++ ) + { + rc = fdt_end_node(fdt); + if ( rc ) + return rc; + } + + return 0; +} + +static int __init domain_handle_dtb_bootmodule(struct domain *d, + struct kernel_info *kinfo) +{ + void *pfdt; + int res; + + pfdt = ioremap_cache(kinfo->dtb_bootmodule->start, + kinfo->dtb_bootmodule->size); + if ( pfdt == NULL ) + return -EFAULT; + + res = device_tree_for_each_node(pfdt, scan_pt_node, kinfo); + + iounmap(pfdt); + + return res; +} + /* * The max size for DT is 2MB. However, the generated DT is small, 4KB * are enough for now, but we might have to increase it in the future. @@ -1777,6 +1874,12 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) goto err; } + if ( kinfo->dtb_bootmodule ) { + ret = domain_handle_dtb_bootmodule(d, kinfo); + if ( ret ) + return ret; + } + ret = fdt_end_node(kinfo->fdt); if ( ret < 0 ) goto err; diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h index 33f3e72b11..720dec4071 100644 --- a/xen/include/asm-arm/kernel.h +++ b/xen/include/asm-arm/kernel.h @@ -28,7 +28,7 @@ struct kernel_info { paddr_t gnttab_size; /* boot blob load addresses */ - const struct bootmodule *kernel_bootmodule, *initrd_bootmodule; + const struct bootmodule *kernel_bootmodule, *initrd_bootmodule, *dtb_bootmodule; const char* cmdline; paddr_t dtb_paddr; paddr_t initrd_paddr;
Read the dtb fragment corresponding to a passthrough device from memory at the location referred to by the "multiboot,dtb" compatible node. Copy the fragment to the guest dtb. Add a dtb_bootmodule field to struct kernel_info to find the dtb fragment for a guest. Some of the code below is taken from tools/libxl/libxl_arm.c. Note that it is OK to take LGPL 2.1 code and including it into a GPLv2 code base. The result is GPLv2 code. Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> ---- Changes in v3: - switch to using device_tree_for_each_node for the copy Changes in v2: - add a note about the code coming from libxl in the commit message - copy /aliases - code style --- xen/arch/arm/domain_build.c | 103 +++++++++++++++++++++++++++++++++++ xen/include/asm-arm/kernel.h | 2 +- 2 files changed, 104 insertions(+), 1 deletion(-)