Re: [Qemu-devel] [PATCH v3] ide: Add resize callback to ide/core

[Markus Armbruster]

John Snow <address@hidden> writes:

> Currently, if the block device backing the IDE drive is resized,
> the information about the device as cached inside of the IDEState
> structure is not updated, thus when a guest OS re-queries the drive,
> it is unable to see the expanded size.
>
> This patch adds a resize callback that updates the IDENTIFY data
> buffer in order to correct this.
>
> Lastly, a Linux guest as-is cannot resize a libata drive while in-use,
> but it can see the expanded size as part of a bus rescan event.
> This patch also allows guests such as Linux to see the new drive size
> after a soft reboot event, without having to exit the QEMU process.
>
> Signed-off-by: John Snow <address@hidden>
> ---
>
> V3:
>  - Factored out the size update into new functions.
>  - Fixed the size update for CFATA.
>  - Added assertion to clarify that ide_resize_cb is non-atapi.
>
> V2:
>  - Do not attempt to update geometry values, to avoid clobbering
>    user-specified values, if they exist.
>  - Do not regenerate the entire IDENTIFY buffer to avoid losing
>    any settings that occurred during normal operation.
>
>  hw/ide/core.c | 69 
> ++++++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 59 insertions(+), 10 deletions(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index db191a6..1fde54e 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -75,6 +75,17 @@ static void put_le16(uint16_t *p, unsigned int v)
>      *p = cpu_to_le16(v);
>  }
>  
> +static void ide_identify_size(IDEState *s)
> +{
> +    uint16_t *p = (uint16_t *)s->identify_data;
> +    put_le16(p + 60, s->nb_sectors);
> +    put_le16(p + 61, s->nb_sectors >> 16);
> +    put_le16(p + 100, s->nb_sectors);
> +    put_le16(p + 101, s->nb_sectors >> 16);
> +    put_le16(p + 102, s->nb_sectors >> 32);
> +    put_le16(p + 103, s->nb_sectors >> 48);
> +}

This gave me pause, because identify_data() updates s->io_buffer, not
s->identify_data.  It works fine, because...

> +
>  static void ide_identify(IDEState *s)
>  {
>      uint16_t *p;
> @@ -116,8 +127,8 @@ static void ide_identify(IDEState *s)
>      put_le16(p + 58, oldsize >> 16);
>      if (s->mult_sectors)
>          put_le16(p + 59, 0x100 | s->mult_sectors);
> -    put_le16(p + 60, s->nb_sectors);
> -    put_le16(p + 61, s->nb_sectors >> 16);
> +    /* *(p + 60) := nb_sectors       -- see ide_identify_size */
> +    /* *(p + 61) := nb_sectors >> 16 -- see ide_identify_size */
>      put_le16(p + 62, 0x07); /* single word dma0-2 supported */
>      put_le16(p + 63, 0x07); /* mdma0-2 supported */
>      put_le16(p + 64, 0x03); /* pio3-4 supported */
> @@ -162,10 +173,10 @@ static void ide_identify(IDEState *s)
>      }
>      put_le16(p + 88, 0x3f | (1 << 13)); /* udma5 set and supported */
>      put_le16(p + 93, 1 | (1 << 14) | 0x2000);
> -    put_le16(p + 100, s->nb_sectors);
> -    put_le16(p + 101, s->nb_sectors >> 16);
> -    put_le16(p + 102, s->nb_sectors >> 32);
> -    put_le16(p + 103, s->nb_sectors >> 48);
> +    /* *(p + 100) := nb_sectors       -- see ide_identify_size */
> +    /* *(p + 101) := nb_sectors >> 16 -- see ide_identify_size */
> +    /* *(p + 102) := nb_sectors >> 32 -- see ide_identify_size */
> +    /* *(p + 103) := nb_sectors >> 48 -- see ide_identify_size */
>  
>      if (dev && dev->conf.physical_block_size)
>          put_le16(p + 106, 0x6000 | get_physical_block_exp(&dev->conf));
> @@ -181,6 +192,7 @@ static void ide_identify(IDEState *s)
>      }
>  
>      memcpy(s->identify_data, p, sizeof(s->identify_data));
> +    ide_identify_size(s);
>      s->identify_set = 1;
>  }
>  

... you make it call ide_identify_size() after copying s->io_buffer to
s->identify_data.

> @@ -237,6 +249,15 @@ static void ide_atapi_identify(IDEState *s)
>      s->identify_set = 1;
>  }
>  
> +static void ide_cfata_identify_size(IDEState *s)
> +{
> +    uint16_t *p = (uint16_t *)s->identify_data;
> +    put_le16(p + 7, s->nb_sectors >> 16);  /* Sectors per card */
> +    put_le16(p + 8, s->nb_sectors);        /* Sectors per card */
> +    put_le16(p + 60, s->nb_sectors);       /* Total LBA sectors */
> +    put_le16(p + 61, s->nb_sectors >> 16); /* Total LBA sectors */
> +}
> +

This one is easier to understand, because ide_cfata_identify()
constructs identify data where it belongs, in s->identify_data.

>  static void ide_cfata_identify(IDEState *s)
>  {
>      uint16_t *p;
> @@ -254,8 +275,8 @@ static void ide_cfata_identify(IDEState *s)
>      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 + 7, s->nb_sectors >> 16);    /* Sectors per card */
> -    put_le16(p + 8, s->nb_sectors);          /* Sectors per card */
> +    /* *(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 */
>      put_le16(p + 22, 0x0004);                        /* ECC bytes */
>      padstr((char *) (p + 23), s->version, 8);        /* Firmware Revision */
> @@ -276,8 +297,8 @@ static void ide_cfata_identify(IDEState *s)
>      put_le16(p + 58, cur_sec >> 16);         /* Current capacity */
>      if (s->mult_sectors)                     /* Multiple sector setting */
>          put_le16(p + 59, 0x100 | s->mult_sectors);
> -    put_le16(p + 60, s->nb_sectors);         /* Total LBA sectors */
> -    put_le16(p + 61, s->nb_sectors >> 16);   /* Total LBA sectors */
> +    /* *(p + 60) := nb_sectors       -- see ide_cfata_identify_size */
> +    /* *(p + 61) := nb_sectors >> 16 -- see ide_cfata_identify_size */
>      put_le16(p + 63, 0x0203);                        /* Multiword DMA 
> capability */
>      put_le16(p + 64, 0x0001);                        /* Flow Control PIO 
> support */
>      put_le16(p + 65, 0x0096);                        /* Min. Multiword DMA 
> cycle */
> @@ -297,6 +318,7 @@ static void ide_cfata_identify(IDEState *s)
>      put_le16(p + 160, 0x8100);                       /* Power requirement */
>      put_le16(p + 161, 0x8001);                       /* CF command set */
>  
> +    ide_cfata_identify_size(s);
>      s->identify_set = 1;
>  
>  fill_buffer:

Factoring out the nb_sectors part removed the code duplication, but the
comments duplicate it right back %-)

I hope you didn't factor out just to please me.  The choice is yours.  I
hope I was able to convey that in my review of v2.

> @@ -2099,6 +2121,28 @@ static bool ide_cd_is_medium_locked(void *opaque)
>      return ((IDEState *)opaque)->tray_locked;
>  }
>  
> +static void ide_resize_cb(void *opaque)
> +{
> +    IDEState *s = opaque;
> +    uint64_t nb_sectors;
> +
> +    if (!s->identify_set) {
> +        return;
> +    }
> +
> +    bdrv_get_geometry(s->bs, &nb_sectors);
> +    s->nb_sectors = nb_sectors;
> +
> +    /* Update the identify data buffer. */
> +    if (s->drive_kind == IDE_CFATA) {
> +        ide_cfata_identify_size(s);
> +    } else {
> +        /* IDE_CD uses a different set of callbacks entirely. */
> +        assert(s->drive_kind != IDE_CD);
> +        ide_identify_size(s);
> +    }

If this happens while s->identify_set is still false, it's write-only.
Safe.

> +}
> +
>  static const BlockDevOps ide_cd_block_ops = {
>      .change_media_cb = ide_cd_change_cb,
>      .eject_request_cb = ide_cd_eject_request_cb,
> @@ -2106,6 +2150,10 @@ static const BlockDevOps ide_cd_block_ops = {
>      .is_medium_locked = ide_cd_is_medium_locked,
>  };
>  
> +static const BlockDevOps ide_hd_block_ops = {
> +    .resize_cb = ide_resize_cb,
> +};
> +
>  int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
>                     const char *version, const char *serial, const char 
> *model,
>                     uint64_t wwn,
> @@ -2142,6 +2190,7 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, 
> IDEDriveKind kind,
>              error_report("Can't use a read-only drive");
>              return -1;
>          }
> +        bdrv_set_dev_ops(bs, &ide_hd_block_ops, s);
>      }
>      if (serial) {
>          pstrcpy(s->drive_serial_str, sizeof(s->drive_serial_str), serial);

Reviewed-by: Markus Armbruster <address@hidden>