[Qemu-devel] Re: optional feature

[Gleb Natapov]

On Wed, Sep 16, 2009 at 01:04:19PM +0200, Juan Quintela wrote:
> "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.
What is wrong about it?

> 
> 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.

--
                        Gleb.