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

[Kevin Wolf <kwolf@redhat.com>]

Am 13.03.2012 14:01, schrieb 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: kwolf@redhat.com
>> Signed-off-by: Floris Bos <dev@noc-ps.com>
>> ---
>>  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?

Table in the spec says: "14 - Shall be set to one"

>> -    /* 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.

What about ide_cfata_identify, while we're at it? Or isn't it supported
there?

But both of them are more missing features than bugs. I'll defer to
Floris whether he wants to send another version.

Kevin