| Message ID | 1455733533-12030-4-git-send-email-rkagan@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
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? I understand what it does rather vaguely. I wonder whether we can just ignore the type and take global maximum in all cases. > + 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? > +} > + > static const VMStateDescription vmstate_isa_fdc ={ > .name = "fdc", > .version_id = 2, > diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h > index adce14f..1749dab 100644 > --- a/include/hw/block/fdc.h > +++ b/include/hw/block/fdc.h > @@ -15,5 +15,7 @@ void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base, > DriveInfo **fds, qemu_irq *fdc_tc); > > FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i); > +void isa_fdc_get_drive_max_chs(FloppyDriveType type, > + uint8_t *maxc, uint8_t *maxh, uint8_t *maxs); > > #endif > -- > 2.5.0
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. > > + 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.
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.
On 02/18/2016 05:01 AM, Michael S. Tsirkin wrote: > 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. > What is right was not always clear :) However, the interpretation that this should be the maximum supported value seems correct, and the function looks mechanically correct. Style shedding aside; Reviewed-by: John Snow <jsnow@redhat.com> >>>> + 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. > This function SHOULD always find something. If it doesn't, it means the fdformat table has no entries for our given FDrive type, which should be an assert during the boot process over in pick_drive_type and children, so we should be safe. --js
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; + } + 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)--; +} + static const VMStateDescription vmstate_isa_fdc ={ .name = "fdc", .version_id = 2, diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h index adce14f..1749dab 100644 --- a/include/hw/block/fdc.h +++ b/include/hw/block/fdc.h @@ -15,5 +15,7 @@ void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base, DriveInfo **fds, qemu_irq *fdc_tc); FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i); +void isa_fdc_get_drive_max_chs(FloppyDriveType type, + uint8_t *maxc, uint8_t *maxh, uint8_t *maxs); #endif
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(+)