Had a (very) quick look. Seems very separate and thus I don't think I have any problem accepting it upstream assuming the others don't voice any strong objection.

One thing that would be nice (tho' not sure how much this would cause "bleeding" into other bits of the code) is if module-udev-detect could somehow detect Xen and automatically load the appropriate sink module rather than alsa ones?

Is this feasible (this is certainly not a blocker to merging, just asking as I'm not a Xen expert)?

Cheers

I wouldn't mind having a separate detection module for Xen. Unless Xen's virtual audio device can actually be detected through udev, I don't think module-udev-detect should care about Xen. I don't know much about Xen either, but I'd guess there wouldn't be any conflict with running module-udev-detect and module-xen-detect side-by-side.

(In reply to comment #2)
> I wouldn't mind having a separate detection module for Xen. Unless Xen's
> virtual audio device can actually be detected through udev, I don't think
> module-udev-detect should care about Xen. I don't know much about Xen either,
> but I'd guess there wouldn't be any conflict with running module-udev-detect
> and module-xen-detect side-by-side.

Well if both detect modules work in parallel that would be nicer, however I had assumed that udev-detect would still find something under xen. If I'm wrong however (and I also know next to nothing about Xen!) then, yes, a separate xen-detect would be preferred.

I'm not exactly a Xen expert either, but I asked one:)

Although without Xen-specific code you wouldn't be able to detect the audio backend itself, it is possible to detect through udev that you are running in a domU guest (i.e., you are not running in the dom0 'host'). In domU guests you can't access any hardware devices directly anyway, so your best chance is to just try the Xen module instead.

I think there shouldn't be a problem with including this in udev-detect. On our part, we would be very excited to have PulseAudio working without any configuration in the guest. I'll try to update the code as soon as possible.

(In reply to comment #4)
> I'm not exactly a Xen expert either, but I asked one:)
> 
> Although without Xen-specific code you wouldn't be able to detect the audio
> backend itself, it is possible to detect through udev that you are running in a
> domU guest (i.e., you are not running in the dom0 'host'). In domU guests you
> can't access any hardware devices directly anyway, so your best chance is to
> just try the Xen module instead.
> 
> I think there shouldn't be a problem with including this in udev-detect.

Tanu, what do you think the best plan is here?

1) Have udev-detect notice that it is running as a guest and just unload itself (cleanly). and have a totally separate module module-xenpv-detect configured in our default.pa file which is similarly designed to exit gracefully if it does not detect a Xen guest?

2) module-udev-detect itself actually loads module-xenpv-detect and unloads itself when it detects Xen guest mode.


While the latter is less "pure" from a configuration aspect, it's nicer to not try and load xen stuff when not needed if the support is compiled in (and doing .nofail blocks in the default.pa is a bit crappy to work around "optional modules" as it's arguably not an optional module when you are running under Xen).


Or we could refactor some more how we structure our default.pa file. Some time ago I added support for including directories containing *.pa files. We should maybe just define a "/etc/pulse/default.pa.d/detect.d/" folder and put udev.pa and xenpv.pa files in there... That way it's 100% a packaging issue at the distro level. That might be nicer than hard coding and unnecessary loading of modules that will not be needed.

I personally prefer this approach, so I would opt for code in the form of suggest 1 above, but any further comments welcome :)

(In reply to comment #5)
> Tanu, what do you think the best plan is here?
> 
> 1) Have udev-detect notice that it is running as a guest and just unload itself
> (cleanly). and have a totally separate module module-xenpv-detect configured in
> our default.pa file which is similarly designed to exit gracefully if it does
> not detect a Xen guest?
> 
> 2) module-udev-detect itself actually loads module-xenpv-detect and unloads
> itself when it detects Xen guest mode.
> 
> 
> While the latter is less "pure" from a configuration aspect, it's nicer to not
> try and load xen stuff when not needed if the support is compiled in (and doing
> .nofail blocks in the default.pa is a bit crappy to work around "optional
> modules" as it's arguably not an optional module when you are running under
> Xen).

I'd argue that the Xen modules still are optional even in Xen guest environment. You can use Pulseaudio also through network, for example. Why is .nofail needed anyway? Isn't .ifexists good enough?

So, my preference would be to have both module-udev-detect and module-xenpv-detect in the default configuration, guarded by .ifexists, and if the modules detect that they are useless they unload themselves (udev is useless if it detects that it's running in a Xen guest, and xenpv is useless in the opposite case). In the usual case module-xenpv-detect won't be installed, so Pulseaudio won't try to load it.

Good patch, it seems like you addressed all my earlier comments.

Some points:


Why is the patch adding src/module-xenpv-sink.c? This file should be in src/modules/.


What's up with this weird hunk?

diff --git a/src/modules/module-xenpv-sink.c b/src/modules/module-xenpv-sink.c
new file mode 120000
index 0000000..6e87ea3
--- /dev/null
+++ b/src/modules/module-xenpv-sink.c
@@ -0,0 +1 @@
+/xen-audio/frontend/module-xenpv-sink.c
\ No newline at end of file


And finally, I don't get the point of the xen_gnt.h file. Is it something specific to this sink, or is it a general Xen header? In the last case it should not be added to the pulseaudio source. At any rate, a comment at the top of that file summarising its purpose, origin and license would be most welcome.

If the header really needs to be in our tree, I'd prefer you make a separate src/modules/xen directory for both xen_gnt.h and module-xenpv-sink.c, to keep src/modules clean, just like e.g. is done for alsa and rtp headers/source files.

Created attachment 57686 [details] [review]
Xen PV driver patch

I made a few cosmetic changes to the patch. xen_gnt.h was a header that included gntalloc.h and gntdev.h, which are the gnt devices interface headers from the linux kernel. I have copied both into the modules/xen directory so that the actual kernel headers don't have to be a dependency and the interface macros defined shouldn't change in the future. Of course, we can always remove them and just check for the presence of the actual kernel headers.

As far as detection goes: from your comments and from what I've figured so far, I think the best solution is to first check if we are in a domU guest in module-udev-detect. This can be done by checking the presence and contents of /proc/xen/capabilities; if it doesn't contain 'control_d' then we are in a guest domain.

Now, if we are in a Xen guest then we know that xenstore will be present. A simple module (module-xenguest-detect) linked to xenstore can be loaded, look for an audio backend there and if found, it will finally try to load module-xenpv-sink. The other option is to keep the detection code in the sink module and prevent it from loading, but I think it's safer to use a different module just to make sure that we'll be leaving Xen guests without PV audio support alone.

Hello.

Thanks for this patch.

I've now included this in git master scheduled for inclusion in PA 2.0 due out shortly. While we're technically in feature freeze I'm happy to include this as it has little impact on any other system without specific manual configuration. If it included changes to udev-detect I'd probably not have included it (although my own delays in reviewing it and getting it in would have to be considered too!).

Anyway, I had to make a couple tweaks:

1. Mention the two headers in the Makefile.am to ensure they are distributed (i.e. in the tarball)
2. Change the path to the headers: You put them in the xen folder, but this only worked for in-tree builds, not out-of-tree builds (which is needed for "make distcheck"). I simply added the include path in the CFLAGS and dropped the xen/ prefix in the .c file.
3. Moved the .c file to the xen folder. Maarten originally stated it should go in src/modules which was correct at the time, but as you added a couple companion files, it makes sense to put everything in the folder.
4. I removed trailing whitespace from the .c files.

As the .h headers are copied from the kernel I didn't touch them for whitespace changes (not even sure if they need them). We do have some external files included from other projects already - e.g. see the sbc files in the bluetooth folder. We have some automatic stuff to update them directly (via gitweb to save a real clone) from their primary location. It would be great if you could supply a rule similar to "make update-sbc" that would pull in any changes to these files from their upstream location.


As for your comments regarding the detection module, I'd be relatively happy for udev-detect to be modified to effectively become a no-op when on a guest, but I think that should be all it does.

The full, independent xen-detect module could be mostly stand-alone.

I think in terms of activating it automatically, we should likely change the default.pa to have a ".include /etc/pulse/default.d/detect" line, then put a small file in that folder: /etc/pulse/default.d/detect/xen.pa that actually does the "load-module module-xen-detect" stuff. This way a non-xen machine (i.e. one who likely does not install the distribution pulse-module-xen package will not waste cycles loading that module.

That's likely not super clearly written, so just bug me for an explanation on IRC if needed :)

I'll close this bug, but feel free to reopen or open a new one for the detection module.

Many thanks for your contribution :)