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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Gerd Hoffmann writes ("Re: [Qemu-devel] Re: [Xen-devel] [PATCH 0/7] merge some 
xen bits into qemu"):
> Ian Jackson wrote:
> > I think what I would really like is to be able to straight away apply
> > to qemu-xen-unstable the subset of Gerd's patches which are
> >   - structural improvements to the qemu-dm pv fb backend
> >     including separating out the 
> >   - code improvements to the above
> > but excluding all of the other non-actual-Xen stuff.
> 
> That are patches 1-4 (upstream) and patches 1-5 (qemu-xen).

I've taken a look at your `qemu-xen' series.  I'm still keen to have
the new backend structure.

I'm afraid I wasn't able in the short time I have today to tease out
the parts which I think are important to go into qemu-xen-unstable now
from the parts which I think should (following discussion) go directly
into qemu upstream.

But as there is stuff here that I definitely want and which ought to
go into qemu-xen-unstable for Xen 3.4, I was wondering if I could ask
you to rework it into a form that I can apply to qemu-xen-unstable
when I get back ?

So on to the details:


Firstly, I should say that this review is purely from the point of
view of `should this stuff go into qemu-xen-unstable'.  One reason not
to put things into qemu-xen-unstable is because they should go
directly upstream; another is of course that they still need work :-).
I'll try to make this distinction clear in my comments.

Re 0002-xen-groundwork-for-xen-support.patch [1]:

 * You introduce a new call to register xenpv_machine.  But our
   existing machinery in qemu-xen-unstable for i386-dm already does
   this.  So I think this is a mistake ?  (I'm not opposed to a change
   to the place where qemu-dm does its machine registration if that
   would suit upstream better but you should surely remove the old
   registration call too?)

 * You introduce the QEMU_OPTION_domid the default behaviour of which
   is XEN_ATTACH.  I don't think this is correct in the long term
   because I think that by default qemu should never attach (or
   create).  The upstream qemu should always emulate Xen things by
   default.  In a real Xen environment the toolstack (xend) will know
   to pass qemu-dm an option saying `please attach'.

   Otherwise people who try to run vanilla qemu in what happens to be
   a Xen guest will find all sorts of weird behaviours.

   I know that we have a -domid option in qemu-xen-unstable but that
   is one of the things that we have deliberately avoided passing
   upstream.  If upstream qemu is going to grow a -domid option it
   needs to default to emulating Xen and we should have a new option
   for enabling attach.

 * I'm not sure that the new Makefile infrastructure you provide here
   is a good idea for the qemu-xen-unstable tree.  The stuff in
   qemu-xen-unstable's build system is very unpleasant but it is
   specifically designed to avoid merge conflicts when qemu upstream
   Makefiles change, as they often do.

   I would very much prefer to continue to deal with the Makefiles on
   this twin-track approach.  So I would prefer it if you would add
   your new object files to xen-hooks.mak and xen-setup and so forth.

   Then when the corresponding changes go upstream I'll just remove
   them again.  This will be much less pain for me than dealing with
   <<< >>> in Makefiles.

   I'm sorry that this means you need to do the build system parts of
   your changes twice.  But I think doing it exactly twice is better
   than doing a similar amount of work every time there are
   textually-nearby changes in upstream's Makefiles.

Re 0003-xen-backend-driver-core.patch:

 * You add xen_backend.o to your new Makefile infrastructure which
   isn't in qemu-xen-unstable yet - see above.  But apart from that I
   think this is probably OK.

 * I did get some compiler warnings eg:
      /u/iwj/work/qemu-iwj.git/hw/xenfb.c:810: warning: passing
      argument 4 of 'xenfb_xs_printf' discards qualifiers from pointer
      target type
   Maybe you don't have -Wwrite-strings in your setup ?

Re 0005-xen-add-framebuffer-backend-driver.patch:

 * I haven't reviewed this in detail but I am keen to have your new
   framebuffer code.  So provided this compiles and appears to work in
   a quick test I would like to apply it ASAP.

   I only haven't done so right away because of the comments above.

Re 0004-xen-add-console-backend-driver.patch:

 * There are many whitespace changes.  Surely some mistake ?
   Or is this with a view to getting the coding style to resemble
   that in most of the rest of qemu ?

   If so then I would prefer (as an exception!) a first separate patch
   to re-indent the existing code in hw/xen_console.c: that is, a patch
   consisting _only_ of whitespace changes.  And then a separate patch
   containing no whitespace changes.

   As it is I haven't been able to review this very deeply and I don't
   have time today to redo the diff myself with -b ...

Re 0006-xen-sync-xen_machine_pv-with-upstream.patch:

 * What is the purpose of this patch ?  `sync xen_machine_pv with
   upstream' ?  Which upstream ?  Many of these changes seem
   cosmetic.

Re 0007-xen-add-block-device-backend-driver.patch
and 0008-xen-add-net-backend-driver.patch:

 * I don't have any objection to these in principle.  However they are
   of no use or relevance to qemu-xen-unstable because we do not and
   will not use them.  So they should go into upstream directly rather
   than via me I think ?  However that needs to wait for the generic
   backend, which definitely should go into qemu-xen-unstable before
   it goes into qemu upstream.  So I think these should wait for now.

Re 0009-xen-blk-nic-configuration-via-cmd-line.patch:

 * I don't think I fully understand the purpose of this patch.  It
   appears to be part of the Xenner emulation AFAICT ?  When running
   under the real Xen toolstack we do not have qemu write these kind
   of entries to xenstore.

   Instead, xenstore.c (in qemu-xen-unstable) reads the necessary
   information from xenstore and sets up the qemu internals
   appropriately.

And one final point:

 * Have you been testing your tree on a real Xen host under xend ?


> I want keep both patch series in sync exactly to avoid merge issues.
> Thats why I'm posting the patches on both lists and take into account
> review comments from both sides.  So when we agree on the final cut of
> the code it can be merged strait into both trees.

That sounds sensible.


> > But I'm prepared to maintain another branch of qemu-xen-unstable if
> > that helps.  I'll discuss this wih Keir tomorrow.
> 
> That will surely help as it will decouple qemu development from xen
> release cycles.  I'd suggest to branch off qemu-xen-3.4 when
> xen-unstable freezes for the 3.4 release.

I was thinking of doing that anyway.


> The naming convention I'm using right now is prefix 'xen_' for anything
> usable when running on the Xen hypervisor.  That includes code which
> will be shared by xen and xenner such as the machine type.  That also
> includes the block and net backends.  They *do* work on Xen, and with
> some glue in xend you should be able to use them.

Before we accept any incompatible changes into qemu-xen-unstable we
obviously need the corresponding changes to xend too ?

But we are not going to be using the block and net backends in qemu.
In a real Xen system these are provided by the dom0 kernel, which is
a far superior arrangement (in our opinion!)

AIUI from your patch the xen_mode specifies whether we're doing Xenner
or real Xen ?  Am I right in thinking that the modes are these:

 * XEN_EMULATE - Xenner (no Xen hypervisor, no xend)
     IMO this should be the default for the qemu xen machine types
     in upstream qemu.
 * XEN_ATTACH - for qemu-dm as invoked by xend
     IMO arrangements should be made so that this is done only with
     a special command-line option, which is provided by xend.
 * XEN_CREATE - Xen hypervisor but no xend
     This is an unusual use case.  I don't see any harm in having this
     as an option for people who want to do it that way.  However,
     how do you control whether the block/net backends used are those
     in the dom0 kernel or those in qemu ?


I'm afraid that since I'm going away now for three weeks I won't be
able to commit any revised versions soon.  But as I say I hope to get
this into 3.4 and I look forward to seeing your new proposals when I
get back.

I hope you find this mail of mine suitably constructive.  Again, sorry
about being overly defensive earlier.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel