[SLOF] [PATCH v2 07/19] virtio-{net, blk, scsi, 9p}: use status variable
Alexey Kardashevskiy
aik at ozlabs.ru
Fri Jan 22 17:37:05 AEDT 2016
On 01/22/2016 04:41 PM, Nikunj A Dadhania wrote:
> 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?
Why does this matter? I just want to know if device stopped working,
straight away, helps with debugging, no?
> 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 ?
You do not have to print an error every time you call virtio_set_status,
just when it becomes set.
>>>>> +
>>>>> +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.
Good, Thanks,
Alexey :)
--
Alexey
More information about the SLOF
mailing list