[Xen-devel] [PATCH] kernel-parameters: document earlycon=xenboot
Add earlycon=xenboot option to Documentation/kernel-parameters.txt. Signed-off-by: Chris Patterson --- Documentation/kernel-parameters.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt index ecc74fa..e01ec39 100644 --- a/Documentation/kernel-parameters.txt +++ b/Documentation/kernel-parameters.txt @@ -1078,6 +1078,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted. address. The serial port must already be setup and configured. Options are not yet supported. + xenboot Use Xen hypervisor console for early console. + earlyprintk=[X86,SH,BLACKFIN,ARM,M68k] earlyprintk=vga earlyprintk=efi -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] kernel-parameters: document earlycon=xenboot
On Tue, Apr 5, 2016 at 9:49 AM, Konrad Rzeszutek Wilk wrote: > On Mon, Apr 04, 2016 at 10:48:11PM -0400, Chris Patterson wrote: >> Add earlycon=xenboot option to Documentation/kernel-parameters.txt. >> > > But it is not true. > > I tried this on x86: "earlycon=xenboot console=tty0" > I can investigate further, but hopefully Stefano can chime in more definitively on what is supported. I used "earlycon=xenboot console=hvc0" with success for dom0 on my ARM64 platform. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] kernel-parameters: document earlycon=xenboot
On Tue, Apr 5, 2016 at 12:42 PM, Konrad Rzeszutek Wilk wrote: > On Tue, Apr 05, 2016 at 11:43:36AM -0400, Chris Patterson wrote: >> On Tue, Apr 5, 2016 at 9:49 AM, Konrad Rzeszutek Wilk >> wrote: >> > On Mon, Apr 04, 2016 at 10:48:11PM -0400, Chris Patterson wrote: >> >> Add earlycon=xenboot option to Documentation/kernel-parameters.txt. >> >> >> > >> > But it is not true. >> > >> > I tried this on x86: "earlycon=xenboot console=tty0" >> > >> >> I can investigate further, but hopefully Stefano can chime in more >> definitively on what is supported. >> >> I used "earlycon=xenboot console=hvc0" with success for dom0 on my >> ARM64 platform. > > Perhaps the documentation should also have [ARM], like some of the > other ones? Absolutely. I will test some combinations and be explicit about what should work in v2. Thanks for the feedback. Cheers, -Chris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] Question about running Xen on NVIDIA Jetson-TK1
On Tue, May 17, 2016 at 9:37 AM, Konrad Rzeszutek Wilk wrote: >> - The serial controller on the Tegra SoCs doesn't behave in the same >> was as most NS16550-compatibles; it actually adheres to the NS16550 >> spec a little more rigidly than most compatible controllers. A >> coworker (Chris Patterson, cc'd) figured out what was going on; from >> what I understand, most 16550s generate the "transmit ready" interrupt >> once, when the device first can accept new FIFO entries. Both the >> original 16550 and the Tegra implementation generate the "transmit >> ready" interrupt /continuously/ when there's space available in the >> FIFO, slewing the CPU with a stream of constant interrupts. > > That may also be an issue on x86 I would think. > On the x86 serial I have tested, it appears that the THRE interrupt goes low after the IIR read reset, and doesn't re-assert until UART_IER is flipped (or data is transmitted). On the Tegra, the interrupt persists (or immediately re-asserts) after the IIR read reset. I suppose this could be a problem on x86 if this behaviour exists in another uart. > Is there some simple 16550 'fix' for this? As in reprogram it > to not be soo ready? Yes, I do have a patch in my queue to address this. It simply implements the start_tx and stop_tx hooks and sets or masks UART_IER_ETHREI accordingly. I believe this behaviour is consistent with the Linux kernel. I have done some limited testing on it with both x86 and the Tegra (with additional patches for Tegra support) and things appear to work well. I can RFC it when our employer gives us approval. Apologies for the delay. Have a good day! -Chris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] stack size limit issues with xen + qemu + rbd
I have spent some time investigating a case where qemu is failing to register xenstore watches for a PV guest once I enable vfb (and thereby triggering the creation of a qemu instance). The qemu logs show something along the lines of: xen be core: xen be core: xen be: watching backend path (backend/console/3) failed xen be: watching backend path (backend/console/3) failed xen be core: xen be core: xen be: watching backend path (backend/vkbd/3) failed xen be: watching backend path (backend/vkbd/3) failed xen be core: xen be core: xen be: watching backend path (backend/qdisk/3) failed xen be: watching backend path (backend/qdisk/3) failed xen be core: xen be core: xen be: watching backend path (backend/qusb/3) failed xen be: watching backend path (backend/qusb/3) failed xen be core: xen be core: xen be: watching backend path (backend/vfb/3) failed xen be: watching backend path (backend/vfb/3) failed xen be core: xen be core: xen be: watching backend path (backend/qnic/3) failed xen be: watching backend path (backend/qnic/3) failed I have tested qemu master, qemu-xen in the master xen tree, as well as a few tags all with the same issue. I came across a similar issue reported by Juergen Gross: https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg03341.html Sure enough, the thread stack size was the culprit. I had been testing with qemu with the associated fix "vnc-tight: fix regression with libxenstore" as it is in master, so that wasn't it... I did some basic analysis of the qemu binary and the libraries it is pulling in: for lib in $(ldd /usr/local/bin/qemu-system-i386 | grep -o '/.* '); do echo "lib=$lib"; readelf -S "$lib" | grep -e tbss -e tdata -A1 ; done The largest consumers were: lib=/usr/lib/x86_64-linux-gnu/librbd.so.1 [17] .tbss NOBITS 0088fed0 0068fed0 1820 WAT 0 0 8 lib=/usr/lib/x86_64-linux-gnu/librados.so.2 [17] .tbss NOBITS 00718600 00518600 0aa0 WAT 0 0 8 IIUC, librbd & librados are using nearly 9k of the 16k alone (I am assuming this thread-local storage must be consumed as part of the thread's stack)? I narrowed that down to Ceph's usage of __thread in stringify() in src/include/stringify.h. To make things functional, the options were either to: (a) disable rbd at configure time for qemu (b) reduce the level of thread-local storage in dependencies (particularly ceph's stringify) (c) increase the stack size specified in xenstore's xs.c Is there is any precedent/policy with regards to expected TLS and/or stack usage for dependencies? Is the best course of action (b)? Or perhaps reconsider the default size for (c)? Thoughts? :) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [RFC PATCH] xs: use system's default stack size for xs_watch's reader thread
From: Chris Patterson
xs_watch() creates a thread to listen to xenstore events. Currently, the
thread is created with the greater of 16K or PTHREAD_MIN_SIZE.
There have been several bug reports and workarounds related to the issue
where xs_watch() fails because its attempt to create the reader thread with
pthread_create() fails. This is due to insufficient stack space size
given the requirements for thread-local storage usage in the applications
and libraries that are linked against libxenstore. [1,2,3,4].
Specifying the stack size appears to have been added to reduce memory
footprint (1d00c73b983b09fbee4d9dc0f58f6663c361c345).
This has already been bumped up once to the greater of 16k and
PTHREAD_STACK_MIN (da6a0e86d6a079102abdd0858a19f1e1fae584fc).
This patch reverts to sticking with the system's default stack size and
removes the code used to set the thread's stack size.
Of course, the alternative is to bump it to another arbitrary value, but the
requirements may change depending on the application and its libraries that
are linking against xenstore.
[1] https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg03341.html
[2] https://lists.xenproject.org/archives/html/xen-users/2016-07/msg00012.html
[3] https://lists.xenproject.org/archives/html/xen-users/2016-07/msg00085.html
[4] https://bugzilla.redhat.com/show_bug.cgi?id=1350264
Signed-off-by: Chris Patterson
---
tools/xenstore/xs.c | 10 --
1 file changed, 10 deletions(-)
diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index d1e01ba..c62b120 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -723,11 +723,6 @@ bool xs_watch(struct xs_handle *h, const char *path, const
char *token)
struct iovec iov[2];
#ifdef USE_PTHREAD
-#define DEFAULT_THREAD_STACKSIZE (16 * 1024)
-#define READ_THREAD_STACKSIZE \
- ((DEFAULT_THREAD_STACKSIZE < PTHREAD_STACK_MIN) ? \
- PTHREAD_STACK_MIN : DEFAULT_THREAD_STACKSIZE)
-
/* We dynamically create a reader thread on demand. */
mutex_lock(&h->request_mutex);
if (!h->read_thr_exists) {
@@ -738,11 +733,6 @@ bool xs_watch(struct xs_handle *h, const char *path, const
char *token)
mutex_unlock(&h->request_mutex);
return false;
}
- if (pthread_attr_setstacksize(&attr, READ_THREAD_STACKSIZE) !=
0) {
- pthread_attr_destroy(&attr);
- mutex_unlock(&h->request_mutex);
- return false;
- }
sigfillset(&set);
pthread_sigmask(SIG_SETMASK, &set, &old_set);
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH] xs: use system's default stack size for xs_watch's reader thread
On Wed, Sep 21, 2016 at 9:00 AM, Wei Liu wrote: > On Wed, Sep 21, 2016 at 08:51:07AM -0400, Konrad Rzeszutek Wilk wrote: >> On Tue, Sep 20, 2016 at 05:29:39PM -0400, Chris Patterson wrote: >> > From: Chris Patterson >> > >> > xs_watch() creates a thread to listen to xenstore events. Currently, the >> > thread is created with the greater of 16K or PTHREAD_MIN_SIZE. >> > >> > There have been several bug reports and workarounds related to the issue >> > where xs_watch() fails because its attempt to create the reader thread with >> > pthread_create() fails. This is due to insufficient stack space size >> > given the requirements for thread-local storage usage in the applications >> > and libraries that are linked against libxenstore. [1,2,3,4]. >> > >> > Specifying the stack size appears to have been added to reduce memory >> > footprint (1d00c73b983b09fbee4d9dc0f58f6663c361c345). >> >> Ugh. 8MB. > > OOI isn't that 8MB virtual memory, which means it shouldn't have real > impact unless it is used? > From what I understand, that is correct. At least in the Linux/glibc case, I believe the stack is allocated using anonymous mmap() and that resident memory usage shouldn't be greater than what you actually end up writing. However, I do not know if this holds true universally... ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [RFC PATCH] xs: use system's default stack size for xs_watch's reader thread
On Wed, Sep 21, 2016 at 10:07 AM, Konrad Rzeszutek Wilk wrote: > On Wed, Sep 21, 2016 at 09:50:30AM -0400, Chris Patterson wrote: >> On Wed, Sep 21, 2016 at 9:00 AM, Wei Liu wrote: >> > On Wed, Sep 21, 2016 at 08:51:07AM -0400, Konrad Rzeszutek Wilk wrote: >> >> On Tue, Sep 20, 2016 at 05:29:39PM -0400, Chris Patterson wrote: >> >> > From: Chris Patterson >> >> > >> >> > xs_watch() creates a thread to listen to xenstore events. Currently, >> >> > the >> >> > thread is created with the greater of 16K or PTHREAD_MIN_SIZE. >> >> > >> >> > There have been several bug reports and workarounds related to the issue >> >> > where xs_watch() fails because its attempt to create the reader thread >> >> > with >> >> > pthread_create() fails. This is due to insufficient stack space size >> >> > given the requirements for thread-local storage usage in the >> >> > applications >> >> > and libraries that are linked against libxenstore. [1,2,3,4]. >> >> > >> >> > Specifying the stack size appears to have been added to reduce memory >> >> > footprint (1d00c73b983b09fbee4d9dc0f58f6663c361c345). >> >> >> >> Ugh. 8MB. >> > >> > OOI isn't that 8MB virtual memory, which means it shouldn't have real >> > impact unless it is used? > > /me nods. That is my recollection too. But it does mean that 'top' > shows the application as bigger (by 8MB). > >> > >> >> >From what I understand, that is correct. At least in the Linux/glibc >> case, I believe the stack is allocated using anonymous mmap() and that >> resident memory usage shouldn't be greater than what you actually end >> up writing. However, I do not know if this holds true universally... > > That should be faily easy to find out. One just needs to check > the RSS size. Not sure how to do that on FreeBSD thought.. I did validate that yesterday (using pmap -x ) and it appeared to hold true (for that case anyways). If using the default stack size (as this patch reverts to), the stack size can be controlled with ulimit -s ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 1/2] libfsimage: replace deprecated readdir_r() with readdir()
From: Chris Patterson
Replace the usage of readdir_r() with readdir() to address
a compilation error due to the deprecation of readdir_r.
glibc has deprecated this for their next release (2.24):
https://sourceware.org/bugzilla/show_bug.cgi?id=19056
Signed-off-by: Chris Patterson
---
tools/libfsimage/common/fsimage_plugin.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/libfsimage/common/fsimage_plugin.c
b/tools/libfsimage/common/fsimage_plugin.c
index 3fa06c7..03212e1 100644
--- a/tools/libfsimage/common/fsimage_plugin.c
+++ b/tools/libfsimage/common/fsimage_plugin.c
@@ -147,7 +147,7 @@ static int load_plugins(void)
bzero(dp, sizeof (struct dirent) + name_max + 1);
- while (readdir_r(dir, dp, &dpp) == 0 && dpp != NULL) {
+ while ((dpp = readdir(dir)) != NULL) {
if (strcmp(dpp->d_name, ".") == 0)
continue;
if (strcmp(dpp->d_name, "..") == 0)
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 2/2] libxl: replace deprecated readdir_r() with readdir()
From: Chris Patterson
Replace the usage of readdir_r() with readdir() to address
a compilation error due to the deprecation of readdir_r.
glibc has deprecated this for their next release (2.24):
https://sourceware.org/bugzilla/show_bug.cgi?id=19056
This also removes the zalloc_dirent() helper function
which is no longer required.
Signed-off-by: Chris Patterson
---
tools/libxl/libxl_pvusb.c | 26 --
tools/libxl/libxl_utils.c | 9 ++---
2 files changed, 6 insertions(+), 29 deletions(-)
diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
index 9f1e842..d1397c4 100644
--- a/tools/libxl/libxl_pvusb.c
+++ b/tools/libxl/libxl_pvusb.c
@@ -508,19 +508,10 @@ int libxl_devid_to_device_usbctrl(libxl_ctx *ctx,
return rc;
}
-static void *zalloc_dirent(libxl__gc *gc, const char *dirpath)
-{
-size_t need = offsetof(struct dirent, d_name) +
- pathconf(dirpath, _PC_NAME_MAX) + 1;
-
-return libxl__zalloc(gc, need);
-}
-
static char *usbdev_busaddr_to_busid(libxl__gc *gc, int bus, int addr)
{
DIR *dir;
char *busid = NULL;
-struct dirent *de_buf;
struct dirent *de;
/* invalid hostbus or hostaddr */
@@ -533,21 +524,17 @@ static char *usbdev_busaddr_to_busid(libxl__gc *gc, int
bus, int addr)
return NULL;
}
-de_buf = zalloc_dirent(gc, SYSFS_USB_DEV);
-
for (;;) {
char *filename;
void *buf;
int busnum = -1;
int devnum = -1;
-int r = readdir_r(dir, de_buf, &de);
-if (r) {
+de = readdir(dir);
+if (!de) {
LOGE(ERROR, "failed to readdir %s", SYSFS_USB_DEV);
break;
}
-if (!de)
-break;
if (!strcmp(de->d_name, ".") ||
!strcmp(de->d_name, ".."))
@@ -1157,7 +1144,6 @@ static int usbdev_get_all_interfaces(libxl__gc *gc, const
char *busid,
{
DIR *dir;
char *buf;
-struct dirent *de_buf;
struct dirent *de;
int rc;
@@ -1172,18 +1158,14 @@ static int usbdev_get_all_interfaces(libxl__gc *gc,
const char *busid,
return ERROR_FAIL;
}
-de_buf = zalloc_dirent(gc, SYSFS_USB_DEV);
-
for (;;) {
-int r = readdir_r(dir, de_buf, &de);
+de = readdir(dir);
-if (r) {
+if (!de) {
LOGE(ERROR, "failed to readdir %s", SYSFS_USB_DEV);
rc = ERROR_FAIL;
goto out;
}
-if (!de)
-break;
if (!strcmp(de->d_name, ".") ||
!strcmp(de->d_name, ".."))
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index ceb8825..613a9d6 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -548,20 +548,15 @@ int libxl__remove_directory(libxl__gc *gc, const char
*dirpath)
goto out;
}
-size_t need = offsetof(struct dirent, d_name) +
-pathconf(dirpath, _PC_NAME_MAX) + 1;
-struct dirent *de_buf = libxl__zalloc(gc, need);
struct dirent *de;
for (;;) {
-int r = readdir_r(d, de_buf, &de);
-if (r) {
+de = readdir(d);
+if (!de) {
LOGE(ERROR, "failed to readdir %s for removal", dirpath);
rc = ERROR_FAIL;
break;
}
-if (!de)
-break;
if (!strcmp(de->d_name, ".") ||
!strcmp(de->d_name, ".."))
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH] xen/Makefile: quote HOSTCC and HOSTCXX args
From: Chris Patterson In some cross-compilation environments, the CC/CXX variables may expand out to more than one argument (to include things like --sysroot=...). Quote these to safely pass along. Signed-off-by: Chris Patterson --- xen/Makefile | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/xen/Makefile b/xen/Makefile index 0d5f240..b59f95d 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -246,14 +246,14 @@ kconfig := silentoldconfig oldconfig config menuconfig defconfig \ randconfig .PHONY: $(kconfig) $(kconfig): - $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC=$(HOSTCC) HOSTCXX=$(HOSTCXX) $@ + $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" $@ include/config/%.conf: include/config/auto.conf.cmd $(KCONFIG_CONFIG) - $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC=$(HOSTCC) HOSTCXX=$(HOSTCXX) silentoldconfig + $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" silentoldconfig # Allow people to just run `make` as before and not force them to configure $(KCONFIG_CONFIG): - $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC=$(HOSTCC) HOSTCXX=$(HOSTCXX) defconfig + $(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" defconfig # Break the dependency chain for the first run include/config/auto.conf.cmd: ; -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] libfsimage: replace deprecated readdir_r() with readdir()
On Tue, May 31, 2016 at 6:42 AM, George Dunlap wrote: > On Mon, May 30, 2016 at 3:32 AM, Chris Patterson wrote: >> From: Chris Patterson >> >> Replace the usage of readdir_r() with readdir() to address >> a compilation error due to the deprecation of readdir_r. >> >> glibc has deprecated this for their next release (2.24): >> https://sourceware.org/bugzilla/show_bug.cgi?id=19056 >> >> Signed-off-by: Chris Patterson > > Thanks for the patch, Chris. A bit more background would have been > helpful -- I did some searching and found a description[1] which says: > Thank you. I should have added these details in the commit message or cover. > In the current POSIX.1 specification (POSIX.1-2008), readdir(3) is > not required to be thread-safe. However, in modern > implementations (including the glibc implementation), concurrent > calls to readdir(3) that specify different directory streams are > thread-safe. Therefore, the use of readdir_r() is generally > unnecessary in multithreaded programs. In cases where multiple > threads must read from the same directory stream, using readdir(3) > with external synchronization is still preferable to the use of > readdir_r(), for the reasons given in the points above. > > The use of the specific directory stream is single-threaded, so for > glibc, it looks like using readdir() will be safe. But libxl needs to > be able to build on a number of libc's that are not glibc and still be > thread-safe. So we need to either 1) verify that readdir() is thread > safe on all libc's against which we may compile, or 2) add some Is there a list of supported libc libraries? I can look into it and provide more definitive answers if there are. > #ifdef-ery to switch to readdir_r() on systems unless we know it's > thread-safe. > > I'm less familiar with best practices for this kind of thing -- Ian, > do you have an idea? > > Thanks again for raising this, Chris. > > -George > > [1] http://man7.org/linux/man-pages/man3/readdir_r.3.html > ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] libfsimage: replace deprecated readdir_r() with readdir()
On Tue, May 31, 2016 at 1:53 PM, Wei Liu wrote: > On Tue, May 31, 2016 at 11:43:13AM -0400, Chris Patterson wrote: >> On Tue, May 31, 2016 at 6:42 AM, George Dunlap >> wrote: >> > On Mon, May 30, 2016 at 3:32 AM, Chris Patterson wrote: >> >> From: Chris Patterson >> >> >> >> Replace the usage of readdir_r() with readdir() to address >> >> a compilation error due to the deprecation of readdir_r. >> >> >> >> glibc has deprecated this for their next release (2.24): >> >> https://sourceware.org/bugzilla/show_bug.cgi?id=19056 >> >> >> >> Signed-off-by: Chris Patterson >> > >> > Thanks for the patch, Chris. A bit more background would have been >> > helpful -- I did some searching and found a description[1] which says: >> > >> >> Thank you. I should have added these details in the commit message or cover. >> >> > In the current POSIX.1 specification (POSIX.1-2008), readdir(3) is >> > not required to be thread-safe. However, in modern >> > implementations (including the glibc implementation), concurrent >> > calls to readdir(3) that specify different directory streams are >> > thread-safe. Therefore, the use of readdir_r() is generally >> > unnecessary in multithreaded programs. In cases where multiple >> > threads must read from the same directory stream, using readdir(3) >> > with external synchronization is still preferable to the use of >> > readdir_r(), for the reasons given in the points above. >> > >> > The use of the specific directory stream is single-threaded, so for >> > glibc, it looks like using readdir() will be safe. But libxl needs to >> > be able to build on a number of libc's that are not glibc and still be >> > thread-safe. So we need to either 1) verify that readdir() is thread >> > safe on all libc's against which we may compile, or 2) add some >> >> Is there a list of supported libc libraries? I can look into it and >> provide more definitive answers if there are. >> > > We at least care about FreeBSD and NetBSD. Sadly their manuaks don't > provide statement regarding thread safety for readdir and readdir_r. > It's better to err on the safe side. > I'm far from the expert here, but it would appear that both NetBSD's and FreeBSD's libc readdir()/readdir_r() implementations are consistent in their locking mechanisms [1,2]. As such, I would expect readdir() to be equally as safe as readdir_r() for their users. As you noted, there does not appear to be any documented distinction within their man pages [3,4] with regards to thread safety and it seems reasonable that they would have documented these limitations, if present. [1] FreeBSD: https://svnweb.freebsd.org/base/head/lib/libc/gen/readdir.c?view=markup#l98 [2] NetBSD: http://cvsweb.netbsd.org/bsdweb.cgi/src/lib/libc/gen/readdir.c?rev=1.25.6.1&content-type=text/x-cvsweb-markup [3] FreeBSD readdir manpage: https://www.freebsd.org/cgi/man.cgi?query=readdir§ion=3 [4] NetBSD readdir manpage: http://netbsd.gw.com/cgi-bin/man-cgi?readdir Are there other known supported libc implementations? ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] libfsimage: replace deprecated readdir_r() with readdir()
On Wed, Jun 1, 2016 at 8:06 AM, Ian Jackson wrote:
> Ian Jackson writes ("Re: [Xen-devel] [PATCH 1/2] libfsimage: replace
> deprecated readdir_r() with readdir()"):
>> Ian Jackson writes ("Re: [Xen-devel] [PATCH 1/2] libfsimage: replace
>> deprecated readdir_r() with readdir()"):
>> > 2. There may be good reasons to deviate from a formal specification.
>> > Formal specifications can be wrong (for example, they can differ from
>> > established practice, or unuseable, or incoherent). But there has
>> > been no discussion (at least in this thread on xen-devel) which might
>> > suggest that the POSIX specification is wrongheaded here.
>>
>> I have been helpfully referred by a local irc channel to the following
>> attempt to change posix to require that readdir() is threadsafe in the
>> senses required by libx, and to deprecate readdir_r():
>>
>> http://austingroupbugs.net/view.php?id=696
>>
>> I find the comment 0001606 by "dalias" (et seq) totally convincing.
>> The published specification of readdir_r is indeed incoherent. And
>> only contrived implementations of readdir will not be threadsafe in
>> the required sense.
> ...
>> Accordingly, I think all occurrences of readdir_r in our codebase
>> should be replaced by readdir, as proposed by Chris.
>>
>> However, I think the patch is not quite complete, as the change from
>> readdir_r to readdir should also involve removing the local dirent
>> variables associated with each call site.
>
> Also, the commit message needs to be expanded to provide the
> rationale. It should restate the reasoning provided by "dalias" and
> provide links to the austingroupbugs thread and references to the
> comment numbers.
>
> Ian.
Agreed, will post a v2. Thanks for the feedback.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [[PATCH v2 2/2] libxl: replace deprecated readdir_r() with readdir()
From: Chris Patterson
Replace the usage of readdir_r() with readdir() to address a
compilation error under glibc due to the deprecation of readdir_r
for their next release (2.24) [1, 2].
Remove code specific to usage of readdir_r which is no longer required,
such as zalloc_dirent().
--
From the GNU libc manual [3]:
"
It is expected that future versions of POSIX will obsolete readdir_r and
mandate the level of thread safety for readdir which is provided by the
GNU C Library and other implementations today.
"
There is a filed bug in the Austin Group Defect Tracker [4] in which 'dalias'
proposes (in comment 0001632) that:
"
I would like to propose an alternate solution. For readdir, replace the text:
"The readdir() function need not be thread-safe."
with:
"If multiple threads call the readdir() function with the same directory
stream argument and without synchronization to preclude simultaneous
access, then the behavior is undefined."
With this change, the clunky readdir_r function is no longer needed or
useful, and should probably be deprecated. As the only reasonable way
to meet the implementation requirements for readdir is to have the dirent
buffer in the DIR structure, this change should not require any change to
existing implementations.
"
[1] https://sourceware.org/ml/libc-alpha/2016-02/msg00093.html
[2] https://sourceware.org/bugzilla/show_bug.cgi?id=19056
[3]
https://www.gnu.org/software/libc/manual/html_node/Reading_002fClosing-Directory.html
[4] http://austingroupbugs.net/view.php?id=696
--
v2:
- Additional detail in commit message
- Fix readdir loop logic (do not treat NULL as error)
- Cleanup additional related (no longer used) code
Signed-off-by: Chris Patterson
---
tools/libxl/libxl_pvusb.c | 42 +++---
tools/libxl/libxl_utils.c | 14 +-
2 files changed, 4 insertions(+), 52 deletions(-)
diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
index 9f1e842..01819bd 100644
--- a/tools/libxl/libxl_pvusb.c
+++ b/tools/libxl/libxl_pvusb.c
@@ -508,19 +508,10 @@ int libxl_devid_to_device_usbctrl(libxl_ctx *ctx,
return rc;
}
-static void *zalloc_dirent(libxl__gc *gc, const char *dirpath)
-{
-size_t need = offsetof(struct dirent, d_name) +
- pathconf(dirpath, _PC_NAME_MAX) + 1;
-
-return libxl__zalloc(gc, need);
-}
-
static char *usbdev_busaddr_to_busid(libxl__gc *gc, int bus, int addr)
{
DIR *dir;
char *busid = NULL;
-struct dirent *de_buf;
struct dirent *de;
/* invalid hostbus or hostaddr */
@@ -533,22 +524,12 @@ static char *usbdev_busaddr_to_busid(libxl__gc *gc, int
bus, int addr)
return NULL;
}
-de_buf = zalloc_dirent(gc, SYSFS_USB_DEV);
-
-for (;;) {
+while ((de = readdir(dir)) != NULL) {
char *filename;
void *buf;
int busnum = -1;
int devnum = -1;
-int r = readdir_r(dir, de_buf, &de);
-if (r) {
-LOGE(ERROR, "failed to readdir %s", SYSFS_USB_DEV);
-break;
-}
-if (!de)
-break;
-
if (!strcmp(de->d_name, ".") ||
!strcmp(de->d_name, ".."))
continue;
@@ -1157,9 +1138,7 @@ static int usbdev_get_all_interfaces(libxl__gc *gc, const
char *busid,
{
DIR *dir;
char *buf;
-struct dirent *de_buf;
struct dirent *de;
-int rc;
*intfs = NULL;
*num = 0;
@@ -1172,19 +1151,7 @@ static int usbdev_get_all_interfaces(libxl__gc *gc,
const char *busid,
return ERROR_FAIL;
}
-de_buf = zalloc_dirent(gc, SYSFS_USB_DEV);
-
-for (;;) {
-int r = readdir_r(dir, de_buf, &de);
-
-if (r) {
-LOGE(ERROR, "failed to readdir %s", SYSFS_USB_DEV);
-rc = ERROR_FAIL;
-goto out;
-}
-if (!de)
-break;
-
+while ((de = readdir(dir)) != NULL) {
if (!strcmp(de->d_name, ".") ||
!strcmp(de->d_name, ".."))
continue;
@@ -1196,11 +1163,8 @@ static int usbdev_get_all_interfaces(libxl__gc *gc,
const char *busid,
}
}
-rc = 0;
-
-out:
closedir(dir);
-return rc;
+return 0;
}
/* Encode usb interface so that it could be written to xenstore as a key.
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index ceb8825..5730774 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -548,21 +548,9 @@ int libxl__remove_directory(libxl__gc *gc, const char
*dirpath)
goto out;
}
-size_t need = offsetof(struct dirent, d_name) +
-pathconf(dirpath, _PC_NAME_MAX) + 1;
-struct dirent *de_buf = libxl__zalloc(gc, need);
struct dirent *de;
-for (;;) {
-int r = readdir_r(d, de_buf, &de);
-
[Xen-devel] [[PATCH v2 1/2] libfsimage: replace deprecated readdir_r() with readdir()
From: Chris Patterson
Replace the usage of readdir_r() with readdir() to address a
compilation error under glibc due to the deprecation of readdir_r
for their next release (2.24) [1, 2].
--
From the GNU libc manual [3]:
"
It is expected that future versions of POSIX will obsolete readdir_r and
mandate the level of thread safety for readdir which is provided by the
GNU C Library and other implementations today.
"
There is a filed bug in the Austin Group Defect Tracker [4] in which 'dalias'
proposes (in comment 0001632) that:
"
I would like to propose an alternate solution. For readdir, replace the text:
"The readdir() function need not be thread-safe."
with:
"If multiple threads call the readdir() function with the same directory
stream argument and without synchronization to preclude simultaneous
access, then the behavior is undefined."
With this change, the clunky readdir_r function is no longer needed or
useful, and should probably be deprecated. As the only reasonable way
to meet the implementation requirements for readdir is to have the dirent
buffer in the DIR structure, this change should not require any change to
existing implementations.
"
[1] https://sourceware.org/ml/libc-alpha/2016-02/msg00093.html
[2] https://sourceware.org/bugzilla/show_bug.cgi?id=19056
[3]
https://www.gnu.org/software/libc/manual/html_node/Reading_002fClosing-Directory.html
[4] http://austingroupbugs.net/view.php?id=696
--
v2:
- Additional detail in commit message
- Cleanup additional related (no longer used) code
Signed-off-by: Chris Patterson
---
tools/libfsimage/common/fsimage_plugin.c | 17 +
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/tools/libfsimage/common/fsimage_plugin.c
b/tools/libfsimage/common/fsimage_plugin.c
index 3fa06c7..5ab8d93 100644
--- a/tools/libfsimage/common/fsimage_plugin.c
+++ b/tools/libfsimage/common/fsimage_plugin.c
@@ -122,8 +122,7 @@ fail:
static int load_plugins(void)
{
const char *fsdir = getenv("FSIMAGE_FSDIR");
- struct dirent *dp = NULL;
- struct dirent *dpp;
+ struct dirent *de;
DIR *dir = NULL;
char *tmp = NULL;
size_t name_max;
@@ -139,22 +138,17 @@ static int load_plugins(void)
if ((tmp = malloc(name_max + 1)) == NULL)
goto fail;
- if ((dp = malloc(sizeof (struct dirent) + name_max + 1)) == NULL)
- goto fail;
-
if ((dir = opendir(fsdir)) == NULL)
goto fail;
- bzero(dp, sizeof (struct dirent) + name_max + 1);
-
- while (readdir_r(dir, dp, &dpp) == 0 && dpp != NULL) {
- if (strcmp(dpp->d_name, ".") == 0)
+ while ((de = readdir(dir)) != NULL) {
+ if (strcmp(de->d_name, ".") == 0)
continue;
- if (strcmp(dpp->d_name, "..") == 0)
+ if (strcmp(de->d_name, "..") == 0)
continue;
(void) snprintf(tmp, name_max, "%s/%s/fsimage.so", fsdir,
- dpp->d_name);
+ de->d_name);
if (init_plugin(tmp) != 0)
goto fail;
@@ -167,7 +161,6 @@ fail:
if (dir != NULL)
(void) closedir(dir);
free(tmp);
- free(dp);
errno = err;
return (ret);
}
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [[PATCH v2 2/2] libxl: replace deprecated readdir_r() with readdir()
On Thu, Jun 2, 2016 at 6:11 AM, Ian Jackson wrote:
> Chris Patterson writes ("[[PATCH v2 2/2] libxl: replace deprecated
> readdir_r() with readdir()"):
>> -for (;;) {
>> +while ((de = readdir(dir)) != NULL) {
> ...
>> -int r = readdir_r(dir, de_buf, &de);
>> -if (r) {
>> -LOGE(ERROR, "failed to readdir %s", SYSFS_USB_DEV);
>> -break;
>
> Sadly this is not right because it mishandles errors when reading the
> directory, treating them all as EOF. See the error handling info
> in the specification for readdir:
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html
>
You're right, it should check for the error afterwards.
How about something along the lines of:
int saved_errno = errno;
errno = 0;
while ((de = readdir(dir)) != NULL) {
...
}
if (errno) {
LOGE(ERROR, "readdir failed: %s", strerror(errno));
rc = ERROR_FAIL;
}
errno = saved_errno;
...
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [[PATCH v2 2/2] libxl: replace deprecated readdir_r() with readdir()
On Thu, Jun 2, 2016 at 12:13 PM, Ian Jackson wrote:
> Chris Patterson writes ("Re: [[PATCH v2 2/2] libxl: replace deprecated
> readdir_r() with readdir()"):
>> You're right, it should check for the error afterwards.
>>
>> How about something along the lines of:
>>
>> int saved_errno = errno;
>> errno = 0;
>> while ((de = readdir(dir)) != NULL) {
>> ...
>
> Wrong because you need to set errno=0 before each call to readdir.
> I really think you should abandon your efforts to keep the readdir
> call inside the while() condition :-).
>
I agree. How does something like this look?
-int r = readdir_r(dir, de_buf, &de);
-
-if (r) {
+ errno = 0;
+ de = readdir(dir);
+
+ if (!de && errno) {
And I'll apply the same construct for tools/libfsimage/common/fsimage_plugin.c.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v3 2/2] libfsimage: replace deprecated readdir_r() with readdir()
From: Chris Patterson
Replace the usage of readdir_r() with readdir() to address a
compilation error under glibc due to the deprecation of readdir_r
for their next release (2.24) [1, 2].
Add new error checking on readdir(), and fail if error occurs.
--
From the GNU libc manual [3]:
"
It is expected that future versions of POSIX will obsolete readdir_r and
mandate the level of thread safety for readdir which is provided by the
GNU C Library and other implementations today.
"
There is a filed bug in the Austin Group Defect Tracker [4] in which 'dalias'
proposes (in comment 0001632) that:
"
I would like to propose an alternate solution. For readdir, replace the text:
"The readdir() function need not be thread-safe."
with:
"If multiple threads call the readdir() function with the same directory
stream argument and without synchronization to preclude simultaneous
access, then the behavior is undefined."
With this change, the clunky readdir_r function is no longer needed or
useful, and should probably be deprecated. As the only reasonable way
to meet the implementation requirements for readdir is to have the dirent
buffer in the DIR structure, this change should not require any change to
existing implementations.
"
[1] https://sourceware.org/ml/libc-alpha/2016-02/msg00093.html
[2] https://sourceware.org/bugzilla/show_bug.cgi?id=19056
[3]
https://www.gnu.org/software/libc/manual/html_node/Reading_002fClosing-Directory.html
[4] http://austingroupbugs.net/view.php?id=696
Signed-off-by: Chris Patterson
---
v2:
- Additional detail in commit message
- Cleanup additional (no longer used) code
v3:
- Add error check and fail if readdir() fails.
tools/libfsimage/common/fsimage_plugin.c | 21 -
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/tools/libfsimage/common/fsimage_plugin.c
b/tools/libfsimage/common/fsimage_plugin.c
index 3fa06c7..0744e7b 100644
--- a/tools/libfsimage/common/fsimage_plugin.c
+++ b/tools/libfsimage/common/fsimage_plugin.c
@@ -123,7 +123,6 @@ static int load_plugins(void)
{
const char *fsdir = getenv("FSIMAGE_FSDIR");
struct dirent *dp = NULL;
- struct dirent *dpp;
DIR *dir = NULL;
char *tmp = NULL;
size_t name_max;
@@ -139,22 +138,26 @@ static int load_plugins(void)
if ((tmp = malloc(name_max + 1)) == NULL)
goto fail;
- if ((dp = malloc(sizeof (struct dirent) + name_max + 1)) == NULL)
- goto fail;
-
if ((dir = opendir(fsdir)) == NULL)
goto fail;
- bzero(dp, sizeof (struct dirent) + name_max + 1);
+ for (;;) {
+ errno = 0;
+ dp = readdir(dir);
+
+ if (dp == NULL && errno != 0)
+ goto fail;
+
+ if (dp == NULL)
+ break;
- while (readdir_r(dir, dp, &dpp) == 0 && dpp != NULL) {
- if (strcmp(dpp->d_name, ".") == 0)
+ if (strcmp(dp->d_name, ".") == 0)
continue;
- if (strcmp(dpp->d_name, "..") == 0)
+ if (strcmp(dp->d_name, "..") == 0)
continue;
(void) snprintf(tmp, name_max, "%s/%s/fsimage.so", fsdir,
- dpp->d_name);
+ dp->d_name);
if (init_plugin(tmp) != 0)
goto fail;
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v3 1/2] libxl: replace deprecated readdir_r() with readdir()
From: Chris Patterson
Replace the usage of readdir_r() with readdir() to address a
compilation error under glibc due to the deprecation of readdir_r
for their next release (2.24) [1, 2].
Remove code specific to usage of readdir_r which is no longer required,
such as zalloc_dirent().
--
From the GNU libc manual [3]:
"
It is expected that future versions of POSIX will obsolete readdir_r and
mandate the level of thread safety for readdir which is provided by the
GNU C Library and other implementations today.
"
There is a filed bug in the Austin Group Defect Tracker [4] in which 'dalias'
proposes (in comment 0001632) that:
"
I would like to propose an alternate solution. For readdir, replace the text:
"The readdir() function need not be thread-safe."
with:
"If multiple threads call the readdir() function with the same directory
stream argument and without synchronization to preclude simultaneous
access, then the behavior is undefined."
With this change, the clunky readdir_r function is no longer needed or
useful, and should probably be deprecated. As the only reasonable way
to meet the implementation requirements for readdir is to have the dirent
buffer in the DIR structure, this change should not require any change to
existing implementations.
"
[1] https://sourceware.org/ml/libc-alpha/2016-02/msg00093.html
[2] https://sourceware.org/bugzilla/show_bug.cgi?id=19056
[3]
https://www.gnu.org/software/libc/manual/html_node/Reading_002fClosing-Directory.html
[4] http://austingroupbugs.net/view.php?id=696
Signed-off-by: Chris Patterson
---
v2:
- Additional detail in commit message
- Fix readdir loop logic (do not treat NULL as error)
- Cleanup additional (no longer used) code
v3:
- Restore original loop & error handling
tools/libxl/libxl_pvusb.c | 24 ++--
tools/libxl/libxl_utils.c | 8 +++-
2 files changed, 9 insertions(+), 23 deletions(-)
diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
index 9f1e842..ec97ff8 100644
--- a/tools/libxl/libxl_pvusb.c
+++ b/tools/libxl/libxl_pvusb.c
@@ -508,19 +508,10 @@ int libxl_devid_to_device_usbctrl(libxl_ctx *ctx,
return rc;
}
-static void *zalloc_dirent(libxl__gc *gc, const char *dirpath)
-{
-size_t need = offsetof(struct dirent, d_name) +
- pathconf(dirpath, _PC_NAME_MAX) + 1;
-
-return libxl__zalloc(gc, need);
-}
-
static char *usbdev_busaddr_to_busid(libxl__gc *gc, int bus, int addr)
{
DIR *dir;
char *busid = NULL;
-struct dirent *de_buf;
struct dirent *de;
/* invalid hostbus or hostaddr */
@@ -533,16 +524,15 @@ static char *usbdev_busaddr_to_busid(libxl__gc *gc, int
bus, int addr)
return NULL;
}
-de_buf = zalloc_dirent(gc, SYSFS_USB_DEV);
-
for (;;) {
char *filename;
void *buf;
int busnum = -1;
int devnum = -1;
-int r = readdir_r(dir, de_buf, &de);
-if (r) {
+errno = 0;
+de = readdir(dir);
+if (!de && errno) {
LOGE(ERROR, "failed to readdir %s", SYSFS_USB_DEV);
break;
}
@@ -1157,7 +1147,6 @@ static int usbdev_get_all_interfaces(libxl__gc *gc, const
char *busid,
{
DIR *dir;
char *buf;
-struct dirent *de_buf;
struct dirent *de;
int rc;
@@ -1172,12 +1161,11 @@ static int usbdev_get_all_interfaces(libxl__gc *gc,
const char *busid,
return ERROR_FAIL;
}
-de_buf = zalloc_dirent(gc, SYSFS_USB_DEV);
-
for (;;) {
-int r = readdir_r(dir, de_buf, &de);
+errno = 0;
+de = readdir(dir);
-if (r) {
+if (!de && errno) {
LOGE(ERROR, "failed to readdir %s", SYSFS_USB_DEV);
rc = ERROR_FAIL;
goto out;
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index ceb8825..4ca6bcb 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -548,14 +548,12 @@ int libxl__remove_directory(libxl__gc *gc, const char
*dirpath)
goto out;
}
-size_t need = offsetof(struct dirent, d_name) +
-pathconf(dirpath, _PC_NAME_MAX) + 1;
-struct dirent *de_buf = libxl__zalloc(gc, need);
struct dirent *de;
for (;;) {
-int r = readdir_r(d, de_buf, &de);
-if (r) {
+errno = 0;
+de = readdir(d);
+if (!de && errno) {
LOGE(ERROR, "failed to readdir %s for removal", dirpath);
rc = ERROR_FAIL;
break;
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH] ns16550: mask transmit holding register empty interrupt when tx is stopped
From: Chris Patterson
The uart generates an interrupt whenever the transmit holding register is
empty and UART_IER_ETHREI is set in UART_IER. Currently, Xen's ns16550
driver does not currently mask this interrupt when transmit is stopped,
unlike other platforms such as Linux [1].
Toggle UART_IER_ETHREI flag in the UART_IER according to the state dictated
by stop_tx and start_tx hooks.
On the Tegra platform (forthcoming series), the reset via reading IIR does not
prevent re-assertion of THRE. This causes Xen to hang in the interrupt
handler's while loop whenever there is no data to transmit. This behavior
(bug?)
is addressed by utilizing the start & stop tx hooks.
This has been tested on various x86 PCs for any obvious signs of regressions.
[1]
http://lxr.free-electrons.com/source/drivers/tty/serial/8250/8250_port.c?v=4.7#L1518
Signed-off-by: Chris Patterson
---
xen/drivers/char/ns16550.c | 26 --
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index b2b5f56..cae7f1b 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -656,8 +656,8 @@ static void ns16550_setup_postirq(struct ns16550 *uart)
ns_write_reg(uart,
UART_MCR, UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS);
-/* Enable receive and transmit interrupts. */
-ns_write_reg(uart, UART_IER, UART_IER_ERDAI | UART_IER_ETHREI);
+/* Enable receive interrupts. */
+ns_write_reg(uart, UART_IER, UART_IER_ERDAI);
}
if ( uart->irq >= 0 )
@@ -813,6 +813,26 @@ static int __init ns16550_irq(struct serial_port *port)
return ((uart->irq > 0) ? uart->irq : -1);
}
+static void ns16550_start_tx(struct serial_port *port)
+{
+struct ns16550 *uart = port->uart;
+unsigned int ier = ns_read_reg(uart, UART_IER);
+
+/* unmask transmit holding register empty interrupt if currently masked */
+if (!(ier & UART_IER_ETHREI))
+ns_write_reg(uart, UART_IER, ier | UART_IER_ETHREI);
+}
+
+static void ns16550_stop_tx(struct serial_port *port)
+{
+struct ns16550 *uart = port->uart;
+unsigned int ier = ns_read_reg(uart, UART_IER);
+
+/* mask off transmit holding register empty interrupt if currently
unmasked */
+if (ier & UART_IER_ETHREI)
+ns_write_reg(uart, UART_IER, ier & ~UART_IER_ETHREI);
+}
+
#ifdef CONFIG_ARM
static const struct vuart_info *ns16550_vuart_info(struct serial_port *port)
{
@@ -832,6 +852,8 @@ static struct uart_driver __read_mostly ns16550_driver = {
.putc = ns16550_putc,
.getc = ns16550_getc,
.irq = ns16550_irq,
+.start_tx = ns16550_start_tx,
+.stop_tx = ns16550_stop_tx,
#ifdef CONFIG_ARM
.vuart_info = ns16550_vuart_info,
#endif
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] ns16550: mask transmit holding register empty interrupt when tx is stopped
On Wed, Aug 17, 2016 at 10:29 AM, Jan Beulich wrote:
>>>> On 17.08.16 at 16:02, wrote:
>> From: Chris Patterson
>>
>> The uart generates an interrupt whenever the transmit holding register is
>> empty and UART_IER_ETHREI is set in UART_IER. Currently, Xen's ns16550
>> driver does not currently mask this interrupt when transmit is stopped,
>> unlike other platforms such as Linux [1].
>>
>> Toggle UART_IER_ETHREI flag in the UART_IER according to the state dictated
>> by stop_tx and start_tx hooks.
>>
>> On the Tegra platform (forthcoming series), the reset via reading IIR does
>> not
>> prevent re-assertion of THRE. This causes Xen to hang in the interrupt
>> handler's while loop whenever there is no data to transmit. This behavior
>> (bug?)
>> is addressed by utilizing the start & stop tx hooks.
>
> Looks mostly okay from a technical pov, but there are a few minor
> (mostly style) issues.
>
>> [1]
>> http://lxr.free-electrons.com/source/drivers/tty/serial/8250/8250_port.c?v=4.7#L1518
>
> I'd appreciate for such references to be to the canonical (i.e. Linus'es)
> tree.
>
>> @@ -813,6 +813,26 @@ static int __init ns16550_irq(struct serial_port *port)
>> return ((uart->irq > 0) ? uart->irq : -1);
>> }
>>
>> +static void ns16550_start_tx(struct serial_port *port)
>> +{
>> +struct ns16550 *uart = port->uart;
>> +unsigned int ier = ns_read_reg(uart, UART_IER);
>
> Please use u8 or unsigned char here, as is done elsewhere in the file.
>
>> +/* unmask transmit holding register empty interrupt if currently masked
>> */
>
> Comment style: Upper case at the beginning; the full stop at the
> end has become optional recently.
>
>> +if (!(ier & UART_IER_ETHREI))
>
> Blanks missing inside the parentheses.
>
> Jan
>
ACK, will fix these in v2. Thank you for the review.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2] ns16550: mask transmit holding register empty interrupt when tx is stopped
From: Chris Patterson
The uart generates an interrupt whenever the transmit holding register is
empty and UART_IER_ETHREI is set in UART_IER. Currently, Xen's ns16550
driver does not currently mask this interrupt when transmit is stopped,
unlike other platforms such as Linux [1].
Toggle UART_IER_ETHREI flag in the UART_IER according to the state dictated
by stop_tx and start_tx hooks.
On the Tegra platform (forthcoming series), the reset via reading IIR does not
prevent re-assertion of THRE. This causes Xen to hang in the interrupt
handler's while loop whenever there is no data to transmit. This behavior
(bug?)
is addressed by utilizing the start & stop tx hooks.
This has been tested on various x86 PCs for any obvious signs of regressions.
[1]
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/8250/8250_port.c?id=refs/tags/v4.8-rc2#n1518
Signed-off-by: Chris Patterson
---
xen/drivers/char/ns16550.c | 26 --
1 file changed, 24 insertions(+), 2 deletions(-)
Changes in v2:
- unsigned int to u8
- formatting fixes
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index b2b5f56..1da103a 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -656,8 +656,8 @@ static void ns16550_setup_postirq(struct ns16550 *uart)
ns_write_reg(uart,
UART_MCR, UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS);
-/* Enable receive and transmit interrupts. */
-ns_write_reg(uart, UART_IER, UART_IER_ERDAI | UART_IER_ETHREI);
+/* Enable receive interrupts. */
+ns_write_reg(uart, UART_IER, UART_IER_ERDAI);
}
if ( uart->irq >= 0 )
@@ -813,6 +813,26 @@ static int __init ns16550_irq(struct serial_port *port)
return ((uart->irq > 0) ? uart->irq : -1);
}
+static void ns16550_start_tx(struct serial_port *port)
+{
+struct ns16550 *uart = port->uart;
+u8 ier = ns_read_reg(uart, UART_IER);
+
+/* Unmask transmit holding register empty interrupt if currently masked. */
+if ( !(ier & UART_IER_ETHREI) )
+ns_write_reg(uart, UART_IER, ier | UART_IER_ETHREI);
+}
+
+static void ns16550_stop_tx(struct serial_port *port)
+{
+struct ns16550 *uart = port->uart;
+u8 ier = ns_read_reg(uart, UART_IER);
+
+/* Mask off transmit holding register empty interrupt if currently
unmasked. */
+if ( ier & UART_IER_ETHREI )
+ns_write_reg(uart, UART_IER, ier & ~UART_IER_ETHREI);
+}
+
#ifdef CONFIG_ARM
static const struct vuart_info *ns16550_vuart_info(struct serial_port *port)
{
@@ -832,6 +852,8 @@ static struct uart_driver __read_mostly ns16550_driver = {
.putc = ns16550_putc,
.getc = ns16550_getc,
.irq = ns16550_irq,
+.start_tx = ns16550_start_tx,
+.stop_tx = ns16550_stop_tx,
#ifdef CONFIG_ARM
.vuart_info = ns16550_vuart_info,
#endif
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] Xen ARM community call
> I would suggest to start with a 1 hour meeting on the Wednesday 23rd > November. I know that people are spread across different timezones, so I > would like to gather thought before choosing a time. > Sounds good! EST (GMT-5). Cheers, -Chris ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH] libxl: compilation warning fix for arm & aarch64
From: Chris Patterson GCC 6 will warn on unused static const variables in c modules: https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00847.html When compiling with LIBXL_HAVE_NO_SUSPEND_RESUME set (arm & aarch64), the compiler emits the following errors: xl_cmdimpl.c:101:19: error: 'migrate_report' defined but not used [-Werror=unused-const-variable=] xl_cmdimpl.c:99:19: error: 'migrate_permission_to_go' defined but not used [-Werror=unused-const-variable=] xl_cmdimpl.c:97:19: error: 'migrate_receiver_ready' defined but not used [-Werror=unused-const-variable=] xl_cmdimpl.c:95:19: error: 'migrate_receiver_banner' defined but not used [-Werror=unused-const-variable=] These unused const variables are only used in functions which exist between the ifndef block: #ifndef LIBXL_HAVE_NO_SUSPEND_RESUME ... #endif Wrap the same ifndef around these variables. Signed-off-by: Chris Patterson --- tools/libxl/xl_cmdimpl.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index d1fcfa4..ada8178 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -92,6 +92,7 @@ static int fd_lock = -1; static const char savefileheader_magic[32]= "Xen saved domain, xl format\n \0 \r"; +#ifndef LIBXL_HAVE_NO_SUSPEND_RESUME static const char migrate_receiver_banner[]= "xl migration receiver ready, send binary domain data.\n"; static const char migrate_receiver_ready[]= @@ -100,6 +101,8 @@ static const char migrate_permission_to_go[]= "domain is yours, you are cleared to unpause"; static const char migrate_report[]= "my copy unpause results are as follows"; +#endif + /* followed by one byte: * 0: everything went well, domain is running *next thing is we all exit -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 2/6] xen/arm: domain_build: Inherit GIC's interrupt-parent from host device tree
From: "Chris Patterson"
Currently, the interrupt parent is left undefined during creation in
make_gic_node(). In cases where a non-GIC interrupt controller is present,
this can lead to incorrect assignment of interrupt parents.
On the Tegra, the gic's interrupt parent is set to itself:
gic: interrupt-controller@0,50041000 {
compatible = "arm,gic-400";
#interrupt-cells = <3>;
interrupt-controller;
reg = <0x0 0x50041000 0x0 0x1000>,
<0x0 0x50042000 0x0 0x2000>,
<0x0 0x50044000 0x0 0x2000>,
<0x0 0x50046000 0x0 0x2000>;
interrupts = ;
interrupt-parent = <&gic>;
};
To prevent the hardware domain from assuming the Legacy Interrupt Controller
(lic) as the GIC's interrupt-parent, this change explicitly assigns
the interrupt-parent property from the host device tree.
Authored-by: Kyle Temkin
Signed-off-by: Kyle Temkin
Signed-off-by: Chris Patterson
Reviewed-by: Stefano Stabellini
---
changes from rfc:
- commit message documentation improvements
---
xen/arch/arm/domain_build.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index de59e5f..cb66304 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -778,8 +778,8 @@ static int make_gic_node(const struct domain *d, void *fdt,
{
const struct dt_device_node *gic = dt_interrupt_controller;
int res = 0;
-const void *addrcells, *sizecells;
-u32 addrcells_len, sizecells_len;
+const void *addrcells, *sizecells, *iparent;
+u32 addrcells_len, sizecells_len, iparent_len;
/*
* Xen currently supports only a single GIC. Discard any secondary
@@ -809,6 +809,19 @@ static int make_gic_node(const struct domain *d, void *fdt,
return res;
}
+/*
+ * If available, explicitly inherit interrupt-parent property from host
+ * device tree. This will prevent the risk of incorrect identification
+ * of the parent on platforms with more than one interrupt controller.
+ */
+iparent = dt_get_property(gic, "interrupt-parent", &iparent_len);
+if ( iparent )
+{
+res = fdt_property(fdt, "interrupt-parent", iparent, iparent_len);
+if ( res )
+ return res;
+}
+
addrcells = dt_get_property(gic, "#address-cells", &addrcells_len);
if ( addrcells )
{
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 5/6] xen/arm: Add function to query IRQ 'ownership'
From: "Chris Patterson"
The addition of new IRQ-related platform hooks now allow platforms to
perform platform-specific interrupt logic, such as allowing virtualization
of platform-specific interrupt controller hardware.
This commit adds the ability for the platform to identify the domain
a given IRQ is routed to, allowing platform logic to deny access to
registers associated with a given IRQ unless the requesting domain
'owns' the IRQ. This will be used on Tegra platforms, where the hardware
domain needs access to its legacy interrupt controller, but should not
be able to control registers that correspond to other domains' IRQs, or
sections associated with IRQs routed to Xen.
Authored-by: Kyle Temkin
Signed-off-by: Kyle Temkin
Signed-off-by: Chris Patterson
Reviewed-by: Stefano Stabellini
---
changes since rfc:
- formatting & code style cleanup
---
xen/arch/arm/irq.c| 10 ++
xen/include/asm-arm/irq.h | 2 ++
2 files changed, 12 insertions(+)
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 0b4eaa9..51bce58 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -143,6 +143,16 @@ static inline struct domain *irq_get_domain(struct
irq_desc *desc)
return irq_get_guest_info(desc)->d;
}
+domid_t irq_get_domain_id(struct irq_desc *desc)
+{
+/* If this domain isn't routed to a guest, return DOMID_XEN. */
+if ( !test_bit(_IRQ_GUEST, &desc->status) )
+return DOMID_XEN;
+
+/* Otherise, get the guest domain's information. */
+return irq_get_domain(desc)->domain_id;
+}
+
void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
{
if ( desc != NULL )
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index 4849f16..d0fd6db 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -44,6 +44,8 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
unsigned int irq, const char *devname);
int release_guest_irq(struct domain *d, unsigned int irq);
+domid_t irq_get_domain_id(struct irq_desc *desc);
+
void arch_move_irqs(struct vcpu *v);
#define arch_evtchn_bind_pirq(d, pirq) ((void)((d) + (pirq)))
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
From: "Chris Patterson"
Tegra devices have a legacy interrupt controller (lic, or ictlr) that
must be programmed in parallel with their primary GIC. For all intents
and purposes, we treat these devices attached to this controller as
connected to the primary GIC, as it will be handling their interrupts.
This commit adds support for exposing the ictlr to the hardware domain;
but a future commit will extend this to support exposing a virtualized
version of the ictlr to the hardware domain, and to ensure that
interrupts are unmasked properly when routed to a Xen, or to a domain
other than the hardware domain.
Authored-by: Kyle Temkin
Signed-off-by: Kyle Temkin
Signed-off-by: Chris Patterson
---
changes since rfc:
- use bool instead of bool_t
- formatting & code style cleanup
- fix dt compat label (nvidia,tegra120 -> nvidia,tegra124) for K1
- separate mediated legacy interrupt controller into its own module
- split tegra_ictlr_set_interrupt_enable() into
tegra_lic_set_interrupt_type_normal() and
tegra_lic_set_interrupt_enable()
- added a couple helper functions to reduce duplicated logic
- added wrapper tegra_lic_readl and writel functions for external use (mlic)
- re-order defines in tegra.h
- cleanup tegra_init() that was previously in patch 6
---
xen/arch/arm/platforms/Makefile | 2 +
xen/arch/arm/platforms/tegra.c| 313 ++
xen/include/asm-arm/platforms/tegra.h | 54 ++
3 files changed, 369 insertions(+)
create mode 100644 xen/arch/arm/platforms/tegra.c
create mode 100644 xen/include/asm-arm/platforms/tegra.h
diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
index 49fa683..d7033d2 100644
--- a/xen/arch/arm/platforms/Makefile
+++ b/xen/arch/arm/platforms/Makefile
@@ -6,5 +6,7 @@ obj-$(CONFIG_ARM_32) += omap5.o
obj-$(CONFIG_ARM_32) += rcar2.o
obj-$(CONFIG_ARM_64) += seattle.o
obj-$(CONFIG_ARM_32) += sunxi.o
+obj-$(CONFIG_ARM_32) += tegra.o
+obj-$(CONFIG_ARM_64) += tegra.o
obj-$(CONFIG_ARM_64) += xgene-storm.o
obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
diff --git a/xen/arch/arm/platforms/tegra.c b/xen/arch/arm/platforms/tegra.c
new file mode 100644
index 000..bdd9966
--- /dev/null
+++ b/xen/arch/arm/platforms/tegra.c
@@ -0,0 +1,312 @@
+/*
+ * NVIDIA Tegra specific settings
+ *
+ * Ian Campbell; Copyright (c) 2014 Citrix Systems
+ * Kyle Temkin; Copyright (c) 2016 Assured Information Security, Inc.
+ * Chris Patterson; Copyright (c) 2016 Assured Information Security, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+
+/* Permanent mapping to the Tegra legacy interrupt controller. */
+static void __iomem *tegra_ictlr_base;
+
+/*
+ * List of legacy interrupt controllers that can be used to route
+ * Tegra interrupts.
+ */
+static const char * const tegra_interrupt_compat[] __initconst =
+{
+"nvidia,tegra124-ictlr", /* Tegra K1 controllers */
+"nvidia,tegra210-ictlr" /* Tegra X1 controllers */
+};
+
+/*
+ * Returns true iff the given IRQ belongs to a supported tegra interrupt
+ * controller.
+ */
+static bool tegra_irq_belongs_to_ictlr(const struct dt_raw_irq * rirq) {
+int i;
+
+for ( i = 0; i < ARRAY_SIZE(tegra_interrupt_compat); i++ ) {
+if ( dt_device_is_compatible(rirq->controller,
tegra_interrupt_compat[i]) )
+return true;
+}
+
+return false;
+}
+
+/*
+ * Returns true iff the given IRQ is routable -- that is, if it is descended
+ * from the platform's primary GIC.
+ */
+static bool tegra_irq_is_routable(const struct dt_raw_irq * rirq)
+{
+/* If the IRQ connects directly to our GIC, it's trivially routable. */
+if ( rirq->controller == dt_interrupt_controller )
+return true;
+
+/*
+ * If the IRQ belongs to a legacy interrupt controller, then it's
+ * effectively owned by the GIC, and is routable.
+ */
+if ( tegra_irq_belongs_to_ictlr(rirq) )
+return true;
+
+return false;
+}
+
+/*
+ * Platform-specific reset code for the Tegra devices.
+ * Should not return.
+ */
+static void tegra_reset(void)
+{
+void __iomem *addr;
+u32 val;
+
+addr = ioremap_nocache(TEGRA_RESET_BASE, TEGRA_RESET_SIZE);
+if ( !addr )
+{
+printk(XENLOG_ERR "Tegra: Unable to map tegra reset address. Reset
failed!\n");
+return;
+}
+
+/* Write into the reset devi
[Xen-devel] [PATCH 0/6] Initial Tegra platform support
The attached patch-set adds support for 32-bit and 64-bit Tegra SoCs; including support for the Jetson TK1 and Jetson TX1 boards, as well as the Pixel C tablet. Previous revisions have been tested against the TK1, TX1, and Pixel C. This current patch set has only been tested against the TX1 due to my curent hardware availability, but should be OK on the other platforms. Many thanks to Ian Campbell, whose original Jetson TK1 patchset contained a lot of pointers in the right direction, and helped us to get started on this one! This patch set includes: - A minor serial quirk to get the NS16550 on the Tegra working. - Support for the Tegra Legacy Interrupt Controller, which is necessary to get interrupt routing working correctly on Tegra devices. In this version of the patchset, the interrupt controller is supported via platform quirks. - A few additional minor features and logic fixes to support the Tegra. An example would be the Tegra-specific reset logic. This patch set does NOT include: - Support for the Tegra-specific IOMMU. This means this platform doesn't yet support device passthrough. This patch set includes fixes and cleanup requested from the original RFC posted by Kyle Temkin. Kyle has done most of the authoring on this, but I am picking it up to address the RFC reviews and push it though. Chris Patterson (6): xen/arm: platforms: Add earlyprintk and serial support for Tegra boards. xen/arm: domain_build: Inherit GIC's interrupt-parent from host device tree xen/arm: Allow platforms to hook IRQ routing xen/arm: platforms: Add Tegra platform to support basic IRQ routing xen/arm: Add function to query IRQ 'ownership' xen/arm: platforms/tegra: Ensure the hwdom can only affect its own interrupts xen/arch/arm/Rules.mk | 1 + xen/arch/arm/domain_build.c| 29 ++- xen/arch/arm/irq.c | 15 +- xen/arch/arm/platform.c| 30 +++ xen/arch/arm/platforms/Makefile| 4 + xen/arch/arm/platforms/tegra-mlic.c| 261 +++ xen/arch/arm/platforms/tegra.c | 326 + xen/common/device_tree.c | 8 +- xen/drivers/char/ns16550.c | 28 ++- xen/include/asm-arm/irq.h | 2 + xen/include/asm-arm/platform.h | 12 ++ xen/include/asm-arm/platforms/tegra-mlic.h | 34 +++ xen/include/asm-arm/platforms/tegra.h | 54 + xen/include/xen/8250-uart.h| 1 + 14 files changed, 788 insertions(+), 17 deletions(-) create mode 100644 xen/arch/arm/platforms/tegra-mlic.c create mode 100644 xen/arch/arm/platforms/tegra.c create mode 100644 xen/include/asm-arm/platforms/tegra-mlic.h create mode 100644 xen/include/asm-arm/platforms/tegra.h -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 3/6] xen/arm: Allow platforms to hook IRQ routing
From: "Chris Patterson"
Some common platforms (e.g. Tegra) have non-traditional IRQ controllers
that must be programmed in addition to their primary GICs-- and which
can come in unusual topologies. Device trees for targets that feature
these controllers often deviate from the conventions that Xen expects.
This commit provides a foundation for support of these platforms, by
allowing the platform to decide which IRQs can be routed by Xen, rather
than assuming that only GIC-connected IRQs can be routed. This enables
platform specific logic to routing the IRQ to Xen and Guest.
As dt_irq_translate() is presently hard-coded to just support the
primary interrupt controller, instead rely on the newly added
platform_irq_is_routable() check instead. The default behaviour of this
new function should be consistent with the previous checks for platforms
that do not implement it.
Authored-by: Kyle Temkin
Signed-off-by: Kyle Temkin
Signed-off-by: Chris Patterson
---
changes since rfc:
- use bool instead of bool_t
- formatting & code style cleanup
- reuse dt_irq_translate() path and drop platform_irq_for_device() approach
- use const qualifier for platform_irq_is_routable rirq argument
---
xen/arch/arm/domain_build.c| 12
xen/arch/arm/irq.c | 5 +++--
xen/arch/arm/platform.c| 30 ++
xen/common/device_tree.c | 8 +++-
xen/include/asm-arm/platform.h | 12
5 files changed, 56 insertions(+), 11 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index cb66304..92536dd 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1120,12 +1120,16 @@ static int handle_device(struct domain *d, struct
dt_device_node *dev,
/*
* Don't map IRQ that have no physical meaning
- * ie: IRQ whose controller is not the GIC
+ * ie: IRQ that does not wind up being controlled by the GIC
+ * (Note that we can't just check to see if an IRQ is owned by the GIC,
+ * as some platforms have a controller between the device irq and the
GIC,
+ * such as the Tegra legacy interrupt controller.)
*/
-if ( rirq.controller != dt_interrupt_controller )
+if ( !platform_irq_is_routable(&rirq) )
{
-dt_dprintk("irq %u not connected to primary controller. Connected
to %s\n",
- i, dt_node_full_name(rirq.controller));
+dt_dprintk("irq %u not (directly or indirectly) connected to
primary"
+"controller. Connected to %s\n", i,
+dt_node_full_name(rirq.controller));
continue;
}
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index f3f20a6..0b4eaa9 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -26,6 +26,7 @@
#include
#include
+#include
static unsigned int local_irqs_type[NR_LOCAL_IRQS];
static DEFINE_SPINLOCK(local_irqs_type_lock);
@@ -369,7 +370,7 @@ int setup_irq(unsigned int irq, unsigned int irqflags,
struct irqaction *new)
/* First time the IRQ is setup */
if ( disabled )
{
-gic_route_irq_to_xen(desc, GIC_PRI_IRQ);
+platform_route_irq_to_xen(desc, GIC_PRI_IRQ);
/* It's fine to use smp_processor_id() because:
* For PPI: irq_desc is banked
* For SPI: we don't care for now which CPU will receive the
@@ -506,7 +507,7 @@ int route_irq_to_guest(struct domain *d, unsigned int virq,
if ( retval )
goto out;
-retval = gic_route_irq_to_guest(d, virq, desc, GIC_PRI_IRQ);
+retval = platform_route_irq_to_guest(d, virq, desc, GIC_PRI_IRQ);
spin_unlock_irqrestore(&desc->lock, flags);
diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
index 0af6d57..539ee3b 100644
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -147,6 +147,36 @@ bool_t platform_device_is_blacklisted(const struct
dt_device_node *node)
return (dt_match_node(blacklist, node) != NULL);
}
+int platform_route_irq_to_guest(struct domain *d, unsigned int virq,
+struct irq_desc *desc, unsigned int priority)
+{
+if ( platform && platform->route_irq_to_guest )
+return platform->route_irq_to_guest(d, virq, desc, priority);
+else
+return gic_route_irq_to_guest(d, virq, desc, priority);
+}
+
+void platform_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
+{
+if ( platform && platform->route_irq_to_xen )
+platform->route_irq_to_xen(desc, priority);
+else
+gic_route_irq_to_xen(desc, priority);
+}
+
+bool platform_irq_is_routable(const struct dt_raw_irq * rirq)
+{
+/*
+ * If we have a platform-specific method to determine if an IRQ is
routable,
+ * check that; otherwise fall back to checkin
[Xen-devel] [PATCH 1/6] xen/arm: platforms: Add earlyprintk and serial support for Tegra boards.
From: Chris Patterson
Tegra boards feature a NS16550-compatible serial mapped into the MMIO
space. Add support for its use both as a full-featured serial port and
as an earlyprintk driver.
This patch adds a quirk for platforms, such as the Tegra, which require
require the NS16550 Rx timeout interrupt to be enabled for receive to
function properly. The same quirk is applied in the eqvuialent Linux
driver [1].
This quirk is selectively enabled for the platform using a new "hw_quirks"
member with a corresponding set of bitmasks. The existing quirk,
dw_usr_bsy was updated to match this approach as well.
[1]
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4539c24fe4f92c09ee668ef959d3e8180df619b9
Signed-off-by: Kyle Temkin
Signed-off-by: Chris Patterson
---
changes from rfc:
- use bitmask for quirks in ns1660, including dw_usr_bsy
---
xen/arch/arm/Rules.mk | 1 +
xen/drivers/char/ns16550.c | 28
xen/include/xen/8250-uart.h | 1 +
3 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
index 569a0ba..43b32d0 100644
--- a/xen/arch/arm/Rules.mk
+++ b/xen/arch/arm/Rules.mk
@@ -44,6 +44,7 @@ EARLY_PRINTK_vexpress := pl011,0x1c09
EARLY_PRINTK_xgene-mcdivitt := 8250,0x1c021000,2
EARLY_PRINTK_xgene-storm:= 8250,0x1c02,2
EARLY_PRINTK_zynqmp := cadence,0xff00
+EARLY_PRINTK_tegra := 8250,0x70006000,2
ifneq ($(EARLY_PRINTK_$(CONFIG_EARLY_PRINTK)),)
EARLY_PRINTK_CFG := $(subst $(comma), ,$(EARLY_PRINTK_$(CONFIG_EARLY_PRINTK)))
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index e4de3b4..1b75e89 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -62,7 +62,7 @@ static struct ns16550 {
struct timer resume_timer;
unsigned int timeout_ms;
bool_t intr_works;
-bool_t dw_usr_bsy;
+uint8_t hw_quirks;
#ifdef CONFIG_HAS_PCI
/* PCI card parameters. */
bool_t pb_bdf_enable; /* if =1, pb-bdf effective, port behind bridge */
@@ -414,6 +414,10 @@ static const struct ns16550_config __initconst
uart_config[] =
};
#endif
+/* Various hardware quirks/features that may be need be enabled per device */
+#define HW_QUIRKS_DW_USR_BSY (1<<0)
+#define HW_QUIRKS_USE_RTOIE (1<<1)
+
static void ns16550_delayed_resume(void *data);
static u8 ns_read_reg(struct ns16550 *uart, unsigned int reg)
@@ -578,7 +582,7 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
/* No interrupts. */
ns_write_reg(uart, UART_IER, 0);
-if ( uart->dw_usr_bsy &&
+if ( (uart->hw_quirks & HW_QUIRKS_DW_USR_BSY) &&
(ns_read_reg(uart, UART_IIR) & UART_IIR_BSY) == UART_IIR_BSY )
{
/* DesignWare 8250 detects if LCR is written while the UART is
@@ -651,12 +655,23 @@ static void ns16550_setup_postirq(struct ns16550 *uart)
{
if ( uart->irq > 0 )
{
+u8 ier_value = 0;
+
/* Master interrupt enable; also keep DTR/RTS asserted. */
ns_write_reg(uart,
UART_MCR, UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS);
/* Enable receive interrupts. */
-ns_write_reg(uart, UART_IER, UART_IER_ERDAI);
+ier_value = UART_IER_ERDAI;
+
+/*
+ * If we're on a platform that needs Rx timeouts enabled, also
+ * set Rx TimeOut Interrupt Enable (RTOIE).
+ */
+if ( uart->hw_quirks & HW_QUIRKS_USE_RTOIE )
+ ier_value |= UART_IER_RTOIE;
+
+ns_write_reg(uart, UART_IER, ier_value);
}
if ( uart->irq >= 0 )
@@ -1271,7 +1286,11 @@ static int __init ns16550_uart_dt_init(struct
dt_device_node *dev,
return -EINVAL;
uart->irq = res;
-uart->dw_usr_bsy = dt_device_is_compatible(dev, "snps,dw-apb-uart");
+if ( dt_device_is_compatible(dev, "snps,dw-apb-uart") )
+uart->hw_quirks |= HW_QUIRKS_DW_USR_BSY;
+
+if ( dt_device_is_compatible(dev, "nvidia,tegra20-uart") )
+uart->hw_quirks |= HW_QUIRKS_USE_RTOIE;
uart->vuart.base_addr = uart->io_base;
uart->vuart.size = uart->io_size;
@@ -1292,6 +1311,7 @@ static const struct dt_device_match ns16550_dt_match[]
__initconst =
DT_MATCH_COMPATIBLE("ns16550"),
DT_MATCH_COMPATIBLE("ns16550a"),
DT_MATCH_COMPATIBLE("snps,dw-apb-uart"),
+DT_MATCH_COMPATIBLE("nvidia,tegra20-uart"),
{ /* sentinel */ },
};
diff --git a/xen/include/xen/8250-uart.h b/xen/include/xen/8250-uart.h
index c6b62c8..2ad0ee6 100644
--- a/xen/include/xen/8250-uart.h
+++ b/xen/include/xen/8250-uart.h
@@ -41,6 +41,7 @@
#define UART_IER_ETHREI 0x02/* tx reg. empty*/
#define UART_IER_ELSI 0x04/* rx line status */
#define UART_IER_EMSI 0x08/* MODEM status */
+#
[Xen-devel] [PATCH 6/6] xen/arm: platforms/tegra: Ensure the hwdom can only affect its own interrupts
From: Chris Patterson
Several Tegra hardware devices, and the Tegra device tree, expect
the presence of a Tegra Legacy Interrupt Controller (LIC) in the hardware
domain. Accordingly, we'll need to expose (most of) the LIC's registers
to the hardware domain.
As the Tegra LIC provides the ability to modify interrupt delivery (e.g.
by masking interrupts, forcing asserting/clearing them, or adjusting
their prority), it's important that the hardware domain's access be
mediated. This commit adds read/write handlers that prohibit
modification of register sections corresponding to interrupts not owned
by the hardware domain.
Note that this is written to be domain agnostic; this allows the
potential to e.g. map the ictlr into multiple domains if this is desired
for passthrough in the future.
Authored-by: Kyle Temkin
Signed-off-by: Kyle Temkin
Signed-off-by: Chris Patterson
---
changes since rfc:
- documentation, formatting & code style cleanup
- drop tegra_init changes (folded into patch 4)
---
xen/arch/arm/platforms/Makefile| 2 +
xen/arch/arm/platforms/tegra-mlic.c| 261 +
xen/arch/arm/platforms/tegra.c | 13 ++
xen/include/asm-arm/platforms/tegra-mlic.h | 34
4 files changed, 310 insertions(+)
create mode 100644 xen/arch/arm/platforms/tegra-mlic.c
create mode 100644 xen/include/asm-arm/platforms/tegra-mlic.h
diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
index d7033d2..5701e62 100644
--- a/xen/arch/arm/platforms/Makefile
+++ b/xen/arch/arm/platforms/Makefile
@@ -7,6 +7,8 @@ obj-$(CONFIG_ARM_32) += rcar2.o
obj-$(CONFIG_ARM_64) += seattle.o
obj-$(CONFIG_ARM_32) += sunxi.o
obj-$(CONFIG_ARM_32) += tegra.o
+obj-$(CONFIG_ARM_32) += tegra-mlic.o
obj-$(CONFIG_ARM_64) += tegra.o
+obj-$(CONFIG_ARM_64) += tegra-mlic.o
obj-$(CONFIG_ARM_64) += xgene-storm.o
obj-$(CONFIG_ARM_64) += xilinx-zynqmp.o
diff --git a/xen/arch/arm/platforms/tegra-mlic.c
b/xen/arch/arm/platforms/tegra-mlic.c
new file mode 100644
index 000..ad77766
--- /dev/null
+++ b/xen/arch/arm/platforms/tegra-mlic.c
@@ -0,0 +1,260 @@
+/*
+ * xen/arch/arm/tegra_mlic.c
+ *
+ * Mediator for Tegra Legacy Interrupt Controller
+ *
+ * This module allow the hardware domain to have access to the sections of
+ * the legacy interrupt controller that correspond to its devices,
+ * but disallow access to the sections controlled by other domains
+ * or by Xen.
+ *
+ * Kyle Temkin; Copyright (c) 2016 Assured Information Security, Inc.
+ * Chris Patterson; Copyright (c) 2016 Assured Information Security, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+static int tegra_mlic_mmio_read(struct vcpu *v, mmio_info_t *info,
+ register_t *r, void *priv);
+static int tegra_mlic_mmio_write(struct vcpu *v, mmio_info_t *info,
+register_t r, void *priv);
+
+static const struct mmio_handler_ops tegra_mlic_mmio_handler = {
+.read = tegra_mlic_mmio_read,
+.write = tegra_mlic_mmio_write,
+};
+
+/*
+ * Parses a LIC MMIO read or write, and extracts the information needed to
+ * complete the request.
+ *
+ * info: Information describing the MMIO read/write being performed
+ * ictlr_index: The interrupt controller number in the ictlr (e.g. 0-5)
+ * register_offset: The register offset into the specified interrupt controller
+ *(e.g. TEGRA_ICTLR_CPU_IER_SET)
+ * irq_base: The number of the first IRQ represented by the given ictlr.
+ */
+static void tegra_mlic_parse_mmio_request(mmio_info_t *info,
+uint32_t *ictlr_index, uint32_t *register_offset, uint32_t *irq_base)
+{
+/* Determine the offset of the access into the ICTLR region. */
+uint32_t offset = info->gpa - TEGRA_ICTLR_BASE;
+uint32_t ictlr = offset / TEGRA_ICTLR_SIZE;
+uint32_t reg = offset % TEGRA_ICTLR_SIZE;
+
+if ( ictlr_index )
+*ictlr_index = ictlr;
+
+if ( register_offset )
+*register_offset = reg;
+
+if ( irq_base )
+*irq_base = (ictlr * TEGRA_IRQS_PER_ICTLR) + NR_LOCAL_IRQS;
+
+/* Ensure that we've only been handed a valid offset within our region. */
+BUG_ON(ictlr >= TEGRA_ICTLR_COUNT);
+BUG_ON(offset >= (TEGRA_ICTLR_COUNT * TEGRA_ICTLR_SIZE));
+BUG_ON((ictlr * TEGRA_ICTLR_SIZE + reg) != offset);
+}
+
+/*
+ * Returns true iff th
Re: [Xen-devel] [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
>> +static const char * const tegra_dt_compat[] __initconst =
>> +{
>> +"nvidia,tegra120", /* Tegra K1 */
>
> This is still tegra120 (not tegra124), is that intended? If so, it is
> still missing from arch/arm*/boot/dts. Do you have a pointer?
It was not intended; thank you for catching it. I must have lost that
fixup somewhere along the way...
> Also, do we need both tegra_dt_compat and tegra_interrupt_compat? Can we
> keep only one?
The purpose of tegra_interrupt_compat is to maintain a tegra-specific
whitelist of interrupt controllers we know how to route. Presumably,
there may be custom boards out there that may have additional
interrupt routing capabilities that this patch set would not support
as-is. I'm not sure of an appropriate way to maintain that logic and
merge them. However, I am certainly open to suggestion, if you have
any ideas.
Thanks for the review!
-Chris
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/6] xen/arm: platforms: Add earlyprintk and serial support for Tegra boards.
Will split patches & fix for v2, thanks! ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
On Fri, Jul 7, 2017 at 2:53 PM, Chris Patterson wrote: > On Fri, Jul 7, 2017 at 12:30 PM, Julien Grall wrote: >> Hi Chris, >> >> On 07/07/17 00:12, Chris Patterson wrote: >>>> >>>> >>>> So why do you want the hardware domain to interact with the ictlr? Could >>>> not >>>> you hide it completely? >>>> >>> >>> snip >>> >>>> What would happen if you enable the interrupt here for the guest? Should >>>> not >>>> you do it when the guest is requesting to enable (see vgic_enable_irqs). >>>> >>>> >>>> Also, how about EOI an interrupt? >>> >>> >>> We could possibly hide the legacy controller, but that has its own >>> challenges. Notably, the LIC allows configuration for forwarding FIQ >>> vs IRQ, and setting wake-up sources. >> >> >> FIQ are not supported for domain. So I am not sure why you would want a >> guest to configure that. >> > > Fair point, I did not know that and didn't want to assume there was > not a case otherwise... > >> Furthermore, could you explain what is wake-up sources and why a guest would >> need it? >> > > I would expect any driver using irq_set_irq_wake()? A little more > background from the reference manual > (http://developer.nvidia.com/embedded/dlc/tegra-x1-technical-reference-manual): > > " > The Legacy Interrupt Controller (LIC) is primarily used for BPMP > (ARM7). But it is also used for generating interrupts as wake > events for CPUs. This is an important use case when the core is in > retention. All of the device hardware interrupt signals are > sent to the LIC first, which routes them to the ARM7 BPMP-Lite as well > as forwards them to the GIC. The LIC also provides a > software set/clear mechanism for all of the interrupts." > > ... > > 3.3.1.5 Interrupt Blocking to Support Retention > > The Tegra X1 device implements blocking of interrupts routed to the > GIC, which supports the CPU retention state. The block > implementing this feature is represented in Figure 4 as Blocking. > > To support retention, the LIC contains a one shot disable for all the > interrupts. When the system goes into retention, BPMP > software sets this bit to disable the interrupts. > > The Flow Controller watches all the interrupts triggered and triggers > the BPMP to bring the core out of retention when any > interrupt bit is asserted. Once the system is out of retention, the > BLOCK _CCPLEX_GIC_INTR bit is cleared, and the interrupt is > serviced by the GIC. > " > > Anything using GPIO to wake (e.g. wifi?) is routed through the LIC. I > did not find anything obvious with a quick scan, but perhaps other > devices directly wired to the LIC may require it as well. > > Thoughts? Hey Julien. Just a quick ping. I was going to submit an updated patch series, just wanted to see if you had any further thoughts or desired changes with regards to the above. Thanks! ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
>> The purpose of tegra_interrupt_compat is to maintain a tegra-specific >> whitelist of interrupt controllers we know how to route. Presumably, >> there may be custom boards out there that may have additional >> interrupt routing capabilities that this patch set would not support >> as-is. I'm not sure of an appropriate way to maintain that logic and >> merge them. However, I am certainly open to suggestion, if you have >> any ideas. > > > Those custom boards would likely have a different machine compatible string > because the SOC would be different. So I think the tegra_interrupt_compat is > not necessary. > Sorry about the delayed response... This check effectively filters out GPIO controllers. Any GPIO-sourced interrupts route through the GPIO controller's interrupt, and need not apply for map_irq_to_domain(). Devices requiring GPIO-based interrupts would need to be passed through with the associated controller (or maybe virtualize the GPIO routing?). ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
> > So why do you want the hardware domain to interact with the ictlr? Could not > you hide it completely? > snip > What would happen if you enable the interrupt here for the guest? Should not > you do it when the guest is requesting to enable (see vgic_enable_irqs). > > > Also, how about EOI an interrupt? We could possibly hide the legacy controller, but that has its own challenges. Notably, the LIC allows configuration for forwarding FIQ vs IRQ, and setting wake-up sources. If we accept limitations to those configurations, we could possibly hide it entirely (or just for non-Dom0 guests with device passthrough?), and then I think we would need platform hooks for accomplishing the masking/unmasking/eoi alongside the vgic. If we don't want to limit the configuration options - we would need to surface the mediated LIC in the device trees for guests with device pass-through. > Will incorporate rest of review into next version, thanks! ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 6/6] xen/arm: platforms/tegra: Ensure the hwdom can only affect its own interrupts
Will fix, thanks! ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
On Fri, Jul 7, 2017 at 12:25 PM, Julien Grall wrote:
> Hi Chris,
>
>
> On 06/07/17 23:00, Chris Patterson wrote:
>>>>
>>>> The purpose of tegra_interrupt_compat is to maintain a tegra-specific
>>>> whitelist of interrupt controllers we know how to route. Presumably,
>>>> there may be custom boards out there that may have additional
>>>> interrupt routing capabilities that this patch set would not support
>>>> as-is. I'm not sure of an appropriate way to maintain that logic and
>>>> merge them. However, I am certainly open to suggestion, if you have
>>>> any ideas.
>>>
>>>
>>>
>>> Those custom boards would likely have a different machine compatible
>>> string
>>> because the SOC would be different. So I think the tegra_interrupt_compat
>>> is
>>> not necessary.
>>>
>>
>> Sorry about the delayed response...
>>
>> This check effectively filters out GPIO controllers. Any GPIO-sourced
>> interrupts route through the GPIO controller's interrupt, and need not
>> apply for map_irq_to_domain(). Devices requiring GPIO-based
>> interrupts would need to be passed through with the associated
>> controller (or maybe virtualize the GPIO routing?).
>
>
> I am a bit confused. Could you give a concrete example based on tegra
> platform?
>
I'm thinking of devices where interrupt-parent is &gpio (as opposed to
&lic or &gic). Here is an example on tegra-smaug:
wifi: wifi_bcm4354 {
compatible = "bcm,bcm4354";
interrupt-parent = <&gpio>;
interrupts = ;
wl_reg_on = <&gpio TEGRA_GPIO(H, 1) GPIO_ACTIVE_HIGH>;
wl_host_wake = <&gpio TEGRA_GPIO(H, 2) IRQ_TYPE_LEVEL_HIGH>;
nvidia,pmc-wakeup = <&pmc
PMC_WAKE_TYPE_EVENT 8 PMC_TRIGGER_TYPE_HIGH>;
};
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 4/6] xen/arm: platforms: Add Tegra platform to support basic IRQ routing
On Fri, Jul 7, 2017 at 12:30 PM, Julien Grall wrote: > Hi Chris, > > On 07/07/17 00:12, Chris Patterson wrote: >>> >>> >>> So why do you want the hardware domain to interact with the ictlr? Could >>> not >>> you hide it completely? >>> >> >> snip >> >>> What would happen if you enable the interrupt here for the guest? Should >>> not >>> you do it when the guest is requesting to enable (see vgic_enable_irqs). >>> >>> >>> Also, how about EOI an interrupt? >> >> >> We could possibly hide the legacy controller, but that has its own >> challenges. Notably, the LIC allows configuration for forwarding FIQ >> vs IRQ, and setting wake-up sources. > > > FIQ are not supported for domain. So I am not sure why you would want a > guest to configure that. > Fair point, I did not know that and didn't want to assume there was not a case otherwise... > Furthermore, could you explain what is wake-up sources and why a guest would > need it? > I would expect any driver using irq_set_irq_wake()? A little more background from the reference manual (http://developer.nvidia.com/embedded/dlc/tegra-x1-technical-reference-manual): " The Legacy Interrupt Controller (LIC) is primarily used for BPMP (ARM7). But it is also used for generating interrupts as wake events for CPUs. This is an important use case when the core is in retention. All of the device hardware interrupt signals are sent to the LIC first, which routes them to the ARM7 BPMP-Lite as well as forwards them to the GIC. The LIC also provides a software set/clear mechanism for all of the interrupts." ... 3.3.1.5 Interrupt Blocking to Support Retention The Tegra X1 device implements blocking of interrupts routed to the GIC, which supports the CPU retention state. The block implementing this feature is represented in Figure 4 as Blocking. To support retention, the LIC contains a one shot disable for all the interrupts. When the system goes into retention, BPMP software sets this bit to disable the interrupts. The Flow Controller watches all the interrupts triggered and triggers the BPMP to bring the core out of retention when any interrupt bit is asserted. Once the system is out of retention, the BLOCK _CCPLEX_GIC_INTR bit is cleared, and the interrupt is serviced by the GIC. " Anything using GPIO to wake (e.g. wifi?) is routed through the LIC. I did not find anything obvious with a quick scan, but perhaps other devices directly wired to the LIC may require it as well. Thoughts? ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
