Re: [Qemu-devel] [RFC PATCH 0/4] Fix subsection ambiguity in the migration format

[Anthony Liguori]

On 07/31/2011 04:25 PM, Dor Laor wrote:
> On 08/01/2011 12:03 AM, Anthony Liguori wrote:
> > On 07/31/2011 03:57 PM, Dor Laor wrote:
> > > On 07/31/2011 11:43 PM, Anthony Liguori wrote:
> > > > > ps: how hard is to finish the vmstate conversion? Can't we just assume
> > > > > not converted code is not functional and just remove all of it?
> > > >
> > > > No. VMState is a solution looking for a problem. Many important device
> > >
> > > The initial target solved some rare bugs, that tend not to bite us with
> > > virtio. On the way, it got enhanced with subsections that was a major
> > > improvement.
> >
> > I should have qualified my statement. VMState did solve many real
> > problems. I meant that at this point in time, we've gotten pretty much
> > what we can get out it.
> >
> > > > models are still not converted and ultimately, it doesn't solve the
> > > > problem we're really trying to solve.
> > >
> > > From the start I supported Michael Tisrkin's idea for ASN.1 protocol.
> > > The question is how visitors and ability to translate from one
> > > representation to another will help us.
> >
> > Because with Visitors you can do:
> >
> > Devices -> internal QObject representation -> ASN.1 -> wire -> ASN.1 ->
> > internal QObject representation -> Device.
>
> I admit that QObject sounds more appealing than VMState, we can convert
> all into it. I'm not sure what's the difference between visitor and the
> load/save functions, potentially with enhanced parameters like name
> which can be part of QObject anyway.

VMStateInfo contains

struct VMStateInfo {
    const char *name;
    int (*get)(QEMUFile *f, void *pv, size_t size);
    void (*put)(QEMUFile *f, void *pv, size_t size);
};

It needs to change to:

struct VMStateInfo {
    const char *name;
    void (*visit)(Visitor *v, const char *name, void *pv, size_t size,
                  Error **errp);
};

For each VMStateInfo, like vmstate_info_bool, we go from:


static int get_bool(QEMUFile *f, void *pv, size_t size)
{
    bool *v = pv;
    *v = qemu_get_byte(f);
    return 0;
}

static void put_bool(QEMUFile *f, void *pv, size_t size)
{
    bool *v = pv;
    qemu_put_byte(f, *v);
}

To:

static void visit_bool(Visitor *v, const char *name, void *pv,
                       size_t size, Error **errp)
{
    bool *v = pv;
    visit_type_bool(v, name, v, errp);
}

For non-converted devices, like virtio, we change:
int virtio_load(VirtIODevice *vdev, QEMUFile *f)
{
    int num, i, ret;
    uint32_t features;
    uint32_t supported_features =
        vdev->binding->get_features(vdev->binding_opaque);

    if (vdev->binding->load_config) {
        ret = vdev->binding->load_config(vdev->binding_opaque, f);
        if (ret)
            return ret;
    }

    qemu_get_8s(f, &vdev->status);
    qemu_get_8s(f, &vdev->isr);
    ...

To:

void visit_type_virtio(Visitor *v, VirtIODevice *vdev,
                       const char *name, Error **errp)
{
    int num, i, ret;
    uint32_t features;
    uint32_t supported_features =
        vdev->binding->get_features(vdev->binding_opaque);

    if (vdev->binding->load_config) {
        ret = vdev->binding->load_config(vdev->binding_opaque, f);
        if (ret)
            return ret;
    }

    visit_start_struct(v, "VirtIODevice", name, errp);
    visit_type_u8(v, "status", &vdev->status);
    visit_type_u8(v, "isr", &vdev->isr);
    ...

You'll notice it's almost entirely mechanical.  It can probably be done 
with a few seds and an afternoons worth of grunt work.

I'm resisting the urge to do this myself because it's a good intro task 
and we've got a number of folks looking for those.

> > While it's in an internal representation, we can make large changes like
> > translating entire device state structures to new formats, splitting one
> > device into two, etc.
> >
> > It's sort of the ultimate mechanism to make compatibility changes. If
> > you just go Devices -> ASN.1, you miss out on that.
>
> What's important in ASN.1 is not the data representation itself but the
> ability to have a flexible protocol. We can have it with VMState and
> QObject as well. I do admit that QObject+ASN.1 will ease the way to make
> it right so you convinced me :).
>
> I still don't see have using ASN.1 will easily join/split several
> devices into few and some other magics. Not that it is not possible but
> it is way too hard.

ASN.1 doesn't do it but having an object representation that we can 
manipulate will.  Think of it like a compiler optimization phase, you 
write a visitor that can identify a node, and transform it into a 
different set of nodes.

> The main 'real' problems you're trying to solve are migration from one
> release to the other while most of our problems were forgotten fields
> here and there (floppy/ide/rtl/kvmclock/etc). I doubt that live
> migration of the same release worked on upstream for the random git
> head. Verifying save(i)== load(i)+save(i+1) is simple but no one
> executing it.

Because it's not easily automated.  I know it's preaching to the choir, 
but we need better unit tests.  We're getting there though, we know have 
a handful of tests in the tree with hopefully more growing now that 
we're embracing glib.

> Looks like we might be ready to go with your suggestion,
> I'm just worried that there are too many other non migration open
> issues. If the above work won't get complete we're better off with the
> current machine type + VMState + subsections. If it will be all
> completed, we're better with your suggestion.

I think the trick is having a long term vision, but also to divide it 
into shorter term, valuable incremental changes.  Even without the full 
object model stuff, just introducing visitors will mean that we're 
pretty instantly get QMP introspection for all device model objects.

Since the change is pretty small and straight forward, I think QMP 
device model introspection justifies it on it's own.  Being able to do 
essentially a live migration dump (minus memory) in a structured format 
as part of sosreport would have made my life a lot easier numerous times :-)

Regards,

Anthony Liguori

> > BTW, another really useful thing that Visitor would enable is the
> > ability to read an individual device to a QObject and implement the
> > equivalent of 'show devicename' which dumps the state of arbitrary
> > devices via QMP. This could be very useful for debugging.
> >
> > > I do see value in it but I don't
> > > think it is that important. If we have one real device serialization
> > > method that is flexible enough we can stick with it w/o translation. If
> > > we define qdev serialization into vmstate/asn.1/json/other and add some
> > > capability negotiation and various other goodies it should be enough.
> > >
> > > btw: separating the live migration protocol from the machine state is
> > > even more important if we take a gradual approach.
> >
> > Yeah, I think the critical technical requirement to achieve this is that
> > the devices need to generate their own serialization format, and then
> > another layer translates that to the "live migration protocol" format.
> >
> > Regards,
> >
> > Anthony Liguori
> >
> > > > Regards,
> > > >
> > > > Anthony Liguori
> > > >
> > > > > > Regards,
> > > > > >
> > > > > > Anthony Liguori