Re: [PATCH 05/15] hw/ide: report the default CHS translation in IDENTIFY DEVICE

On 17/8/26 22:52, Denis V. Lunev wrote:
> From: Denis V. Lunev <[email protected]>
> 
> IDENTIFY DEVICE words 1, 3 and 6 describe the default CHS translation,
> and ATA-5 8.16.8 requires INITIALIZE DEVICE PARAMETERS to leave them
> alone; the translation in effect is described by words 54 to 56 instead.
> Words 3 and 6 were filled from s->heads and s->sectors, which the command
> replaces, so a guest that selected a translation of its own was told that
> its choice was what the drive came with, and could no longer find out the
> default. Word 1 is already right, as no command changes s->cylinders.
> 
> Report s->drive_heads and s->drive_sectors, which ide_init_drive() keeps
> for exactly this, along with the retired word 4 derived from them. The
> CompactFlash data labels those words as the default geometry too, and
> INITIALIZE DEVICE PARAMETERS is accepted for CFA drives, so fix both.
Cc: [email protected]

> 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/core.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 747fa71677..befbab9486 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -138,11 +138,12 @@ static void ide_identify(IDEState *s)
>       memset(p, 0, sizeof(s->identify_data));
>   
>       put_le16(p + 0, 0x0040);
> +    /* Words 1, 3 and 6 describe the default translation (ATA-5 8.16.8) */
>       put_le16(p + 1, s->cylinders);
> -    put_le16(p + 3, s->heads);
> -    put_le16(p + 4, 512 * s->sectors); /* XXX: retired, remove ? */
> +    put_le16(p + 3, s->drive_heads);
> +    put_le16(p + 4, 512 * s->drive_sectors); /* XXX: retired, remove ? */

If we are deliberately keeping this, can we update the XXX comment?
Otherwise LGTM:
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>

>       put_le16(p + 5, 512); /* XXX: retired, remove ? */
> -    put_le16(p + 6, s->sectors);
> +    put_le16(p + 6, s->drive_sectors);
>       padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
>       put_le16(p + 20, 3); /* XXX: retired, remove ? */
>       put_le16(p + 21, 512); /* cache size in sectors */
> @@ -332,8 +333,8 @@ static void ide_cfata_identify(IDEState *s)
>   
>       put_le16(p + 0, 0x848a);                    /* CF Storage Card signature */
>       put_le16(p + 1, s->cylinders);              /* Default cylinders */
> -    put_le16(p + 3, s->heads);                  /* Default heads */
> -    put_le16(p + 6, s->sectors);                /* Default sectors per track */
> +    put_le16(p + 3, s->drive_heads);            /* Default heads */
> +    put_le16(p + 6, s->drive_sectors);          /* Default sectors per track */
>       /* *(p + 7) := nb_sectors >> 16 -- see ide_cfata_identify_size */
>       /* *(p + 8) := nb_sectors       -- see ide_cfata_identify_size */
>       padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */