Re: [Qemu-devel] [PATCH v8 3/4] fdc: add function to determine drive chs limits

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Feb 18, 2016 at 12:50:51PM +0300, Roman Kagan wrote:
> On Wed, Feb 17, 2016 at 10:15:32PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Feb 17, 2016 at 09:25:32PM +0300, Roman Kagan wrote:
> > > When populating ACPI objects for floppy drives one needs to provide the
> > > maximum values for cylinder, sector, and head number the drive supports.
> > > 
> > > This patch adds a function that iterates through the array of predefined
> > > floppy drive formats and returns the maximum values of c, h, s, out of
> > > those matching the given floppy drive type.
> > > 
> > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> > > Cc: Igor Mammedov <imammedo@redhat.com>
> > > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > Cc: Marcel Apfelbaum <marcel@redhat.com>
> > > Cc: John Snow <jsnow@redhat.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > Cc: Kevin O'Connor <kevin@koconnor.net>
> > > ---
> > > changes since v7:
> > >  - use drive max c,h,s rather than the current diskette geometry
> > > 
> > >  hw/block/fdc.c         | 23 +++++++++++++++++++++++
> > >  include/hw/block/fdc.h |  2 ++
> > >  2 files changed, 25 insertions(+)
> > > 
> > > diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> > > index 9838d21..fc3aef9 100644
> > > --- a/hw/block/fdc.c
> > > +++ b/hw/block/fdc.c
> > > @@ -2557,6 +2557,29 @@ FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
> > >      return isa->state.drives[i].drive;
> > >  }
> > >  
> > > +void isa_fdc_get_drive_max_chs(FloppyDriveType type,
> > > +                               uint8_t *maxc, uint8_t *maxh, uint8_t *maxs)
> > > +{
> > > +    const FDFormat *fdf;
> > > +
> > > +    *maxc = *maxh = *maxs = 0;
> > > +    for (fdf = fd_formats; fdf->drive != FLOPPY_DRIVE_TYPE_NONE; fdf++) {
> > > +        if (fdf->drive != type) {
> > > +            continue;
> > > +        }
> > 
> > Hmm. How does this interact with the fallback/autodetect thing?
> 
> AFAICS the drive type is chosen at realize time once and for good, based
> on the fdtype property, the size of the inserted image (autodetect), and
> the fallback property.
> 
> So basically there's no interaction: this function only determines the
> max c,h,s among all the formats of that type.
> 
> > I wonder whether we can just ignore the type and take
> > global maximum in all cases.
> 
> There's non-zero chance that we can; however it's up to the
> closed-source windows driver and can only be proven by testing relevant
> image sizes across relevant windows versions.  OTOH the resulting code
> wouldn't differ too much from this patch.  The patch also appears closer
> to the definition in the ACPI spec, and I tested it already, so I'd be
> reluctant to invest more effort into it.

Whatever jsnow says is right is fine by me.

> > > +        if (*maxc < fdf->max_track) {
> > > +            *maxc = fdf->max_track;
> > > +        }
> > > +        if (*maxh < fdf->max_head) {
> > > +            *maxh = fdf->max_head;
> > > +        }
> > > +        if (*maxs < fdf->last_sect) {
> > > +            *maxs = fdf->last_sect;
> > > +        }
> > > +    }
> > > +    (*maxc)--;
> > 
> > Why not just *maxc = fdf->max_track - 1 above?
> 
> No particular reason, I just thought it was more readable to not mess
> with - 1 when searching for the maximum.  I don't care either way, can
> rewrite if you want me to.
> 
> Roman.

It's confusing IMHO - also, if there's an error and you find nothing,
you get 0xFF instead of 0x0.

-- 
MST