[Qemu-devel] [PATCH V3 WIP 0/3] vhost-scsi: new device supporting the tcm_vhost Linux kernel module

[Qemu-devel] [PATCH V3 WIP 0/3] vhost-scsi: new device supporting the tcm_vhost Linux kernel module

[Asias He]

This is on top of Paolo and Nick's work.

Current status:
Basically, tcm_vhost + seabios works now. We still have one more issue,
vhost_verify_ring_mappings fails. The hotplug also works with the latest
tcm_vhost.ko hotplug patch.

Asias He (1):
  disable vhost_verify_ring_mappings check

Paolo Bonzini (2):
  virtio-scsi: create VirtIOSCSICommon
  vhost-scsi: new device supporting the tcm_vhost Linux kernel module

 configure                  |  15 ++-
 hw/Makefile.objs           |   5 +-
 hw/s390x/s390-virtio-bus.c |  35 +++++++
 hw/vhost-scsi.c            | 242 +++++++++++++++++++++++++++++++++++++++++++++
 hw/vhost-scsi.h            |  65 ++++++++++++
 hw/vhost.c                 |   2 +
 hw/virtio-pci.c            |  62 ++++++++++++
 hw/virtio-scsi.c           | 199 +++++++++----------------------------
 hw/virtio-scsi.h           | 129 ++++++++++++++++++++++++
 include/qemu/osdep.h       |   4 +
 10 files changed, 606 insertions(+), 152 deletions(-)
 create mode 100644 hw/vhost-scsi.c
 create mode 100644 hw/vhost-scsi.h

-- 
1.8.1.4

[Qemu-devel] [PATCH V3 WIP 1/3] virtio-scsi: create VirtIOSCSICommon

[Asias He]

From: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio-scsi.c     | 199 +++++++++++++--------------------------------------
 hw/virtio-scsi.h     | 127 ++++++++++++++++++++++++++++++++
 include/qemu/osdep.h |   4 ++
 3 files changed, 180 insertions(+), 150 deletions(-)

diff --git a/hw/virtio-scsi.c b/hw/virtio-scsi.c
index 72cc519..6cd0272 100644
--- a/hw/virtio-scsi.c
+++ b/hw/virtio-scsi.c
@@ -18,131 +18,12 @@
 #include <hw/scsi.h>
 #include <hw/scsi-defs.h>
 
-#define VIRTIO_SCSI_VQ_SIZE     128
-#define VIRTIO_SCSI_CDB_SIZE    32
-#define VIRTIO_SCSI_SENSE_SIZE  96
-#define VIRTIO_SCSI_MAX_CHANNEL 0
-#define VIRTIO_SCSI_MAX_TARGET  255
-#define VIRTIO_SCSI_MAX_LUN     16383
-
-/* Response codes */
-#define VIRTIO_SCSI_S_OK                       0
-#define VIRTIO_SCSI_S_OVERRUN                  1
-#define VIRTIO_SCSI_S_ABORTED                  2
-#define VIRTIO_SCSI_S_BAD_TARGET               3
-#define VIRTIO_SCSI_S_RESET                    4
-#define VIRTIO_SCSI_S_BUSY                     5
-#define VIRTIO_SCSI_S_TRANSPORT_FAILURE        6
-#define VIRTIO_SCSI_S_TARGET_FAILURE           7
-#define VIRTIO_SCSI_S_NEXUS_FAILURE            8
-#define VIRTIO_SCSI_S_FAILURE                  9
-#define VIRTIO_SCSI_S_FUNCTION_SUCCEEDED       10
-#define VIRTIO_SCSI_S_FUNCTION_REJECTED        11
-#define VIRTIO_SCSI_S_INCORRECT_LUN            12
-
-/* Controlq type codes.  */
-#define VIRTIO_SCSI_T_TMF                      0
-#define VIRTIO_SCSI_T_AN_QUERY                 1
-#define VIRTIO_SCSI_T_AN_SUBSCRIBE             2
-
-/* Valid TMF subtypes.  */
-#define VIRTIO_SCSI_T_TMF_ABORT_TASK           0
-#define VIRTIO_SCSI_T_TMF_ABORT_TASK_SET       1
-#define VIRTIO_SCSI_T_TMF_CLEAR_ACA            2
-#define VIRTIO_SCSI_T_TMF_CLEAR_TASK_SET       3
-#define VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET      4
-#define VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET   5
-#define VIRTIO_SCSI_T_TMF_QUERY_TASK           6
-#define VIRTIO_SCSI_T_TMF_QUERY_TASK_SET       7
-
-/* Events.  */
-#define VIRTIO_SCSI_T_EVENTS_MISSED            0x80000000
-#define VIRTIO_SCSI_T_NO_EVENT                 0
-#define VIRTIO_SCSI_T_TRANSPORT_RESET          1
-#define VIRTIO_SCSI_T_ASYNC_NOTIFY             2
-#define VIRTIO_SCSI_T_PARAM_CHANGE             3
-
-/* Reasons for transport reset event */
-#define VIRTIO_SCSI_EVT_RESET_HARD             0
-#define VIRTIO_SCSI_EVT_RESET_RESCAN           1
-#define VIRTIO_SCSI_EVT_RESET_REMOVED          2
-
-/* SCSI command request, followed by data-out */
 typedef struct {
-    uint8_t lun[8];              /* Logical Unit Number */
-    uint64_t tag;                /* Command identifier */
-    uint8_t task_attr;           /* Task attribute */
-    uint8_t prio;
-    uint8_t crn;
-    uint8_t cdb[];
-} QEMU_PACKED VirtIOSCSICmdReq;
-
-/* Response, followed by sense data and data-in */
-typedef struct {
-    uint32_t sense_len;          /* Sense data length */
-    uint32_t resid;              /* Residual bytes in data buffer */
-    uint16_t status_qualifier;   /* Status qualifier */
-    uint8_t status;              /* Command completion status */
-    uint8_t response;            /* Response values */
-    uint8_t sense[];
-} QEMU_PACKED VirtIOSCSICmdResp;
-
-/* Task Management Request */
-typedef struct {
-    uint32_t type;
-    uint32_t subtype;
-    uint8_t lun[8];
-    uint64_t tag;
-} QEMU_PACKED VirtIOSCSICtrlTMFReq;
-
-typedef struct {
-    uint8_t response;
-} QEMU_PACKED VirtIOSCSICtrlTMFResp;
-
-/* Asynchronous notification query/subscription */
-typedef struct {
-    uint32_t type;
-    uint8_t lun[8];
-    uint32_t event_requested;
-} QEMU_PACKED VirtIOSCSICtrlANReq;
-
-typedef struct {
-    uint32_t event_actual;
-    uint8_t response;
-} QEMU_PACKED VirtIOSCSICtrlANResp;
-
-typedef struct {
-    uint32_t event;
-    uint8_t lun[8];
-    uint32_t reason;
-} QEMU_PACKED VirtIOSCSIEvent;
-
-typedef struct {
-    uint32_t num_queues;
-    uint32_t seg_max;
-    uint32_t max_sectors;
-    uint32_t cmd_per_lun;
-    uint32_t event_info_size;
-    uint32_t sense_size;
-    uint32_t cdb_size;
-    uint16_t max_channel;
-    uint16_t max_target;
-    uint32_t max_lun;
-} QEMU_PACKED VirtIOSCSIConfig;
-
-typedef struct {
-    VirtIODevice vdev;
-    DeviceState *qdev;
-    VirtIOSCSIConf *conf;
+    VirtIOSCSICommon vs;
 
     SCSIBus bus;
-    uint32_t sense_size;
-    uint32_t cdb_size;
     int resetting;
     bool events_dropped;
-    VirtQueue *ctrl_vq;
-    VirtQueue *event_vq;
-    VirtQueue *cmd_vqs[0];
 } VirtIOSCSI;
 
 typedef struct VirtIOSCSIReq {
@@ -193,7 +74,7 @@ static void virtio_scsi_complete_req(VirtIOSCSIReq *req)
         scsi_req_unref(req->sreq);
     }
     g_free(req);
-    virtio_notify(&s->vdev, vq);
+    virtio_notify(&s->vs.vdev, vq);
 }
 
 static void virtio_scsi_bad_req(void)
@@ -252,7 +133,7 @@ static void virtio_scsi_save_request(QEMUFile *f, SCSIRequest *sreq)
     VirtIOSCSIReq *req = sreq->hba_private;
     uint32_t n = virtio_queue_get_id(req->vq) - 2;
 
-    assert(n < req->dev->conf->num_queues);
+    assert(n < req->dev->vs.conf->num_queues);
     qemu_put_be32s(f, &n);
     qemu_put_buffer(f, (unsigned char *)&req->elem, sizeof(req->elem));
 }
@@ -266,9 +147,9 @@ static void *virtio_scsi_load_request(QEMUFile *f, SCSIRequest *sreq)
 
     req = g_malloc(sizeof(*req));
     qemu_get_be32s(f, &n);
-    assert(n < s->conf->num_queues);
+    assert(n < s->vs.conf->num_queues);
     qemu_get_buffer(f, (unsigned char *)&req->elem, sizeof(req->elem));
-    virtio_scsi_parse_req(s, s->cmd_vqs[n], req);
+    virtio_scsi_parse_req(s, s->vs.cmd_vqs[n], req);
 
     scsi_req_ref(sreq);
     req->sreq = sreq;
@@ -483,8 +364,8 @@ static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq)
 
         out_size = req->elem.out_sg[0].iov_len;
         in_size = req->elem.in_sg[0].iov_len;
-        if (out_size < sizeof(VirtIOSCSICmdReq) + s->cdb_size ||
-            in_size < sizeof(VirtIOSCSICmdResp) + s->sense_size) {
+        if (out_size < sizeof(VirtIOSCSICmdReq) + s->vs.cdb_size ||
+            in_size < sizeof(VirtIOSCSICmdResp) + s->vs.sense_size) {
             virtio_scsi_bad_req();
         }
 
@@ -526,7 +407,7 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
                                    uint8_t *config)
 {
     VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
-    VirtIOSCSI *s = (VirtIOSCSI *)vdev;
+    VirtIOSCSICommon *s = (VirtIOSCSICommon *)vdev;
 
     stl_raw(&scsiconf->num_queues, s->conf->num_queues);
     stl_raw(&scsiconf->seg_max, 128 - 2);
@@ -552,8 +433,8 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
         exit(1);
     }
 
-    s->sense_size = ldl_raw(&scsiconf->sense_size);
-    s->cdb_size = ldl_raw(&scsiconf->cdb_size);
+    s->vs.sense_size = ldl_raw(&scsiconf->sense_size);
+    s->vs.cdb_size = ldl_raw(&scsiconf->cdb_size);
 }
 
 static uint32_t virtio_scsi_get_features(VirtIODevice *vdev,
@@ -570,8 +451,8 @@ static void virtio_scsi_reset(VirtIODevice *vdev)
     qbus_reset_all(&s->bus.qbus);
     s->resetting--;
 
-    s->sense_size = VIRTIO_SCSI_SENSE_SIZE;
-    s->cdb_size = VIRTIO_SCSI_CDB_SIZE;
+    s->vs.sense_size = VIRTIO_SCSI_SENSE_SIZE;
+    s->vs.cdb_size = VIRTIO_SCSI_CDB_SIZE;
     s->events_dropped = false;
 }
 
@@ -581,7 +462,7 @@ static void virtio_scsi_reset(VirtIODevice *vdev)
 static void virtio_scsi_save(QEMUFile *f, void *opaque)
 {
     VirtIOSCSI *s = opaque;
-    virtio_save(&s->vdev, f);
+    virtio_save(&s->vs.vdev, f);
 }
 
 static int virtio_scsi_load(QEMUFile *f, void *opaque, int version_id)
@@ -589,7 +470,7 @@ static int virtio_scsi_load(QEMUFile *f, void *opaque, int version_id)
     VirtIOSCSI *s = opaque;
     int ret;
 
-    ret = virtio_load(&s->vdev, f);
+    ret = virtio_load(&s->vs.vdev, f);
     if (ret) {
         return ret;
     }
@@ -599,11 +480,11 @@ static int virtio_scsi_load(QEMUFile *f, void *opaque, int version_id)
 static void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
                                    uint32_t event, uint32_t reason)
 {
-    VirtIOSCSIReq *req = virtio_scsi_pop_req(s, s->event_vq);
+    VirtIOSCSIReq *req = virtio_scsi_pop_req(s, s->vs.event_vq);
     VirtIOSCSIEvent *evt;
     int in_size;
 
-    if (!(s->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK)) {
+    if (!(s->vs.vdev.status & VIRTIO_CONFIG_S_DRIVER_OK)) {
         return;
     }
 
@@ -658,7 +539,7 @@ static void virtio_scsi_change(SCSIBus *bus, SCSIDevice *dev, SCSISense sense)
 {
     VirtIOSCSI *s = container_of(bus, VirtIOSCSI, bus);
 
-    if (((s->vdev.guest_features >> VIRTIO_SCSI_F_CHANGE) & 1) &&
+    if (((s->vs.vdev.guest_features >> VIRTIO_SCSI_F_CHANGE) & 1) &&
         dev->type != TYPE_ROM) {
         virtio_scsi_push_event(s, dev, VIRTIO_SCSI_T_PARAM_CHANGE,
                                sense.asc | (sense.ascq << 8));
@@ -669,7 +550,7 @@ static void virtio_scsi_hotplug(SCSIBus *bus, SCSIDevice *dev)
 {
     VirtIOSCSI *s = container_of(bus, VirtIOSCSI, bus);
 
-    if ((s->vdev.guest_features >> VIRTIO_SCSI_F_HOTPLUG) & 1) {
+    if ((s->vs.vdev.guest_features >> VIRTIO_SCSI_F_HOTPLUG) & 1) {
         virtio_scsi_push_event(s, dev, VIRTIO_SCSI_T_TRANSPORT_RESET,
                                VIRTIO_SCSI_EVT_RESET_RESCAN);
     }
@@ -679,7 +560,7 @@ static void virtio_scsi_hot_unplug(SCSIBus *bus, SCSIDevice *dev)
 {
     VirtIOSCSI *s = container_of(bus, VirtIOSCSI, bus);
 
-    if ((s->vdev.guest_features >> VIRTIO_SCSI_F_HOTPLUG) & 1) {
+    if ((s->vs.vdev.guest_features >> VIRTIO_SCSI_F_HOTPLUG) & 1) {
         virtio_scsi_push_event(s, dev, VIRTIO_SCSI_T_TRANSPORT_RESET,
                                VIRTIO_SCSI_EVT_RESET_REMOVED);
     }
@@ -701,35 +582,53 @@ static struct SCSIBusInfo virtio_scsi_scsi_info = {
     .load_request = virtio_scsi_load_request,
 };
 
-VirtIODevice *virtio_scsi_init(DeviceState *dev, VirtIOSCSIConf *proxyconf)
+VirtIOSCSICommon *virtio_scsi_init_common(DeviceState *dev, VirtIOSCSIConf *proxyconf,
+                                          size_t sz)
 {
-    VirtIOSCSI *s;
-    static int virtio_scsi_id;
-    size_t sz;
+    VirtIOSCSICommon *s;
     int i;
 
-    sz = sizeof(VirtIOSCSI) + proxyconf->num_queues * sizeof(VirtQueue *);
-    s = (VirtIOSCSI *)virtio_common_init("virtio-scsi", VIRTIO_ID_SCSI,
-                                         sizeof(VirtIOSCSIConfig), sz);
+    /* Place the cmd_vqs array at the end of the block that the caller
+     * requested.  Align the array properly, just in case.
+     */
+    sz = ROUND_UP(sz, sizeof(void *));
+    s = (VirtIOSCSICommon *)
+            virtio_common_init("virtio-scsi", VIRTIO_ID_SCSI,
+                               sizeof(VirtIOSCSIConfig),
+                               sz + sizeof(VirtQueue *) * proxyconf->num_queues);
 
     s->qdev = dev;
     s->conf = proxyconf;
+    s->sense_size = VIRTIO_SCSI_SENSE_SIZE;
+    s->cdb_size = VIRTIO_SCSI_CDB_SIZE;
 
-    /* TODO set up vdev function pointers */
     s->vdev.get_config = virtio_scsi_get_config;
-    s->vdev.set_config = virtio_scsi_set_config;
     s->vdev.get_features = virtio_scsi_get_features;
-    s->vdev.reset = virtio_scsi_reset;
 
     s->ctrl_vq = virtio_add_queue(&s->vdev, VIRTIO_SCSI_VQ_SIZE,
                                    virtio_scsi_handle_ctrl);
     s->event_vq = virtio_add_queue(&s->vdev, VIRTIO_SCSI_VQ_SIZE,
                                    virtio_scsi_handle_event);
+
+    s->cmd_vqs = (VirtQueue **) ((char *)s + sz);
     for (i = 0; i < s->conf->num_queues; i++) {
         s->cmd_vqs[i] = virtio_add_queue(&s->vdev, VIRTIO_SCSI_VQ_SIZE,
                                          virtio_scsi_handle_cmd);
     }
 
+    return s;
+}
+
+VirtIODevice *virtio_scsi_init(DeviceState *dev, VirtIOSCSIConf *proxyconf)
+{
+    VirtIOSCSI *s;
+    static int virtio_scsi_id;
+
+    s = (VirtIOSCSI *)virtio_scsi_init_common(dev, proxyconf, sizeof(VirtIOSCSI));
+
+    s->vs.vdev.set_config = virtio_scsi_set_config;
+    s->vs.vdev.reset = virtio_scsi_reset;
+
     scsi_bus_new(&s->bus, dev, &virtio_scsi_scsi_info);
     if (!dev->hotplugged) {
         scsi_bus_legacy_handle_cmdline(&s->bus);
@@ -738,12 +637,12 @@ VirtIODevice *virtio_scsi_init(DeviceState *dev, VirtIOSCSIConf *proxyconf)
     register_savevm(dev, "virtio-scsi", virtio_scsi_id++, 1,
                     virtio_scsi_save, virtio_scsi_load, s);
 
-    return &s->vdev;
+    return &s->vs.vdev;
 }
 
 void virtio_scsi_exit(VirtIODevice *vdev)
 {
     VirtIOSCSI *s = (VirtIOSCSI *)vdev;
-    unregister_savevm(s->qdev, "virtio-scsi", s);
+    unregister_savevm(s->vs.qdev, "virtio-scsi", s);
     virtio_cleanup(vdev);
 }
diff --git a/hw/virtio-scsi.h b/hw/virtio-scsi.h
index 81b3279..5acb721 100644
--- a/hw/virtio-scsi.h
+++ b/hw/virtio-scsi.h
@@ -25,12 +25,136 @@
 #define VIRTIO_SCSI_F_HOTPLUG                  1
 #define VIRTIO_SCSI_F_CHANGE                   2
 
+#define VIRTIO_SCSI_VQ_SIZE     128
+#define VIRTIO_SCSI_CDB_SIZE    32
+#define VIRTIO_SCSI_SENSE_SIZE  96
+#define VIRTIO_SCSI_MAX_CHANNEL 0
+#define VIRTIO_SCSI_MAX_TARGET  255
+#define VIRTIO_SCSI_MAX_LUN     16383
+
+/* Response codes */
+#define VIRTIO_SCSI_S_OK                       0
+#define VIRTIO_SCSI_S_OVERRUN                  1
+#define VIRTIO_SCSI_S_ABORTED                  2
+#define VIRTIO_SCSI_S_BAD_TARGET               3
+#define VIRTIO_SCSI_S_RESET                    4
+#define VIRTIO_SCSI_S_BUSY                     5
+#define VIRTIO_SCSI_S_TRANSPORT_FAILURE        6
+#define VIRTIO_SCSI_S_TARGET_FAILURE           7
+#define VIRTIO_SCSI_S_NEXUS_FAILURE            8
+#define VIRTIO_SCSI_S_FAILURE                  9
+#define VIRTIO_SCSI_S_FUNCTION_SUCCEEDED       10
+#define VIRTIO_SCSI_S_FUNCTION_REJECTED        11
+#define VIRTIO_SCSI_S_INCORRECT_LUN            12
+
+/* Controlq type codes.  */
+#define VIRTIO_SCSI_T_TMF                      0
+#define VIRTIO_SCSI_T_AN_QUERY                 1
+#define VIRTIO_SCSI_T_AN_SUBSCRIBE             2
+
+/* Valid TMF subtypes.  */
+#define VIRTIO_SCSI_T_TMF_ABORT_TASK           0
+#define VIRTIO_SCSI_T_TMF_ABORT_TASK_SET       1
+#define VIRTIO_SCSI_T_TMF_CLEAR_ACA            2
+#define VIRTIO_SCSI_T_TMF_CLEAR_TASK_SET       3
+#define VIRTIO_SCSI_T_TMF_I_T_NEXUS_RESET      4
+#define VIRTIO_SCSI_T_TMF_LOGICAL_UNIT_RESET   5
+#define VIRTIO_SCSI_T_TMF_QUERY_TASK           6
+#define VIRTIO_SCSI_T_TMF_QUERY_TASK_SET       7
+
+/* Events.  */
+#define VIRTIO_SCSI_T_EVENTS_MISSED            0x80000000
+#define VIRTIO_SCSI_T_NO_EVENT                 0
+#define VIRTIO_SCSI_T_TRANSPORT_RESET          1
+#define VIRTIO_SCSI_T_ASYNC_NOTIFY             2
+#define VIRTIO_SCSI_T_PARAM_CHANGE             3
+
+/* Reasons for transport reset event */
+#define VIRTIO_SCSI_EVT_RESET_HARD             0
+#define VIRTIO_SCSI_EVT_RESET_RESCAN           1
+#define VIRTIO_SCSI_EVT_RESET_REMOVED          2
+
+/* SCSI command request, followed by data-out */
+typedef struct {
+    uint8_t lun[8];              /* Logical Unit Number */
+    uint64_t tag;                /* Command identifier */
+    uint8_t task_attr;           /* Task attribute */
+    uint8_t prio;
+    uint8_t crn;
+    uint8_t cdb[];
+} QEMU_PACKED VirtIOSCSICmdReq;
+
+/* Response, followed by sense data and data-in */
+typedef struct {
+    uint32_t sense_len;          /* Sense data length */
+    uint32_t resid;              /* Residual bytes in data buffer */
+    uint16_t status_qualifier;   /* Status qualifier */
+    uint8_t status;              /* Command completion status */
+    uint8_t response;            /* Response values */
+    uint8_t sense[];
+} QEMU_PACKED VirtIOSCSICmdResp;
+
+/* Task Management Request */
+typedef struct {
+    uint32_t type;
+    uint32_t subtype;
+    uint8_t lun[8];
+    uint64_t tag;
+} QEMU_PACKED VirtIOSCSICtrlTMFReq;
+
+typedef struct {
+    uint8_t response;
+} QEMU_PACKED VirtIOSCSICtrlTMFResp;
+
+/* Asynchronous notification query/subscription */
+typedef struct {
+    uint32_t type;
+    uint8_t lun[8];
+    uint32_t event_requested;
+} QEMU_PACKED VirtIOSCSICtrlANReq;
+
+typedef struct {
+    uint32_t event_actual;
+    uint8_t response;
+} QEMU_PACKED VirtIOSCSICtrlANResp;
+
+typedef struct {
+    uint32_t event;
+    uint8_t lun[8];
+    uint32_t reason;
+} QEMU_PACKED VirtIOSCSIEvent;
+
+typedef struct {
+    uint32_t num_queues;
+    uint32_t seg_max;
+    uint32_t max_sectors;
+    uint32_t cmd_per_lun;
+    uint32_t event_info_size;
+    uint32_t sense_size;
+    uint32_t cdb_size;
+    uint16_t max_channel;
+    uint16_t max_target;
+    uint32_t max_lun;
+} QEMU_PACKED VirtIOSCSIConfig;
+
 struct VirtIOSCSIConf {
     uint32_t num_queues;
     uint32_t max_sectors;
     uint32_t cmd_per_lun;
 };
 
+typedef struct VirtIOSCSICommon {
+    VirtIODevice vdev;
+    DeviceState *qdev;
+    VirtIOSCSIConf *conf;
+
+    uint32_t sense_size;
+    uint32_t cdb_size;
+    VirtQueue *ctrl_vq;
+    VirtQueue *event_vq;
+    VirtQueue **cmd_vqs;
+} VirtIOSCSICommon;
+
 #define DEFINE_VIRTIO_SCSI_PROPERTIES(_state, _features_field, _conf_field) \
     DEFINE_VIRTIO_COMMON_FEATURES(_state, _features_field), \
     DEFINE_PROP_UINT32("num_queues", _state, _conf_field.num_queues, 1), \
@@ -39,4 +163,7 @@ struct VirtIOSCSIConf {
     DEFINE_PROP_BIT("hotplug", _state, _features_field, VIRTIO_SCSI_F_HOTPLUG, true), \
     DEFINE_PROP_BIT("param_change", _state, _features_field, VIRTIO_SCSI_F_CHANGE, true)
 
+VirtIOSCSICommon *virtio_scsi_init_common(DeviceState *dev, VirtIOSCSIConf *proxyconf,
+                                          size_t sz);
+
 #endif /* _QEMU_VIRTIO_SCSI_H */
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index df24400..8b465fd 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -67,6 +67,10 @@ typedef signed int              int_fast16_t;
 #define MAX(a, b) (((a) > (b)) ? (a) : (b))
 #endif
 
+#ifndef ROUND_UP
+#define ROUND_UP(n,d) (((n) + (d) - 1) & -(d))
+#endif
+
 #ifndef DIV_ROUND_UP
 #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
 #endif
-- 
1.8.1.4

[Qemu-devel] [PATCH V3 WIP 2/3] vhost-scsi: new device supporting the tcm_vhost Linux kernel module

[Asias He]

From: Paolo Bonzini <pbonzini@redhat.com>

The WWPN specified in configfs is passed to "-device vhost-scsi-pci".
The tgpt field of the SET_ENDPOINT ioctl is obsolete now, so it is not
available from the QEMU command-line.  Instead, I hardcode it to zero.

Changes in V3:
   - Drop ioeventfd vhost_scsi_properties (asias, thanks stefanha)
   - Add CONFIG_VHOST_SCSI (asias, thanks stefanha)
   - Add hotplug feature bit

Changes in V2:
   - Add backend guest masking support (nab)
   - Bump ABI_VERSION to 1 (nab)
   - Set up set_guest_notifiers (asias)
   - Set up vs->dev.vq_index (asias)
   - Drop vs->vs.vdev.{set,clear}_vhost_endpoint (asias)
   - Drop VIRTIO_CONFIG_S_DRIVER check in vhost_scsi_set_status (asias)

Howto:
   Use the latest seabios
   git clone git://git.seabios.org/seabios.git
   make
   cp out/bios.bin /usr/share/qemu/bios.bin
   qemu -device vhost-scsi-pci,wwpn=naa.6001405bd4e8476d,event_idx=off ...

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
Signed-off-by: Asias He <asias@redhat.com>
---
 configure                  |  15 ++-
 hw/Makefile.objs           |   5 +-
 hw/s390x/s390-virtio-bus.c |  35 +++++++
 hw/vhost-scsi.c            | 242 +++++++++++++++++++++++++++++++++++++++++++++
 hw/vhost-scsi.h            |  65 ++++++++++++
 hw/virtio-pci.c            |  62 ++++++++++++
 hw/virtio-scsi.h           |   2 +
 7 files changed, 424 insertions(+), 2 deletions(-)
 create mode 100644 hw/vhost-scsi.c
 create mode 100644 hw/vhost-scsi.h

diff --git a/configure b/configure
index 84317c6..dca4a66 100755
--- a/configure
+++ b/configure
@@ -169,6 +169,7 @@ libattr=""
 xfs=""
 
 vhost_net="no"
+vhost_scsi="no"
 kvm="no"
 gprof="no"
 debug_tcg="no"
@@ -531,6 +532,7 @@ Haiku)
   usb="linux"
   kvm="yes"
   vhost_net="yes"
+  vhost_scsi="yes"
   if [ "$cpu" = "i386" -o "$cpu" = "x86_64" ] ; then
     audio_possible_drivers="$audio_possible_drivers fmod"
   fi
@@ -857,6 +859,10 @@ for opt do
   ;;
   --enable-vhost-net) vhost_net="yes"
   ;;
+  --disable-vhost-scsi) vhost_scsi="no"
+  ;;
+  --enable-vhost-scsi) vhost_scsi="yes"
+  ;;
   --disable-opengl) opengl="no"
   ;;
   --enable-opengl) opengl="yes"
@@ -3058,7 +3064,7 @@ fi
 # __sync_fetch_and_and requires at least -march=i486. Many toolchains
 # use i686 as default anyway, but for those that don't, an explicit
 # specification is necessary
-if test "$vhost_net" = "yes" && test "$cpu" = "i386"; then
+if (test "$vhost_net" = "yes" -o "$vhost_scsi" = "yes") && test "$cpu" = "i386"; then
   cat > $TMPC << EOF
 static int sfaa(int *ptr)
 {
@@ -3404,6 +3410,7 @@ echo "sigev_thread_id   $sigev_thread_id"
 echo "uuid support      $uuid"
 echo "libcap-ng support $cap_ng"
 echo "vhost-net support $vhost_net"
+echo "vhost-scsi support $vhost_scsi"
 echo "Trace backend     $trace_backend"
 echo "Trace output file $trace_file-<pid>"
 echo "spice support     $spice ($spice_protocol_version/$spice_server_version)"
@@ -3673,6 +3680,9 @@ fi
 if test "$virtfs" = "yes" ; then
   echo "CONFIG_VIRTFS=y" >> $config_host_mak
 fi
+if test "$vhost_scsi" = "yes" ; then
+  echo "CONFIG_VHOST_SCSI=y" >> $config_host_mak
+fi
 if test "$blobs" = "yes" ; then
   echo "INSTALL_BLOBS=yes" >> $config_host_mak
 fi
@@ -4149,6 +4159,9 @@ case "$target_arch2" in
       if test "$vhost_net" = "yes" ; then
         echo "CONFIG_VHOST_NET=y" >> $config_target_mak
       fi
+      if test "$vhost_scsi" = "yes" ; then
+        echo "CONFIG_VHOST_SCSI=y" >> $config_target_mak
+      fi
     fi
 esac
 case "$target_arch2" in
diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index eb7eb31..df57e1f 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -202,8 +202,11 @@ common-obj-$(CONFIG_XEN_BACKEND) += xen_console.o xenfb.o xen_disk.o xen_nic.o
 obj-$(CONFIG_VIRTIO) += dataplane/
 obj-$(CONFIG_VIRTIO) += virtio.o virtio-blk.o virtio-balloon.o virtio-net.o
 obj-$(CONFIG_VIRTIO) += virtio-serial-bus.o virtio-scsi.o
+ifeq ($(CONFIG_VIRTIO), y)
+obj-$(CONFIG_LINUX) += vhost-scsi.o
+endif
 obj-$(CONFIG_SOFTMMU) += vhost_net.o
-obj-$(CONFIG_VHOST_NET) += vhost.o
+obj-$(CONFIG_LINUX) += vhost.o
 obj-$(CONFIG_REALLY_VIRTFS) += 9pfs/
 obj-$(CONFIG_VGA) += vga.o
 
diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index d9b7f83..d86365b 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -28,6 +28,8 @@
 #include "hw/virtio-rng.h"
 #include "hw/virtio-serial.h"
 #include "hw/virtio-net.h"
+#include "hw/virtio-scsi.h"
+#include "hw/vhost-scsi.h"
 #include "hw/sysbus.h"
 #include "sysemu/kvm.h"
 
@@ -207,6 +209,18 @@ static int s390_virtio_scsi_init(VirtIOS390Device *dev)
     return s390_virtio_device_init(dev, vdev);
 }
 
+static int s390_vhost_scsi_init(VirtIOS390Device *dev)
+{
+    VirtIODevice *vdev;
+
+    vdev = vhost_scsi_init((DeviceState *)dev, &dev->scsi);
+    if (!vdev) {
+        return -1;
+    }
+
+    return s390_virtio_device_init(dev, vdev);
+}
+
 static int s390_virtio_rng_init(VirtIOS390Device *dev)
 {
     VirtIODevice *vdev;
@@ -534,6 +548,27 @@ static const TypeInfo virtio_s390_device_info = {
     .abstract = true,
 };
 
+static Property s390_vhost_scsi_properties[] = {
+    DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOS390Device, host_features, scsi),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void s390_vhost_scsi_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    VirtIOS390DeviceClass *k = VIRTIO_S390_DEVICE_CLASS(klass);
+
+    k->init = s390_vhost_scsi_init;
+    dc->props = s390_vhost_scsi_properties;
+}
+
+static const TypeInfo s390_vhost_scsi = {
+    .name          = "vhost-scsi-s390",
+    .parent        = TYPE_VIRTIO_S390_DEVICE,
+    .instance_size = sizeof(VirtIOS390Device),
+    .class_init    = s390_vhost_scsi_class_init,
+};
+
 static Property s390_virtio_scsi_properties[] = {
     DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOS390Device, host_features, scsi),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/vhost-scsi.c b/hw/vhost-scsi.c
new file mode 100644
index 0000000..45762d7
--- /dev/null
+++ b/hw/vhost-scsi.c
@@ -0,0 +1,242 @@
+/*
+ * vhost_scsi host device
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Stefan Hajnoczi   <stefanha@linux.vnet.ibm.com>
+ *
+ * Changes for QEMU mainline + tcm_vhost kernel upstream:
+ *  Nicholas Bellinger <nab@risingtidesystems.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include <sys/ioctl.h>
+#include "config.h"
+#include "qemu/queue.h"
+#include "monitor/monitor.h"
+#include "migration/migration.h"
+#include "vhost-scsi.h"
+#include "vhost.h"
+#include "virtio-scsi.h"
+
+typedef struct VHostSCSI {
+    VirtIOSCSICommon vs;
+
+    Error *migration_blocker;
+
+    struct vhost_dev dev;
+} VHostSCSI;
+
+static int vhost_scsi_set_endpoint(VirtIODevice *vdev)
+{
+    VHostSCSI *vs = (VHostSCSI *)vdev;
+    struct vhost_scsi_target backend;
+    int ret;
+
+    memset(&backend, 0, sizeof(backend));
+    pstrcpy(backend.vhost_wwpn, sizeof(backend.vhost_wwpn), vs->vs.conf->wwpn);
+    ret = ioctl(vs->dev.control, VHOST_SCSI_SET_ENDPOINT, &backend);
+    if (ret < 0) {
+        return -errno;
+    }
+    return 0;
+}
+
+static void vhost_scsi_clear_endpoint(VirtIODevice *vdev)
+{
+    VHostSCSI *vs = (VHostSCSI *)vdev;
+    struct vhost_scsi_target backend;
+
+    memset(&backend, 0, sizeof(backend));
+    pstrcpy(backend.vhost_wwpn, sizeof(backend.vhost_wwpn), vs->vs.conf->wwpn);
+    ioctl(vs->dev.control, VHOST_SCSI_CLEAR_ENDPOINT, &backend);
+}
+
+static int vhost_scsi_start(VHostSCSI *vs, VirtIODevice *vdev)
+{
+    int ret, abi_version;
+
+    if (!vdev->binding->set_guest_notifiers) {
+        error_report("binding does not support guest notifiers");
+        return -ENOSYS;
+    }
+
+    ret = ioctl(vs->dev.control, VHOST_SCSI_GET_ABI_VERSION, &abi_version);
+    if (ret < 0) {
+        return -errno;
+    }
+    if (abi_version > VHOST_SCSI_ABI_VERSION) {
+        error_report("vhost-scsi: The running tcm_vhost kernel abi_version:"
+                     " %d is greater than vhost_scsi userspace supports: %d, please"
+                     " upgrade your version of QEMU\n", abi_version,
+                     VHOST_SCSI_ABI_VERSION);
+        return -ENOSYS;
+    }
+
+    ret = vhost_dev_enable_notifiers(&vs->dev, vdev);
+    if (ret < 0) {
+        return ret;
+    }
+
+    ret = vhost_dev_start(&vs->dev, vdev);
+    if (ret < 0) {
+        error_report("Error start vhost dev");
+        goto err_notifiers;
+    }
+
+    ret = vhost_scsi_set_endpoint(vdev);
+    if (ret < 0) {
+        error_report("Error set vhost-scsi endpoint");
+        goto err_vhost_stop;
+    }
+
+    ret = vdev->binding->set_guest_notifiers(vdev->binding_opaque, vs->dev.nvqs, true);
+    if (ret < 0) {
+        error_report("Error binding guest notifier");
+        goto err_endpoint;
+    }
+    return ret;
+
+err_endpoint:
+    vhost_scsi_clear_endpoint(vdev);
+err_vhost_stop:
+    vhost_dev_stop(&vs->dev, vdev);
+err_notifiers:
+    vhost_dev_disable_notifiers(&vs->dev, vdev);
+    return ret;
+}
+
+static void vhost_scsi_stop(VHostSCSI *vs, VirtIODevice *vdev)
+{
+    int ret = 0;
+
+    if (!vdev->binding->set_guest_notifiers) {
+        ret = vdev->binding->set_guest_notifiers(vdev->binding_opaque,
+                                                 vs->dev.nvqs, false);
+        if (ret < 0) {
+                error_report("vhost guest notifier cleanup failed: %d\n", ret);
+        }
+    }
+    assert(ret >= 0);
+
+    vhost_scsi_clear_endpoint(vdev);
+    vhost_dev_stop(&vs->dev, vdev);
+    vhost_dev_disable_notifiers(&vs->dev, vdev);
+}
+
+static void vhost_scsi_set_config(VirtIODevice *vdev,
+                                  const uint8_t *config)
+{
+    VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
+    VHostSCSI *vs = (VHostSCSI *)vdev;
+
+    if ((uint32_t) ldl_raw(&scsiconf->sense_size) != vs->vs.sense_size ||
+        (uint32_t) ldl_raw(&scsiconf->cdb_size) != vs->vs.cdb_size) {
+        error_report("vhost-scsi does not support changing the sense data and CDB sizes");
+        exit(1);
+    }
+}
+
+static void vhost_scsi_set_status(VirtIODevice *vdev, uint8_t val)
+{
+    VHostSCSI *vs = (VHostSCSI *)vdev;
+    bool start = (val & VIRTIO_CONFIG_S_DRIVER_OK);
+
+    if (vs->dev.started == start) {
+        return;
+    }
+
+    if (start) {
+        int ret;
+
+        ret = vhost_scsi_start(vs, vdev);
+        if (ret < 0) {
+            error_report("virtio-scsi: unable to start vhost: %s\n",
+                         strerror(-ret));
+
+            /* There is no userspace virtio-scsi fallback so exit */
+            exit(1);
+        }
+    } else {
+        vhost_scsi_stop(vs, vdev);
+    }
+}
+
+static void vhost_scsi_guest_notifier_mask(VirtIODevice *vdev, int idx,
+                                           bool mask)
+{
+    VHostSCSI *vs = (VHostSCSI *)vdev;
+
+    vhost_virtqueue_mask(&vs->dev, vdev, idx, mask);
+}
+
+static bool vhost_scsi_guest_notifier_pending(VirtIODevice *vdev, int idx)
+{
+    VHostSCSI *vs = (VHostSCSI *)vdev;
+
+    return vhost_virtqueue_pending(&vs->dev, idx);
+}
+
+VirtIODevice *vhost_scsi_init(DeviceState *dev, VirtIOSCSIConf *proxyconf)
+{
+    VHostSCSI *vs;
+    int vhostfd = -1;
+    int ret;
+
+    if (!proxyconf->wwpn) {
+        error_report("vhost-scsi: missing wwpn\n");
+        return NULL;
+    }
+
+    if (proxyconf->vhostfd) {
+        vhostfd = monitor_handle_fd_param(cur_mon, proxyconf->vhostfd);
+        if (vhostfd == -1) {
+            error_report("vhost-scsi: unable to parse vhostfd\n");
+            return NULL;
+        }
+    }
+
+    vs = (VHostSCSI *)virtio_scsi_init_common(dev, proxyconf,
+                                              sizeof(VHostSCSI));
+
+    vs->vs.vdev.set_config = vhost_scsi_set_config;
+    vs->vs.vdev.set_status = vhost_scsi_set_status;
+    vs->vs.vdev.guest_notifier_mask = vhost_scsi_guest_notifier_mask;
+    vs->vs.vdev.guest_notifier_pending = vhost_scsi_guest_notifier_pending;
+
+    vs->dev.nvqs = VHOST_SCSI_VQ_NUM_FIXED + vs->vs.conf->num_queues;
+    vs->dev.vqs = g_new(struct vhost_virtqueue, vs->dev.nvqs);
+    vs->dev.vq_index = 0;
+
+    ret = vhost_dev_init(&vs->dev, vhostfd, "/dev/vhost-scsi", true);
+    if (ret < 0) {
+        error_report("vhost-scsi: vhost initialization failed: %s\n",
+                strerror(-ret));
+        return NULL;
+    }
+    vs->dev.backend_features = 0;
+    vs->dev.acked_features = 1ULL << VIRTIO_SCSI_F_HOTPLUG;
+
+    error_setg(&vs->migration_blocker,
+            "vhost-scsi does not support migration");
+    migrate_add_blocker(vs->migration_blocker);
+
+    return &vs->vs.vdev;
+}
+
+void vhost_scsi_exit(VirtIODevice *vdev)
+{
+    VHostSCSI *vs = (VHostSCSI *)vdev;
+    migrate_del_blocker(vs->migration_blocker);
+    error_free(vs->migration_blocker);
+
+    /* This will stop vhost backend. */
+    vhost_scsi_set_status(vdev, 0);
+    g_free(vs->dev.vqs);
+    virtio_cleanup(vdev);
+}
+
diff --git a/hw/vhost-scsi.h b/hw/vhost-scsi.h
new file mode 100644
index 0000000..2e389b4
--- /dev/null
+++ b/hw/vhost-scsi.h
@@ -0,0 +1,65 @@
+/*
+ * vhost_scsi host device
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Stefan Hajnoczi   <stefanha@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef VHOST_SCSI_H
+#define VHOST_SCSI_H
+
+#include "qemu-common.h"
+#include "qdev.h"
+#include "virtio-scsi.h"
+
+/*
+ * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
+ *
+ * ABI Rev 0: July 2012 version starting point for v3.6-rc merge candidate +
+ *            RFC-v2 vhost-scsi userspace.  Add GET_ABI_VERSION ioctl usage
+ * ABI Rev 1: January 2013. Ignore vhost_tpgt filed in struct vhost_scsi_target.
+ * 	      All the targets under vhost_wwpn can be seen and used by guest.
+ */
+
+#define VHOST_SCSI_ABI_VERSION 1
+
+/* TODO #include <linux/vhost.h> properly */
+/* For VHOST_SCSI_SET_ENDPOINT/VHOST_SCSI_CLEAR_ENDPOINT ioctl */
+struct vhost_scsi_target {
+    int abi_version;
+    char vhost_wwpn[224];
+    unsigned short vhost_tpgt;
+    unsigned short reserved;
+};
+
+enum vhost_scsi_vq_list {
+    VHOST_SCSI_VQ_CONTROL = 0,
+    VHOST_SCSI_VQ_EVENT = 1,
+    VHOST_SCSI_VQ_NUM_FIXED = 2,
+};
+
+#define VHOST_VIRTIO 0xAF
+#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 DEFINE_VHOST_SCSI_PROPERTIES(_state, _features_field, _conf_field) \
+    DEFINE_VIRTIO_COMMON_FEATURES(_state, _features_field), \
+    DEFINE_PROP_STRING("vhostfd", _state, _conf_field.vhostfd), \
+    DEFINE_PROP_STRING("wwpn", _state, _conf_field.wwpn), \
+    DEFINE_PROP_UINT32("num_queues", _state, _conf_field.num_queues, 1), \
+    DEFINE_PROP_UINT32("max_sectors", _state, _conf_field.max_sectors, 0xFFFF), \
+    DEFINE_PROP_UINT32("cmd_per_lun", _state, _conf_field.cmd_per_lun, 128), \
+    DEFINE_PROP_BIT("hotplug", _state, _features_field, VIRTIO_SCSI_F_HOTPLUG, true)
+
+VirtIODevice *vhost_scsi_init(DeviceState *dev, VirtIOSCSIConf *proxyconf);
+void vhost_scsi_exit(VirtIODevice *vdev);
+
+
+#endif
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 39c1966..281a7e2 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -22,6 +22,7 @@
 #include "hw/virtio-net.h"
 #include "hw/virtio-serial.h"
 #include "hw/virtio-scsi.h"
+#include "hw/vhost-scsi.h"
 #include "hw/pci/pci.h"
 #include "qemu/error-report.h"
 #include "hw/pci/msi.h"
@@ -1316,6 +1317,64 @@ static const TypeInfo virtio_scsi_info = {
     .class_init    = virtio_scsi_class_init,
 };
 
+#ifdef CONFIG_VHOST_SCSI
+static int vhost_scsi_init_pci(PCIDevice *pci_dev)
+{
+    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
+    VirtIODevice *vdev;
+
+    vdev = vhost_scsi_init(&pci_dev->qdev, &proxy->scsi);
+    if (!vdev) {
+        return -EINVAL;
+    }
+
+    vdev->nvectors = proxy->nvectors == DEV_NVECTORS_UNSPECIFIED
+                                        ? proxy->scsi.num_queues + 3
+                                        : proxy->nvectors;
+    virtio_init_pci(proxy, vdev);
+
+    /* make the actual value visible */
+    proxy->nvectors = vdev->nvectors;
+    return 0;
+}
+
+static void vhost_scsi_exit_pci(PCIDevice *pci_dev)
+{
+    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
+
+    vhost_scsi_exit(proxy->vdev);
+    virtio_exit_pci(pci_dev);
+}
+
+static Property vhost_scsi_properties[] = {
+    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, DEV_NVECTORS_UNSPECIFIED),
+    DEFINE_VHOST_SCSI_PROPERTIES(VirtIOPCIProxy, host_features, scsi),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void vhost_scsi_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->init = vhost_scsi_init_pci;
+    k->exit = vhost_scsi_exit_pci;
+    k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
+    k->device_id = PCI_DEVICE_ID_VIRTIO_SCSI;
+    k->revision = 0x00;
+    k->class_id = PCI_CLASS_STORAGE_SCSI;
+    dc->reset = virtio_pci_reset;
+    dc->props = vhost_scsi_properties;
+}
+
+static const TypeInfo vhost_scsi_info = {
+    .name          = "vhost-scsi-pci",
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(VirtIOPCIProxy),
+    .class_init    = vhost_scsi_class_init,
+};
+#endif
+
 #ifdef CONFIG_VIRTFS
 static int virtio_9p_init_pci(PCIDevice *pci_dev)
 {
@@ -1520,6 +1579,9 @@ static void virtio_pci_register_types(void)
     type_register_static(&virtio_serial_info);
     type_register_static(&virtio_balloon_info);
     type_register_static(&virtio_scsi_info);
+#ifdef CONFIG_VHOST_SCSI
+    type_register_static(&vhost_scsi_info);
+#endif
     type_register_static(&virtio_rng_info);
     type_register_static(&virtio_pci_bus_info);
     type_register_static(&virtio_pci_info);
diff --git a/hw/virtio-scsi.h b/hw/virtio-scsi.h
index 5acb721..bbcf206 100644
--- a/hw/virtio-scsi.h
+++ b/hw/virtio-scsi.h
@@ -141,6 +141,8 @@ struct VirtIOSCSIConf {
     uint32_t num_queues;
     uint32_t max_sectors;
     uint32_t cmd_per_lun;
+    char *vhostfd;
+    char *wwpn;
 };
 
 typedef struct VirtIOSCSICommon {
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH V3 WIP 2/3] vhost-scsi: new device supporting the tcm_vhost Linux kernel module

[Stefan Hajnoczi]

On Tue, Mar 19, 2013 at 08:34:44AM +0800, Asias He wrote:
> +static void vhost_scsi_stop(VHostSCSI *vs, VirtIODevice *vdev)
> +{
> +    int ret = 0;
> +
> +    if (!vdev->binding->set_guest_notifiers) {
> +        ret = vdev->binding->set_guest_notifiers(vdev->binding_opaque,
> +                                                 vs->dev.nvqs, false);
> +        if (ret < 0) {
> +                error_report("vhost guest notifier cleanup failed: %d\n", ret);

Indentation.  scripts/checkpatch.pl should catch this.

> +        }
> +    }
> +    assert(ret >= 0);
> +
> +    vhost_scsi_clear_endpoint(vdev);
> +    vhost_dev_stop(&vs->dev, vdev);
> +    vhost_dev_disable_notifiers(&vs->dev, vdev);
> +}
> +
> +static void vhost_scsi_set_config(VirtIODevice *vdev,
> +                                  const uint8_t *config)
> +{
> +    VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
> +    VHostSCSI *vs = (VHostSCSI *)vdev;
> +
> +    if ((uint32_t) ldl_raw(&scsiconf->sense_size) != vs->vs.sense_size ||
> +        (uint32_t) ldl_raw(&scsiconf->cdb_size) != vs->vs.cdb_size) {
> +        error_report("vhost-scsi does not support changing the sense data and CDB sizes");
> +        exit(1);

Guest-triggerable exits can be used as a denial of service - especially
under nested virtualization where killing the L1 hypervisor would kill
all L2 guests!

I would just log a warning here.

> +    }
> +}
> +
> +static void vhost_scsi_set_status(VirtIODevice *vdev, uint8_t val)
> +{
> +    VHostSCSI *vs = (VHostSCSI *)vdev;
> +    bool start = (val & VIRTIO_CONFIG_S_DRIVER_OK);
> +
> +    if (vs->dev.started == start) {
> +        return;
> +    }
> +
> +    if (start) {
> +        int ret;
> +
> +        ret = vhost_scsi_start(vs, vdev);
> +        if (ret < 0) {
> +            error_report("virtio-scsi: unable to start vhost: %s\n",
> +                         strerror(-ret));
> +
> +            /* There is no userspace virtio-scsi fallback so exit */
> +            exit(1);

It's questionable whether to kill the guest or simply disable this
virtio-scsi-pci adapter.  Fine for now but we may want to allow a policy
here in the future.

> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 39c1966..281a7e2 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -22,6 +22,7 @@
>  #include "hw/virtio-net.h"
>  #include "hw/virtio-serial.h"
>  #include "hw/virtio-scsi.h"
> +#include "hw/vhost-scsi.h"

Can this header be included unconditionally?  It uses _IOW() which may
not be available on all host platforms.

[Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Asias He]

---
 hw/vhost.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/vhost.c b/hw/vhost.c
index 4d6aee3..0c52ec4 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -421,10 +421,12 @@ static void vhost_set_memory(MemoryListener *listener,
         return;
     }
 
+#if 0
     if (dev->started) {
         r = vhost_verify_ring_mappings(dev, start_addr, size);
         assert(r >= 0);
     }
+#endif
 
     if (!dev->log_enabled) {
         r = ioctl(dev->control, VHOST_SET_MEM_TABLE, dev->mem);
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Stefan Hajnoczi]

On Tue, Mar 19, 2013 at 08:34:45AM +0800, Asias He wrote:
> ---
>  hw/vhost.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/vhost.c b/hw/vhost.c
> index 4d6aee3..0c52ec4 100644
> --- a/hw/vhost.c
> +++ b/hw/vhost.c
> @@ -421,10 +421,12 @@ static void vhost_set_memory(MemoryListener *listener,
>          return;
>      }
>  
> +#if 0
>      if (dev->started) {
>          r = vhost_verify_ring_mappings(dev, start_addr, size);
>          assert(r >= 0);
>      }
> +#endif

Please add a comment to explain why.

Stefan

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Asias He]

On Tue, Mar 19, 2013 at 09:40:57AM +0100, Stefan Hajnoczi wrote:
> On Tue, Mar 19, 2013 at 08:34:45AM +0800, Asias He wrote:
> > ---
> >  hw/vhost.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/vhost.c b/hw/vhost.c
> > index 4d6aee3..0c52ec4 100644
> > --- a/hw/vhost.c
> > +++ b/hw/vhost.c
> > @@ -421,10 +421,12 @@ static void vhost_set_memory(MemoryListener *listener,
> >          return;
> >      }
> >  
> > +#if 0
> >      if (dev->started) {
> >          r = vhost_verify_ring_mappings(dev, start_addr, size);
> >          assert(r >= 0);
> >      }
> > +#endif
> 
> Please add a comment to explain why.

Apparently, this is just a workaround for testing not for merge.

> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Asias

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Nicholas A. Bellinger]

On Tue, 2013-03-19 at 09:40 +0100, Stefan Hajnoczi wrote:
> On Tue, Mar 19, 2013 at 08:34:45AM +0800, Asias He wrote:
> > ---
> >  hw/vhost.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/vhost.c b/hw/vhost.c
> > index 4d6aee3..0c52ec4 100644
> > --- a/hw/vhost.c
> > +++ b/hw/vhost.c
> > @@ -421,10 +421,12 @@ static void vhost_set_memory(MemoryListener *listener,
> >          return;
> >      }
> >  
> > +#if 0
> >      if (dev->started) {
> >          r = vhost_verify_ring_mappings(dev, start_addr, size);
> >          assert(r >= 0);
> >      }
> > +#endif
> 
> Please add a comment to explain why.
> 

Btw, the output that Asias added in the failure case at the behest of
MST is here:

http://www.spinics.net/lists/target-devel/msg04077.html

MST seemed to think it may be a bug in cpu_physical_memory_map, but as
this worked with the original vhost-scsi code it would seem to indicate
something else at fault..

I'll be comparing what the original code did vs. vhost-scsi-pci to track
this down, but any extra ideas to track is down is appreciated.  ;)

--nab

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Michael S. Tsirkin]

On Tue, Mar 19, 2013 at 06:57:08PM -0700, Nicholas A. Bellinger wrote:
> On Tue, 2013-03-19 at 09:40 +0100, Stefan Hajnoczi wrote:
> > On Tue, Mar 19, 2013 at 08:34:45AM +0800, Asias He wrote:
> > > ---
> > >  hw/vhost.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/hw/vhost.c b/hw/vhost.c
> > > index 4d6aee3..0c52ec4 100644
> > > --- a/hw/vhost.c
> > > +++ b/hw/vhost.c
> > > @@ -421,10 +421,12 @@ static void vhost_set_memory(MemoryListener *listener,
> > >          return;
> > >      }
> > >  
> > > +#if 0
> > >      if (dev->started) {
> > >          r = vhost_verify_ring_mappings(dev, start_addr, size);
> > >          assert(r >= 0);
> > >      }
> > > +#endif
> > 
> > Please add a comment to explain why.
> > 
> 
> Btw, the output that Asias added in the failure case at the behest of
> MST is here:
> 
> http://www.spinics.net/lists/target-devel/msg04077.html

Yes I suspected we could get l > ring_size, but this is
not the case here.

> MST seemed to think it may be a bug in cpu_physical_memory_map, but as
> this worked with the original vhost-scsi code it would seem to indicate
> something else at fault..
> 
> I'll be comparing what the original code did vs. vhost-scsi-pci to track
> this down, but any extra ideas to track is down is appreciated.  ;)
> 
> --nab

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Nicholas A. Bellinger]

On Wed, 2013-03-20 at 11:51 +0200, Michael S. Tsirkin wrote:
> On Tue, Mar 19, 2013 at 06:57:08PM -0700, Nicholas A. Bellinger wrote:
> > On Tue, 2013-03-19 at 09:40 +0100, Stefan Hajnoczi wrote:
> > > On Tue, Mar 19, 2013 at 08:34:45AM +0800, Asias He wrote:
> > > > ---
> > > >  hw/vhost.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/hw/vhost.c b/hw/vhost.c
> > > > index 4d6aee3..0c52ec4 100644
> > > > --- a/hw/vhost.c
> > > > +++ b/hw/vhost.c
> > > > @@ -421,10 +421,12 @@ static void vhost_set_memory(MemoryListener *listener,
> > > >          return;
> > > >      }
> > > >  
> > > > +#if 0
> > > >      if (dev->started) {
> > > >          r = vhost_verify_ring_mappings(dev, start_addr, size);
> > > >          assert(r >= 0);
> > > >      }
> > > > +#endif
> > > 
> > > Please add a comment to explain why.
> > > 
> > 
> > Btw, the output that Asias added in the failure case at the behest of
> > MST is here:
> > 
> > http://www.spinics.net/lists/target-devel/msg04077.html
> 
> Yes I suspected we could get l > ring_size, but this is
> not the case here.
> 

Hi MST & Co,

A quick update here..

So this issue appears to be related to performing the
vhost_verify_ring_mappings() call after vhost_dev_unassign_memory() has
been invoked with vhost_set_memory(..., add=false).

AFAICT from the logs below, things appear to work as expected when
vhost_verify_ring_mappings() is called only for the
vhost_set_memory(..., add=true) case.

Calling vhost_verify_ring_mappings() when dev->started == true +
vhost_set_memory(..., add=false) appears to be a bug caused by fallout
from:

commit 24f4fe345c1b80bab1ee18573914123d8028a9e6
Author: Michael S. Tsirkin <mst@redhat.com>
Date:   Tue Dec 25 17:41:07 2012 +0200

    vhost: set started flag while start is in progress

I'm including the following patch in the forth-coming vhost-scsi series.
Please let me know if you have any concerns.

diff --git a/hw/vhost.c b/hw/vhost.c
index 4d6aee3..687a689 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -421,7 +421,7 @@ static void vhost_set_memory(MemoryListener *listener,
         return;
     }
 
-    if (dev->started) {
+    if (dev->started && add) {
         r = vhost_verify_ring_mappings(dev, start_addr, size);
         assert(r >= 0);
     }

Thanks!

--nab

vhost_set_memory: section: 0x7f2249986b60 section->size: 2146697216 add: 0
Before vhost_verify_ring_mappings: start_addr: c0000 size: 2146697216
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Calling l: 5124 for start_addr: c0000 for vq 2
Unable to map ring buffer for ring 2
l: 4096 ring_size: 5124
vhost_set_memory: section: 0x7f2249986aa0 section->size: 32768 add: 1
Before vhost_verify_ring_mappings: start_addr: c0000 size: 32768
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 2 ring_phys: ee000 ring_size: 5124
vhost_set_memory: section: 0x7f2249986aa0 section->size: 2146664448 add: 1
Before vhost_verify_ring_mappings: start_addr: c8000 size: 2146664448
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Calling l: 5124 for start_addr: c8000 for vq 2
vhost_set_memory: section: 0x7f2249986b60 section->size: 32768 add: 0
Before vhost_verify_ring_mappings: start_addr: c0000 size: 32768
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 2 ring_phys: ee000 ring_size: 5124
vhost_set_memory: section: 0x7f2249986b60 section->size: 2146664448 add: 0
Before vhost_verify_ring_mappings: start_addr: c8000 size: 2146664448
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Calling l: 5124 for start_addr: c8000 for vq 2
Unable to map ring buffer for ring 2
l: 0 ring_size: 5124
vhost_set_memory: section: 0x7f2249986aa0 section->size: 36864 add: 1
Before vhost_verify_ring_mappings: start_addr: c0000 size: 36864
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 2 ring_phys: ee000 ring_size: 5124
vhost_set_memory: section: 0x7f2249986aa0 section->size: 2146660352 add: 1
Before vhost_verify_ring_mappings: start_addr: c9000 size: 2146660352
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Calling l: 5124 for start_addr: c9000 for vq 2
vhost_set_memory: section: 0x7f2249986b60 section->size: 2146660352 add: 0
Before vhost_verify_ring_mappings: start_addr: c9000 size: 2146660352
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Calling l: 5124 for start_addr: c9000 for vq 2
Unable to map ring buffer for ring 2
l: 0 ring_size: 5124
vhost_set_memory: section: 0x7f2249986aa0 section->size: 159744 add: 1
Before vhost_verify_ring_mappings: start_addr: c9000 size: 159744
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Calling l: 5124 for start_addr: c9000 for vq 2
vhost_set_memory: section: 0x7f2249986aa0 section->size: 65536 add: 1
Before vhost_verify_ring_mappings: start_addr: f0000 size: 65536
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 2 ring_phys: ee000 ring_size: 5124
vhost_set_memory: section: 0x7f2249986aa0 section->size: 2146435072 add: 1
Before vhost_verify_ring_mappings: start_addr: 100000 size: 2146435072
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 2 ring_phys: ee000 ring_size: 5124

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Michael S. Tsirkin]

On Wed, Mar 27, 2013 at 02:31:27PM -0700, Nicholas A. Bellinger wrote:
> On Wed, 2013-03-20 at 11:51 +0200, Michael S. Tsirkin wrote:
> > On Tue, Mar 19, 2013 at 06:57:08PM -0700, Nicholas A. Bellinger wrote:
> > > On Tue, 2013-03-19 at 09:40 +0100, Stefan Hajnoczi wrote:
> > > > On Tue, Mar 19, 2013 at 08:34:45AM +0800, Asias He wrote:
> > > > > ---
> > > > >  hw/vhost.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > > 
> > > > > diff --git a/hw/vhost.c b/hw/vhost.c
> > > > > index 4d6aee3..0c52ec4 100644
> > > > > --- a/hw/vhost.c
> > > > > +++ b/hw/vhost.c
> > > > > @@ -421,10 +421,12 @@ static void vhost_set_memory(MemoryListener *listener,
> > > > >          return;
> > > > >      }
> > > > >  
> > > > > +#if 0
> > > > >      if (dev->started) {
> > > > >          r = vhost_verify_ring_mappings(dev, start_addr, size);
> > > > >          assert(r >= 0);
> > > > >      }
> > > > > +#endif
> > > > 
> > > > Please add a comment to explain why.
> > > > 
> > > 
> > > Btw, the output that Asias added in the failure case at the behest of
> > > MST is here:
> > > 
> > > http://www.spinics.net/lists/target-devel/msg04077.html
> > 
> > Yes I suspected we could get l > ring_size, but this is
> > not the case here.
> > 
> 
> Hi MST & Co,
> 
> A quick update here..
> 
> So this issue appears to be related to performing the
> vhost_verify_ring_mappings() call after vhost_dev_unassign_memory() has
> been invoked with vhost_set_memory(..., add=false).
> 
> AFAICT from the logs below, things appear to work as expected when
> vhost_verify_ring_mappings() is called only for the
> vhost_set_memory(..., add=true) case.
> 
> Calling vhost_verify_ring_mappings() when dev->started == true +
> vhost_set_memory(..., add=false) appears to be a bug caused by fallout
> from:
> 
> commit 24f4fe345c1b80bab1ee18573914123d8028a9e6
> Author: Michael S. Tsirkin <mst@redhat.com>
> Date:   Tue Dec 25 17:41:07 2012 +0200
> 
>     vhost: set started flag while start is in progress
> 
> I'm including the following patch in the forth-coming vhost-scsi series.
> Please let me know if you have any concerns.
> 
> diff --git a/hw/vhost.c b/hw/vhost.c
> index 4d6aee3..687a689 100644
> --- a/hw/vhost.c
> +++ b/hw/vhost.c
> @@ -421,7 +421,7 @@ static void vhost_set_memory(MemoryListener *listener,
>          return;
>      }
>  
> -    if (dev->started) {
> +    if (dev->started && add) {
>          r = vhost_verify_ring_mappings(dev, start_addr, size);
>          assert(r >= 0);
>      }
> 
> Thanks!
> 
> --nab

Sorry NAK,
I think this will shut down too much stuff:
the main reason to check is when we delete a region.

> vhost_set_memory: section: 0x7f2249986b60 section->size: 2146697216 add: 0
> Before vhost_verify_ring_mappings: start_addr: c0000 size: 2146697216
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Calling l: 5124 for start_addr: c0000 for vq 2
> Unable to map ring buffer for ring 2
> l: 4096 ring_size: 5124
> vhost_set_memory: section: 0x7f2249986aa0 section->size: 32768 add: 1
> Before vhost_verify_ring_mappings: start_addr: c0000 size: 32768
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 2 ring_phys: ee000 ring_size: 5124
> vhost_set_memory: section: 0x7f2249986aa0 section->size: 2146664448 add: 1
> Before vhost_verify_ring_mappings: start_addr: c8000 size: 2146664448
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Calling l: 5124 for start_addr: c8000 for vq 2
> vhost_set_memory: section: 0x7f2249986b60 section->size: 32768 add: 0
> Before vhost_verify_ring_mappings: start_addr: c0000 size: 32768
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 2 ring_phys: ee000 ring_size: 5124
> vhost_set_memory: section: 0x7f2249986b60 section->size: 2146664448 add: 0
> Before vhost_verify_ring_mappings: start_addr: c8000 size: 2146664448
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Calling l: 5124 for start_addr: c8000 for vq 2
> Unable to map ring buffer for ring 2
> l: 0 ring_size: 5124
> vhost_set_memory: section: 0x7f2249986aa0 section->size: 36864 add: 1
> Before vhost_verify_ring_mappings: start_addr: c0000 size: 36864
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 2 ring_phys: ee000 ring_size: 5124
> vhost_set_memory: section: 0x7f2249986aa0 section->size: 2146660352 add: 1
> Before vhost_verify_ring_mappings: start_addr: c9000 size: 2146660352
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Calling l: 5124 for start_addr: c9000 for vq 2
> vhost_set_memory: section: 0x7f2249986b60 section->size: 2146660352 add: 0
> Before vhost_verify_ring_mappings: start_addr: c9000 size: 2146660352
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Calling l: 5124 for start_addr: c9000 for vq 2
> Unable to map ring buffer for ring 2
> l: 0 ring_size: 5124
> vhost_set_memory: section: 0x7f2249986aa0 section->size: 159744 add: 1
> Before vhost_verify_ring_mappings: start_addr: c9000 size: 159744
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Calling l: 5124 for start_addr: c9000 for vq 2
> vhost_set_memory: section: 0x7f2249986aa0 section->size: 65536 add: 1
> Before vhost_verify_ring_mappings: start_addr: f0000 size: 65536
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 2 ring_phys: ee000 ring_size: 5124
> vhost_set_memory: section: 0x7f2249986aa0 section->size: 2146435072 add: 1
> Before vhost_verify_ring_mappings: start_addr: 100000 size: 2146435072
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 2 ring_phys: ee000 ring_size: 5124

I still do not understand how this happened.  Somehow a memory region
was deleted after vhost_dev_start but before vhost_virtqueue_start was
called?
Can you set a breakpoint there and see please?

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Nicholas A. Bellinger]

On Wed, 2013-03-27 at 23:56 +0200, Michael S. Tsirkin wrote:
> On Wed, Mar 27, 2013 at 02:31:27PM -0700, Nicholas A. Bellinger wrote:
> > On Wed, 2013-03-20 at 11:51 +0200, Michael S. Tsirkin wrote:
> > > On Tue, Mar 19, 2013 at 06:57:08PM -0700, Nicholas A. Bellinger wrote:
> > > > On Tue, 2013-03-19 at 09:40 +0100, Stefan Hajnoczi wrote:
> > > > > On Tue, Mar 19, 2013 at 08:34:45AM +0800, Asias He wrote:
> > > > > > ---
> > > > > >  hw/vhost.c | 2 ++
> > > > > >  1 file changed, 2 insertions(+)
> > > > > > 
> > > > > > diff --git a/hw/vhost.c b/hw/vhost.c
> > > > > > index 4d6aee3..0c52ec4 100644
> > > > > > --- a/hw/vhost.c
> > > > > > +++ b/hw/vhost.c
> > > > > > @@ -421,10 +421,12 @@ static void vhost_set_memory(MemoryListener *listener,
> > > > > >          return;
> > > > > >      }
> > > > > >  
> > > > > > +#if 0
> > > > > >      if (dev->started) {
> > > > > >          r = vhost_verify_ring_mappings(dev, start_addr, size);
> > > > > >          assert(r >= 0);
> > > > > >      }
> > > > > > +#endif
> > > > > 
> > > > > Please add a comment to explain why.
> > > > > 
> > > > 
> > > > Btw, the output that Asias added in the failure case at the behest of
> > > > MST is here:
> > > > 
> > > > http://www.spinics.net/lists/target-devel/msg04077.html
> > > 
> > > Yes I suspected we could get l > ring_size, but this is
> > > not the case here.
> > > 
> > 
> > Hi MST & Co,
> > 
> > A quick update here..
> > 
> > So this issue appears to be related to performing the
> > vhost_verify_ring_mappings() call after vhost_dev_unassign_memory() has
> > been invoked with vhost_set_memory(..., add=false).
> > 
> > AFAICT from the logs below, things appear to work as expected when
> > vhost_verify_ring_mappings() is called only for the
> > vhost_set_memory(..., add=true) case.
> > 
> > Calling vhost_verify_ring_mappings() when dev->started == true +
> > vhost_set_memory(..., add=false) appears to be a bug caused by fallout
> > from:
> > 
> > commit 24f4fe345c1b80bab1ee18573914123d8028a9e6
> > Author: Michael S. Tsirkin <mst@redhat.com>
> > Date:   Tue Dec 25 17:41:07 2012 +0200
> > 
> >     vhost: set started flag while start is in progress
> > 
> > I'm including the following patch in the forth-coming vhost-scsi series.
> > Please let me know if you have any concerns.
> > 
> > diff --git a/hw/vhost.c b/hw/vhost.c
> > index 4d6aee3..687a689 100644
> > --- a/hw/vhost.c
> > +++ b/hw/vhost.c
> > @@ -421,7 +421,7 @@ static void vhost_set_memory(MemoryListener *listener,
> >          return;
> >      }
> >  
> > -    if (dev->started) {
> > +    if (dev->started && add) {
> >          r = vhost_verify_ring_mappings(dev, start_addr, size);
> >          assert(r >= 0);
> >      }
> > 
> > Thanks!
> > 
> > --nab
> 
> Sorry NAK,
> I think this will shut down too much stuff:
> the main reason to check is when we delete a region.
> 

<nod>

> > vhost_set_memory: section: 0x7f2249986b60 section->size: 2146697216 add: 0
> > Before vhost_verify_ring_mappings: start_addr: c0000 size: 2146697216
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Calling l: 5124 for start_addr: c0000 for vq 2
> > Unable to map ring buffer for ring 2
> > l: 4096 ring_size: 5124
> > vhost_set_memory: section: 0x7f2249986aa0 section->size: 32768 add: 1
> > Before vhost_verify_ring_mappings: start_addr: c0000 size: 32768
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 2 ring_phys: ee000 ring_size: 5124
> > vhost_set_memory: section: 0x7f2249986aa0 section->size: 2146664448 add: 1
> > Before vhost_verify_ring_mappings: start_addr: c8000 size: 2146664448
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Calling l: 5124 for start_addr: c8000 for vq 2
> > vhost_set_memory: section: 0x7f2249986b60 section->size: 32768 add: 0
> > Before vhost_verify_ring_mappings: start_addr: c0000 size: 32768
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 2 ring_phys: ee000 ring_size: 5124
> > vhost_set_memory: section: 0x7f2249986b60 section->size: 2146664448 add: 0
> > Before vhost_verify_ring_mappings: start_addr: c8000 size: 2146664448
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Calling l: 5124 for start_addr: c8000 for vq 2
> > Unable to map ring buffer for ring 2
> > l: 0 ring_size: 5124
> > vhost_set_memory: section: 0x7f2249986aa0 section->size: 36864 add: 1
> > Before vhost_verify_ring_mappings: start_addr: c0000 size: 36864
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 2 ring_phys: ee000 ring_size: 5124
> > vhost_set_memory: section: 0x7f2249986aa0 section->size: 2146660352 add: 1
> > Before vhost_verify_ring_mappings: start_addr: c9000 size: 2146660352
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Calling l: 5124 for start_addr: c9000 for vq 2
> > vhost_set_memory: section: 0x7f2249986b60 section->size: 2146660352 add: 0
> > Before vhost_verify_ring_mappings: start_addr: c9000 size: 2146660352
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Calling l: 5124 for start_addr: c9000 for vq 2
> > Unable to map ring buffer for ring 2
> > l: 0 ring_size: 5124
> > vhost_set_memory: section: 0x7f2249986aa0 section->size: 159744 add: 1
> > Before vhost_verify_ring_mappings: start_addr: c9000 size: 159744
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Calling l: 5124 for start_addr: c9000 for vq 2
> > vhost_set_memory: section: 0x7f2249986aa0 section->size: 65536 add: 1
> > Before vhost_verify_ring_mappings: start_addr: f0000 size: 65536
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 2 ring_phys: ee000 ring_size: 5124
> > vhost_set_memory: section: 0x7f2249986aa0 section->size: 2146435072 add: 1
> > Before vhost_verify_ring_mappings: start_addr: 100000 size: 2146435072
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ee000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 2 ring_phys: ee000 ring_size: 5124
> 
> I still do not understand how this happened.  Somehow a memory region
> was deleted after vhost_dev_start but before vhost_virtqueue_start was
> called?

Not sure..

To clarify, this is only happening during seabios setup+scan of
virtio-scsi, and not during normal virtio_scsi LLD operation.

> Can you set a breakpoint there and see please?
> 
> 

Sure, will have a look here.

Thanks,

--nab

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Nicholas A. Bellinger]

On Wed, 2013-03-27 at 15:33 -0700, Nicholas A. Bellinger wrote:
> On Wed, 2013-03-27 at 23:56 +0200, Michael S. Tsirkin wrote:
> > On Wed, Mar 27, 2013 at 02:31:27PM -0700, Nicholas A. Bellinger wrote:
> > > On Wed, 2013-03-20 at 11:51 +0200, Michael S. Tsirkin wrote:
> > > > On Tue, Mar 19, 2013 at 06:57:08PM -0700, Nicholas A. Bellinger wrote:
> > > > > On Tue, 2013-03-19 at 09:40 +0100, Stefan Hajnoczi wrote:
> > > > > > On Tue, Mar 19, 2013 at 08:34:45AM +0800, Asias He wrote:
> > > > > > > ---
> > > > > > >  hw/vhost.c | 2 ++
> > > > > > >  1 file changed, 2 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/hw/vhost.c b/hw/vhost.c
> > > > > > > index 4d6aee3..0c52ec4 100644
> > > > > > > --- a/hw/vhost.c
> > > > > > > +++ b/hw/vhost.c
> > > > > > > @@ -421,10 +421,12 @@ static void vhost_set_memory(MemoryListener *listener,
> > > > > > >          return;
> > > > > > >      }
> > > > > > >  
> > > > > > > +#if 0
> > > > > > >      if (dev->started) {
> > > > > > >          r = vhost_verify_ring_mappings(dev, start_addr, size);
> > > > > > >          assert(r >= 0);
> > > > > > >      }
> > > > > > > +#endif
> > > > > > 
> > > > > > Please add a comment to explain why.
> > > > > > 
> > > > > 
> > > > > Btw, the output that Asias added in the failure case at the behest of
> > > > > MST is here:
> > > > > 
> > > > > http://www.spinics.net/lists/target-devel/msg04077.html
> > > > 
> > > > Yes I suspected we could get l > ring_size, but this is
> > > > not the case here.
> > > > 
> > > 
> > > Hi MST & Co,
> > > 
> > > A quick update here..
> > > 
> > > So this issue appears to be related to performing the
> > > vhost_verify_ring_mappings() call after vhost_dev_unassign_memory() has
> > > been invoked with vhost_set_memory(..., add=false).
> > > 
> > > AFAICT from the logs below, things appear to work as expected when
> > > vhost_verify_ring_mappings() is called only for the
> > > vhost_set_memory(..., add=true) case.
> > > 
> > > Calling vhost_verify_ring_mappings() when dev->started == true +
> > > vhost_set_memory(..., add=false) appears to be a bug caused by fallout
> > > from:
> > > 
> > > commit 24f4fe345c1b80bab1ee18573914123d8028a9e6
> > > Author: Michael S. Tsirkin <mst@redhat.com>
> > > Date:   Tue Dec 25 17:41:07 2012 +0200
> > > 
> > >     vhost: set started flag while start is in progress
> > > 
> > > I'm including the following patch in the forth-coming vhost-scsi series.
> > > Please let me know if you have any concerns.
> > > 
> > > diff --git a/hw/vhost.c b/hw/vhost.c
> > > index 4d6aee3..687a689 100644
> > > --- a/hw/vhost.c
> > > +++ b/hw/vhost.c
> > > @@ -421,7 +421,7 @@ static void vhost_set_memory(MemoryListener *listener,
> > >          return;
> > >      }
> > >  
> > > -    if (dev->started) {
> > > +    if (dev->started && add) {
> > >          r = vhost_verify_ring_mappings(dev, start_addr, size);
> > >          assert(r >= 0);
> > >      }
> > > 
> > > Thanks!
> > > 
> > > --nab
> > 
> > Sorry NAK,
> > I think this will shut down too much stuff:
> > the main reason to check is when we delete a region.
> > 
> 

<SNIP>

> > I still do not understand how this happened.  Somehow a memory region
> > was deleted after vhost_dev_start but before vhost_virtqueue_start was
> > called?
> 
> Not sure..
> 
> To clarify, this is only happening during seabios setup+scan of
> virtio-scsi, and not during normal virtio_scsi LLD operation.
> 
> > Can you set a breakpoint there and see please?
> > 
> > 
> 

A bit more context here..

After debugging seabios this evening, I've isolated the spot where
things begin to go south for vhost_verify_ring_mappings check()

Below are logs from qemu + seabios serial output mixed to (attempt) to
demonstrate what's going on..  

root@tifa:/usr/src# qemu-system-x86_64 -enable-kvm -smp 4 -m 2048
-serial file:/tmp/vhost-serial.txt
-hda /usr/src/qemu-paolo.git/debian_squeeze_amd64_standard.qcow2 -device
vhost-scsi-pci,wwpn=naa.600140579ad21088
qemu-system-x86_64: pci_add_option_rom: failed to find romfile "efi-e1000.rom"
Calling ->region_add: section.size: 655360
vhost_set_memory: section: 0x7fff962c2580 section->size: 655360 add: 1
Calling ->region_add: section.size: 131072
Calling ->region_add: section.size: 131072
vhost_set_memory: section: 0x7fff962c2580 section->size: 131072 add: 1
Calling ->region_add: section.size: 131072
vhost_set_memory: section: 0x7fff962c2580 section->size: 131072 add: 1
Calling ->region_add: section.size: 2146435072
vhost_set_memory: section: 0x7fff962c2580 section->size: 2146435072 add: 1
Calling ->region_add: section.size: 4096
Calling ->region_add: section.size: 1024
Calling ->region_add: section.size: 1048576
Calling ->region_add: section.size: 262144
vhost_set_memory: section: 0x7fff962c2580 section->size: 262144 add: 1
vhost_scsi_init_pci Before virtio_init_pci
virtio_init_pci: size: 60
virtio_init_pci: new size: 64
vhost_set_memory: section: 0x7fe2801f2b60 section->size: 131072 add: 0
vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 32768 add: 1
vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 98304 add: 1
vhost_set_memory: section: 0x7fe2801f2b60 section->size: 32768 add: 0
vhost_set_memory: section: 0x7fe2801f2b60 section->size: 98304 add: 0
vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 65536 add: 1
vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 65536 add: 1
vhost_set_memory: section: 0x7fe2801f2b60 section->size: 65536 add: 0
vhost_set_memory: section: 0x7fe2801f2b60 section->size: 65536 add: 0
vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 98304 add: 1
vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 32768 add: 1
vhost_set_memory: section: 0x7fe2801f2b60 section->size: 98304 add: 0
vhost_set_memory: section: 0x7fe2801f2b60 section->size: 32768 add: 0
vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 131072 add: 1
vhost_set_memory: section: 0x7fe2801f2b60 section->size: 131072 add: 0
vhost_set_memory: section: 0x7fe2801f2b60 section->size: 131072 add: 0
vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 163840 add: 1
vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 98304 add: 1
vhost_set_memory: section: 0x7fe2801f2b60 section->size: 163840 add: 0
vhost_set_memory: section: 0x7fe2801f2b60 section->size: 98304 add: 0
vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 196608 add: 1
vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 65536 add: 1
vhost_set_memory: section: 0x7fe2801f2b60 section->size: 196608 add: 0
vhost_set_memory: section: 0x7fe2801f2b60 section->size: 65536 add: 0
vhost_set_memory: section: 0x7fe2801f2b60 section->size: 2146435072 add: 0
vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 2146697216 add: 1
vhost_set_memory: section: 0x7fe2801f29f0 section->size: 65536 add: 1
vhost_set_memory: section: 0x7fe2801f2ab0 section->size: 65536 add: 0
vhost_set_memory: section: 0x7fe2801f2a70 section->size: 8388608 add: 1
Entering vhost_dev_start >>>>>>>>>>>>>>>>>>>>>.
Before vhost_virtqueue_start >>>>>>>>>>>>>>>>>>>>>>>>>>.
After vhost_virtqueue_start >>>>>>>>>>>>>>>>>>>>>>>>>>.

<Start seabios serial output from init virtio-scsi code>

init virtio-scsi
found virtio-scsi at 0:4
Searching bootorder for: /pci@i0cf8/*@4/*@0/*@0,0
Entering virtio_scsi_cmd: 0x12
|7ffdc000| ata0-0: QEMU HARDDISK ATA-7 Hard-Disk (16384 MiBytes)
|7ffdc000| Searching bootorder for: /pci@i0cf8/*@1,1/drive@0/disk@0
|7ffdc000| Registering bootable: ata0-0: QEMU HARDDISK ATA-7 Hard-Disk (16384 MiBytes) (type:2 prio:101 data:f4ab0)
\7ffdc000/ End thread
|7ffdb000| DVD/CD [ata1-0: QEMU DVD-ROM ATAPI-4 DVD/CD]
|7ffdb000| Searching bootorder for: /pci@i0cf8/*@1,1/drive@1/disk@0
|7ffdb000| Registering bootable: DVD/CD [ata1-0: QEMU DVD-ROM ATAPI-4 DVD/CD] (type:3 prio:103 data:f4a80)
\7ffdb000/ End thread
Searching bootorder for: /pci@i0cf8/*@4/*@0/*@1,0
Entering virtio_scsi_cmd: 0x12
virtio-scsi vendor='LIO-ORG' product='RAMDISK-MCP' rev='4.0' type=0 removable=0
Entering virtio_scsi_cmd: 0x00
Entering virtio_scsi_cmd: 0x25
virtio-scsi blksize=512 sectors=524288
Registering bootable: virtio-scsi Drive LIO-ORG RAMDISK-MCP 4.0 (type:2 prio:101 data:f4ae0)
Searching bootorder for: /pci@i0cf8/*@4/*@0/*@2,0
Entering virtio_scsi_cmd: 0x12

<SNIP> target IDs up to 256...

init lsi53c895a
init esp
init megasas
Scan for option roms
Attempting to init PCI bdf 00:00.0 (vd 8086:1237)
Attempting to init PCI bdf 00:01.0 (vd 8086:7000)
Attempting to init PCI bdf 00:01.3 (vd 8086:7113)
Attempting to init PCI bdf 00:03.0 (vd 8086:100e)
Attempting to init PCI bdf 00:04.0 (vd 1af4:1004)
Searching bootorder for: /rom@genroms/kvmvapic.bin
Registering bootable: Legacy option rom (type:129 prio:101 data:c9000003)
Before prepareboot >>>>>>>>>>>>>>>.
Searching bootorder for: HALT
Mapping hd drive 0x000f4ab0 to 0
drive 0x000f4ab0: PCHS=16383/16/63 translation=lba LCHS=1024/255/63 s=33554432
Mapping hd drive 0x000f4ae0 to 1
drive 0x000f4ae0: PCHS=0/0/0 translation=lba LCHS=520/16/63 s=524288
Running option rom at c900:0003
Mapping floppy drive 0x000f4b40
Mapping cd drive 0x000f4a80
finalize PMM
malloc finalize
Space available for UMB: cb800-ec000, f4690-f4a50
Returned 57344 bytes of ZoneHigh
e820 map has 7 items:
  0: 0000000000000000 - 000000000009fc00 = 1 RAM
  1: 000000000009fc00 - 00000000000a0000 = 2 RESERVED
  2: 00000000000f0000 - 0000000000100000 = 2 RESERVED
  3: 0000000000100000 - 000000007fffe000 = 1 RAM
  4: 000000007fffe000 - 0000000080000000 = 2 RESERVED
  5: 00000000feffc000 - 00000000ff000000 = 2 RESERVED
  6: 00000000fffc0000 - 0000000100000000 = 2 RESERVED
Before make_bios_readonly >>>>>>>
locking shadow ram
Calling pci_config_writeb(0x11): bdf: 0x0000 pam: 0x0000005a
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

<No QEMU output after pci_config_writeb(0x11) in make_bios_readonly..>

Calling pci_config_writeb(0x31): bdf: 0x0000 pam: 0x0000005b
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

<QEMU output after pci_config_writeb(0x31) in make_bios_readonly..>

vhost_set_memory: section: 0x7fe2801f2b60 section->size: 2146697216 add: 0
Before vhost_verify_ring_mappings: start_addr: c0000 size: 2146697216
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Calling l: 5124 for start_addr: c0000 for vq 2
Unable to map ring buffer for ring 2
l: 4096 ring_size: 5124
vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 32768 add: 1
Before vhost_verify_ring_mappings: start_addr: c0000 size: 32768
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 2 ring_phys: ed000 ring_size: 5124
vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 2146664448 add: 1
Before vhost_verify_ring_mappings: start_addr: c8000 size: 2146664448
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Calling l: 5124 for start_addr: c8000 for vq 2

<Seabios output>

Calling pci_config_writeb(0x10): bdf: 0x0000 pam0: 0x00000059
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

<<QEMU output for the pci_config_writeb(0x10) in make_bios_readonly..>

vhost_set_memory: section: 0x7fe2801f2b60 section->size: 32768 add: 0
Before vhost_verify_ring_mappings: start_addr: c0000 size: 32768
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 2 ring_phys: ed000 ring_size: 5124
vhost_set_memory: section: 0x7fe2801f2b60 section->size: 2146664448 add: 0
Before vhost_verify_ring_mappings: start_addr: c8000 size: 2146664448
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Calling l: 5124 for start_addr: c8000 for vq 2
Unable to map ring buffer for ring 2
l: 0 ring_size: 5124
vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 36864 add: 1
Before vhost_verify_ring_mappings: start_addr: c0000 size: 36864
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 2 ring_phys: ed000 ring_size: 5124
vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 2146660352 add: 1
Before vhost_verify_ring_mappings: start_addr: c9000 size: 2146660352
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Calling l: 5124 for start_addr: c9000 for vq 2

<Seabios calls into StartBoot, and the last bit of QEMU output>

vhost_set_memory: section: 0x7fe2801f2b60 section->size: 2146660352 add: 0
Before vhost_verify_ring_mappings: start_addr: c9000 size: 2146660352
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Calling l: 5124 for start_addr: c9000 for vq 2
Unable to map ring buffer for ring 2
l: 0 ring_size: 5124
vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 159744 add: 1
Before vhost_verify_ring_mappings: start_addr: c9000 size: 159744
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Calling l: 5124 for start_addr: c9000 for vq 2
vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 65536 add: 1
Before vhost_verify_ring_mappings: start_addr: f0000 size: 65536
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 2 ring_phys: ed000 ring_size: 5124
vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 2146435072 add: 1
Before vhost_verify_ring_mappings: start_addr: 100000 size: 2146435072
Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
Got ranges_overlap for vq: 2 ring_phys: ed000 ring_size: 5124

So not being a seabios expert, this is as far as I've gotten..  One
change that does appear to avoid this behavior is when vp_reset() is
called right after virtio_scsi_scan_target() occurs.  (See below)

Obviously this prevents virtio-scsi root device operation, but it seems
to be a hint that some left-over PCI config space is triggering the
above in vhost_verify_ring_mappings().

WDYT..?

diff --git a/src/virtio-scsi.c b/src/virtio-scsi.c
index 4de1255..cafefff 100644
--- a/src/virtio-scsi.c
+++ b/src/virtio-scsi.c
@@ -153,7 +155,10 @@ init_virtio_scsi(struct pci_device *pci)
     int i, tot;
     for (tot = 0, i = 0; i < 256; i++)
         tot += virtio_scsi_scan_target(pci, ioaddr, vq, i);
-
+#if 1 
+    /* vhost-scsi-pci needs an vp_reset here, otherwise bad things happen */
+    vp_reset(ioaddr);
+#endif
     if (!tot)
         goto fail;

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Nicholas A. Bellinger]

On Wed, 2013-03-27 at 23:45 -0700, Nicholas A. Bellinger wrote:
> On Wed, 2013-03-27 at 15:33 -0700, Nicholas A. Bellinger wrote:
> > On Wed, 2013-03-27 at 23:56 +0200, Michael S. Tsirkin wrote:
> > > On Wed, Mar 27, 2013 at 02:31:27PM -0700, Nicholas A. Bellinger wrote:

<SNIP>

Adding a bit more detailed seabios PCI setup info, and
make_bios_readonly_intel() debug output for Kevin & Co to review.

> >
> > > I still do not understand how this happened.  Somehow a memory region
> > > was deleted after vhost_dev_start but before vhost_virtqueue_start was
> > > called?
> > 
> > Not sure..
> > 
> > To clarify, this is only happening during seabios setup+scan of
> > virtio-scsi, and not during normal virtio_scsi LLD operation.
> > 
> > > Can you set a breakpoint there and see please?
> > > 
> > > 
> > 
> 
> A bit more context here..
> 
> After debugging seabios this evening, I've isolated the spot where
> things begin to go south for vhost_verify_ring_mappings check()
> 
> Below are logs from qemu + seabios serial output mixed to (attempt) to
> demonstrate what's going on..  
> 
> root@tifa:/usr/src# qemu-system-x86_64 -enable-kvm -smp 4 -m 2048
> -serial file:/tmp/vhost-serial.txt
> -hda /usr/src/qemu-paolo.git/debian_squeeze_amd64_standard.qcow2 -device
> vhost-scsi-pci,wwpn=naa.600140579ad21088
> qemu-system-x86_64: pci_add_option_rom: failed to find romfile "efi-e1000.rom"
> Calling ->region_add: section.size: 655360
> vhost_set_memory: section: 0x7fff962c2580 section->size: 655360 add: 1
> Calling ->region_add: section.size: 131072
> Calling ->region_add: section.size: 131072
> vhost_set_memory: section: 0x7fff962c2580 section->size: 131072 add: 1
> Calling ->region_add: section.size: 131072
> vhost_set_memory: section: 0x7fff962c2580 section->size: 131072 add: 1
> Calling ->region_add: section.size: 2146435072
> vhost_set_memory: section: 0x7fff962c2580 section->size: 2146435072 add: 1
> Calling ->region_add: section.size: 4096
> Calling ->region_add: section.size: 1024
> Calling ->region_add: section.size: 1048576
> Calling ->region_add: section.size: 262144
> vhost_set_memory: section: 0x7fff962c2580 section->size: 262144 add: 1
> vhost_scsi_init_pci Before virtio_init_pci
> virtio_init_pci: size: 60
> virtio_init_pci: new size: 64
> vhost_set_memory: section: 0x7fe2801f2b60 section->size: 131072 add: 0
> vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 32768 add: 1
> vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 98304 add: 1
> vhost_set_memory: section: 0x7fe2801f2b60 section->size: 32768 add: 0
> vhost_set_memory: section: 0x7fe2801f2b60 section->size: 98304 add: 0
> vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 65536 add: 1
> vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 65536 add: 1
> vhost_set_memory: section: 0x7fe2801f2b60 section->size: 65536 add: 0
> vhost_set_memory: section: 0x7fe2801f2b60 section->size: 65536 add: 0
> vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 98304 add: 1
> vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 32768 add: 1
> vhost_set_memory: section: 0x7fe2801f2b60 section->size: 98304 add: 0
> vhost_set_memory: section: 0x7fe2801f2b60 section->size: 32768 add: 0
> vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 131072 add: 1
> vhost_set_memory: section: 0x7fe2801f2b60 section->size: 131072 add: 0
> vhost_set_memory: section: 0x7fe2801f2b60 section->size: 131072 add: 0
> vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 163840 add: 1
> vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 98304 add: 1
> vhost_set_memory: section: 0x7fe2801f2b60 section->size: 163840 add: 0
> vhost_set_memory: section: 0x7fe2801f2b60 section->size: 98304 add: 0
> vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 196608 add: 1
> vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 65536 add: 1
> vhost_set_memory: section: 0x7fe2801f2b60 section->size: 196608 add: 0
> vhost_set_memory: section: 0x7fe2801f2b60 section->size: 65536 add: 0
> vhost_set_memory: section: 0x7fe2801f2b60 section->size: 2146435072 add: 0
> vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 2146697216 add: 1
> vhost_set_memory: section: 0x7fe2801f29f0 section->size: 65536 add: 1
> vhost_set_memory: section: 0x7fe2801f2ab0 section->size: 65536 add: 0
> vhost_set_memory: section: 0x7fe2801f2a70 section->size: 8388608 add: 1
> Entering vhost_dev_start >>>>>>>>>>>>>>>>>>>>>.
> Before vhost_virtqueue_start >>>>>>>>>>>>>>>>>>>>>>>>>>.
> After vhost_virtqueue_start >>>>>>>>>>>>>>>>>>>>>>>>>>.
> 
> <Start seabios serial output from init virtio-scsi code>
> 

=== PCI device probing ===
PCI probe
PCI device 00:00.0 (vd=8086:1237 c=0600)
PCI device 00:01.0 (vd=8086:7000 c=0601)
PCI device 00:01.1 (vd=8086:7010 c=0101)
PCI device 00:01.3 (vd=8086:7113 c=0680)
PCI device 00:02.0 (vd=1013:00b8 c=0300)
PCI device 00:03.0 (vd=8086:100e c=0200)
PCI device 00:04.0 (vd=1af4:1004 c=0100)
Found 7 PCI devices (max PCI bus is 00)
=== PCI new allocation pass #1 ===
PCI: check devices
=== PCI new allocation pass #2 ===
PCI: map device bdf=00:03.0  bar 1, addr 0000c000, size 00000040 [io]
PCI: map device bdf=00:04.0  bar 0, addr 0000c040, size 00000040 [io]
PCI: map device bdf=00:01.1  bar 4, addr 0000c080, size 00000010 [io]
PCI: map device bdf=00:03.0  bar 0, addr febc0000, size 00020000 [mem]
PCI: map device bdf=00:02.0  bar 6, addr febe0000, size 00010000 [mem]
PCI: map device bdf=00:02.0  bar 1, addr febf0000, size 00001000 [mem]
PCI: map device bdf=00:04.0  bar 1, addr febf1000, size 00001000 [mem]
PCI: map device bdf=00:02.0  bar 0, addr fc000000, size 02000000
[prefmem]
PCI: init bdf=00:00.0 id=8086:1237
PCI: init bdf=00:01.0 id=8086:7000
PIIX3/PIIX4 init: elcr=00 0c
PCI: init bdf=00:01.1 id=8086:7010
PCI: init bdf=00:01.3 id=8086:7113
Using pmtimer, ioport 0xb008, freq 3579 kHz
PCI: init bdf=00:02.0 id=1013:00b8
PCI: init bdf=00:03.0 id=8086:100e
PCI: init bdf=00:04.0 id=1af4:1004

> init virtio-scsi
> found virtio-scsi at 0:4

init virtio-scsi
found virtio-scsi at 0:4
init_virtio_scsi: ioaddr: 0xc040

> Searching bootorder for: /pci@i0cf8/*@4/*@0/*@0,0
> Entering virtio_scsi_cmd: 0x12
> |7ffdc000| ata0-0: QEMU HARDDISK ATA-7 Hard-Disk (16384 MiBytes)
> |7ffdc000| Searching bootorder for: /pci@i0cf8/*@1,1/drive@0/disk@0
> |7ffdc000| Registering bootable: ata0-0: QEMU HARDDISK ATA-7 Hard-Disk (16384 MiBytes) (type:2 prio:101 data:f4ab0)
> \7ffdc000/ End thread
> |7ffdb000| DVD/CD [ata1-0: QEMU DVD-ROM ATAPI-4 DVD/CD]
> |7ffdb000| Searching bootorder for: /pci@i0cf8/*@1,1/drive@1/disk@0
> |7ffdb000| Registering bootable: DVD/CD [ata1-0: QEMU DVD-ROM ATAPI-4 DVD/CD] (type:3 prio:103 data:f4a80)
> \7ffdb000/ End thread
> Searching bootorder for: /pci@i0cf8/*@4/*@0/*@1,0
> Entering virtio_scsi_cmd: 0x12
> virtio-scsi vendor='LIO-ORG' product='RAMDISK-MCP' rev='4.0' type=0 removable=0
> Entering virtio_scsi_cmd: 0x00
> Entering virtio_scsi_cmd: 0x25
> virtio-scsi blksize=512 sectors=524288
> Registering bootable: virtio-scsi Drive LIO-ORG RAMDISK-MCP 4.0 (type:2 prio:101 data:f4ae0)
> Searching bootorder for: /pci@i0cf8/*@4/*@0/*@2,0
> Entering virtio_scsi_cmd: 0x12
> 
> <SNIP> target IDs up to 256...
> 
> init lsi53c895a
> init esp
> init megasas
> Scan for option roms
> Attempting to init PCI bdf 00:00.0 (vd 8086:1237)
> Attempting to init PCI bdf 00:01.0 (vd 8086:7000)
> Attempting to init PCI bdf 00:01.3 (vd 8086:7113)
> Attempting to init PCI bdf 00:03.0 (vd 8086:100e)
> Attempting to init PCI bdf 00:04.0 (vd 1af4:1004)
> Searching bootorder for: /rom@genroms/kvmvapic.bin
> Registering bootable: Legacy option rom (type:129 prio:101 data:c9000003)
> Before prepareboot >>>>>>>>>>>>>>>.
> Searching bootorder for: HALT
> Mapping hd drive 0x000f4ab0 to 0
> drive 0x000f4ab0: PCHS=16383/16/63 translation=lba LCHS=1024/255/63 s=33554432
> Mapping hd drive 0x000f4ae0 to 1
> drive 0x000f4ae0: PCHS=0/0/0 translation=lba LCHS=520/16/63 s=524288
> Running option rom at c900:0003
> Mapping floppy drive 0x000f4b40
> Mapping cd drive 0x000f4a80
> finalize PMM
> malloc finalize
> Space available for UMB: cb800-ec000, f4690-f4a50
> Returned 57344 bytes of ZoneHigh
> e820 map has 7 items:
>   0: 0000000000000000 - 000000000009fc00 = 1 RAM
>   1: 000000000009fc00 - 00000000000a0000 = 2 RESERVED
>   2: 00000000000f0000 - 0000000000100000 = 2 RESERVED
>   3: 0000000000100000 - 000000007fffe000 = 1 RAM
>   4: 000000007fffe000 - 0000000080000000 = 2 RESERVED
>   5: 00000000feffc000 - 00000000ff000000 = 2 RESERVED
>   6: 00000000fffc0000 - 0000000100000000 = 2 RESERVED
> Before make_bios_readonly >>>>>>>
> locking shadow ram
> Calling pci_config_writeb(0x11): bdf: 0x0000 pam: 0x0000005a
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 

locking shadow ram
romend: 0x000cb800 romtop: 0x000ec000
mem: 0x000c0000, pam: 0x0000005a
Calling pci_config_writeb(0x11): bdf: 0x0000 pam: 0x0000005a

> <No QEMU output after pci_config_writeb(0x11) in make_bios_readonly..>
> 
> Calling pci_config_writeb(0x31): bdf: 0x0000 pam: 0x0000005b
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 

mem: 0x000c8000, pam: 0x0000005b
romend: 0x000cb800 mem + 16*1024: 0x000cc000
romtop: 0x000ec000 mem + 32*1024: 0x000d0000
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
romend: 0x000cb800, mem: 0x000c8000, romtop: 0x000ec000, mem + 16*1024: 0x000cc000
Calling pci_config_writeb(0x31): bdf: 0x0000 pam: 0x0000005b


> <QEMU output after pci_config_writeb(0x31) in make_bios_readonly..>
> 
> vhost_set_memory: section: 0x7fe2801f2b60 section->size: 2146697216 add: 0
> Before vhost_verify_ring_mappings: start_addr: c0000 size: 2146697216
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Calling l: 5124 for start_addr: c0000 for vq 2
> Unable to map ring buffer for ring 2
> l: 4096 ring_size: 5124
> vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 32768 add: 1
> Before vhost_verify_ring_mappings: start_addr: c0000 size: 32768
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 2 ring_phys: ed000 ring_size: 5124
> vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 2146664448 add: 1
> Before vhost_verify_ring_mappings: start_addr: c8000 size: 2146664448
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Calling l: 5124 for start_addr: c8000 for vq 2
> 
> <Seabios output>
> 
> Calling pci_config_writeb(0x10): bdf: 0x0000 pam0: 0x00000059
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> <<QEMU output for the pci_config_writeb(0x10) in make_bios_readonly..>
> 
> vhost_set_memory: section: 0x7fe2801f2b60 section->size: 32768 add: 0
> Before vhost_verify_ring_mappings: start_addr: c0000 size: 32768
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 2 ring_phys: ed000 ring_size: 5124
> vhost_set_memory: section: 0x7fe2801f2b60 section->size: 2146664448 add: 0
> Before vhost_verify_ring_mappings: start_addr: c8000 size: 2146664448
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Calling l: 5124 for start_addr: c8000 for vq 2
> Unable to map ring buffer for ring 2
> l: 0 ring_size: 5124
> vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 36864 add: 1
> Before vhost_verify_ring_mappings: start_addr: c0000 size: 36864
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 2 ring_phys: ed000 ring_size: 5124
> vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 2146660352 add: 1
> Before vhost_verify_ring_mappings: start_addr: c9000 size: 2146660352
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Calling l: 5124 for start_addr: c9000 for vq 2
> 
> <Seabios calls into StartBoot, and the last bit of QEMU output>
> 
> vhost_set_memory: section: 0x7fe2801f2b60 section->size: 2146660352 add: 0
> Before vhost_verify_ring_mappings: start_addr: c9000 size: 2146660352
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Calling l: 5124 for start_addr: c9000 for vq 2
> Unable to map ring buffer for ring 2
> l: 0 ring_size: 5124
> vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 159744 add: 1
> Before vhost_verify_ring_mappings: start_addr: c9000 size: 159744
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Calling l: 5124 for start_addr: c9000 for vq 2
> vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 65536 add: 1
> Before vhost_verify_ring_mappings: start_addr: f0000 size: 65536
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 2 ring_phys: ed000 ring_size: 5124
> vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 2146435072 add: 1
> Before vhost_verify_ring_mappings: start_addr: 100000 size: 2146435072
> Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> Got ranges_overlap for vq: 2 ring_phys: ed000 ring_size: 5124
> 
> So not being a seabios expert, this is as far as I've gotten..  One
> change that does appear to avoid this behavior is when vp_reset() is
> called right after virtio_scsi_scan_target() occurs.  (See below)
> 
> Obviously this prevents virtio-scsi root device operation, but it seems
> to be a hint that some left-over PCI config space is triggering the
> above in vhost_verify_ring_mappings().
> 
> WDYT..?
> 
> diff --git a/src/virtio-scsi.c b/src/virtio-scsi.c
> index 4de1255..cafefff 100644
> --- a/src/virtio-scsi.c
> +++ b/src/virtio-scsi.c
> @@ -153,7 +155,10 @@ init_virtio_scsi(struct pci_device *pci)
>      int i, tot;
>      for (tot = 0, i = 0; i < 256; i++)
>          tot += virtio_scsi_scan_target(pci, ioaddr, vq, i);
> -
> +#if 1 
> +    /* vhost-scsi-pci needs an vp_reset here, otherwise bad things happen */
> +    vp_reset(ioaddr);
> +#endif
>      if (!tot)
>          goto fail;
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Michael S. Tsirkin]

On Thu, Mar 28, 2013 at 12:35:42AM -0700, Nicholas A. Bellinger wrote:
> On Wed, 2013-03-27 at 23:45 -0700, Nicholas A. Bellinger wrote:
> > On Wed, 2013-03-27 at 15:33 -0700, Nicholas A. Bellinger wrote:
> > > On Wed, 2013-03-27 at 23:56 +0200, Michael S. Tsirkin wrote:
> > > > On Wed, Mar 27, 2013 at 02:31:27PM -0700, Nicholas A. Bellinger wrote:
> 
> <SNIP>
> 
> Adding a bit more detailed seabios PCI setup info, and
> make_bios_readonly_intel() debug output for Kevin & Co to review.
> 
> > >
> > > > I still do not understand how this happened.  Somehow a memory region
> > > > was deleted after vhost_dev_start but before vhost_virtqueue_start was
> > > > called?
> > > 
> > > Not sure..
> > > 
> > > To clarify, this is only happening during seabios setup+scan of
> > > virtio-scsi, and not during normal virtio_scsi LLD operation.
> > > 
> > > > Can you set a breakpoint there and see please?
> > > > 
> > > > 
> > > 
> > 
> > A bit more context here..
> > 
> > After debugging seabios this evening, I've isolated the spot where
> > things begin to go south for vhost_verify_ring_mappings check()
> > 
> > Below are logs from qemu + seabios serial output mixed to (attempt) to
> > demonstrate what's going on..  
> > 
> > root@tifa:/usr/src# qemu-system-x86_64 -enable-kvm -smp 4 -m 2048
> > -serial file:/tmp/vhost-serial.txt
> > -hda /usr/src/qemu-paolo.git/debian_squeeze_amd64_standard.qcow2 -device
> > vhost-scsi-pci,wwpn=naa.600140579ad21088
> > qemu-system-x86_64: pci_add_option_rom: failed to find romfile "efi-e1000.rom"
> > Calling ->region_add: section.size: 655360
> > vhost_set_memory: section: 0x7fff962c2580 section->size: 655360 add: 1
> > Calling ->region_add: section.size: 131072
> > Calling ->region_add: section.size: 131072
> > vhost_set_memory: section: 0x7fff962c2580 section->size: 131072 add: 1
> > Calling ->region_add: section.size: 131072
> > vhost_set_memory: section: 0x7fff962c2580 section->size: 131072 add: 1
> > Calling ->region_add: section.size: 2146435072
> > vhost_set_memory: section: 0x7fff962c2580 section->size: 2146435072 add: 1
> > Calling ->region_add: section.size: 4096
> > Calling ->region_add: section.size: 1024
> > Calling ->region_add: section.size: 1048576
> > Calling ->region_add: section.size: 262144
> > vhost_set_memory: section: 0x7fff962c2580 section->size: 262144 add: 1
> > vhost_scsi_init_pci Before virtio_init_pci
> > virtio_init_pci: size: 60
> > virtio_init_pci: new size: 64
> > vhost_set_memory: section: 0x7fe2801f2b60 section->size: 131072 add: 0
> > vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 32768 add: 1
> > vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 98304 add: 1
> > vhost_set_memory: section: 0x7fe2801f2b60 section->size: 32768 add: 0
> > vhost_set_memory: section: 0x7fe2801f2b60 section->size: 98304 add: 0
> > vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 65536 add: 1
> > vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 65536 add: 1
> > vhost_set_memory: section: 0x7fe2801f2b60 section->size: 65536 add: 0
> > vhost_set_memory: section: 0x7fe2801f2b60 section->size: 65536 add: 0
> > vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 98304 add: 1
> > vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 32768 add: 1
> > vhost_set_memory: section: 0x7fe2801f2b60 section->size: 98304 add: 0
> > vhost_set_memory: section: 0x7fe2801f2b60 section->size: 32768 add: 0
> > vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 131072 add: 1
> > vhost_set_memory: section: 0x7fe2801f2b60 section->size: 131072 add: 0
> > vhost_set_memory: section: 0x7fe2801f2b60 section->size: 131072 add: 0
> > vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 163840 add: 1
> > vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 98304 add: 1
> > vhost_set_memory: section: 0x7fe2801f2b60 section->size: 163840 add: 0
> > vhost_set_memory: section: 0x7fe2801f2b60 section->size: 98304 add: 0
> > vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 196608 add: 1
> > vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 65536 add: 1
> > vhost_set_memory: section: 0x7fe2801f2b60 section->size: 196608 add: 0
> > vhost_set_memory: section: 0x7fe2801f2b60 section->size: 65536 add: 0
> > vhost_set_memory: section: 0x7fe2801f2b60 section->size: 2146435072 add: 0
> > vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 2146697216 add: 1
> > vhost_set_memory: section: 0x7fe2801f29f0 section->size: 65536 add: 1
> > vhost_set_memory: section: 0x7fe2801f2ab0 section->size: 65536 add: 0
> > vhost_set_memory: section: 0x7fe2801f2a70 section->size: 8388608 add: 1
> > Entering vhost_dev_start >>>>>>>>>>>>>>>>>>>>>.
> > Before vhost_virtqueue_start >>>>>>>>>>>>>>>>>>>>>>>>>>.
> > After vhost_virtqueue_start >>>>>>>>>>>>>>>>>>>>>>>>>>.
> > 
> > <Start seabios serial output from init virtio-scsi code>
> > 
> 
> === PCI device probing ===
> PCI probe
> PCI device 00:00.0 (vd=8086:1237 c=0600)
> PCI device 00:01.0 (vd=8086:7000 c=0601)
> PCI device 00:01.1 (vd=8086:7010 c=0101)
> PCI device 00:01.3 (vd=8086:7113 c=0680)
> PCI device 00:02.0 (vd=1013:00b8 c=0300)
> PCI device 00:03.0 (vd=8086:100e c=0200)
> PCI device 00:04.0 (vd=1af4:1004 c=0100)
> Found 7 PCI devices (max PCI bus is 00)
> === PCI new allocation pass #1 ===
> PCI: check devices
> === PCI new allocation pass #2 ===
> PCI: map device bdf=00:03.0  bar 1, addr 0000c000, size 00000040 [io]
> PCI: map device bdf=00:04.0  bar 0, addr 0000c040, size 00000040 [io]
> PCI: map device bdf=00:01.1  bar 4, addr 0000c080, size 00000010 [io]
> PCI: map device bdf=00:03.0  bar 0, addr febc0000, size 00020000 [mem]
> PCI: map device bdf=00:02.0  bar 6, addr febe0000, size 00010000 [mem]
> PCI: map device bdf=00:02.0  bar 1, addr febf0000, size 00001000 [mem]
> PCI: map device bdf=00:04.0  bar 1, addr febf1000, size 00001000 [mem]
> PCI: map device bdf=00:02.0  bar 0, addr fc000000, size 02000000
> [prefmem]
> PCI: init bdf=00:00.0 id=8086:1237
> PCI: init bdf=00:01.0 id=8086:7000
> PIIX3/PIIX4 init: elcr=00 0c
> PCI: init bdf=00:01.1 id=8086:7010
> PCI: init bdf=00:01.3 id=8086:7113
> Using pmtimer, ioport 0xb008, freq 3579 kHz
> PCI: init bdf=00:02.0 id=1013:00b8
> PCI: init bdf=00:03.0 id=8086:100e
> PCI: init bdf=00:04.0 id=1af4:1004
> 
> > init virtio-scsi
> > found virtio-scsi at 0:4
> 
> init virtio-scsi
> found virtio-scsi at 0:4
> init_virtio_scsi: ioaddr: 0xc040
> 
> > Searching bootorder for: /pci@i0cf8/*@4/*@0/*@0,0
> > Entering virtio_scsi_cmd: 0x12
> > |7ffdc000| ata0-0: QEMU HARDDISK ATA-7 Hard-Disk (16384 MiBytes)
> > |7ffdc000| Searching bootorder for: /pci@i0cf8/*@1,1/drive@0/disk@0
> > |7ffdc000| Registering bootable: ata0-0: QEMU HARDDISK ATA-7 Hard-Disk (16384 MiBytes) (type:2 prio:101 data:f4ab0)
> > \7ffdc000/ End thread
> > |7ffdb000| DVD/CD [ata1-0: QEMU DVD-ROM ATAPI-4 DVD/CD]
> > |7ffdb000| Searching bootorder for: /pci@i0cf8/*@1,1/drive@1/disk@0
> > |7ffdb000| Registering bootable: DVD/CD [ata1-0: QEMU DVD-ROM ATAPI-4 DVD/CD] (type:3 prio:103 data:f4a80)
> > \7ffdb000/ End thread
> > Searching bootorder for: /pci@i0cf8/*@4/*@0/*@1,0
> > Entering virtio_scsi_cmd: 0x12
> > virtio-scsi vendor='LIO-ORG' product='RAMDISK-MCP' rev='4.0' type=0 removable=0
> > Entering virtio_scsi_cmd: 0x00
> > Entering virtio_scsi_cmd: 0x25
> > virtio-scsi blksize=512 sectors=524288
> > Registering bootable: virtio-scsi Drive LIO-ORG RAMDISK-MCP 4.0 (type:2 prio:101 data:f4ae0)
> > Searching bootorder for: /pci@i0cf8/*@4/*@0/*@2,0
> > Entering virtio_scsi_cmd: 0x12
> > 
> > <SNIP> target IDs up to 256...
> > 
> > init lsi53c895a
> > init esp
> > init megasas
> > Scan for option roms
> > Attempting to init PCI bdf 00:00.0 (vd 8086:1237)
> > Attempting to init PCI bdf 00:01.0 (vd 8086:7000)
> > Attempting to init PCI bdf 00:01.3 (vd 8086:7113)
> > Attempting to init PCI bdf 00:03.0 (vd 8086:100e)
> > Attempting to init PCI bdf 00:04.0 (vd 1af4:1004)
> > Searching bootorder for: /rom@genroms/kvmvapic.bin
> > Registering bootable: Legacy option rom (type:129 prio:101 data:c9000003)
> > Before prepareboot >>>>>>>>>>>>>>>.
> > Searching bootorder for: HALT
> > Mapping hd drive 0x000f4ab0 to 0
> > drive 0x000f4ab0: PCHS=16383/16/63 translation=lba LCHS=1024/255/63 s=33554432
> > Mapping hd drive 0x000f4ae0 to 1
> > drive 0x000f4ae0: PCHS=0/0/0 translation=lba LCHS=520/16/63 s=524288
> > Running option rom at c900:0003
> > Mapping floppy drive 0x000f4b40
> > Mapping cd drive 0x000f4a80
> > finalize PMM
> > malloc finalize
> > Space available for UMB: cb800-ec000, f4690-f4a50
> > Returned 57344 bytes of ZoneHigh
> > e820 map has 7 items:
> >   0: 0000000000000000 - 000000000009fc00 = 1 RAM
> >   1: 000000000009fc00 - 00000000000a0000 = 2 RESERVED
> >   2: 00000000000f0000 - 0000000000100000 = 2 RESERVED
> >   3: 0000000000100000 - 000000007fffe000 = 1 RAM
> >   4: 000000007fffe000 - 0000000080000000 = 2 RESERVED
> >   5: 00000000feffc000 - 00000000ff000000 = 2 RESERVED
> >   6: 00000000fffc0000 - 0000000100000000 = 2 RESERVED
> > Before make_bios_readonly >>>>>>>
> > locking shadow ram
> > Calling pci_config_writeb(0x11): bdf: 0x0000 pam: 0x0000005a
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> 
> locking shadow ram
> romend: 0x000cb800 romtop: 0x000ec000
> mem: 0x000c0000, pam: 0x0000005a
> Calling pci_config_writeb(0x11): bdf: 0x0000 pam: 0x0000005a
> 
> > <No QEMU output after pci_config_writeb(0x11) in make_bios_readonly..>
> > 
> > Calling pci_config_writeb(0x31): bdf: 0x0000 pam: 0x0000005b
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> 
> mem: 0x000c8000, pam: 0x0000005b
> romend: 0x000cb800 mem + 16*1024: 0x000cc000
> romtop: 0x000ec000 mem + 32*1024: 0x000d0000
> >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> romend: 0x000cb800, mem: 0x000c8000, romtop: 0x000ec000, mem + 16*1024: 0x000cc000
> Calling pci_config_writeb(0x31): bdf: 0x0000 pam: 0x0000005b
> 
> 
> > <QEMU output after pci_config_writeb(0x31) in make_bios_readonly..>
> > 
> > vhost_set_memory: section: 0x7fe2801f2b60 section->size: 2146697216 add: 0
> > Before vhost_verify_ring_mappings: start_addr: c0000 size: 2146697216
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.

This is also a bug. -net always initializes VQs 0..N so this is what
vhost assumed.  Please teach vhost that it should skip uninitialized
VQs. There are more places to fix.
Basically look for if (!virtio_queue_get_num(vdev, queue_no)),
all of them need to be updated to skip uninitialized vqs.
Probably switch to a new API checking PA too.
See patch below.

> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Calling l: 5124 for start_addr: c0000 for vq 2
> > Unable to map ring buffer for ring 2
> > l: 4096 ring_size: 5124

okay so the ring address is within ROM.
Unsurprisingly it fails.
bios should stop device before write protect.

> > vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 32768 add: 1
> > Before vhost_verify_ring_mappings: start_addr: c0000 size: 32768
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 2 ring_phys: ed000 ring_size: 5124
> > vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 2146664448 add: 1
> > Before vhost_verify_ring_mappings: start_addr: c8000 size: 2146664448
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Calling l: 5124 for start_addr: c8000 for vq 2
> > 
> > <Seabios output>
> > 
> > Calling pci_config_writeb(0x10): bdf: 0x0000 pam0: 0x00000059
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > <<QEMU output for the pci_config_writeb(0x10) in make_bios_readonly..>
> > 
> > vhost_set_memory: section: 0x7fe2801f2b60 section->size: 32768 add: 0
> > Before vhost_verify_ring_mappings: start_addr: c0000 size: 32768
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 2 ring_phys: ed000 ring_size: 5124
> > vhost_set_memory: section: 0x7fe2801f2b60 section->size: 2146664448 add: 0
> > Before vhost_verify_ring_mappings: start_addr: c8000 size: 2146664448
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Calling l: 5124 for start_addr: c8000 for vq 2
> > Unable to map ring buffer for ring 2
> > l: 0 ring_size: 5124
> > vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 36864 add: 1
> > Before vhost_verify_ring_mappings: start_addr: c0000 size: 36864
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 2 ring_phys: ed000 ring_size: 5124
> > vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 2146660352 add: 1
> > Before vhost_verify_ring_mappings: start_addr: c9000 size: 2146660352
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Calling l: 5124 for start_addr: c9000 for vq 2
> > 
> > <Seabios calls into StartBoot, and the last bit of QEMU output>
> > 
> > vhost_set_memory: section: 0x7fe2801f2b60 section->size: 2146660352 add: 0
> > Before vhost_verify_ring_mappings: start_addr: c9000 size: 2146660352
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Calling l: 5124 for start_addr: c9000 for vq 2
> > Unable to map ring buffer for ring 2
> > l: 0 ring_size: 5124
> > vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 159744 add: 1
> > Before vhost_verify_ring_mappings: start_addr: c9000 size: 159744
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Calling l: 5124 for start_addr: c9000 for vq 2
> > vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 65536 add: 1
> > Before vhost_verify_ring_mappings: start_addr: f0000 size: 65536
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 2 ring_phys: ed000 ring_size: 5124
> > vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 2146435072 add: 1
> > Before vhost_verify_ring_mappings: start_addr: 100000 size: 2146435072
> > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > Got ranges_overlap for vq: 2 ring_phys: ed000 ring_size: 5124
> > 
> > So not being a seabios expert, this is as far as I've gotten..  One
> > change that does appear to avoid this behavior is when vp_reset() is
> > called right after virtio_scsi_scan_target() occurs.  (See below)
> > 
> > Obviously this prevents virtio-scsi root device operation, but it seems
> > to be a hint that some left-over PCI config space is triggering the
> > above in vhost_verify_ring_mappings().
> > 
> > WDYT..?
> > 
> > diff --git a/src/virtio-scsi.c b/src/virtio-scsi.c
> > index 4de1255..cafefff 100644
> > --- a/src/virtio-scsi.c
> > +++ b/src/virtio-scsi.c
> > @@ -153,7 +155,10 @@ init_virtio_scsi(struct pci_device *pci)
> >      int i, tot;
> >      for (tot = 0, i = 0; i < 256; i++)
> >          tot += virtio_scsi_scan_target(pci, ioaddr, vq, i);
> > -
> > +#if 1 
> > +    /* vhost-scsi-pci needs an vp_reset here, otherwise bad things happen */
> > +    vp_reset(ioaddr);
> > +#endif
> >      if (!tot)
> >          goto fail;
> > 
> > 

I think it's the right thing to do, but maybe not the right place
to do this, need to reset after all IO is done, before
ring memory is write protected.

> > --
> > To unsubscribe from this list: send the line "unsubscribe target-devel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

----

virtio: add API to check that ring is setup

virtio scsi makes it legal to only setup a subset of rings.  The only
way to detect the ring is setup seems to be to check whether PA was
written to.  Add API to do this, and teach code to use it instead of
checking hardware queue size.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

--->

diff --git a/hw/virtio.c b/hw/virtio.c
index 26fbc79..ac12c01 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -651,6 +651,11 @@ int virtio_queue_get_num(VirtIODevice *vdev, int n)
     return vdev->vq[n].vring.num;
 }
 
+bool virtio_queue_valid(VirtIODevice *vdev, int n)
+{
+    return vdev->vq[n].vring.num && vdev->vq[n].vring.pa;
+}
+
 int virtio_queue_get_id(VirtQueue *vq)
 {
     VirtIODevice *vdev = vq->vdev;
diff --git a/hw/virtio.h b/hw/virtio.h
index ca43fd7..d3f23c2 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -227,6 +227,7 @@ void virtio_config_writel(VirtIODevice *vdev, uint32_t addr, uint32_t data);
 void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr);
 hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n);
 int virtio_queue_get_num(VirtIODevice *vdev, int n);
+bool virtio_queue_valid(VirtIODevice *vdev, int n);
 void virtio_queue_notify(VirtIODevice *vdev, int n);
 uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
 void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector);

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Paolo Bonzini]

Il 28/03/2013 10:04, Michael S. Tsirkin ha scritto:
>>> > > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
>>> > > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
>>> > > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
>>> > > Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
>>> > > Calling l: 5124 for start_addr: c0000 for vq 2
>>> > > Unable to map ring buffer for ring 2
>>> > > l: 4096 ring_size: 5124
> okay so the ring address is within ROM.
> Unsurprisingly it fails.
> bios should stop device before write protect.
> 

The above log is very early, when everything is RAM:

  vhost_set_memory: section: 0x7fe2801f2b60 section->size: 2146697216 add: 0
  Before vhost_verify_ring_mappings: start_addr: c0000 size: 2146697216

The rings are not within ROM.  ROM is at 0xc0000-0xcc000 according to the
PAM registers.

The way I followed the debug output, "Got ranges_overlap" means 
actually "bailing out because ranges do not overlap".  In particular, 
here all three virtqueues fail the test, because this is the ROM area 
0xc0000..0xc7fff:

  vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 32768 add: 1
  Before vhost_verify_ring_mappings: start_addr: c0000 size: 32768
  Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
  Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
  Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
  Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
  Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
  Got ranges_overlap for vq: 2 ring_phys: ed000 ring_size: 5124

Just below, vhost looks at the large RAM area starting at 0xc8000
(it's large because 0xf0000..0xfffff is still RAM):

  vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 2146664448 add: 1
  Before vhost_verify_ring_mappings: start_addr: c8000 size: 2146664448
  Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
  Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
  Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
  Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
  Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
  Calling l: 5124 for start_addr: c8000 for vq 2

Here vq 0 and 1 fail the test because they are in low RAM, vq 2 passes.

After 0xf0000..0xfffff is marked readonly, vhost looks at the RAM
between 0xc9000 and 0xf0000:

  vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 159744 add: 1
  Before vhost_verify_ring_mappings: start_addr: c9000 size: 159744
  Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
  Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
  Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
  Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
  Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
  Calling l: 5124 for start_addr: c9000 for vq 2

and the ROM between 0xf0000 and 0xfffff, which no ring overlaps with:

  vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 65536 add: 1
  Before vhost_verify_ring_mappings: start_addr: f0000 size: 65536
  Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
  Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
  Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
  Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
  Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
  Got ranges_overlap for vq: 2 ring_phys: ed000 ring_size: 5124



SeaBIOS is indeed not initializing vqs 0/1 (the control and event 
queues), so their ring_phys is 0.  But the one that is failing is vq 2, 
the first request queue.

Your patch seems good, but shouldn't fix this problem.

Paolo

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Nicholas A. Bellinger]

On Thu, 2013-03-28 at 11:03 +0100, Paolo Bonzini wrote:
> Il 28/03/2013 10:04, Michael S. Tsirkin ha scritto:
> >>> > > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> >>> > > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> >>> > > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> >>> > > Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> >>> > > Calling l: 5124 for start_addr: c0000 for vq 2
> >>> > > Unable to map ring buffer for ring 2
> >>> > > l: 4096 ring_size: 5124
> > okay so the ring address is within ROM.
> > Unsurprisingly it fails.
> > bios should stop device before write protect.
> > 
> 
> The above log is very early, when everything is RAM:
> 
>   vhost_set_memory: section: 0x7fe2801f2b60 section->size: 2146697216 add: 0
>   Before vhost_verify_ring_mappings: start_addr: c0000 size: 2146697216
> 
> The rings are not within ROM.  ROM is at 0xc0000-0xcc000 according to the
> PAM registers.
> 
> The way I followed the debug output, "Got ranges_overlap" means 
> actually "bailing out because ranges do not overlap".

Yes, this is when !ranges_overlap() is hit in
vhost_verify_ring_mappings(), so the offending cpu_physical_memory_map()
is skipped..

> In particular, 
> here all three virtqueues fail the test, because this is the ROM area 
> 0xc0000..0xc7fff:
> 
>   vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 32768 add: 1
>   Before vhost_verify_ring_mappings: start_addr: c0000 size: 32768
>   Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
>   Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
>   Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
>   Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
>   Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
>   Got ranges_overlap for vq: 2 ring_phys: ed000 ring_size: 5124
> 
> Just below, vhost looks at the large RAM area starting at 0xc8000
> (it's large because 0xf0000..0xfffff is still RAM):
> 
>   vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 2146664448 add: 1
>   Before vhost_verify_ring_mappings: start_addr: c8000 size: 2146664448
>   Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
>   Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
>   Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
>   Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
>   Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
>   Calling l: 5124 for start_addr: c8000 for vq 2
> 
> Here vq 0 and 1 fail the test because they are in low RAM, vq 2 passes.
> 
> After 0xf0000..0xfffff is marked readonly,

Btw, the first vhost_set_memory() and failing
vhost_verify_ring_mappings() do not occur until the
pci_config_writeb(..., 0x31) code is executed in
src/shadow.c:make_bios_readonly_intel() below:

static void
make_bios_readonly_intel(u16 bdf, u32 pam0)
{
    // Flush any pending writes before locking memory.
    wbinvd();

    // Write protect roms from 0xc0000-0xf0000
    u32 romend = rom_get_last(), romtop = rom_get_max();
    int i;
    for (i=0; i<6; i++) {
        u32 mem = BUILD_ROM_START + i * 32*1024;
        u32 pam = pam0 + 1 + i;
        if (romend <= mem + 16*1024 || romtop <= mem + 32*1024) {
            if (romend > mem && romtop > mem + 16*1024)
                pci_config_writeb(bdf, pam, 0x31);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

            break;
        }
        pci_config_writeb(bdf, pam, 0x11);
    }

    // Write protect 0xf0000-0x100000
    pci_config_writeb(bdf, pam0, 0x10);
}

Up until this point, vhost_verify_ring_mappings() is not called by
vhost_set_memory() as vhost_dev_start() has not been invoked to set
vdev->started yet..

>  vhost looks at the RAM
> between 0xc9000 and 0xf0000:
> 
>   vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 159744 add: 1
>   Before vhost_verify_ring_mappings: start_addr: c9000 size: 159744
>   Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
>   Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
>   Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
>   Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
>   Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
>   Calling l: 5124 for start_addr: c9000 for vq 2
> 
> and the ROM between 0xf0000 and 0xfffff, which no ring overlaps with:
> 
>   vhost_set_memory: section: 0x7fe2801f2aa0 section->size: 65536 add: 1
>   Before vhost_verify_ring_mappings: start_addr: f0000 size: 65536
>   Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
>   Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
>   Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
>   Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
>   Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
>   Got ranges_overlap for vq: 2 ring_phys: ed000 ring_size: 5124
> 
> 
> 
> SeaBIOS is indeed not initializing vqs 0/1 (the control and event 
> queues), so their ring_phys is 0.  But the one that is failing is vq 2, 
> the first request queue.
> 
> Your patch seems good, but shouldn't fix this problem.
> 
> Paolo
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Paolo Bonzini]

> I think it's the right thing to do, but maybe not the right place
> to do this, need to reset after all IO is done, before
> ring memory is write protected.

Our emails are crossing each other unfortunately, but I want to
reinforce this: ring memory is not write protected.  Remember that
SeaBIOS can even provide virtio-scsi access to DOS, so you must
not reset the device.  It must remain functional all the time,
and the OS's own driver will reset it when it's started.

Paolo

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Nicholas A. Bellinger]

On Thu, 2013-03-28 at 06:13 -0400, Paolo Bonzini wrote:
> > I think it's the right thing to do, but maybe not the right place
> > to do this, need to reset after all IO is done, before
> > ring memory is write protected.
> 
> Our emails are crossing each other unfortunately, but I want to
> reinforce this: ring memory is not write protected.

Understood.  However, AFAICT the act of write protecting these ranges
for ROM generates the offending callbacks to vhost_set_memory().

The part that I'm missing is if ring memory is not being write protected
by make_bios_readonly_intel(), why are the vhost_set_memory() calls
being invoked..?

> Remember that
> SeaBIOS can even provide virtio-scsi access to DOS, so you must
> not reset the device.  It must remain functional all the time,
> and the OS's own driver will reset it when it's started.
> 

Mmmm, so a vp_reset() is out of the question then..

--nab

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Paolo Bonzini]

Il 29/03/2013 03:53, Nicholas A. Bellinger ha scritto:
> On Thu, 2013-03-28 at 06:13 -0400, Paolo Bonzini wrote:
>>> I think it's the right thing to do, but maybe not the right place
>>> to do this, need to reset after all IO is done, before
>>> ring memory is write protected.
>>
>> Our emails are crossing each other unfortunately, but I want to
>> reinforce this: ring memory is not write protected.
> 
> Understood.  However, AFAICT the act of write protecting these ranges
> for ROM generates the offending callbacks to vhost_set_memory().
> 
> The part that I'm missing is if ring memory is not being write protected
> by make_bios_readonly_intel(), why are the vhost_set_memory() calls
> being invoked..?

Because mappings change for the region that contains the ring.  vhost
doesn't know yet that the changes do not affect ring memory,
vhost_set_memory() is called exactly to ascertain that.

Paolo

> 
>> Remember that
>> SeaBIOS can even provide virtio-scsi access to DOS, so you must
>> not reset the device.  It must remain functional all the time,
>> and the OS's own driver will reset it when it's started.
>>
> 
> Mmmm, so a vp_reset() is out of the question then..
> 
> --nab
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Nicholas A. Bellinger]

On Fri, 2013-03-29 at 09:14 +0100, Paolo Bonzini wrote: 
> Il 29/03/2013 03:53, Nicholas A. Bellinger ha scritto:
> > On Thu, 2013-03-28 at 06:13 -0400, Paolo Bonzini wrote:
> >>> I think it's the right thing to do, but maybe not the right place
> >>> to do this, need to reset after all IO is done, before
> >>> ring memory is write protected.
> >>
> >> Our emails are crossing each other unfortunately, but I want to
> >> reinforce this: ring memory is not write protected.
> > 
> > Understood.  However, AFAICT the act of write protecting these ranges
> > for ROM generates the offending callbacks to vhost_set_memory().
> > 
> > The part that I'm missing is if ring memory is not being write protected
> > by make_bios_readonly_intel(), why are the vhost_set_memory() calls
> > being invoked..?
> 
> Because mappings change for the region that contains the ring.  vhost
> doesn't know yet that the changes do not affect ring memory,
> vhost_set_memory() is called exactly to ascertain that.
> 

Hi Paolo & Co,

Here's a bit more information on what is going on with the same
cpu_physical_memory_map() failure in vhost_verify_ring_mappings()..

So as before, at the point that seabios is marking memory as readonly
for ROM in src/shadow.c:make_bios_readonly_intel() with the following
call:

Calling pci_config_writeb(0x31): bdf: 0x0000 pam: 0x0000005b

the memory API update hook triggers back into vhost_region_del() code,
and following occurs:

Entering vhost_region_del section: 0x7fd30a213b60 offset_within_region: 0xc0000 size: 2146697216 readonly: 0
vhost_region_del: is_rom: 0, rom_device: 0
vhost_region_del: readable: 1
vhost_region_del: ram_addr 0x0, addr: 0x0 size: 2147483648
vhost_region_del: name: pc.ram
Entering vhost_set_memory, section: 0x7fd30a213b60 add: 0, dev->started: 1
Entering verify_ring_mappings: start_addr 0x00000000000c0000 size: 2146697216
verify_ring_mappings: ring_phys 0x0 ring_size: 0
verify_ring_mappings: ring_phys 0x0 ring_size: 0
verify_ring_mappings: ring_phys 0xed000 ring_size: 5124
verify_ring_mappings: calling cpu_physical_memory_map ring_phys: 0xed000 l: 5124
address_space_map: addr: 0xed000, plen: 5124
address_space_map: l: 4096, len: 5124
phys_page_find got PHYS_MAP_NODE_NIL >>>>>>>>>>>>>>>>>>>>>>..
address_space_map: section: 0x7fd30fabaed0 memory_region_is_ram: 0 readonly: 0
address_space_map: section: 0x7fd30fabaed0 offset_within_region: 0x0 section size: 18446744073709551615
Unable to map ring buffer for ring 2, l: 4096

So the interesting part is that phys_page_find() is not able to locate
the corresponding page for vq->ring_phys: 0xed000 from the
vhost_region_del() callback with section->offset_within_region:
0xc0000..

Is there any case where this would not be considered a bug..? 

register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
Entering vhost_region_add section: 0x7fd30a213aa0 offset_within_region: 0xc0000 size: 32768 readonly: 1
vhost_region_add: is_rom: 0, rom_device: 0
vhost_region_add: readable: 1
vhost_region_add: ram_addr 0x0000000000000000, addr: 0x               0 size: 2147483648
vhost_region_add: name: pc.ram
Entering vhost_set_memory, section: 0x7fd30a213aa0 add: 1, dev->started: 1
Entering verify_ring_mappings: start_addr 0x00000000000c0000 size: 32768
verify_ring_mappings: ring_phys 0x0 ring_size: 0
verify_ring_mappings: ring_phys 0x0 ring_size: 0
verify_ring_mappings: ring_phys 0xed000 ring_size: 5124
verify_ring_mappings: Got !ranges_overlap, skipping
register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
Entering vhost_region_add section: 0x7fd30a213aa0 offset_within_region: 0xc8000 size: 2146664448 readonly: 0
vhost_region_add: is_rom: 0, rom_device: 0
vhost_region_add: readable: 1
vhost_region_add: ram_addr 0x0000000000000000, addr: 0x               0 size: 2147483648
vhost_region_add: name: pc.ram
Entering vhost_set_memory, section: 0x7fd30a213aa0 add: 1, dev->started: 1
Entering verify_ring_mappings: start_addr 0x00000000000c8000 size: 2146664448
verify_ring_mappings: ring_phys 0x0 ring_size: 0
verify_ring_mappings: ring_phys 0x0 ring_size: 0
verify_ring_mappings: ring_phys 0xed000 ring_size: 5124
verify_ring_mappings: calling cpu_physical_memory_map ring_phys: 0xed000 l: 5124
address_space_map: addr: 0xed000, plen: 5124
address_space_map: l: 4096, len: 5124
address_space_map: section: 0x7fd30fabb020 memory_region_is_ram: 1 readonly: 0
address_space_map: section: 0x7fd30fabb020 offset_within_region: 0xc8000 section size: 2146664448
address_space_map: l: 4096, len: 1028
address_space_map: section: 0x7fd30fabb020 memory_region_is_ram: 1 readonly: 0
address_space_map: section: 0x7fd30fabb020 offset_within_region: 0xc8000 section size: 2146664448
address_space_map: Calling qemu_ram_ptr_length: raddr: 0x           ed000 rlen: 5124
address_space_map: After qemu_ram_ptr_length: raddr: 0x           ed000 rlen: 5124

So here the vhost_region_add() callback for
section->offset_within_region: 0xc8000 for vq->ring_phys: 0xed000 is
able to locate *section via phys_page_find() within address_space_map(),
and cpu_physical_memory_map() completes as expected..

register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
phys_page_find got PHYS_MAP_NODE_NIL >>>>>>>>>>>>>>>>>>>>>>..

So while plodding my way through the memory API, the thing that would be
useful to know is if the offending *section that is missing for the
first phys_page_find() call is getting removed before the callback makes
it's way into vhost_verify_ring_mappings() code, or that some other bug
is occuring..?

Any idea on how this could be verified..?

Thanks,

--nab

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Michael S. Tsirkin]

On Mon, Apr 01, 2013 at 06:05:47PM -0700, Nicholas A. Bellinger wrote:
> On Fri, 2013-03-29 at 09:14 +0100, Paolo Bonzini wrote: 
> > Il 29/03/2013 03:53, Nicholas A. Bellinger ha scritto:
> > > On Thu, 2013-03-28 at 06:13 -0400, Paolo Bonzini wrote:
> > >>> I think it's the right thing to do, but maybe not the right place
> > >>> to do this, need to reset after all IO is done, before
> > >>> ring memory is write protected.
> > >>
> > >> Our emails are crossing each other unfortunately, but I want to
> > >> reinforce this: ring memory is not write protected.
> > > 
> > > Understood.  However, AFAICT the act of write protecting these ranges
> > > for ROM generates the offending callbacks to vhost_set_memory().
> > > 
> > > The part that I'm missing is if ring memory is not being write protected
> > > by make_bios_readonly_intel(), why are the vhost_set_memory() calls
> > > being invoked..?
> > 
> > Because mappings change for the region that contains the ring.  vhost
> > doesn't know yet that the changes do not affect ring memory,
> > vhost_set_memory() is called exactly to ascertain that.
> > 
> 
> Hi Paolo & Co,
> 
> Here's a bit more information on what is going on with the same
> cpu_physical_memory_map() failure in vhost_verify_ring_mappings()..
> 
> So as before, at the point that seabios is marking memory as readonly
> for ROM in src/shadow.c:make_bios_readonly_intel() with the following
> call:
> 
> Calling pci_config_writeb(0x31): bdf: 0x0000 pam: 0x0000005b
> 
> the memory API update hook triggers back into vhost_region_del() code,
> and following occurs:
> 
> Entering vhost_region_del section: 0x7fd30a213b60 offset_within_region: 0xc0000 size: 2146697216 readonly: 0
> vhost_region_del: is_rom: 0, rom_device: 0
> vhost_region_del: readable: 1
> vhost_region_del: ram_addr 0x0, addr: 0x0 size: 2147483648
> vhost_region_del: name: pc.ram
> Entering vhost_set_memory, section: 0x7fd30a213b60 add: 0, dev->started: 1
> Entering verify_ring_mappings: start_addr 0x00000000000c0000 size: 2146697216
> verify_ring_mappings: ring_phys 0x0 ring_size: 0
> verify_ring_mappings: ring_phys 0x0 ring_size: 0
> verify_ring_mappings: ring_phys 0xed000 ring_size: 5124
> verify_ring_mappings: calling cpu_physical_memory_map ring_phys: 0xed000 l: 5124
> address_space_map: addr: 0xed000, plen: 5124
> address_space_map: l: 4096, len: 5124
> phys_page_find got PHYS_MAP_NODE_NIL >>>>>>>>>>>>>>>>>>>>>>..
> address_space_map: section: 0x7fd30fabaed0 memory_region_is_ram: 0 readonly: 0
> address_space_map: section: 0x7fd30fabaed0 offset_within_region: 0x0 section size: 18446744073709551615
> Unable to map ring buffer for ring 2, l: 4096
> 
> So the interesting part is that phys_page_find() is not able to locate
> the corresponding page for vq->ring_phys: 0xed000 from the
> vhost_region_del() callback with section->offset_within_region:
> 0xc0000..
> 
> Is there any case where this would not be considered a bug..? 
> 
> register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> Entering vhost_region_add section: 0x7fd30a213aa0 offset_within_region: 0xc0000 size: 32768 readonly: 1
> vhost_region_add: is_rom: 0, rom_device: 0
> vhost_region_add: readable: 1
> vhost_region_add: ram_addr 0x0000000000000000, addr: 0x               0 size: 2147483648
> vhost_region_add: name: pc.ram
> Entering vhost_set_memory, section: 0x7fd30a213aa0 add: 1, dev->started: 1
> Entering verify_ring_mappings: start_addr 0x00000000000c0000 size: 32768
> verify_ring_mappings: ring_phys 0x0 ring_size: 0
> verify_ring_mappings: ring_phys 0x0 ring_size: 0
> verify_ring_mappings: ring_phys 0xed000 ring_size: 5124
> verify_ring_mappings: Got !ranges_overlap, skipping
> register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> Entering vhost_region_add section: 0x7fd30a213aa0 offset_within_region: 0xc8000 size: 2146664448 readonly: 0
> vhost_region_add: is_rom: 0, rom_device: 0
> vhost_region_add: readable: 1
> vhost_region_add: ram_addr 0x0000000000000000, addr: 0x               0 size: 2147483648
> vhost_region_add: name: pc.ram
> Entering vhost_set_memory, section: 0x7fd30a213aa0 add: 1, dev->started: 1
> Entering verify_ring_mappings: start_addr 0x00000000000c8000 size: 2146664448
> verify_ring_mappings: ring_phys 0x0 ring_size: 0
> verify_ring_mappings: ring_phys 0x0 ring_size: 0
> verify_ring_mappings: ring_phys 0xed000 ring_size: 5124
> verify_ring_mappings: calling cpu_physical_memory_map ring_phys: 0xed000 l: 5124
> address_space_map: addr: 0xed000, plen: 5124
> address_space_map: l: 4096, len: 5124
> address_space_map: section: 0x7fd30fabb020 memory_region_is_ram: 1 readonly: 0
> address_space_map: section: 0x7fd30fabb020 offset_within_region: 0xc8000 section size: 2146664448
> address_space_map: l: 4096, len: 1028
> address_space_map: section: 0x7fd30fabb020 memory_region_is_ram: 1 readonly: 0
> address_space_map: section: 0x7fd30fabb020 offset_within_region: 0xc8000 section size: 2146664448
> address_space_map: Calling qemu_ram_ptr_length: raddr: 0x           ed000 rlen: 5124
> address_space_map: After qemu_ram_ptr_length: raddr: 0x           ed000 rlen: 5124
> 
> So here the vhost_region_add() callback for
> section->offset_within_region: 0xc8000 for vq->ring_phys: 0xed000 is
> able to locate *section via phys_page_find() within address_space_map(),
> and cpu_physical_memory_map() completes as expected..
> 
> register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> phys_page_find got PHYS_MAP_NODE_NIL >>>>>>>>>>>>>>>>>>>>>>..
> 
> So while plodding my way through the memory API, the thing that would be
> useful to know is if the offending *section that is missing for the
> first phys_page_find() call is getting removed before the callback makes
> it's way into vhost_verify_ring_mappings() code, or that some other bug
> is occuring..?
> 
> Any idea on how this could be verified..?
> 
> Thanks,
> 
> --nab

Is it possible that what is going on here,
is that we had a region at address 0x0 size 0x80000000,
and now a chunk from it is being made readonly,
and to this end the whole old region is removed
then new ones are added?

If yes maybe the problem is that we don't use the atomic
begin/commit ops in the memory API.
Maybe the following will help?
Completely untested, posting just to give you the idea:

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/hw/vhost.c b/hw/vhost.c
index 4d6aee3..716cfaa 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -416,6 +416,25 @@ static void vhost_set_memory(MemoryListener *listener,
         /* Remove old mapping for this memory, if any. */
         vhost_dev_unassign_memory(dev, start_addr, size);
     }
+    dev->memory_changed = true;
+}
+
+static bool vhost_section(MemoryRegionSection *section)
+{
+    return memory_region_is_ram(section->mr);
+}
+
+static void vhost_begin(MemoryListener *listener)
+{
+}
+
+static void vhost_commit(MemoryListener *listener)
+{
+    struct vhost_dev *dev = container_of(listener, struct vhost_dev,
+                                         memory_listener);
+    if (!dev->memory_changed) {
+        return;
+    }
 
     if (!dev->started) {
         return;
@@ -445,19 +464,7 @@ static void vhost_set_memory(MemoryListener *listener,
     if (dev->log_size > log_size + VHOST_LOG_BUFFER) {
         vhost_dev_log_resize(dev, log_size);
     }
-}
-
-static bool vhost_section(MemoryRegionSection *section)
-{
-    return memory_region_is_ram(section->mr);
-}
-
-static void vhost_begin(MemoryListener *listener)
-{
-}
-
-static void vhost_commit(MemoryListener *listener)
-{
+    dev->memory_changed = false;
 }
 
 static void vhost_region_add(MemoryListener *listener,
@@ -842,6 +849,7 @@ int vhost_dev_init(struct vhost_dev *hdev, int devfd, const char *devpath,
     hdev->log_size = 0;
     hdev->log_enabled = false;
     hdev->started = false;
+    hdev->memory_changed = false;
     memory_listener_register(&hdev->memory_listener, &address_space_memory);
     hdev->force = force;
     return 0;

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Nicholas A. Bellinger]

On Tue, 2013-04-02 at 16:27 +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 01, 2013 at 06:05:47PM -0700, Nicholas A. Bellinger wrote:
> > On Fri, 2013-03-29 at 09:14 +0100, Paolo Bonzini wrote: 
> > > Il 29/03/2013 03:53, Nicholas A. Bellinger ha scritto:
> > > > On Thu, 2013-03-28 at 06:13 -0400, Paolo Bonzini wrote:
> > > >>> I think it's the right thing to do, but maybe not the right place
> > > >>> to do this, need to reset after all IO is done, before
> > > >>> ring memory is write protected.
> > > >>
> > > >> Our emails are crossing each other unfortunately, but I want to
> > > >> reinforce this: ring memory is not write protected.
> > > > 
> > > > Understood.  However, AFAICT the act of write protecting these ranges
> > > > for ROM generates the offending callbacks to vhost_set_memory().
> > > > 
> > > > The part that I'm missing is if ring memory is not being write protected
> > > > by make_bios_readonly_intel(), why are the vhost_set_memory() calls
> > > > being invoked..?
> > > 
> > > Because mappings change for the region that contains the ring.  vhost
> > > doesn't know yet that the changes do not affect ring memory,
> > > vhost_set_memory() is called exactly to ascertain that.
> > > 
> > 
> > Hi Paolo & Co,
> > 
> > Here's a bit more information on what is going on with the same
> > cpu_physical_memory_map() failure in vhost_verify_ring_mappings()..
> > 
> > So as before, at the point that seabios is marking memory as readonly
> > for ROM in src/shadow.c:make_bios_readonly_intel() with the following
> > call:
> > 
> > Calling pci_config_writeb(0x31): bdf: 0x0000 pam: 0x0000005b
> > 
> > the memory API update hook triggers back into vhost_region_del() code,
> > and following occurs:
> > 
> > Entering vhost_region_del section: 0x7fd30a213b60 offset_within_region: 0xc0000 size: 2146697216 readonly: 0
> > vhost_region_del: is_rom: 0, rom_device: 0
> > vhost_region_del: readable: 1
> > vhost_region_del: ram_addr 0x0, addr: 0x0 size: 2147483648
> > vhost_region_del: name: pc.ram
> > Entering vhost_set_memory, section: 0x7fd30a213b60 add: 0, dev->started: 1
> > Entering verify_ring_mappings: start_addr 0x00000000000c0000 size: 2146697216
> > verify_ring_mappings: ring_phys 0x0 ring_size: 0
> > verify_ring_mappings: ring_phys 0x0 ring_size: 0
> > verify_ring_mappings: ring_phys 0xed000 ring_size: 5124
> > verify_ring_mappings: calling cpu_physical_memory_map ring_phys: 0xed000 l: 5124
> > address_space_map: addr: 0xed000, plen: 5124
> > address_space_map: l: 4096, len: 5124
> > phys_page_find got PHYS_MAP_NODE_NIL >>>>>>>>>>>>>>>>>>>>>>..
> > address_space_map: section: 0x7fd30fabaed0 memory_region_is_ram: 0 readonly: 0
> > address_space_map: section: 0x7fd30fabaed0 offset_within_region: 0x0 section size: 18446744073709551615
> > Unable to map ring buffer for ring 2, l: 4096
> > 
> > So the interesting part is that phys_page_find() is not able to locate
> > the corresponding page for vq->ring_phys: 0xed000 from the
> > vhost_region_del() callback with section->offset_within_region:
> > 0xc0000..
> > 
> > Is there any case where this would not be considered a bug..? 
> > 
> > register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> > register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> > register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> > Entering vhost_region_add section: 0x7fd30a213aa0 offset_within_region: 0xc0000 size: 32768 readonly: 1
> > vhost_region_add: is_rom: 0, rom_device: 0
> > vhost_region_add: readable: 1
> > vhost_region_add: ram_addr 0x0000000000000000, addr: 0x               0 size: 2147483648
> > vhost_region_add: name: pc.ram
> > Entering vhost_set_memory, section: 0x7fd30a213aa0 add: 1, dev->started: 1
> > Entering verify_ring_mappings: start_addr 0x00000000000c0000 size: 32768
> > verify_ring_mappings: ring_phys 0x0 ring_size: 0
> > verify_ring_mappings: ring_phys 0x0 ring_size: 0
> > verify_ring_mappings: ring_phys 0xed000 ring_size: 5124
> > verify_ring_mappings: Got !ranges_overlap, skipping
> > register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> > Entering vhost_region_add section: 0x7fd30a213aa0 offset_within_region: 0xc8000 size: 2146664448 readonly: 0
> > vhost_region_add: is_rom: 0, rom_device: 0
> > vhost_region_add: readable: 1
> > vhost_region_add: ram_addr 0x0000000000000000, addr: 0x               0 size: 2147483648
> > vhost_region_add: name: pc.ram
> > Entering vhost_set_memory, section: 0x7fd30a213aa0 add: 1, dev->started: 1
> > Entering verify_ring_mappings: start_addr 0x00000000000c8000 size: 2146664448
> > verify_ring_mappings: ring_phys 0x0 ring_size: 0
> > verify_ring_mappings: ring_phys 0x0 ring_size: 0
> > verify_ring_mappings: ring_phys 0xed000 ring_size: 5124
> > verify_ring_mappings: calling cpu_physical_memory_map ring_phys: 0xed000 l: 5124
> > address_space_map: addr: 0xed000, plen: 5124
> > address_space_map: l: 4096, len: 5124
> > address_space_map: section: 0x7fd30fabb020 memory_region_is_ram: 1 readonly: 0
> > address_space_map: section: 0x7fd30fabb020 offset_within_region: 0xc8000 section size: 2146664448
> > address_space_map: l: 4096, len: 1028
> > address_space_map: section: 0x7fd30fabb020 memory_region_is_ram: 1 readonly: 0
> > address_space_map: section: 0x7fd30fabb020 offset_within_region: 0xc8000 section size: 2146664448
> > address_space_map: Calling qemu_ram_ptr_length: raddr: 0x           ed000 rlen: 5124
> > address_space_map: After qemu_ram_ptr_length: raddr: 0x           ed000 rlen: 5124
> > 
> > So here the vhost_region_add() callback for
> > section->offset_within_region: 0xc8000 for vq->ring_phys: 0xed000 is
> > able to locate *section via phys_page_find() within address_space_map(),
> > and cpu_physical_memory_map() completes as expected..
> > 
> > register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> > register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> > register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> > register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> > register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> > register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> > register_multipage : d: 0x7fd30f7d0ed0 section: 0x7fd30a2139b0
> > phys_page_find got PHYS_MAP_NODE_NIL >>>>>>>>>>>>>>>>>>>>>>..
> > 
> > So while plodding my way through the memory API, the thing that would be
> > useful to know is if the offending *section that is missing for the
> > first phys_page_find() call is getting removed before the callback makes
> > it's way into vhost_verify_ring_mappings() code, or that some other bug
> > is occuring..?
> > 
> > Any idea on how this could be verified..?
> > 
> > Thanks,
> > 
> > --nab
> 
> Is it possible that what is going on here,
> is that we had a region at address 0x0 size 0x80000000,
> and now a chunk from it is being made readonly,
> and to this end the whole old region is removed
> then new ones are added?

Yes, I believe this is exactly what is happening..

> 
> If yes maybe the problem is that we don't use the atomic
> begin/commit ops in the memory API.
> Maybe the following will help?
> Completely untested, posting just to give you the idea:
> 

Mmmm, one question on how vhost_region_del() + vhost_region_add() +
vhost_commit() should work..

Considering the following when the same seabios code snippet:

   pci_config_writeb(0x31): bdf: 0x0000 pam: 0x0000005b

is executed to mark an pc.ram area 0xc0000 as readonly:

Entering vhost_begin >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
Entering vhost_region_del section: 0x7fd037a4bb60 offset_within_region: 0xc0000 size: 2146697216 readonly: 0
vhost_region_del: is_rom: 0, rom_device: 0
vhost_region_del: readable: 1
vhost_region_del: ram_addr 0x0, addr: 0x0 size: 2147483648
vhost_region_del: name: pc.ram
Entering vhost_set_memory, section: 0x7fd037a4bb60 add: 0, dev->started: 1
vhost_set_memory: Setting dev->memory_changed = true for start_addr: 0xc0000
Entering vhost_region_add section: 0x7fd037a4baa0 offset_within_region: 0xc0000 size: 32768 readonly: 1
vhost_region_add is readonly !!!!!!!!!!!!!!!!!!!
vhost_region_add: is_rom: 0, rom_device: 0
vhost_region_add: readable: 1
vhost_region_add: ram_addr 0x0000000000000000, addr: 0x               0 size: 2147483648
vhost_region_add: name: pc.ram
Entering vhost_set_memory, section: 0x7fd037a4baa0 add: 1, dev->started: 1
vhost_dev_assign_memory(); >>>>>>>>>>>>>>>>>>>>>>>>>>>> reg->guest_phys_addr: 0xc0000
vhost_set_memory: Setting dev->memory_changed = true for start_addr: 0xc0000
Entering vhost_region_add section: 0x7fd037a4baa0 offset_within_region: 0xc8000 size: 2146664448 readonly: 0
vhost_region_add: is_rom: 0, rom_device: 0
vhost_region_add: readable: 1
vhost_region_add: ram_addr 0x0000000000000000, addr: 0x               0 size: 2147483648
vhost_region_add: name: pc.ram
Entering vhost_set_memory, section: 0x7fd037a4baa0 add: 1, dev->started: 1
vhost_set_memory: Setting dev->memory_changed = true for start_addr: 0xc8000
phys_page_find got PHYS_MAP_NODE_NIL >>>>>>>>>>>>>>>>>>>>>>..
Entering vhost_commit >>>>>>>>>>>>>>>>>>>>>>>>>>>

Note that originally we'd see the cpu_physical_memory_map() failure in
vhost_verify_ring_mappings() after the first ->region_del() above.

Adding a hardcoded cpu_physical_memory_map() testcase in vhost_commit()
for phys_addr=0xed000, len=5124 (vq ring) does locate the correct
*section from address_space_map(), which correct points to the section
generated by the last vhost_region_add() above:

Entering vhost_commit >>>>>>>>>>>>>>>>>>>>>>>>>>>
address_space_map: addr: 0xed000, plen: 5124
address_space_map: l: 4096, len: 5124
address_space_map: section: 0x7f41b325f020 memory_region_is_ram: 1 readonly: 0
address_space_map: section: 0x7f41b325f020 offset_within_region: 0xc8000 section size: 2146664448
address_space_map: l: 4096, len: 1028
address_space_map: section: 0x7f41b325f020 memory_region_is_ram: 1 readonly: 0
address_space_map: section: 0x7f41b325f020 offset_within_region: 0xc8000 section size: 2146664448
address_space_map: Calling qemu_ram_ptr_length: raddr: 0x           ed000 rlen: 5124
address_space_map: After qemu_ram_ptr_length: raddr: 0x           ed000 rlen: 5124
cpu_physical_memory_map(0xed000) got l: 5124

So, does using a ->commit callback for MemoryListener  mean that
vhost_verify_ring_mappings() is OK to be called only from the final
->commit callback, and not from each ->region_del + ->region_add
callback..?   Eg: I seem to recall something about
vhost_verify_ring_mappings() being called during each ->region_del()
when dev->started == true was important, no..?

If this OK, then it seems a matter of keeping an updated bit for each of
the regions in vhost_dev->mem_sections[] and performing the
vhost_verify_ring_mappings() on all three above during the final
->commit() call, right..?

WDYT..?

--nab

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> ---
> 
> diff --git a/hw/vhost.c b/hw/vhost.c
> index 4d6aee3..716cfaa 100644
> --- a/hw/vhost.c
> +++ b/hw/vhost.c
> @@ -416,6 +416,25 @@ static void vhost_set_memory(MemoryListener *listener,
>          /* Remove old mapping for this memory, if any. */
>          vhost_dev_unassign_memory(dev, start_addr, size);
>      }
> +    dev->memory_changed = true;
> +}
> +
> +static bool vhost_section(MemoryRegionSection *section)
> +{
> +    return memory_region_is_ram(section->mr);
> +}
> +
> +static void vhost_begin(MemoryListener *listener)
> +{
> +}
> +
> +static void vhost_commit(MemoryListener *listener)
> +{
> +    struct vhost_dev *dev = container_of(listener, struct vhost_dev,
> +                                         memory_listener);
> +    if (!dev->memory_changed) {
> +        return;
> +    }
>  
>      if (!dev->started) {
>          return;
> @@ -445,19 +464,7 @@ static void vhost_set_memory(MemoryListener *listener,
>      if (dev->log_size > log_size + VHOST_LOG_BUFFER) {
>          vhost_dev_log_resize(dev, log_size);
>      }
> -}
> -
> -static bool vhost_section(MemoryRegionSection *section)
> -{
> -    return memory_region_is_ram(section->mr);
> -}
> -
> -static void vhost_begin(MemoryListener *listener)
> -{
> -}
> -
> -static void vhost_commit(MemoryListener *listener)
> -{
> +    dev->memory_changed = false;
>  }
>  
>  static void vhost_region_add(MemoryListener *listener,
> @@ -842,6 +849,7 @@ int vhost_dev_init(struct vhost_dev *hdev, int devfd, const char *devpath,
>      hdev->log_size = 0;
>      hdev->log_enabled = false;
>      hdev->started = false;
> +    hdev->memory_changed = false;
>      memory_listener_register(&hdev->memory_listener, &address_space_memory);
>      hdev->force = force;
>      return 0;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Nicholas A. Bellinger]

On Tue, 2013-04-02 at 21:04 -0700, Nicholas A. Bellinger wrote:
> On Tue, 2013-04-02 at 16:27 +0300, Michael S. Tsirkin wrote:
> > On Mon, Apr 01, 2013 at 06:05:47PM -0700, Nicholas A. Bellinger wrote:
> > > On Fri, 2013-03-29 at 09:14 +0100, Paolo Bonzini wrote: 
> > > > Il 29/03/2013 03:53, Nicholas A. Bellinger ha scritto:
> > > > > On Thu, 2013-03-28 at 06:13 -0400, Paolo Bonzini wrote:
> > > > >>> I think it's the right thing to do, but maybe not the right place
> > > > >>> to do this, need to reset after all IO is done, before
> > > > >>> ring memory is write protected.
> > > > >>
> > > > >> Our emails are crossing each other unfortunately, but I want to
> > > > >> reinforce this: ring memory is not write protected.
> > > > > 
> > > > > Understood.  However, AFAICT the act of write protecting these ranges
> > > > > for ROM generates the offending callbacks to vhost_set_memory().
> > > > > 
> > > > > The part that I'm missing is if ring memory is not being write protected
> > > > > by make_bios_readonly_intel(), why are the vhost_set_memory() calls
> > > > > being invoked..?
> > > > 
> > > > Because mappings change for the region that contains the ring.  vhost
> > > > doesn't know yet that the changes do not affect ring memory,
> > > > vhost_set_memory() is called exactly to ascertain that.
> > > > 

<SNIP>

> > 
> > Is it possible that what is going on here,
> > is that we had a region at address 0x0 size 0x80000000,
> > and now a chunk from it is being made readonly,
> > and to this end the whole old region is removed
> > then new ones are added?
> 
> Yes, I believe this is exactly what is happening..
> 
> > 
> > If yes maybe the problem is that we don't use the atomic
> > begin/commit ops in the memory API.
> > Maybe the following will help?
> > Completely untested, posting just to give you the idea:
> > 
> 
> Mmmm, one question on how vhost_region_del() + vhost_region_add() +
> vhost_commit() should work..
> 
> Considering the following when the same seabios code snippet:
> 
>    pci_config_writeb(0x31): bdf: 0x0000 pam: 0x0000005b
> 
> is executed to mark an pc.ram area 0xc0000 as readonly:
> 
> Entering vhost_begin >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> Entering vhost_region_del section: 0x7fd037a4bb60 offset_within_region: 0xc0000 size: 2146697216 readonly: 0
> vhost_region_del: is_rom: 0, rom_device: 0
> vhost_region_del: readable: 1
> vhost_region_del: ram_addr 0x0, addr: 0x0 size: 2147483648
> vhost_region_del: name: pc.ram
> Entering vhost_set_memory, section: 0x7fd037a4bb60 add: 0, dev->started: 1
> vhost_set_memory: Setting dev->memory_changed = true for start_addr: 0xc0000
> Entering vhost_region_add section: 0x7fd037a4baa0 offset_within_region: 0xc0000 size: 32768 readonly: 1
> vhost_region_add is readonly !!!!!!!!!!!!!!!!!!!
> vhost_region_add: is_rom: 0, rom_device: 0
> vhost_region_add: readable: 1
> vhost_region_add: ram_addr 0x0000000000000000, addr: 0x               0 size: 2147483648
> vhost_region_add: name: pc.ram
> Entering vhost_set_memory, section: 0x7fd037a4baa0 add: 1, dev->started: 1
> vhost_dev_assign_memory(); >>>>>>>>>>>>>>>>>>>>>>>>>>>> reg->guest_phys_addr: 0xc0000
> vhost_set_memory: Setting dev->memory_changed = true for start_addr: 0xc0000
> Entering vhost_region_add section: 0x7fd037a4baa0 offset_within_region: 0xc8000 size: 2146664448 readonly: 0
> vhost_region_add: is_rom: 0, rom_device: 0
> vhost_region_add: readable: 1
> vhost_region_add: ram_addr 0x0000000000000000, addr: 0x               0 size: 2147483648
> vhost_region_add: name: pc.ram
> Entering vhost_set_memory, section: 0x7fd037a4baa0 add: 1, dev->started: 1
> vhost_set_memory: Setting dev->memory_changed = true for start_addr: 0xc8000
> phys_page_find got PHYS_MAP_NODE_NIL >>>>>>>>>>>>>>>>>>>>>>..
> Entering vhost_commit >>>>>>>>>>>>>>>>>>>>>>>>>>>
> 
> Note that originally we'd see the cpu_physical_memory_map() failure in
> vhost_verify_ring_mappings() after the first ->region_del() above.
> 
> Adding a hardcoded cpu_physical_memory_map() testcase in vhost_commit()
> for phys_addr=0xed000, len=5124 (vq ring) does locate the correct
> *section from address_space_map(), which correct points to the section
> generated by the last vhost_region_add() above:
> 
> Entering vhost_commit >>>>>>>>>>>>>>>>>>>>>>>>>>>
> address_space_map: addr: 0xed000, plen: 5124
> address_space_map: l: 4096, len: 5124
> address_space_map: section: 0x7f41b325f020 memory_region_is_ram: 1 readonly: 0
> address_space_map: section: 0x7f41b325f020 offset_within_region: 0xc8000 section size: 2146664448
> address_space_map: l: 4096, len: 1028
> address_space_map: section: 0x7f41b325f020 memory_region_is_ram: 1 readonly: 0
> address_space_map: section: 0x7f41b325f020 offset_within_region: 0xc8000 section size: 2146664448
> address_space_map: Calling qemu_ram_ptr_length: raddr: 0x           ed000 rlen: 5124
> address_space_map: After qemu_ram_ptr_length: raddr: 0x           ed000 rlen: 5124
> cpu_physical_memory_map(0xed000) got l: 5124
> 
> So, does using a ->commit callback for MemoryListener  mean that
> vhost_verify_ring_mappings() is OK to be called only from the final
> ->commit callback, and not from each ->region_del + ->region_add
> callback..?   Eg: I seem to recall something about
> vhost_verify_ring_mappings() being called during each ->region_del()
> when dev->started == true was important, no..?
> 
> If this OK, then it seems a matter of keeping an updated bit for each of
> the regions in vhost_dev->mem_sections[] and performing the
> vhost_verify_ring_mappings() on all three above during the final
> ->commit() call, right..?

Or even better, what about just invoking vhost_verify_ring_mappings()
once from ->commit without section start_addr+size + drop the existing
!rings_overlap check..?

Does the following look like you'd expect for marking 0xc0000 area as
read-only case..?

Entering vhost_begin >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
Entering vhost_region_del section: 0x7f16d3a1db60 offset_within_region: 0xc0000 size: 2146697216 readonly: 0
vhost_region_del: is_rom: 0, rom_device: 0
vhost_region_del: readable: 1
vhost_region_del: ram_addr 0x0, addr: 0x0 size: 2147483648
vhost_region_del: name: pc.ram
Entering vhost_set_memory, section: 0x7f16d3a1db60 add: 0, dev->started: 1
vhost_set_memory: Setting dev->memory_changed = true for start_addr: 0xc0000
Entering vhost_region_add section: 0x7f16d3a1daa0 offset_within_region: 0xc0000 size: 32768 readonly: 1
vhost_region_add is readonly !!!!!!!!!!!!!!!!!!!
vhost_region_add: is_rom: 0, rom_device: 0
vhost_region_add: readable: 1
vhost_region_add: ram_addr 0x0000000000000000, addr: 0x               0 size: 2147483648
vhost_region_add: name: pc.ram
Entering vhost_set_memory, section: 0x7f16d3a1daa0 add: 1, dev->started: 1
vhost_dev_assign_memory(); >>>>>>>>>>>>>>>>>>>>>>>>>>>> reg->guest_phys_addr: 0xc0000
vhost_set_memory: Setting dev->memory_changed = true for start_addr: 0xc0000
Entering vhost_region_add section: 0x7f16d3a1daa0 offset_within_region: 0xc8000 size: 2146664448 readonly: 0
vhost_region_add: is_rom: 0, rom_device: 0
vhost_region_add: readable: 1
vhost_region_add: ram_addr 0x0000000000000000, addr: 0x               0 size: 2147483648
vhost_region_add: name: pc.ram
Entering vhost_set_memory, section: 0x7f16d3a1daa0 add: 1, dev->started: 1
vhost_set_memory: Setting dev->memory_changed = true for start_addr: 0xc8000
phys_page_find got PHYS_MAP_NODE_NIL >>>>>>>>>>>>>>>>>>>>>>..
Entering vhost_commit >>>>>>>>>>>>>>>>>>>>>>>>>>>
vhost_commit, skipping vhost_verify_ring_mappings without start_addr !!!!!!!!!!!!!
Entering verify_ring_mappings: start_addr 0x0000000000000000 size: 0
verify_ring_mappings: ring_phys 0x0 ring_size: 0
verify_ring_mappings: ring_phys 0x0 ring_size: 0
verify_ring_mappings: ring_phys 0xed000 ring_size: 5124
verify_ring_mappings: calling cpu_physical_memory_map ring_phys: 0xed000 l: 5124
address_space_map: addr: 0xed000, plen: 5124
address_space_map: l: 4096, len: 5124
address_space_map: section: 0x7f16d92c6020 memory_region_is_ram: 1 readonly: 0
address_space_map: section: 0x7f16d92c6020 offset_within_region: 0xc8000 section size: 2146664448
address_space_map: l: 4096, len: 1028
address_space_map: section: 0x7f16d92c6020 memory_region_is_ram: 1 readonly: 0
address_space_map: section: 0x7f16d92c6020 offset_within_region: 0xc8000 section size: 2146664448
address_space_map: Calling qemu_ram_ptr_length: raddr: 0x           ed000 rlen: 5124
address_space_map: After qemu_ram_ptr_length: raddr: 0x           ed000 rlen: 5124

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Paolo Bonzini]

> > Considering the following when the same seabios code snippet:
> > 
> >    pci_config_writeb(0x31): bdf: 0x0000 pam: 0x0000005b
> > 
> > is executed to mark an pc.ram area 0xc0000 as readonly:
> > 
> > Entering vhost_begin >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> > Entering vhost_region_del section: 0x7fd037a4bb60 offset_within_region:
> > 0xc0000 size: 2146697216 readonly: 0
> > vhost_region_del: is_rom: 0, rom_device: 0
> > vhost_region_del: readable: 1
> > vhost_region_del: ram_addr 0x0, addr: 0x0 size: 2147483648
> > vhost_region_del: name: pc.ram
> > Entering vhost_set_memory, section: 0x7fd037a4bb60 add: 0, dev->started: 1
> > vhost_set_memory: Setting dev->memory_changed = true for start_addr:
> > 0xc0000
> > Entering vhost_region_add section: 0x7fd037a4baa0 offset_within_region:
> > 0xc0000 size: 32768 readonly: 1
> > vhost_region_add is readonly !!!!!!!!!!!!!!!!!!!
> > vhost_region_add: is_rom: 0, rom_device: 0
> > vhost_region_add: readable: 1
> > vhost_region_add: ram_addr 0x0000000000000000, addr: 0x               0
> > size: 2147483648
> > vhost_region_add: name: pc.ram
> > Entering vhost_set_memory, section: 0x7fd037a4baa0 add: 1, dev->started: 1
> > vhost_dev_assign_memory(); >>>>>>>>>>>>>>>>>>>>>>>>>>>>
> > reg->guest_phys_addr: 0xc0000
> > vhost_set_memory: Setting dev->memory_changed = true for start_addr:
> > 0xc0000
> > Entering vhost_region_add section: 0x7fd037a4baa0 offset_within_region:
> > 0xc8000 size: 2146664448 readonly: 0
> > vhost_region_add: is_rom: 0, rom_device: 0
> > vhost_region_add: readable: 1
> > vhost_region_add: ram_addr 0x0000000000000000, addr: 0x               0
> > size: 2147483648
> > vhost_region_add: name: pc.ram
> > Entering vhost_set_memory, section: 0x7fd037a4baa0 add: 1, dev->started: 1
> > vhost_set_memory: Setting dev->memory_changed = true for start_addr:
> > 0xc8000
> > phys_page_find got PHYS_MAP_NODE_NIL >>>>>>>>>>>>>>>>>>>>>>..
> > Entering vhost_commit >>>>>>>>>>>>>>>>>>>>>>>>>>>
> > 
> > Note that originally we'd see the cpu_physical_memory_map() failure in
> > vhost_verify_ring_mappings() after the first ->region_del() above.
> > 
> > So, does using a ->commit callback for MemoryListener  mean that
> > vhost_verify_ring_mappings() is OK to be called only from the final
> > ->commit callback, and not from each ->region_del + ->region_add
> > callback..?   Eg: I seem to recall something about
> > vhost_verify_ring_mappings() being called during each ->region_del()
> > when dev->started == true was important, no..?

It is important in the case there are only some deleted regions, and
no added region, or in general when the last callback is a ->region_del().

But it is even better to just call vhost_verify_ring_mappings() once,
from the ->region_commit() callback.

> > If this OK, then it seems a matter of keeping an updated bit for each of
> > the regions in vhost_dev->mem_sections[] and performing the
> > vhost_verify_ring_mappings() on all three above during the final
> > ->commit() call, right..?
> 
> Or even better, what about just invoking vhost_verify_ring_mappings()
> once from ->commit without section start_addr+size + drop the existing
> !rings_overlap check..?

Either drop the ranges_overlap check, or keep the start_addr/end_addr
up-to-date.  Compared to Michael's prototype patch, that would be like
this:

   vhost_begin()
   {
       dev->mem_changed_end_addr = 0;
       dev->mem_changed_start_addr = -1;
   }

   vhost_set_memory()
   {
       ...
       dev->mem_changed_start_addr = MIN(dev->mem_changed_start_addr, start_addr);
       dev->mem_changed_end_addr = MAX(dev->mem_changed_end_addr, start_addr + size - 1);
   }

   vhost_commit()
   {
       if (dev->mem_changed_start_addr > dev->mem_changed_end_addr) {
           return;
       }
       start_addr = dev->mem_changed_start_addr;
       size = dev->mem_changed_end_addr - dev->mem_changed_start_addr + 1;
       ...
   }

Paolo

> Does the following look like you'd expect for marking 0xc0000 area as
> read-only case..?
> 
> Entering vhost_begin >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> Entering vhost_region_del section: 0x7f16d3a1db60 offset_within_region:
> 0xc0000 size: 2146697216 readonly: 0
> vhost_region_del: is_rom: 0, rom_device: 0
> vhost_region_del: readable: 1
> vhost_region_del: ram_addr 0x0, addr: 0x0 size: 2147483648
> vhost_region_del: name: pc.ram
> Entering vhost_set_memory, section: 0x7f16d3a1db60 add: 0, dev->started: 1
> vhost_set_memory: Setting dev->memory_changed = true for start_addr: 0xc0000
> Entering vhost_region_add section: 0x7f16d3a1daa0 offset_within_region:
> 0xc0000 size: 32768 readonly: 1
> vhost_region_add is readonly !!!!!!!!!!!!!!!!!!!
> vhost_region_add: is_rom: 0, rom_device: 0
> vhost_region_add: readable: 1
> vhost_region_add: ram_addr 0x0000000000000000, addr: 0x               0 size:
> 2147483648
> vhost_region_add: name: pc.ram
> Entering vhost_set_memory, section: 0x7f16d3a1daa0 add: 1, dev->started: 1
> vhost_dev_assign_memory(); >>>>>>>>>>>>>>>>>>>>>>>>>>>> reg->guest_phys_addr:
> 0xc0000
> vhost_set_memory: Setting dev->memory_changed = true for start_addr: 0xc0000
> Entering vhost_region_add section: 0x7f16d3a1daa0 offset_within_region:
> 0xc8000 size: 2146664448 readonly: 0
> vhost_region_add: is_rom: 0, rom_device: 0
> vhost_region_add: readable: 1
> vhost_region_add: ram_addr 0x0000000000000000, addr: 0x               0 size:
> 2147483648
> vhost_region_add: name: pc.ram
> Entering vhost_set_memory, section: 0x7f16d3a1daa0 add: 1, dev->started: 1
> vhost_set_memory: Setting dev->memory_changed = true for start_addr: 0xc8000
> phys_page_find got PHYS_MAP_NODE_NIL >>>>>>>>>>>>>>>>>>>>>>..
> Entering vhost_commit >>>>>>>>>>>>>>>>>>>>>>>>>>>
> vhost_commit, skipping vhost_verify_ring_mappings without start_addr
> !!!!!!!!!!!!!
> Entering verify_ring_mappings: start_addr 0x0000000000000000 size: 0
> verify_ring_mappings: ring_phys 0x0 ring_size: 0
> verify_ring_mappings: ring_phys 0x0 ring_size: 0
> verify_ring_mappings: ring_phys 0xed000 ring_size: 5124
> verify_ring_mappings: calling cpu_physical_memory_map ring_phys: 0xed000 l:
> 5124
> address_space_map: addr: 0xed000, plen: 5124
> address_space_map: l: 4096, len: 5124
> address_space_map: section: 0x7f16d92c6020 memory_region_is_ram: 1 readonly:
> 0
> address_space_map: section: 0x7f16d92c6020 offset_within_region: 0xc8000
> section size: 2146664448
> address_space_map: l: 4096, len: 1028
> address_space_map: section: 0x7f16d92c6020 memory_region_is_ram: 1 readonly:
> 0
> address_space_map: section: 0x7f16d92c6020 offset_within_region: 0xc8000
> section size: 2146664448
> address_space_map: Calling qemu_ram_ptr_length: raddr: 0x           ed000
> rlen: 5124
> address_space_map: After qemu_ram_ptr_length: raddr: 0x           ed000 rlen:
> 5124
> 
> 

Re: [Qemu-devel] [PATCH V3 WIP 3/3] disable vhost_verify_ring_mappings check

[Nicholas A. Bellinger]

On Thu, 2013-03-28 at 11:04 +0200, Michael S. Tsirkin wrote:
> On Thu, Mar 28, 2013 at 12:35:42AM -0700, Nicholas A. Bellinger wrote:
> > On Wed, 2013-03-27 at 23:45 -0700, Nicholas A. Bellinger wrote:
> > > On Wed, 2013-03-27 at 15:33 -0700, Nicholas A. Bellinger wrote:
> > > > On Wed, 2013-03-27 at 23:56 +0200, Michael S. Tsirkin wrote:
> > > > > On Wed, Mar 27, 2013 at 02:31:27PM -0700, Nicholas A. Bellinger wrote:
> > 

<SNIP>

> > locking shadow ram
> > romend: 0x000cb800 romtop: 0x000ec000
> > mem: 0x000c0000, pam: 0x0000005a
> > Calling pci_config_writeb(0x11): bdf: 0x0000 pam: 0x0000005a
> > 
> > > <No QEMU output after pci_config_writeb(0x11) in make_bios_readonly..>
> > > 
> > > Calling pci_config_writeb(0x31): bdf: 0x0000 pam: 0x0000005b
> > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > 
> > 
> > mem: 0x000c8000, pam: 0x0000005b
> > romend: 0x000cb800 mem + 16*1024: 0x000cc000
> > romtop: 0x000ec000 mem + 32*1024: 0x000d0000
> > >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> > romend: 0x000cb800, mem: 0x000c8000, romtop: 0x000ec000, mem + 16*1024: 0x000cc000
> > Calling pci_config_writeb(0x31): bdf: 0x0000 pam: 0x0000005b
> > 
> > 
> > > <QEMU output after pci_config_writeb(0x31) in make_bios_readonly..>
> > > 
> > > vhost_set_memory: section: 0x7fe2801f2b60 section->size: 2146697216 add: 0
> > > Before vhost_verify_ring_mappings: start_addr: c0000 size: 2146697216
> > > Checking vq: 0 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> 
> This is also a bug. -net always initializes VQs 0..N so this is what
> vhost assumed.  Please teach vhost that it should skip uninitialized
> VQs. There are more places to fix.
> Basically look for if (!virtio_queue_get_num(vdev, queue_no)),
> all of them need to be updated to skip uninitialized vqs.
> Probably switch to a new API checking PA too.
> See patch below.

<nod>

> 
> > > Got ranges_overlap for vq: 0 ring_phys: 0 ring_size: 1028
> > > Checking vq: 1 ring_phys: 0 ring_size: 1028 >>>>>>>>>>>>>>>>>>.
> > > Got ranges_overlap for vq: 1 ring_phys: 0 ring_size: 1028
> > > Checking vq: 2 ring_phys: ed000 ring_size: 5124 >>>>>>>>>>>>>>>>>>.
> > > Calling l: 5124 for start_addr: c0000 for vq 2
> > > Unable to map ring buffer for ring 2
> > > l: 4096 ring_size: 5124
> 
> okay so the ring address is within ROM.
> Unsurprisingly it fails.
> bios should stop device before write protect.

<SNIP>

> ---
> 
> virtio: add API to check that ring is setup
> 
> virtio scsi makes it legal to only setup a subset of rings.  The only
> way to detect the ring is setup seems to be to check whether PA was
> written to.  Add API to do this, and teach code to use it instead of
> checking hardware queue size.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> --->
> 
> diff --git a/hw/virtio.c b/hw/virtio.c
> index 26fbc79..ac12c01 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -651,6 +651,11 @@ int virtio_queue_get_num(VirtIODevice *vdev, int n)
>      return vdev->vq[n].vring.num;
>  }
>  
> +bool virtio_queue_valid(VirtIODevice *vdev, int n)
> +{
> +    return vdev->vq[n].vring.num && vdev->vq[n].vring.pa;
> +}

I assume you mean vring.desc here, right..?

Sending out these as a separate patch series shortly.

--nab