[Qemu-devel] Re: optional feature

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Sep 16, 2009 at 01:48:35PM +0200, Juan Quintela wrote:
> Gleb Natapov <gleb@redhat.com> wrote:
> > On Wed, Sep 16, 2009 at 01:04:19PM +0200, Juan Quintela wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> 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?
> 
> See below, we are changing the state to one table, and tables don't have
> neither if's or whiles (we have a limited for that just walks arrays).

Let's just bite the bullet and add support for if's?  It's not like it's
hard to invent 'struct vmstate_condition' or some such.

> I don't really know how to handle virtio in a sane way.  The _saner_ way
> that I thought was to split it into three devices as I sketched below,
> but I still don't understood virtio creation to see how to "fix" it.
> It is next on queue after cpu (BIGGG), and ide (was waiting for Gerd
> patches to be commited.  After that, I will thought again on how to
> handle virtio.
> 
> 
> >> 
> >> 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.