| Message ID | 1452861718-25806-2-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 15 Jan 2016 13:41:49 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > The next patch will make virtqueue_pop/vring_pop allocate memory for a s/will make/will make it possible for/ ? I had to spend some time grepping through the code to find that blk and scsi (and gpu, which already had elem at the beginning of its structure) are the only ones that work like this and that other devices do not need any change. > "subclass" of VirtQueueElement. For this to work, VirtQueueElement > must be the first field in the containing struct. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/scsi/virtio-scsi.c | 3 +-- > include/hw/virtio/virtio-blk.h | 2 +- > include/hw/virtio/virtio-scsi.h | 13 ++++++------- > 3 files changed, 8 insertions(+), 10 deletions(-) Otherwise, Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
On 19/01/2016 13:09, Cornelia Huck wrote: >> > The next patch will make virtqueue_pop/vring_pop allocate memory for a > s/will make/will make it possible for/ > > ? This patch will actually do that. This patch makes it possible. > I had to spend some time grepping through the code to find that blk and > scsi (and gpu, which already had elem at the beginning of its > structure) are the only ones that work like this and that other devices > do not need any change. > >> > "subclass" of VirtQueueElement. For this to work, VirtQueueElement >> > must be the first field in the containing struct. So... The next patch will make virtqueue_pop/vring_pop allocate memory for the VirtQueueElement. In some cases (blk, scsi, gpu) the device wants to extend VirtQueueElement with device-specific fields and, until now, the place of the VirtQueueElement within the containing struct didn't matter. When allocating the entire block in virtqueue_pop/vring_pop, however, the containing struct must basically be a "subclass" of VirtQueueElement, with the VirtQueueElement as the first field. Make that the case for blk and scsi; gpu is already doing it. Paolo >> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> > --- >> > hw/scsi/virtio-scsi.c | 3 +-- >> > include/hw/virtio/virtio-blk.h | 2 +- >> > include/hw/virtio/virtio-scsi.h | 13 ++++++------- >> > 3 files changed, 8 insertions(+), 10 deletions(-) > Otherwise, > > Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com> > > >
On Tue, 19 Jan 2016 14:22:37 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > The next patch will make virtqueue_pop/vring_pop allocate memory for the > VirtQueueElement. In some cases (blk, scsi, gpu) the device wants to > extend VirtQueueElement with device-specific fields and, until now, the > place of the VirtQueueElement within the containing struct didn't > matter. When allocating the entire block in virtqueue_pop/vring_pop, > however, the containing struct must basically be a "subclass" of > VirtQueueElement, with the VirtQueueElement as the first field. Make > that the case for blk and scsi; gpu is already doing it. Sounds good!
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c index 3a4f520..1567da8 100644 --- a/hw/scsi/virtio-scsi.c +++ b/hw/scsi/virtio-scsi.c @@ -44,8 +44,7 @@ VirtIOSCSIReq *virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq) { VirtIOSCSIReq *req; VirtIOSCSICommon *vs = (VirtIOSCSICommon *)s; - const size_t zero_skip = offsetof(VirtIOSCSIReq, elem) - + sizeof(VirtQueueElement); + const size_t zero_skip = offsetof(VirtIOSCSIReq, vring); req = g_malloc(sizeof(*req) + vs->cdb_size); req->vq = vq; diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h index ae11a63..403ab86 100644 --- a/include/hw/virtio/virtio-blk.h +++ b/include/hw/virtio/virtio-blk.h @@ -60,9 +60,9 @@ typedef struct VirtIOBlock { } VirtIOBlock; typedef struct VirtIOBlockReq { + VirtQueueElement elem; int64_t sector_num; VirtIOBlock *dev; - VirtQueueElement elem; struct virtio_blk_inhdr *in; struct virtio_blk_outhdr out; QEMUIOVector qiov; diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h index 088fe9f..63f5b51 100644 --- a/include/hw/virtio/virtio-scsi.h +++ b/include/hw/virtio/virtio-scsi.h @@ -102,18 +102,17 @@ typedef struct VirtIOSCSI { } VirtIOSCSI; typedef struct VirtIOSCSIReq { + /* Note: + * - fields up to resp_iov are initialized by virtio_scsi_init_req; + * - fields starting at vring are zeroed by virtio_scsi_init_req. + * */ + VirtQueueElement elem; + VirtIOSCSI *dev; VirtQueue *vq; QEMUSGList qsgl; QEMUIOVector resp_iov; - /* Note: - * - fields before elem are initialized by virtio_scsi_init_req; - * - elem is uninitialized at the time of allocation. - * - fields after elem are zeroed by virtio_scsi_init_req. - * */ - - VirtQueueElement elem; /* Set by dataplane code. */ VirtIOSCSIVring *vring;
The next patch will make virtqueue_pop/vring_pop allocate memory for a "subclass" of VirtQueueElement. For this to work, VirtQueueElement must be the first field in the containing struct. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/scsi/virtio-scsi.c | 3 +-- include/hw/virtio/virtio-blk.h | 2 +- include/hw/virtio/virtio-scsi.h | 13 ++++++------- 3 files changed, 8 insertions(+), 10 deletions(-)