[Qemu-devel] Re: optional feature

[Juan Quintela]

"Michael S. Tsirkin" <address@hidden> wrote:
> On Wed, Sep 09, 2009 at 10:47:27AM +0200, Juan Quintela wrote:
>> How do we deal with optional features?
>
> Here's an idea that Gleb suggested in a private
> conversation: make optional features into
> separate, non-user-visible devices.
>
> Thus we would have vmstate for virtio and additionally, if msix is
> enabled, vmstate for msix. This solves the problem of the number of
> devices becoming exponential with the number of features: we have device
> per feature.
>
> I understand that RTC does something like this.

And it is wrong :)  I sent a patch to fix it properly, but we have the
problem of backward compatibility with kvm.

Forget msix for virtio, virtio has the problem already with pci.

virtio_save()
{

    if (vdev->binding->save_config)
        vdev->binding->save_config(vdev->binding_opaque, f);

    qemu_put_8s(f, &vdev->status);

    .... some other normal fields ...

    for (i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
        if (vdev->vq[i].vring.num == 0)
            break;

Not a problem, we can precalculate i on pre_save()


        qemu_put_be32(f, vdev->vq[i].vring.num);
        qemu_put_be64(f, vdev->vq[i].pa);
        qemu_put_be16s(f, &vdev->vq[i].last_avail_idx);

This is sending a partial array of struct (the "i" 1st entries)
No problem here.

        if (vdev->binding->save_queue)
            vdev->binding->save_queue(vdev->binding_opaque, i, f);

Again, what to do with this one.

    }

}

Looking at what does virtio_pci_save_queue()

static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f)
{
    VirtIOPCIProxy *proxy = opaque;
    if (msix_present(&proxy->pci_dev))
        qemu_put_be16(f, virtio_queue_vector(proxy->vdev, n));
}

i.e. and now, an optional field.

And no, I don't have either a clean design that will be backward
compatible and clean.  Clean design is easy:

virtio
virtio-pci (it does the equivalent of save_config() and then call
            virtio_save)
virtio-pci-msix (it calls virtio-pci and then send a partial array of
queues. (the save queue thing)

Before you ask, partial arrays are sent: <num_elems> + array
where num_elems == 0 is valid.

But this is the "good" design if we started _now_, that is not the case,
and I am trying to get something clean and bacward compatible.

Later, Juan.

PD.  Optional fields are going to have to be in, arm cpus really need
     them if we want to maintain backward compatibility.