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

Anthony Liguori
2007-11-11T19:25:37Z

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