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

[Qemu-devel] [PATCH v3] 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>
---

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);
+}
+
 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;
 }
 
@@ -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 */
+}
+
 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:
@@ -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);
+    }
+}
+
 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);
-- 
1.9.3

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> 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 <jsnow@redhat.com>
> ---
>
> 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 <armbru@redhat.com>

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

[Stefan Hajnoczi]

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

On Thu, Aug 14, 2014 at 05:03:06PM -0400, John Snow wrote:
> @@ -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 */

These comments bitrot easily.  I'd prefer not to have them, but not a
reason to respin:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

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

[John Snow]

On 08/18/2014 09:12 AM, Stefan Hajnoczi wrote:
> On Thu, Aug 14, 2014 at 05:03:06PM -0400, John Snow wrote:
>> @@ -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 */
>
> These comments bitrot easily.  I'd prefer not to have them, but not a
> reason to respin:
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>

Ping? Respin w/o comments, or is this fine?

-- 
—js

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

[Stefan Hajnoczi]

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

On Tue, Aug 19, 2014 at 03:20:06PM -0400, John Snow wrote:
> 
> 
> On 08/18/2014 09:12 AM, Stefan Hajnoczi wrote:
> >On Thu, Aug 14, 2014 at 05:03:06PM -0400, John Snow wrote:
> >>@@ -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 */
> >
> >These comments bitrot easily.  I'd prefer not to have them, but not a
> >reason to respin:
> >
> >Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> >
> 
> Ping? Respin w/o comments, or is this fine?

I gave my Reviewed-by: and am not asking for a respin.

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

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

[Stefan Hajnoczi]

This patch seems to break tests/bios-tables-test.c:
ERROR:tests/bios-tables-test.c:744:test_acpi_one: assertion failed
(signature == SIGNATURE): (0x00000000 == 0x0000dead)
GTester: last random seed: R02S3d881198f35228a485b4c3d116dff3b1

I have run it many times to make sure the failure is deterministic and
I used git-bisect(1) to find this commit as the cause.

Not sure why but it seems to break the test.  Please take a look.

Dropped from the block branch.

Stefan

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

[John Snow]

On 09/04/2014 12:13 PM, Stefan Hajnoczi wrote:
> This patch seems to break tests/bios-tables-test.c:
> ERROR:tests/bios-tables-test.c:744:test_acpi_one: assertion failed
> (signature == SIGNATURE): (0x00000000 == 0x0000dead)
> GTester: last random seed: R02S3d881198f35228a485b4c3d116dff3b1
>
> I have run it many times to make sure the failure is deterministic and
> I used git-bisect(1) to find this commit as the cause.
>
> Not sure why but it seems to break the test.  Please take a look.
>
> Dropped from the block branch.
>
> Stefan
>

I've fixed it in a v4, but before I submit and while I'm at it, you 
didn't like the comments I left in the identify functions because they 
were "prone to bitrot." would you prefer I excised them entirely?

I found them helpful because it makes sense to read these functions 
alongside the identify data specs, and excising any references to those 
word indices in their natural order obfuscates the code needlessly.

My inclination is to leave them in.

Meanwhile, the bios-tables-test problem is such:
We serve the identify request out of the io_buffer, not the 
identify_data cache, thus for 2nd and subsequent requests for 
identify_data, we get correct size information, but for the 1st request 
to ide_identify, we get zeroes. I corrected this by making ide_identify 
and ide_atapi_identify mimic the flow of ide_cfata_identify, which is 
more clear about the nature of the two buffers.

Why this causes a failure here and for only the Q35 machine type I am 
not certain, but this is the causative bug.

--j

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 09/04/2014 12:13 PM, Stefan Hajnoczi wrote:
>> This patch seems to break tests/bios-tables-test.c:
>> ERROR:tests/bios-tables-test.c:744:test_acpi_one: assertion failed
>> (signature == SIGNATURE): (0x00000000 == 0x0000dead)
>> GTester: last random seed: R02S3d881198f35228a485b4c3d116dff3b1
>>
>> I have run it many times to make sure the failure is deterministic and
>> I used git-bisect(1) to find this commit as the cause.
>>
>> Not sure why but it seems to break the test.  Please take a look.
>>
>> Dropped from the block branch.
>>
>> Stefan
>>
>
> I've fixed it in a v4, but before I submit and while I'm at it, you
> didn't like the comments I left in the identify functions because they
> were "prone to bitrot." would you prefer I excised them entirely?
>
> I found them helpful because it makes sense to read these functions
> alongside the identify data specs, and excising any references to
> those word indices in their natural order obfuscates the code
> needlessly.
>
> My inclination is to leave them in.

I'd replace them by a pointer to the factored-out code.

> Meanwhile, the bios-tables-test problem is such:
> We serve the identify request out of the io_buffer, not the
> identify_data cache, thus for 2nd and subsequent requests for
> identify_data, we get correct size information, but for the 1st
> request to ide_identify, we get zeroes.

I found that part confusing and stared at it until I believed it works.
Looks like I believed too quickly.

>                                         I corrected this by making
> ide_identify and ide_atapi_identify mimic the flow of
> ide_cfata_identify, which is more clear about the nature of the two
> buffers.

Yup, that one's much better.

> Why this causes a failure here and for only the Q35 machine type I am
> not certain, but this is the causative bug.

My best guess is different firmware behavior.