Re: [PATCH v4 3/4] tools/bootconfig: Align the bootconfig applied initrd image size to 4

[Masami Hiramatsu <mhiramat@kernel.org>]

On Wed, 18 Nov 2020 11:22:49 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 19 Nov 2020 00:35:44 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > +
> > +	/* To align up the total size to BOOTCONFIG_ALIGN, get padding size */
> > +	total_size = stat.st_size + size + sizeof(u32) * 2 + BOOTCONFIG_MAGIC_LEN;
> > +	pad = BOOTCONFIG_ALIGN - total_size % BOOTCONFIG_ALIGN;
> > +	if (pad == BOOTCONFIG_ALIGN)
> > +		pad = 0;
> 
> If alignment is always a power of two, you could simply do:
> 
> 	pad = (total_size + BOOTCONFIG_ALIGN - 1) &  ~(BOOTCONFIG_ALIGN - 1)) - total_size;
> 
> Which will give you the proper padding, without the if.

Thanks, and in that case, I would like to introduce BOOTCONFIG_ALINE_BITS and
BOOTCONFIG_ALINE_MASK so that we make sure the alignment is power of two.

> > +	size += pad;
> > +
> > +	/* Add a footer */
> > +	*(u32 *)(data + size) = size;
> > +	*(u32 *)(data + size + sizeof(u32)) = csum;
> > +	memcpy(data + size + sizeof(u32) * 2, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
> > +	total_size = size + sizeof(u32) * 2 + BOOTCONFIG_MAGIC_LEN;
> 
> I wonder if it would be cleaner to just have a void pointer index for the above:
> 
> 	void *p;
> 
> 	p = data + size;
> 	*(u32 *)p = size;
> 	p += sizeof(u32);
> 
> 	*(u32 *)p = csum;
> 	p += sizeof(u32);
> 
> 	memcpy(p, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
> 	p += BOOTCONFIG_MAGIC_LEN;
> 
> 	total_size = p - (void *)data;

OK.

> Also, how does this work if we run this on a little endian box for a big
> endian crossbuild?

Hmm, good point. I expected that the bootconfig command will be used
natively. Hmm, there are 2 options.

- Add endian option to the bootconfig (-le/-be) for cross build.
- Update the footer format to use "ascii" size and checksum.

To generalize the format, latter one is better. But it also involves
the kernel side update too.

Thank you,

> 
> -- Steve
> 
> 
> > +
> > +	ret = write(fd, data, total_size);
> > +	if (ret < total_size) {
> >  		if (ret < 0)
> >  			ret = -errno;
> >  		pr_err("Failed to apply a boot config: %d\n", ret);
> > -		if (ret < 0)
> > -			goto out;
> > -		goto out_rollback;
> > -	}
> > -	/* Write a magic word of the bootconfig */
> > -	ret = write(fd, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
> > -	if (ret < BOOTCONFIG_MAGIC_LEN) {
> > -		if (ret < 0)
> > -			ret = -errno;
> > -		pr_err("Failed to apply a boot config magic: %d\n", ret);
> > -		goto out_rollback;
> > -	}
> > -	ret = 0;
> > +		if (ret > 0)
> > +			goto out_rollback;
> > +	} else
> > +		ret = 0;
> > +
> >  out:
> >  	close(fd);
> >  	free(data);


-- 
Masami Hiramatsu <mhiramat@kernel.org>