Re: [PATCH 2/4] hw/block/fdc: Remove the check_media_rate property
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
---
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.
H, 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
Acked-by: John Snow
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
Re: [PATCH 2/4] hw/block/fdc: Remove the check_media_rate property
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
> ---
> 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.
H, 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
>> Acked-by: John Snow
>
> Thanks! ... since the first patch has already
Re: [PATCH 2/4] hw/block/fdc: Remove the check_media_rate property
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
---
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.
H, 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
Acked-by: John Snow
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
Re: [PATCH 2/4] hw/block/fdc: Remove the check_media_rate property
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
---
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.
H, 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
Acked-by: John Snow
Re: [PATCH 2/4] hw/block/fdc: Remove the check_media_rate property
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
---
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.
H, 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
Re: [PATCH 2/4] hw/block/fdc: Remove the check_media_rate property
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
---
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.
H, 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
[PATCH 2/4] hw/block/fdc: Remove the check_media_rate property
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
---
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"
