Re: [PATCH v3 3/8] of/address: Introduce of_dma_get_max_cpu_address()
On Thu, Oct 15, 2020 at 12:42 AM Christoph Hellwig wrote: > > > +phys_addr_t __init of_dma_get_max_cpu_address(struct device_node *np) > > +{ > > + phys_addr_t max_cpu_addr = PHYS_ADDR_MAX; > > + struct of_range_parser parser; > > + phys_addr_t subtree_max_addr; > > + struct device_node *child; > > + phys_addr_t cpu_end = 0; > > + struct of_range range; > > + const __be32 *ranges; > > + int len; > > + > > + if (!np) > > + np = of_root; > > Requiring of_root to be passed explicitly would seem more natural > to me than the magic NULL argument. There doesn't seem to be any > precedent for that kind of calling convention either. I prefer that of_root is not more widely exposed and NULL regularly means 'the whole tree'. Rob ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 3/8] of/address: Introduce of_dma_get_max_cpu_address()
On Thu, 2020-10-15 at 07:42 +0200, Christoph Hellwig wrote: > > +phys_addr_t __init of_dma_get_max_cpu_address(struct device_node *np) > > +{ > > + phys_addr_t max_cpu_addr = PHYS_ADDR_MAX; > > + struct of_range_parser parser; > > + phys_addr_t subtree_max_addr; > > + struct device_node *child; > > + phys_addr_t cpu_end = 0; > > + struct of_range range; > > + const __be32 *ranges; > > + int len; > > + > > + if (!np) > > + np = of_root; > > Requiring of_root to be passed explicitly would seem more natural > to me than the magic NULL argument. There doesn't seem to be any > precedent for that kind of calling convention either. I inspired that behavior from __of_find_all_nodes(). I'll change it. signature.asc Description: This is a digitally signed message part ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 3/8] of/address: Introduce of_dma_get_max_cpu_address()
On Thu, 15 Oct 2020 at 11:16, Nicolas Saenz Julienne wrote: > > On Thu, 2020-10-15 at 08:56 +0200, Ard Biesheuvel wrote: > > On Thu, 15 Oct 2020 at 00:03, Rob Herring wrote: > > > On Wed, Oct 14, 2020 at 2:12 PM Nicolas Saenz Julienne > > > wrote: > > > > Introduce of_dma_get_max_cpu_address(), which provides the highest CPU > > > > physical address addressable by all DMA masters in the system. It's > > > > specially useful for setting memory zones sizes at early boot time. > > > > > > > > Signed-off-by: Nicolas Saenz Julienne > > > > > > > > --- > > > > > > > > Changes since v2: > > > > - Use PHYS_ADDR_MAX > > > > - return phys_dma_t > > > > - Rename function > > > > - Correct subject > > > > - Add support to start parsing from an arbitrary device node in order > > > >for the function to work with unit tests > > > > > > > > drivers/of/address.c | 42 ++ > > > > include/linux/of.h | 7 +++ > > > > 2 files changed, 49 insertions(+) > > > > > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c > > > > index eb9ab4f1e80b..b5a9695aaf82 100644 > > > > --- a/drivers/of/address.c > > > > +++ b/drivers/of/address.c > > > > @@ -1024,6 +1024,48 @@ int of_dma_get_range(struct device_node *np, > > > > const struct bus_dma_region **map) > > > > } > > > > #endif /* CONFIG_HAS_DMA */ > > > > > > > > +/** > > > > + * of_dma_get_max_cpu_address - Gets highest CPU address suitable for > > > > DMA > > > > + * @np: The node to start searching from or NULL to start from the root > > > > + * > > > > + * Gets the highest CPU physical address that is addressable by all > > > > DMA masters > > > > + * in the system (or subtree when np is non-NULL). If no DMA > > > > constrained device > > > > + * is found, it returns PHYS_ADDR_MAX. > > > > + */ > > > > +phys_addr_t __init of_dma_get_max_cpu_address(struct device_node *np) > > > > +{ > > > > + phys_addr_t max_cpu_addr = PHYS_ADDR_MAX; > > > > > > One issue with using phys_addr_t is it may be 32-bit even though the > > > DT is 64-bit addresses. LPAE capable system with LPAE disabled. Maybe > > > the truncation is fine here? Maybe not. > > > > > > > PHYS_ADDR_MAX is the max addressable CPU address on the system, and so > > it makes sense to use it for the return type, and for the preliminary > > return value: this is actually what /prevents/ truncation, because we > > will only overwrite max_cpu_addr if the new u64 value is lower. > > > > Actually I now see how things might go south. > > > > > + if (ranges && len) { > > > > + of_dma_range_parser_init(, np); > > > > + for_each_of_range(, ) > > > > + if (range.cpu_addr + range.size > cpu_end) > > > > + cpu_end = range.cpu_addr + range.size; > > If cpu_end hits 0x1_, it'll overflow to 0. This is possible on 32-bit > systems (LPAE or not). And something similar might happen on LPAE disabled > systems. > > I could add some extra logic, something like: > > /* We overflowed */ > if (cpu_end < range.cpu_addr) > cpu_end = PHYS_ADDR_MAX; > > Which is not perfect but will cover most sensible cases. > > Or simply deal internally in u64s, and upon returning, check if "max_cpu_addr" > falls higher than PHYS_ADDR_MAX. > Just use a u64 for cpu_end > > > > + > > > > + if (max_cpu_addr > cpu_end) > > > > + max_cpu_addr = cpu_end; ... then this comparison and assignment will work as expected. > > > > + } > > > > + > > > > + for_each_available_child_of_node(np, child) { > > > > + subtree_max_addr = of_dma_get_max_cpu_address(child); > > > > + if (max_cpu_addr > subtree_max_addr) > > > > + max_cpu_addr = subtree_max_addr; > > > > + } > > > > + > > > > + return max_cpu_addr; > > > > +} > > Regards, > Nicolas > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 3/8] of/address: Introduce of_dma_get_max_cpu_address()
On Thu, 2020-10-15 at 08:56 +0200, Ard Biesheuvel wrote: > On Thu, 15 Oct 2020 at 00:03, Rob Herring wrote: > > On Wed, Oct 14, 2020 at 2:12 PM Nicolas Saenz Julienne > > wrote: > > > Introduce of_dma_get_max_cpu_address(), which provides the highest CPU > > > physical address addressable by all DMA masters in the system. It's > > > specially useful for setting memory zones sizes at early boot time. > > > > > > Signed-off-by: Nicolas Saenz Julienne > > > > > > --- > > > > > > Changes since v2: > > > - Use PHYS_ADDR_MAX > > > - return phys_dma_t > > > - Rename function > > > - Correct subject > > > - Add support to start parsing from an arbitrary device node in order > > >for the function to work with unit tests > > > > > > drivers/of/address.c | 42 ++ > > > include/linux/of.h | 7 +++ > > > 2 files changed, 49 insertions(+) > > > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c > > > index eb9ab4f1e80b..b5a9695aaf82 100644 > > > --- a/drivers/of/address.c > > > +++ b/drivers/of/address.c > > > @@ -1024,6 +1024,48 @@ int of_dma_get_range(struct device_node *np, const > > > struct bus_dma_region **map) > > > } > > > #endif /* CONFIG_HAS_DMA */ > > > > > > +/** > > > + * of_dma_get_max_cpu_address - Gets highest CPU address suitable for DMA > > > + * @np: The node to start searching from or NULL to start from the root > > > + * > > > + * Gets the highest CPU physical address that is addressable by all DMA > > > masters > > > + * in the system (or subtree when np is non-NULL). If no DMA constrained > > > device > > > + * is found, it returns PHYS_ADDR_MAX. > > > + */ > > > +phys_addr_t __init of_dma_get_max_cpu_address(struct device_node *np) > > > +{ > > > + phys_addr_t max_cpu_addr = PHYS_ADDR_MAX; > > > > One issue with using phys_addr_t is it may be 32-bit even though the > > DT is 64-bit addresses. LPAE capable system with LPAE disabled. Maybe > > the truncation is fine here? Maybe not. > > > > PHYS_ADDR_MAX is the max addressable CPU address on the system, and so > it makes sense to use it for the return type, and for the preliminary > return value: this is actually what /prevents/ truncation, because we > will only overwrite max_cpu_addr if the new u64 value is lower. > Actually I now see how things might go south. > > > + if (ranges && len) { > > > + of_dma_range_parser_init(, np); > > > + for_each_of_range(, ) > > > + if (range.cpu_addr + range.size > cpu_end) > > > + cpu_end = range.cpu_addr + range.size; If cpu_end hits 0x1_, it'll overflow to 0. This is possible on 32-bit systems (LPAE or not). And something similar might happen on LPAE disabled systems. I could add some extra logic, something like: /* We overflowed */ if (cpu_end < range.cpu_addr) cpu_end = PHYS_ADDR_MAX; Which is not perfect but will cover most sensible cases. Or simply deal internally in u64s, and upon returning, check if "max_cpu_addr" falls higher than PHYS_ADDR_MAX. > > > + > > > + if (max_cpu_addr > cpu_end) > > > + max_cpu_addr = cpu_end; > > > + } > > > + > > > + for_each_available_child_of_node(np, child) { > > > + subtree_max_addr = of_dma_get_max_cpu_address(child); > > > + if (max_cpu_addr > subtree_max_addr) > > > + max_cpu_addr = subtree_max_addr; > > > + } > > > + > > > + return max_cpu_addr; > > > +} Regards, Nicolas signature.asc Description: This is a digitally signed message part ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 3/8] of/address: Introduce of_dma_get_max_cpu_address()
On Wed, 2020-10-14 at 17:02 -0500, Rob Herring wrote: > On Wed, Oct 14, 2020 at 2:12 PM Nicolas Saenz Julienne > wrote: > > Introduce of_dma_get_max_cpu_address(), which provides the highest CPU > > physical address addressable by all DMA masters in the system. It's > > specially useful for setting memory zones sizes at early boot time. > > > > Signed-off-by: Nicolas Saenz Julienne > > > > --- [...] > > + struct of_range_parser parser; > > + phys_addr_t subtree_max_addr; > > + struct device_node *child; > > + phys_addr_t cpu_end = 0; > > + struct of_range range; > > + const __be32 *ranges; > > + int len; > > + > > + if (!np) > > + np = of_root; > > + > > + ranges = of_get_property(np, "dma-ranges", ); > > I'm not really following why you changed the algorithm here. You're > skipping disabled nodes which is good. Was there some other reason? Yes, it's a little more complex. But I had to change it in order to be able to start parsing down from an arbitrary device node, which is needed for the unit tests. for_each_of_allnodes() and friends will traverse the whole tree, regardless of the starting point. I couldn't find a similar function that would just iterate over a subsection of the tree, so I went with this recursive approach. Regards, Nicolas signature.asc Description: This is a digitally signed message part ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 3/8] of/address: Introduce of_dma_get_max_cpu_address()
On Thu, 15 Oct 2020 at 00:03, Rob Herring wrote: > > On Wed, Oct 14, 2020 at 2:12 PM Nicolas Saenz Julienne > wrote: > > > > Introduce of_dma_get_max_cpu_address(), which provides the highest CPU > > physical address addressable by all DMA masters in the system. It's > > specially useful for setting memory zones sizes at early boot time. > > > > Signed-off-by: Nicolas Saenz Julienne > > > > --- > > > > Changes since v2: > > - Use PHYS_ADDR_MAX > > - return phys_dma_t > > - Rename function > > - Correct subject > > - Add support to start parsing from an arbitrary device node in order > >for the function to work with unit tests > > > > drivers/of/address.c | 42 ++ > > include/linux/of.h | 7 +++ > > 2 files changed, 49 insertions(+) > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c > > index eb9ab4f1e80b..b5a9695aaf82 100644 > > --- a/drivers/of/address.c > > +++ b/drivers/of/address.c > > @@ -1024,6 +1024,48 @@ int of_dma_get_range(struct device_node *np, const > > struct bus_dma_region **map) > > } > > #endif /* CONFIG_HAS_DMA */ > > > > +/** > > + * of_dma_get_max_cpu_address - Gets highest CPU address suitable for DMA > > + * @np: The node to start searching from or NULL to start from the root > > + * > > + * Gets the highest CPU physical address that is addressable by all DMA > > masters > > + * in the system (or subtree when np is non-NULL). If no DMA constrained > > device > > + * is found, it returns PHYS_ADDR_MAX. > > + */ > > +phys_addr_t __init of_dma_get_max_cpu_address(struct device_node *np) > > +{ > > + phys_addr_t max_cpu_addr = PHYS_ADDR_MAX; > > One issue with using phys_addr_t is it may be 32-bit even though the > DT is 64-bit addresses. LPAE capable system with LPAE disabled. Maybe > the truncation is fine here? Maybe not. > PHYS_ADDR_MAX is the max addressable CPU address on the system, and so it makes sense to use it for the return type, and for the preliminary return value: this is actually what /prevents/ truncation, because we will only overwrite max_cpu_addr if the new u64 value is lower. > > + struct of_range_parser parser; > > + phys_addr_t subtree_max_addr; > > + struct device_node *child; > > + phys_addr_t cpu_end = 0; > > + struct of_range range; > > + const __be32 *ranges; > > + int len; > > + > > + if (!np) > > + np = of_root; > > + > > + ranges = of_get_property(np, "dma-ranges", ); > > I'm not really following why you changed the algorithm here. You're > skipping disabled nodes which is good. Was there some other reason? > > > + if (ranges && len) { > > + of_dma_range_parser_init(, np); > > + for_each_of_range(, ) > > + if (range.cpu_addr + range.size > cpu_end) > > + cpu_end = range.cpu_addr + range.size; > > + > > + if (max_cpu_addr > cpu_end) > > + max_cpu_addr = cpu_end; > > + } > > + > > + for_each_available_child_of_node(np, child) { > > + subtree_max_addr = of_dma_get_max_cpu_address(child); > > + if (max_cpu_addr > subtree_max_addr) > > + max_cpu_addr = subtree_max_addr; > > + } > > + > > + return max_cpu_addr; > > +} > > + > > /** > > * of_dma_is_coherent - Check if device is coherent > > * @np:device node > > diff --git a/include/linux/of.h b/include/linux/of.h > > index 481ec0467285..db8db8f2c967 100644 > > --- a/include/linux/of.h > > +++ b/include/linux/of.h > > @@ -558,6 +558,8 @@ int of_map_id(struct device_node *np, u32 id, > >const char *map_name, const char *map_mask_name, > >struct device_node **target, u32 *id_out); > > > > +phys_addr_t of_dma_get_max_cpu_address(struct device_node *np); > > + > > #else /* CONFIG_OF */ > > > > static inline void of_core_init(void) > > @@ -995,6 +997,11 @@ static inline int of_map_id(struct device_node *np, > > u32 id, > > return -EINVAL; > > } > > > > +static inline phys_addr_t of_dma_get_max_cpu_address(struct device_node > > *np) > > +{ > > + return PHYS_ADDR_MAX; > > +} > > + > > #define of_match_ptr(_ptr) NULL > > #define of_match_node(_matches, _node) NULL > > #endif /* CONFIG_OF */ > > -- > > 2.28.0 > > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 3/8] of/address: Introduce of_dma_get_max_cpu_address()
> +phys_addr_t __init of_dma_get_max_cpu_address(struct device_node *np) > +{ > + phys_addr_t max_cpu_addr = PHYS_ADDR_MAX; > + struct of_range_parser parser; > + phys_addr_t subtree_max_addr; > + struct device_node *child; > + phys_addr_t cpu_end = 0; > + struct of_range range; > + const __be32 *ranges; > + int len; > + > + if (!np) > + np = of_root; Requiring of_root to be passed explicitly would seem more natural to me than the magic NULL argument. There doesn't seem to be any precedent for that kind of calling convention either. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 3/8] of/address: Introduce of_dma_get_max_cpu_address()
On Wed, Oct 14, 2020 at 2:12 PM Nicolas Saenz Julienne wrote: > > Introduce of_dma_get_max_cpu_address(), which provides the highest CPU > physical address addressable by all DMA masters in the system. It's > specially useful for setting memory zones sizes at early boot time. > > Signed-off-by: Nicolas Saenz Julienne > > --- > > Changes since v2: > - Use PHYS_ADDR_MAX > - return phys_dma_t > - Rename function > - Correct subject > - Add support to start parsing from an arbitrary device node in order >for the function to work with unit tests > > drivers/of/address.c | 42 ++ > include/linux/of.h | 7 +++ > 2 files changed, 49 insertions(+) > > diff --git a/drivers/of/address.c b/drivers/of/address.c > index eb9ab4f1e80b..b5a9695aaf82 100644 > --- a/drivers/of/address.c > +++ b/drivers/of/address.c > @@ -1024,6 +1024,48 @@ int of_dma_get_range(struct device_node *np, const > struct bus_dma_region **map) > } > #endif /* CONFIG_HAS_DMA */ > > +/** > + * of_dma_get_max_cpu_address - Gets highest CPU address suitable for DMA > + * @np: The node to start searching from or NULL to start from the root > + * > + * Gets the highest CPU physical address that is addressable by all DMA > masters > + * in the system (or subtree when np is non-NULL). If no DMA constrained > device > + * is found, it returns PHYS_ADDR_MAX. > + */ > +phys_addr_t __init of_dma_get_max_cpu_address(struct device_node *np) > +{ > + phys_addr_t max_cpu_addr = PHYS_ADDR_MAX; One issue with using phys_addr_t is it may be 32-bit even though the DT is 64-bit addresses. LPAE capable system with LPAE disabled. Maybe the truncation is fine here? Maybe not. > + struct of_range_parser parser; > + phys_addr_t subtree_max_addr; > + struct device_node *child; > + phys_addr_t cpu_end = 0; > + struct of_range range; > + const __be32 *ranges; > + int len; > + > + if (!np) > + np = of_root; > + > + ranges = of_get_property(np, "dma-ranges", ); I'm not really following why you changed the algorithm here. You're skipping disabled nodes which is good. Was there some other reason? > + if (ranges && len) { > + of_dma_range_parser_init(, np); > + for_each_of_range(, ) > + if (range.cpu_addr + range.size > cpu_end) > + cpu_end = range.cpu_addr + range.size; > + > + if (max_cpu_addr > cpu_end) > + max_cpu_addr = cpu_end; > + } > + > + for_each_available_child_of_node(np, child) { > + subtree_max_addr = of_dma_get_max_cpu_address(child); > + if (max_cpu_addr > subtree_max_addr) > + max_cpu_addr = subtree_max_addr; > + } > + > + return max_cpu_addr; > +} > + > /** > * of_dma_is_coherent - Check if device is coherent > * @np:device node > diff --git a/include/linux/of.h b/include/linux/of.h > index 481ec0467285..db8db8f2c967 100644 > --- a/include/linux/of.h > +++ b/include/linux/of.h > @@ -558,6 +558,8 @@ int of_map_id(struct device_node *np, u32 id, >const char *map_name, const char *map_mask_name, >struct device_node **target, u32 *id_out); > > +phys_addr_t of_dma_get_max_cpu_address(struct device_node *np); > + > #else /* CONFIG_OF */ > > static inline void of_core_init(void) > @@ -995,6 +997,11 @@ static inline int of_map_id(struct device_node *np, u32 > id, > return -EINVAL; > } > > +static inline phys_addr_t of_dma_get_max_cpu_address(struct device_node *np) > +{ > + return PHYS_ADDR_MAX; > +} > + > #define of_match_ptr(_ptr) NULL > #define of_match_node(_matches, _node) NULL > #endif /* CONFIG_OF */ > -- > 2.28.0 > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v3 3/8] of/address: Introduce of_dma_get_max_cpu_address()
Introduce of_dma_get_max_cpu_address(), which provides the highest CPU physical address addressable by all DMA masters in the system. It's specially useful for setting memory zones sizes at early boot time. Signed-off-by: Nicolas Saenz Julienne --- Changes since v2: - Use PHYS_ADDR_MAX - return phys_dma_t - Rename function - Correct subject - Add support to start parsing from an arbitrary device node in order for the function to work with unit tests drivers/of/address.c | 42 ++ include/linux/of.h | 7 +++ 2 files changed, 49 insertions(+) diff --git a/drivers/of/address.c b/drivers/of/address.c index eb9ab4f1e80b..b5a9695aaf82 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -1024,6 +1024,48 @@ int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map) } #endif /* CONFIG_HAS_DMA */ +/** + * of_dma_get_max_cpu_address - Gets highest CPU address suitable for DMA + * @np: The node to start searching from or NULL to start from the root + * + * Gets the highest CPU physical address that is addressable by all DMA masters + * in the system (or subtree when np is non-NULL). If no DMA constrained device + * is found, it returns PHYS_ADDR_MAX. + */ +phys_addr_t __init of_dma_get_max_cpu_address(struct device_node *np) +{ + phys_addr_t max_cpu_addr = PHYS_ADDR_MAX; + struct of_range_parser parser; + phys_addr_t subtree_max_addr; + struct device_node *child; + phys_addr_t cpu_end = 0; + struct of_range range; + const __be32 *ranges; + int len; + + if (!np) + np = of_root; + + ranges = of_get_property(np, "dma-ranges", ); + if (ranges && len) { + of_dma_range_parser_init(, np); + for_each_of_range(, ) + if (range.cpu_addr + range.size > cpu_end) + cpu_end = range.cpu_addr + range.size; + + if (max_cpu_addr > cpu_end) + max_cpu_addr = cpu_end; + } + + for_each_available_child_of_node(np, child) { + subtree_max_addr = of_dma_get_max_cpu_address(child); + if (max_cpu_addr > subtree_max_addr) + max_cpu_addr = subtree_max_addr; + } + + return max_cpu_addr; +} + /** * of_dma_is_coherent - Check if device is coherent * @np:device node diff --git a/include/linux/of.h b/include/linux/of.h index 481ec0467285..db8db8f2c967 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -558,6 +558,8 @@ int of_map_id(struct device_node *np, u32 id, const char *map_name, const char *map_mask_name, struct device_node **target, u32 *id_out); +phys_addr_t of_dma_get_max_cpu_address(struct device_node *np); + #else /* CONFIG_OF */ static inline void of_core_init(void) @@ -995,6 +997,11 @@ static inline int of_map_id(struct device_node *np, u32 id, return -EINVAL; } +static inline phys_addr_t of_dma_get_max_cpu_address(struct device_node *np) +{ + return PHYS_ADDR_MAX; +} + #define of_match_ptr(_ptr) NULL #define of_match_node(_matches, _node) NULL #endif /* CONFIG_OF */ -- 2.28.0 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu