Re: [PATCH 13/15] hw/ide: revert the CHS translation on a hardware reset

Hi Denis,

On 17/8/26 22:52, Denis V. Lunev wrote:
> From: Denis V. Lunev <[email protected]>
> 
> A power on or hardware reset returns the device parameters to their
> power-on defaults (ATA-5 9.1). A software reset keeps them unless the
> guest asked with SET FEATURES 0xCC for the next reset to revert (ATA-5 9.2
> and 8.16.6). ide_reset() applied the second rule to every reset, so a
> translation a guest selected outlived the reset of the machine it selected
> it on, and the guest that came up next addressed the disk through a
> geometry it never asked for.
> 
> Neither ide_reset() nor, for AHCI, ide_bus_reset() could tell the two
> apart: a guest clearing SRST in the second host to device FIS of the
> software reset protocol lands in the same ahci_reset_port() as a COMRESET
> or a reset of the host adapter. Pass the kind down from the callers, which
> do know.
> 
> ide_drive_pre_load() stays necessary: it restores the same fields, but a
> vmstate cannot depend on its device having been reset first.
> 
> Cc: John Snow <[email protected]>
> Cc: Peter Maydell <[email protected]>
> Fixes: 176e4961bb33 ("hw/ide/core.c: Implement ATA INITIALIZE_DEVICE_PARAMETERS command")
> Signed-off-by: Denis V. Lunev <[email protected]>
> ---
>   hw/ide/ahci.c         | 12 ++++++------
>   hw/ide/cmd646.c       |  2 +-
>   hw/ide/core.c         | 18 +++++++++---------
>   hw/ide/ide-internal.h |  7 ++++++-
>   hw/ide/isa.c          |  2 +-
>   hw/ide/macio.c        |  2 +-
>   hw/ide/mmio.c         |  2 +-
>   hw/ide/piix.c         |  2 +-
>   hw/ide/sii3112.c      |  6 +++---
>   hw/ide/via.c          |  2 +-
>   10 files changed, 30 insertions(+), 25 deletions(-)


> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index c330508128..bd155f8d3a 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1347,7 +1347,7 @@ void ide_ioport_write(void *opaque, uint32_t addr, uint32_t val)
>       }
>   }
>   
> -static void ide_reset(IDEState *s)
> +static void ide_reset(IDEState *s, IDEResetKind kind)
>   {
>       trace_ide_reset(s);
>   
> @@ -1356,7 +1356,7 @@ static void ide_reset(IDEState *s)
>           s->pio_aiocb = NULL;
>       }
>   
> -    if (s->reset_reverts) {
> +    if (kind == IDE_RESET_HARDWARE || s->reset_reverts) {
>           s->reset_reverts = false;
>           s->heads         = s->drive_heads;
>           s->sectors       = s->drive_sectors;

The change is good, so:
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>

Still I think it could be logically better to rework ide_reset() and
use IDEResetKind as a IDEResetLevel instead, having IDE_RESET_HARDWARE
the higher level. Anyway just thinking about it, feel free to ignore me.

> diff --git a/hw/ide/ide-internal.h b/hw/ide/ide-internal.h
> index 281d07c9d5..094772209d 100644
> --- a/hw/ide/ide-internal.h
> +++ b/hw/ide/ide-internal.h
> @@ -393,7 +393,12 @@ extern const VMStateDescription vmstate_ide_drive;
>   #define VMSTATE_IDE_DRIVE(_field, _state) \
>       VMSTATE_STRUCT(_field, _state, 1, vmstate_ide_drive, IDEState)
>   
> -void ide_bus_reset(IDEBus *bus);
> +typedef enum {
> +    IDE_RESET_HARDWARE, /* power on, hardware reset or COMRESET, ATA-5 9.1 */
> +    IDE_RESET_SOFTWARE, /* SRST or DEVICE RESET, ATA-5 9.2 */
> +} IDEResetKind;