[SLOF] [PATCH v2 07/19] virtio-{net, blk, scsi, 9p}: use status variable

[Nikunj A Dadhania]

Alexey Kardashevskiy <aik at ozlabs.ru> writes:

> On 01/22/2016 04:11 PM, Nikunj A Dadhania wrote:
>> Alexey Kardashevskiy <aik at ozlabs.ru> writes:
>>
>>> On 01/20/2016 11:10 PM, Nikunj A Dadhania wrote:
>>>> Signed-off-by: Nikunj A Dadhania <nikunj at linux.vnet.ibm.com>
>>>> ---
>>>>    lib/libvirtio/virtio-9p.c   | 22 +++++++++++++---------
>>>>    lib/libvirtio/virtio-blk.c  | 22 +++++++++++++---------
>>>>    lib/libvirtio/virtio-net.c  | 17 +++++++++++------
>>>>    lib/libvirtio/virtio-scsi.c | 10 ++++++----
>>>>    4 files changed, 43 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/lib/libvirtio/virtio-9p.c b/lib/libvirtio/virtio-9p.c
>>>> index 2d1d3f5..f1595c7 100644
>>>> --- a/lib/libvirtio/virtio-9p.c
>>>> +++ b/lib/libvirtio/virtio-9p.c
>>>> @@ -166,6 +166,7 @@ int virtio_9p_init(struct virtio_device *dev, void *tx_buf, void *rx_buf,
>>>>    		   int buf_size)
>>>>    {
>>>>    	struct vring_avail *vq_avail;
>>>> +	uint32_t status = VIRTIO_STAT_ACKNOWLEDGE;
>>>>
>>>>    	/* Check for double open */
>>>>    	if (__buf_size)
>>>> @@ -178,27 +179,25 @@ int virtio_9p_init(struct virtio_device *dev, void *tx_buf, void *rx_buf,
>>>>    	virtio_reset_device(dev);
>>>>
>>>>    	/* Acknowledge device. */
>>>> -	virtio_set_status(dev, VIRTIO_STAT_ACKNOWLEDGE);
>>>> +	virtio_set_status(dev, status);
>>>>
>>>>    	/* Tell HV that we know how to drive the device. */
>>>> -	virtio_set_status(dev, VIRTIO_STAT_ACKNOWLEDGE | VIRTIO_STAT_DRIVER);
>>>> +	status |= VIRTIO_STAT_DRIVER;
>>>> +	virtio_set_status(dev, status);
>>>>
>>>>    	/* Device specific setup - we do not support special features */
>>>>    	virtio_set_guest_features(dev,  0);
>>>>
>>>> -	if(!virtio_queue_init_vq(dev, &vq, 0)) {
>>>> -		virtio_set_status(dev, VIRTIO_STAT_ACKNOWLEDGE|VIRTIO_STAT_DRIVER
>>>> -				  |VIRTIO_STAT_FAILED);
>>>> -		return -1;
>>>> -	}
>>>> +	if(!virtio_queue_init_vq(dev, &vq, 0))
>>>> +		goto dev_error;
>>>>
>>>>    	vq_avail = virtio_get_vring_avail(dev, 0);
>>>>    	vq_avail->flags = VRING_AVAIL_F_NO_INTERRUPT;
>>>>    	vq_avail->idx = 0;
>>>>
>>>>    	/* Tell HV that setup succeeded */
>>>> -	virtio_set_status(dev, VIRTIO_STAT_ACKNOWLEDGE | VIRTIO_STAT_DRIVER
>>>> -			  |VIRTIO_STAT_DRIVER_OK);
>>>> +	status |= VIRTIO_STAT_DRIVER_OK;
>>>> +	virtio_set_status(dev, status);
>>>>
>>>>    	/* Setup 9P library. */
>>>>    	p9_reg_transport(virtio_9p_transact, dev,(uint8_t *)tx_buf,
>>>> @@ -206,6 +205,11 @@ int virtio_9p_init(struct virtio_device *dev, void *tx_buf, void *rx_buf,
>>>>
>>>>    	dprintf("%s : complete\n", __func__);
>>>>    	return 0;
>>>> +
>>>> +dev_error:
>>>> +	status |= VIRTIO_STAT_FAILED;
>>>> +	virtio_set_status(dev, status);
>>>> +	return -1;
>>>>    }
>>>>
>>>>    /**
>>>> diff --git a/lib/libvirtio/virtio-blk.c b/lib/libvirtio/virtio-blk.c
>>>> index 8ae61bc..98c1a80 100644
>>>> --- a/lib/libvirtio/virtio-blk.c
>>>> +++ b/lib/libvirtio/virtio-blk.c
>>>> @@ -31,32 +31,31 @@ virtioblk_init(struct virtio_device *dev)
>>>>    	struct vring_avail *vq_avail;
>>>>    	int blk_size = DEFAULT_SECTOR_SIZE;
>>>>    	int features;
>>>> +	uint32_t status = VIRTIO_STAT_ACKNOWLEDGE;
>>>>
>>>>    	/* Reset device */
>>>>    	virtio_reset_device(dev);
>>>>
>>>>    	/* Acknowledge device. */
>>>> -	virtio_set_status(dev, VIRTIO_STAT_ACKNOWLEDGE);
>>>> +	virtio_set_status(dev, status);
>>>>
>>>>    	/* Tell HV that we know how to drive the device. */
>>>> -	virtio_set_status(dev, VIRTIO_STAT_ACKNOWLEDGE|VIRTIO_STAT_DRIVER);
>>>> +	status |= VIRTIO_STAT_DRIVER;
>>>> +	virtio_set_status(dev, status);
>>>>
>>>>    	/* Device specific setup - we support F_BLK_SIZE */
>>>>    	virtio_set_guest_features(dev,  VIRTIO_BLK_F_BLK_SIZE);
>>>>
>>>> -	if(!virtio_queue_init_vq(dev, &vq, 0)) {
>>>> -		virtio_set_status(dev, VIRTIO_STAT_ACKNOWLEDGE|VIRTIO_STAT_DRIVER
>>>> -				  |VIRTIO_STAT_FAILED);
>>>> -		return 0;
>>>> -	}
>>>> +	if(!virtio_queue_init_vq(dev, &vq, 0))
>>>> +		goto dev_error;
>>>>
>>>>    	vq_avail = virtio_get_vring_avail(dev, 0);
>>>>    	vq_avail->flags = VRING_AVAIL_F_NO_INTERRUPT;
>>>>    	vq_avail->idx = 0;
>>>>
>>>>    	/* Tell HV that setup succeeded */
>>>> -	virtio_set_status(dev, VIRTIO_STAT_ACKNOWLEDGE|VIRTIO_STAT_DRIVER
>>>> -				|VIRTIO_STAT_DRIVER_OK);
>>>> +	status |= VIRTIO_STAT_DRIVER_OK;
>>>> +	virtio_set_status(dev, status);
>>>>
>>>>    	virtio_get_host_features(dev, &features);
>>>>    	if (features & VIRTIO_BLK_F_BLK_SIZE) {
>>>> @@ -66,6 +65,11 @@ virtioblk_init(struct virtio_device *dev)
>>>>    	}
>>>>
>>>>    	return blk_size;
>>>> +dev_error:
>>>> +	fprintf(stderr, "Device Error \n");
>>>
>>>
>>> This seems to be unrelated change but ok. I'd think that having
>>> VIRTIO_STAT_FAILED bit set will trigger some message in SLOF anyway...
>>
>> There wont be any error in SLOF by writing this, its will just inform
>> the HV about the inability of the guest to drive the device.
>
>
> I'd add a printf into virtio_set_status() when VIRTIO_STAT_FAILED is set...

How do you know which file/device is calling it?

On the side note, I would not want to do checking in my low-level
routines to log errors. This slows down virtio_set_status fast path,
isn't it ?
>>>> +
>>>> +dev_error:
>>>> +	driver->running = 0;
>>>
>>>
>>> This is new, it should not hurt but still - why?
>>
>> There was no case of error earlier, we have it now, so i need to mark
>> that the driver is not functional.
>
>
> My concern was - an error cannot happen between "running = 1" and
> "return 0" so the only case when "running != 0" at dev_error is it was
> "1" when virtionet_init_pci() was called; and if this happened - do
> not we want a loud error message here?

Looking more closely, driver is allocated in virtionet_open, and
"running" is set as 0 there, virtionet_init_pci does not have access to
driver struct, so this is a redundant change. Will remove in next spin.
I think i was extra cautious.

Regards
Nikunj