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

[Gerd Hoffmann <kraxel@redhat.com>]

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

Fine with me, I can't send patches with renames though ...

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

Get consistent naming (all xen stuff in hw/ is prefixed with xen-).

> - Why dashes instead of underscores?

(1) It looks more pleasent to my eyes.
(2) It is easier to type (no shift needed) on both us and german
    keyboards.
(3) The files in the qemu source tree don't have a consistent style
    in respect to '-' vs. '_', so I had no reason to not use my
    personal preference ;)

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

Dunno whenever there ever will be a xenfv machine type upstream as most
likely the normal 'pc' machine type will get a fancy interface to switch
between emulation, kqemu and kvm.  And I think the best option for xen
hvm would be to hook in there too.  That is a long-term thing though,
xen_machine_fv.c will probably stay for quite a while at in the xen
tree, so maybe 'pv' in the name helps avoiding confusion.  I'd prefer to
stay with the xen-machine.c though.

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

We have two files defining xenpv_machine then.  I don't think it is a
good idea.

> Misc stuff:
> - I guess the sys-queue.h file should be merged and used in qemu first?

Rebase.  Then it will be there, and you can skip the patch ;)

> - xen_domid is re-declared in xen-backend.h

Oops.  Fixed.

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

xen-backend.h is supposed to be included by xen-related code only (it
barfs when /usr/include/xen isn't there), so it shouldn't clash in
theory.  Well, I can prefix it nevertheless, better safe than sorry.

> - container_of would probably be useful in other parts of qemu, I guess
> it could be merged in qemu first?

Has qemu a nice place for that kind of helper macros?

> 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'd prefer to do it the other way around (push depending changes
upstream, then adapt xen-framebuffer.c).  I want xen-framebuffer.c look
the same in xen and upstream.

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

Thats actually a bugfix.  Doing screen updates outside the ->update
callback isn't safe.

And as we must queuing up update rectangles anyway to fix that it also
tries to optimize stuff a bit and avoid unneeded bitblits.  Right now
that catches only frequent fullscreen updates (such as a scrolling
textconsole) and doesn't copy stuff multiple times in case the update
events from the guest are more frequent than update callback calls from
qemu.

Could be improved to analyze the rectangles in more detail (for example:
figure a update is a superset of another one, so one can be dropped).

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

Don't confuse blkback and blktap.  I don't want to replace the in-kernel
blkback driver.  blktap isn't a in-kernel driver though.  It is just an
interface between the hypervisor and a userspace application,
specialized for block devices.  My backend driver does pretty much the
same, but uses the generic gntdev interface instead of blktap.  I can't
see any reason why it shouldn't match blktap performance-wise.  I have
no benchmarks numbers though.

cheers,
  Gerd

-- 
http://kraxel.fedorapeople.org/xenner/