Xendev xenbus fix #83

Merged
merged 12 commits into from Oct 18, 2016

Projects

None yet

2 participants

@ijackson-citrix
Contributor
ijackson-citrix commented Oct 7, 2016 edited

This fixes the header namespace problems in the xebus driver by splitting the driver up.
The privcmd and evtchn drivers have not been tacked yet, and their built remains disabled.

Thanks,
Ian.
[v2: fixed the build when XEN_HEADERS is not already set. I hadn't appreciated that build-rr.sh runs with set -u]
[v3: fixed the build, avoid using <inttypes.h> in busdev_user.c. Evidently on my test box it must be picking up a system <Inttypes.h>]

ijackson-citrix added some commits May 26, 2016
@ijackson-citrix @ijackson-citrix ijackson-citrix Search existing value of XEN_HEADERS for Xen headers
This allows a caller to specify a specific location for the Xen
headers.  For example, from a version of Xen just built in a
neighbouring directory.

(Without this it is not possible to build against a specific Xen
without being able to overwrite system header areas.)

If XEN_HEADERS is unset, or not suitable, use the existing search
strategy.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
3104392
@ijackson-citrix @ijackson-citrix ijackson-citrix librumpxen_xendev: xenbus: Reorganise to split minios from rumpkernel…
… parts

Split the xenbus driver into two pieces, busdev.c (in the netbsd
kernel namespacve) and busdev_user.c (in the minios namespace).  They
communicate via the (somewhat ad-hoc) interface in busdev_user.h.  The
interface uses `rumpxenbus_*' names so that the two sides can call
each other.  We split the state structure up into three: a netbsd
part, a minios part, and a common part.

This is actually largely a combination of code motion and function and
type renaming.  There is little functional change from the previous
"header abuse" approach, other than some minor interface adjustments.

In much of the code `d' was used to refer to the device struct.  Now
there are three context stucts.  For now I have retained in each
function the use of `d' which producese the lowest amount of code
churn.  The compiler makes sure the types all line up right.

Later, the uses of `d' will each be changed to `dc', `du' or `dd'.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
1c330ae
@ijackson-citrix @ijackson-citrix ijackson-citrix librumpxen_xendev: Disable privcmd and evtchn drivers for now
We have fixed the xenbus driver, but the privcmd and evtchn drivers
are still broken.  (Issue 73.)

Disable these two (and add the note about the issue number).  This
will allow us to enable the libbrumpxen_xendev build to get the xenbus
driver.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
1cfe2a3
@ijackson-citrix @ijackson-citrix ijackson-citrix librumpxen_xendev: Re-enable build for xenbus, and add it to the defa…
…ult bake

Fixes issue #73 for the xenbus driver.

privcmd and evtchn are not fixed yet - they are still disabled.

Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
0c83c34
@ijackson-citrix @ijackson-citrix ijackson-citrix librumpxen_xendev: Reorganise debug enablement slightly
Invite people to turn on debug by defining the make variable
RUMP_DEV_XEN_DEBUG rather than by editing .c files.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
40e2c5e
@ijackson-citrix @ijackson-citrix ijackson-citrix librumpxen_xendev: xenbus: Remove dependency on <inttypes.h> in "user…
…" code

busdev_user.c is compiled in a minios context which sadly lacks
<inttypes.h>.  Drop it, and change the one use of PRIx32 to a %lx
(fixing the type of the cast, while we're at it).

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
92dd08b
@ijackson-citrix @ijackson-citrix ijackson-citrix librumpxen_xendev: Add more debug
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
343526e
@ijackson-citrix @ijackson-citrix ijackson-citrix librumpxen_xendev: Rename applicable `d' variables to `dd'
This used to be a reference to the shared device context structure.
Now there are three structures.  Change all the variables referring to
what is now struct rumpxenbus_data_dev to `dd'.  This includes all the
references in busdev.c.

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
928e296
@ijackson-citrix @ijackson-citrix ijackson-citrix librumpxen_xendev: Rename applicable `d' variables to `dc'
This used to be a reference to the shared device context structure.
Now there are three structures.  Change all the variables referring to
what is now struct rumpxenbus_data_common to `dc'.  This includes
about half of the refernces in busdev_user.c.  No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
d25ab6f
@ijackson-citrix @ijackson-citrix ijackson-citrix librumpxen_xendev: Rename applicable `d' variables to `du'
This used to be a reference to the shared device context structure.
Now there are three structures.  Now there are three structures.
Change all the variables referring to what is now struct
rumpxenbus_data_user to `du'.  This completes the elimination of the
conventional variable name `d'.  No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
b3d9b30
@ijackson-citrix @ijackson-citrix ijackson-citrix librumpxen_xendev: Make WTROUBLE more anaphoric again
In "librumpxen_xendev: xenbus: Reorganise to split minios from
rumpkernel parts" WTROUBLE gained an argument for the common struct
pointer, because the different call sites did (in that patch) call
that struct pointer by different names.

Since "librumpxen_xendev: busdev.c: Rename applicable `d' variables to
`dc'" the common pointer is always `dc', so WTROUBLE can rely on that
again.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
2f23dc0
@ijackson-citrix @ijackson-citrix ijackson-citrix librumpxen_xendev: Do not test uio->uio_offset in xenbus_dev_write
This field is undefined on entry to f_write functions.

(The copying of the file offset to and from this field is done by
vn_read and vn_write, only for vnode entries, which this isn't.)

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
ef0bfb8
@ijackson-citrix ijackson-citrix merged commit 39a97f3 into rumpkernel:master Oct 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@anttikantee
Member

re v3: _user.c files must pick up system headers (otherwise they can't access the host).
(didn't review everything, that just happened to catch my eye while I was clicking myself up-to-date on github. thanks.)

@ijackson-citrix
Contributor

Antti Kantee writes ("Re: [rumpkernel/rumprun] Xendev xenbus fix (#83)"):

re v3: _user.c files must pick up system headers (otherwise they can't access
the host).
(didn't review everything, that just happened to catch my eye while I was
clicking myself up-to-date on github. thanks.)

Thanks for the review. I'm afraid I don't follow. The file
busdev_user.c works just fine as it is. It don't think it needs to
`access the host' in that sense.

Maybe I have some fundamental misunderstanding. So I will explain
my understanding. Sorry that this is long.

Background:

Necessarily, building a rump kernel system involves assembling an
image out of C files operating in two separate namespaces. Each
namespace is both a separate namespace at link-time for external
symbols (done by symbol rewriting) and a separate set of header files.

One of these is the netbsd kernel namespace; the other is for the
application (the "user" namespace).

Certain functions and facilities (eg rump*) are made available in both
namespaces, by providing self-contained sets of headers for their
declarations. This is one of the ways that calls can be made across
the namespaces.

When targetting a rump kernel at Xen, (a fork of) the xen.git's minios
is used. minios makes a lot of (IMO unfortunate) assumptions about
the header file and symbol namespaces it lives in. It wants to be
provided with something roughly like a standard C library. This is
most easily done by putting minios into the "user" namespace.

Calls by ordinary library functions in the application (or calls
originating in minios) eventually result in "system calls" which are
actually cross-namespace function calls, into the netbsd kernel
namespace. If the netbsd kernel ultimately needs to implement the
requested operation (eg, write to stdout) with functionality provided
by the platform (in this case Xen), that involves calling back out to
minios. Normally this is done by a "hypercall" which is actually
again a cross-namespace function call in the other direction. This
allows different platforms to provide the same facilities and makes
the rump kernels portable to different platforms and operating
environments.

Now on to the xenbus driver in librumpxen_xendev:

The purpose of this module is to provide a /dev/xen/xenbus which can
be used from a rump kernel running on Xen to talk to the system's
actual xenstored. This is necessary for some Xen-specific rump kernel
applications (such as device models).

The driver needs to do the multiplexing/demultiplexing that is usual
in a kernel xenbus driver, because the application might open the
device multiple times. It also needs to provide working select()
because the Xen Project userspace libraries which access
/dev/xen/xenbus expect it.

So essentially, this driver needs to implement the complete xenbus
driver interface as might be found in any kernel. But it needs to be
implemented in terms of the xenstore interface provided by minios.
This is because any one Xen domain has only one xenstore ring
connection. Other parts of the rump kernel Xen platform support use
minios xenstore support to implement (for example) networking.
minios's xenbus support expects to own the domain's xenstore ring. So
everything in the rump kernel must use minios to access xenbus.

The minios xenbus support functions do not resemble very much a raw
ring, or indeed the internal xenbus access functions one might find in
a "proper" kernel. So it is not sensible to try to reuse an ordinary
kernel xenbus driver. This is why xenbus*.c is bespoke.

It does not make sense to try to provide a xenbus node in /dev when
the platform is not Xen. So there is no need to make this
functionality available on non-Xen platforms. It should not be linked
into non-Xen rump kernel images.

Conceptually, therefore, the rump kernel xenbus driver is a single
piece of glue code.

On one side it uses netbsd kernel calls to register a kernel
filesystem entry, provide the appropriate fileops, negotiate with the
netbsd kernel to implement select() for the xenbus fd, and so on.

On the other side it uses minios calls (including callbacks) to send
and receive xenstore commands and replies, and watch events.

Previously this was all in one file. But that file had to #include
both minios and netbsd headers, violating the separation of
namespaces. This "header abuse" broke the build.

Now I have split the driver into two pieces. The separation is rather
arbitrary, and simply designed to reduce the total amount of
unnecessary code and unnecessary abstraction.

There is a single shared header "busdev_user.h" which is included in
both namespaces. It is entirely self-contained, except that it
requires <xen/io/xs_wire.h>. That header is part of the Xen public
headers and is itself sensibly namespaced and self-contained. So as a
whole, #include "busdev_user.h" can be written into both halves of the
xenbus driver.

The common struct contains much of the state of the driver, including
any partially-write(2)'d message from the application, and
declarations of all the functions in either half of the driver which
need calling from the other half.

busdev_user.c is not a piece of the application. It's in the user
namespace because it needs minios headers. It does not need anything
else other than some basic C library functions which are also
available in the bmk-* headers.

Am I wrong to #include <bmk-*> ? xenif_user.c and mini-os/wait.h do
it too.

Thanks for your attention.

Ian.

@anttikantee
Member

On 19/10/16 10:47, Ian Jackson wrote:

[snip]

yes

Calls by ordinary library functions in the application (or calls
originating in minios) eventually result in "system calls" which are
actually cross-namespace function calls, into the netbsd kernel
namespace. If the netbsd kernel ultimately needs to implement the
requested operation (eg, write to stdout) with functionality provided
by the platform (in this case Xen), that involves calling back out to
minios. Normally this is done by a "hypercall" which is actually
again a cross-namespace function call in the other direction. This
allows different platforms to provide the same facilities and makes
the rump kernels portable to different platforms and operating
environments.

Think of the underlying layer, minios(+bmk) in this case, as a firmware.
The firmware is separate and doesn't really know what's going on top,
except for passing initial control and perhaps delivering
interrupt/exception-type events. So, normally you wouldn't have calls
from minios resulting in system calls. (I'm not sure if you are even
misunderstanding anything here, given your otherwise rather correct
understanding, just thought I'd be extra-clear)

[snip]

more yes

busdev_user.c is not a piece of the application. It's in the user
namespace because it needs minios headers. It does not need anything
else other than some basic C library functions which are also
available in the bmk-* headers.

Am I wrong to #include <bmk-*> ? xenif_user.c and mini-os/wait.h do
it too.

You are entirely correct in doing so.

When I said "access the host", I meant access minios routines, perhaps
that was the source of the confusion. Also, I didn't mean it as a
"things are now broken", just as an explanation of why you had to remove
the use of <inttypes.h>.

@ijackson-citrix
Contributor

Antti Kantee writes ("Re: [rumpkernel/rumprun] Xendev xenbus fix (#83)"):

When I said "access the host", I meant access minios routines, perhaps
that was the source of the confusion. Also, I didn't mean it as a
"things are now broken", just as an explanation of why you had to remove
the use of <inttypes.h>.

Oh, I see :-). Right. Thanks for your attention.

Ian.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment