[SLOF] [PATCH v2 07/19] virtio-{net, blk, scsi, 9p}: use status variable
Alexey Kardashevskiy
aik at ozlabs.ru
Fri Jan 22 16:23:00 AEDT 2016
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...
> That was the reason for this error. As I mentioned in my mail to Thomas,
> i will make this change consistent and have it in virtio-9p has well.
>
>>
>>
>>> + status |= VIRTIO_STAT_FAILED;
>>> + virtio_set_status(dev, status);
>>> + return 0;
>>> }
>>>
>>>
>>> diff --git a/lib/libvirtio/virtio-net.c b/lib/libvirtio/virtio-net.c
>>> index 5c7d771..cafaa4b 100644
>>> --- a/lib/libvirtio/virtio-net.c
>>> +++ b/lib/libvirtio/virtio-net.c
>>> @@ -97,6 +97,7 @@ static int virtionet_init_pci(struct virtio_device *dev)
>>> static int virtionet_init(net_driver_t *driver)
>>> {
>>> int i;
>>> + uint32_t status = VIRTIO_STAT_ACKNOWLEDGE | VIRTIO_STAT_DRIVER;
>>>
>>> dprintf("virtionet_init(%02x:%02x:%02x:%02x:%02x:%02x)\n",
>>> driver->mac_addr[0], driver->mac_addr[1],
>>> @@ -107,7 +108,7 @@ static int virtionet_init(net_driver_t *driver)
>>> return 0;
>>>
>>> /* Tell HV that we know how to drive the device. */
>>> - virtio_set_status(&virtiodev, VIRTIO_STAT_ACKNOWLEDGE|VIRTIO_STAT_DRIVER);
>>> + virtio_set_status(&virtiodev, status);
>>>
>>> /* Device specific setup - we do not support special features right now */
>>> virtio_set_guest_features(&virtiodev, 0);
>>> @@ -117,8 +118,7 @@ static int virtionet_init(net_driver_t *driver)
>>> * RX_QUEUE_SIZE);
>>> if (!vq[VQ_RX].buf_mem) {
>>> printf("virtionet: Failed to allocate buffers!\n");
>>> - virtio_set_status(&virtiodev, VIRTIO_STAT_FAILED);
>>> - return -1;
>>> + goto dev_error;
>>> }
>>>
>>> /* Prepare receive buffer queue */
>>> @@ -151,9 +151,8 @@ static int virtionet_init(net_driver_t *driver)
>>> vq[VQ_TX].avail->idx = 0;
>>>
>>> /* Tell HV that setup succeeded */
>>> - virtio_set_status(&virtiodev, VIRTIO_STAT_ACKNOWLEDGE
>>> - |VIRTIO_STAT_DRIVER
>>> - |VIRTIO_STAT_DRIVER_OK);
>>> + status |= VIRTIO_STAT_DRIVER_OK;
>>> + virtio_set_status(&virtiodev, status);
>>>
>>> /* Tell HV that RX queues are ready */
>>> virtio_queue_notify(&virtiodev, VQ_RX);
>>> @@ -161,6 +160,12 @@ static int virtionet_init(net_driver_t *driver)
>>> driver->running = 1;
>>>
>>> return 0;
>>> +
>>> +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?
--
Alexey
More information about the SLOF
mailing list