[Qemu-devel] [PATCH v4 0/2] add resize callback to ide/core

[Qemu-devel] [PATCH v4 0/2] add resize callback to ide/core

[John Snow]

This patch series fixes incorrect IDENTIFY data returned
for an IDE drive after a block_resize event by adding
a resize callback for IDE devices.

Inconsistencies between identify routines are also
removed so that they read easier.

V4:
 - Added patch that makes the buffer and cache fill order for
   identify routines more consistent.
 - Fixed a bug where the very first call to IDENTIFY does not
   return any size information.

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.

John Snow (2):
  IDE: Fill the IDENTIFY request consistently
  ide: Add resize callback to ide/core

 hw/ide/core.c | 97 +++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 74 insertions(+), 23 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v4 1/2] IDE: Fill the IDENTIFY request consistently

[John Snow]

IDE-HD, IDE-ATAPI and IDE-CFATA all fill the
identify buffer in slightly different ways,
this is a relatively minor patch to make them
uniform, to emphasize that:

(1) We build the s->identify_data cache first, then
(2) We copy it to s->io_buffer to fulfill the request.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/core.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index b48127f..f5b1ef2 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -81,13 +81,12 @@ static void ide_identify(IDEState *s)
     unsigned int oldsize;
     IDEDevice *dev = s->unit ? s->bus->slave : s->bus->master;
 
+    p = (uint16_t *)s->identify_data;
     if (s->identify_set) {
-	memcpy(s->io_buffer, s->identify_data, sizeof(s->identify_data));
-	return;
+        goto fill_buffer;
     }
+    memset(p, 0, sizeof(s->identify_data));
 
-    memset(s->io_buffer, 0, 512);
-    p = (uint16_t *)s->io_buffer;
     put_le16(p + 0, 0x0040);
     put_le16(p + 1, s->cylinders);
     put_le16(p + 3, s->heads);
@@ -180,21 +179,22 @@ static void ide_identify(IDEState *s)
         put_le16(p + 169, 1); /* TRIM support */
     }
 
-    memcpy(s->identify_data, p, sizeof(s->identify_data));
     s->identify_set = 1;
+
+fill_buffer:
+    memcpy(s->io_buffer, p, sizeof(s->identify_data));
 }
 
 static void ide_atapi_identify(IDEState *s)
 {
     uint16_t *p;
 
+    p = (uint16_t *)s->identify_data;
     if (s->identify_set) {
-	memcpy(s->io_buffer, s->identify_data, sizeof(s->identify_data));
-	return;
+        goto fill_buffer;
     }
+    memset(p, 0, sizeof(s->identify_data));
 
-    memset(s->io_buffer, 0, 512);
-    p = (uint16_t *)s->io_buffer;
     /* Removable CDROM, 50us response, 12 byte packets */
     put_le16(p + 0, (2 << 14) | (5 << 8) | (1 << 7) | (2 << 5) | (0 << 0));
     padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
@@ -233,8 +233,10 @@ static void ide_atapi_identify(IDEState *s)
 #ifdef USE_DMA_CDROM
     put_le16(p + 88, 0x3f | (1 << 13)); /* udma5 set and supported */
 #endif
-    memcpy(s->identify_data, p, sizeof(s->identify_data));
     s->identify_set = 1;
+
+fill_buffer:
+    memcpy(s->io_buffer, p, sizeof(s->identify_data));
 }
 
 static void ide_cfata_identify(IDEState *s)
@@ -242,10 +244,10 @@ static void ide_cfata_identify(IDEState *s)
     uint16_t *p;
     uint32_t cur_sec;
 
-    p = (uint16_t *) s->identify_data;
-    if (s->identify_set)
+    p = (uint16_t *)s->identify_data;
+    if (s->identify_set) {
         goto fill_buffer;
-
+    }
     memset(p, 0, sizeof(s->identify_data));
 
     cur_sec = s->cylinders * s->heads * s->sectors;
-- 
1.9.3

[Qemu-devel] [PATCH v4 2/2] ide: Add resize callback to ide/core

[John Snow]

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 <jsnow@redhat.com>
---
 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 f5b1ef2..f077c0f 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);
+}
+
 static void ide_identify(IDEState *s)
 {
     uint16_t *p;
@@ -115,8 +126,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 */
@@ -161,10 +172,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));
@@ -179,6 +190,7 @@ static void ide_identify(IDEState *s)
         put_le16(p + 169, 1); /* TRIM support */
     }
 
+    ide_identify_size(s);
     s->identify_set = 1;
 
 fill_buffer:
@@ -239,6 +251,15 @@ fill_buffer:
     memcpy(s->io_buffer, p, sizeof(s->identify_data));
 }
 
+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 */
+}
+
 static void ide_cfata_identify(IDEState *s)
 {
     uint16_t *p;
@@ -256,8 +277,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 */
@@ -278,8 +299,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 */
@@ -299,6 +320,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:
@@ -2117,6 +2139,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);
+    }
+}
+
 static const BlockDevOps ide_cd_block_ops = {
     .change_media_cb = ide_cd_change_cb,
     .eject_request_cb = ide_cd_eject_request_cb,
@@ -2124,6 +2168,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,
@@ -2160,6 +2208,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);
-- 
1.9.3

Re: [Qemu-devel] [PATCH v4 0/2] add resize callback to ide/core

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]

On Thu, Sep 04, 2014 at 11:42:15PM -0400, John Snow wrote:
> This patch series fixes incorrect IDENTIFY data returned
> for an IDE drive after a block_resize event by adding
> a resize callback for IDE devices.
> 
> Inconsistencies between identify routines are also
> removed so that they read easier.
> 
> V4:
>  - Added patch that makes the buffer and cache fill order for
>    identify routines more consistent.
>  - Fixed a bug where the very first call to IDENTIFY does not
>    return any size information.
> 
> 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.
> 
> John Snow (2):
>   IDE: Fill the IDENTIFY request consistently
>   ide: Add resize callback to ide/core
> 
>  hw/ide/core.c | 97 +++++++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 74 insertions(+), 23 deletions(-)
> 
> -- 
> 1.9.3
> 
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]