| Message ID | 20190821035315.12812-4-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/8] xen/arm: introduce handle_device_interrupts | expand |
Hi Stefano, On 8/21/19 4:53 AM, Stefano Stabellini wrote: > Read the dtb fragment corresponding to a passthrough device from memory > at the location referred to by the "multiboot,device-tree" compatible > node. > > Add a new field named dtb_bootmodule to struct kernel_info to keep track > of the dtb fragment location. > > Copy the fragment to the guest dtb (only /aliases and /passthrough). > > Set kinfo->guest_phandle_gic based on the phandle of the special "/gic" > node in the device tree fragment. "/gic" is a dummy node in the dtb > fragment that represents the gic interrupt controller. Other properties > in the dtb fragment might refer to it (for instance interrupt-parent of > a device node). We reuse the phandle of "/gic" from the dtb fragment as > the phandle of the full GIC node that will be created for the guest > device tree. That way, when we copy properties from the device tree > fragment to the domU device tree the links remain unbroken. > > 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. I would be much in favor to keep the code as close a possible to the libxl code rather than re-inventing the wheel copying the partial DT. In other words, if there are reason to diverge, then I would like to understand why. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > ---- > Changes in v4: > - use recursion in the implementation > - rename handle_properties to handle_prop_pfdt > - rename scan_pt_node to scan_pfdt_node > - pass kinfo to handle_properties > - use uint32_t instead of u32 > - rename r to res > - add "passthrough" and "aliases" check > - add a name == NULL check > - code style > - move DTB fragment scanning earlier, before DomU GIC node creation > - set guest_phandle_gic based on "/gic" > - in-code comment > > 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 | 112 +++++++++++++++++++++++++++++++++++ > xen/include/asm-arm/kernel.h | 2 +- > 2 files changed, 113 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index cd585f05ca..c71b9f2889 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> > @@ -1713,6 +1714,111 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo) > } > #endif > > +static int __init handle_prop_pfdt(struct kernel_info *kinfo, > + const void *pfdt, int nodeoff, > + uint32_t address_cells, uint32_t size_cells) Why do you need address_cells and size_cells in parameter? > +{ > + void *fdt = kinfo->fdt; > + int propoff, nameoff, res; > + 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); > + res = fdt_property(fdt, fdt_string(pfdt, nameoff), > + prop->data, fdt32_to_cpu(prop->len)); > + if ( res ) > + return res; > + } > + > + /* FDT_ERR_NOTFOUND => There is no more properties for this node */ > + return ( propoff != -FDT_ERR_NOTFOUND ) ? propoff : 0; > +} > + > +static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt, > + int nodeoff, int depth, > + uint32_t address_cells, uint32_t size_cells) > +{ > + int rc = 0; > + void *fdt = kinfo->fdt; > + int node_next; > + const char *name = fdt_get_name(pfdt, nodeoff, NULL); > + > + /* > + * Take the GIC phandle value from the special /gic node in the DTB > + * fragment. > + */ > + if ( depth == 1 && dt_node_cmp(name, "gic") == 0 ) > + { > + kinfo->guest_phandle_gic = fdt_get_phandle(pfdt, nodeoff); > + return 0; > + } I don't like this solution. You are bypassing most of the function just for the benefits of have the name in hand. Can this be done separately? This would also avoid to have an extra parameter (depth) for the only benefits of this check. > + > + rc = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL)); > + if ( rc ) > + return rc; > + > + rc = handle_prop_pfdt(kinfo, pfdt, nodeoff, address_cells, size_cells); > + if ( rc ) > + return rc; > + > + address_cells = device_tree_get_u32(pfdt, nodeoff, "#address-cells", > + address_cells); > + size_cells = device_tree_get_u32(pfdt, nodeoff, "#size-cells", > + size_cells); I am pretty sure I mention it before (though not on this patch...), this is not matching the DT spec. address_cells and size_cells are not propagated to the next level. So these should be DT_ROOT_NODE_{ADDR, SIZE}_CELLS_DEFAULT. > + > + node_next = fdt_first_subnode(pfdt, nodeoff); > + while ( node_next > 0 ) > + { > + scan_pfdt_node(kinfo, pfdt, node_next, depth + 1, address_cells, size_cells); > + node_next = fdt_next_subnode(pfdt, node_next); > + } > + > + return fdt_end_node(fdt); > +} > + > +static int __init domain_handle_dtb_bootmodule(struct domain *d, > + struct kernel_info *kinfo) > +{ > + void *pfdt; > + int res, node_next; > + > + pfdt = ioremap_cache(kinfo->dtb_bootmodule->start, > + kinfo->dtb_bootmodule->size); > + if ( pfdt == NULL ) > + return -EFAULT; Shouldn't you check you actually have a device-tree in hand and that it fits in the size specified by the user? See check_partial_fdt() in libxl_arm.c. > + > + node_next = fdt_first_subnode(pfdt, 0); > + while ( node_next > 0 ) > + { Why do we have to go through the all the nodes of the first level? Can't we just lookup for the path and copy the node as libxl does? > + const char *name = fdt_get_name(pfdt, node_next, NULL); > + > + /* only scan /gic /aliases /passthrough, ignore the rest */ > + if ( name != NULL && > + (dt_node_cmp(name, "passthrough") == 0 || > + dt_node_cmp(name, "aliases") == 0 || > + dt_node_cmp(name, "gic") == 0) ) > + { > + res = scan_pfdt_node(kinfo, pfdt, node_next, 1, > + DT_ROOT_NODE_ADDR_CELLS_DEFAULT, > + DT_ROOT_NODE_SIZE_CELLS_DEFAULT); > + if ( res ) > + return res; > + } > + > + node_next = fdt_next_subnode(pfdt, node_next); > + } > + > + 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. > @@ -1768,6 +1874,12 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) > if ( ret ) > goto err; > > + if ( kinfo->dtb_bootmodule ) { Coding style: if ( ... ) { > + ret = domain_handle_dtb_bootmodule(d, kinfo); > + if ( ret ) > + return ret; > + } > + > ret = make_gic_domU_node(kinfo); > if ( ret ) > goto err; > diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h > index 760434369b..7f5e659561 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 Wed, 11 Sep 2019, Julien Grall wrote: > Hi Stefano, > > On 8/21/19 4:53 AM, Stefano Stabellini wrote: > > Read the dtb fragment corresponding to a passthrough device from memory > > at the location referred to by the "multiboot,device-tree" compatible > > node. > > > > Add a new field named dtb_bootmodule to struct kernel_info to keep track > > of the dtb fragment location. > > > > Copy the fragment to the guest dtb (only /aliases and /passthrough). > > > > Set kinfo->guest_phandle_gic based on the phandle of the special "/gic" > > node in the device tree fragment. "/gic" is a dummy node in the dtb > > fragment that represents the gic interrupt controller. Other properties > > in the dtb fragment might refer to it (for instance interrupt-parent of > > a device node). We reuse the phandle of "/gic" from the dtb fragment as > > the phandle of the full GIC node that will be created for the guest > > device tree. That way, when we copy properties from the device tree > > fragment to the domU device tree the links remain unbroken. > > > > 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. > > I would be much in favor to keep the code as close a possible to the libxl > code rather than re-inventing the wheel copying the partial DT. In other > words, if there are reason to diverge, then I would like to understand why. We started from something similar to libxl (see v1) but through several rounds of review we came to this version. The main reason is that the libxl code has only to copy the partial device tree. This patch has to copy and also to scan the partial device tree, taking action when appropriate. If you look at this patch in isolation, it looks like it is different for no particular reason, but if you take the patch series as a whole, this approach is better suited I think. But you do have a point that in a couple of places this is diverging for no reason. Specifically, we should have a check_partial_fdt function here too. I'll add it. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > > > ---- > > Changes in v4: > > - use recursion in the implementation > > - rename handle_properties to handle_prop_pfdt > > - rename scan_pt_node to scan_pfdt_node > > - pass kinfo to handle_properties > > - use uint32_t instead of u32 > > - rename r to res > > - add "passthrough" and "aliases" check > > - add a name == NULL check > > - code style > > - move DTB fragment scanning earlier, before DomU GIC node creation > > - set guest_phandle_gic based on "/gic" > > - in-code comment > > > > 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 | 112 +++++++++++++++++++++++++++++++++++ > > xen/include/asm-arm/kernel.h | 2 +- > > 2 files changed, 113 insertions(+), 1 deletion(-) > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > index cd585f05ca..c71b9f2889 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> > > @@ -1713,6 +1714,111 @@ static int __init make_vpl011_uart_node(struct > > kernel_info *kinfo) > > } > > #endif > > +static int __init handle_prop_pfdt(struct kernel_info *kinfo, > > + const void *pfdt, int nodeoff, > > + uint32_t address_cells, uint32_t > > size_cells) > > Why do you need address_cells and size_cells in parameter? Yes, it will be necessary for later patches. > > +{ > > + void *fdt = kinfo->fdt; > > + int propoff, nameoff, res; > > + 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); > > + res = fdt_property(fdt, fdt_string(pfdt, nameoff), > > + prop->data, fdt32_to_cpu(prop->len)); > > + if ( res ) > > + return res; > > + } > > + > > + /* FDT_ERR_NOTFOUND => There is no more properties for this node */ > > + return ( propoff != -FDT_ERR_NOTFOUND ) ? propoff : 0; > > +} > > + > > +static int __init scan_pfdt_node(struct kernel_info *kinfo, const void > > *pfdt, > > + int nodeoff, int depth, > > + uint32_t address_cells, uint32_t > > size_cells) > > +{ > > + int rc = 0; > > + void *fdt = kinfo->fdt; > > + int node_next; > > + const char *name = fdt_get_name(pfdt, nodeoff, NULL); > > + > > + /* > > + * Take the GIC phandle value from the special /gic node in the DTB > > + * fragment. > > + */ > > + if ( depth == 1 && dt_node_cmp(name, "gic") == 0 ) > > + { > > + kinfo->guest_phandle_gic = fdt_get_phandle(pfdt, nodeoff); > > + return 0; > > + } > > I don't like this solution. You are bypassing most of the function just for > the benefits of have the name in hand. Can this be done separately? This would > also avoid to have an extra parameter (depth) for the only benefits of this > check. All right, I'll change it and remove depth. > > + > > + rc = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL)); > > + if ( rc ) > > + return rc; > > + > > + rc = handle_prop_pfdt(kinfo, pfdt, nodeoff, address_cells, size_cells); > > + if ( rc ) > > + return rc; > > + > > + address_cells = device_tree_get_u32(pfdt, nodeoff, "#address-cells", > > + address_cells); > > + size_cells = device_tree_get_u32(pfdt, nodeoff, "#size-cells", > > + size_cells); > > I am pretty sure I mention it before (though not on this patch...), this is > not matching the DT spec. address_cells and size_cells are not propagated to > the next level. So these should be DT_ROOT_NODE_{ADDR, SIZE}_CELLS_DEFAULT. They are only propagated from parent to children, not from parent to grandchildren. This function is recursive. In this case we are reading #address-cells and #size-cells just to pass it on by 1 level as by the spec. Am I misunderstanding something? > > + > > + node_next = fdt_first_subnode(pfdt, nodeoff); > > + while ( node_next > 0 ) > > + { > > + scan_pfdt_node(kinfo, pfdt, node_next, depth + 1, address_cells, > > size_cells); > > + node_next = fdt_next_subnode(pfdt, node_next); > > + } > > + > > + return fdt_end_node(fdt); > > +} > > + > > +static int __init domain_handle_dtb_bootmodule(struct domain *d, > > + struct kernel_info *kinfo) > > +{ > > + void *pfdt; > > + int res, node_next; > > + > > + pfdt = ioremap_cache(kinfo->dtb_bootmodule->start, > > + kinfo->dtb_bootmodule->size); > > + if ( pfdt == NULL ) > > + return -EFAULT; > > Shouldn't you check you actually have a device-tree in hand and that it fits > in the size specified by the user? See check_partial_fdt() in libxl_arm.c. Yes, you are right about that. I'll add a check_partial_fdt function call here. > > + > > + node_next = fdt_first_subnode(pfdt, 0); > > + while ( node_next > 0 ) > > + { > > Why do we have to go through the all the nodes of the first level? Can't we > just lookup for the path and copy the node as libxl does? Yes, we could do that, but fdt_path_offset is implemented as a loop anyway and we would still have the same "gic", "aliases" and "passthrough" checks. I tried the change but the code doesn't look much nicer and we would end up increasing runtime complexity. > > + const char *name = fdt_get_name(pfdt, node_next, NULL); > > + > > + /* only scan /gic /aliases /passthrough, ignore the rest */ > > + if ( name != NULL && > > + (dt_node_cmp(name, "passthrough") == 0 || > > + dt_node_cmp(name, "aliases") == 0 || > > + dt_node_cmp(name, "gic") == 0) ) > > + { > > + res = scan_pfdt_node(kinfo, pfdt, node_next, 1, > > + DT_ROOT_NODE_ADDR_CELLS_DEFAULT, > > + DT_ROOT_NODE_SIZE_CELLS_DEFAULT); > > + if ( res ) > > + return res; > > + } > > + > > + node_next = fdt_next_subnode(pfdt, node_next); > > + } > > + > > + 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. > > @@ -1768,6 +1874,12 @@ static int __init prepare_dtb_domU(struct domain *d, > > struct kernel_info *kinfo) > > if ( ret ) > > goto err; > > + if ( kinfo->dtb_bootmodule ) { > > Coding style: > > if ( ... ) > { I'll fix > > + ret = domain_handle_dtb_bootmodule(d, kinfo); > > + if ( ret ) > > + return ret; > > + } > > + > > ret = make_gic_domU_node(kinfo); > > if ( ret ) > > goto err; > > diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h > > index 760434369b..7f5e659561 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;
On 24/09/2019 22:06, Stefano Stabellini wrote: > On Wed, 11 Sep 2019, Julien Grall wrote: >> On 8/21/19 4:53 AM, Stefano Stabellini wrote: >>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> >>> >>> ---- >>> Changes in v4: >>> - use recursion in the implementation >>> - rename handle_properties to handle_prop_pfdt >>> - rename scan_pt_node to scan_pfdt_node >>> - pass kinfo to handle_properties >>> - use uint32_t instead of u32 >>> - rename r to res >>> - add "passthrough" and "aliases" check >>> - add a name == NULL check >>> - code style >>> - move DTB fragment scanning earlier, before DomU GIC node creation >>> - set guest_phandle_gic based on "/gic" >>> - in-code comment >>> >>> 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 | 112 +++++++++++++++++++++++++++++++++++ >>> xen/include/asm-arm/kernel.h | 2 +- >>> 2 files changed, 113 insertions(+), 1 deletion(-) >>> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index cd585f05ca..c71b9f2889 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> >>> @@ -1713,6 +1714,111 @@ static int __init make_vpl011_uart_node(struct >>> kernel_info *kinfo) >>> } >>> #endif >>> +static int __init handle_prop_pfdt(struct kernel_info *kinfo, >>> + const void *pfdt, int nodeoff, >>> + uint32_t address_cells, uint32_t >>> size_cells) >> >> Why do you need address_cells and size_cells in parameter? > > Yes, it will be necessary for later patches. ok. > > >>> +{ >>> + void *fdt = kinfo->fdt; >>> + int propoff, nameoff, res; >>> + 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); >>> + res = fdt_property(fdt, fdt_string(pfdt, nameoff), >>> + prop->data, fdt32_to_cpu(prop->len)); >>> + if ( res ) >>> + return res; >>> + } >>> + >>> + /* FDT_ERR_NOTFOUND => There is no more properties for this node */ >>> + return ( propoff != -FDT_ERR_NOTFOUND ) ? propoff : 0; >>> +} >>> + >>> +static int __init scan_pfdt_node(struct kernel_info *kinfo, const void >>> *pfdt, >>> + int nodeoff, int depth, >>> + uint32_t address_cells, uint32_t >>> size_cells) >>> +{ >>> + int rc = 0; >>> + void *fdt = kinfo->fdt; >>> + int node_next; >>> + const char *name = fdt_get_name(pfdt, nodeoff, NULL); >>> + >>> + /* >>> + * Take the GIC phandle value from the special /gic node in the DTB >>> + * fragment. >>> + */ >>> + if ( depth == 1 && dt_node_cmp(name, "gic") == 0 ) >>> + { >>> + kinfo->guest_phandle_gic = fdt_get_phandle(pfdt, nodeoff); >>> + return 0; >>> + } >> >> I don't like this solution. You are bypassing most of the function just for >> the benefits of have the name in hand. Can this be done separately? This would >> also avoid to have an extra parameter (depth) for the only benefits of this >> check. > > All right, I'll change it and remove depth. Thinking again about this function, you will allow a users to describe a device in the node /aliases. So there are more to do in this function. > > >>> + >>> + rc = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL)); >>> + if ( rc ) >>> + return rc; >>> + >>> + rc = handle_prop_pfdt(kinfo, pfdt, nodeoff, address_cells, size_cells); >>> + if ( rc ) >>> + return rc; >>> + >>> + address_cells = device_tree_get_u32(pfdt, nodeoff, "#address-cells", >>> + address_cells); >>> + size_cells = device_tree_get_u32(pfdt, nodeoff, "#size-cells", >>> + size_cells); >> >> I am pretty sure I mention it before (though not on this patch...), this is >> not matching the DT spec. address_cells and size_cells are not propagated to >> the next level. So these should be DT_ROOT_NODE_{ADDR, SIZE}_CELLS_DEFAULT. > > They are only propagated from parent to children, not from parent to > grandchildren. This function is recursive. In this case we are reading > #address-cells and #size-cells just to pass it on by 1 level as by the > spec. Am I misunderstanding something? I am afraid this is not correct. device_tree_get_u32 take the default number of cells as 3rd parameter. This is used if the property does not exist. So if the children does not have the two properties, then you will end up to use the parent's value as default when parsing grandchildren "reg" properties. >>> + >>> + node_next = fdt_first_subnode(pfdt, 0); >>> + while ( node_next > 0 ) >>> + { >> >> Why do we have to go through the all the nodes of the first level? Can't we >> just lookup for the path and copy the node as libxl does? > > Yes, we could do that, but fdt_path_offset is implemented as a loop > anyway and we would still have the same "gic", "aliases" and > "passthrough" checks. I tried the change but the code doesn't look much > nicer and we would end up increasing runtime complexity. This is ok as long as they don't depend on each other. This is not very clear from the code that "/gic" does not need to be parsed first, so you may want to explain in a comment. Cheers,
On Wed, 25 Sep 2019, Julien Grall wrote: > On 24/09/2019 22:06, Stefano Stabellini wrote: > > On Wed, 11 Sep 2019, Julien Grall wrote: > > > On 8/21/19 4:53 AM, Stefano Stabellini wrote: > > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > > > > > > > ---- > > > > Changes in v4: > > > > - use recursion in the implementation > > > > - rename handle_properties to handle_prop_pfdt > > > > - rename scan_pt_node to scan_pfdt_node > > > > - pass kinfo to handle_properties > > > > - use uint32_t instead of u32 > > > > - rename r to res > > > > - add "passthrough" and "aliases" check > > > > - add a name == NULL check > > > > - code style > > > > - move DTB fragment scanning earlier, before DomU GIC node creation > > > > - set guest_phandle_gic based on "/gic" > > > > - in-code comment > > > > > > > > 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 | 112 > > > > +++++++++++++++++++++++++++++++++++ > > > > xen/include/asm-arm/kernel.h | 2 +- > > > > 2 files changed, 113 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > > > index cd585f05ca..c71b9f2889 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> > > > > @@ -1713,6 +1714,111 @@ static int __init make_vpl011_uart_node(struct > > > > kernel_info *kinfo) > > > > } > > > > #endif > > > > +static int __init handle_prop_pfdt(struct kernel_info *kinfo, > > > > + const void *pfdt, int nodeoff, > > > > + uint32_t address_cells, uint32_t > > > > size_cells) > > > > > > Why do you need address_cells and size_cells in parameter? > > > > Yes, it will be necessary for later patches. > > ok. > > > > > > > > > +{ > > > > + void *fdt = kinfo->fdt; > > > > + int propoff, nameoff, res; > > > > + 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); > > > > + res = fdt_property(fdt, fdt_string(pfdt, nameoff), > > > > + prop->data, fdt32_to_cpu(prop->len)); > > > > + if ( res ) > > > > + return res; > > > > + } > > > > + > > > > + /* FDT_ERR_NOTFOUND => There is no more properties for this node */ > > > > + return ( propoff != -FDT_ERR_NOTFOUND ) ? propoff : 0; > > > > +} > > > > + > > > > +static int __init scan_pfdt_node(struct kernel_info *kinfo, const void > > > > *pfdt, > > > > + int nodeoff, int depth, > > > > + uint32_t address_cells, uint32_t > > > > size_cells) > > > > +{ > > > > + int rc = 0; > > > > + void *fdt = kinfo->fdt; > > > > + int node_next; > > > > + const char *name = fdt_get_name(pfdt, nodeoff, NULL); > > > > + > > > > + /* > > > > + * Take the GIC phandle value from the special /gic node in the DTB > > > > + * fragment. > > > > + */ > > > > + if ( depth == 1 && dt_node_cmp(name, "gic") == 0 ) > > > > + { > > > > + kinfo->guest_phandle_gic = fdt_get_phandle(pfdt, nodeoff); > > > > + return 0; > > > > + } > > > > > > I don't like this solution. You are bypassing most of the function just > > > for > > > the benefits of have the name in hand. Can this be done separately? This > > > would > > > also avoid to have an extra parameter (depth) for the only benefits of > > > this > > > check. > > > > All right, I'll change it and remove depth. > > Thinking again about this function, you will allow a users to describe a > device in the node /aliases. So there are more to do in this function. Yes, that's true. I can pass a parameter to make sure the proper behavior is applied to /aliases (only copy) and /passthrough (copy and look for device assignment properties). > > > > + > > > > + rc = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL)); > > > > + if ( rc ) > > > > + return rc; > > > > + > > > > + rc = handle_prop_pfdt(kinfo, pfdt, nodeoff, address_cells, > > > > size_cells); > > > > + if ( rc ) > > > > + return rc; > > > > + > > > > + address_cells = device_tree_get_u32(pfdt, nodeoff, > > > > "#address-cells", > > > > + address_cells); > > > > + size_cells = device_tree_get_u32(pfdt, nodeoff, "#size-cells", > > > > + size_cells); > > > > > > I am pretty sure I mention it before (though not on this patch...), this > > > is > > > not matching the DT spec. address_cells and size_cells are not propagated > > > to > > > the next level. So these should be DT_ROOT_NODE_{ADDR, > > > SIZE}_CELLS_DEFAULT. > > > > They are only propagated from parent to children, not from parent to > > grandchildren. This function is recursive. In this case we are reading > > #address-cells and #size-cells just to pass it on by 1 level as by the > > spec. Am I misunderstanding something? > > I am afraid this is not correct. device_tree_get_u32 take the default number > of cells as 3rd parameter. This is used if the property does not exist. > > So if the children does not have the two properties, then you will end up to > use the parent's value as default when parsing grandchildren "reg" properties. I understand your point now. I'll fix it. > > > > + > > > > + node_next = fdt_first_subnode(pfdt, 0); > > > > + while ( node_next > 0 ) > > > > + { > > > > > > Why do we have to go through the all the nodes of the first level? Can't > > > we > > > just lookup for the path and copy the node as libxl does? > > > > Yes, we could do that, but fdt_path_offset is implemented as a loop > > anyway and we would still have the same "gic", "aliases" and > > "passthrough" checks. I tried the change but the code doesn't look much > > nicer and we would end up increasing runtime complexity. > > This is ok as long as they don't depend on each other. This is not very clear > from the code that "/gic" does not need to be parsed first, so you may want to > explain in a comment. OK, I'll clarify
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index cd585f05ca..c71b9f2889 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> @@ -1713,6 +1714,111 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo) } #endif +static int __init handle_prop_pfdt(struct kernel_info *kinfo, + const void *pfdt, int nodeoff, + uint32_t address_cells, uint32_t size_cells) +{ + void *fdt = kinfo->fdt; + int propoff, nameoff, res; + 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); + res = fdt_property(fdt, fdt_string(pfdt, nameoff), + prop->data, fdt32_to_cpu(prop->len)); + if ( res ) + return res; + } + + /* FDT_ERR_NOTFOUND => There is no more properties for this node */ + return ( propoff != -FDT_ERR_NOTFOUND ) ? propoff : 0; +} + +static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt, + int nodeoff, int depth, + uint32_t address_cells, uint32_t size_cells) +{ + int rc = 0; + void *fdt = kinfo->fdt; + int node_next; + const char *name = fdt_get_name(pfdt, nodeoff, NULL); + + /* + * Take the GIC phandle value from the special /gic node in the DTB + * fragment. + */ + if ( depth == 1 && dt_node_cmp(name, "gic") == 0 ) + { + kinfo->guest_phandle_gic = fdt_get_phandle(pfdt, nodeoff); + return 0; + } + + rc = fdt_begin_node(fdt, fdt_get_name(pfdt, nodeoff, NULL)); + if ( rc ) + return rc; + + rc = handle_prop_pfdt(kinfo, pfdt, nodeoff, address_cells, size_cells); + if ( rc ) + return rc; + + address_cells = device_tree_get_u32(pfdt, nodeoff, "#address-cells", + address_cells); + size_cells = device_tree_get_u32(pfdt, nodeoff, "#size-cells", + size_cells); + + node_next = fdt_first_subnode(pfdt, nodeoff); + while ( node_next > 0 ) + { + scan_pfdt_node(kinfo, pfdt, node_next, depth + 1, address_cells, size_cells); + node_next = fdt_next_subnode(pfdt, node_next); + } + + return fdt_end_node(fdt); +} + +static int __init domain_handle_dtb_bootmodule(struct domain *d, + struct kernel_info *kinfo) +{ + void *pfdt; + int res, node_next; + + pfdt = ioremap_cache(kinfo->dtb_bootmodule->start, + kinfo->dtb_bootmodule->size); + if ( pfdt == NULL ) + return -EFAULT; + + node_next = fdt_first_subnode(pfdt, 0); + while ( node_next > 0 ) + { + const char *name = fdt_get_name(pfdt, node_next, NULL); + + /* only scan /gic /aliases /passthrough, ignore the rest */ + if ( name != NULL && + (dt_node_cmp(name, "passthrough") == 0 || + dt_node_cmp(name, "aliases") == 0 || + dt_node_cmp(name, "gic") == 0) ) + { + res = scan_pfdt_node(kinfo, pfdt, node_next, 1, + DT_ROOT_NODE_ADDR_CELLS_DEFAULT, + DT_ROOT_NODE_SIZE_CELLS_DEFAULT); + if ( res ) + return res; + } + + node_next = fdt_next_subnode(pfdt, node_next); + } + + 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. @@ -1768,6 +1874,12 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) if ( ret ) goto err; + if ( kinfo->dtb_bootmodule ) { + ret = domain_handle_dtb_bootmodule(d, kinfo); + if ( ret ) + return ret; + } + ret = make_gic_domU_node(kinfo); if ( ret ) goto err; diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h index 760434369b..7f5e659561 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,device-tree" compatible node. Add a new field named dtb_bootmodule to struct kernel_info to keep track of the dtb fragment location. Copy the fragment to the guest dtb (only /aliases and /passthrough). Set kinfo->guest_phandle_gic based on the phandle of the special "/gic" node in the device tree fragment. "/gic" is a dummy node in the dtb fragment that represents the gic interrupt controller. Other properties in the dtb fragment might refer to it (for instance interrupt-parent of a device node). We reuse the phandle of "/gic" from the dtb fragment as the phandle of the full GIC node that will be created for the guest device tree. That way, when we copy properties from the device tree fragment to the domU device tree the links remain unbroken. 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 v4: - use recursion in the implementation - rename handle_properties to handle_prop_pfdt - rename scan_pt_node to scan_pfdt_node - pass kinfo to handle_properties - use uint32_t instead of u32 - rename r to res - add "passthrough" and "aliases" check - add a name == NULL check - code style - move DTB fragment scanning earlier, before DomU GIC node creation - set guest_phandle_gic based on "/gic" - in-code comment 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 | 112 +++++++++++++++++++++++++++++++++++ xen/include/asm-arm/kernel.h | 2 +- 2 files changed, 113 insertions(+), 1 deletion(-)