Re: [PATCH 06/15] hw/ide: keep the IDENTIFY DEVICE current geometry in sync

On 17/8/26 22:52, Denis V. Lunev wrote:
> From: Denis V. Lunev <[email protected]>
> 
> Bit 0 of IDENTIFY DEVICE word 53 says that words 54 to 58 describe the CHS
> translation in effect, and ATA-5 8.16.8 has INITIALIZE DEVICE PARAMETERS
> set words 55 and 56 to the heads and sectors per track it was given. The
> data is built once and then cached, so those words kept describing
> whatever was in effect when a guest first asked for IDENTIFY DEVICE: the
> device reported one geometry while addressing the medium with another, and
> nothing reported an error. The revert SET FEATURES 0xCC asks for on the
> next reset left the same disagreement.
> 
> Do not drop the cached data on a change, as parts of it are guest state
> rather than a description of the drive: SET FEATURES records the write
> cache setting in word 85, which ide_drive_post_load() reads back after
> migration. Update the affected words in place instead, the way
> ide_identify_size() does for the capacity words, and share the code with
> the two places that build the data.
> 
> An ATAPI device has no translation but does take SET FEATURES 0xCC, so
> leave its IDENTIFY PACKET DEVICE data alone, where those words differ.
> 
> 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 | 36 +++++++++++++++++++++---------------
>   1 file changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index befbab9486..9959437a63 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -110,6 +110,18 @@ static void put_le16(uint16_t *p, unsigned int v)
>       *p = cpu_to_le16(v);
>   }
>   
> +static void ide_identify_chs(IDEState *s)
> +{
> +    uint16_t *p = (uint16_t *)s->identify_data;
> +    unsigned int cur_sec = s->cylinders * s->heads * s->sectors;
> +
> +    put_le16(p + 54, s->cylinders);
> +    put_le16(p + 55, s->heads);
> +    put_le16(p + 56, s->sectors);
> +    put_le16(p + 57, cur_sec);
> +    put_le16(p + 58, cur_sec >> 16);
> +}
> +
>   static void ide_identify_size(IDEState *s)
>   {
>       uint16_t *p = (uint16_t *)s->identify_data;
> @@ -128,7 +140,6 @@ static void ide_identify_size(IDEState *s)
>   static void ide_identify(IDEState *s)
>   {
>       uint16_t *p;
> -    unsigned int oldsize;
>       IDEDevice *dev = s->unit ? s->bus->slave : s->bus->master;
>   
>       p = (uint16_t *)s->identify_data;
> @@ -158,12 +169,7 @@ static void ide_identify(IDEState *s)
>       put_le16(p + 51, 0x200); /* PIO transfer cycle */
>       put_le16(p + 52, 0x200); /* DMA transfer cycle */
>       put_le16(p + 53, 1 | (1 << 1) | (1 << 2)); /* words 54-58,64-70,88 are valid */
> -    put_le16(p + 54, s->cylinders);
> -    put_le16(p + 55, s->heads);
> -    put_le16(p + 56, s->sectors);
> -    oldsize = s->cylinders * s->heads * s->sectors;
> -    put_le16(p + 57, oldsize);
> -    put_le16(p + 58, oldsize >> 16);
> +    ide_identify_chs(s);
>       if (s->mult_sectors)
>           put_le16(p + 59, 0x100 | s->mult_sectors);
>       /* *(p + 60) := nb_sectors       -- see ide_identify_size */
> @@ -321,7 +327,6 @@ static void ide_cfata_identify_size(IDEState *s)
>   static void ide_cfata_identify(IDEState *s)
>   {
>       uint16_t *p;
> -    uint32_t cur_sec;
>   
>       p = (uint16_t *)s->identify_data;
>       if (s->identify_set) {
> @@ -329,8 +334,6 @@ static void ide_cfata_identify(IDEState *s)
>       }
>       memset(p, 0, sizeof(s->identify_data));
>   
> -    cur_sec = s->cylinders * s->heads * s->sectors;
> -
>       put_le16(p + 0, 0x848a);                    /* CF Storage Card signature */
>       put_le16(p + 1, s->cylinders);              /* Default cylinders */
>       put_le16(p + 3, s->drive_heads);            /* Default heads */
> @@ -350,11 +353,7 @@ static void ide_cfata_identify(IDEState *s)
>       put_le16(p + 51, 0x0002);                   /* PIO cycle timing mode */
>       put_le16(p + 52, 0x0001);                   /* DMA cycle timing mode */
>       put_le16(p + 53, 0x0003);                   /* Translation params valid */
> -    put_le16(p + 54, s->cylinders);             /* Current cylinders */
> -    put_le16(p + 55, s->heads);                 /* Current heads */
> -    put_le16(p + 56, s->sectors);               /* Current sectors */
> -    put_le16(p + 57, cur_sec);                  /* Current capacity */
> -    put_le16(p + 58, cur_sec >> 16);            /* Current capacity */
> +    ide_identify_chs(s);                        /* Current C/H/S and capacity */
>       if (s->mult_sectors)                        /* Multiple sector setting */
>           put_le16(p + 59, 0x100 | s->mult_sectors);
>       /* *(p + 60) := nb_sectors       -- see ide_cfata_identify_size */

Better split this patch in 2 atomic changes, extracting
ide_identify_chs() up to here and using it where appropriate
below here.

> @@ -1361,6 +1360,10 @@ static void ide_reset(IDEState *s)
>           s->reset_reverts = false;
>           s->heads         = s->drive_heads;
>           s->sectors       = s->drive_sectors;
> +        /* An ATAPI device takes SET FEATURES 0xCC but has no translation */
> +        if (s->identify_set && s->drive_kind != IDE_CD) {
> +            ide_identify_chs(s);
> +        }
>       }
>       if (s->drive_kind == IDE_CFATA)
>           s->mult_sectors = 0;
> @@ -1669,6 +1672,9 @@ static bool cmd_specify(IDEState *s, uint8_t cmd)
>   
>       s->heads = (s->select & (ATA_DEV_HS)) + 1;
>       s->sectors = s->nsector;
> +    if (s->identify_set) {
> +        ide_identify_chs(s);
> +    }
>       ide_bus_set_irq(s->bus);
>   
>       return true;