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

[Paolo Bonzini]

Il 13/03/2012 13:31, Floris Bos ha scritto:
> 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: address@hidden
> Signed-off-by: Floris Bos <address@hidden>
> ---
>  hw/ide/core.c     |   29 +++++++++++++++++++++++------
>  hw/ide/internal.h |    5 ++++-
>  hw/ide/qdev.c     |    3 ++-
>  3 files changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 3e50c52..79b4f69 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -143,8 +143,12 @@ static void ide_identify(IDEState *s)
>      put_le16(p + 82, (1 << 14) | (1 << 5) | 1);
>      /* 13=flush_cache_ext,12=flush_cache,10=lba48 */
>      put_le16(p + 83, (1 << 14) | (1 << 13) | (1 <<12) | (1 << 10));

Bit 14 should not be set here.  Not your fault, but perhaps you can fix
it too?

> -    /* 14=set to 1, 1=SMART self test, 0=SMART error logging */
> -    put_le16(p + 84, (1 << 14) | 0);
> +    /* 14=set to 1, 8=has WWN, 1=SMART self test, 0=SMART error logging */
> +    if (s->wwn) {
> +        put_le16(p + 84, (1 << 14) | (1 << 8) | 0);
> +    } else {
> +        put_le16(p + 84, (1 << 14) | 0);
> +    }
>      /* 14 = NOP supported, 5=WCACHE enabled, 0=SMART feature set enabled */
>      if (bdrv_enable_write_cache(s->bs))
>           put_le16(p + 85, (1 << 14) | (1 << 5) | 1);
> @@ -152,8 +156,12 @@ static void ide_identify(IDEState *s)
>           put_le16(p + 85, (1 << 14) | 1);
>      /* 13=flush_cache_ext,12=flush_cache,10=lba48 */
>      put_le16(p + 86, (1 << 14) | (1 << 13) | (1 <<12) | (1 << 10));
> -    /* 14=set to 1, 1=smart self test, 0=smart error logging */
> -    put_le16(p + 87, (1 << 14) | 0);
> +    /* 14=set to 1, 8=has WWN, 1=SMART self test, 0=SMART error logging */
> +    if (s->wwn) {
> +        put_le16(p + 87, (1 << 14) | (1 << 8) | 0);
> +    } else {
> +        put_le16(p + 87, (1 << 14) | 0);
> +    }
>      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);
> @@ -163,6 +171,13 @@ static void ide_identify(IDEState *s)
>  
>      if (dev && dev->conf.physical_block_size)
>          put_le16(p + 106, 0x6000 | get_physical_block_exp(&dev->conf));
> +    if (s->wwn) {
> +        /* LE 16-bit words 111-108 contain 64-bit World Wide Name */
> +        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);
> +    }
>      if (dev && dev->conf.discard_granularity) {
>          put_le16(p + 169, 1); /* TRIM support */
>      }

Words 84, 87, 108-111 should also be set in the ide_atapi_identify routine:

- 84 and 87 to 0x4000 if no WWN, 0x4100 if WWN provided

- 108..111 to the WWN in the same awkward-endian layout.

Otherwise looks good.

Paolo

> @@ -1834,7 +1849,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 +1876,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 +2011,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)
>