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

[Cy Schubert]

In message <CANCZdfpYbZ2vD-iXpDC3J4fPcz3SZPacr8U8Tr+7bYqZj8je9Q at mail.gma
il.com>
, Warner Losh writes:
> --00000000000017c64d05729a1cbf
> Content-Type: text/plain; charset="UTF-8"
> Content-Transfer-Encoding: quoted-printable
>
> 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 Mediason=
> ic
> > >>  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
> > >>
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D
> > >> --- 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 =3D (uint64_t)params.cylinders *
> > >>                  params.heads * params.sectors_per_track;
> > >> -            if (bd->bd_sectors < total)
> > >> +            if (bd->bd_sectors > total)
> > >>                      bd->bd_sectors =3D total;
> > >>
> > >>              ret =3D 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, functio=
> n
> > > 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 make=
> s
> > > 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=E2=80=
> =99t had
> > chance to complete it because I have no =E2=80=9Cweird=E2=80=9D systems t=
> o validate the
> > idea:D I=E2=80=99ll 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

It looks good.

The only case that hasn't been considered so far is if bd_sectors is 
arbitrarily low but not zero and the calculated value is the correct 
one.


-- 
Cheers,
Cy Schubert <Cy.Schubert at cschubert.com>
FreeBSD UNIX:  <cy at FreeBSD.org>   Web:  http://www.FreeBSD.org

	The need of the many outweighs the greed of the few.