svn commit: r337271 - head/stand/i386/libi386

[Toomas Soome]

> On 4 Aug 2018, at 11:54, Xin Li <delphij at delphij.net> wrote:
> 
> Hi, Cy,
> 
> On 8/3/18 12:11, Cy Schubert wrote:
>> Author: cy
>> Date: Fri Aug  3 19:11:00 2018
>> New Revision: 337271
>> URL: https://svnweb.freebsd.org/changeset/base/337271
>> 
>> Log:
>>  Some drives report a geometry that is inconsisetent with the total
>>  number of sectors reported through the BIOS. Cylinders * heads *
>>  sectors may not necessarily be equal to the total number of sectors
>>  reported through int13h function 48h.
>> 
>>  An example of this is when a Mediasonic HD3-U2B PATA to USB enclosure
>>  with a 80 GB disk is attached. Loader hangs at line 506 of
>>  stand/i386/libi386/biosdisk.c while attempting to read sectors beyond
>>  the end of the disk, sector 156906855. I discovered that the Mediasonic
>>  enclosure was reporting the disk with 9767 cylinders, 255 heads, 63
>>  sectors/track. That's 156906855 sectors. However camcontrol and
>>  Windows 10 both report report the disk having 156301488 sectors, not
>>  the calculated value. At line 280 biosdisk.c sets the sectors to the
>>  higher of either bd->bd_sectors or the total calculated at line 276
>>  (156906855) instead of the lower and correct value of 156301488 reported
>>  by int 13h 48h.
>> 
>>  This was tested on all three of my Mediasonic HD3-U2B PATA to USB
>>  enclosures.
>> 
>>  Instead of using the higher of bd_sectors (returned by int13h) or the
>>  calculated value, this patch uses the lower and safer of the values.
>> 
>>  Reviewed by:	tsoome@
>>  Differential Revision:	https://reviews.freebsd.org/D16577
>> 
>> Modified:
>>  head/stand/i386/libi386/biosdisk.c
>> 
>> Modified: head/stand/i386/libi386/biosdisk.c
>> ==============================================================================
>> --- head/stand/i386/libi386/biosdisk.c	Fri Aug  3 18:52:51 2018	(r337270)
>> +++ head/stand/i386/libi386/biosdisk.c	Fri Aug  3 19:11:00 2018	(r337271)
>> @@ -275,7 +275,7 @@ bd_int13probe(struct bdinfo *bd)
>> 
>> 		total = (uint64_t)params.cylinders *
>> 		    params.heads * params.sectors_per_track;
>> -		if (bd->bd_sectors < total)
>> +		if (bd->bd_sectors > total)
>> 			bd->bd_sectors = total;
>> 
>> 		ret = 1;
>> 
> 
> This broke loader on my system, but I think your reasoning was valid so
> I took a deeper look and discovered that on my system, INT 13h, function
> 48h would give zeros in EDD parameters' CHS fields.  With that, the
> calculated CHS based total would be 0, and your change would cause
> bd_sectors be zeroed.
> 
> Could you please let me know if https://reviews.freebsd.org/D16588 makes
> sense to you?  (I'm not 100% certain if I have followed the code).  It
> allowed my Asrock C2750D4I based board to boot from ZFS.
> 

I have in mind something a bit different for some time, but haven’t had chance to complete it because I have no “weird” systems to validate the idea:D I’ll try to get a bit of time and post an phabricator soon.

rgds,
toomas