| Message ID | 1377701263-3319-17-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote: > if ( early_info.modules.nr_mods >= MOD_KERNEL && > early_info.modules.module[MOD_KERNEL].cmdline[0] ) > bootargs = &early_info.modules.module[MOD_KERNEL].cmdline[0]; > > - for ( prop = fdt_first_property_offset(fdt, node); > - prop >= 0; > - prop = fdt_next_property_offset(fdt, prop) ) > + for_each_property_of_node (np, pp) Is "of" here as in "the property of the node" or is it a stray Open Firmware from the Linux naming of these functions? Perhaps a dt_ prefix to match all the others? > -/* Returns the next node in fdt (starting from offset) which should be > - * passed through to dom0. > +/* > + * Helper to write an interrupts with the GIC format > + * This code is assuming the irq is an PPI. > + * TODO: Handle SPI > */ > -static int fdt_next_dom0_node(const void *fdt, int node, > - int *depth_out) > -{ > - int depth = *depth_out; > - > - while ( (node = fdt_next_node(fdt, node, &depth)) && > - node >= 0 && depth >= 0 ) > - { > - if ( depth >= DEVICE_TREE_MAX_DEPTH ) > - break; > > - /* Skip /hypervisor/ node. We will inject our own. */ > - if ( fdt_node_check_compatible(fdt, node, "xen,xen" ) == 0 ) > - { > - printk("Device-tree contains \"xen,xen\" node. Ignoring.\n"); > - continue; > - } > +typedef __be32 interrupt_t[3]; > > - /* Skip multiboot subnodes */ > - if ( fdt_node_check_compatible(fdt, node, > - "xen,multiboot-module" ) == 0 ) > - continue; > +static void set_interrupts(interrupt_t interrupt, unsigned int irq, > + unsigned int cpumask, unsigned int level) > +{ > + __be32 *cells = interrupt; This function and the interrupt_t type is only used for the event-channel ppi? I think gic_interrupt_t would be a better typedef name The function name is plural but I think it only handles one interrupt at a time, and it only handles PPIs? Unless there are other users of this code I think it could continue to be inside make_hypervisor_node, given that it is pretty particular already. > > - /* We've arrived at a node which dom0 is interested in. */ > - break; > - } > + BUG_ON(irq < 16 && irq >= 32); > > - *depth_out = depth; > - return node; > + /* See linux Documentation/devictree/bindings/arm/gic.txt */ devicetree. > + dt_set_cell(&cells, 1, 1); /* is a PPI */ > + dt_set_cell(&cells, 1, irq - 16); /* PPIs start at 16 */ > + dt_set_cell(&cells, 1, (cpumask << 8) | level); > } > > -static void make_hypervisor_node(void *fdt, int addrcells, int sizecells) > +static int make_hypervisor_node(void *fdt, const struct dt_device_node *parent) > { > const char compat[] = > "xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0" > "xen,xen"; > - u32 reg[4]; > - u32 intr[3]; > - u32 *cell; > + __be32 reg[4]; > + interrupt_t intr[1]; A single element array seems a bit pointless in this context. > @@ -463,21 +418,39 @@ static int handle_node(struct domain *d, const struct dt_device_node *np) > return res; > } > > + /* > + * The property "name" is used to have a different name on older FDT "is used" => "used" > + * version. We want to keep the name retrieved during the tree > + * structure creation, that is store in the node path. "stored" This comment is saying that the name of the name property used to be something else? What was it? Which version of FDT was that -- do we need to care? Ian.
On 09/09/2013 12:33 PM, Ian Campbell wrote: > On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote: >> if ( early_info.modules.nr_mods >= MOD_KERNEL && >> early_info.modules.module[MOD_KERNEL].cmdline[0] ) >> bootargs = &early_info.modules.module[MOD_KERNEL].cmdline[0]; >> >> - for ( prop = fdt_first_property_offset(fdt, node); >> - prop >= 0; >> - prop = fdt_next_property_offset(fdt, prop) ) >> + for_each_property_of_node (np, pp) > > Is "of" here as in "the property of the node" or is it a stray Open > Firmware from the Linux naming of these functions? > > Perhaps a dt_ prefix to match all the others? Right. I will send a patch to rename for_each_property_of_node to dt_for_each_property_of_node. > >> -/* Returns the next node in fdt (starting from offset) which should be >> - * passed through to dom0. >> +/* >> + * Helper to write an interrupts with the GIC format >> + * This code is assuming the irq is an PPI. >> + * TODO: Handle SPI >> */ >> -static int fdt_next_dom0_node(const void *fdt, int node, >> - int *depth_out) >> -{ >> - int depth = *depth_out; >> - >> - while ( (node = fdt_next_node(fdt, node, &depth)) && >> - node >= 0 && depth >= 0 ) >> - { >> - if ( depth >= DEVICE_TREE_MAX_DEPTH ) >> - break; >> >> - /* Skip /hypervisor/ node. We will inject our own. */ >> - if ( fdt_node_check_compatible(fdt, node, "xen,xen" ) == 0 ) >> - { >> - printk("Device-tree contains \"xen,xen\" node. Ignoring.\n"); >> - continue; >> - } >> +typedef __be32 interrupt_t[3]; >> >> - /* Skip multiboot subnodes */ >> - if ( fdt_node_check_compatible(fdt, node, >> - "xen,multiboot-module" ) == 0 ) >> - continue; >> +static void set_interrupts(interrupt_t interrupt, unsigned int irq, >> + unsigned int cpumask, unsigned int level) >> +{ >> + __be32 *cells = interrupt; > > This function and the interrupt_t type is only used for the > event-channel ppi? I think gic_interrupt_t would be a better typedef > name > The function name is plural but I think it only handles one interrupt at > a time, and it only handles PPIs? > > Unless there are other users of this code I think it could continue to > be inside make_hypervisor_node, given that it is pretty particular > already. This function is used in another patch later. I will rename the different name in the next patch series. >> >> - /* We've arrived at a node which dom0 is interested in. */ >> - break; >> - } >> + BUG_ON(irq < 16 && irq >= 32); >> >> - *depth_out = depth; >> - return node; >> + /* See linux Documentation/devictree/bindings/arm/gic.txt */ > > devicetree. > >> + dt_set_cell(&cells, 1, 1); /* is a PPI */ >> + dt_set_cell(&cells, 1, irq - 16); /* PPIs start at 16 */ >> + dt_set_cell(&cells, 1, (cpumask << 8) | level); >> } >> >> -static void make_hypervisor_node(void *fdt, int addrcells, int sizecells) >> +static int make_hypervisor_node(void *fdt, const struct dt_device_node *parent) >> { >> const char compat[] = >> "xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0" >> "xen,xen"; >> - u32 reg[4]; >> - u32 intr[3]; >> - u32 *cell; >> + __be32 reg[4]; >> + interrupt_t intr[1]; > > A single element array seems a bit pointless in this context. > >> @@ -463,21 +418,39 @@ static int handle_node(struct domain *d, const struct dt_device_node *np) >> return res; >> } >> >> + /* >> + * The property "name" is used to have a different name on older FDT > > "is used" => "used" > >> + * version. We want to keep the name retrieved during the tree >> + * structure creation, that is store in the node path. > > "stored" > > This comment is saying that the name of the name property used to be > something else? What was it? Which version of FDT was that -- do we need > to care? Right, on older FDT version (< 0x10) each node has 2 different name: - the name just after FDT_BEGIN_NODE in the fdt which correspond to the "filename". - the name in property "name" which is a convenient name. So we can't use the name field in device tree to retrieve the name to create the node. For the FDT version, I don't know if we need to care. Linux pays attention to it in the device tree code.
On Mon, 2013-09-09 at 13:26 +0100, Julien Grall wrote: > On 09/09/2013 12:33 PM, Ian Campbell wrote: > > On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote: > >> if ( early_info.modules.nr_mods >= MOD_KERNEL && > >> early_info.modules.module[MOD_KERNEL].cmdline[0] ) > >> bootargs = &early_info.modules.module[MOD_KERNEL].cmdline[0]; > >> > >> - for ( prop = fdt_first_property_offset(fdt, node); > >> - prop >= 0; > >> - prop = fdt_next_property_offset(fdt, prop) ) > >> + for_each_property_of_node (np, pp) > > > > Is "of" here as in "the property of the node" or is it a stray Open > > Firmware from the Linux naming of these functions? > > > > Perhaps a dt_ prefix to match all the others? > > Right. I will send a patch to rename for_each_property_of_node to > dt_for_each_property_of_node. Might dt_for_each_node_property fit better with the other functions? > > > > This comment is saying that the name of the name property used to be > > something else? What was it? Which version of FDT was that -- do we need > > to care? > > Right, on older FDT version (< 0x10) each node has 2 different name: > - the name just after FDT_BEGIN_NODE in the fdt which correspond to > the "filename". > - the name in property "name" which is a convenient name. > > So we can't use the name field in device tree to retrieve the name to > create the node. > > For the FDT version, I don't know if we need to care. Linux pays > attention to it in the device tree code. I'm not sure we need to care either, I expect we will never see <0x10 in our uses (they are probbaly burnt into the ROMs of PPC machines) but of it is easy enough to so we might as well I guess? The alternative would be an explicit check for versions we know we understand. Ian.
On 09/09/2013 01:39 PM, Ian Campbell wrote: > On Mon, 2013-09-09 at 13:26 +0100, Julien Grall wrote: >> On 09/09/2013 12:33 PM, Ian Campbell wrote: >>> On Wed, 2013-08-28 at 15:47 +0100, Julien Grall wrote: >>>> if ( early_info.modules.nr_mods >= MOD_KERNEL && >>>> early_info.modules.module[MOD_KERNEL].cmdline[0] ) >>>> bootargs = &early_info.modules.module[MOD_KERNEL].cmdline[0]; >>>> >>>> - for ( prop = fdt_first_property_offset(fdt, node); >>>> - prop >= 0; >>>> - prop = fdt_next_property_offset(fdt, prop) ) >>>> + for_each_property_of_node (np, pp) >>> >>> Is "of" here as in "the property of the node" or is it a stray Open >>> Firmware from the Linux naming of these functions? >>> >>> Perhaps a dt_ prefix to match all the others? >> >> Right. I will send a patch to rename for_each_property_of_node to >> dt_for_each_property_of_node. > > Might dt_for_each_node_property fit better with the other functions? Sounds good. >>> >>> This comment is saying that the name of the name property used to be >>> something else? What was it? Which version of FDT was that -- do we need >>> to care? >> >> Right, on older FDT version (< 0x10) each node has 2 different name: >> - the name just after FDT_BEGIN_NODE in the fdt which correspond to >> the "filename". >> - the name in property "name" which is a convenient name. >> >> So we can't use the name field in device tree to retrieve the name to >> create the node. >> >> For the FDT version, I don't know if we need to care. Linux pays >> attention to it in the device tree code. > > I'm not sure we need to care either, I expect we will never see <0x10 in > our uses (they are probbaly burnt into the ROMs of PPC machines) but of > it is easy enough to so we might as well I guess? I forgot that there is another issue, the ePAR describes the name has node-name@unit-address. The name field will contains node-name and not the full node name. Lets say Xen only uses the field name (ie node-name) to create the FDT node name. We Linux will create the procfs for the device tree (/proc/devicetree), it's possible to have numerous warning because there is 2 nodes with the same name. > The alternative would be an explicit check for versions we know we > understand.
On Mon, 2013-09-09 at 22:53 +0100, Julien Grall wrote: > >>> > >>> This comment is saying that the name of the name property used to be > >>> something else? What was it? Which version of FDT was that -- do we need > >>> to care? > >> > >> Right, on older FDT version (< 0x10) each node has 2 different name: > >> - the name just after FDT_BEGIN_NODE in the fdt which correspond to > >> the "filename". > >> - the name in property "name" which is a convenient name. > >> > >> So we can't use the name field in device tree to retrieve the name to > >> create the node. > >> > >> For the FDT version, I don't know if we need to care. Linux pays > >> attention to it in the device tree code. > > > > I'm not sure we need to care either, I expect we will never see <0x10 in > > our uses (they are probbaly burnt into the ROMs of PPC machines) but of > > it is easy enough to so we might as well I guess? > > I forgot that there is another issue, the ePAR describes the name has > node-name@unit-address. The name field will contains node-name and not > the full node name. > > Lets say Xen only uses the field name (ie node-name) to create the FDT > node name. We Linux will create the procfs for the device tree > (/proc/devicetree), it's possible to have numerous warning because there > is 2 nodes with the same name. Yes, we need to avoid that. Isn't there a full_name field or something? (I've tripped over this in debugging, it's a bit annoying that name is just node-name and not unit-address too) Ian.
On 09/10/2013 09:58 AM, Ian Campbell wrote: > On Mon, 2013-09-09 at 22:53 +0100, Julien Grall wrote: > >>>>> >>>>> This comment is saying that the name of the name property used to be >>>>> something else? What was it? Which version of FDT was that -- do we need >>>>> to care? >>>> >>>> Right, on older FDT version (< 0x10) each node has 2 different name: >>>> - the name just after FDT_BEGIN_NODE in the fdt which correspond to >>>> the "filename". >>>> - the name in property "name" which is a convenient name. >>>> >>>> So we can't use the name field in device tree to retrieve the name to >>>> create the node. >>>> >>>> For the FDT version, I don't know if we need to care. Linux pays >>>> attention to it in the device tree code. >>> >>> I'm not sure we need to care either, I expect we will never see <0x10 in >>> our uses (they are probbaly burnt into the ROMs of PPC machines) but of >>> it is easy enough to so we might as well I guess? >> >> I forgot that there is another issue, the ePAR describes the name has >> node-name@unit-address. The name field will contains node-name and not >> the full node name. >> >> Lets say Xen only uses the field name (ie node-name) to create the FDT >> node name. We Linux will create the procfs for the device tree >> (/proc/devicetree), it's possible to have numerous warning because there >> is 2 nodes with the same name. > > Yes, we need to avoid that. Isn't there a full_name field or something? The full_name field contains the full path to this node, for instance /cpus/cpu@0. So we can retrieve the node-name@unit-address with a basename-like function. > (I've tripped over this in debugging, it's a bit annoying that name is > just node-name and not unit-address too)
On Tue, 2013-09-10 at 11:39 +0100, Julien Grall wrote: > On 09/10/2013 09:58 AM, Ian Campbell wrote: > > On Mon, 2013-09-09 at 22:53 +0100, Julien Grall wrote: > > > >>>>> > >>>>> This comment is saying that the name of the name property used to be > >>>>> something else? What was it? Which version of FDT was that -- do we need > >>>>> to care? > >>>> > >>>> Right, on older FDT version (< 0x10) each node has 2 different name: > >>>> - the name just after FDT_BEGIN_NODE in the fdt which correspond to > >>>> the "filename". > >>>> - the name in property "name" which is a convenient name. > >>>> > >>>> So we can't use the name field in device tree to retrieve the name to > >>>> create the node. > >>>> > >>>> For the FDT version, I don't know if we need to care. Linux pays > >>>> attention to it in the device tree code. > >>> > >>> I'm not sure we need to care either, I expect we will never see <0x10 in > >>> our uses (they are probbaly burnt into the ROMs of PPC machines) but of > >>> it is easy enough to so we might as well I guess? > >> > >> I forgot that there is another issue, the ePAR describes the name has > >> node-name@unit-address. The name field will contains node-name and not > >> the full node name. > >> > >> Lets say Xen only uses the field name (ie node-name) to create the FDT > >> node name. We Linux will create the procfs for the device tree > >> (/proc/devicetree), it's possible to have numerous warning because there > >> is 2 nodes with the same name. > > > > Yes, we need to avoid that. Isn't there a full_name field or something? > > The full_name field contains the full path to this node, for instance > /cpus/cpu@0. So we can retrieve the node-name@unit-address with a > basename-like function. Bit of a shame to have to jump through such hoop, but OK. Do we keep the unit-address in the datastructure?
On 09/10/2013 11:47 AM, Ian Campbell wrote: > On Tue, 2013-09-10 at 11:39 +0100, Julien Grall wrote: >> On 09/10/2013 09:58 AM, Ian Campbell wrote: >>> On Mon, 2013-09-09 at 22:53 +0100, Julien Grall wrote: >>> >>>>>>> >>>>>>> This comment is saying that the name of the name property used to be >>>>>>> something else? What was it? Which version of FDT was that -- do we need >>>>>>> to care? >>>>>> >>>>>> Right, on older FDT version (< 0x10) each node has 2 different name: >>>>>> - the name just after FDT_BEGIN_NODE in the fdt which correspond to >>>>>> the "filename". >>>>>> - the name in property "name" which is a convenient name. >>>>>> >>>>>> So we can't use the name field in device tree to retrieve the name to >>>>>> create the node. >>>>>> >>>>>> For the FDT version, I don't know if we need to care. Linux pays >>>>>> attention to it in the device tree code. >>>>> >>>>> I'm not sure we need to care either, I expect we will never see <0x10 in >>>>> our uses (they are probbaly burnt into the ROMs of PPC machines) but of >>>>> it is easy enough to so we might as well I guess? >>>> >>>> I forgot that there is another issue, the ePAR describes the name has >>>> node-name@unit-address. The name field will contains node-name and not >>>> the full node name. >>>> >>>> Lets say Xen only uses the field name (ie node-name) to create the FDT >>>> node name. We Linux will create the procfs for the device tree >>>> (/proc/devicetree), it's possible to have numerous warning because there >>>> is 2 nodes with the same name. >>> >>> Yes, we need to avoid that. Isn't there a full_name field or something? >> >> The full_name field contains the full path to this node, for instance >> /cpus/cpu@0. So we can retrieve the node-name@unit-address with a >> basename-like function. > > Bit of a shame to have to jump through such hoop, but OK. > > Do we keep the unit-address in the datastructure? No because the unit-address is optional and is equal to the first address of the property "reg".
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index eb8b662..76decf4 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -63,10 +63,10 @@ struct vcpu *__init alloc_dom0_vcpu0(void) } static int set_memory_reg_11(struct domain *d, struct kernel_info *kinfo, - const void *fdt, const u32 *cell, int len, - int address_cells, int size_cells, u32 *new_cell) + const struct dt_property *pp, + const struct dt_device_node *np, __be32 *new_cell) { - int reg_size = (address_cells + size_cells) * sizeof(*cell); + int reg_size = dt_cells_to_size(dt_n_addr_cells(np) + dt_n_size_cells(np)); paddr_t start; paddr_t size; struct page_info *pg; @@ -90,7 +90,7 @@ static int set_memory_reg_11(struct domain *d, struct kernel_info *kinfo, if ( res ) panic("Unable to add pages in DOM0: %d\n", res); - device_tree_set_reg(&new_cell, address_cells, size_cells, start, size); + dt_set_range(&new_cell, np, start, size); kinfo->mem.bank[0].start = start; kinfo->mem.bank[0].size = size; @@ -100,25 +100,30 @@ static int set_memory_reg_11(struct domain *d, struct kernel_info *kinfo, } static int set_memory_reg(struct domain *d, struct kernel_info *kinfo, - const void *fdt, const u32 *cell, int len, - int address_cells, int size_cells, u32 *new_cell) + const struct dt_property *pp, + const struct dt_device_node *np, __be32 *new_cell) { - int reg_size = (address_cells + size_cells) * sizeof(*cell); + int reg_size = dt_cells_to_size(dt_n_addr_cells(np) + dt_n_size_cells(np)); int l = 0; + unsigned int bank = 0; u64 start; u64 size; + int ret; if ( platform_has_quirk(PLATFORM_QUIRK_DOM0_MAPPING_11) ) - return set_memory_reg_11(d, kinfo, fdt, cell, len, address_cells, - size_cells, new_cell); + return set_memory_reg_11(d, kinfo, pp, np, new_cell); - while ( kinfo->unassigned_mem > 0 && l + reg_size <= len + while ( kinfo->unassigned_mem > 0 && l + reg_size <= pp->length && kinfo->mem.nr_banks < NR_MEM_BANKS ) { - device_tree_get_reg(&cell, address_cells, size_cells, &start, &size); + ret = dt_device_get_address(np, bank, &start, &size); + if ( ret ) + panic("Unable to retrieve the bank %u for %s\n", + bank, dt_node_full_name(np)); + if ( size > kinfo->unassigned_mem ) size = kinfo->unassigned_mem; - device_tree_set_reg(&new_cell, address_cells, size_cells, start, size); + dt_set_range(&new_cell, np, start, size); printk("Populate P2M %#"PRIx64"->%#"PRIx64"\n", start, start + size); if ( p2m_populate_ram(d, start, start + size) < 0 ) @@ -135,31 +140,21 @@ static int set_memory_reg(struct domain *d, struct kernel_info *kinfo, } static int write_properties(struct domain *d, struct kernel_info *kinfo, - const void *fdt, - int node, const char *name, int depth, - u32 address_cells, u32 size_cells) + const struct dt_device_node *np) { const char *bootargs = NULL; - int prop; + const struct dt_property *pp; + int res = 0; if ( early_info.modules.nr_mods >= MOD_KERNEL && early_info.modules.module[MOD_KERNEL].cmdline[0] ) bootargs = &early_info.modules.module[MOD_KERNEL].cmdline[0]; - for ( prop = fdt_first_property_offset(fdt, node); - prop >= 0; - prop = fdt_next_property_offset(fdt, prop) ) + for_each_property_of_node (np, pp) { - const struct fdt_property *p; - const char *prop_name; - const char *prop_data; - int prop_len; - char *new_data = NULL; - - p = fdt_get_property_by_offset(fdt, prop, NULL); - prop_name = fdt_string(fdt, fdt32_to_cpu(p->nameoff)); - prop_data = p->data; - prop_len = fdt32_to_cpu(p->len); + const void *prop_data = pp->value; + void *new_data = NULL; + u32 prop_len = pp->length; /* * In chosen node: @@ -168,105 +163,93 @@ static int write_properties(struct domain *d, struct kernel_info *kinfo, * bootargs (from module #1, above). * * remove bootargs and xen,dom0-bootargs. */ - if ( device_tree_node_matches(fdt, node, "chosen") ) + if ( dt_node_path_is_equal(np, "/chosen") ) { - if ( strcmp(prop_name, "bootargs") == 0 ) + if ( dt_property_name_is_equal(pp, "bootargs") ) continue; - else if ( strcmp(prop_name, "xen,dom0-bootargs") == 0 ) + else if ( dt_property_name_is_equal(pp, "xen,dom0-bootargs") ) { if ( !bootargs ) - bootargs = prop_data; + bootargs = pp->value; continue; } } /* * In a memory node: adjust reg property. + * TODO: handle properly memory node (ie: device_type = "memory") */ - else if ( device_tree_node_matches(fdt, node, "memory") ) + else if ( dt_node_name_is_equal(np, "memory") ) { - if ( strcmp(prop_name, "reg") == 0 ) + if ( dt_property_name_is_equal(pp, "reg") ) { - new_data = xzalloc_bytes(prop_len); + new_data = xzalloc_bytes(pp->length); if ( new_data == NULL ) return -FDT_ERR_XEN(ENOMEM); - prop_len = set_memory_reg(d, kinfo, fdt, - (u32 *)prop_data, prop_len, - address_cells, size_cells, - (u32 *)new_data); + prop_len = set_memory_reg(d, kinfo, pp, np, + (__be32 *)new_data); prop_data = new_data; } } - /* - * TODO: Should call map_mmio_regions() for all devices in the - * tree that have a "reg" parameter (except cpus). This - * requires looking into the parent node's "ranges" property - * to translate the bus address in the "reg" value into - * physical addresses. Regions also need to be rounded up to - * whole pages. - */ - - fdt_property(kinfo->fdt, prop_name, prop_data, prop_len); + res = fdt_property(kinfo->fdt, pp->name, prop_data, prop_len); xfree(new_data); + + if ( res ) + return res; } - if ( device_tree_node_matches(fdt, node, "chosen") && bootargs ) - fdt_property(kinfo->fdt, "bootargs", bootargs, strlen(bootargs) + 1); + if ( dt_node_path_is_equal(np, "/chosen") && bootargs ) + { + res = fdt_property(kinfo->fdt, "bootargs", bootargs, + strlen(bootargs) + 1); + if ( res ) + return res; + } /* * XXX should populate /chosen/linux,initrd-{start,end} here if we * have module[2] */ - if ( prop == -FDT_ERR_NOTFOUND ) - return 0; - return prop; + return 0; } -/* Returns the next node in fdt (starting from offset) which should be - * passed through to dom0. +/* + * Helper to write an interrupts with the GIC format + * This code is assuming the irq is an PPI. + * TODO: Handle SPI */ -static int fdt_next_dom0_node(const void *fdt, int node, - int *depth_out) -{ - int depth = *depth_out; - - while ( (node = fdt_next_node(fdt, node, &depth)) && - node >= 0 && depth >= 0 ) - { - if ( depth >= DEVICE_TREE_MAX_DEPTH ) - break; - /* Skip /hypervisor/ node. We will inject our own. */ - if ( fdt_node_check_compatible(fdt, node, "xen,xen" ) == 0 ) - { - printk("Device-tree contains \"xen,xen\" node. Ignoring.\n"); - continue; - } +typedef __be32 interrupt_t[3]; - /* Skip multiboot subnodes */ - if ( fdt_node_check_compatible(fdt, node, - "xen,multiboot-module" ) == 0 ) - continue; +static void set_interrupts(interrupt_t interrupt, unsigned int irq, + unsigned int cpumask, unsigned int level) +{ + __be32 *cells = interrupt; - /* We've arrived at a node which dom0 is interested in. */ - break; - } + BUG_ON(irq < 16 && irq >= 32); - *depth_out = depth; - return node; + /* See linux Documentation/devictree/bindings/arm/gic.txt */ + dt_set_cell(&cells, 1, 1); /* is a PPI */ + dt_set_cell(&cells, 1, irq - 16); /* PPIs start at 16 */ + dt_set_cell(&cells, 1, (cpumask << 8) | level); } -static void make_hypervisor_node(void *fdt, int addrcells, int sizecells) +static int make_hypervisor_node(void *fdt, const struct dt_device_node *parent) { const char compat[] = "xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0" "xen,xen"; - u32 reg[4]; - u32 intr[3]; - u32 *cell; + __be32 reg[4]; + interrupt_t intr[1]; + __be32 *cells; + int res; + int addrcells = dt_n_addr_cells(parent); + int sizecells = dt_n_size_cells(parent); + + DPRINT("Create hypervisor node\n"); /* * Sanity-check address sizes, since addresses and sizes which do @@ -277,82 +260,42 @@ static void make_hypervisor_node(void *fdt, int addrcells, int sizecells) panic("Cannot cope with this size"); /* See linux Documentation/devicetree/bindings/arm/xen.txt */ - fdt_begin_node(fdt, "hypervisor"); + res = fdt_begin_node(fdt, "hypervisor"); + if ( res ) + return res; /* Cannot use fdt_property_string due to embedded nulls */ - fdt_property(fdt, "compatible", compat, sizeof(compat)); + res = fdt_property(fdt, "compatible", compat, sizeof(compat)); + if ( res ) + return res; + DPRINT(" Grant table range: 0xb0000000-0x20000\n"); /* reg 0 is grant table space */ - cell = ®[0]; - device_tree_set_reg(&cell, addrcells, sizecells, 0xb0000000, 0x20000); - fdt_property(fdt, "reg", reg, - sizeof(reg[0]) * (addrcells + sizecells)); + cells = ®[0]; + dt_set_range(&cells, parent, 0xb0000000, 0x20000); + res = fdt_property(fdt, "reg", reg, + dt_cells_to_size(addrcells + sizecells)); + if ( res ) + return res; /* - * interrupts is evtchn upcall <1 15 0xf08> - * See linux Documentation/devicetree/bindings/arm/gic.txt + * interrupts is evtchn upcall: + * - Active-low level-sensitive + * - All cpus + * + * TODO: Handle correctly the cpumask */ - intr[0] = cpu_to_fdt32(1); /* is a PPI */ - intr[1] = cpu_to_fdt32(VGIC_IRQ_EVTCHN_CALLBACK - 16); /* PPIs start at 16 */ - intr[2] = cpu_to_fdt32(0xf08); /* Active-low level-sensitive */ - - fdt_property(fdt, "interrupts", intr, sizeof(intr[0]) * 3); - - fdt_end_node(fdt); -} - -static int write_nodes(struct domain *d, struct kernel_info *kinfo, - const void *fdt) -{ - int node; - int depth = 0, last_depth = -1; - u32 address_cells[DEVICE_TREE_MAX_DEPTH]; - u32 size_cells[DEVICE_TREE_MAX_DEPTH]; - int ret; - - for ( node = 0, depth = 0; - node >= 0 && depth >= 0; - node = fdt_next_dom0_node(fdt, node, &depth) ) - { - const char *name; - - name = fdt_get_name(fdt, node, NULL); - - if ( depth >= DEVICE_TREE_MAX_DEPTH ) - { - printk("warning: node `%s' is nested too deep (%d)\n", - name, depth); - continue; - } - - /* We cannot handle descending more than one level at a time */ - ASSERT( depth <= last_depth + 1 ); - - while ( last_depth-- >= depth ) - fdt_end_node(kinfo->fdt); - - address_cells[depth] = device_tree_get_u32(fdt, node, "#address-cells", - depth > 0 ? address_cells[depth-1] : 0); - size_cells[depth] = device_tree_get_u32(fdt, node, "#size-cells", - depth > 0 ? size_cells[depth-1] : 0); - - fdt_begin_node(kinfo->fdt, name); + DPRINT(" Event channel interrupt to %u\n", VGIC_IRQ_EVTCHN_CALLBACK); + set_interrupts(intr[0], VGIC_IRQ_EVTCHN_CALLBACK, 0xf, + DT_IRQ_TYPE_LEVEL_LOW); - ret = write_properties(d, kinfo, fdt, node, name, depth, - address_cells[depth-1], size_cells[depth-1]); - if ( ret < 0 ) - return ret; - - last_depth = depth; - } - - while ( last_depth-- >= 1 ) - fdt_end_node(kinfo->fdt); + res = fdt_property(fdt, "interrupts", intr, sizeof(intr[0])); + if ( res ) + return res; - make_hypervisor_node(kinfo->fdt, address_cells[0], size_cells[0]); + res = fdt_end_node(fdt); - fdt_end_node(kinfo->fdt); - return 0; + return res; } /* Map the device in the domain */ @@ -435,27 +378,39 @@ static int map_device(struct domain *d, const struct dt_device_node *dev) return 0; } -static int handle_node(struct domain *d, const struct dt_device_node *np) +static int handle_node(struct domain *d, struct kernel_info *kinfo, + const struct dt_device_node *np) { static const struct dt_device_match skip_matches[] __initconst = { DT_MATCH_COMPATIBLE("xen,xen"), - DT_MATCH_TYPE("memory"), - DT_MATCH_PATH("/chosen"), + DT_MATCH_COMPATIBLE("xen,multiboot-module"), { /* sentinel */ }, }; const struct dt_device_node *child; int res; + const char *name; + const char *path; + path = dt_node_full_name(np); - - DPRINT("handle %s\n", dt_node_full_name(np)); + DPRINT("handle %s\n", path); /* Skip theses nodes and the sub-nodes */ if ( dt_match_node(skip_matches, np ) ) + { + DPRINT(" Skip it!\n"); return 0; + } - if ( dt_device_used_by(np) != DOMID_XEN ) + /* + * Some device doesn't need to be mapped in Xen: + * - Device used by Xen: Obviously dom0 can't use them + * - Memory: the guest will see a different view of memory. It will + * be allocated later. + */ + if ( dt_device_used_by(np) != DOMID_XEN && + !dt_device_type_is_equal(np, "memory") ) { res = map_device(d, np); @@ -463,21 +418,39 @@ static int handle_node(struct domain *d, const struct dt_device_node *np) return res; } + /* + * The property "name" is used to have a different name on older FDT + * version. We want to keep the name retrieved during the tree + * structure creation, that is store in the node path. + */ + name = strrchr(path, '/'); + name = name ? name + 1 : path; + + res = fdt_begin_node(kinfo->fdt, name); + if ( res ) + return res; + + res = write_properties(d, kinfo, np); + if ( res ) + return res; + for ( child = np->child; child != NULL; child = child->sibling ) { - res = handle_node(d, child); + res = handle_node(d, kinfo, child); if ( res ) return res; } - return 0; -} + if ( np == dt_host ) + { + res = make_hypervisor_node(kinfo->fdt, np); + if ( res ) + return res; + } -static int map_devices_from_device_tree(struct domain *d) -{ - ASSERT(dt_host && (dt_host->sibling == NULL)); + res = fdt_end_node(kinfo->fdt); - return handle_node(d, dt_host); + return res; } static int prepare_dtb(struct domain *d, struct kernel_info *kinfo) @@ -487,6 +460,8 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo) int ret; paddr_t end; + ASSERT(dt_host && (dt_host->sibling == NULL)); + kinfo->unassigned_mem = dom0_mem; fdt = device_tree_flattened; @@ -502,8 +477,8 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo) fdt_finish_reservemap(kinfo->fdt); - ret = write_nodes(d, kinfo, fdt); - if ( ret < 0 ) + ret = handle_node(d, kinfo, dt_host); + if ( ret ) goto err; ret = fdt_finish(kinfo->fdt); @@ -575,7 +550,6 @@ int construct_dom0(struct domain *d) if ( rc < 0 ) return rc; - map_devices_from_device_tree(d); rc = platform_specific_mapping(d); if ( rc < 0 ) return rc;
Remove the usage of the FDT in benefit of the device tree structure. The latter is easier to use and can embedded meta-data for Xen (ie: is the device is used by Xen...). Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v2: - use dt_set_range - add set_interrupts - add more debug print - add comment to explain why some device are not mapped - mix kbasename directly in handle_node - fix malloc check when memory cells are allocated --- xen/arch/arm/domain_build.c | 310 ++++++++++++++++++++----------------------- 1 file changed, 142 insertions(+), 168 deletions(-)