[SLOF] [PATCH v4] virtio-scsi: initialize vring avail queue buffers
Laurent Vivier
lvivier at redhat.com
Fri Feb 3 18:58:30 AEDT 2017
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 at 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 at 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 at lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/slof
>>
>
>
More information about the SLOF
mailing list