Bug 233097 - U-Boot/ubldr loaderdev broken
Summary: U-Boot/ubldr loaderdev broken
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: arm (show other bugs)
Version: 12.0-STABLE
Hardware: arm Any
: --- Affects Some People
Assignee: Ian Lepore
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-11-09 21:10 UTC by Pawel Worach
Modified: 2019-04-22 15:35 UTC (History)
2 users (show)

See Also:


Attachments
add attribute(packed) (376 bytes, patch)
2019-02-16 19:48 UTC, Manuel Stühn
no flags Details | Diff
add attribute(packed) (360 bytes, patch)
2019-02-16 19:49 UTC, Manuel Stühn
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pawel Worach 2018-11-09 21:10:02 UTC
Board: bcm283x armv6
U-Boot: 2018.09
FreeBSD: stable/12 r340282

Setting loaderdev in U-Boot is supposed to set the correct currdev for ubldr, this seems to be broken.

Setting loaderdev to "mmc 0:3.0" seems to cause ubldr to use unit=0, slice=0 and partition=3 instead of slice=3 and partition=0.

This is caused by stor_open() in stand/uboot/lib/disk.c which loads a vararg uboot_devdesc struct as a disk_devdesc struct.

My workaround was to use the disk_devdesc struct in stand/uboot/common/main.c instead of the uboot_devdesc struct.

Post-workaround:
U-Boot> setenv loaderdev mmc 0:3.0
U-Boot> boot
...
U-Boot env: loaderdev='mmc 0:3.0'
Found U-Boot device: disk
  Checking unit=0 slice=3 partition=0... good.
Booting from disk0s3a:
Comment 1 Manuel Stühn 2019-02-16 19:48:58 UTC
Created attachment 202071 [details]
add attribute(packed)
Comment 2 Manuel Stühn 2019-02-16 19:49:42 UTC
Created attachment 202072 [details]
add attribute(packed)
Comment 3 Manuel Stühn 2019-02-16 19:49:54 UTC
The structs disk_devdesc (stand/common/disk.h) and uboot_devdesc (stand/uboot/lib/libuboot.h) should be compiled with __attribute__((packed)). Otherwise they get padded and break loaderdev-variable is completely.
Comment 4 commit-hook freebsd_committer freebsd_triage 2019-02-18 04:45:44 UTC
A commit references this bug:

Author: ian
Date: Mon Feb 18 04:44:53 UTC 2019
New revision: 344247
URL: https://svnweb.freebsd.org/changeset/base/344247

Log:
  Make uboot_devdesc properly alias disk_devdesc, so that parsing the u-boot
  loaderdev variable works correctly.

  The uboot_devdesc struct is variously cast back and forth between
  uboot_devdesc and disk_devdesc as pointers are handed off through various
  opaque interfaces.  uboot_devdesc attempted to mimic the layout of
  disk_devdesc by having a devdesc struct, followed by a union of some
  device-specific stuff that included a struct that contains the same fields
  as a disk_devdesc.  However, one of those fields inside the struct is 64-bit
  which causes the entire union to be 64-bit aligned -- 32 bits of padding
  is added between the struct devdesc and the union, so the whole mess ends
  up NOT properly mimicking a disk_devdesc after all.  (In disk_devdesc there
  is also 32 bits of padding, but it shows up immediately before the d_offset
  field, rather than before the whole collection of d_* fields.)

  This fixes the problem by using an anonymous union to overlay the devdesc
  field uboot network devices need with the disk_devdesc that uboot storage
  devices need.  This is a different solution than the one contributed with
  the PR (so if anything goes wrong, the blame goes to me), but 95% of the
  credit for this fix goes to Pawel Worach and Manuel Stuhn who analyzed the
  problem and proposed a fix.

  PR:		233097

Changes:
  head/stand/uboot/common/main.c
  head/stand/uboot/lib/libuboot.h
Comment 5 commit-hook freebsd_committer freebsd_triage 2019-04-21 04:36:53 UTC
A commit references this bug:

Author: kevans
Date: Sun Apr 21 04:35:52 UTC 2019
New revision: 346483
URL: https://svnweb.freebsd.org/changeset/base/346483

Log:
  MFC r343911, r344238-r344241, r344247, r344254-r344255, r344260, r344268,
  r344335, r344839, r345066, r345330

  r343911:
  Allow reading the UEFI variable size

  When loading bigger variables form UEFI it is necessary to know their
  size beforehand, so that an appropriate amount of memory can be
  allocated. The easiest way to do this is to try to read the variable
  with buffer size equal 0, expecting EFI_BUFFER_TOO_SMALL error to be
  returned. Allow such possible approach in efi_getenv routine.

  Extracted from a bigger patch as suggested by imp.

  r344238:
  Restore loader(8)'s ability for lsdev to show partitions within a bsd slice.

  I'm pretty sure this used to work at one time, perhaps long ago.  It has
  been failing recently because if you call disk_open() with dev->d_partition
  set to -1 when d_slice refers to a bsd slice, it assumes you want it to
  open the first partition within that slice.  When you then pass that open
  dev instance to ptable_open(), it tries to read the start of the 'a'
  partition and decides there is no recognizable partition type there.

  This restores the old functionality by resetting d_offset to the start
  of the raw slice after disk_open() returns.  For good measure, d_partition
  is also set back to -1, although that doesn't currently affect anything.

  I would have preferred to make disk_open() avoid such rude assumptions and
  if you ask for partition -1 you get the raw slice.  But the commit history
  shows that someone already did that once (r239058), and had to revert it
  (r239232), so I didn't even try to go down that road.

  r344239:
  Use a couple local variables to avoid repetitive long expressions that
  cause line-wrapping.

  r344240:
  Make lsdev -v output line up in neat columns by using a fixed width for
  the size field and a tab between the partition type and the size.

  Changes this

    disk devices:
          disk0 (MMC)
          disk0s1: DOS/Windows            49MB
          disk0s2: FreeBSD                14GB
          disk0s2a: FreeBSD UFS         14GB
          disk0s2b: Unknown             2048KB
          disk0s2d: FreeBSD UFS         2040KB

  to this

    disk devices:
          disk0 (MMC)
          disk0s1: DOS/Windows      49MB
          disk0s2: FreeBSD          14GB
          disk0s2a: FreeBSD UFS     14GB
          disk0s2b: Unknown       2048KB
          disk0s2d: FreeBSD UFS   2040KB

  r344241:
  Garbage collection no-longer-used constant.

  r344247:
  Make uboot_devdesc properly alias disk_devdesc, so that parsing the u-boot
  loaderdev variable works correctly.

  The uboot_devdesc struct is variously cast back and forth between
  uboot_devdesc and disk_devdesc as pointers are handed off through various
  opaque interfaces.  uboot_devdesc attempted to mimic the layout of
  disk_devdesc by having a devdesc struct, followed by a union of some
  device-specific stuff that included a struct that contains the same fields
  as a disk_devdesc.  However, one of those fields inside the struct is 64-bit
  which causes the entire union to be 64-bit aligned -- 32 bits of padding
  is added between the struct devdesc and the union, so the whole mess ends
  up NOT properly mimicking a disk_devdesc after all.  (In disk_devdesc there
  is also 32 bits of padding, but it shows up immediately before the d_offset
  field, rather than before the whole collection of d_* fields.)

  This fixes the problem by using an anonymous union to overlay the devdesc
  field uboot network devices need with the disk_devdesc that uboot storage
  devices need.  This is a different solution than the one contributed with
  the PR (so if anything goes wrong, the blame goes to me), but 95% of the
  credit for this fix goes to Pawel Worach and Manuel Stuhn who analyzed the
  problem and proposed a fix.

  r344254:
  Use DEV_TYP_NONE instead of -1 to indicate no device was specified.

  DEV_TYP_NONE has a value of zero, which makes more sense since the device
  type is a bunch of bits describing the device, crammed into an int.

  r344255:
  Fix more places to use DEV_TYP_NONE instead of -1 to indicate 'no device'.

  r344260:
  Allow the u-boot loaderdev env var to be formatted in the "usual" loader(8)
  way: device<unit>[s|p]<slice><partition>.  E.g., disk0s2a or disk3p12.
  The code first tries to parse the variable in this format using the
  standard disk_parsedev().  If that fails, it falls back to parsing the
  legacy format that has been supported by ubldr for years.

  In addition to 'disk', all the valid uboot device names can also be used:
  mmc, sata, usb, ide, scsi. The 'disk' device serves as an alias for all
  those types and will match the Nth storage-type device found (where N is
  the unit number).

  r344268:
  loader: ptable_close() should check its argument

  If the passed in table is NULL, just return.

  r344335:
  Fix the handling of legacy-format devices in the u-boot loaderdev variable.
  When I added support for the standard loader(8) disk0s2a: type formats,
  the parsing of legacy format was broken because it also contains a colon,
  but it comes before the slice and partition. That would cause disk_parsedev()
  to return success with the slice and partition set to wildcard values.

  This change examines the string first, and if it contains spaces, dots, or
  a colon at any position other than the end, it must be a legacy-format
  string and we don't even try to use disk_parsedev() on it.

  r344839:
  Add retry loop around GetMemoryMap call to fix fragmentation bug

  The call to BS->AllocatePages can cause the memory map to become framented,
  causing BS->GetMemoryMap to return EFI_BUFFER_TOO_SMALL more than once. For
  example this can happen on the MinnowBoard Turbot, causing the boot to stop
  with an error. Avoid this by calling GetMemoryMap in a loop.

  r345066:
  stand: Improve some debugging experience

  Some of these files using <FOO>_DEBUG defined a DEBUG() macro to serve as a
  debug-printf. -DDEBUG is useful to enable some debugging output across
  multiple ELF/common parts, so switch the DEBUG-as-printf macros over to
  something more like DPRINTF that is more commonly used for this kind of
  thing and less likely to conflict.

  userboot/elf64_freebsd debugging also assumed %llx for uint64; use PRIx64
  instead.

  r345330:
  loader: fix loading of kernels with . in path

  The loader indended to search the kernel file name (only) for . but
  instead searched the entire path, so paths like
  "boot/test.elfv2/kernel" would not work.

  PR:		233097

Changes:
_U  stable/11/
  stable/11/stand/common/bcache.c
  stable/11/stand/common/disk.c
  stable/11/stand/common/interp_forth.c
  stable/11/stand/common/load_elf.c
  stable/11/stand/common/part.c
  stable/11/stand/efi/libefi/efienv.c
  stable/11/stand/efi/loader/bootinfo.c
  stable/11/stand/efi/loader/copy.c
  stable/11/stand/uboot/common/main.c
  stable/11/stand/uboot/lib/libuboot.h
  stable/11/stand/userboot/userboot/elf64_freebsd.c
Comment 6 commit-hook freebsd_committer freebsd_triage 2019-04-21 20:41:33 UTC
A commit references this bug:

Author: ian
Date: Sun Apr 21 20:40:50 UTC 2019
New revision: 346496
URL: https://svnweb.freebsd.org/changeset/base/346496

Log:
  MFC r344247:

  Make uboot_devdesc properly alias disk_devdesc, so that parsing the u-boot
  loaderdev variable works correctly.

  The uboot_devdesc struct is variously cast back and forth between
  uboot_devdesc and disk_devdesc as pointers are handed off through various
  opaque interfaces.  uboot_devdesc attempted to mimic the layout of
  disk_devdesc by having a devdesc struct, followed by a union of some
  device-specific stuff that included a struct that contains the same fields
  as a disk_devdesc.  However, one of those fields inside the struct is 64-bit
  which causes the entire union to be 64-bit aligned -- 32 bits of padding
  is added between the struct devdesc and the union, so the whole mess ends
  up NOT properly mimicking a disk_devdesc after all.  (In disk_devdesc there
  is also 32 bits of padding, but it shows up immediately before the d_offset
  field, rather than before the whole collection of d_* fields.)

  This fixes the problem by using an anonymous union to overlay the devdesc
  field uboot network devices need with the disk_devdesc that uboot storage
  devices need.  This is a different solution than the one contributed with
  the PR (so if anything goes wrong, the blame goes to me), but 95% of the
  credit for this fix goes to Pawel Worach and Manuel Stuhn who analyzed the
  problem and proposed a fix.

  PR:		233097

Changes:
_U  stable/12/
  stable/12/stand/uboot/common/main.c
  stable/12/stand/uboot/lib/libuboot.h