Re: [PATCH-for-6.0?] hw/block/fdc: Fix 'fallback' property on sysbus floppy disk controllers

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> Setting the 'fallback' property corrupts the QOM instance state
> (FDCtrlSysBus) because it accesses an incorrect offset (it uses
> the offset of the FDCtrlISABus state).
>
> Fixes: a73275dd6fc ("fdc: Add fallback option")
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/block/fdc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 82afda7f3a7..a825c2acbae 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -2893,7 +2893,7 @@ static Property sysbus_fdc_properties[] = {
>      DEFINE_PROP_SIGNED("fdtypeB", FDCtrlSysBus, 
> state.qdev_for_drives[1].type,
>                          FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
>                          FloppyDriveType),
> -    DEFINE_PROP_SIGNED("fallback", FDCtrlISABus, state.fallback,
> +    DEFINE_PROP_SIGNED("fallback", FDCtrlSysBus, state.fallback,
>                          FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
>                          FloppyDriveType),
>      DEFINE_PROP_END_OF_LIST(),
> @@ -2918,7 +2918,7 @@ static Property sun4m_fdc_properties[] = {
>      DEFINE_PROP_SIGNED("fdtype", FDCtrlSysBus, state.qdev_for_drives[0].type,
>                          FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
>                          FloppyDriveType),
> -    DEFINE_PROP_SIGNED("fallback", FDCtrlISABus, state.fallback,
> +    DEFINE_PROP_SIGNED("fallback", FDCtrlSysBus, state.fallback,
>                          FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
>                          FloppyDriveType),
>      DEFINE_PROP_END_OF_LIST(),

Reviewed-by: Markus Armbruster <armbru@redhat.com>

On whether to pick this into 6.0...

The patch has no effect unless someone or something uses "fallback" with
a non-ISA FDC.  There it fixes a bug.  The bug's exact impact is
unknown.  I figure I could find out, but it doesn't seem to be worth the
bother.

Commit a73275dd6fc:

    Currently, QEMU chooses a drive type automatically based on the inserted
    media. If there is no disk inserted, it chooses a 1.44MB drive type.
    
    Change this behavior to be configurable, but leave it defaulted to 1.44.
    
    This is not earnestly intended to be used by a user or a management
    library, but rather exists so that pre-2.6 board types can configure it
    to be a legacy value.

We do so only for "isa-fdc", in hw/core/machine.c.

I don't understand why we don't for the other devices, but that's
outside this patch's scope.

Downstreams could do it, but it wouldn't work.  They need this commit to
make it work.

Users (human or management application) should not use it, but of course
they might anyway.  This commit makes such (unadvisable) usage safe.

The reward is low, but so is the risk.  If I was the maintainer, I'd be
tempted to take it up to rc3.