[PATCH v5] FIT: Parse `load` and `entry` addresses.

[PATCH v5] FIT: Parse `load` and `entry` addresses.

[Christian Mauderer]

According to the U-Boot documentation for the FIT file format, the load
and entry have to be allways defined for a "kernel" or "standalone".
But Barebox ignored the parameters. That changes with this patch.

For backward compatibility the default address is still used for images
without `load` or `entry`.

Signed-off-by: Christian Mauderer <christian.mauderer@embedded-brains.de>
---
 common/blspec.c     |  1 +
 common/boot.c       |  1 +
 common/bootm.c      | 24 ++++++++++-
 common/image-fit.c  | 97 ++++++++++++++++++++++++++++++++++++++-------
 include/image-fit.h |  3 ++
 5 files changed, 110 insertions(+), 16 deletions(-)

diff --git a/common/blspec.c b/common/blspec.c
index 7fb62d310..050aed26a 100644
--- a/common/blspec.c
+++ b/common/blspec.c
@@ -142,6 +142,7 @@ static int blspec_boot(struct bootentry *be, int verbose, int dryrun)
 	globalvar_set_match("bootm.initrd", "");
 
 	bootm_data_init_defaults(&data);
+	data.os_entry = 0;
 
 	devicetree = blspec_entry_var_get(entry, "devicetree");
 	initrd = blspec_entry_var_get(entry, "initrd");
diff --git a/common/boot.c b/common/boot.c
index dcbe5cc2e..93ac1612d 100644
--- a/common/boot.c
+++ b/common/boot.c
@@ -104,6 +104,7 @@ static int bootscript_boot(struct bootentry *entry, int verbose, int dryrun)
 
 	bootm_data_init_defaults(&data);
 
+	data.os_entry = 0;
 	if (verbose)
 		data.verbose = verbose;
 	if (dryrun)
diff --git a/common/bootm.c b/common/bootm.c
index 366f31455..8f8dbe6cb 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -58,6 +58,7 @@ void bootm_data_init_defaults(struct bootm_data *data)
 {
 	data->initrd_address = UIMAGE_INVALID_ADDRESS;
 	data->os_address = UIMAGE_SOME_ADDRESS;
+	data->os_entry = UIMAGE_SOME_ADDRESS;
 	data->oftree_file = getenv_nonempty("global.bootm.oftree");
 	data->tee_file = getenv_nonempty("global.bootm.tee");
 	data->os_file = getenv_nonempty("global.bootm.image");
@@ -601,6 +602,7 @@ int bootm_boot(struct bootm_data *bootm_data)
 
 	if (IS_ENABLED(CONFIG_FITIMAGE) && os_type == filetype_oftree) {
 		struct fit_handle *fit;
+		static const char *kernel_img = "kernel";
 
 		fit = fit_open(data->os_file, data->verbose, data->verify);
 		if (IS_ERR(fit)) {
@@ -621,10 +623,28 @@ int bootm_boot(struct bootm_data *bootm_data)
 			goto err_out;
 		}
 
-		ret = fit_open_image(data->os_fit, data->fit_config, "kernel",
+		ret = fit_open_image(data->os_fit, data->fit_config, kernel_img,
 				     &data->fit_kernel, &data->fit_kernel_size);
 		if (ret)
 			goto err_out;
+		if (data->os_address == UIMAGE_SOME_ADDRESS) {
+			ret = fit_get_image_address(data->os_fit,
+						    data->fit_config,
+						    kernel_img,
+						    "load", &data->os_address);
+			if (ret)
+				goto err_out;
+		}
+		if (data->os_entry == UIMAGE_SOME_ADDRESS) {
+			unsigned long entry;
+			ret = fit_get_image_address(data->os_fit,
+						    data->fit_config,
+						    kernel_img,
+						    "entry", &entry);
+			if (!ret)
+				data->os_entry = entry - data->os_address;
+				/* Note: Error case used default value. */
+		}
 	}
 
 	if (os_type == filetype_uimage) {
@@ -672,6 +692,8 @@ int bootm_boot(struct bootm_data *bootm_data)
 
 	if (data->os_address == UIMAGE_SOME_ADDRESS)
 		data->os_address = UIMAGE_INVALID_ADDRESS;
+	if (data->os_entry == UIMAGE_SOME_ADDRESS)
+		data->os_entry = 0;
 
 	handler = bootm_find_handler(os_type, data);
 	if (!handler) {
diff --git a/common/image-fit.c b/common/image-fit.c
index 2681d62a9..61aa299b1 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -517,6 +517,83 @@ int fit_has_image(struct fit_handle *handle, void *configuration,
 	return 1;
 }
 
+static int fit_get_address(struct device_node *image, const char *property,
+			   unsigned long *addr)
+{
+	const __be32 *cell;
+	int len = 0;
+
+	cell = of_get_property(image, property, &len);
+	if (!cell)
+		return -EINVAL;
+	if (len > sizeof(*addr))
+		return -ENOTSUPP;
+
+	*addr = (unsigned long)of_read_number(cell, len / sizeof(*cell));
+	return 0;
+}
+
+static int
+fit_get_image(struct fit_handle *handle, void *configuration,
+	      const char **unit, struct device_node **image)
+{
+	struct device_node *conf_node = configuration;
+
+	if (conf_node) {
+		if (of_property_read_string(conf_node, *unit, unit)) {
+			pr_err("No image named '%s'\n", *unit);
+			return -ENOENT;
+		}
+	}
+
+	*image = of_get_child_by_name(handle->images, *unit);
+	if (!*image)
+		return -ENOENT;
+
+	return 0;
+}
+
+/**
+ * fit_get_image_address - Get an address from an image in a FIT image
+ * @handle: The FIT image handle
+ * @name: The name of the image to open
+ * @property: The name of the address to get (for example "load" or "entry")
+ * @address: The address given by the image
+ *
+ * Try to parse the @property in the image @name as an address. @configuration
+ * holds the cookie returned from fit_open_configuration() if the image is
+ * opened as part of a configuration, or NULL if the image is opened without a
+ * configuration. If it exists the value will be returned in @address. Otherwise
+ * @address won't be changed.
+ *
+ * Return: 0 for success, negative error code otherwise
+ */
+int fit_get_image_address(struct fit_handle *handle, void *configuration,
+			  const char *name, const char *property,
+			  unsigned long *address)
+{
+	struct device_node *image;
+	const char *unit = name;
+	int ret;
+
+	if (!address || !property || !name)
+		return -EINVAL;
+
+	ret = fit_get_image(handle, configuration, &unit, &image);
+	if (ret)
+		return ret;
+
+	pr_info("%s/%s: ", image->full_name, property);
+
+	ret = fit_get_address(image, property, address);
+	if (ret < 0)
+		pr_cont("<not found>\n");
+	else
+		pr_cont("0x%lx\n", *address);
+
+	return ret;
+}
+
 /**
  * fit_open_image - Open an image in a FIT image
  * @handle: The FIT image handle
@@ -539,24 +616,14 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
 		   unsigned long *outsize)
 {
 	struct device_node *image;
-	const char *unit, *type = NULL, *desc= "(no description)";
+	const char *unit = name, *type = NULL, *desc= "(no description)";
 	const void *data;
 	int data_len;
 	int ret = 0;
-	struct device_node *conf_node = configuration;
 
-	if (conf_node) {
-		if (of_property_read_string(conf_node, name, &unit)) {
-			pr_err("No image named '%s'\n", name);
-			return -ENOENT;
-		}
-	} else {
-		unit = name;
-	}
-
-	image = of_get_child_by_name(handle->images, unit);
-	if (!image)
-		return -ENOENT;
+	ret = fit_get_image(handle, configuration, &unit, &image);
+	if (ret)
+		return ret;
 
 	of_property_read_string(image, "description", &desc);
 	pr_info("image '%s': '%s'\n", unit, desc);
@@ -573,7 +640,7 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
 		return -EINVAL;
 	}
 
-	if (conf_node)
+	if (configuration)
 		ret = fit_verify_hash(handle, image, data, data_len);
 	else
 		ret = fit_image_verify_signature(handle, image, data, data_len);
diff --git a/include/image-fit.h b/include/image-fit.h
index 27c9e8351..f21545988 100644
--- a/include/image-fit.h
+++ b/include/image-fit.h
@@ -32,6 +32,9 @@ int fit_has_image(struct fit_handle *handle, void *configuration,
 int fit_open_image(struct fit_handle *handle, void *configuration,
 		   const char *name, const void **outdata,
 		   unsigned long *outsize);
+int fit_get_image_address(struct fit_handle *handle, void *configuration,
+			  const char *name, const char *property,
+			  unsigned long *address);
 
 void fit_close(struct fit_handle *handle);
 
-- 
2.26.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v5] FIT: Parse `load` and `entry` addresses.

[Christian Mauderer]

Hello,

are there any other changes to make that patch acceptable?

Best regards

Christian Mauderer

On 15/07/2020 11:26, Christian Mauderer wrote:
> According to the U-Boot documentation for the FIT file format, the load
> and entry have to be allways defined for a "kernel" or "standalone".
> But Barebox ignored the parameters. That changes with this patch.
> 
> For backward compatibility the default address is still used for images
> without `load` or `entry`.
> 
> Signed-off-by: Christian Mauderer <christian.mauderer@embedded-brains.de>
> ---
>  common/blspec.c     |  1 +
>  common/boot.c       |  1 +
>  common/bootm.c      | 24 ++++++++++-
>  common/image-fit.c  | 97 ++++++++++++++++++++++++++++++++++++++-------
>  include/image-fit.h |  3 ++
>  5 files changed, 110 insertions(+), 16 deletions(-)
> 
> diff --git a/common/blspec.c b/common/blspec.c
> index 7fb62d310..050aed26a 100644
> --- a/common/blspec.c
> +++ b/common/blspec.c
> @@ -142,6 +142,7 @@ static int blspec_boot(struct bootentry *be, int verbose, int dryrun)
>  	globalvar_set_match("bootm.initrd", "");
>  
>  	bootm_data_init_defaults(&data);
> +	data.os_entry = 0;
>  
>  	devicetree = blspec_entry_var_get(entry, "devicetree");
>  	initrd = blspec_entry_var_get(entry, "initrd");
> diff --git a/common/boot.c b/common/boot.c
> index dcbe5cc2e..93ac1612d 100644
> --- a/common/boot.c
> +++ b/common/boot.c
> @@ -104,6 +104,7 @@ static int bootscript_boot(struct bootentry *entry, int verbose, int dryrun)
>  
>  	bootm_data_init_defaults(&data);
>  
> +	data.os_entry = 0;
>  	if (verbose)
>  		data.verbose = verbose;
>  	if (dryrun)
> diff --git a/common/bootm.c b/common/bootm.c
> index 366f31455..8f8dbe6cb 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -58,6 +58,7 @@ void bootm_data_init_defaults(struct bootm_data *data)
>  {
>  	data->initrd_address = UIMAGE_INVALID_ADDRESS;
>  	data->os_address = UIMAGE_SOME_ADDRESS;
> +	data->os_entry = UIMAGE_SOME_ADDRESS;
>  	data->oftree_file = getenv_nonempty("global.bootm.oftree");
>  	data->tee_file = getenv_nonempty("global.bootm.tee");
>  	data->os_file = getenv_nonempty("global.bootm.image");
> @@ -601,6 +602,7 @@ int bootm_boot(struct bootm_data *bootm_data)
>  
>  	if (IS_ENABLED(CONFIG_FITIMAGE) && os_type == filetype_oftree) {
>  		struct fit_handle *fit;
> +		static const char *kernel_img = "kernel";
>  
>  		fit = fit_open(data->os_file, data->verbose, data->verify);
>  		if (IS_ERR(fit)) {
> @@ -621,10 +623,28 @@ int bootm_boot(struct bootm_data *bootm_data)
>  			goto err_out;
>  		}
>  
> -		ret = fit_open_image(data->os_fit, data->fit_config, "kernel",
> +		ret = fit_open_image(data->os_fit, data->fit_config, kernel_img,
>  				     &data->fit_kernel, &data->fit_kernel_size);
>  		if (ret)
>  			goto err_out;
> +		if (data->os_address == UIMAGE_SOME_ADDRESS) {
> +			ret = fit_get_image_address(data->os_fit,
> +						    data->fit_config,
> +						    kernel_img,
> +						    "load", &data->os_address);
> +			if (ret)
> +				goto err_out;
> +		}
> +		if (data->os_entry == UIMAGE_SOME_ADDRESS) {
> +			unsigned long entry;
> +			ret = fit_get_image_address(data->os_fit,
> +						    data->fit_config,
> +						    kernel_img,
> +						    "entry", &entry);
> +			if (!ret)
> +				data->os_entry = entry - data->os_address;
> +				/* Note: Error case used default value. */
> +		}
>  	}
>  
>  	if (os_type == filetype_uimage) {
> @@ -672,6 +692,8 @@ int bootm_boot(struct bootm_data *bootm_data)
>  
>  	if (data->os_address == UIMAGE_SOME_ADDRESS)
>  		data->os_address = UIMAGE_INVALID_ADDRESS;
> +	if (data->os_entry == UIMAGE_SOME_ADDRESS)
> +		data->os_entry = 0;
>  
>  	handler = bootm_find_handler(os_type, data);
>  	if (!handler) {
> diff --git a/common/image-fit.c b/common/image-fit.c
> index 2681d62a9..61aa299b1 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -517,6 +517,83 @@ int fit_has_image(struct fit_handle *handle, void *configuration,
>  	return 1;
>  }
>  
> +static int fit_get_address(struct device_node *image, const char *property,
> +			   unsigned long *addr)
> +{
> +	const __be32 *cell;
> +	int len = 0;
> +
> +	cell = of_get_property(image, property, &len);
> +	if (!cell)
> +		return -EINVAL;
> +	if (len > sizeof(*addr))
> +		return -ENOTSUPP;
> +
> +	*addr = (unsigned long)of_read_number(cell, len / sizeof(*cell));
> +	return 0;
> +}
> +
> +static int
> +fit_get_image(struct fit_handle *handle, void *configuration,
> +	      const char **unit, struct device_node **image)
> +{
> +	struct device_node *conf_node = configuration;
> +
> +	if (conf_node) {
> +		if (of_property_read_string(conf_node, *unit, unit)) {
> +			pr_err("No image named '%s'\n", *unit);
> +			return -ENOENT;
> +		}
> +	}
> +
> +	*image = of_get_child_by_name(handle->images, *unit);
> +	if (!*image)
> +		return -ENOENT;
> +
> +	return 0;
> +}
> +
> +/**
> + * fit_get_image_address - Get an address from an image in a FIT image
> + * @handle: The FIT image handle
> + * @name: The name of the image to open
> + * @property: The name of the address to get (for example "load" or "entry")
> + * @address: The address given by the image
> + *
> + * Try to parse the @property in the image @name as an address. @configuration
> + * holds the cookie returned from fit_open_configuration() if the image is
> + * opened as part of a configuration, or NULL if the image is opened without a
> + * configuration. If it exists the value will be returned in @address. Otherwise
> + * @address won't be changed.
> + *
> + * Return: 0 for success, negative error code otherwise
> + */
> +int fit_get_image_address(struct fit_handle *handle, void *configuration,
> +			  const char *name, const char *property,
> +			  unsigned long *address)
> +{
> +	struct device_node *image;
> +	const char *unit = name;
> +	int ret;
> +
> +	if (!address || !property || !name)
> +		return -EINVAL;
> +
> +	ret = fit_get_image(handle, configuration, &unit, &image);
> +	if (ret)
> +		return ret;
> +
> +	pr_info("%s/%s: ", image->full_name, property);
> +
> +	ret = fit_get_address(image, property, address);
> +	if (ret < 0)
> +		pr_cont("<not found>\n");
> +	else
> +		pr_cont("0x%lx\n", *address);
> +
> +	return ret;
> +}
> +
>  /**
>   * fit_open_image - Open an image in a FIT image
>   * @handle: The FIT image handle
> @@ -539,24 +616,14 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>  		   unsigned long *outsize)
>  {
>  	struct device_node *image;
> -	const char *unit, *type = NULL, *desc= "(no description)";
> +	const char *unit = name, *type = NULL, *desc= "(no description)";
>  	const void *data;
>  	int data_len;
>  	int ret = 0;
> -	struct device_node *conf_node = configuration;
>  
> -	if (conf_node) {
> -		if (of_property_read_string(conf_node, name, &unit)) {
> -			pr_err("No image named '%s'\n", name);
> -			return -ENOENT;
> -		}
> -	} else {
> -		unit = name;
> -	}
> -
> -	image = of_get_child_by_name(handle->images, unit);
> -	if (!image)
> -		return -ENOENT;
> +	ret = fit_get_image(handle, configuration, &unit, &image);
> +	if (ret)
> +		return ret;
>  
>  	of_property_read_string(image, "description", &desc);
>  	pr_info("image '%s': '%s'\n", unit, desc);
> @@ -573,7 +640,7 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>  		return -EINVAL;
>  	}
>  
> -	if (conf_node)
> +	if (configuration)
>  		ret = fit_verify_hash(handle, image, data, data_len);
>  	else
>  		ret = fit_image_verify_signature(handle, image, data, data_len);
> diff --git a/include/image-fit.h b/include/image-fit.h
> index 27c9e8351..f21545988 100644
> --- a/include/image-fit.h
> +++ b/include/image-fit.h
> @@ -32,6 +32,9 @@ int fit_has_image(struct fit_handle *handle, void *configuration,
>  int fit_open_image(struct fit_handle *handle, void *configuration,
>  		   const char *name, const void **outdata,
>  		   unsigned long *outsize);
> +int fit_get_image_address(struct fit_handle *handle, void *configuration,
> +			  const char *name, const char *property,
> +			  unsigned long *address);
>  
>  void fit_close(struct fit_handle *handle);
>  
> 

-- 
--------------------------------------------
embedded brains GmbH
Herr Christian Mauderer
Dornierstr. 4
D-82178 Puchheim
Germany
email: christian.mauderer@embedded-brains.de
Phone: +49-89-18 94 741 - 18
Fax:   +49-89-18 94 741 - 08
PGP: Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v5] FIT: Parse `load` and `entry` addresses.

[Ahmad Fatoum]

Hello Christian,

On 7/28/20 3:57 PM, Christian Mauderer wrote:
> Hello,
> 
> are there any other changes to make that patch acceptable?

Sascha is on vacation, but patches on the mailing list will
be reviewed before the next release.

Cheers
Ahmad

> 
> Best regards
> 
> Christian Mauderer
> 
> On 15/07/2020 11:26, Christian Mauderer wrote:
>> According to the U-Boot documentation for the FIT file format, the load
>> and entry have to be allways defined for a "kernel" or "standalone".
>> But Barebox ignored the parameters. That changes with this patch.
>>
>> For backward compatibility the default address is still used for images
>> without `load` or `entry`.
>>
>> Signed-off-by: Christian Mauderer <christian.mauderer@embedded-brains.de>
>> ---
>>  common/blspec.c     |  1 +
>>  common/boot.c       |  1 +
>>  common/bootm.c      | 24 ++++++++++-
>>  common/image-fit.c  | 97 ++++++++++++++++++++++++++++++++++++++-------
>>  include/image-fit.h |  3 ++
>>  5 files changed, 110 insertions(+), 16 deletions(-)
>>
>> diff --git a/common/blspec.c b/common/blspec.c
>> index 7fb62d310..050aed26a 100644
>> --- a/common/blspec.c
>> +++ b/common/blspec.c
>> @@ -142,6 +142,7 @@ static int blspec_boot(struct bootentry *be, int verbose, int dryrun)
>>  	globalvar_set_match("bootm.initrd", "");
>>  
>>  	bootm_data_init_defaults(&data);
>> +	data.os_entry = 0;
>>  
>>  	devicetree = blspec_entry_var_get(entry, "devicetree");
>>  	initrd = blspec_entry_var_get(entry, "initrd");
>> diff --git a/common/boot.c b/common/boot.c
>> index dcbe5cc2e..93ac1612d 100644
>> --- a/common/boot.c
>> +++ b/common/boot.c
>> @@ -104,6 +104,7 @@ static int bootscript_boot(struct bootentry *entry, int verbose, int dryrun)
>>  
>>  	bootm_data_init_defaults(&data);
>>  
>> +	data.os_entry = 0;
>>  	if (verbose)
>>  		data.verbose = verbose;
>>  	if (dryrun)
>> diff --git a/common/bootm.c b/common/bootm.c
>> index 366f31455..8f8dbe6cb 100644
>> --- a/common/bootm.c
>> +++ b/common/bootm.c
>> @@ -58,6 +58,7 @@ void bootm_data_init_defaults(struct bootm_data *data)
>>  {
>>  	data->initrd_address = UIMAGE_INVALID_ADDRESS;
>>  	data->os_address = UIMAGE_SOME_ADDRESS;
>> +	data->os_entry = UIMAGE_SOME_ADDRESS;
>>  	data->oftree_file = getenv_nonempty("global.bootm.oftree");
>>  	data->tee_file = getenv_nonempty("global.bootm.tee");
>>  	data->os_file = getenv_nonempty("global.bootm.image");
>> @@ -601,6 +602,7 @@ int bootm_boot(struct bootm_data *bootm_data)
>>  
>>  	if (IS_ENABLED(CONFIG_FITIMAGE) && os_type == filetype_oftree) {
>>  		struct fit_handle *fit;
>> +		static const char *kernel_img = "kernel";
>>  
>>  		fit = fit_open(data->os_file, data->verbose, data->verify);
>>  		if (IS_ERR(fit)) {
>> @@ -621,10 +623,28 @@ int bootm_boot(struct bootm_data *bootm_data)
>>  			goto err_out;
>>  		}
>>  
>> -		ret = fit_open_image(data->os_fit, data->fit_config, "kernel",
>> +		ret = fit_open_image(data->os_fit, data->fit_config, kernel_img,
>>  				     &data->fit_kernel, &data->fit_kernel_size);
>>  		if (ret)
>>  			goto err_out;
>> +		if (data->os_address == UIMAGE_SOME_ADDRESS) {
>> +			ret = fit_get_image_address(data->os_fit,
>> +						    data->fit_config,
>> +						    kernel_img,
>> +						    "load", &data->os_address);
>> +			if (ret)
>> +				goto err_out;
>> +		}
>> +		if (data->os_entry == UIMAGE_SOME_ADDRESS) {
>> +			unsigned long entry;
>> +			ret = fit_get_image_address(data->os_fit,
>> +						    data->fit_config,
>> +						    kernel_img,
>> +						    "entry", &entry);
>> +			if (!ret)
>> +				data->os_entry = entry - data->os_address;
>> +				/* Note: Error case used default value. */
>> +		}
>>  	}
>>  
>>  	if (os_type == filetype_uimage) {
>> @@ -672,6 +692,8 @@ int bootm_boot(struct bootm_data *bootm_data)
>>  
>>  	if (data->os_address == UIMAGE_SOME_ADDRESS)
>>  		data->os_address = UIMAGE_INVALID_ADDRESS;
>> +	if (data->os_entry == UIMAGE_SOME_ADDRESS)
>> +		data->os_entry = 0;
>>  
>>  	handler = bootm_find_handler(os_type, data);
>>  	if (!handler) {
>> diff --git a/common/image-fit.c b/common/image-fit.c
>> index 2681d62a9..61aa299b1 100644
>> --- a/common/image-fit.c
>> +++ b/common/image-fit.c
>> @@ -517,6 +517,83 @@ int fit_has_image(struct fit_handle *handle, void *configuration,
>>  	return 1;
>>  }
>>  
>> +static int fit_get_address(struct device_node *image, const char *property,
>> +			   unsigned long *addr)
>> +{
>> +	const __be32 *cell;
>> +	int len = 0;
>> +
>> +	cell = of_get_property(image, property, &len);
>> +	if (!cell)
>> +		return -EINVAL;
>> +	if (len > sizeof(*addr))
>> +		return -ENOTSUPP;
>> +
>> +	*addr = (unsigned long)of_read_number(cell, len / sizeof(*cell));
>> +	return 0;
>> +}
>> +
>> +static int
>> +fit_get_image(struct fit_handle *handle, void *configuration,
>> +	      const char **unit, struct device_node **image)
>> +{
>> +	struct device_node *conf_node = configuration;
>> +
>> +	if (conf_node) {
>> +		if (of_property_read_string(conf_node, *unit, unit)) {
>> +			pr_err("No image named '%s'\n", *unit);
>> +			return -ENOENT;
>> +		}
>> +	}
>> +
>> +	*image = of_get_child_by_name(handle->images, *unit);
>> +	if (!*image)
>> +		return -ENOENT;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * fit_get_image_address - Get an address from an image in a FIT image
>> + * @handle: The FIT image handle
>> + * @name: The name of the image to open
>> + * @property: The name of the address to get (for example "load" or "entry")
>> + * @address: The address given by the image
>> + *
>> + * Try to parse the @property in the image @name as an address. @configuration
>> + * holds the cookie returned from fit_open_configuration() if the image is
>> + * opened as part of a configuration, or NULL if the image is opened without a
>> + * configuration. If it exists the value will be returned in @address. Otherwise
>> + * @address won't be changed.
>> + *
>> + * Return: 0 for success, negative error code otherwise
>> + */
>> +int fit_get_image_address(struct fit_handle *handle, void *configuration,
>> +			  const char *name, const char *property,
>> +			  unsigned long *address)
>> +{
>> +	struct device_node *image;
>> +	const char *unit = name;
>> +	int ret;
>> +
>> +	if (!address || !property || !name)
>> +		return -EINVAL;
>> +
>> +	ret = fit_get_image(handle, configuration, &unit, &image);
>> +	if (ret)
>> +		return ret;
>> +
>> +	pr_info("%s/%s: ", image->full_name, property);
>> +
>> +	ret = fit_get_address(image, property, address);
>> +	if (ret < 0)
>> +		pr_cont("<not found>\n");
>> +	else
>> +		pr_cont("0x%lx\n", *address);
>> +
>> +	return ret;
>> +}
>> +
>>  /**
>>   * fit_open_image - Open an image in a FIT image
>>   * @handle: The FIT image handle
>> @@ -539,24 +616,14 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>>  		   unsigned long *outsize)
>>  {
>>  	struct device_node *image;
>> -	const char *unit, *type = NULL, *desc= "(no description)";
>> +	const char *unit = name, *type = NULL, *desc= "(no description)";
>>  	const void *data;
>>  	int data_len;
>>  	int ret = 0;
>> -	struct device_node *conf_node = configuration;
>>  
>> -	if (conf_node) {
>> -		if (of_property_read_string(conf_node, name, &unit)) {
>> -			pr_err("No image named '%s'\n", name);
>> -			return -ENOENT;
>> -		}
>> -	} else {
>> -		unit = name;
>> -	}
>> -
>> -	image = of_get_child_by_name(handle->images, unit);
>> -	if (!image)
>> -		return -ENOENT;
>> +	ret = fit_get_image(handle, configuration, &unit, &image);
>> +	if (ret)
>> +		return ret;
>>  
>>  	of_property_read_string(image, "description", &desc);
>>  	pr_info("image '%s': '%s'\n", unit, desc);
>> @@ -573,7 +640,7 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>>  		return -EINVAL;
>>  	}
>>  
>> -	if (conf_node)
>> +	if (configuration)
>>  		ret = fit_verify_hash(handle, image, data, data_len);
>>  	else
>>  		ret = fit_image_verify_signature(handle, image, data, data_len);
>> diff --git a/include/image-fit.h b/include/image-fit.h
>> index 27c9e8351..f21545988 100644
>> --- a/include/image-fit.h
>> +++ b/include/image-fit.h
>> @@ -32,6 +32,9 @@ int fit_has_image(struct fit_handle *handle, void *configuration,
>>  int fit_open_image(struct fit_handle *handle, void *configuration,
>>  		   const char *name, const void **outdata,
>>  		   unsigned long *outsize);
>> +int fit_get_image_address(struct fit_handle *handle, void *configuration,
>> +			  const char *name, const char *property,
>> +			  unsigned long *address);
>>  
>>  void fit_close(struct fit_handle *handle);
>>  
>>
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v5] FIT: Parse `load` and `entry` addresses.

[Christian Mauderer]

Hello Ahmed,

On 29/07/2020 19:23, Ahmad Fatoum wrote:
> Hello Christian,
> 
> On 7/28/20 3:57 PM, Christian Mauderer wrote:
>> Hello,
>>
>> are there any other changes to make that patch acceptable?
> 
> Sascha is on vacation, but patches on the mailing list will
> be reviewed before the next release.

Thanks for the information. I wasn't aware of that procedure.

Best regards

Christian

> 
> Cheers
> Ahmad
> 
>>
>> Best regards
>>
>> Christian Mauderer
>>
>> On 15/07/2020 11:26, Christian Mauderer wrote:
>>> According to the U-Boot documentation for the FIT file format, the load
>>> and entry have to be allways defined for a "kernel" or "standalone".
>>> But Barebox ignored the parameters. That changes with this patch.
>>>
>>> For backward compatibility the default address is still used for images
>>> without `load` or `entry`.
>>>
>>> Signed-off-by: Christian Mauderer <christian.mauderer@embedded-brains.de>
>>> ---
>>>  common/blspec.c     |  1 +
>>>  common/boot.c       |  1 +
>>>  common/bootm.c      | 24 ++++++++++-
>>>  common/image-fit.c  | 97 ++++++++++++++++++++++++++++++++++++++-------
>>>  include/image-fit.h |  3 ++
>>>  5 files changed, 110 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/common/blspec.c b/common/blspec.c
>>> index 7fb62d310..050aed26a 100644
>>> --- a/common/blspec.c
>>> +++ b/common/blspec.c
>>> @@ -142,6 +142,7 @@ static int blspec_boot(struct bootentry *be, int verbose, int dryrun)
>>>  	globalvar_set_match("bootm.initrd", "");
>>>  
>>>  	bootm_data_init_defaults(&data);
>>> +	data.os_entry = 0;
>>>  
>>>  	devicetree = blspec_entry_var_get(entry, "devicetree");
>>>  	initrd = blspec_entry_var_get(entry, "initrd");
>>> diff --git a/common/boot.c b/common/boot.c
>>> index dcbe5cc2e..93ac1612d 100644
>>> --- a/common/boot.c
>>> +++ b/common/boot.c
>>> @@ -104,6 +104,7 @@ static int bootscript_boot(struct bootentry *entry, int verbose, int dryrun)
>>>  
>>>  	bootm_data_init_defaults(&data);
>>>  
>>> +	data.os_entry = 0;
>>>  	if (verbose)
>>>  		data.verbose = verbose;
>>>  	if (dryrun)
>>> diff --git a/common/bootm.c b/common/bootm.c
>>> index 366f31455..8f8dbe6cb 100644
>>> --- a/common/bootm.c
>>> +++ b/common/bootm.c
>>> @@ -58,6 +58,7 @@ void bootm_data_init_defaults(struct bootm_data *data)
>>>  {
>>>  	data->initrd_address = UIMAGE_INVALID_ADDRESS;
>>>  	data->os_address = UIMAGE_SOME_ADDRESS;
>>> +	data->os_entry = UIMAGE_SOME_ADDRESS;
>>>  	data->oftree_file = getenv_nonempty("global.bootm.oftree");
>>>  	data->tee_file = getenv_nonempty("global.bootm.tee");
>>>  	data->os_file = getenv_nonempty("global.bootm.image");
>>> @@ -601,6 +602,7 @@ int bootm_boot(struct bootm_data *bootm_data)
>>>  
>>>  	if (IS_ENABLED(CONFIG_FITIMAGE) && os_type == filetype_oftree) {
>>>  		struct fit_handle *fit;
>>> +		static const char *kernel_img = "kernel";
>>>  
>>>  		fit = fit_open(data->os_file, data->verbose, data->verify);
>>>  		if (IS_ERR(fit)) {
>>> @@ -621,10 +623,28 @@ int bootm_boot(struct bootm_data *bootm_data)
>>>  			goto err_out;
>>>  		}
>>>  
>>> -		ret = fit_open_image(data->os_fit, data->fit_config, "kernel",
>>> +		ret = fit_open_image(data->os_fit, data->fit_config, kernel_img,
>>>  				     &data->fit_kernel, &data->fit_kernel_size);
>>>  		if (ret)
>>>  			goto err_out;
>>> +		if (data->os_address == UIMAGE_SOME_ADDRESS) {
>>> +			ret = fit_get_image_address(data->os_fit,
>>> +						    data->fit_config,
>>> +						    kernel_img,
>>> +						    "load", &data->os_address);
>>> +			if (ret)
>>> +				goto err_out;
>>> +		}
>>> +		if (data->os_entry == UIMAGE_SOME_ADDRESS) {
>>> +			unsigned long entry;
>>> +			ret = fit_get_image_address(data->os_fit,
>>> +						    data->fit_config,
>>> +						    kernel_img,
>>> +						    "entry", &entry);
>>> +			if (!ret)
>>> +				data->os_entry = entry - data->os_address;
>>> +				/* Note: Error case used default value. */
>>> +		}
>>>  	}
>>>  
>>>  	if (os_type == filetype_uimage) {
>>> @@ -672,6 +692,8 @@ int bootm_boot(struct bootm_data *bootm_data)
>>>  
>>>  	if (data->os_address == UIMAGE_SOME_ADDRESS)
>>>  		data->os_address = UIMAGE_INVALID_ADDRESS;
>>> +	if (data->os_entry == UIMAGE_SOME_ADDRESS)
>>> +		data->os_entry = 0;
>>>  
>>>  	handler = bootm_find_handler(os_type, data);
>>>  	if (!handler) {
>>> diff --git a/common/image-fit.c b/common/image-fit.c
>>> index 2681d62a9..61aa299b1 100644
>>> --- a/common/image-fit.c
>>> +++ b/common/image-fit.c
>>> @@ -517,6 +517,83 @@ int fit_has_image(struct fit_handle *handle, void *configuration,
>>>  	return 1;
>>>  }
>>>  
>>> +static int fit_get_address(struct device_node *image, const char *property,
>>> +			   unsigned long *addr)
>>> +{
>>> +	const __be32 *cell;
>>> +	int len = 0;
>>> +
>>> +	cell = of_get_property(image, property, &len);
>>> +	if (!cell)
>>> +		return -EINVAL;
>>> +	if (len > sizeof(*addr))
>>> +		return -ENOTSUPP;
>>> +
>>> +	*addr = (unsigned long)of_read_number(cell, len / sizeof(*cell));
>>> +	return 0;
>>> +}
>>> +
>>> +static int
>>> +fit_get_image(struct fit_handle *handle, void *configuration,
>>> +	      const char **unit, struct device_node **image)
>>> +{
>>> +	struct device_node *conf_node = configuration;
>>> +
>>> +	if (conf_node) {
>>> +		if (of_property_read_string(conf_node, *unit, unit)) {
>>> +			pr_err("No image named '%s'\n", *unit);
>>> +			return -ENOENT;
>>> +		}
>>> +	}
>>> +
>>> +	*image = of_get_child_by_name(handle->images, *unit);
>>> +	if (!*image)
>>> +		return -ENOENT;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +/**
>>> + * fit_get_image_address - Get an address from an image in a FIT image
>>> + * @handle: The FIT image handle
>>> + * @name: The name of the image to open
>>> + * @property: The name of the address to get (for example "load" or "entry")
>>> + * @address: The address given by the image
>>> + *
>>> + * Try to parse the @property in the image @name as an address. @configuration
>>> + * holds the cookie returned from fit_open_configuration() if the image is
>>> + * opened as part of a configuration, or NULL if the image is opened without a
>>> + * configuration. If it exists the value will be returned in @address. Otherwise
>>> + * @address won't be changed.
>>> + *
>>> + * Return: 0 for success, negative error code otherwise
>>> + */
>>> +int fit_get_image_address(struct fit_handle *handle, void *configuration,
>>> +			  const char *name, const char *property,
>>> +			  unsigned long *address)
>>> +{
>>> +	struct device_node *image;
>>> +	const char *unit = name;
>>> +	int ret;
>>> +
>>> +	if (!address || !property || !name)
>>> +		return -EINVAL;
>>> +
>>> +	ret = fit_get_image(handle, configuration, &unit, &image);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	pr_info("%s/%s: ", image->full_name, property);
>>> +
>>> +	ret = fit_get_address(image, property, address);
>>> +	if (ret < 0)
>>> +		pr_cont("<not found>\n");
>>> +	else
>>> +		pr_cont("0x%lx\n", *address);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>  /**
>>>   * fit_open_image - Open an image in a FIT image
>>>   * @handle: The FIT image handle
>>> @@ -539,24 +616,14 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>>>  		   unsigned long *outsize)
>>>  {
>>>  	struct device_node *image;
>>> -	const char *unit, *type = NULL, *desc= "(no description)";
>>> +	const char *unit = name, *type = NULL, *desc= "(no description)";
>>>  	const void *data;
>>>  	int data_len;
>>>  	int ret = 0;
>>> -	struct device_node *conf_node = configuration;
>>>  
>>> -	if (conf_node) {
>>> -		if (of_property_read_string(conf_node, name, &unit)) {
>>> -			pr_err("No image named '%s'\n", name);
>>> -			return -ENOENT;
>>> -		}
>>> -	} else {
>>> -		unit = name;
>>> -	}
>>> -
>>> -	image = of_get_child_by_name(handle->images, unit);
>>> -	if (!image)
>>> -		return -ENOENT;
>>> +	ret = fit_get_image(handle, configuration, &unit, &image);
>>> +	if (ret)
>>> +		return ret;
>>>  
>>>  	of_property_read_string(image, "description", &desc);
>>>  	pr_info("image '%s': '%s'\n", unit, desc);
>>> @@ -573,7 +640,7 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>>>  		return -EINVAL;
>>>  	}
>>>  
>>> -	if (conf_node)
>>> +	if (configuration)
>>>  		ret = fit_verify_hash(handle, image, data, data_len);
>>>  	else
>>>  		ret = fit_image_verify_signature(handle, image, data, data_len);
>>> diff --git a/include/image-fit.h b/include/image-fit.h
>>> index 27c9e8351..f21545988 100644
>>> --- a/include/image-fit.h
>>> +++ b/include/image-fit.h
>>> @@ -32,6 +32,9 @@ int fit_has_image(struct fit_handle *handle, void *configuration,
>>>  int fit_open_image(struct fit_handle *handle, void *configuration,
>>>  		   const char *name, const void **outdata,
>>>  		   unsigned long *outsize);
>>> +int fit_get_image_address(struct fit_handle *handle, void *configuration,
>>> +			  const char *name, const char *property,
>>> +			  unsigned long *address);
>>>  
>>>  void fit_close(struct fit_handle *handle);
>>>  
>>>
>>
> 

-- 
--------------------------------------------
embedded brains GmbH
Herr Christian Mauderer
Dornierstr. 4
D-82178 Puchheim
Germany
email: christian.mauderer@embedded-brains.de
Phone: +49-89-18 94 741 - 18
Fax:   +49-89-18 94 741 - 08
PGP: Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v5] FIT: Parse `load` and `entry` addresses.

[Sascha Hauer]

Hi Christian,

On Wed, Jul 15, 2020 at 11:26:56AM +0200, Christian Mauderer wrote:
> According to the U-Boot documentation for the FIT file format, the load
> and entry have to be allways defined for a "kernel" or "standalone".
> But Barebox ignored the parameters. That changes with this patch.
> 
> For backward compatibility the default address is still used for images
> without `load` or `entry`.
> 
> Signed-off-by: Christian Mauderer <christian.mauderer@embedded-brains.de>
> ---
>  common/blspec.c     |  1 +
>  common/boot.c       |  1 +
>  common/bootm.c      | 24 ++++++++++-
>  common/image-fit.c  | 97 ++++++++++++++++++++++++++++++++++++++-------
>  include/image-fit.h |  3 ++
>  5 files changed, 110 insertions(+), 16 deletions(-)
> 
> diff --git a/common/blspec.c b/common/blspec.c
> index 7fb62d310..050aed26a 100644
> --- a/common/blspec.c
> +++ b/common/blspec.c
> @@ -142,6 +142,7 @@ static int blspec_boot(struct bootentry *be, int verbose, int dryrun)
>  	globalvar_set_match("bootm.initrd", "");
>  
>  	bootm_data_init_defaults(&data);
> +	data.os_entry = 0;

You set data.os_entry explicitly to 0 here...

>  
>  	devicetree = blspec_entry_var_get(entry, "devicetree");
>  	initrd = blspec_entry_var_get(entry, "initrd");
> diff --git a/common/boot.c b/common/boot.c
> index dcbe5cc2e..93ac1612d 100644
> --- a/common/boot.c
> +++ b/common/boot.c
> @@ -104,6 +104,7 @@ static int bootscript_boot(struct bootentry *entry, int verbose, int dryrun)
>  
>  	bootm_data_init_defaults(&data);
>  
> +	data.os_entry = 0;

...and here. Why is this done? I think these should be left to the
default UIMAGE_SOME_ADDRESS. In the end the kernels bootet from blspec
or a boot script could be a FIT image as well.

> +int fit_get_image_address(struct fit_handle *handle, void *configuration,
> +			  const char *name, const char *property,
> +			  unsigned long *address)
> +{
> +	struct device_node *image;
> +	const char *unit = name;
> +	int ret;
> +
> +	if (!address || !property || !name)
> +		return -EINVAL;
> +
> +	ret = fit_get_image(handle, configuration, &unit, &image);
> +	if (ret)
> +		return ret;
> +
> +	pr_info("%s/%s: ", image->full_name, property);
> +
> +	ret = fit_get_address(image, property, address);
> +	if (ret < 0)
> +		pr_cont("<not found>\n");
> +	else
> +		pr_cont("0x%lx\n", *address);

pr_cont() doesn't work well in barebox and should be avoided.

Also I think this function shouldn't print anything, the caller should
if it wishes to.

Otherwise the patch looks fine to me.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v5] FIT: Parse `load` and `entry` addresses.

[Christian Mauderer]

Hello Sascha,

thanks for the review.

On 11/08/2020 09:57, Sascha Hauer wrote:
> Hi Christian,
> 
> On Wed, Jul 15, 2020 at 11:26:56AM +0200, Christian Mauderer wrote:
>> According to the U-Boot documentation for the FIT file format, the load
>> and entry have to be allways defined for a "kernel" or "standalone".
>> But Barebox ignored the parameters. That changes with this patch.
>>
>> For backward compatibility the default address is still used for images
>> without `load` or `entry`.
>>
>> Signed-off-by: Christian Mauderer <christian.mauderer@embedded-brains.de>
>> ---
>>  common/blspec.c     |  1 +
>>  common/boot.c       |  1 +
>>  common/bootm.c      | 24 ++++++++++-
>>  common/image-fit.c  | 97 ++++++++++++++++++++++++++++++++++++++-------
>>  include/image-fit.h |  3 ++
>>  5 files changed, 110 insertions(+), 16 deletions(-)
>>
>> diff --git a/common/blspec.c b/common/blspec.c
>> index 7fb62d310..050aed26a 100644
>> --- a/common/blspec.c
>> +++ b/common/blspec.c
>> @@ -142,6 +142,7 @@ static int blspec_boot(struct bootentry *be, int verbose, int dryrun)
>>  	globalvar_set_match("bootm.initrd", "");
>>  
>>  	bootm_data_init_defaults(&data);
>> +	data.os_entry = 0;
> 
> You set data.os_entry explicitly to 0 here...
> 
>>  
>>  	devicetree = blspec_entry_var_get(entry, "devicetree");
>>  	initrd = blspec_entry_var_get(entry, "initrd");
>> diff --git a/common/boot.c b/common/boot.c
>> index dcbe5cc2e..93ac1612d 100644
>> --- a/common/boot.c
>> +++ b/common/boot.c
>> @@ -104,6 +104,7 @@ static int bootscript_boot(struct bootentry *entry, int verbose, int dryrun)
>>  
>>  	bootm_data_init_defaults(&data);
>>  
>> +	data.os_entry = 0;
> 
> ...and here. Why is this done? I think these should be left to the
> default UIMAGE_SOME_ADDRESS. In the end the kernels bootet from blspec
> or a boot script could be a FIT image as well.
> 

You maybe noted that I added the default of UIMAGE_SOME_ADDRESS to
bootm_data_init_defaults. I think that it is a sensible default and it
was useful for adding the command.

Before I did that, in these two cases the value for os_entry was
initialized with 0. With setting it explicitly to 0 I wanted to make
sure that the behavior doesn't change.

But you are right: I added a check for that in bootm_boot later. I just
checked again: There is no case where the os_entry is used in between.
So these two should be not unnecessary.

I'll remove it in a v6 of the patch.

>> +int fit_get_image_address(struct fit_handle *handle, void *configuration,
>> +			  const char *name, const char *property,
>> +			  unsigned long *address)
>> +{
>> +	struct device_node *image;
>> +	const char *unit = name;
>> +	int ret;
>> +
>> +	if (!address || !property || !name)
>> +		return -EINVAL;
>> +
>> +	ret = fit_get_image(handle, configuration, &unit, &image);
>> +	if (ret)
>> +		return ret;
>> +
>> +	pr_info("%s/%s: ", image->full_name, property);
>> +
>> +	ret = fit_get_address(image, property, address);
>> +	if (ret < 0)
>> +		pr_cont("<not found>\n");
>> +	else
>> +		pr_cont("0x%lx\n", *address);
> 
> pr_cont() doesn't work well in barebox and should be avoided.

I wasn't aware of that. In one of the earlier versions of the patch it
was suggested to print that info. I'll find another solution or remove it.

> 
> Also I think this function shouldn't print anything, the caller should
> if it wishes to.

I had the impression that most of the functions print the information
themselves. For example fit_open_image prints a lot of information about
the image. fit_find_compatible_unit (which is used in
fit_open_configuration) prints that it found a matching unit.

It is a bit unclear when it would be OK for a function to print anything
and when not. But I can move the print to bootm_boot where the function
is called. Or would you prefer that it is removed completely? I'm not
sure whether bootm_boot prints that information later?

Best regards

Christian

> 
> Otherwise the patch looks fine to me.
> 
> Sascha
> 

-- 
--------------------------------------------
embedded brains GmbH
Herr Christian Mauderer
Dornierstr. 4
D-82178 Puchheim
Germany
email: christian.mauderer@embedded-brains.de
Phone: +49-89-18 94 741 - 18
Fax:   +49-89-18 94 741 - 08
PGP: Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v5] FIT: Parse `load` and `entry` addresses.

[Sascha Hauer]

On Wed, Aug 12, 2020 at 08:47:47AM +0200, Christian Mauderer wrote:
> Hello Sascha,
> 
> thanks for the review.
> 
> On 11/08/2020 09:57, Sascha Hauer wrote:
> > Hi Christian,
> > 
> > On Wed, Jul 15, 2020 at 11:26:56AM +0200, Christian Mauderer wrote:
> >> According to the U-Boot documentation for the FIT file format, the load
> >> and entry have to be allways defined for a "kernel" or "standalone".
> >> But Barebox ignored the parameters. That changes with this patch.
> >>
> >> For backward compatibility the default address is still used for images
> >> without `load` or `entry`.
> >>
> >> Signed-off-by: Christian Mauderer <christian.mauderer@embedded-brains.de>
> >> ---
> >>  common/blspec.c     |  1 +
> >>  common/boot.c       |  1 +
> >>  common/bootm.c      | 24 ++++++++++-
> >>  common/image-fit.c  | 97 ++++++++++++++++++++++++++++++++++++++-------
> >>  include/image-fit.h |  3 ++
> >>  5 files changed, 110 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/common/blspec.c b/common/blspec.c
> >> index 7fb62d310..050aed26a 100644
> >> --- a/common/blspec.c
> >> +++ b/common/blspec.c
> >> @@ -142,6 +142,7 @@ static int blspec_boot(struct bootentry *be, int verbose, int dryrun)
> >>  	globalvar_set_match("bootm.initrd", "");
> >>  
> >>  	bootm_data_init_defaults(&data);
> >> +	data.os_entry = 0;
> > 
> > You set data.os_entry explicitly to 0 here...
> > 
> >>  
> >>  	devicetree = blspec_entry_var_get(entry, "devicetree");
> >>  	initrd = blspec_entry_var_get(entry, "initrd");
> >> diff --git a/common/boot.c b/common/boot.c
> >> index dcbe5cc2e..93ac1612d 100644
> >> --- a/common/boot.c
> >> +++ b/common/boot.c
> >> @@ -104,6 +104,7 @@ static int bootscript_boot(struct bootentry *entry, int verbose, int dryrun)
> >>  
> >>  	bootm_data_init_defaults(&data);
> >>  
> >> +	data.os_entry = 0;
> > 
> > ...and here. Why is this done? I think these should be left to the
> > default UIMAGE_SOME_ADDRESS. In the end the kernels bootet from blspec
> > or a boot script could be a FIT image as well.
> > 
> 
> You maybe noted that I added the default of UIMAGE_SOME_ADDRESS to
> bootm_data_init_defaults. I think that it is a sensible default and it
> was useful for adding the command.

Yes.

> 
> Before I did that, in these two cases the value for os_entry was
> initialized with 0. With setting it explicitly to 0 I wanted to make
> sure that the behavior doesn't change.
> 
> But you are right: I added a check for that in bootm_boot later. I just
> checked again: There is no case where the os_entry is used in between.
> So these two should be not unnecessary.
> 
> I'll remove it in a v6 of the patch.

Ok, thanks

> 
> >> +int fit_get_image_address(struct fit_handle *handle, void *configuration,
> >> +			  const char *name, const char *property,
> >> +			  unsigned long *address)
> >> +{
> >> +	struct device_node *image;
> >> +	const char *unit = name;
> >> +	int ret;
> >> +
> >> +	if (!address || !property || !name)
> >> +		return -EINVAL;
> >> +
> >> +	ret = fit_get_image(handle, configuration, &unit, &image);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	pr_info("%s/%s: ", image->full_name, property);
> >> +
> >> +	ret = fit_get_address(image, property, address);
> >> +	if (ret < 0)
> >> +		pr_cont("<not found>\n");
> >> +	else
> >> +		pr_cont("0x%lx\n", *address);
> > 
> > pr_cont() doesn't work well in barebox and should be avoided.
> 
> I wasn't aware of that. In one of the earlier versions of the patch it
> was suggested to print that info. I'll find another solution or remove it.
> 
> > 
> > Also I think this function shouldn't print anything, the caller should
> > if it wishes to.
> 
> I had the impression that most of the functions print the information
> themselves. For example fit_open_image prints a lot of information about
> the image. fit_find_compatible_unit (which is used in
> fit_open_configuration) prints that it found a matching unit.
> 
> It is a bit unclear when it would be OK for a function to print anything
> and when not.

Indeed it is unclear :)

Generally it's nice when a function prints some information, but here I
had the feeling that this function might get called in places where we
don't want to print anything. It doesn't matter much at the moment since
this function is called in this single place only anyway.

> But I can move the print to bootm_boot where the function
> is called. Or would you prefer that it is removed completely? I'm not
> sure whether bootm_boot prints that information later?

bootm will print later where it puts the kernel, but not where it got
the address from, so I think printing it here is valuable.

I just noticed that with your patch bootm refuses to boot FIT images
that don't have load and entry address explicitly given, right? That
shouldn't be the case.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v5] FIT: Parse `load` and `entry` addresses.

[Christian Mauderer]

Hello Sascha,

On 12/08/2020 12:08, Sascha Hauer wrote:
> On Wed, Aug 12, 2020 at 08:47:47AM +0200, Christian Mauderer wrote:
>> Hello Sascha,
>>
>> thanks for the review.
>>
>> On 11/08/2020 09:57, Sascha Hauer wrote:
>>> Hi Christian,
>>>
>>> On Wed, Jul 15, 2020 at 11:26:56AM +0200, Christian Mauderer wrote:
>>>> According to the U-Boot documentation for the FIT file format, the load
>>>> and entry have to be allways defined for a "kernel" or "standalone".
>>>> But Barebox ignored the parameters. That changes with this patch.
>>>>
>>>> For backward compatibility the default address is still used for images
>>>> without `load` or `entry`.
>>>>
>>>> Signed-off-by: Christian Mauderer <christian.mauderer@embedded-brains.de>
>>>> ---
>>>>  common/blspec.c     |  1 +
>>>>  common/boot.c       |  1 +
>>>>  common/bootm.c      | 24 ++++++++++-
>>>>  common/image-fit.c  | 97 ++++++++++++++++++++++++++++++++++++++-------
>>>>  include/image-fit.h |  3 ++
>>>>  5 files changed, 110 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/common/blspec.c b/common/blspec.c
>>>> index 7fb62d310..050aed26a 100644
>>>> --- a/common/blspec.c
>>>> +++ b/common/blspec.c
>>>> @@ -142,6 +142,7 @@ static int blspec_boot(struct bootentry *be, int verbose, int dryrun)
>>>>  	globalvar_set_match("bootm.initrd", "");
>>>>  
>>>>  	bootm_data_init_defaults(&data);
>>>> +	data.os_entry = 0;
>>>
>>> You set data.os_entry explicitly to 0 here...
>>>
>>>>  
>>>>  	devicetree = blspec_entry_var_get(entry, "devicetree");
>>>>  	initrd = blspec_entry_var_get(entry, "initrd");
>>>> diff --git a/common/boot.c b/common/boot.c
>>>> index dcbe5cc2e..93ac1612d 100644
>>>> --- a/common/boot.c
>>>> +++ b/common/boot.c
>>>> @@ -104,6 +104,7 @@ static int bootscript_boot(struct bootentry *entry, int verbose, int dryrun)
>>>>  
>>>>  	bootm_data_init_defaults(&data);
>>>>  
>>>> +	data.os_entry = 0;
>>>
>>> ...and here. Why is this done? I think these should be left to the
>>> default UIMAGE_SOME_ADDRESS. In the end the kernels bootet from blspec
>>> or a boot script could be a FIT image as well.
>>>
>>
>> You maybe noted that I added the default of UIMAGE_SOME_ADDRESS to
>> bootm_data_init_defaults. I think that it is a sensible default and it
>> was useful for adding the command.
> 
> Yes.
> 
>>
>> Before I did that, in these two cases the value for os_entry was
>> initialized with 0. With setting it explicitly to 0 I wanted to make
>> sure that the behavior doesn't change.
>>
>> But you are right: I added a check for that in bootm_boot later. I just
>> checked again: There is no case where the os_entry is used in between.
>> So these two should be not unnecessary.
>>
>> I'll remove it in a v6 of the patch.
> 
> Ok, thanks
> 
>>
>>>> +int fit_get_image_address(struct fit_handle *handle, void *configuration,
>>>> +			  const char *name, const char *property,
>>>> +			  unsigned long *address)
>>>> +{
>>>> +	struct device_node *image;
>>>> +	const char *unit = name;
>>>> +	int ret;
>>>> +
>>>> +	if (!address || !property || !name)
>>>> +		return -EINVAL;
>>>> +
>>>> +	ret = fit_get_image(handle, configuration, &unit, &image);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	pr_info("%s/%s: ", image->full_name, property);
>>>> +
>>>> +	ret = fit_get_address(image, property, address);
>>>> +	if (ret < 0)
>>>> +		pr_cont("<not found>\n");
>>>> +	else
>>>> +		pr_cont("0x%lx\n", *address);
>>>
>>> pr_cont() doesn't work well in barebox and should be avoided.
>>
>> I wasn't aware of that. In one of the earlier versions of the patch it
>> was suggested to print that info. I'll find another solution or remove it.
>>
>>>
>>> Also I think this function shouldn't print anything, the caller should
>>> if it wishes to.
>>
>> I had the impression that most of the functions print the information
>> themselves. For example fit_open_image prints a lot of information about
>> the image. fit_find_compatible_unit (which is used in
>> fit_open_configuration) prints that it found a matching unit.
>>
>> It is a bit unclear when it would be OK for a function to print anything
>> and when not.
> 
> Indeed it is unclear :)
> 
> Generally it's nice when a function prints some information, but here I
> had the feeling that this function might get called in places where we
> don't want to print anything. It doesn't matter much at the moment since
> this function is called in this single place only anyway.
> 
>> But I can move the print to bootm_boot where the function
>> is called. Or would you prefer that it is removed completely? I'm not
>> sure whether bootm_boot prints that information later?
> 
> bootm will print later where it puts the kernel, but not where it got
> the address from, so I think printing it here is valuable.
> 
> I just noticed that with your patch bootm refuses to boot FIT images
> that don't have load and entry address explicitly given, right? That
> shouldn't be the case.

Also according to the spec for a FIT image the load and entry are
mandatory, my intention was to implement it backward compatible. The
values should be only parsed if no one set them earlier. And if nothing
is found, the default value of UIMAGE_INVALID_ADDRESS (for address) and
0 (for entry) should be used. But I'll check and test that before I send
a v6.

Best regards

Christian

> 
> Sascha
> 

-- 
--------------------------------------------
embedded brains GmbH
Herr Christian Mauderer
Dornierstr. 4
D-82178 Puchheim
Germany
email: christian.mauderer@embedded-brains.de
Phone: +49-89-18 94 741 - 18
Fax:   +49-89-18 94 741 - 08
PGP: Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v5] FIT: Parse `load` and `entry` addresses.

[Sascha Hauer]

On Wed, Aug 12, 2020 at 01:01:31PM +0200, Christian Mauderer wrote:
> Hello Sascha,
> 
> On 12/08/2020 12:08, Sascha Hauer wrote:
> > On Wed, Aug 12, 2020 at 08:47:47AM +0200, Christian Mauderer wrote:
> >> Hello Sascha,
> >>
> >> thanks for the review.
> >>
> >> On 11/08/2020 09:57, Sascha Hauer wrote:
> >>> Hi Christian,
> >>>
> >>> On Wed, Jul 15, 2020 at 11:26:56AM +0200, Christian Mauderer wrote:
> >>>> According to the U-Boot documentation for the FIT file format, the load
> >>>> and entry have to be allways defined for a "kernel" or "standalone".
> >>>> But Barebox ignored the parameters. That changes with this patch.
> >>>>
> >>>> For backward compatibility the default address is still used for images
> >>>> without `load` or `entry`.
> >>>>
> >>>> Signed-off-by: Christian Mauderer <christian.mauderer@embedded-brains.de>
> >>>> ---
> >>>>  common/blspec.c     |  1 +
> >>>>  common/boot.c       |  1 +
> >>>>  common/bootm.c      | 24 ++++++++++-
> >>>>  common/image-fit.c  | 97 ++++++++++++++++++++++++++++++++++++++-------
> >>>>  include/image-fit.h |  3 ++
> >>>>  5 files changed, 110 insertions(+), 16 deletions(-)
> >>>>
> >>>> diff --git a/common/blspec.c b/common/blspec.c
> >>>> index 7fb62d310..050aed26a 100644
> >>>> --- a/common/blspec.c
> >>>> +++ b/common/blspec.c
> >>>> @@ -142,6 +142,7 @@ static int blspec_boot(struct bootentry *be, int verbose, int dryrun)
> >>>>  	globalvar_set_match("bootm.initrd", "");
> >>>>  
> >>>>  	bootm_data_init_defaults(&data);
> >>>> +	data.os_entry = 0;
> >>>
> >>> You set data.os_entry explicitly to 0 here...
> >>>
> >>>>  
> >>>>  	devicetree = blspec_entry_var_get(entry, "devicetree");
> >>>>  	initrd = blspec_entry_var_get(entry, "initrd");
> >>>> diff --git a/common/boot.c b/common/boot.c
> >>>> index dcbe5cc2e..93ac1612d 100644
> >>>> --- a/common/boot.c
> >>>> +++ b/common/boot.c
> >>>> @@ -104,6 +104,7 @@ static int bootscript_boot(struct bootentry *entry, int verbose, int dryrun)
> >>>>  
> >>>>  	bootm_data_init_defaults(&data);
> >>>>  
> >>>> +	data.os_entry = 0;
> >>>
> >>> ...and here. Why is this done? I think these should be left to the
> >>> default UIMAGE_SOME_ADDRESS. In the end the kernels bootet from blspec
> >>> or a boot script could be a FIT image as well.
> >>>
> >>
> >> You maybe noted that I added the default of UIMAGE_SOME_ADDRESS to
> >> bootm_data_init_defaults. I think that it is a sensible default and it
> >> was useful for adding the command.
> > 
> > Yes.
> > 
> >>
> >> Before I did that, in these two cases the value for os_entry was
> >> initialized with 0. With setting it explicitly to 0 I wanted to make
> >> sure that the behavior doesn't change.
> >>
> >> But you are right: I added a check for that in bootm_boot later. I just
> >> checked again: There is no case where the os_entry is used in between.
> >> So these two should be not unnecessary.
> >>
> >> I'll remove it in a v6 of the patch.
> > 
> > Ok, thanks
> > 
> >>
> >>>> +int fit_get_image_address(struct fit_handle *handle, void *configuration,
> >>>> +			  const char *name, const char *property,
> >>>> +			  unsigned long *address)
> >>>> +{
> >>>> +	struct device_node *image;
> >>>> +	const char *unit = name;
> >>>> +	int ret;
> >>>> +
> >>>> +	if (!address || !property || !name)
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	ret = fit_get_image(handle, configuration, &unit, &image);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	pr_info("%s/%s: ", image->full_name, property);
> >>>> +
> >>>> +	ret = fit_get_address(image, property, address);
> >>>> +	if (ret < 0)
> >>>> +		pr_cont("<not found>\n");
> >>>> +	else
> >>>> +		pr_cont("0x%lx\n", *address);
> >>>
> >>> pr_cont() doesn't work well in barebox and should be avoided.
> >>
> >> I wasn't aware of that. In one of the earlier versions of the patch it
> >> was suggested to print that info. I'll find another solution or remove it.
> >>
> >>>
> >>> Also I think this function shouldn't print anything, the caller should
> >>> if it wishes to.
> >>
> >> I had the impression that most of the functions print the information
> >> themselves. For example fit_open_image prints a lot of information about
> >> the image. fit_find_compatible_unit (which is used in
> >> fit_open_configuration) prints that it found a matching unit.
> >>
> >> It is a bit unclear when it would be OK for a function to print anything
> >> and when not.
> > 
> > Indeed it is unclear :)
> > 
> > Generally it's nice when a function prints some information, but here I
> > had the feeling that this function might get called in places where we
> > don't want to print anything. It doesn't matter much at the moment since
> > this function is called in this single place only anyway.
> > 
> >> But I can move the print to bootm_boot where the function
> >> is called. Or would you prefer that it is removed completely? I'm not
> >> sure whether bootm_boot prints that information later?
> > 
> > bootm will print later where it puts the kernel, but not where it got
> > the address from, so I think printing it here is valuable.
> > 
> > I just noticed that with your patch bootm refuses to boot FIT images
> > that don't have load and entry address explicitly given, right? That
> > shouldn't be the case.
> 
> Also according to the spec for a FIT image the load and entry are
> mandatory

barebox deliberately ignores this. Background is that a ARM zImage
compiled with CONFIG_AUTO_ZRELADDR enabled can be placed anywhere in the
first 128MiB from start of memory. This means we can build a kernel
image that runs on various architectures which have their memory at
different locations. Forcing the entry to a single address limits the
kernel image to machines which have a common memory start address.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox