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

[Gerd Hoffmann <kraxel@xxxxxxxxxx>]

  Hi,

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

Nevertheless I'm happy to finally see some progress here.
Is somebody (Samuel?) working on qemu-xen while you are away?

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

It isn't a mistake.  It is in the initialization path for
qemu-system-{i386,x86_64}, because for upstream qemu I plan to have this
as runtime switch, not as separate binary.  The code in question isn't
even compiled for qemu-dm, so the existing register call must stay.

I can drop the chunk for qemu-xen though if you find that less
confusing.  Then you'll get it with one of the next upstream merges instead.

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

Yes, that was the agreement we reached last time this was discussed.

The patch for upstream qemu implements exactly this behaviour:
http://kraxel.fedorapeople.org/patches/qemu-upstream/0002-xen-groundwork-for-xen-support.patch

The version for qemu-xen doesn't change the command line arguments to
avoid breaking qemu-dm <=> xend interaction.  I don't want to have xend
patches in this series, to avoid making the merge even more complicated
than it already is.

Adding the new ones while keeping the old ones for xend compatibility
should work though.  I can do that, with comments making clear what the
long-term plan is.

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

The very same Makefile infrastructure is intended for upstream though.
But I can leave Makefile.target alone and just use xen-hooks.mak if you
prefer that.  Either way is fine for me.

> Re 0003-xen-backend-driver-core.patch:
> 
>  * 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 ?

Will check.

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

Fine ;)

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

Yep, was done to adapt qemu style.

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

There is a "diff -b" version if the patch series at
http://kraxel.fedorapeople.org/patches/qemu-xen/no-ws/

Is that good enougth for review?

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

Patch 0002 + 0006 for qemu-xen is the same as Patch 0002 for
qemu-upstream.  It has been splitted in the qemu-xen series for better
bisectability:  0002 for qemu-xen does the minimal needed changes only,
and 0006 brings xen_machine_pv in sync with the upstream patch.

The code has been restructed a bit with the upcoming emulation bits in
mind.  There should be no functional change for qemu-dm.

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

Whatever you prefer.

>    However that needs to wait for the generic
>    backend, which definitely should go into qemu-xen-unstable before
>    it goes into qemu upstream.

Sure.

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

This is for the use cases where xend doesn't create these entries in
xenstore.  Yes, one user is Xenner.  There also is the patch to
(optionally) makes qemu create the domain, which needs this too.

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

The backends within qemu get their config information from xenstore.
Whenever the xenstore entries are created by qemu or by xend doesn't
matter, they can deal with both cases.

> And one final point:
> 
>  * Have you been testing your tree on a real Xen host under xend ?

I did a while back, when I did most of the coding work for these
patches.  I didn't retest the most recent version though.

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

There shouldn't be any incompatible changes (see the command line
argument discussion above).

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

Well, the disk backend within qemu isn't fundamentally different from
blktap.  Right now it doesn't do aio due to some limitations in qemu.
But the qemu block layer gets some love to improve the performance right
now, so once this is done and aio is enabled in the disk backend I'd
expect it to match blktap performance-wise.

The net backend proably is much less useful when you can use the dom0
kernel netback driver instead, although might be use cases the qemu
network support can handle and the kernel netback driver can't (-net
socket comes to mind).

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

Yes.

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

Yes.

>  * XEN_CREATE - Xen hypervisor but no xend

Yes.

>      However,
>      how do you control whether the block/net backends used are those
>      in the dom0 kernel or those in qemu ?

You can't right now, that it is still on my TODO list.  Making
XEN_CREATE work needs more patches too, so this isn't a problem yet ;)

My plan is to have an additional option for -drive, might look this way:

 -drive if=xen,backend=vbd,file=...    # blkback backend
 -drive if=xen,backend=tap,file=...    # blktap backend
 -drive if=xen,backend=qdisk,file=...  # qemu backend

Likewise for -net.

cheers,
  Gerd

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