| Message ID | 1485255263-12612-1-git-send-email-lvivier@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
On 24.01.2017 11:54, Laurent Vivier wrote: > From: Laurent Vivier <lvivier@ibm-p8-virt-03.lab.eng.rdu.redhat.com> > > virtioscsi_init() uses the result of virtio_get_vring_avail() whereas > the queue is not enabled: on the first boot, its value is NULL and > the driver uses this to communicate with the device. After a reboot, > its value is the one given by the OS driver, and it works if the > address is in a range SLOF can access. > > In some cases, for instance with NUMA nodes and hotplugged memory, > SLOF cannot access the address set by the kernel, and virtioscsi_init() > fails with a data storage exception. > > To set the vring avail buffer address, we need to enable the queue, what > is done by virtio_set_qaddr(). > > This patch fixes the problem by calling virtio_queue_init_vq() (like other > virtio drivers) in virtioscsi_init() as it allocates memory and enables the > queue. virtio_queue_init_vq() also replaces the calls to virtio_vring_size() > and virtio_get_vring_avail(). > --- > v4: remove queue-control-addr, queue-event-addr, queue-cmd-addr > > v3: remove setup-virt-queues > > v2: move vqs declarations to the function > only call virtio_cpu_to_modern16() once > add CC: to Nikunj > > board-qemu/slof/virtio-scsi.fs | 21 --------------------- > lib/libvirtio/virtio-scsi.c | 28 +++++++++++++++------------- > 2 files changed, 15 insertions(+), 34 deletions(-) > > diff --git a/board-qemu/slof/virtio-scsi.fs b/board-qemu/slof/virtio-scsi.fs > index 4fedeee..d52741e 100644 > --- a/board-qemu/slof/virtio-scsi.fs > +++ b/board-qemu/slof/virtio-scsi.fs > @@ -165,26 +165,6 @@ scsi-open > > scsi-close \ no further scsi words required > > -0 VALUE queue-control-addr > -0 VALUE queue-event-addr > -0 VALUE queue-cmd-addr > - > -: setup-virt-queues > - \ add 3 queues 0-controlq, 1-eventq, 2-cmdq > - \ fixme: do we need to find more than the above 3 queues if exists > - virtiodev 0 virtio-get-qsize virtio-vring-size > - alloc-mem to queue-control-addr > - virtiodev 0 queue-control-addr virtio-set-qaddr > - > - virtiodev 1 virtio-get-qsize virtio-vring-size > - alloc-mem to queue-event-addr > - virtiodev 1 queue-event-addr virtio-set-qaddr > - > - virtiodev 2 virtio-get-qsize virtio-vring-size > - alloc-mem to queue-cmd-addr > - virtiodev 2 queue-cmd-addr virtio-set-qaddr > -; > - > \ Set scsi alias if none is set yet > : setup-alias > s" scsi" find-alias 0= IF > @@ -212,7 +192,6 @@ scsi-close \ no further scsi words required > \ Scan the VSCSI bus: > virtiodev virtio-scsi-init > 0= IF > - setup-virt-queues > scsi-find-disks > setup-alias > TRUE to initialized? > diff --git a/lib/libvirtio/virtio-scsi.c b/lib/libvirtio/virtio-scsi.c > index 36b62d1..e95352d 100644 > --- a/lib/libvirtio/virtio-scsi.c > +++ b/lib/libvirtio/virtio-scsi.c > @@ -99,10 +99,9 @@ int virtioscsi_send(struct virtio_device *dev, > */ > int virtioscsi_init(struct virtio_device *dev) > { > - struct vring_avail *vq_avail; > - unsigned int idx = 0; > - int qsize = 0; > + struct vqs vq_ctrl, vq_event, vq_request; > int status = VIRTIO_STAT_ACKNOWLEDGE; > + uint16_t flags; > > /* Reset device */ > // XXX That will clear the virtq base. We need to move > @@ -126,17 +125,20 @@ int virtioscsi_init(struct virtio_device *dev) > virtio_set_guest_features(dev, 0); > } > > - while(1) { > - qsize = virtio_get_qsize(dev, idx); > - if (!qsize) > - break; > - virtio_vring_size(qsize); > + if (virtio_queue_init_vq(dev, &vq_ctrl, VIRTIO_SCSI_CONTROL_VQ) || > + virtio_queue_init_vq(dev, &vq_event, VIRTIO_SCSI_EVENT_VQ) || > + virtio_queue_init_vq(dev, &vq_request, VIRTIO_SCSI_REQUEST_VQ)) > + goto dev_error; > > - vq_avail = virtio_get_vring_avail(dev, idx); > - vq_avail->flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT); > - vq_avail->idx = 0; > - idx++; > - } > + flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT); > + vq_ctrl.avail->flags = flags; > + vq_ctrl.avail->idx = 0; > + > + vq_event.avail->flags = flags; > + vq_event.avail->idx = 0; > + > + vq_request.avail->flags = flags; > + vq_request.avail->idx = 0; > > /* Tell HV that setup succeeded */ > status |= VIRTIO_STAT_DRIVER_OK; > Reviewed-by: Thomas Huth <thuth@redhat.com>
Ping? Laurent On 24/01/2017 11:54, Laurent Vivier wrote: > From: Laurent Vivier <lvivier@ibm-p8-virt-03.lab.eng.rdu.redhat.com> > > virtioscsi_init() uses the result of virtio_get_vring_avail() whereas > the queue is not enabled: on the first boot, its value is NULL and > the driver uses this to communicate with the device. After a reboot, > its value is the one given by the OS driver, and it works if the > address is in a range SLOF can access. > > In some cases, for instance with NUMA nodes and hotplugged memory, > SLOF cannot access the address set by the kernel, and virtioscsi_init() > fails with a data storage exception. > > To set the vring avail buffer address, we need to enable the queue, what > is done by virtio_set_qaddr(). > > This patch fixes the problem by calling virtio_queue_init_vq() (like other > virtio drivers) in virtioscsi_init() as it allocates memory and enables the > queue. virtio_queue_init_vq() also replaces the calls to virtio_vring_size() > and virtio_get_vring_avail(). > --- > v4: remove queue-control-addr, queue-event-addr, queue-cmd-addr > > v3: remove setup-virt-queues > > v2: move vqs declarations to the function > only call virtio_cpu_to_modern16() once > add CC: to Nikunj > > board-qemu/slof/virtio-scsi.fs | 21 --------------------- > lib/libvirtio/virtio-scsi.c | 28 +++++++++++++++------------- > 2 files changed, 15 insertions(+), 34 deletions(-) > > diff --git a/board-qemu/slof/virtio-scsi.fs b/board-qemu/slof/virtio-scsi.fs > index 4fedeee..d52741e 100644 > --- a/board-qemu/slof/virtio-scsi.fs > +++ b/board-qemu/slof/virtio-scsi.fs > @@ -165,26 +165,6 @@ scsi-open > > scsi-close \ no further scsi words required > > -0 VALUE queue-control-addr > -0 VALUE queue-event-addr > -0 VALUE queue-cmd-addr > - > -: setup-virt-queues > - \ add 3 queues 0-controlq, 1-eventq, 2-cmdq > - \ fixme: do we need to find more than the above 3 queues if exists > - virtiodev 0 virtio-get-qsize virtio-vring-size > - alloc-mem to queue-control-addr > - virtiodev 0 queue-control-addr virtio-set-qaddr > - > - virtiodev 1 virtio-get-qsize virtio-vring-size > - alloc-mem to queue-event-addr > - virtiodev 1 queue-event-addr virtio-set-qaddr > - > - virtiodev 2 virtio-get-qsize virtio-vring-size > - alloc-mem to queue-cmd-addr > - virtiodev 2 queue-cmd-addr virtio-set-qaddr > -; > - > \ Set scsi alias if none is set yet > : setup-alias > s" scsi" find-alias 0= IF > @@ -212,7 +192,6 @@ scsi-close \ no further scsi words required > \ Scan the VSCSI bus: > virtiodev virtio-scsi-init > 0= IF > - setup-virt-queues > scsi-find-disks > setup-alias > TRUE to initialized? > diff --git a/lib/libvirtio/virtio-scsi.c b/lib/libvirtio/virtio-scsi.c > index 36b62d1..e95352d 100644 > --- a/lib/libvirtio/virtio-scsi.c > +++ b/lib/libvirtio/virtio-scsi.c > @@ -99,10 +99,9 @@ int virtioscsi_send(struct virtio_device *dev, > */ > int virtioscsi_init(struct virtio_device *dev) > { > - struct vring_avail *vq_avail; > - unsigned int idx = 0; > - int qsize = 0; > + struct vqs vq_ctrl, vq_event, vq_request; > int status = VIRTIO_STAT_ACKNOWLEDGE; > + uint16_t flags; > > /* Reset device */ > // XXX That will clear the virtq base. We need to move > @@ -126,17 +125,20 @@ int virtioscsi_init(struct virtio_device *dev) > virtio_set_guest_features(dev, 0); > } > > - while(1) { > - qsize = virtio_get_qsize(dev, idx); > - if (!qsize) > - break; > - virtio_vring_size(qsize); > + if (virtio_queue_init_vq(dev, &vq_ctrl, VIRTIO_SCSI_CONTROL_VQ) || > + virtio_queue_init_vq(dev, &vq_event, VIRTIO_SCSI_EVENT_VQ) || > + virtio_queue_init_vq(dev, &vq_request, VIRTIO_SCSI_REQUEST_VQ)) > + goto dev_error; > > - vq_avail = virtio_get_vring_avail(dev, idx); > - vq_avail->flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT); > - vq_avail->idx = 0; > - idx++; > - } > + flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT); > + vq_ctrl.avail->flags = flags; > + vq_ctrl.avail->idx = 0; > + > + vq_event.avail->flags = flags; > + vq_event.avail->idx = 0; > + > + vq_request.avail->flags = flags; > + vq_request.avail->idx = 0; > > /* Tell HV that setup succeeded */ > status |= VIRTIO_STAT_DRIVER_OK; >
On 03/02/17 03:02, Laurent Vivier wrote: > Ping? Please repost with "Signed-off" or I can add the line myself but I need you ack for this. Thanks. Also, one question - how did you recreate the situation when SLOF cannot access the address set by the kernel? > > Laurent > > On 24/01/2017 11:54, Laurent Vivier wrote: >> From: Laurent Vivier <lvivier@ibm-p8-virt-03.lab.eng.rdu.redhat.com> >> >> virtioscsi_init() uses the result of virtio_get_vring_avail() whereas >> the queue is not enabled: on the first boot, its value is NULL and >> the driver uses this to communicate with the device. After a reboot, >> its value is the one given by the OS driver, and it works if the >> address is in a range SLOF can access. >> >> In some cases, for instance with NUMA nodes and hotplugged memory, >> SLOF cannot access the address set by the kernel, and virtioscsi_init() >> fails with a data storage exception. >> >> To set the vring avail buffer address, we need to enable the queue, what >> is done by virtio_set_qaddr(). >> >> This patch fixes the problem by calling virtio_queue_init_vq() (like other >> virtio drivers) in virtioscsi_init() as it allocates memory and enables the >> queue. virtio_queue_init_vq() also replaces the calls to virtio_vring_size() >> and virtio_get_vring_avail(). >> --- >> v4: remove queue-control-addr, queue-event-addr, queue-cmd-addr >> >> v3: remove setup-virt-queues >> >> v2: move vqs declarations to the function >> only call virtio_cpu_to_modern16() once >> add CC: to Nikunj >> >> board-qemu/slof/virtio-scsi.fs | 21 --------------------- >> lib/libvirtio/virtio-scsi.c | 28 +++++++++++++++------------- >> 2 files changed, 15 insertions(+), 34 deletions(-) >> >> diff --git a/board-qemu/slof/virtio-scsi.fs b/board-qemu/slof/virtio-scsi.fs >> index 4fedeee..d52741e 100644 >> --- a/board-qemu/slof/virtio-scsi.fs >> +++ b/board-qemu/slof/virtio-scsi.fs >> @@ -165,26 +165,6 @@ scsi-open >> >> scsi-close \ no further scsi words required >> >> -0 VALUE queue-control-addr >> -0 VALUE queue-event-addr >> -0 VALUE queue-cmd-addr >> - >> -: setup-virt-queues >> - \ add 3 queues 0-controlq, 1-eventq, 2-cmdq >> - \ fixme: do we need to find more than the above 3 queues if exists >> - virtiodev 0 virtio-get-qsize virtio-vring-size >> - alloc-mem to queue-control-addr >> - virtiodev 0 queue-control-addr virtio-set-qaddr >> - >> - virtiodev 1 virtio-get-qsize virtio-vring-size >> - alloc-mem to queue-event-addr >> - virtiodev 1 queue-event-addr virtio-set-qaddr >> - >> - virtiodev 2 virtio-get-qsize virtio-vring-size >> - alloc-mem to queue-cmd-addr >> - virtiodev 2 queue-cmd-addr virtio-set-qaddr >> -; >> - >> \ Set scsi alias if none is set yet >> : setup-alias >> s" scsi" find-alias 0= IF >> @@ -212,7 +192,6 @@ scsi-close \ no further scsi words required >> \ Scan the VSCSI bus: >> virtiodev virtio-scsi-init >> 0= IF >> - setup-virt-queues >> scsi-find-disks >> setup-alias >> TRUE to initialized? >> diff --git a/lib/libvirtio/virtio-scsi.c b/lib/libvirtio/virtio-scsi.c >> index 36b62d1..e95352d 100644 >> --- a/lib/libvirtio/virtio-scsi.c >> +++ b/lib/libvirtio/virtio-scsi.c >> @@ -99,10 +99,9 @@ int virtioscsi_send(struct virtio_device *dev, >> */ >> int virtioscsi_init(struct virtio_device *dev) >> { >> - struct vring_avail *vq_avail; >> - unsigned int idx = 0; >> - int qsize = 0; >> + struct vqs vq_ctrl, vq_event, vq_request; >> int status = VIRTIO_STAT_ACKNOWLEDGE; >> + uint16_t flags; >> >> /* Reset device */ >> // XXX That will clear the virtq base. We need to move >> @@ -126,17 +125,20 @@ int virtioscsi_init(struct virtio_device *dev) >> virtio_set_guest_features(dev, 0); >> } >> >> - while(1) { >> - qsize = virtio_get_qsize(dev, idx); >> - if (!qsize) >> - break; >> - virtio_vring_size(qsize); >> + if (virtio_queue_init_vq(dev, &vq_ctrl, VIRTIO_SCSI_CONTROL_VQ) || >> + virtio_queue_init_vq(dev, &vq_event, VIRTIO_SCSI_EVENT_VQ) || >> + virtio_queue_init_vq(dev, &vq_request, VIRTIO_SCSI_REQUEST_VQ)) >> + goto dev_error; >> >> - vq_avail = virtio_get_vring_avail(dev, idx); >> - vq_avail->flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT); >> - vq_avail->idx = 0; >> - idx++; >> - } >> + flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT); >> + vq_ctrl.avail->flags = flags; >> + vq_ctrl.avail->idx = 0; >> + >> + vq_event.avail->flags = flags; >> + vq_event.avail->idx = 0; >> + >> + vq_request.avail->flags = flags; >> + vq_request.avail->idx = 0; >> >> /* Tell HV that setup succeeded */ >> status |= VIRTIO_STAT_DRIVER_OK; >> > > _______________________________________________ > SLOF mailing list > SLOF@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/slof >
On 03/02/2017 04:04, Alexey Kardashevskiy wrote: > On 03/02/17 03:02, Laurent Vivier wrote: >> Ping? > > > Please repost with "Signed-off" or I can add the line myself but I need you > ack for this. Thanks. Add the line, please. Signed-off-by: Laurent Vivier <lvivier@redhat.com> > > Also, one question - how did you recreate the situation when SLOF cannot > access the address set by the kernel? You can add traces in SLOF to see the pointer is NULL on first boot, or to have the crash, add some hotplugged memory starting SLOF: ... -m 1G,slots=4,maxmem=32G \ -numa node,nodeid=0 \ -numa node,nodeid=1 \ -smp 16,maxcpus=16,cores=8,threads=1,sockets=2 \ -object memory-backend-file,policy=default,mem-path=/var/tmp/backed-mem,size=1G,id=mem-mem2 \ -device pc-dimm,id=dimm-mem2,memdev=mem-mem2 more details: https://bugzilla.redhat.com/show_bug.cgi?id=1393273 Thanks, Laurent >> >> Laurent >> >> On 24/01/2017 11:54, Laurent Vivier wrote: >>> From: Laurent Vivier <lvivier@ibm-p8-virt-03.lab.eng.rdu.redhat.com> >>> >>> virtioscsi_init() uses the result of virtio_get_vring_avail() whereas >>> the queue is not enabled: on the first boot, its value is NULL and >>> the driver uses this to communicate with the device. After a reboot, >>> its value is the one given by the OS driver, and it works if the >>> address is in a range SLOF can access. >>> >>> In some cases, for instance with NUMA nodes and hotplugged memory, >>> SLOF cannot access the address set by the kernel, and virtioscsi_init() >>> fails with a data storage exception. >>> >>> To set the vring avail buffer address, we need to enable the queue, what >>> is done by virtio_set_qaddr(). >>> >>> This patch fixes the problem by calling virtio_queue_init_vq() (like other >>> virtio drivers) in virtioscsi_init() as it allocates memory and enables the >>> queue. virtio_queue_init_vq() also replaces the calls to virtio_vring_size() >>> and virtio_get_vring_avail(). >>> --- >>> v4: remove queue-control-addr, queue-event-addr, queue-cmd-addr >>> >>> v3: remove setup-virt-queues >>> >>> v2: move vqs declarations to the function >>> only call virtio_cpu_to_modern16() once >>> add CC: to Nikunj >>> >>> board-qemu/slof/virtio-scsi.fs | 21 --------------------- >>> lib/libvirtio/virtio-scsi.c | 28 +++++++++++++++------------- >>> 2 files changed, 15 insertions(+), 34 deletions(-) >>> >>> diff --git a/board-qemu/slof/virtio-scsi.fs b/board-qemu/slof/virtio-scsi.fs >>> index 4fedeee..d52741e 100644 >>> --- a/board-qemu/slof/virtio-scsi.fs >>> +++ b/board-qemu/slof/virtio-scsi.fs >>> @@ -165,26 +165,6 @@ scsi-open >>> >>> scsi-close \ no further scsi words required >>> >>> -0 VALUE queue-control-addr >>> -0 VALUE queue-event-addr >>> -0 VALUE queue-cmd-addr >>> - >>> -: setup-virt-queues >>> - \ add 3 queues 0-controlq, 1-eventq, 2-cmdq >>> - \ fixme: do we need to find more than the above 3 queues if exists >>> - virtiodev 0 virtio-get-qsize virtio-vring-size >>> - alloc-mem to queue-control-addr >>> - virtiodev 0 queue-control-addr virtio-set-qaddr >>> - >>> - virtiodev 1 virtio-get-qsize virtio-vring-size >>> - alloc-mem to queue-event-addr >>> - virtiodev 1 queue-event-addr virtio-set-qaddr >>> - >>> - virtiodev 2 virtio-get-qsize virtio-vring-size >>> - alloc-mem to queue-cmd-addr >>> - virtiodev 2 queue-cmd-addr virtio-set-qaddr >>> -; >>> - >>> \ Set scsi alias if none is set yet >>> : setup-alias >>> s" scsi" find-alias 0= IF >>> @@ -212,7 +192,6 @@ scsi-close \ no further scsi words required >>> \ Scan the VSCSI bus: >>> virtiodev virtio-scsi-init >>> 0= IF >>> - setup-virt-queues >>> scsi-find-disks >>> setup-alias >>> TRUE to initialized? >>> diff --git a/lib/libvirtio/virtio-scsi.c b/lib/libvirtio/virtio-scsi.c >>> index 36b62d1..e95352d 100644 >>> --- a/lib/libvirtio/virtio-scsi.c >>> +++ b/lib/libvirtio/virtio-scsi.c >>> @@ -99,10 +99,9 @@ int virtioscsi_send(struct virtio_device *dev, >>> */ >>> int virtioscsi_init(struct virtio_device *dev) >>> { >>> - struct vring_avail *vq_avail; >>> - unsigned int idx = 0; >>> - int qsize = 0; >>> + struct vqs vq_ctrl, vq_event, vq_request; >>> int status = VIRTIO_STAT_ACKNOWLEDGE; >>> + uint16_t flags; >>> >>> /* Reset device */ >>> // XXX That will clear the virtq base. We need to move >>> @@ -126,17 +125,20 @@ int virtioscsi_init(struct virtio_device *dev) >>> virtio_set_guest_features(dev, 0); >>> } >>> >>> - while(1) { >>> - qsize = virtio_get_qsize(dev, idx); >>> - if (!qsize) >>> - break; >>> - virtio_vring_size(qsize); >>> + if (virtio_queue_init_vq(dev, &vq_ctrl, VIRTIO_SCSI_CONTROL_VQ) || >>> + virtio_queue_init_vq(dev, &vq_event, VIRTIO_SCSI_EVENT_VQ) || >>> + virtio_queue_init_vq(dev, &vq_request, VIRTIO_SCSI_REQUEST_VQ)) >>> + goto dev_error; >>> >>> - vq_avail = virtio_get_vring_avail(dev, idx); >>> - vq_avail->flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT); >>> - vq_avail->idx = 0; >>> - idx++; >>> - } >>> + flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT); >>> + vq_ctrl.avail->flags = flags; >>> + vq_ctrl.avail->idx = 0; >>> + >>> + vq_event.avail->flags = flags; >>> + vq_event.avail->idx = 0; >>> + >>> + vq_request.avail->flags = flags; >>> + vq_request.avail->idx = 0; >>> >>> /* Tell HV that setup succeeded */ >>> status |= VIRTIO_STAT_DRIVER_OK; >>> >> >> _______________________________________________ >> SLOF mailing list >> SLOF@lists.ozlabs.org >> https://lists.ozlabs.org/listinfo/slof >> > >
On 03/02/17 18:58, Laurent Vivier wrote: > On 03/02/2017 04:04, Alexey Kardashevskiy wrote: >> On 03/02/17 03:02, Laurent Vivier wrote: >>> Ping? >> >> >> Please repost with "Signed-off" or I can add the line myself but I need you >> ack for this. Thanks. > > Add the line, please. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> Thanks, applied. I am just curious - why is this happening exactly and so stable? Is that because the guest kernel allocates rings beyond the very first 1G from your command line? > >> >> Also, one question - how did you recreate the situation when SLOF cannot >> access the address set by the kernel? > > You can add traces in SLOF to see the pointer is NULL on first boot, or > to have the crash, add some hotplugged memory starting SLOF: > ... > -m 1G,slots=4,maxmem=32G \ > -numa node,nodeid=0 \ > -numa node,nodeid=1 \ > -smp 16,maxcpus=16,cores=8,threads=1,sockets=2 \ > -object > memory-backend-file,policy=default,mem-path=/var/tmp/backed-mem,size=1G,id=mem-mem2 > \ > -device pc-dimm,id=dimm-mem2,memdev=mem-mem2 > > more details: > > https://bugzilla.redhat.com/show_bug.cgi?id=1393273 > > Thanks, > Laurent >>> >>> Laurent >>> >>> On 24/01/2017 11:54, Laurent Vivier wrote: >>>> From: Laurent Vivier <lvivier@ibm-p8-virt-03.lab.eng.rdu.redhat.com> >>>> >>>> virtioscsi_init() uses the result of virtio_get_vring_avail() whereas >>>> the queue is not enabled: on the first boot, its value is NULL and >>>> the driver uses this to communicate with the device. After a reboot, >>>> its value is the one given by the OS driver, and it works if the >>>> address is in a range SLOF can access. >>>> >>>> In some cases, for instance with NUMA nodes and hotplugged memory, >>>> SLOF cannot access the address set by the kernel, and virtioscsi_init() >>>> fails with a data storage exception. >>>> >>>> To set the vring avail buffer address, we need to enable the queue, what >>>> is done by virtio_set_qaddr(). >>>> >>>> This patch fixes the problem by calling virtio_queue_init_vq() (like other >>>> virtio drivers) in virtioscsi_init() as it allocates memory and enables the >>>> queue. virtio_queue_init_vq() also replaces the calls to virtio_vring_size() >>>> and virtio_get_vring_avail(). >>>> --- >>>> v4: remove queue-control-addr, queue-event-addr, queue-cmd-addr >>>> >>>> v3: remove setup-virt-queues >>>> >>>> v2: move vqs declarations to the function >>>> only call virtio_cpu_to_modern16() once >>>> add CC: to Nikunj >>>> >>>> board-qemu/slof/virtio-scsi.fs | 21 --------------------- >>>> lib/libvirtio/virtio-scsi.c | 28 +++++++++++++++------------- >>>> 2 files changed, 15 insertions(+), 34 deletions(-) >>>> >>>> diff --git a/board-qemu/slof/virtio-scsi.fs b/board-qemu/slof/virtio-scsi.fs >>>> index 4fedeee..d52741e 100644 >>>> --- a/board-qemu/slof/virtio-scsi.fs >>>> +++ b/board-qemu/slof/virtio-scsi.fs >>>> @@ -165,26 +165,6 @@ scsi-open >>>> >>>> scsi-close \ no further scsi words required >>>> >>>> -0 VALUE queue-control-addr >>>> -0 VALUE queue-event-addr >>>> -0 VALUE queue-cmd-addr >>>> - >>>> -: setup-virt-queues >>>> - \ add 3 queues 0-controlq, 1-eventq, 2-cmdq >>>> - \ fixme: do we need to find more than the above 3 queues if exists >>>> - virtiodev 0 virtio-get-qsize virtio-vring-size >>>> - alloc-mem to queue-control-addr >>>> - virtiodev 0 queue-control-addr virtio-set-qaddr >>>> - >>>> - virtiodev 1 virtio-get-qsize virtio-vring-size >>>> - alloc-mem to queue-event-addr >>>> - virtiodev 1 queue-event-addr virtio-set-qaddr >>>> - >>>> - virtiodev 2 virtio-get-qsize virtio-vring-size >>>> - alloc-mem to queue-cmd-addr >>>> - virtiodev 2 queue-cmd-addr virtio-set-qaddr >>>> -; >>>> - >>>> \ Set scsi alias if none is set yet >>>> : setup-alias >>>> s" scsi" find-alias 0= IF >>>> @@ -212,7 +192,6 @@ scsi-close \ no further scsi words required >>>> \ Scan the VSCSI bus: >>>> virtiodev virtio-scsi-init >>>> 0= IF >>>> - setup-virt-queues >>>> scsi-find-disks >>>> setup-alias >>>> TRUE to initialized? >>>> diff --git a/lib/libvirtio/virtio-scsi.c b/lib/libvirtio/virtio-scsi.c >>>> index 36b62d1..e95352d 100644 >>>> --- a/lib/libvirtio/virtio-scsi.c >>>> +++ b/lib/libvirtio/virtio-scsi.c >>>> @@ -99,10 +99,9 @@ int virtioscsi_send(struct virtio_device *dev, >>>> */ >>>> int virtioscsi_init(struct virtio_device *dev) >>>> { >>>> - struct vring_avail *vq_avail; >>>> - unsigned int idx = 0; >>>> - int qsize = 0; >>>> + struct vqs vq_ctrl, vq_event, vq_request; >>>> int status = VIRTIO_STAT_ACKNOWLEDGE; >>>> + uint16_t flags; >>>> >>>> /* Reset device */ >>>> // XXX That will clear the virtq base. We need to move >>>> @@ -126,17 +125,20 @@ int virtioscsi_init(struct virtio_device *dev) >>>> virtio_set_guest_features(dev, 0); >>>> } >>>> >>>> - while(1) { >>>> - qsize = virtio_get_qsize(dev, idx); >>>> - if (!qsize) >>>> - break; >>>> - virtio_vring_size(qsize); >>>> + if (virtio_queue_init_vq(dev, &vq_ctrl, VIRTIO_SCSI_CONTROL_VQ) || >>>> + virtio_queue_init_vq(dev, &vq_event, VIRTIO_SCSI_EVENT_VQ) || >>>> + virtio_queue_init_vq(dev, &vq_request, VIRTIO_SCSI_REQUEST_VQ)) >>>> + goto dev_error; >>>> >>>> - vq_avail = virtio_get_vring_avail(dev, idx); >>>> - vq_avail->flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT); >>>> - vq_avail->idx = 0; >>>> - idx++; >>>> - } >>>> + flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT); >>>> + vq_ctrl.avail->flags = flags; >>>> + vq_ctrl.avail->idx = 0; >>>> + >>>> + vq_event.avail->flags = flags; >>>> + vq_event.avail->idx = 0; >>>> + >>>> + vq_request.avail->flags = flags; >>>> + vq_request.avail->idx = 0; >>>> >>>> /* Tell HV that setup succeeded */ >>>> status |= VIRTIO_STAT_DRIVER_OK; >>>> >>> >>> _______________________________________________ >>> SLOF mailing list >>> SLOF@lists.ozlabs.org >>> https://lists.ozlabs.org/listinfo/slof >>> >> >> >
On 06/02/2017 05:39, Alexey Kardashevskiy wrote: > On 03/02/17 18:58, Laurent Vivier wrote: >> On 03/02/2017 04:04, Alexey Kardashevskiy wrote: >>> On 03/02/17 03:02, Laurent Vivier wrote: >>>> Ping? >>> >>> >>> Please repost with "Signed-off" or I can add the line myself but I need you >>> ack for this. Thanks. >> >> Add the line, please. >> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> > > > Thanks, applied. > > I am just curious - why is this happening exactly and so stable? Is that > because the guest kernel allocates rings beyond the very first 1G from your > command line? I think it was working because, in fact, the queues are initialized by the setup-virt-queues before being used, just after the init function. So, while the address given for the avail queue (zero or configured by the previous kernel boot) can be accessed, all is fine. Laurent
diff --git a/board-qemu/slof/virtio-scsi.fs b/board-qemu/slof/virtio-scsi.fs index 4fedeee..d52741e 100644 --- a/board-qemu/slof/virtio-scsi.fs +++ b/board-qemu/slof/virtio-scsi.fs @@ -165,26 +165,6 @@ scsi-open scsi-close \ no further scsi words required -0 VALUE queue-control-addr -0 VALUE queue-event-addr -0 VALUE queue-cmd-addr - -: setup-virt-queues - \ add 3 queues 0-controlq, 1-eventq, 2-cmdq - \ fixme: do we need to find more than the above 3 queues if exists - virtiodev 0 virtio-get-qsize virtio-vring-size - alloc-mem to queue-control-addr - virtiodev 0 queue-control-addr virtio-set-qaddr - - virtiodev 1 virtio-get-qsize virtio-vring-size - alloc-mem to queue-event-addr - virtiodev 1 queue-event-addr virtio-set-qaddr - - virtiodev 2 virtio-get-qsize virtio-vring-size - alloc-mem to queue-cmd-addr - virtiodev 2 queue-cmd-addr virtio-set-qaddr -; - \ Set scsi alias if none is set yet : setup-alias s" scsi" find-alias 0= IF @@ -212,7 +192,6 @@ scsi-close \ no further scsi words required \ Scan the VSCSI bus: virtiodev virtio-scsi-init 0= IF - setup-virt-queues scsi-find-disks setup-alias TRUE to initialized? diff --git a/lib/libvirtio/virtio-scsi.c b/lib/libvirtio/virtio-scsi.c index 36b62d1..e95352d 100644 --- a/lib/libvirtio/virtio-scsi.c +++ b/lib/libvirtio/virtio-scsi.c @@ -99,10 +99,9 @@ int virtioscsi_send(struct virtio_device *dev, */ int virtioscsi_init(struct virtio_device *dev) { - struct vring_avail *vq_avail; - unsigned int idx = 0; - int qsize = 0; + struct vqs vq_ctrl, vq_event, vq_request; int status = VIRTIO_STAT_ACKNOWLEDGE; + uint16_t flags; /* Reset device */ // XXX That will clear the virtq base. We need to move @@ -126,17 +125,20 @@ int virtioscsi_init(struct virtio_device *dev) virtio_set_guest_features(dev, 0); } - while(1) { - qsize = virtio_get_qsize(dev, idx); - if (!qsize) - break; - virtio_vring_size(qsize); + if (virtio_queue_init_vq(dev, &vq_ctrl, VIRTIO_SCSI_CONTROL_VQ) || + virtio_queue_init_vq(dev, &vq_event, VIRTIO_SCSI_EVENT_VQ) || + virtio_queue_init_vq(dev, &vq_request, VIRTIO_SCSI_REQUEST_VQ)) + goto dev_error; - vq_avail = virtio_get_vring_avail(dev, idx); - vq_avail->flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT); - vq_avail->idx = 0; - idx++; - } + flags = virtio_cpu_to_modern16(dev, VRING_AVAIL_F_NO_INTERRUPT); + vq_ctrl.avail->flags = flags; + vq_ctrl.avail->idx = 0; + + vq_event.avail->flags = flags; + vq_event.avail->idx = 0; + + vq_request.avail->flags = flags; + vq_request.avail->idx = 0; /* Tell HV that setup succeeded */ status |= VIRTIO_STAT_DRIVER_OK;
From: Laurent Vivier <lvivier@ibm-p8-virt-03.lab.eng.rdu.redhat.com> virtioscsi_init() uses the result of virtio_get_vring_avail() whereas the queue is not enabled: on the first boot, its value is NULL and the driver uses this to communicate with the device. After a reboot, its value is the one given by the OS driver, and it works if the address is in a range SLOF can access. In some cases, for instance with NUMA nodes and hotplugged memory, SLOF cannot access the address set by the kernel, and virtioscsi_init() fails with a data storage exception. To set the vring avail buffer address, we need to enable the queue, what is done by virtio_set_qaddr(). This patch fixes the problem by calling virtio_queue_init_vq() (like other virtio drivers) in virtioscsi_init() as it allocates memory and enables the queue. virtio_queue_init_vq() also replaces the calls to virtio_vring_size() and virtio_get_vring_avail(). --- v4: remove queue-control-addr, queue-event-addr, queue-cmd-addr v3: remove setup-virt-queues v2: move vqs declarations to the function only call virtio_cpu_to_modern16() once add CC: to Nikunj board-qemu/slof/virtio-scsi.fs | 21 --------------------- lib/libvirtio/virtio-scsi.c | 28 +++++++++++++++------------- 2 files changed, 15 insertions(+), 34 deletions(-)