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 <rka...@virtuozzo.com>
> > > Cc: Igor Mammedov <imamm...@redhat.com>
> > > Cc: "Michael S. Tsirkin" <m...@redhat.com>
> > > Cc: Marcel Apfelbaum <mar...@redhat.com>
> > > Cc: John Snow <js...@redhat.com>
> > > Cc: Laszlo Ersek <ler...@redhat.com>
> > > Cc: Kevin O'Connor <ke...@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
