> Date: Thu, 11 Mar 2021 15:05:11 +0900 (JST)
> From: YASUOKA Masahiko <yasu...@openbsd.org>
>
> On Wed, 10 Mar 2021 13:15:58 +0100 (CET)
> Mark Kettenis <mark.kette...@xs4all.nl> wrote:
> >> On Wed, 10 Mar 2021 20:35:41 +0900 (JST)
> >> YASUOKA Masahiko <yasu...@openbsd.org> wrote:
> >> > efiboot cannot load the kernel properly on some machines if booted
> >> > from CD-ROM. In that case boot fails with a message like follow:
> >> >
> >> > booting cd0a:..... [359648read symbols: Unknown error: code 255
> >> >
> >> > As far as Asou and my test, this happens on hosts on VMware ESXi 6.7,
> >> > 7.0 and asou's physical machine.
> >> >
> >> > The problem happens because efiboot calls ReadBlocks function with an
> >> > unaligned pointer for medias which requires an aligned pointer. When
> >> > efiboot loads a kernel, the pointer becomes unaligned since there is
> >> > an ELF section located at unaligned place in CD-ROM. Previously our
> >> > kernel didn't have such a section but it does after switching lld as
> >> > the default linker.
> >> >
> >> > For test, let me show sample commands which creates a bootable cdrom
> >> > image for EFI:
> >> >
> >> > mkdir -p efiboot/EFI/BOOT
> >> > cp /usr/mdec/BOOTX64.EFI efiboot/EFI/BOOT
> >> > makefs -M 1m -m 1m -t msdos -o fat_type=12,sectors_per_cluster=1
> >> > \
> >> > efiboot.img efiboot
> >> > mkdir -p cd-dir/etc
> >> > cp bsd.rd cd-dir/
> >> > echo "set image bsd.rd" > cd-dir/etc/boot.conf
> >> > makefs -t cd9660 -o
> >> > 'rockridge,bootimage=i386;/usr/mdec/cdbr,no-emul-boot,allow-multidot,bootimage=efi;efiboot.img,no-emul-boot'
> >> > \
> >> > boot.iso cd-dir
> >> >
> >> > the diff is to fix the problem.
> >> >
> >> > ok?
> >
> > Maybe it is better to always bounce through an aligned buffer? That
> > would make the code a little bit slower but a lot simpler. And the
> > overhead of doing the copy should be small compared to the actual I/O.
>
> Indeed. It became much simpler. As I tested on ESXi 7.0, vaio, and
> qemu, I don't feel significant performance regression.
>
> ok?
Yes, that is much better!
See below for a suggestion on how to improve the comment.
ok kettenis@
>
> Index: sys/arch/amd64/stand/efiboot/efidev.c
> ===================================================================
> RCS file: /var/cvs/openbsd/src/sys/arch/amd64/stand/efiboot/efidev.c,v
> retrieving revision 1.32
> diff -u -p -r1.32 efidev.c
> --- sys/arch/amd64/stand/efiboot/efidev.c 9 Dec 2020 18:10:18 -0000
> 1.32
> +++ sys/arch/amd64/stand/efiboot/efidev.c 11 Mar 2021 05:59:41 -0000
> @@ -84,10 +84,10 @@ efid_init(struct diskinfo *dip, void *ha
> static EFI_STATUS
> efid_io(int rw, efi_diskinfo_t ed, u_int off, int nsect, void *buf)
> {
> - u_int blks, lba, i_lblks, i_tblks, i_nblks;
> + u_int blks, start, end;
> EFI_STATUS status = EFI_SUCCESS;
> - static u_char *iblk = NULL;
> - static u_int iblksz = 0;
> + static u_char *ibuf = NULL;
> + static u_int ibufsz = 0;
>
> /* block count of the intrisic block size in DEV_BSIZE */
> blks = EFI_BLKSPERSEC(ed);
> @@ -95,90 +95,46 @@ efid_io(int rw, efi_diskinfo_t ed, u_int
> /* block size < 512. HP Stream 13 actually has such a disk. */
> return (EFI_UNSUPPORTED);
>
> - /* leading and trailing unaligned blocks in intrisic block */
> - i_lblks = ((off % blks) == 0)? 0 : blks - (off % blks);
> - i_tblks = (nsect > i_lblks)? (off + nsect) % blks : 0;
> -
> - /* aligned blocks in intrisic block */
> - i_nblks = (nsect > i_lblks + i_tblks)? nsect - (i_lblks + i_tblks) : 0;
> -
> - lba = (off + i_lblks) / blks;
> -
> - /* allocate the space for reading unaligned blocks */
> - if (ed->blkio->Media->BlockSize != DEV_BSIZE) {
> - if (iblk && iblksz < ed->blkio->Media->BlockSize) {
> - free(iblk, iblksz);
> - iblk = NULL;
> - }
> - if (iblk == NULL) {
> - iblk = alloc(ed->blkio->Media->BlockSize);
> - iblksz = ed->blkio->Media->BlockSize;
> - }
> + start = off / blks;
> + end = (off + nsect + blks - 1) / blks;
> + /*
> + * Prepare a buffer to use an aligned memory always that might be
> + * required by some medias
> + */
/* always use an aligned buffer as some media require this */
> + if (ibuf && ibufsz < (end - start) * ed->blkio->Media->BlockSize) {
> + free(ibuf, ibufsz);
> + ibuf = NULL;
> + }
> + if (ibuf == NULL) {
> + ibufsz = (end - start) * ed->blkio->Media->BlockSize;
> + ibuf = alloc(ibufsz);
> }
> +
> switch (rw) {
> case F_READ:
> - if (i_lblks > 0) {
> - status = EFI_CALL(ed->blkio->ReadBlocks,
> - ed->blkio, ed->mediaid, lba - 1,
> - ed->blkio->Media->BlockSize, iblk);
> - if (EFI_ERROR(status))
> - goto on_eio;
> - memcpy(buf, iblk + (blks - i_lblks) * DEV_BSIZE,
> - min(nsect, i_lblks) * DEV_BSIZE);
> - }
> - if (i_nblks > 0) {
> - status = EFI_CALL(ed->blkio->ReadBlocks,
> - ed->blkio, ed->mediaid, lba,
> - ed->blkio->Media->BlockSize * (i_nblks / blks),
> - buf + (i_lblks * DEV_BSIZE));
> - if (EFI_ERROR(status))
> - goto on_eio;
> - }
> - if (i_tblks > 0) {
> - status = EFI_CALL(ed->blkio->ReadBlocks,
> - ed->blkio, ed->mediaid, lba + (i_nblks / blks),
> - ed->blkio->Media->BlockSize, iblk);
> - if (EFI_ERROR(status))
> - goto on_eio;
> - memcpy(buf + (i_lblks + i_nblks) * DEV_BSIZE, iblk,
> - i_tblks * DEV_BSIZE);
> - }
> + status = EFI_CALL(ed->blkio->ReadBlocks,
> + ed->blkio, ed->mediaid, start,
> + (end - start) * ed->blkio->Media->BlockSize, ibuf);
> + if (EFI_ERROR(status))
> + goto on_eio;
> + memcpy(buf, ibuf + DEV_BSIZE * (off - start * blks),
> + DEV_BSIZE * nsect);
> break;
> case F_WRITE:
> - if (ed->blkio->Media->ReadOnly)
> - goto on_eio;
> - if (i_lblks > 0) {
> - status = EFI_CALL(ed->blkio->ReadBlocks,
> - ed->blkio, ed->mediaid, lba - 1,
> - ed->blkio->Media->BlockSize, iblk);
> - if (EFI_ERROR(status))
> - goto on_eio;
> - memcpy(iblk + (blks - i_lblks) * DEV_BSIZE, buf,
> - min(nsect, i_lblks) * DEV_BSIZE);
> - status = EFI_CALL(ed->blkio->WriteBlocks,
> - ed->blkio, ed->mediaid, lba - 1,
> - ed->blkio->Media->BlockSize, iblk);
> - }
> - if (i_nblks > 0) {
> - status = EFI_CALL(ed->blkio->WriteBlocks,
> - ed->blkio, ed->mediaid, lba,
> - ed->blkio->Media->BlockSize * (i_nblks / blks),
> - buf + (i_lblks * DEV_BSIZE));
> - if (EFI_ERROR(status))
> - goto on_eio;
> - }
> - if (i_tblks > 0) {
> + if (off % blks != 0 || nsect % blks != 0) {
> status = EFI_CALL(ed->blkio->ReadBlocks,
> - ed->blkio, ed->mediaid, lba + (i_nblks / blks),
> - ed->blkio->Media->BlockSize, iblk);
> + ed->blkio, ed->mediaid, start,
> + (end - start) * ed->blkio->Media->BlockSize, ibuf);
> if (EFI_ERROR(status))
> goto on_eio;
> - memcpy(iblk, buf + (i_lblks + i_nblks) * DEV_BSIZE,
> - i_tblks * DEV_BSIZE);
> - status = EFI_CALL(ed->blkio->WriteBlocks,
> - ed->blkio, ed->mediaid, lba + (i_nblks / blks),
> - ed->blkio->Media->BlockSize, iblk);
> }
> + memcpy(ibuf + DEV_BSIZE * (off - start * blks), buf,
> + DEV_BSIZE * nsect);
> + status = EFI_CALL(ed->blkio->WriteBlocks,
> + ed->blkio, ed->mediaid, start,
> + (end - start) * ed->blkio->Media->BlockSize, ibuf);
> + if (EFI_ERROR(status))
> + goto on_eio;
> break;
> }
> return (EFI_SUCCESS);
>
