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

[Warner Losh]

On Sat, Aug 4, 2018, 11:58 AM Toomas Soome <tsoome at me.com> wrote:

>
>
> > 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.
>

I think the phab looks good, but I am only on my phone so can't say so
there...

Warner

>