[Qemu-devel] Re: optional feature

[Juan Quintela]

Gleb Natapov <address@hidden> wrote:
> 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?

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

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.