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

[Roman Kagan]

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 <address@hidden>
> > Cc: Igor Mammedov <address@hidden>
> > Cc: "Michael S. Tsirkin" <address@hidden>
> > Cc: Marcel Apfelbaum <address@hidden>
> > Cc: John Snow <address@hidden>
> > Cc: Laszlo Ersek <address@hidden>
> > Cc: Kevin O'Connor <address@hidden>
> > ---
> > 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.

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