[Qemu-devel] [PATCH 1/3] ide: Adds "model=s" qdev option, allowing the user to override the default disk model name "QEMU HARDDISK"

[Qemu-devel] [PATCH 1/3] ide: Adds "model=s" qdev option, allowing the user to override the default disk model name "QEMU HARDDISK"

[Floris Bos]

Some Linux distributions use the /dev/disk/by-id/scsi-SATA_name-of-disk-model_serial addressing scheme
when refering to partitions in /etc/fstab and elsewhere.
This causes problems when starting a disk image taken from an existing physical server under qemu,
because when running under qemu name-of-disk-model is always "QEMU HARDDISK"
This patch introduces a model=s option which in combination with the existing serial=s option can be used to
fake the disk the operating system was previously on, allowing the OS to boot properly.

Cc: kwolf@redhat.com
Signed-off-by: Floris Bos <dev@noc-ps.com>
---
 hw/ide/core.c     |   27 ++++++++++++++++++++++-----
 hw/ide/internal.h |    4 +++-
 hw/ide/qdev.c     |    6 ++++--
 3 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 4d568ac..c9a0e24 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -101,7 +101,7 @@ static void ide_identify(IDEState *s)
     put_le16(p + 21, 512); /* cache size in sectors */
     put_le16(p + 22, 4); /* ecc bytes */
     padstr((char *)(p + 23), s->version, 8); /* firmware version */
-    padstr((char *)(p + 27), "QEMU HARDDISK", 40); /* model */
+    padstr((char *)(p + 27), s->drive_model_str, 40); /* model */
 #if MAX_MULT_SECTORS > 1
     put_le16(p + 47, 0x8000 | MAX_MULT_SECTORS);
 #endif
@@ -189,7 +189,7 @@ static void ide_atapi_identify(IDEState *s)
     put_le16(p + 21, 512); /* cache size in sectors */
     put_le16(p + 22, 4); /* ecc bytes */
     padstr((char *)(p + 23), s->version, 8); /* firmware version */
-    padstr((char *)(p + 27), "QEMU DVD-ROM", 40); /* model */
+    padstr((char *)(p + 27), s->drive_model_str, 40); /* model */
     put_le16(p + 48, 1); /* dword I/O (XXX: should not be set on CDROM) */
 #ifdef USE_DMA_CDROM
     put_le16(p + 49, 1 << 9 | 1 << 8); /* DMA and LBA supported */
@@ -246,7 +246,7 @@ static void ide_cfata_identify(IDEState *s)
     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 */
-    padstr((char *) (p + 27), "QEMU MICRODRIVE", 40);/* Model number */
+    padstr((char *) (p + 27), s->drive_model_str, 40);/* Model number */
 #if MAX_MULT_SECTORS > 1
     put_le16(p + 47, 0x8000 | MAX_MULT_SECTORS);
 #else
@@ -1834,7 +1834,7 @@ static const BlockDevOps ide_cd_block_ops = {
 };
 
 int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
-                   const char *version, const char *serial)
+                   const char *version, const char *serial, const char *model)
 {
     int cylinders, heads, secs;
     uint64_t nb_sectors;
@@ -1885,6 +1885,22 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
         snprintf(s->drive_serial_str, sizeof(s->drive_serial_str),
                  "QM%05d", s->drive_serial);
     }
+    if (model) {
+        pstrcpy(s->drive_model_str, sizeof(s->drive_model_str), model);
+    } else {
+        switch (kind) {
+        case IDE_CD:
+            strcpy(s->drive_model_str, "QEMU DVD-ROM");
+            break;
+        case IDE_CFATA:
+            strcpy(s->drive_model_str, "QEMU MICRODRIVE");
+            break;
+        default:
+            strcpy(s->drive_model_str, "QEMU HARDDISK");
+            break;
+        }
+    }
+
     if (version) {
         pstrcpy(s->version, sizeof(s->version), version);
     } else {
@@ -1977,7 +1993,8 @@ void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
         if (dinfo) {
             if (ide_init_drive(&bus->ifs[i], dinfo->bdrv,
                                dinfo->media_cd ? IDE_CD : IDE_HD, NULL,
-                               *dinfo->serial ? dinfo->serial : NULL) < 0) {
+                               *dinfo->serial ? dinfo->serial : NULL,
+                               NULL) < 0) {
                 error_report("Can't set up IDE drive %s", dinfo->id);
                 exit(1);
             }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index c808a0d..b1319dc 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -348,6 +348,7 @@ struct IDEState {
     uint8_t identify_data[512];
     int drive_serial;
     char drive_serial_str[21];
+    char drive_model_str[41];
     /* ide regs */
     uint8_t feature;
     uint8_t error;
@@ -468,6 +469,7 @@ struct IDEDevice {
     BlockConf conf;
     char *version;
     char *serial;
+    char *model;
 };
 
 #define BM_STATUS_DMAING 0x01
@@ -534,7 +536,7 @@ void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
 uint32_t ide_data_readl(void *opaque, uint32_t addr);
 
 int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
-                   const char *version, const char *serial);
+                   const char *version, const char *serial, const char *model);
 void ide_init2(IDEBus *bus, qemu_irq irq);
 void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
                                     DriveInfo *hd1, qemu_irq irq);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index f6a4896..07227c7 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -136,7 +136,8 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
         }
     }
 
-    if (ide_init_drive(s, dev->conf.bs, kind, dev->version, serial) < 0) {
+    if (ide_init_drive(s, dev->conf.bs, kind,
+                       dev->version, serial, dev->model) < 0) {
         return -1;
     }
 
@@ -173,7 +174,8 @@ static int ide_drive_initfn(IDEDevice *dev)
 #define DEFINE_IDE_DEV_PROPERTIES()                     \
     DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),        \
     DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),  \
-    DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial)
+    DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial),\
+    DEFINE_PROP_STRING("model", IDEDrive, dev.model)
 
 static Property ide_hd_properties[] = {
     DEFINE_IDE_DEV_PROPERTIES(),
-- 
1.7.5.4

[Qemu-devel] [PATCH 2/3] Disk serial number string copy function: change strncpy() to pstrcpy()

[Floris Bos]

Cc: kwolf@redhat.com
Signed-off-by: Floris Bos <dev@noc-ps.com>
---
 blockdev.c    |    5 +++--
 hw/ide/core.c |    2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index d78aa51..e52449e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -532,8 +532,9 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
     dinfo->unit = unit_id;
     dinfo->opts = opts;
     dinfo->refcount = 1;
-    if (serial)
-        strncpy(dinfo->serial, serial, sizeof(dinfo->serial) - 1);
+    if (serial) {
+        pstrcpy(dinfo->serial, sizeof(dinfo->serial), serial);
+    }
     QTAILQ_INSERT_TAIL(&drives, dinfo, next);
 
     bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error);
diff --git a/hw/ide/core.c b/hw/ide/core.c
index c9a0e24..3e50c52 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1880,7 +1880,7 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
         }
     }
     if (serial) {
-        strncpy(s->drive_serial_str, serial, sizeof(s->drive_serial_str));
+        pstrcpy(s->drive_serial_str, sizeof(s->drive_serial_str), serial);
     } else {
         snprintf(s->drive_serial_str, sizeof(s->drive_serial_str),
                  "QM%05d", s->drive_serial);
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH 2/3] Disk serial number string copy function: change strncpy() to pstrcpy()

[Markus Armbruster]

Floris Bos <bos@je-eigen-domein.nl> writes:

> Cc: kwolf@redhat.com
> Signed-off-by: Floris Bos <dev@noc-ps.com>
> ---
>  blockdev.c    |    5 +++--
>  hw/ide/core.c |    2 +-
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index d78aa51..e52449e 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -532,8 +532,9 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>      dinfo->unit = unit_id;
>      dinfo->opts = opts;
>      dinfo->refcount = 1;
> -    if (serial)
> -        strncpy(dinfo->serial, serial, sizeof(dinfo->serial) - 1);
> +    if (serial) {
> +        pstrcpy(dinfo->serial, sizeof(dinfo->serial), serial);
> +    }
>      QTAILQ_INSERT_TAIL(&drives, dinfo, next);
>  
>      bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error);

The old code works because dinfo->serial[]'s last byte is set to zero by
g_malloc0().  The new code achieves the same result in a less subtle
manner.

> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index c9a0e24..3e50c52 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1880,7 +1880,7 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
>          }
>      }
>      if (serial) {
> -        strncpy(s->drive_serial_str, serial, sizeof(s->drive_serial_str));
> +        pstrcpy(s->drive_serial_str, sizeof(s->drive_serial_str), serial);
>      } else {
>          snprintf(s->drive_serial_str, sizeof(s->drive_serial_str),
>                   "QM%05d", s->drive_serial);

This actually fixes a latent bug.  The old code fails to zero-terminate
s->drive_serial_str when serial is longer than 21 characters.
ide_identify(), ide_atapi_identify() and ide_cfata_identify() don't
care, but ide_dev_initfn() passes it to g_strdup().  Fortunately, we
reach that only when !dev->serial, and then the value copied into
s->drive_serial_str[] comes from DriveInfo member serial[], which has
space for just 20 characters plus terminating zero.

Thanks!

Two down, 34 uses of strncpy() left in the code.

[Qemu-devel] [PATCH 3/3] ide: Adds wwn=hex qdev option allowing the user to specify a disk's World Wide Name

[Floris Bos]

Linux guests can address disks by their unique World Wide Name number (e.g. /dev/disk/by-id/wwn-0x5001517959123522)
This patch adds support for assigning a World Wide Name number to a virtual IDE disk.

Cc: kwolf@redhat.com
Signed-off-by: Floris Bos <dev@noc-ps.com>
---
 hw/ide/core.c     |   13 +++++++++++--
 hw/ide/internal.h |    5 ++++-
 hw/ide/qdev.c     |    3 ++-
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 3e50c52..b48e5c2 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -166,6 +166,13 @@ static void ide_identify(IDEState *s)
     if (dev && dev->conf.discard_granularity) {
         put_le16(p + 169, 1); /* TRIM support */
     }
+
+    if (s->wwn) {
+        put_le16(p + 108, s->wwn >> 48);
+        put_le16(p + 109, s->wwn >> 32);
+        put_le16(p + 110, s->wwn >> 16);
+        put_le16(p + 111, s->wwn);
+    }
 
     memcpy(s->identify_data, p, sizeof(s->identify_data));
     s->identify_set = 1;
@@ -1834,7 +1841,8 @@ static const BlockDevOps ide_cd_block_ops = {
 };
 
 int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
-                   const char *version, const char *serial, const char *model)
+                   const char *version, const char *serial, const char *model,
+                   uint64_t wwn)
 {
     int cylinders, heads, secs;
     uint64_t nb_sectors;
@@ -1860,6 +1868,7 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
     s->heads = heads;
     s->sectors = secs;
     s->nb_sectors = nb_sectors;
+    s->wwn = wwn;
     /* The SMART values should be preserved across power cycles
        but they aren't.  */
     s->smart_enabled = 1;
@@ -1994,7 +2003,7 @@ void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
             if (ide_init_drive(&bus->ifs[i], dinfo->bdrv,
                                dinfo->media_cd ? IDE_CD : IDE_HD, NULL,
                                *dinfo->serial ? dinfo->serial : NULL,
-                               NULL) < 0) {
+                               NULL, 0) < 0) {
                 error_report("Can't set up IDE drive %s", dinfo->id);
                 exit(1);
             }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index b1319dc..100efd3 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -349,6 +349,7 @@ struct IDEState {
     int drive_serial;
     char drive_serial_str[21];
     char drive_model_str[41];
+    uint64_t wwn;
     /* ide regs */
     uint8_t feature;
     uint8_t error;
@@ -470,6 +471,7 @@ struct IDEDevice {
     char *version;
     char *serial;
     char *model;
+    uint64_t wwn;
 };
 
 #define BM_STATUS_DMAING 0x01
@@ -536,7 +538,8 @@ void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
 uint32_t ide_data_readl(void *opaque, uint32_t addr);
 
 int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
-                   const char *version, const char *serial, const char *model);
+                   const char *version, const char *serial, const char *model,
+                   uint64_t wwn);
 void ide_init2(IDEBus *bus, qemu_irq irq);
 void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
                                     DriveInfo *hd1, qemu_irq irq);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 07227c7..a46578d 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -137,7 +137,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
     }
 
     if (ide_init_drive(s, dev->conf.bs, kind,
-                       dev->version, serial, dev->model) < 0) {
+                       dev->version, serial, dev->model, dev->wwn) < 0) {
         return -1;
     }
 
@@ -174,6 +174,7 @@ static int ide_drive_initfn(IDEDevice *dev)
 #define DEFINE_IDE_DEV_PROPERTIES()                     \
     DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),        \
     DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),  \
+    DEFINE_PROP_HEX64("wwn",  IDEDrive, dev.wwn, 0),    \
     DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial),\
     DEFINE_PROP_STRING("model", IDEDrive, dev.model)
 
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH 3/3] ide: Adds wwn=hex qdev option allowing the user to specify a disk's World Wide Name

[Stefan Hajnoczi]

On Mon, Mar 12, 2012 at 8:05 PM, Floris Bos <bos@je-eigen-domein.nl> wrote:
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 3e50c52..b48e5c2 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -166,6 +166,13 @@ static void ide_identify(IDEState *s)
>     if (dev && dev->conf.discard_granularity) {
>         put_le16(p + 169, 1); /* TRIM support */
>     }
> +
> +    if (s->wwn) {
> +        put_le16(p + 108, s->wwn >> 48);
> +        put_le16(p + 109, s->wwn >> 32);
> +        put_le16(p + 110, s->wwn >> 16);
> +        put_le16(p + 111, s->wwn);
> +    }

Little-endian 16-bit seems weird at first but the spec requires it, so
it's fine.  A comment would be nice.

ATA8-ACS says:

"Bit 8 of word 84 shall be set to one indicating the mandatory World
Wide Name in words 108-111 is supported."

I think we're missing this.

Re: [Qemu-devel] [PATCH 3/3] ide: Adds wwn=hex qdev option allowing the user to specify a disk's World Wide Name

[Kevin Wolf]

Am 13.03.2012 11:29, schrieb Stefan Hajnoczi:
> On Mon, Mar 12, 2012 at 8:05 PM, Floris Bos <bos@je-eigen-domein.nl> wrote:
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 3e50c52..b48e5c2 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -166,6 +166,13 @@ static void ide_identify(IDEState *s)
>>     if (dev && dev->conf.discard_granularity) {
>>         put_le16(p + 169, 1); /* TRIM support */
>>     }
>> +
>> +    if (s->wwn) {
>> +        put_le16(p + 108, s->wwn >> 48);
>> +        put_le16(p + 109, s->wwn >> 32);
>> +        put_le16(p + 110, s->wwn >> 16);
>> +        put_le16(p + 111, s->wwn);
>> +    }
> 
> Little-endian 16-bit seems weird at first but the spec requires it, so
> it's fine.  A comment would be nice.
> 
> ATA8-ACS says:
> 
> "Bit 8 of word 84 shall be set to one indicating the mandatory World
> Wide Name in words 108-111 is supported."
> 
> I think we're missing this.

And this:

"Bit 8 of word 87 is a copy of word 84 bit 8."

Otherwise the series looks good to me.

Kevin

Re: [Qemu-devel] [PATCH 3/3] ide: Adds wwn=hex qdev option allowing the user to specify a disk's World Wide Name

[Floris Bos / Maxnet]

On 03/13/2012 01:06 PM, Kevin Wolf wrote:
> Am 13.03.2012 11:29, schrieb Stefan Hajnoczi:
>> On Mon, Mar 12, 2012 at 8:05 PM, Floris Bos<bos@je-eigen-domein.nl>  wrote:
>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>> index 3e50c52..b48e5c2 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -166,6 +166,13 @@ static void ide_identify(IDEState *s)
>>>      if (dev&&  dev->conf.discard_granularity) {
>>>          put_le16(p + 169, 1); /* TRIM support */
>>>      }
>>> +
>>> +    if (s->wwn) {
>>> +        put_le16(p + 108, s->wwn>>  48);
>>> +        put_le16(p + 109, s->wwn>>  32);
>>> +        put_le16(p + 110, s->wwn>>  16);
>>> +        put_le16(p + 111, s->wwn);
>>> +    }
>> Little-endian 16-bit seems weird at first but the spec requires it, so
>> it's fine.  A comment would be nice.
>>
>> ATA8-ACS says:
>>
>> "Bit 8 of word 84 shall be set to one indicating the mandatory World
>> Wide Name in words 108-111 is supported."
>>
>> I think we're missing this.
> And this:
>
> "Bit 8 of word 87 is a copy of word 84 bit 8."
>
> Otherwise the series looks good to me.
>
> Kevin

Oops, you are right, overlooked those bits.
Linux only seems to care that the WWN starts with a 5, and doesn't look 
at the bits, so it didn't turn up in my testing either.
Will send a fixed patch.

Yours sincerely,

Floris Bos

Re: [Qemu-devel] [PATCH 1/3] ide: Adds "model=s" qdev option, allowing the user to override the default disk model name "QEMU HARDDISK"

[Stefan Hajnoczi]

On Mon, Mar 12, 2012 at 8:05 PM, Floris Bos <bos@je-eigen-domein.nl> wrote:
> Some Linux distributions use the /dev/disk/by-id/scsi-SATA_name-of-disk-model_serial addressing scheme
> when refering to partitions in /etc/fstab and elsewhere.
> This causes problems when starting a disk image taken from an existing physical server under qemu,
> because when running under qemu name-of-disk-model is always "QEMU HARDDISK"
> This patch introduces a model=s option which in combination with the existing serial=s option can be used to
> fake the disk the operating system was previously on, allowing the OS to boot properly.
>
> Cc: kwolf@redhat.com
> Signed-off-by: Floris Bos <dev@noc-ps.com>
> ---
>  hw/ide/core.c     |   27 ++++++++++++++++++++++-----
>  hw/ide/internal.h |    4 +++-
>  hw/ide/qdev.c     |    6 ++++--
>  3 files changed, 29 insertions(+), 8 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>