Re: [Qemu-devel] [PATCH] scsi-disk: Add support for port WWN and index descriptors in VPD page 83h

Re: [Qemu-devel] [PATCH] scsi-disk: Add support for port WWN and index descriptors in VPD page 83h

[Roland Dreier]

On Thu, Feb 20, 2014 at 7:35 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Looks like _ is more common than - for device properties:
>
> $ git grep DEFINE_PROP_.*\(\".*_.*\" | wc -l
> 132
> $ git grep DEFINE_PROP_.*\(\".*-.*\" | wc -l
> 77

And more locally, "scsi-id" in scsi-bus.c is the only property in
hw/scsi with a '-'.  scsi-disk.c already has "max_unmap_size" and
megasas.c has several '_' already.

With that said I'm fine either way, and if you want to munge my patch
to use '-' that's perfectly OK with me.  Or I'm happy to resubmit in
the other form.

 - R.

Re: [Qemu-devel] [PATCH] scsi-disk: Add support for port WWN and index descriptors in VPD page 83h

[Paolo Bonzini]

Il 20/02/2014 18:14, Roland Dreier ha scritto:
>> > Looks like _ is more common than - for device properties:
>> >
>> > $ git grep DEFINE_PROP_.*\(\".*_.*\" | wc -l
>> > 132
>> > $ git grep DEFINE_PROP_.*\(\".*-.*\" | wc -l
>> > 77
> And more locally, "scsi-id" in scsi-bus.c is the only property in
> hw/scsi with a '-'.  scsi-disk.c already has "max_unmap_size" and
> megasas.c has several '_' already.
>
> With that said I'm fine either way, and if you want to munge my patch
> to use '-' that's perfectly OK with me.  Or I'm happy to resubmit in
> the other form.

No, your patch is fine.  We'll sort out what to do with underscores and 
dashes, looks like there is a plan...

Thanks for your contribution to QEMU!

Paolo

[Qemu-devel] [PATCH] scsi-disk: Add support for port WWN and index descriptors in VPD page 83h

[Roland Dreier]

From: Roland Dreier <roland@purestorage.com>

To make a VM more convincing to my application, it's useful to be able
to add a port WWN and relative target port index to the descriptors
returned for VPD page 83h.  Add device properties to allow setting
these, and return them from INQUIRY commands.

Signed-off-by: Roland Dreier <roland@purestorage.com>
---
 hw/scsi/scsi-disk.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index a8d0f15ebe61..12013a6768a1 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -75,6 +75,8 @@ struct SCSIDiskState
     bool media_event;
     bool eject_request;
     uint64_t wwn;
+    uint64_t port_wwn;
+    uint16_t port_index;
     uint64_t max_unmap_size;
     QEMUBH *bh;
     char *version;
@@ -617,6 +619,24 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
                 stq_be_p(&outbuf[buflen], s->wwn);
                 buflen += 8;
             }
+
+            if (s->port_wwn) {
+                outbuf[buflen++] = 0x61; // SAS / Binary
+                outbuf[buflen++] = 0x93; // PIV / Target port / NAA
+                outbuf[buflen++] = 0;    // reserved
+                outbuf[buflen++] = 8;
+                stq_be_p(&outbuf[buflen], s->port_wwn);
+                buflen += 8;
+            }
+
+            if (s->port_index) {
+                outbuf[buflen++] = 0x61; // SAS / Binary
+                outbuf[buflen++] = 0x94; // PIV / Target port / relative target port
+                outbuf[buflen++] = 0;    // reserved
+                outbuf[buflen++] = 4;
+                stw_be_p(&outbuf[buflen + 2], s->port_index);
+                buflen += 4;
+            }
             break;
         }
         case 0xb0: /* block limits */
@@ -2536,6 +2556,8 @@ static Property scsi_hd_properties[] = {
     DEFINE_PROP_BIT("dpofua", SCSIDiskState, features,
                     SCSI_DISK_F_DPOFUA, false),
     DEFINE_PROP_HEX64("wwn", SCSIDiskState, wwn, 0),
+    DEFINE_PROP_HEX64("port_wwn", SCSIDiskState, port_wwn, 0),
+    DEFINE_PROP_UINT16("port_index", SCSIDiskState, port_index, 0),
     DEFINE_PROP_UINT64("max_unmap_size", SCSIDiskState, max_unmap_size,
                        DEFAULT_MAX_UNMAP_SIZE),
     DEFINE_BLOCK_CHS_PROPERTIES(SCSIDiskState, qdev.conf),
@@ -2584,6 +2606,8 @@ static const TypeInfo scsi_hd_info = {
 static Property scsi_cd_properties[] = {
     DEFINE_SCSI_DISK_PROPERTIES(),
     DEFINE_PROP_HEX64("wwn", SCSIDiskState, wwn, 0),
+    DEFINE_PROP_HEX64("port_wwn", SCSIDiskState, port_wwn, 0),
+    DEFINE_PROP_UINT16("port_index", SCSIDiskState, port_index, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2647,6 +2671,8 @@ static Property scsi_disk_properties[] = {
     DEFINE_PROP_BIT("dpofua", SCSIDiskState, features,
                     SCSI_DISK_F_DPOFUA, false),
     DEFINE_PROP_HEX64("wwn", SCSIDiskState, wwn, 0),
+    DEFINE_PROP_HEX64("port_wwn", SCSIDiskState, port_wwn, 0),
+    DEFINE_PROP_UINT16("port_index", SCSIDiskState, port_index, 0),
     DEFINE_PROP_UINT64("max_unmap_size", SCSIDiskState, max_unmap_size,
                        DEFAULT_MAX_UNMAP_SIZE),
     DEFINE_PROP_END_OF_LIST(),
-- 
1.9.0

Re: [Qemu-devel] [PATCH] scsi-disk: Add support for port WWN and index descriptors in VPD page 83h

[Paolo Bonzini]

Il 19/02/2014 17:28, Roland Dreier ha scritto:
> From: Roland Dreier <roland@purestorage.com>
>
> To make a VM more convincing to my application, it's useful to be able
> to add a port WWN and relative target port index to the descriptors
> returned for VPD page 83h.  Add device properties to allow setting
> these, and return them from INQUIRY commands.
>
> Signed-off-by: Roland Dreier <roland@purestorage.com>

I'm applying to scsi-next, thanks.

Paolo

> ---
>  hw/scsi/scsi-disk.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index a8d0f15ebe61..12013a6768a1 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -75,6 +75,8 @@ struct SCSIDiskState
>      bool media_event;
>      bool eject_request;
>      uint64_t wwn;
> +    uint64_t port_wwn;
> +    uint16_t port_index;
>      uint64_t max_unmap_size;
>      QEMUBH *bh;
>      char *version;
> @@ -617,6 +619,24 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
>                  stq_be_p(&outbuf[buflen], s->wwn);
>                  buflen += 8;
>              }
> +
> +            if (s->port_wwn) {
> +                outbuf[buflen++] = 0x61; // SAS / Binary
> +                outbuf[buflen++] = 0x93; // PIV / Target port / NAA
> +                outbuf[buflen++] = 0;    // reserved
> +                outbuf[buflen++] = 8;
> +                stq_be_p(&outbuf[buflen], s->port_wwn);
> +                buflen += 8;
> +            }
> +
> +            if (s->port_index) {
> +                outbuf[buflen++] = 0x61; // SAS / Binary
> +                outbuf[buflen++] = 0x94; // PIV / Target port / relative target port
> +                outbuf[buflen++] = 0;    // reserved
> +                outbuf[buflen++] = 4;
> +                stw_be_p(&outbuf[buflen + 2], s->port_index);
> +                buflen += 4;
> +            }
>              break;
>          }
>          case 0xb0: /* block limits */
> @@ -2536,6 +2556,8 @@ static Property scsi_hd_properties[] = {
>      DEFINE_PROP_BIT("dpofua", SCSIDiskState, features,
>                      SCSI_DISK_F_DPOFUA, false),
>      DEFINE_PROP_HEX64("wwn", SCSIDiskState, wwn, 0),
> +    DEFINE_PROP_HEX64("port_wwn", SCSIDiskState, port_wwn, 0),
> +    DEFINE_PROP_UINT16("port_index", SCSIDiskState, port_index, 0),
>      DEFINE_PROP_UINT64("max_unmap_size", SCSIDiskState, max_unmap_size,
>                         DEFAULT_MAX_UNMAP_SIZE),
>      DEFINE_BLOCK_CHS_PROPERTIES(SCSIDiskState, qdev.conf),
> @@ -2584,6 +2606,8 @@ static const TypeInfo scsi_hd_info = {
>  static Property scsi_cd_properties[] = {
>      DEFINE_SCSI_DISK_PROPERTIES(),
>      DEFINE_PROP_HEX64("wwn", SCSIDiskState, wwn, 0),
> +    DEFINE_PROP_HEX64("port_wwn", SCSIDiskState, port_wwn, 0),
> +    DEFINE_PROP_UINT16("port_index", SCSIDiskState, port_index, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> @@ -2647,6 +2671,8 @@ static Property scsi_disk_properties[] = {
>      DEFINE_PROP_BIT("dpofua", SCSIDiskState, features,
>                      SCSI_DISK_F_DPOFUA, false),
>      DEFINE_PROP_HEX64("wwn", SCSIDiskState, wwn, 0),
> +    DEFINE_PROP_HEX64("port_wwn", SCSIDiskState, port_wwn, 0),
> +    DEFINE_PROP_UINT16("port_index", SCSIDiskState, port_index, 0),
>      DEFINE_PROP_UINT64("max_unmap_size", SCSIDiskState, max_unmap_size,
>                         DEFAULT_MAX_UNMAP_SIZE),
>      DEFINE_PROP_END_OF_LIST(),
>

Re: [Qemu-devel] [PATCH] scsi-disk: Add support for port WWN and index descriptors in VPD page 83h

[Andreas Färber]

Am 19.02.2014 18:07, schrieb Paolo Bonzini:
> Il 19/02/2014 17:28, Roland Dreier ha scritto:
>> From: Roland Dreier <roland@purestorage.com>
>>
>> To make a VM more convincing to my application, it's useful to be able
>> to add a port WWN and relative target port index to the descriptors
>> returned for VPD page 83h.  Add device properties to allow setting
>> these, and return them from INQUIRY commands.
>>
>> Signed-off-by: Roland Dreier <roland@purestorage.com>
> 
> I'm applying to scsi-next, thanks.

HEX64 will conflict with your patches in the pending pull.

Also I notice that underscores are being used in new properties -
oversight or intentional?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] scsi-disk: Add support for port WWN and index descriptors in VPD page 83h

[Roland Dreier]

On Wed, Feb 19, 2014 at 10:11 AM, Andreas Färber <afaerber@suse.de> wrote:
> HEX64 will conflict with your patches in the pending pull.

I'm not aware of the issue.  Is there a better tree for me to work
against than qemu.git master?

> Also I notice that underscores are being used in new properties -
> oversight or intentional?

Forgive me, but I'm not that famliar with the customs for naming
device properties in QEMU.  What would a better choice be?

Thanks,
  Roland

Re: [Qemu-devel] [PATCH] scsi-disk: Add support for port WWN and index descriptors in VPD page 83h

[Paolo Bonzini]

Il 19/02/2014 19:18, Roland Dreier ha scritto:
> On Wed, Feb 19, 2014 at 10:11 AM, Andreas Färber <afaerber@suse.de> wrote:
>> > HEX64 will conflict with your patches in the pending pull.
> I'm not aware of the issue.  Is there a better tree for me to work
> against than qemu.git master?

No, I'll take care of rebasing and won't send a pull request until 
Andreas's pending pull request is in.

>> > Also I notice that underscores are being used in new properties -
>> > oversight or intentional?
> Forgive me, but I'm not that famliar with the customs for naming
> device properties in QEMU.  What would a better choice be?

Oversight, but it convinces me more that we should just match [_-] also 
against the other in the pair.  Must find time to write that patch...

Paolo

Re: [Qemu-devel] [PATCH] scsi-disk: Add support for port WWN and index descriptors in VPD page 83h

[Paolo Bonzini]

Il 19/02/2014 23:09, Paolo Bonzini ha scritto:
> Oversight, but it convinces me more that we should just match [_-] also
> against the other in the pair.  Must find time to write that patch...

Looks like _ is more common than - for device properties:

$ git grep DEFINE_PROP_.*\(\".*_.*\" | wc -l
132
$ git grep DEFINE_PROP_.*\(\".*-.*\" | wc -l
77

Paolo

Re: [Qemu-devel] [PATCH] scsi-disk: Add support for port WWN and index descriptors in VPD page 83h

[Andreas Färber]

Am 20.02.2014 16:35, schrieb Paolo Bonzini:
> Il 19/02/2014 23:09, Paolo Bonzini ha scritto:
>> Oversight, but it convinces me more that we should just match [_-] also
>> against the other in the pair.  Must find time to write that patch...
> 
> Looks like _ is more common than - for device properties:
> 
> $ git grep DEFINE_PROP_.*\(\".*_.*\" | wc -l
> 132
> $ git grep DEFINE_PROP_.*\(\".*-.*\" | wc -l
> 77

Not unexpected, it's the older; the - convention was introduced possibly
with QOM around start of 2012. Or at least there it's been enforced, and
by my understanding of QOM and QMP visibility it then applies to devices
as well.

Regarding QMP, I consider it smarter to do the _ -> - matching at
QemuOpts level than somewhere inside QOM.

For -cpu we have such compatibility code (although non-QemuOpts) in
target-i386/cpu.c, converting all underscores. Unfortunately that won't
work as long as there are underscores in old properties. Maybe you have
some cool patch idea?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] scsi-disk: Add support for port WWN and index descriptors in VPD page 83h

[Paolo Bonzini]

Il 20/02/2014 17:02, Andreas Färber ha scritto:
> Not unexpected, it's the older; the - convention was introduced possibly
> with QOM around start of 2012. Or at least there it's been enforced, and
> by my understanding of QOM and QMP visibility it then applies to devices
> as well.
>
> Regarding QMP, I consider it smarter to do the _ -> - matching at
> QemuOpts level than somewhere inside QOM.

That's fine, because device_add uses QemuOpts internally.

> For -cpu we have such compatibility code (although non-QemuOpts) in
> target-i386/cpu.c, converting all underscores. Unfortunately that won't
> work as long as there are underscores in old properties. Maybe you have
> some cool patch idea?

Well, no cool idea except "once conversion is done at the QemuOpts 
level, do a full sweep of s/_/-/".  This should be done before we'll be 
able to create devices with object-add.  I think as long as we have a 
plan, consistency trumps design-by-committee convention.

Paolo

Re: [Qemu-devel] [PATCH] scsi-disk: Add support for port WWN and index descriptors in VPD page 83h

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 20/02/2014 17:02, Andreas Färber ha scritto:
>> Not unexpected, it's the older; the - convention was introduced possibly
>> with QOM around start of 2012. Or at least there it's been enforced, and

QMP preferred '-' from the start, but insufficient decoupling from
command line and HMP let in many '_', and the bad examples then got
copied around, as usual.

>> by my understanding of QOM and QMP visibility it then applies to devices
>> as well.
>>
>> Regarding QMP, I consider it smarter to do the _ -> - matching at
>> QemuOpts level than somewhere inside QOM.
>
> That's fine, because device_add uses QemuOpts internally.
>
>> For -cpu we have such compatibility code (although non-QemuOpts) in
>> target-i386/cpu.c, converting all underscores. Unfortunately that won't
>> work as long as there are underscores in old properties. Maybe you have
>> some cool patch idea?
>
> Well, no cool idea except "once conversion is done at the QemuOpts
> level, do a full sweep of s/_/-/".

Required anyway, to get the help texts consistent, and to get rid of the
bad examples.

>                                     This should be done before we'll
> be able to create devices with object-add.  I think as long as we have
> a plan, consistency trumps design-by-committee convention.