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