[Qemu-devel] Re: optional feature

[Michael S. Tsirkin]

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

The reason seems to be that you already decided what is clean
(below) and it is not backwards compatible :)

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

So writing num_elems == 0 in the image is wrong imo, we should have a
way to skip the array altogether.  This will let as add any number of
arrays down the road, and have them treated as zero size if an old image
is loaded.

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