WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

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