[SLOF] [PATCH v4] virtio-scsi: initialize vring avail queue buffers
Alexey Kardashevskiy
aik at ozlabs.ru
Mon Feb 6 15:39:25 AEDT 2017
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 at 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 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
>>>
>>
>>
>
--
Alexey
More information about the SLOF
mailing list