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