Re: [Xen-devel] Re: [Qemu-devel] [PATCH 0/7] merge some xen bits into qemu

[Samuel Thibault <samuel.thibault@eu.citrix.com>]

Gerd Hoffmann, le Thu 07 Aug 2008 09:33:06 +0200, a écrit :
> Samuel Thibault wrote:
> > Samuel Thibault, le Wed 06 Aug 2008 15:25:26 +0100, a écrit :
> >> Pushing the cleaning changes to Xen first can be done and would entail
> >> much easier to tackle breakage, and the merge back from qemu would then
> >> be trivial, why not doing so?
> > 
> > You didn't answer that part.  Really, my only concern is about having
> > things tested.  Isn't it possible for instance to just merge the backend
> > core (and console/xenfb updates) in Ian's tree first for a start?
> 
> http://kraxel.fedorapeople.org/patches/qemu-xen/

Ok, thanks, that's already better.

I'd still say that "delete a file then add another one" is far from
convenient both from a point of view of proof-reading the patches and
repository history, but I guess we can manage that with a renaming step
first.

Any reason for the renames, though? (they tend to bother developpers who
have to change their habits, so we can not do that without a reason)
- Why dashes instead of underscores?
- In Xen, we call xen-machine.c xen_machine_pv.c because there is also
a xen_machine_fv.c.  Can't the xen_machine_pv.c name be fine for qemu
upstream too?  Or xen can keep its own xen_machine_{pv,fv}.c and your
xen-machine.c goes upstream, I don't think that would be a problem.
- I guess a xenfb -> xen[-_]{fb,framebuffer} rename is indeed more
coherent, Markus, do you prefer just "fb" or the longer "framebuffer"?


Misc stuff:
- I guess the sys-queue.h file should be merged and used in qemu first?
- xen_domid is re-declared in xen-backend.h
- The BUFSIZE macro in xen-backend.h is a bit unfortunate, but still
better than what we have currently.  Maybe it should be renamed with a
- XEN_ prefix to avoid any clash?
- container_of would probably be useful in other parts of qemu, I guess
it could be merged in qemu first?
- nice work on moving stuff from console and fb to the backend, console
is easier to read now :)

> I also largely left vl.c as-is, so xend shouldn't need any changes.  The
> -domid switch sets an additional (redundant) variable, to keep the
> amount of changes as small as possible for now.  Also -name and
> -domain-name are aliased, both set qemu_name and domain_name.

That's a good thing :)

> The framebuffer driver probably has some performance regressions.
> Fixing those depends on the display patches being pushed upstream.

It would have been easier to review if you had provided a delta patch,
not a delete/add patch </grin>.

That's actually the kind of things I was afraid of and that would have
been harder to spot when just pulling your work from qemu upstream.

In your xen patch, since idle and GUI_REFRESH_INTERVAL are there, you
can just use them.  We'll push them to qemu.  We'll manage the _shared
stuff (and push it eventually).

I'll let Markus comment on the rest (again, thanks for moving the xenbus
stuff to the backend part).  There is one change that is not backend
changes or just moving code around: you are queuing rectangle updates,
why?  (I'm not arguing, just wondering the kind of optimization that can
be).

> > Then you can push your code to qemu,
> > I guess that could be fine, as you said xen will not need to use e.g.
> > the block and net backends.
> 
> blk and net backends are not there (yet).  But they should be a nop for
> xen anyway

Indeed, I'd say don't bother to port those.

> The block backend should be a good replacement for blktap though and
> maybe can save you the effort of porting the blktap kernel driver to
> the pv_ops kernel.

Well, for better performance an in-kernel blktap would still be useful.

Samuel