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(),
-- 
2.26.3

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.

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.

Am 07.04.2021 um 15:37 hat Philippe Mathieu-Daudé geschrieben:
> 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>

Thanks, applied to the block branch.

Kevin