Re: [PATCH 2/6] virtio block driver for QEMU (v2)

[Anthony Liguori <aliguori-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

Dor Laor wrote:
>> +
>> +    if (1) {
>> +    BlockDriverState *bs = bdrv_new("vda");
>> +    if (bdrv_open(bs, "/home/anthony/images/linux.img", 
>> BDRV_O_SNAPSHOT))
>> +        exit(1);
>>   
> Can you add a printf to the exit(1). I had to gdb the code to find why 
> my qemu is not running no more (in earlier version
> I did remember to change the path but not after the new patches.

Heh, sorry about that.

> [snip]  
>> +    virtqueue_push(vq, &elem, wlen);
>> +    virtio_notify(vdev, vq);
>> +    }
>>   
> You can move the notify out of the while loop. This way you save irqs.

I don't think it does actually.  This raises the line multiple times, 
but if the line is already raised, it's effectively a nop.  Now, with 
KVM's in-kernel APIC, it ends up generating more syscalls than is 
necessary so it's worth changing.

>> +}
>> +
>> +static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t 
>> *config)
>> +{
>> +    VirtIOBlock *s = to_virtio_blk(vdev);
>> +    int64_t capacity;
>> +    uint32_t v;
>> +
>> +    bdrv_get_geometry(s->bs, &capacity);
>> +    memcpy(config + VIRTIO_CONFIG_BLK_F_CAPACITY, &capacity, 
>> sizeof(capacity));
>> +
>> +    v = VIRTQUEUE_MAX_SIZE - 2;
>> +    memcpy(config + VIRTIO_CONFIG_BLK_F_SEG_MAX, &v, sizeof(v));
>> +}
>> +
>> +static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
>> +{
>> +    return (1 << VIRTIO_BLK_F_SEG_MAX);
>>   
> In general I think we need to add another feature or even version 
> number ( I know you guys hate it).
> The reason is - Let's say you dont change functionality but change the 
> irq protocol (for example the isr won't be zeroed on read), then an old
> guest driver wouldn't know it runs on a new host version and will have 
> its irq line pulled up.
> So I suggest adding a capability of VIRTIO_ISR_CLEAR_XXX or adding a 
> version number.
> Comments?

I don't think we'll actually change the ISR protocol.  I think it's the 
best that it can actually be.  However, if we do need to change the ABI 
for some reason, I think the right thing to do is just use a new device 
ID (since it's effectively a new device).

Regards,

Anthony Liguori

>> +}
>> +
>> +VirtIODevice *virtio_blk_init(PCIBus *bus, uint16_t vendor, uint16_t 
>> device,
>> +                  BlockDriverState *bs)
>> +{
>> +    VirtIOBlock *s;
>> +
>> +    s = (VirtIOBlock *)virtio_init_pci(bus, "virtio-blk", vendor, 
>> device,
>> +                       vendor, VIRTIO_ID_BLOCK,
>> +                       16, sizeof(VirtIOBlock));
>> +
>> +    s->vdev.update_config = virtio_blk_update_config;
>> +    s->vdev.get_features = virtio_blk_get_features;
>> +    s->bs = bs;
>> +
>> +    virtio_add_queue(&s->vdev, virtio_blk_handle_output);
>> +
>> +    return &s->vdev;
>> +}
>> diff --git a/qemu/vl.h b/qemu/vl.h
>> index fafcf09..249ede2 100644
>> --- a/qemu/vl.h
>> +++ b/qemu/vl.h
>> @@ -1396,6 +1396,9 @@ void vmchannel_init(CharDriverState *hd, 
>> uint32_t deviceid, uint32_t index);
>>  
>>  typedef struct VirtIODevice VirtIODevice;
>>  
>> +VirtIODevice *virtio_blk_init(PCIBus *bus, uint16_t vendor, uint16_t 
>> device,
>> +                  BlockDriverState *bs);
>> +
>>  /* buf = NULL means polling */
>>  typedef int ADBDeviceRequest(ADBDevice *d, uint8_t *buf_out,
>>                                const uint8_t *buf, int len);
>>
>>   
>


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/