[Qemu-devel] [PATCH 0/4] vhost-scsi: support to assign boot order

[Qemu-devel] [PATCH 0/4] vhost-scsi: support to assign boot order

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Qemu haven't provide a bootindex property for vhost-scsi device.
So, we can not assign the boot order for it at present. But
Some clients/users have requirements for that in some scenarios.
This patch achieve the aim in Qemu side.

TODO:
- Post a patch to vhost-scsi driver module to realize get target id
  for bootable vhost-scsi device.

Gonglei (4):
  qdev: support to get a device firmware path directly
  vhost-scsi: add bootindex property
  vhost-scsi: realize the TYPE_FW_PATH_PROVIDER interface
  vhost-scsi: add an ioctl interface to get target id

 hw/core/qdev.c                 | 14 +++++++++
 hw/scsi/vhost-scsi.c           | 68 ++++++++++++++++++++++++++++++++++++++++++
 hw/virtio/virtio-pci.c         |  2 ++
 include/hw/virtio/vhost-scsi.h |  5 ++++
 4 files changed, 89 insertions(+)

-- 
1.7.12.4

[Qemu-devel] [PATCH 1/4] qdev: support to get a device firmware path directly

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

commit 6b1566c (qdev: Introduce FWPathProvider interface) did a
good job for supproting to get firmware path on some different
architectures.

Moreover further more, we can use the interface to get firmware
path name for a device which isn't attached a specific bus,
such as virtio-bus, scsi-bus etc.

When the device (such as vhost-scsi) realize the TYPE_FW_PATH_PROVIDER
interface, we should introduce a new function to get the correct firmware
path name for it.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/core/qdev.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 901f289..fb0a150 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -768,6 +768,14 @@ static char *qdev_get_fw_dev_path_from_handler(BusState *bus, DeviceState *dev)
     return d;
 }
 
+static char *qdev_get_own_fw_dev_path_from_handler(BusState *bus,
+                                                   DeviceState *dev)
+{
+    Object *obj = OBJECT(dev);
+
+    return fw_path_provider_try_get_dev_path(obj, bus, dev);
+}
+
 static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
 {
     int l = 0;
@@ -780,6 +788,12 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
             d = bus_get_fw_dev_path(dev->parent_bus, dev);
         }
         if (d) {
+            l += snprintf(p + l, size - l, "%s/", d);
+            g_free(d);
+        }
+
+        d = qdev_get_own_fw_dev_path_from_handler(dev->parent_bus, dev);
+        if (d) {
             l += snprintf(p + l, size - l, "%s", d);
             g_free(d);
         } else {
-- 
1.7.12.4

[Qemu-devel] [PATCH 2/4] vhost-scsi: add bootindex property

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/scsi/vhost-scsi.c           | 9 +++++++++
 hw/virtio/virtio-pci.c         | 2 ++
 include/hw/virtio/vhost-scsi.h | 1 +
 3 files changed, 12 insertions(+)

diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index dcb2bc5..9c4f613 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -290,11 +290,20 @@ static void vhost_scsi_class_init(ObjectClass *klass, void *data)
     vdc->set_status = vhost_scsi_set_status;
 }
 
+static void vhost_scsi_instance_init(Object *obj)
+{
+    VHostSCSI *dev = VHOST_SCSI(obj);
+
+    device_add_bootindex_property(obj, &dev->bootindex, "bootindex", NULL,
+                                  DEVICE(dev), NULL);
+}
+
 static const TypeInfo vhost_scsi_info = {
     .name = TYPE_VHOST_SCSI,
     .parent = TYPE_VIRTIO_SCSI_COMMON,
     .instance_size = sizeof(VHostSCSI),
     .class_init = vhost_scsi_class_init,
+    .instance_init = vhost_scsi_instance_init,
 };
 
 static void virtio_register_types(void)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index dde1d73..604cb5b 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1238,6 +1238,8 @@ static void vhost_scsi_pci_instance_init(Object *obj)
 
     virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
                                 TYPE_VHOST_SCSI);
+    object_property_add_alias(obj, "bootindex", OBJECT(&dev->vdev),
+                              "bootindex", &error_abort);
 }
 
 static const TypeInfo vhost_scsi_pci_info = {
diff --git a/include/hw/virtio/vhost-scsi.h b/include/hw/virtio/vhost-scsi.h
index 85cc031..ed50289 100644
--- a/include/hw/virtio/vhost-scsi.h
+++ b/include/hw/virtio/vhost-scsi.h
@@ -60,6 +60,7 @@ typedef struct VHostSCSI {
     Error *migration_blocker;
 
     struct vhost_dev dev;
+    int32_t bootindex;
 } VHostSCSI;
 
 #define DEFINE_VHOST_SCSI_PROPERTIES(_state, _conf_field) \
-- 
1.7.12.4

[Qemu-devel] [PATCH 3/4] vhost-scsi: realize the TYPE_FW_PATH_PROVIDER interface

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

In the way, we can make the bootindex property take effect.
At the meanwhile, the firmware path name of vhost-scsi is
"channel@channel/vhost-scsi@target,lun".

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/scsi/vhost-scsi.c           | 20 ++++++++++++++++++++
 include/hw/virtio/vhost-scsi.h |  3 +++
 2 files changed, 23 insertions(+)

diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 9c4f613..dc9076e 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -24,6 +24,7 @@
 #include "hw/virtio/virtio-scsi.h"
 #include "hw/virtio/virtio-bus.h"
 #include "hw/virtio/virtio-access.h"
+#include "hw/fw-path-provider.h"
 
 /* Features supported by host kernel. */
 static const int kernel_feature_bits[] = {
@@ -271,6 +272,19 @@ static void vhost_scsi_unrealize(DeviceState *dev, Error **errp)
     virtio_scsi_common_unrealize(dev, errp);
 }
 
+/*
+ * Implementation of an interface to adjust firmware path
+ * for the bootindex property handling.
+ */
+static char *vhost_scsi_get_fw_dev_path(FWPathProvider *p, BusState *bus,
+                                        DeviceState *dev)
+{
+    VHostSCSI *s = VHOST_SCSI(dev);
+    /* format: channel@channel/vhost-scsi@target,lun */
+    return g_strdup_printf("channel@%x/%s@%x,%x", s->channel,
+                           qdev_fw_name(dev), s->target, s->lun);
+}
+
 static Property vhost_scsi_properties[] = {
     DEFINE_VHOST_SCSI_PROPERTIES(VHostSCSI, parent_obj.conf),
     DEFINE_PROP_END_OF_LIST(),
@@ -280,6 +294,7 @@ static void vhost_scsi_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
+    FWPathProviderClass *fwc = FW_PATH_PROVIDER_CLASS(klass);
 
     dc->props = vhost_scsi_properties;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
@@ -288,6 +303,7 @@ static void vhost_scsi_class_init(ObjectClass *klass, void *data)
     vdc->get_features = vhost_scsi_get_features;
     vdc->set_config = vhost_scsi_set_config;
     vdc->set_status = vhost_scsi_set_status;
+    fwc->get_dev_path = vhost_scsi_get_fw_dev_path;
 }
 
 static void vhost_scsi_instance_init(Object *obj)
@@ -304,6 +320,10 @@ static const TypeInfo vhost_scsi_info = {
     .instance_size = sizeof(VHostSCSI),
     .class_init = vhost_scsi_class_init,
     .instance_init = vhost_scsi_instance_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_FW_PATH_PROVIDER },
+        { }
+    },
 };
 
 static void virtio_register_types(void)
diff --git a/include/hw/virtio/vhost-scsi.h b/include/hw/virtio/vhost-scsi.h
index ed50289..c0056c2 100644
--- a/include/hw/virtio/vhost-scsi.h
+++ b/include/hw/virtio/vhost-scsi.h
@@ -61,6 +61,9 @@ typedef struct VHostSCSI {
 
     struct vhost_dev dev;
     int32_t bootindex;
+    int channel;
+    int target;
+    int lun;
 } VHostSCSI;
 
 #define DEFINE_VHOST_SCSI_PROPERTIES(_state, _conf_field) \
-- 
1.7.12.4

[Qemu-devel] [PATCH 4/4] vhost-scsi: add an ioctl interface to get target id

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Because vhost-scsi module do not support VHOST_SCSI_GET_TPGT
at present, so I use "#if 0" handle it, and set the target
default to 1. In addition, channel and lun both are 0 for
bootable vhost-scsi device.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Bo Su <subo7@huawei.com>
---
 hw/scsi/vhost-scsi.c           | 39 +++++++++++++++++++++++++++++++++++++++
 include/hw/virtio/vhost-scsi.h |  1 +
 2 files changed, 40 insertions(+)

diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index dc9076e..a7cd420 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -201,6 +201,36 @@ static void vhost_dummy_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
 }
 
+static int vhost_scsi_get_target_id(VHostSCSI *s, Error **errp)
+{
+    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
+    const VhostOps *vhost_ops = s->dev.vhost_ops;
+    struct vhost_scsi_target backend;
+    int ret;
+
+    memset(&backend, 0, sizeof(backend));
+    pstrcpy(backend.vhost_wwpn, sizeof(backend.vhost_wwpn), vs->conf.wwpn);
+    ret = vhost_ops->vhost_call(&s->dev, VHOST_SCSI_GET_TPGT, &backend);
+    /* Fixme: wait vhost-scsi module supporting VHOST_SCSI_GET_TPGT */
+#if 0
+    if (ret < 0) {
+        error_setg_errno(errp, -ret,
+                         "vhost-scsi: failed to get the target id");
+        return ret;
+    }
+#elseif
+    if (ret < 0) {
+        error_report("Warning: vhost-scsi failed to get the target id: %s",
+                     strerror(-ret));
+        /* set default target to 1 */
+        backend.vhost_tpgt = 1;
+    }
+#endif
+
+    s->target = backend.vhost_tpgt;
+    return 0;
+}
+
 static void vhost_scsi_realize(DeviceState *dev, Error **errp)
 {
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
@@ -251,6 +281,15 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    /* At present, channel and lun both are 0 for bootable vhost-scsi disk */
+    s->channel = 0;
+    s->lun = 0;
+    if (vhost_scsi_get_target_id(s, &err)) {
+        error_propagate(errp, err);
+        close(vhostfd);
+        return;
+    }
+
     error_setg(&s->migration_blocker,
             "vhost-scsi does not support migration");
     migrate_add_blocker(s->migration_blocker);
diff --git a/include/hw/virtio/vhost-scsi.h b/include/hw/virtio/vhost-scsi.h
index c0056c2..a6bd031 100644
--- a/include/hw/virtio/vhost-scsi.h
+++ b/include/hw/virtio/vhost-scsi.h
@@ -49,6 +49,7 @@ enum vhost_scsi_vq_list {
 #define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct vhost_scsi_target)
 #define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct vhost_scsi_target)
 #define VHOST_SCSI_GET_ABI_VERSION _IOW(VHOST_VIRTIO, 0x42, int)
+#define VHOST_SCSI_GET_TPGT _IOW(VHOST_VIRTIO, 0x45, struct vhost_scsi_target)
 
 #define TYPE_VHOST_SCSI "vhost-scsi"
 #define VHOST_SCSI(obj) \
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 4/4] vhost-scsi: add an ioctl interface to get target id

[Michael S. Tsirkin]

On Mon, Jan 19, 2015 at 09:23:38PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Because vhost-scsi module do not support VHOST_SCSI_GET_TPGT
> at present, so I use "#if 0" handle it, and set the target
> default to 1. In addition, channel and lun both are 0 for
> bootable vhost-scsi device.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Signed-off-by: Bo Su <subo7@huawei.com>

OK but let's see the kernel patch get accepted first.

> ---
>  hw/scsi/vhost-scsi.c           | 39 +++++++++++++++++++++++++++++++++++++++
>  include/hw/virtio/vhost-scsi.h |  1 +
>  2 files changed, 40 insertions(+)
> 
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index dc9076e..a7cd420 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -201,6 +201,36 @@ static void vhost_dummy_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>  {
>  }
>  
> +static int vhost_scsi_get_target_id(VHostSCSI *s, Error **errp)
> +{
> +    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
> +    const VhostOps *vhost_ops = s->dev.vhost_ops;
> +    struct vhost_scsi_target backend;
> +    int ret;
> +
> +    memset(&backend, 0, sizeof(backend));
> +    pstrcpy(backend.vhost_wwpn, sizeof(backend.vhost_wwpn), vs->conf.wwpn);
> +    ret = vhost_ops->vhost_call(&s->dev, VHOST_SCSI_GET_TPGT, &backend);
> +    /* Fixme: wait vhost-scsi module supporting VHOST_SCSI_GET_TPGT */
> +#if 0
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret,
> +                         "vhost-scsi: failed to get the target id");
> +        return ret;
> +    }
> +#elseif
> +    if (ret < 0) {
> +        error_report("Warning: vhost-scsi failed to get the target id: %s",
> +                     strerror(-ret));
> +        /* set default target to 1 */
> +        backend.vhost_tpgt = 1;
> +    }
> +#endif
> +
> +    s->target = backend.vhost_tpgt;
> +    return 0;
> +}
> +
>  static void vhost_scsi_realize(DeviceState *dev, Error **errp)
>  {
>      VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
> @@ -251,6 +281,15 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> +    /* At present, channel and lun both are 0 for bootable vhost-scsi disk */
> +    s->channel = 0;
> +    s->lun = 0;
> +    if (vhost_scsi_get_target_id(s, &err)) {
> +        error_propagate(errp, err);
> +        close(vhostfd);
> +        return;
> +    }
> +
>      error_setg(&s->migration_blocker,
>              "vhost-scsi does not support migration");
>      migrate_add_blocker(s->migration_blocker);
> diff --git a/include/hw/virtio/vhost-scsi.h b/include/hw/virtio/vhost-scsi.h
> index c0056c2..a6bd031 100644
> --- a/include/hw/virtio/vhost-scsi.h
> +++ b/include/hw/virtio/vhost-scsi.h
> @@ -49,6 +49,7 @@ enum vhost_scsi_vq_list {
>  #define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct vhost_scsi_target)
>  #define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct vhost_scsi_target)
>  #define VHOST_SCSI_GET_ABI_VERSION _IOW(VHOST_VIRTIO, 0x42, int)
> +#define VHOST_SCSI_GET_TPGT _IOW(VHOST_VIRTIO, 0x45, struct vhost_scsi_target)
>  
>  #define TYPE_VHOST_SCSI "vhost-scsi"
>  #define VHOST_SCSI(obj) \
> -- 
> 1.7.12.4
> 

Re: [Qemu-devel] [PATCH 1/4] qdev: support to get a device firmware path directly

[Paolo Bonzini]

On 19/01/2015 14:23, arei.gonglei@huawei.com wrote:
> @@ -780,6 +788,12 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
>              d = bus_get_fw_dev_path(dev->parent_bus, dev);
>          }
>          if (d) {
> +            l += snprintf(p + l, size - l, "%s/", d);
> +            g_free(d);
> +        }
> +
> +        d = qdev_get_own_fw_dev_path_from_handler(dev->parent_bus, dev);

This changes preexisting behavior.  If d was true, you wouldn't go down
the following else.  Now it does.

I was thinking it should be handled though the "suffix" argument to
add_boot_device_path, but that's harder now that the suffix has to be
passed to device_add_bootindex_property.

Perhaps you could call qdev_get_own_fw_dev_path_from_handler in
get_boot_devices_list, and convert non-NULL suffixes to implementations
of FWPathProvider?

Paolo

> +        if (d) {
>              l += snprintf(p + l, size - l, "%s", d);
>              g_free(d);
>          } else {

Re: [Qemu-devel] [PATCH 1/4] qdev: support to get a device firmware path directly

[Gonglei]

On 2015/1/21 0:10, Paolo Bonzini wrote:

> 
> 
> On 19/01/2015 14:23, arei.gonglei@huawei.com wrote:
>> @@ -780,6 +788,12 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
>>              d = bus_get_fw_dev_path(dev->parent_bus, dev);
>>          }
>>          if (d) {
>> +            l += snprintf(p + l, size - l, "%s/", d);
>> +            g_free(d);
>> +        }
>> +
>> +        d = qdev_get_own_fw_dev_path_from_handler(dev->parent_bus, dev);
> 
> This changes preexisting behavior.  If d was true, you wouldn't go down
> the following else.  Now it does.
> 

On the face of things, it is. But actually they are equal. Please notice I added a "/" at the
end p, and then the function can return if d was NULL.
  l += snprintf(p + l, size - l, "%s/", d);

> I was thinking it should be handled though the "suffix" argument to
> add_boot_device_path, but that's harder now that the suffix has to be
> passed to device_add_bootindex_property.
> 

Yes.

> Perhaps you could call qdev_get_own_fw_dev_path_from_handler in
> get_boot_devices_list, and convert non-NULL suffixes to implementations
> of FWPathProvider?
> 

Maybe your meaning is "convert NULL suffixes to implementations
 of FWPathProvider"?  Something like below:

diff --git a/bootdevice.c b/bootdevice.c
index 5914417..546ca9d 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -225,7 +225,18 @@ char *get_boot_devices_list(size_t *size, bool ignore_suffixes)
             snprintf(bootpath, bootpathlen, "%s%s", devpath, i->suffix);
             g_free(devpath);
         } else if (devpath) {
-            bootpath = devpath;
+            char *d = NULL;
+            size_t bootpathlen;
+
+            d = qdev_get_own_fw_dev_path_from_handler(i->dev->parent_bus, i->dev);
+            if (d) {
+                bootpathlen = strlen(devpath) + strlen(d) + 1;
+                bootpath = g_malloc(bootpathlen);
+                snprintf(bootpath, bootpathlen, "%s%s", devpath, d);
+                g_free(devpath);
+            } else {
+                bootpath = devpath;
+            }
         } else if (!ignore_suffixes) {
             assert(i->suffix);
             bootpath = g_strdup(i->suffix);

But I feel this more complicated. Isn't ?

Regards,
-Gonglei

> Paolo
> 
>> +        if (d) {
>>              l += snprintf(p + l, size - l, "%s", d);
>>              g_free(d);
>>          } else {

Re: [Qemu-devel] [PATCH 1/4] qdev: support to get a device firmware path directly

[Paolo Bonzini]

On 21/01/2015 03:14, Gonglei wrote:
> On 2015/1/21 0:10, Paolo Bonzini wrote:
> 
>>
>>
>> On 19/01/2015 14:23, arei.gonglei@huawei.com wrote:
>>> @@ -780,6 +788,12 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
>>>              d = bus_get_fw_dev_path(dev->parent_bus, dev);
>>>          }
>>>          if (d) {
>>> +            l += snprintf(p + l, size - l, "%s/", d);
>>> +            g_free(d);
>>> +        }
>>> +
>>> +        d = qdev_get_own_fw_dev_path_from_handler(dev->parent_bus, dev);
>>
>> This changes preexisting behavior.  If d was true, you wouldn't go down
>> the following else.  Now it does.
>>
> 
> On the face of things, it is. But actually they are equal. Please notice I added a "/" at the
> end p, and then the function can return if d was NULL.
>   l += snprintf(p + l, size - l, "%s/", d);

But in this case I think the "return l" should become unconditional.  
It should move out of the "else".

>> I was thinking it should be handled though the "suffix" argument to
>> add_boot_device_path, but that's harder now that the suffix has to be
>> passed to device_add_bootindex_property.
>>
> 
> Yes.
> 
>> Perhaps you could call qdev_get_own_fw_dev_path_from_handler in
>> get_boot_devices_list, and convert non-NULL suffixes to implementations
>> of FWPathProvider?
> 
> Maybe your meaning is "convert NULL suffixes to implementations
>  of FWPathProvider"?  Something like below:

No, I meant non-NULL.

In the beginning it can be something like this:

diff --git a/bootdevice.c b/bootdevice.c
index 5914417..916bfb7 100644
--- a/bootdevice.c
+++ b/bootdevice.c
@@ -210,7 +210,8 @@ char *get_boot_devices_list(size_t *size, bool ignore_suffixes)
     char *list = NULL;
 
     QTAILQ_FOREACH(i, &fw_boot_order, link) {
-        char *devpath = NULL, *bootpath;
+        char *devpath = NULL,  *suffix = NULL;
+        char *bootpath;
         size_t len;
 
         if (i->dev) {
@@ -218,21 +219,22 @@ char *get_boot_devices_list(size_t *size, bool ignore_suffixes)
             assert(devpath);
         }
 
-        if (i->suffix && !ignore_suffixes && devpath) {
-            size_t bootpathlen = strlen(devpath) + strlen(i->suffix) + 1;
-
-            bootpath = g_malloc(bootpathlen);
-            snprintf(bootpath, bootpathlen, "%s%s", devpath, i->suffix);
-            g_free(devpath);
-        } else if (devpath) {
-            bootpath = devpath;
-        } else if (!ignore_suffixes) {
-            assert(i->suffix);
-            bootpath = g_strdup(i->suffix);
-        } else {
-            bootpath = g_strdup("");
+        if (!ignore_suffixes) {
+            d = qdev_get_own_fw_dev_path_from_handler(i->dev->parent_bus, i->dev);
+            if (d) {
+                assert(!i->suffix);
+                suffix = d;
+            } else {
+                suffix = g_strdup(i->suffix);
+            }
         }
 
+        bootpath = g_strdup_printf("%s%s",
+                                   devpath ? devpath : "",
+                                   suffix ? suffix : "");
+        g_free(devpath);
+        g_free(suffix);
+
         if (total) {
             list[total-1] = '\n';
         }


Then as time permits the suffix can be phased out and replaced by
FWPathProvider on the device.

Paolo

> But I feel this more complicated. Isn't ?
> 
> Regards,
> -Gonglei
> 
>> Paolo
>>
>>> +        if (d) {
>>>              l += snprintf(p + l, size - l, "%s", d);
>>>              g_free(d);
>>>          } else {
> 
> 
> 
> 
> 

Re: [Qemu-devel] [PATCH 1/4] qdev: support to get a device firmware path directly

[Gonglei]

On 2015/1/21 18:52, Paolo Bonzini wrote:

> 
> 
> On 21/01/2015 03:14, Gonglei wrote:
>> On 2015/1/21 0:10, Paolo Bonzini wrote:
>>
>>>
>>>
>>> On 19/01/2015 14:23, arei.gonglei@huawei.com wrote:
>>>> @@ -780,6 +788,12 @@ static int qdev_get_fw_dev_path_helper(DeviceState *dev, char *p, int size)
>>>>              d = bus_get_fw_dev_path(dev->parent_bus, dev);
>>>>          }
>>>>          if (d) {
>>>> +            l += snprintf(p + l, size - l, "%s/", d);
>>>> +            g_free(d);
>>>> +        }
>>>> +
>>>> +        d = qdev_get_own_fw_dev_path_from_handler(dev->parent_bus, dev);
>>>
>>> This changes preexisting behavior.  If d was true, you wouldn't go down
>>> the following else.  Now it does.
>>>
>>
>> On the face of things, it is. But actually they are equal. Please notice I added a "/" at the
>> end p, and then the function can return if d was NULL.
>>   l += snprintf(p + l, size - l, "%s/", d);
> 
> But in this case I think the "return l" should become unconditional.  
> It should move out of the "else".
> 

After calling qdev_get_own_fw_dev_path_from_handler(),  if d was true, there is also
 l += snprintf(p + l, size - l, "%s", d);
we still have to add a '/' too. These two situation are different.

>>> I was thinking it should be handled though the "suffix" argument to
>>> add_boot_device_path, but that's harder now that the suffix has to be
>>> passed to device_add_bootindex_property.
>>>
>>
>> Yes.
>>
>>> Perhaps you could call qdev_get_own_fw_dev_path_from_handler in
>>> get_boot_devices_list, and convert non-NULL suffixes to implementations
>>> of FWPathProvider?
>>
>> Maybe your meaning is "convert NULL suffixes to implementations
>>  of FWPathProvider"?  Something like below:
> 
> No, I meant non-NULL.
> 
> In the beginning it can be something like this:
> 
> diff --git a/bootdevice.c b/bootdevice.c
> index 5914417..916bfb7 100644
> --- a/bootdevice.c
> +++ b/bootdevice.c
> @@ -210,7 +210,8 @@ char *get_boot_devices_list(size_t *size, bool ignore_suffixes)
>      char *list = NULL;
>  
>      QTAILQ_FOREACH(i, &fw_boot_order, link) {
> -        char *devpath = NULL, *bootpath;
> +        char *devpath = NULL,  *suffix = NULL;
> +        char *bootpath;
>          size_t len;
>  
>          if (i->dev) {
> @@ -218,21 +219,22 @@ char *get_boot_devices_list(size_t *size, bool ignore_suffixes)
>              assert(devpath);
>          }
>  
> -        if (i->suffix && !ignore_suffixes && devpath) {
> -            size_t bootpathlen = strlen(devpath) + strlen(i->suffix) + 1;
> -
> -            bootpath = g_malloc(bootpathlen);
> -            snprintf(bootpath, bootpathlen, "%s%s", devpath, i->suffix);
> -            g_free(devpath);
> -        } else if (devpath) {
> -            bootpath = devpath;
> -        } else if (!ignore_suffixes) {
> -            assert(i->suffix);
> -            bootpath = g_strdup(i->suffix);
> -        } else {
> -            bootpath = g_strdup("");
> +        if (!ignore_suffixes) {
> +            d = qdev_get_own_fw_dev_path_from_handler(i->dev->parent_bus, i->dev);
> +            if (d) {
> +                assert(!i->suffix);
> +                suffix = d;
> +            } else {
> +                suffix = g_strdup(i->suffix);
> +            }
>          }
>  
> +        bootpath = g_strdup_printf("%s%s",
> +                                   devpath ? devpath : "",
> +                                   suffix ? suffix : "");
> +        g_free(devpath);
> +        g_free(suffix);
> +
>          if (total) {
>              list[total-1] = '\n';
>          }
> 
> 
> Then as time permits the suffix can be phased out and replaced by
> FWPathProvider on the device.
> 
Anyway, I like your approach, it's so clear. :)


Regards,
-Gonglei

Re: [Qemu-devel] [PATCH 4/4] vhost-scsi: add an ioctl interface to get target id

[Gonglei]

On 2015/1/20 5:51, Michael S. Tsirkin wrote:

> On Mon, Jan 19, 2015 at 09:23:38PM +0800, arei.gonglei@huawei.com wrote:
>> > From: Gonglei <arei.gonglei@huawei.com>
>> > 
>> > Because vhost-scsi module do not support VHOST_SCSI_GET_TPGT
>> > at present, so I use "#if 0" handle it, and set the target
>> > default to 1. In addition, channel and lun both are 0 for
>> > bootable vhost-scsi device.
>> > 
>> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> > Signed-off-by: Bo Su <subo7@huawei.com>
> OK but let's see the kernel patch get accepted first.
> 

Hi, Michael & Paolo

Because Qemu only accept an wwpn argument for vhost-scsi, we
cannot assign a tpgt. That's say tpg is transparent for Qemu, Qemu
doesn't know which tpg can boot, but vhost-scsi driver module
doesn't know too for one assigned wwpn. At present, we assume that
the first tpg can boot only, and add an ioctl to get the first tpgt,
can we? Looking  forward to your reply for any suggestions.

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH 4/4] vhost-scsi: add an ioctl interface to get target id

[Paolo Bonzini]

On 26/01/2015 09:00, Gonglei wrote:
> Hi, Michael & Paolo
> 
> Because Qemu only accept an wwpn argument for vhost-scsi, we
> cannot assign a tpgt. That's say tpg is transparent for Qemu, Qemu
> doesn't know which tpg can boot, but vhost-scsi driver module
> doesn't know too for one assigned wwpn. At present, we assume that
> the first tpg can boot only, and add an ioctl to get the first tpgt,
> can we? Looking  forward to your reply for any suggestions.

That's okay, alternatively you could add a boot_tpgt argument that
defaults to 1 (is it correct that 0 is not a valid tpgt?).

Paolo

Re: [Qemu-devel] [PATCH 4/4] vhost-scsi: add an ioctl interface to get target id

[Gonglei]

On 2015/1/26 17:32, Paolo Bonzini wrote:

> 
> 
> On 26/01/2015 09:00, Gonglei wrote:
>> Hi, Michael & Paolo
>>
>> Because Qemu only accept an wwpn argument for vhost-scsi, we
>> cannot assign a tpgt. That's say tpg is transparent for Qemu, Qemu
>> doesn't know which tpg can boot, but vhost-scsi driver module
>> doesn't know too for one assigned wwpn. At present, we assume that
>> the first tpg can boot only, and add an ioctl to get the first tpgt,
>> can we? Looking  forward to your reply for any suggestions.
> 
> That's okay, alternatively you could add a boot_tpgt argument that
> defaults to 1 (is it correct that 0 is not a valid tpgt?).
> 

No, 0 is the minimize valid value. :)
Paolo, where do you think we should add a boot_tpgt argument?

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH 4/4] vhost-scsi: add an ioctl interface to get target id

[Paolo Bonzini]

On 26/01/2015 13:13, Gonglei wrote:
>> > 
>> > That's okay, alternatively you could add a boot_tpgt argument that
>> > defaults to 1 (is it correct that 0 is not a valid tpgt?).
>> > 
> No, 0 is the minimize valid value. :)
> Paolo, where do you think we should add a boot_tpgt argument?

To vhost-scsi-pci.  (Property, not argument).

Paolo

Re: [Qemu-devel] [PATCH 4/4] vhost-scsi: add an ioctl interface to get target id

[Gonglei]

On 2015/1/26 20:16, Paolo Bonzini wrote:

> 
> 
> On 26/01/2015 13:13, Gonglei wrote:
>>>>
>>>> That's okay, alternatively you could add a boot_tpgt argument that
>>>> defaults to 1 (is it correct that 0 is not a valid tpgt?).
>>>>
>> No, 0 is the minimize valid value. :)
>> Paolo, where do you think we should add a boot_tpgt argument?
> 
> To vhost-scsi-pci.  (Property, not argument).
> 

Got it. Thanks.

BTW, I will send the kernel patch later. :)

Regards,
-Gonglei