This was only required for the pc-1.0 and earlier machine types.
Now that these have been removed, we can also drop the corresponding
code from the FDC device.
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
hw/block/fdc.c | 17 ++---------------
tests/qemu-iotests/172.out | 35 -----------------------------------
2 files changed, 2 insertions(+), 50 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 292ea87805..198940e737 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -874,7 +874,6 @@ struct FDCtrl {
FloppyDriveType type;
} qdev_for_drives[MAX_FD];
int reset_sensei;
- uint32_t check_media_rate;
FloppyDriveType fallback; /* type=auto failure fallback */
/* Timers state */
uint8_t timer0;
@@ -1021,18 +1020,10 @@ static const VMStateDescription vmstate_fdrive_media_changed = {
}
};
-static bool fdrive_media_rate_needed(void *opaque)
-{
- FDrive *drive = opaque;
-
- return drive->fdctrl->check_media_rate;
-}
-
static const VMStateDescription vmstate_fdrive_media_rate = {
.name = "fdrive/media_rate",
.version_id = 1,
.minimum_version_id = 1,
- .needed = fdrive_media_rate_needed,
.fields = (VMStateField[]) {
VMSTATE_UINT8(media_rate, FDrive),
VMSTATE_END_OF_LIST()
@@ -1689,8 +1680,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
/* Check the data rate. If the programmed data rate does not match
* the currently inserted medium, the operation has to fail. */
- if (fdctrl->check_media_rate &&
- (fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
+ if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
FLOPPY_DPRINTF("data rate mismatch (fdc=%d, media=%d)\n",
fdctrl->dsr & FD_DSR_DRATEMASK, cur_drv->media_rate);
fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
@@ -2489,8 +2479,7 @@ static void fdctrl_result_timer(void *opaque)
cur_drv->sect = (cur_drv->sect % cur_drv->last_sect) + 1;
}
/* READ_ID can't automatically succeed! */
- if (fdctrl->check_media_rate &&
- (fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
+ if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
FLOPPY_DPRINTF("read id rate mismatch (fdc=%d, media=%d)\n",
fdctrl->dsr & FD_DSR_DRATEMASK, cur_drv->media_rate);
fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
@@ -2895,8 +2884,6 @@ static Property isa_fdc_properties[] = {
DEFINE_PROP_UINT32("dma", FDCtrlISABus, dma, 2),
DEFINE_PROP_DRIVE("driveA", FDCtrlISABus, state.qdev_for_drives[0].blk),
DEFINE_PROP_DRIVE("driveB", FDCtrlISABus, state.qdev_for_drives[1].blk),
- DEFINE_PROP_BIT("check_media_rate", FDCtrlISABus, state.check_media_rate,
- 0, true),
DEFINE_PROP_SIGNED("fdtypeA", FDCtrlISABus, state.qdev_for_drives[0].type,
FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
FloppyDriveType),
diff --git a/tests/qemu-iotests/172.out b/tests/qemu-iotests/172.out
index 2cd4a8fd83..349ae51d6c 100644
--- a/tests/qemu-iotests/172.out
+++ b/tests/qemu-iotests/172.out
@@ -14,7 +14,6 @@ Testing:
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -44,7 +43,6 @@ Testing: -fda TEST_DIR/t.qcow2
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -84,7 +82,6 @@ Testing: -fdb TEST_DIR/t.qcow2
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -139,7 +136,6 @@ Testing: -fda TEST_DIR/t.qcow2 -fdb TEST_DIR/t.qcow2.2
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -195,7 +191,6 @@ Testing: -fdb
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -236,7 +231,6 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -276,7 +270,6 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2,index=1
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -331,7 +324,6 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=floppy,file=TEST_DIR/t
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -392,7 +384,6 @@ Use -device floppy,unit=0,drive=... instead.
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -434,7 +425,6 @@ Use -device floppy,unit=1,drive=... instead.
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -478,7 +468,6 @@ Use -device floppy,unit=1,drive=... instead.
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -537,7 +526,6 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -577,7 +565,6 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,unit=1
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -617,7 +604,6 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qco
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -678,7 +664,6 @@ Use -device floppy,unit=1,drive=... instead.
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -736,7 +721,6 @@ Use -device floppy,unit=0,drive=... instead.
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -808,7 +792,6 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -864,7 +847,6 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -920,7 +902,6 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -976,7 +957,6 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -1041,7 +1021,6 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -1097,7 +1076,6 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -1161,7 +1139,6 @@ Use -device floppy,unit=0,drive=... instead.
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -1219,7 +1196,6 @@ Use -device floppy,unit=0,drive=... instead.
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -1277,7 +1253,6 @@ Use -device floppy,unit=1,drive=... instead.
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -1335,7 +1310,6 @@ Use -device floppy,unit=1,drive=... instead.
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -1391,7 +1365,6 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -global floppy.drive=none0 -device
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -1473,7 +1446,6 @@ Testing: -device floppy
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -1500,7 +1472,6 @@ Testing: -device floppy,drive-type=120
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -1527,7 +1498,6 @@ Testing: -device floppy,drive-type=144
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -1554,7 +1524,6 @@ Testing: -device floppy,drive-type=288
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -1584,7 +1553,6 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,drive-t
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -1624,7 +1592,6 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,drive-t
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -1667,7 +1634,6 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,logical
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
@@ -1707,7 +1673,6 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,physica
dma = 2 (0x2)
driveA = ""
driveB = ""
- check_media_rate = true
fdtypeA = "auto"
fdtypeB = "auto"
fallback = "288"
--
2.27.0
On 2/3/21 12:18 PM, Thomas Huth wrote:
> This was only required for the pc-1.0 and earlier machine types.
> Now that these have been removed, we can also drop the corresponding
> code from the FDC device.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> hw/block/fdc.c | 17 ++---------------
> tests/qemu-iotests/172.out | 35 -----------------------------------
> 2 files changed, 2 insertions(+), 50 deletions(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 292ea87805..198940e737 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -874,7 +874,6 @@ struct FDCtrl {
> FloppyDriveType type;
> } qdev_for_drives[MAX_FD];
> int reset_sensei;
> - uint32_t check_media_rate;
I am a bit of a dunce when it comes to the compatibility properties...
does this mess with the migration format?
I guess it doesn't, since it's not in the VMSTATE declaration.
Hmmmm, alright.
> FloppyDriveType fallback; /* type=auto failure fallback */
> /* Timers state */
> uint8_t timer0;
> @@ -1021,18 +1020,10 @@ static const VMStateDescription vmstate_fdrive_media_changed = {
> }
> };
>
> -static bool fdrive_media_rate_needed(void *opaque)
> -{
> - FDrive *drive = opaque;
> -
> - return drive->fdctrl->check_media_rate;
> -}
> -
> static const VMStateDescription vmstate_fdrive_media_rate = {
> .name = "fdrive/media_rate",
> .version_id = 1,
> .minimum_version_id = 1,
> - .needed = fdrive_media_rate_needed,
> .fields = (VMStateField[]) {
> VMSTATE_UINT8(media_rate, FDrive),
> VMSTATE_END_OF_LIST()
> @@ -1689,8 +1680,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
>
> /* Check the data rate. If the programmed data rate does not match
> * the currently inserted medium, the operation has to fail. */
> - if (fdctrl->check_media_rate &&
> - (fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
> + if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
> FLOPPY_DPRINTF("data rate mismatch (fdc=%d, media=%d)\n",
> fdctrl->dsr & FD_DSR_DRATEMASK, cur_drv->media_rate);
> fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
> @@ -2489,8 +2479,7 @@ static void fdctrl_result_timer(void *opaque)
> cur_drv->sect = (cur_drv->sect % cur_drv->last_sect) + 1;
> }
> /* READ_ID can't automatically succeed! */
> - if (fdctrl->check_media_rate &&
> - (fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
> + if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
> FLOPPY_DPRINTF("read id rate mismatch (fdc=%d, media=%d)\n",
> fdctrl->dsr & FD_DSR_DRATEMASK, cur_drv->media_rate);
> fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
> @@ -2895,8 +2884,6 @@ static Property isa_fdc_properties[] = {
> DEFINE_PROP_UINT32("dma", FDCtrlISABus, dma, 2),
> DEFINE_PROP_DRIVE("driveA", FDCtrlISABus, state.qdev_for_drives[0].blk),
> DEFINE_PROP_DRIVE("driveB", FDCtrlISABus, state.qdev_for_drives[1].blk),
> - DEFINE_PROP_BIT("check_media_rate", FDCtrlISABus, state.check_media_rate,
> - 0, true),
Could you theoretically set this via QOM commands in QMP, and claim that
this is a break in behavior?
Though, it's ENTIRELY undocumented, so ... it's probably fine, I think.
Probably. (Please soothe my troubled mind.)
--js
On 05/02/2021 01.40, John Snow wrote:
> On 2/3/21 12:18 PM, Thomas Huth wrote:
>> This was only required for the pc-1.0 and earlier machine types.
>> Now that these have been removed, we can also drop the corresponding
>> code from the FDC device.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>> hw/block/fdc.c | 17 ++---------------
>> tests/qemu-iotests/172.out | 35 -----------------------------------
>> 2 files changed, 2 insertions(+), 50 deletions(-)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index 292ea87805..198940e737 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -874,7 +874,6 @@ struct FDCtrl {
>> FloppyDriveType type;
>> } qdev_for_drives[MAX_FD];
>> int reset_sensei;
>> - uint32_t check_media_rate;
>
> I am a bit of a dunce when it comes to the compatibility properties... does
> this mess with the migration format?
>
> I guess it doesn't, since it's not in the VMSTATE declaration.
>
> Hmmmm, alright.
I think that should be fine, yes.
>> FloppyDriveType fallback; /* type=auto failure fallback */
>> /* Timers state */
>> uint8_t timer0;
>> @@ -1021,18 +1020,10 @@ static const VMStateDescription
>> vmstate_fdrive_media_changed = {
>> }
>> };
>> -static bool fdrive_media_rate_needed(void *opaque)
>> -{
>> - FDrive *drive = opaque;
>> -
>> - return drive->fdctrl->check_media_rate;
>> -}
>> -
>> static const VMStateDescription vmstate_fdrive_media_rate = {
>> .name = "fdrive/media_rate",
>> .version_id = 1,
>> .minimum_version_id = 1,
>> - .needed = fdrive_media_rate_needed,
>> .fields = (VMStateField[]) {
>> VMSTATE_UINT8(media_rate, FDrive),
>> VMSTATE_END_OF_LIST()
>> @@ -1689,8 +1680,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl,
>> int direction)
>> /* Check the data rate. If the programmed data rate does not match
>> * the currently inserted medium, the operation has to fail. */
>> - if (fdctrl->check_media_rate &&
>> - (fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
>> + if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
>> FLOPPY_DPRINTF("data rate mismatch (fdc=%d, media=%d)\n",
>> fdctrl->dsr & FD_DSR_DRATEMASK,
>> cur_drv->media_rate);
>> fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
>> @@ -2489,8 +2479,7 @@ static void fdctrl_result_timer(void *opaque)
>> cur_drv->sect = (cur_drv->sect % cur_drv->last_sect) + 1;
>> }
>> /* READ_ID can't automatically succeed! */
>> - if (fdctrl->check_media_rate &&
>> - (fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
>> + if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
>> FLOPPY_DPRINTF("read id rate mismatch (fdc=%d, media=%d)\n",
>> fdctrl->dsr & FD_DSR_DRATEMASK,
>> cur_drv->media_rate);
>> fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
>> @@ -2895,8 +2884,6 @@ static Property isa_fdc_properties[] = {
>> DEFINE_PROP_UINT32("dma", FDCtrlISABus, dma, 2),
>> DEFINE_PROP_DRIVE("driveA", FDCtrlISABus,
>> state.qdev_for_drives[0].blk),
>> DEFINE_PROP_DRIVE("driveB", FDCtrlISABus,
>> state.qdev_for_drives[1].blk),
>> - DEFINE_PROP_BIT("check_media_rate", FDCtrlISABus,
>> state.check_media_rate,
>> - 0, true),
>
> Could you theoretically set this via QOM commands in QMP, and claim that
> this is a break in behavior?
>
> Though, it's ENTIRELY undocumented, so ... it's probably fine, I think.
> Probably. (Please soothe my troubled mind.)
A user actually could mess with this property even on the command line, e.g.
by using:
qemu-system-x86_64 -global isa-fdc.check_media_rate=false
... but, as you said, it's completely undocumented, the property is really
just there for the internal use of machine type compatibility. We've done
such clean-ups in the past already, see e.g. c6026998eef382d7ad76 or
2a4dbaf1c0db2453ab78f, so I think this should be fine. But if you disagree,
I could replace this by a patch that adds this property to the list of
deprecated features instead, so we could at least remove it after it has
been deprecated for two releases?
Thomas
On 2/5/21 1:37 AM, Thomas Huth wrote:
> On 05/02/2021 01.40, John Snow wrote:
>> On 2/3/21 12:18 PM, Thomas Huth wrote:
>>> This was only required for the pc-1.0 and earlier machine types.
>>> Now that these have been removed, we can also drop the corresponding
>>> code from the FDC device.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>> hw/block/fdc.c | 17 ++---------------
>>> tests/qemu-iotests/172.out | 35 -----------------------------------
>>> 2 files changed, 2 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>> index 292ea87805..198940e737 100644
>>> --- a/hw/block/fdc.c
>>> +++ b/hw/block/fdc.c
>>> @@ -874,7 +874,6 @@ struct FDCtrl {
>>> FloppyDriveType type;
>>> } qdev_for_drives[MAX_FD];
>>> int reset_sensei;
>>> - uint32_t check_media_rate;
>>
>> I am a bit of a dunce when it comes to the compatibility properties...
>> does this mess with the migration format?
>>
>> I guess it doesn't, since it's not in the VMSTATE declaration.
>>
>> Hmmmm, alright.
>
> I think that should be fine, yes.
>
>>> FloppyDriveType fallback; /* type=auto failure fallback */
>>> /* Timers state */
>>> uint8_t timer0;
>>> @@ -1021,18 +1020,10 @@ static const VMStateDescription
>>> vmstate_fdrive_media_changed = {
>>> }
>>> };
>>> -static bool fdrive_media_rate_needed(void *opaque)
>>> -{
>>> - FDrive *drive = opaque;
>>> -
>>> - return drive->fdctrl->check_media_rate;
>>> -}
>>> -
>>> static const VMStateDescription vmstate_fdrive_media_rate = {
>>> .name = "fdrive/media_rate",
>>> .version_id = 1,
>>> .minimum_version_id = 1,
>>> - .needed = fdrive_media_rate_needed,
>>> .fields = (VMStateField[]) {
>>> VMSTATE_UINT8(media_rate, FDrive),
>>> VMSTATE_END_OF_LIST()
>>> @@ -1689,8 +1680,7 @@ static void fdctrl_start_transfer(FDCtrl
>>> *fdctrl, int direction)
>>> /* Check the data rate. If the programmed data rate does not match
>>> * the currently inserted medium, the operation has to fail. */
>>> - if (fdctrl->check_media_rate &&
>>> - (fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
>>> + if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
>>> FLOPPY_DPRINTF("data rate mismatch (fdc=%d, media=%d)\n",
>>> fdctrl->dsr & FD_DSR_DRATEMASK,
>>> cur_drv->media_rate);
>>> fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
>>> @@ -2489,8 +2479,7 @@ static void fdctrl_result_timer(void *opaque)
>>> cur_drv->sect = (cur_drv->sect % cur_drv->last_sect) + 1;
>>> }
>>> /* READ_ID can't automatically succeed! */
>>> - if (fdctrl->check_media_rate &&
>>> - (fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
>>> + if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
>>> FLOPPY_DPRINTF("read id rate mismatch (fdc=%d, media=%d)\n",
>>> fdctrl->dsr & FD_DSR_DRATEMASK,
>>> cur_drv->media_rate);
>>> fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
>>> @@ -2895,8 +2884,6 @@ static Property isa_fdc_properties[] = {
>>> DEFINE_PROP_UINT32("dma", FDCtrlISABus, dma, 2),
>>> DEFINE_PROP_DRIVE("driveA", FDCtrlISABus,
>>> state.qdev_for_drives[0].blk),
>>> DEFINE_PROP_DRIVE("driveB", FDCtrlISABus,
>>> state.qdev_for_drives[1].blk),
>>> - DEFINE_PROP_BIT("check_media_rate", FDCtrlISABus,
>>> state.check_media_rate,
>>> - 0, true),
>>
>> Could you theoretically set this via QOM commands in QMP, and claim
>> that this is a break in behavior?
>>
>> Though, it's ENTIRELY undocumented, so ... it's probably fine, I
>> think. Probably. (Please soothe my troubled mind.)
>
> A user actually could mess with this property even on the command line,
> e.g. by using:
>
> qemu-system-x86_64 -global isa-fdc.check_media_rate=false
>
> ... but, as you said, it's completely undocumented, the property is
> really just there for the internal use of machine type compatibility.
> We've done such clean-ups in the past already, see e.g.
> c6026998eef382d7ad76 or 2a4dbaf1c0db2453ab78f, so I think this should be
> fine. But if you disagree, I could replace this by a patch that adds
> this property to the list of deprecated features instead, so we could at
> least remove it after it has been deprecated for two releases?
>
I don't think it's necessary, personally -- just wanted to make sure I
knew the exact stakes here.
Reviewed-by: John Snow <jsnow@redhat.com>
Acked-by: John Snow <jsnow@redhat.com>
On 05/02/2021 21.15, John Snow wrote:
> On 2/5/21 1:37 AM, Thomas Huth wrote:
>> On 05/02/2021 01.40, John Snow wrote:
>>> On 2/3/21 12:18 PM, Thomas Huth wrote:
>>>> This was only required for the pc-1.0 and earlier machine types.
>>>> Now that these have been removed, we can also drop the corresponding
>>>> code from the FDC device.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>> hw/block/fdc.c | 17 ++---------------
>>>> tests/qemu-iotests/172.out | 35 -----------------------------------
>>>> 2 files changed, 2 insertions(+), 50 deletions(-)
>>>>
>>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>>> index 292ea87805..198940e737 100644
>>>> --- a/hw/block/fdc.c
>>>> +++ b/hw/block/fdc.c
>>>> @@ -874,7 +874,6 @@ struct FDCtrl {
>>>> FloppyDriveType type;
>>>> } qdev_for_drives[MAX_FD];
>>>> int reset_sensei;
>>>> - uint32_t check_media_rate;
>>>
>>> I am a bit of a dunce when it comes to the compatibility properties...
>>> does this mess with the migration format?
>>>
>>> I guess it doesn't, since it's not in the VMSTATE declaration.
>>>
>>> Hmmmm, alright.
>>
>> I think that should be fine, yes.
>>
>>>> FloppyDriveType fallback; /* type=auto failure fallback */
>>>> /* Timers state */
>>>> uint8_t timer0;
>>>> @@ -1021,18 +1020,10 @@ static const VMStateDescription
>>>> vmstate_fdrive_media_changed = {
>>>> }
>>>> };
>>>> -static bool fdrive_media_rate_needed(void *opaque)
>>>> -{
>>>> - FDrive *drive = opaque;
>>>> -
>>>> - return drive->fdctrl->check_media_rate;
>>>> -}
>>>> -
>>>> static const VMStateDescription vmstate_fdrive_media_rate = {
>>>> .name = "fdrive/media_rate",
>>>> .version_id = 1,
>>>> .minimum_version_id = 1,
>>>> - .needed = fdrive_media_rate_needed,
>>>> .fields = (VMStateField[]) {
>>>> VMSTATE_UINT8(media_rate, FDrive),
>>>> VMSTATE_END_OF_LIST()
>>>> @@ -1689,8 +1680,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl,
>>>> int direction)
>>>> /* Check the data rate. If the programmed data rate does not match
>>>> * the currently inserted medium, the operation has to fail. */
>>>> - if (fdctrl->check_media_rate &&
>>>> - (fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
>>>> + if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
>>>> FLOPPY_DPRINTF("data rate mismatch (fdc=%d, media=%d)\n",
>>>> fdctrl->dsr & FD_DSR_DRATEMASK,
>>>> cur_drv->media_rate);
>>>> fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
>>>> @@ -2489,8 +2479,7 @@ static void fdctrl_result_timer(void *opaque)
>>>> cur_drv->sect = (cur_drv->sect % cur_drv->last_sect) + 1;
>>>> }
>>>> /* READ_ID can't automatically succeed! */
>>>> - if (fdctrl->check_media_rate &&
>>>> - (fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
>>>> + if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
>>>> FLOPPY_DPRINTF("read id rate mismatch (fdc=%d, media=%d)\n",
>>>> fdctrl->dsr & FD_DSR_DRATEMASK,
>>>> cur_drv->media_rate);
>>>> fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
>>>> @@ -2895,8 +2884,6 @@ static Property isa_fdc_properties[] = {
>>>> DEFINE_PROP_UINT32("dma", FDCtrlISABus, dma, 2),
>>>> DEFINE_PROP_DRIVE("driveA", FDCtrlISABus,
>>>> state.qdev_for_drives[0].blk),
>>>> DEFINE_PROP_DRIVE("driveB", FDCtrlISABus,
>>>> state.qdev_for_drives[1].blk),
>>>> - DEFINE_PROP_BIT("check_media_rate", FDCtrlISABus,
>>>> state.check_media_rate,
>>>> - 0, true),
>>>
>>> Could you theoretically set this via QOM commands in QMP, and claim that
>>> this is a break in behavior?
>>>
>>> Though, it's ENTIRELY undocumented, so ... it's probably fine, I think.
>>> Probably. (Please soothe my troubled mind.)
>>
>> A user actually could mess with this property even on the command line,
>> e.g. by using:
>>
>> qemu-system-x86_64 -global isa-fdc.check_media_rate=false
>>
>> ... but, as you said, it's completely undocumented, the property is really
>> just there for the internal use of machine type compatibility. We've done
>> such clean-ups in the past already, see e.g. c6026998eef382d7ad76 or
>> 2a4dbaf1c0db2453ab78f, so I think this should be fine. But if you
>> disagree, I could replace this by a patch that adds this property to the
>> list of deprecated features instead, so we could at least remove it after
>> it has been deprecated for two releases?
>>
>
> I don't think it's necessary, personally -- just wanted to make sure I knew
> the exact stakes here.
>
> Reviewed-by: John Snow <jsnow@redhat.com>
> Acked-by: John Snow <jsnow@redhat.com>
Thanks! ... since the first patch has already been merged through Michael's
tree, could you then please take this patch here through your floppy / block
branch, John? Or maybe it could also go via qemu-trivial?
Thomas
Le 08/02/2021 à 08:05, Thomas Huth a écrit :
> On 05/02/2021 21.15, John Snow wrote:
>> On 2/5/21 1:37 AM, Thomas Huth wrote:
>>> On 05/02/2021 01.40, John Snow wrote:
>>>> On 2/3/21 12:18 PM, Thomas Huth wrote:
>>>>> This was only required for the pc-1.0 and earlier machine types.
>>>>> Now that these have been removed, we can also drop the corresponding
>>>>> code from the FDC device.
>>>>>
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>> hw/block/fdc.c | 17 ++---------------
>>>>> tests/qemu-iotests/172.out | 35 -----------------------------------
>>>>> 2 files changed, 2 insertions(+), 50 deletions(-)
>>>>>
>>>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>>>> index 292ea87805..198940e737 100644
>>>>> --- a/hw/block/fdc.c
>>>>> +++ b/hw/block/fdc.c
>>>>> @@ -874,7 +874,6 @@ struct FDCtrl {
>>>>> FloppyDriveType type;
>>>>> } qdev_for_drives[MAX_FD];
>>>>> int reset_sensei;
>>>>> - uint32_t check_media_rate;
>>>>
>>>> I am a bit of a dunce when it comes to the compatibility properties... does this mess with the
>>>> migration format?
>>>>
>>>> I guess it doesn't, since it's not in the VMSTATE declaration.
>>>>
>>>> Hmmmm, alright.
>>>
>>> I think that should be fine, yes.
>>>
>>>>> FloppyDriveType fallback; /* type=auto failure fallback */
>>>>> /* Timers state */
>>>>> uint8_t timer0;
>>>>> @@ -1021,18 +1020,10 @@ static const VMStateDescription vmstate_fdrive_media_changed = {
>>>>> }
>>>>> };
>>>>> -static bool fdrive_media_rate_needed(void *opaque)
>>>>> -{
>>>>> - FDrive *drive = opaque;
>>>>> -
>>>>> - return drive->fdctrl->check_media_rate;
>>>>> -}
>>>>> -
>>>>> static const VMStateDescription vmstate_fdrive_media_rate = {
>>>>> .name = "fdrive/media_rate",
>>>>> .version_id = 1,
>>>>> .minimum_version_id = 1,
>>>>> - .needed = fdrive_media_rate_needed,
>>>>> .fields = (VMStateField[]) {
>>>>> VMSTATE_UINT8(media_rate, FDrive),
>>>>> VMSTATE_END_OF_LIST()
>>>>> @@ -1689,8 +1680,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
>>>>> /* Check the data rate. If the programmed data rate does not match
>>>>> * the currently inserted medium, the operation has to fail. */
>>>>> - if (fdctrl->check_media_rate &&
>>>>> - (fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
>>>>> + if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
>>>>> FLOPPY_DPRINTF("data rate mismatch (fdc=%d, media=%d)\n",
>>>>> fdctrl->dsr & FD_DSR_DRATEMASK, cur_drv->media_rate);
>>>>> fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
>>>>> @@ -2489,8 +2479,7 @@ static void fdctrl_result_timer(void *opaque)
>>>>> cur_drv->sect = (cur_drv->sect % cur_drv->last_sect) + 1;
>>>>> }
>>>>> /* READ_ID can't automatically succeed! */
>>>>> - if (fdctrl->check_media_rate &&
>>>>> - (fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
>>>>> + if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
>>>>> FLOPPY_DPRINTF("read id rate mismatch (fdc=%d, media=%d)\n",
>>>>> fdctrl->dsr & FD_DSR_DRATEMASK, cur_drv->media_rate);
>>>>> fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
>>>>> @@ -2895,8 +2884,6 @@ static Property isa_fdc_properties[] = {
>>>>> DEFINE_PROP_UINT32("dma", FDCtrlISABus, dma, 2),
>>>>> DEFINE_PROP_DRIVE("driveA", FDCtrlISABus, state.qdev_for_drives[0].blk),
>>>>> DEFINE_PROP_DRIVE("driveB", FDCtrlISABus, state.qdev_for_drives[1].blk),
>>>>> - DEFINE_PROP_BIT("check_media_rate", FDCtrlISABus, state.check_media_rate,
>>>>> - 0, true),
>>>>
>>>> Could you theoretically set this via QOM commands in QMP, and claim that this is a break in
>>>> behavior?
>>>>
>>>> Though, it's ENTIRELY undocumented, so ... it's probably fine, I think. Probably. (Please soothe
>>>> my troubled mind.)
>>>
>>> A user actually could mess with this property even on the command line, e.g. by using:
>>>
>>> qemu-system-x86_64 -global isa-fdc.check_media_rate=false
>>>
>>> ... but, as you said, it's completely undocumented, the property is really just there for the
>>> internal use of machine type compatibility. We've done such clean-ups in the past already, see
>>> e.g. c6026998eef382d7ad76 or 2a4dbaf1c0db2453ab78f, so I think this should be fine. But if you
>>> disagree, I could replace this by a patch that adds this property to the list of deprecated
>>> features instead, so we could at least remove it after it has been deprecated for two releases?
>>>
>>
>> I don't think it's necessary, personally -- just wanted to make sure I knew the exact stakes here.
>>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>> Acked-by: John Snow <jsnow@redhat.com>
>
> Thanks! ... since the first patch has already been merged through Michael's tree, could you then
> please take this patch here through your floppy / block branch, John? Or maybe it could also go via
> qemu-trivial?
Applied to my trivial-patches branch.
Thanks,
Laurent
On 2/13/21 5:41 PM, Laurent Vivier wrote:
> Le 08/02/2021 à 08:05, Thomas Huth a écrit :
>> On 05/02/2021 21.15, John Snow wrote:
>>> On 2/5/21 1:37 AM, Thomas Huth wrote:
>>>> On 05/02/2021 01.40, John Snow wrote:
>>>>> On 2/3/21 12:18 PM, Thomas Huth wrote:
>>>>>> This was only required for the pc-1.0 and earlier machine types.
>>>>>> Now that these have been removed, we can also drop the corresponding
>>>>>> code from the FDC device.
>>>>>>
>>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>>> ---
>>>>>> hw/block/fdc.c | 17 ++---------------
>>>>>> tests/qemu-iotests/172.out | 35 -----------------------------------
>>>>>> 2 files changed, 2 insertions(+), 50 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>>>>> index 292ea87805..198940e737 100644
>>>>>> --- a/hw/block/fdc.c
>>>>>> +++ b/hw/block/fdc.c
>>>>>> @@ -874,7 +874,6 @@ struct FDCtrl {
>>>>>> FloppyDriveType type;
>>>>>> } qdev_for_drives[MAX_FD];
>>>>>> int reset_sensei;
>>>>>> - uint32_t check_media_rate;
>>>>>
>>>>> I am a bit of a dunce when it comes to the compatibility properties... does this mess with the
>>>>> migration format?
>>>>>
>>>>> I guess it doesn't, since it's not in the VMSTATE declaration.
>>>>>
>>>>> Hmmmm, alright.
>>>>
>>>> I think that should be fine, yes.
>>>>
>>>>>> FloppyDriveType fallback; /* type=auto failure fallback */
>>>>>> /* Timers state */
>>>>>> uint8_t timer0;
>>>>>> @@ -1021,18 +1020,10 @@ static const VMStateDescription vmstate_fdrive_media_changed = {
>>>>>> }
>>>>>> };
>>>>>> -static bool fdrive_media_rate_needed(void *opaque)
>>>>>> -{
>>>>>> - FDrive *drive = opaque;
>>>>>> -
>>>>>> - return drive->fdctrl->check_media_rate;
>>>>>> -}
>>>>>> -
>>>>>> static const VMStateDescription vmstate_fdrive_media_rate = {
>>>>>> .name = "fdrive/media_rate",
>>>>>> .version_id = 1,
>>>>>> .minimum_version_id = 1,
>>>>>> - .needed = fdrive_media_rate_needed,
>>>>>> .fields = (VMStateField[]) {
>>>>>> VMSTATE_UINT8(media_rate, FDrive),
>>>>>> VMSTATE_END_OF_LIST()
>>>>>> @@ -1689,8 +1680,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
>>>>>> /* Check the data rate. If the programmed data rate does not match
>>>>>> * the currently inserted medium, the operation has to fail. */
>>>>>> - if (fdctrl->check_media_rate &&
>>>>>> - (fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
>>>>>> + if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
>>>>>> FLOPPY_DPRINTF("data rate mismatch (fdc=%d, media=%d)\n",
>>>>>> fdctrl->dsr & FD_DSR_DRATEMASK, cur_drv->media_rate);
>>>>>> fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
>>>>>> @@ -2489,8 +2479,7 @@ static void fdctrl_result_timer(void *opaque)
>>>>>> cur_drv->sect = (cur_drv->sect % cur_drv->last_sect) + 1;
>>>>>> }
>>>>>> /* READ_ID can't automatically succeed! */
>>>>>> - if (fdctrl->check_media_rate &&
>>>>>> - (fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
>>>>>> + if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
>>>>>> FLOPPY_DPRINTF("read id rate mismatch (fdc=%d, media=%d)\n",
>>>>>> fdctrl->dsr & FD_DSR_DRATEMASK, cur_drv->media_rate);
>>>>>> fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
>>>>>> @@ -2895,8 +2884,6 @@ static Property isa_fdc_properties[] = {
>>>>>> DEFINE_PROP_UINT32("dma", FDCtrlISABus, dma, 2),
>>>>>> DEFINE_PROP_DRIVE("driveA", FDCtrlISABus, state.qdev_for_drives[0].blk),
>>>>>> DEFINE_PROP_DRIVE("driveB", FDCtrlISABus, state.qdev_for_drives[1].blk),
>>>>>> - DEFINE_PROP_BIT("check_media_rate", FDCtrlISABus, state.check_media_rate,
>>>>>> - 0, true),
>>>>>
>>>>> Could you theoretically set this via QOM commands in QMP, and claim that this is a break in
>>>>> behavior?
>>>>>
>>>>> Though, it's ENTIRELY undocumented, so ... it's probably fine, I think. Probably. (Please soothe
>>>>> my troubled mind.)
>>>>
>>>> A user actually could mess with this property even on the command line, e.g. by using:
>>>>
>>>> qemu-system-x86_64 -global isa-fdc.check_media_rate=false
>>>>
>>>> ... but, as you said, it's completely undocumented, the property is really just there for the
>>>> internal use of machine type compatibility. We've done such clean-ups in the past already, see
>>>> e.g. c6026998eef382d7ad76 or 2a4dbaf1c0db2453ab78f, so I think this should be fine. But if you
>>>> disagree, I could replace this by a patch that adds this property to the list of deprecated
>>>> features instead, so we could at least remove it after it has been deprecated for two releases?
>>>>
>>>
>>> I don't think it's necessary, personally -- just wanted to make sure I knew the exact stakes here.
>>>
>>> Reviewed-by: John Snow <jsnow@redhat.com>
>>> Acked-by: John Snow <jsnow@redhat.com>
>>
>> Thanks! ... since the first patch has already been merged through Michael's tree, could you then
>> please take this patch here through your floppy / block branch, John? Or maybe it could also go via
>> qemu-trivial?
>
> Applied to my trivial-patches branch.
>
> Thanks,
> Laurent
>
Thank you, Laurent!
--js
© 2016 - 2026 Red Hat, Inc.