Page MenuHomeFreeBSD

loader: r316585 did miss sparc/ofw and userboot update
ClosedPublic

Authored by tsoome on Apr 6 2017, 11:12 PM.

Details

Summary

The r316585 did break the sparc build, as the current code does not implement method to detect the disk size, The userboot does query for disk size, but we need to update ioctl method and provide callback for vdev_probe().

The OFW actually does define methods:
block-size - to get the media sector size
#blocks (and/or #blocks64) to get the disk size.

However, even as I wrote the wrappers for the methods, I have no way to test the implementations, also the disk size itself does not help us as the ofw code does not really use common/disk.c API and therefore the those functions are likely useful in future work, but not right now.

The solution to provide partition size for vdev_probe() is about making partition size info available, For now, I am providing the partition size via global variable, as we call zfs_dev_probe() for each (recognized) partition.

For userboot, the fix is quite straight forward, as userboot is already using common/disk.c API.

Test Plan

Need to verify the build and boot on sparc....

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

tsoome created this revision.Apr 6 2017, 11:12 PM
imp added a comment.Apr 6 2017, 11:23 PM

This suggests that getting the size is wrong and using it the way you do is flawed.

The backup GPT is defined to be where the primary GPT says it is, which is not necessarily the last sector(s).

In D10302#213231, @imp wrote:

This suggests that getting the size is wrong and using it the way you do is flawed.

For the pool, yes, we really should get the partition size eventually - assuming we want to get the correct location for the last 2 labels.

The backup GPT is defined to be where the primary GPT says it is, which is not necessarily the last sector(s).

yea, but in this case it is not really even about GPT, as the old sparcs only do support vtoc and GPT support was added for T4 and up. Anyhow, as you do mention GPT, for practical purposes using the GPT backup label location is enough, as it is the last thing *after* the data, and so that will give us good size limit in any case, even if the physical device is larger, it has no data after the backup label.

imp added a comment.Apr 7 2017, 12:16 AM
In D10302#213231, @imp wrote:

This suggests that getting the size is wrong and using it the way you do is flawed.

For the pool, yes, we really should get the partition size eventually - assuming we want to get the correct location for the last 2 labels.

The backup GPT is defined to be where the primary GPT says it is, which is not necessarily the last sector(s).

yea, but in this case it is not really even about GPT, as the old sparcs only do support vtoc and GPT support was added for T4 and up. Anyhow, as you do mention GPT, for practical purposes using the GPT backup label location is enough, as it is the last thing *after* the data, and so that will give us good size limit in any case, even if the physical device is larger, it has no data after the backup label.

True. The reason I mention it is that our mem sticks all have backup labels that aren't at the end of media except for a vanishingly small set of USB sticks...

In D10302#213242, @imp wrote:
In D10302#213231, @imp wrote:

This suggests that getting the size is wrong and using it the way you do is flawed.

For the pool, yes, we really should get the partition size eventually - assuming we want to get the correct location for the last 2 labels.

The backup GPT is defined to be where the primary GPT says it is, which is not necessarily the last sector(s).

yea, but in this case it is not really even about GPT, as the old sparcs only do support vtoc and GPT support was added for T4 and up. Anyhow, as you do mention GPT, for practical purposes using the GPT backup label location is enough, as it is the last thing *after* the data, and so that will give us good size limit in any case, even if the physical device is larger, it has no data after the backup label.

True. The reason I mention it is that our mem sticks all have backup labels that aren't at the end of media except for a vanishingly small set of USB sticks...

Yes, thats exactly the case when this happens - or when you have grown the lun, but not yet adjusted the GPT with new size. Also the GPT "usable" sectors end where the backup label starts:)

tsoome updated this revision to Diff 27166.Apr 7 2017, 12:36 AM

Second take - provide partition size as global variable and hand it back to
the vdev_probe.

tsoome updated this revision to Diff 27176.Apr 7 2017, 12:20 PM

Shame on me, userboot was also missed.

ldi_get_size() added, userdisk_ioctl() updated, userdisk_init callback return
int, not NULL.

tested userboot.so with bhyve.

tsoome retitled this revision from loader: sparc/ofw disk size detection to loader: r316585 did miss sparc/ofw and userboot update.Apr 7 2017, 4:23 PM
tsoome edited the summary of this revision. (Show Details)
tsoome edited the test plan for this revision. (Show Details)
imp accepted this revision.Apr 7 2017, 4:27 PM

looks good to me.

This revision is now accepted and ready to land.Apr 7 2017, 4:27 PM
tsoome updated this revision to Diff 27223.Apr 8 2017, 7:35 AM
tsoome edited edge metadata.

need to include sys/disk.h

This revision now requires review to proceed.Apr 8 2017, 7:35 AM
tsoome updated this revision to Diff 27245.Apr 8 2017, 9:13 PM

i is not the loop variable.

This revision was automatically updated to reflect the committed changes.
lidl added a subscriber: lidl.May 26 2017, 2:59 PM

This change, or one of the others related to it, breaks the operation of zfsloader on sparc64.

I only got to testing a new zfsloader last night, and ones built from the following snapshots
all fail:
r318502
r317181
r318137

I was able to 'dd' in just the zfsloader from a stable/11 build to the reinstall of r318137 that I
did last night, and that allows the machine to reboot.

The failure message is:

FreeBSD/sparc64 ZFS boot block

Boot path:   /pci@1c,600000/scsi@2/disk@0,0:a

Consoles: Open Firmware console
ERROR: Last Trap: Fast Data Access MMU Miss