[SLOF] [PATCH v4] virtio-scsi: initialize vring avail queue buffers

[Alexey Kardashevskiy]

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 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