Support VirtualBox: Ensure that binary size is a multiple of 512 bytes

[phil-opp]

For some reason VirtualBox expects that disk images are a multiple of 512 bytes. We had an option to add the required padding in an old version of bootimage but we haven't ported that option to the new build system yet.

Reported in phil-opp/blog_os#403 (comment)

[toothbrush7777777]

@phil-opp What needs to be changed?

P.S. I think it is reasonable to expect the size of a disk image to be a multiple of the minimum block size.

[phil-opp]

Yes, it's absolutely reasonable, it's just not what objcopy does by default. The relevant code is:

bootimage/src/builder.rs

Lines 283 to 298 in c2ce530

	// convert bootloader to binary
	let mut cmd = Command::new(objcopy);
	cmd.arg("-I").arg("elf64-x86-64");
	cmd.arg("-O").arg("binary");
	cmd.arg("--binary-architecture=i386:x86-64");
	cmd.arg(&bootloader_elf_path);
	cmd.arg(&output_bin_path);
	let output = cmd.output().map_err(|err| CreateBootimageError::Io {
	message: "failed to execute llvm-objcopy command",
	error: err,
	})?;
	if !output.status.success() {
	return Err(CreateBootimageError::ObjcopyFailed {
	stderr: output.stderr,
	});
	}

To fix this we need to either find some objcopy argument that performs the alignment or pad the created binary manually after it's created.

[toothbrush7777777]

There are 2 options for objcopy which seem to do that: --pad-to and --gap-fill

[phil-opp]

Hmm, --gap-fill seems to be about the value that is used to fill alignment gaps (instead of leaving them undefined. The --pad-to argument seems fitting, but it expects an address as argument, which is a bit cumbersome to find out (read the ELF file, get the load address and size of the last section, and round it up).

Given that the result is a binary file, it's probably easier to just open it afterwards and using set_len to pad it to the next 512-byte boundary.

[toothbrush7777777]

@phil-opp Would something like this work? I can't test it now.

        // Pad to nearest block size
        {
            // BLOCK_SIZE must be a power of 2
            const BLOCK_SIZE = 512;
            use std::fs::{OpenOptions, File};
            let mut file = OpenOptions::new().append(true).open(&output_bin_path).map_err(|err| CreateBootimageError::Io {
                message: "failed to open output file",
                error: err,
            })?;
            let file_size = file.metadata().map_err(|err| CreateBootimageError::Io {
                message: "failed to get size of output file",
                error: err,
            })?.len();
            file.set_len((file_size + BLOCK_SIZE - 1) & -BLOCK_SIZE).map_err(|err| CreateBootimageError::Io {
                message: "failed to pad output file to a multiple of the block size",
                error: err,
            })?;
        }

[phil-opp]

Yes, I think so.

One nit: I would prefer a different approach for aligning so that it becomes more apparent that we never make the file length smaller. Maybe something like file_size + padding with let padding: u64 = […] is possible?

[toothbrush7777777]

@phil-opp How about this?

let padding = file_size % BLOCK_SIZE;
let padding = if padding > 0 { BLOCK_SIZE - padding } else { 0 };
file.set_len(file_size + padding).map_err(_)?;

Actually, since BLOCK_SIZE is a power of 2, the first line could be this:

let padding = file_size & -BLOCK_SIZE;

[phil-opp]

Looks good! Do you mind opening a pull request?

Actually, since BLOCK_SIZE is a power of 2, the first line could be this:

I think the compiler should be able to perform this optimization automatically. I like the first variant more because it is more obvious what happens. (Performance shouldn't be relevant at this place anyway since it's clearly IO-bound.

[toothbrush7777777]

@phil-opp I created a pull request.
Also, I agree with you that the compiler (or rather LLVM) should perform this optimization itself.