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

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

On 4/8/21 11:38 AM, Markus Armbruster wrote:
> 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.

non-ISA FDC is only used on MIPS/SPARC.

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

Thanks for the impact analysis.

The fix seems harmless to me, but I'm fine having it fixed in 6.1
(this is an old bug, so not critical to have it fixed for 6.0).

Phil.