John Snow <js...@redhat.com> writes:
> On 07/03/2015 09:34 AM, Markus Armbruster wrote:
>> John Snow <js...@redhat.com> writes:
>>
>>> Respect the default drive type as proffered via the CLI.
>>>
>>> This patch overloads the "drive out" parameter of pick_geometry
>>> to be used as a "default hint" which is offered by fd_revalidate.
>>>
>>> Signed-off-by: John Snow <js...@redhat.com>
>>> ---
>>> hw/block/fdc.c | 27 +++++++++++++++++++++++++--
>>> 1 file changed, 25 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>> index 1023a01..87fd770 100644
>>> --- a/hw/block/fdc.c
>>> +++ b/hw/block/fdc.c
>>> @@ -128,7 +128,13 @@ static void pick_geometry(BlockBackend *blk, int
>>> *nb_heads,
>>> /* Pick a default drive type if there's no media inserted AND we have
>>> * not yet announced our drive type to the CMOS. */
>>> if (!blk_is_inserted(blk) && drive_in == FDRIVE_DRV_NONE) {
>>> - parse = &fd_formats[0];
>>> + for (i = 0; ; i++) {
>>> + parse = &fd_formats[i];
>>> + if ((parse->drive == FDRIVE_DRV_NONE) ||
>>> + (parse->drive == *drive)) {
>>> + break;
>>> + }
>>> + }
>>> goto out;
>>> }
>>>
>>
>> Before the patch: if we have no medium, and drv->drive (a.k.a. drive_in)
>> is still FDRIVE_DRV_NONE, pick format #0 (standard 3.5" 1.44MiB).
>>
>
> My commit message is actually wrong, the code actually picks format #1,
Now I'm confused: &fd_formats[0] looks like format #0 to me.
> but that's only useful for choosing the drive type while there's no
> floppy. As soon as one is inserted it will re-validate to the closest
> 1.44MB type.
>
>> Afterwards: pick first one matching the value of
>> get_default_drive_type(), i.e. the value of the new property.
>>
>> So the property is really a default type, which applies only when we
>> start without a medium in the drive.
>>
>> Is that what we want?
>>
>
> It is a "minimal" change that just allows you to configure what it
> defaults back to. It's a 'weak' setting.
>
> Was that a good call? hmm...
>
>> When I specifically ask for a 5.25" 1.2MiB drive, I'd be rather
>> surprised when it silently morphs into a 3.5" 2.88MiB drive just because
>> I've forced a funny medium in before startup.
>>
>
> Ah, here it is. I can just add "typeA" and "typeB" properties instead of
> defaultA/B, and force the drive into that role.
>
>> The obvious way to do drive types is selecting one with a property,
>> defaulting to the most common type, i.e. standard 3.5" 1.44Mib.
>>
>
> Hm?
>
> Not sure I follow, but the goal here is to use the 2.88MB type, because
> it can accept both kinds of diskettes, so it has the greatest
> compatibility for floppy images (including the virtio driver diskette
> which is a 2.88MB type.)
The 2.88 type may well be a more useful default, because it takes a
strict superset of media. On the other hand, it puts a different value
into the CMOS floppy byte, which could conceivably confuse guests that
haven't been updated for this kind of high-tech gadget. I'm not telling
you what default to pick, only what the tradeoffs are.
> The problem is the 1.44MB drive type and the current inflexibility of
> the revalidate+pick_geometry algorithm that doesn't allow you to change
> from 1.44 to 2.88 or vice versa.
Well, you can't change physical drives from 1.44 to 2.88 or vice versa,
either.
>> To preserve backward compatibility, we need a way to say "pick one for
>> the medium, else pick standard, and we need to make it the default. At
>> least for old machine types.
>>
>> Opinions?
>>
>
> Machines prior to (let's say 2.5 here) should use something close to the
> old behavior: "Choose 1.44MB if no diskette, pick the best match (by
> sector count) otherwise."
If you want the new, saner behavior with and old machine type, pick the
appropriate type, e.g. say type=288.
> New machines apply nearly the same behavior, except they opt for 2.88MB
> as the default.
>
> New properties allow users to override the default-selection behavior
> and/or just force a drive type.
Possible alternative for new machines: default to 2.88 type, done.
If you want the old behavior with a new machine type then, you get to
specify type=magic or something.
Matter of UI taste.
>>> @@ -295,11 +301,13 @@ static void fd_recalibrate(FDrive *drv)
>>> fd_seek(drv, 0, 0, 1, 1);
>>> }
>>>
>>> +static FDriveType get_default_drive_type(FDrive *drv);
>>> +
>>> /* Revalidate a disk drive after a disk change */
>>> static void fd_revalidate(FDrive *drv)
>>> {
>>> int nb_heads, max_track, last_sect, ro;
>>> - FDriveType drive;
>>> + FDriveType drive = get_default_drive_type(drv);
>>> FDriveRate rate;
>>>
>>> FLOPPY_DPRINTF("revalidate\n");
>>> @@ -609,6 +617,21 @@ typedef struct FDCtrlISABus {
>>> int32_t bootindexB;
>>> } FDCtrlISABus;
>>>
>>> +static FDriveType get_default_drive_type(FDrive *drv)
>>> +{
>>> + FDCtrl *fdctrl = drv->fdctrl;
>>> +
>>> + if (0 < MAX_FD && (&fdctrl->drives[0] == drv)) {
>>> + return fdctrl->defaultA;
>>> + }
>>> +
>>> + if (1 < MAX_FD && (&fdctrl->drives[1] == drv)) {
>>> + return fdctrl->defaultB;
>>> + }
>>> +
>>> + return FDRIVE_DEFAULT;
>>> +}
>>> +
>>> static uint32_t fdctrl_read (void *opaque, uint32_t reg)
>>> {
>>> FDCtrl *fdctrl = opaque;
>>
>> Why do you need to guard with MAX_FD? If MAX_FD < 2, surely the
>> properties don't exist, and fdctrl->drives[i] still has its initial
>> value FDRIVE_DEFAULT, doesn't it?
>>
>
> Why would the properties not exist?
Before I answer your question: currently, MAX_FD is always 2, so the
question is moot. The #ifdeffery in fdc.c suggests the code is also
prepared for MAX_FD 4, but no other values.
Now your question. MAX_FD limits the number of drives the controller
supports. Surely the controller's QOM/qdev interface shouldn't expose
more drives than you can actually connect.
"isa-fdc" and "sysbus-fdc" support two, "driveA" and "driveB", mapping
to state.drives[0..1]. "SUNW,fdtwo" supports one "drive", mapping to
state.drives[0].
Note that state.drives[] has MAX_FD elements. If we changed MAX_FD to
one, we'd have to drop "isa-fdc"'s and "sysbus-fdc"'s "driveB", or else
they'd overrun state.drives[].
