Re: possible deprecation and removal of some old QEMU Arm machine types (pxa2xx, omap, sa1110)
On Tue, Feb 13, 2024 at 9:12 PM Arnd Bergmann wrote: > On Tue, Feb 13, 2024, at 16:36, Guenter Roeck wrote: > > On Tue, Feb 13, 2024 at 03:14:21PM +, Peter Maydell wrote: > >> On Mon, 12 Feb 2024 at 14:36, Guenter Roeck wrote: > >> > On 2/12/24 04:32, Peter Maydell wrote: > >> > > The one SA1110 machine: > >> > > > >> > > collie Sharp SL-5500 (Collie) PDA (SA-1110) > >> > > > >> > I do test collie. > > Adding Linus Walleij and Stefan Lehner to Cc, as they were > interested in modernizing sa1100 back in 2022. If they > are still interested in that, they might want to keep collie > support. I'm not personally interested in the Collie, I have a SA1100 hardware but not that one. > Surprisingly, at the time I removed unused old board files, > there was a lot more interest in sa1100 than in the newer > pxa platform, which I guess wasn't as appealing for > retrocomputing yet. Andrea Adami and Dmitry Eremin-Solenikov did the work in 2017 to modernize it a bit, and Russell helped out. I was under the impression that they only used real hardware though! The Collie is popular because it is/was easy to get hold of and easy to hack. PXA was in candybar phones (right?) which are just veritable fortresses and really hard to hack so that is why there is no interest (except for the occasional hyperfocused Harald Welte), so those are a bit like the iPhones: you *can* boot something custom on them, but it won't be easy or quick, and not as fun and rewarding. The thriving world of PostmarketOS only exist because Google was clever to realize devices should have a developer mode. Yours, Linus Walleij
Re: [PATCH v4] fcntl: Add 32bit filesystem mode
On Thu, May 19, 2022 at 4:23 PM Icenowy Zheng wrote: > 在 2020-11-18星期三的 00:39 +0100，Linus Walleij写道： > > It was brought to my attention that this bug from 2018 was > > still unresolved: 32 bit emulators like QEMU were given > > 64 bit hashes when running 32 bit emulation on 64 bit systems. > > Sorry for replying such an old mail, but I found that using 32-bit file > syscalls in 32-bit QEMU user on 64-bit hosts are still broken today, > and google sent me here. Yeah the bug was 2 years old when I started patching it and now it is 4 years old... > This mail does not get any reply according to linux-ext4 patchwork, so > could I ping it? I suppose, I think the patch is authored according to the maintainer requirements, but I'm happy to revise and resend it if it no longer applies. Arnd and others suggested to maybe use F_SETFL instead: https://lore.kernel.org/linux-fsdevel/CAK8P3a2SN2zeK=dj01Br-m86rJmK8mOyH=ghaidwspgkaet...@mail.gmail.com/ I am happy to do it either way by need to have some input from the maintainer (Ted). Maybe someone else on the fsdevel list want to chime in? Maybe any FS maintainer can actually apply this? Yours, Linus Walleij
[PATCH v4] fcntl: Add 32bit filesystem mode
It was brought to my attention that this bug from 2018 was
still unresolved: 32 bit emulators like QEMU were given
64 bit hashes when running 32 bit emulation on 64 bit systems.
This adds a flag to the fcntl() F_GETFD and F_SETFD operations
to set the underlying filesystem into 32bit mode even if the
file handle was opened using 64bit mode without the compat
syscalls.
Programs that need the 32 bit file system behavior need to
issue a fcntl() system call such as in this example:
#define FD_32BIT_MODE 2
int main(int argc, char** argv) {
DIR* dir;
int err;
int mode;
int fd;
dir = opendir("/boot");
fd = dirfd(dir);
mode = fcntl(fd, F_GETFD);
mode |= FD_32BIT_MODE;
err = fcntl(fd, F_SETFD, mode);
if (err) {
printf("fcntl() failed! err=%d\n", err);
return 1;
}
printf("dir=%p\n", dir);
printf("readdir(dir)=%p\n", readdir(dir));
printf("errno=%d: %s\n", errno, strerror(errno));
return 0;
}
This can be pretty hard to test since C libraries and linux
userspace security extensions aggressively filter the parameters
that are passed down and allowed to commit into actual system
calls.
Cc: Florian Weimer
Cc: Peter Maydell
Cc: Andy Lutomirski
Cc: Eric Blake
Reported-by: 罗勇刚(Yonggang Luo)
Suggested-by: Theodore Ts'o
Link: https://bugs.launchpad.net/qemu/+bug/1805913
Link: https://lore.kernel.org/lkml/87bm56vqg4@mid.deneb.enyo.de/
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=205957
Signed-off-by: Linus Walleij
---
ChangeLog v3 RESEND 1-> v4:
- Update the example in the commit message to a read/modify/write
version.
- Notice that Yonggang Luo sees the sema problem on i386 binaries
as we see on ARM 32bit binaries.
ChangeLog v3->v3 RESEND 1:
- Resending during the v5.10 merge window to get attention.
ChangeLog v2->v3:
- Realized that I also have to clear the flag correspondingly
if someone ask for !FD_32BIT_MODE after setting it the
first time.
ChangeLog v1->v2:
- Use a new flag FD_32BIT_MODE to F_GETFD and F_SETFD
instead of a new fcntl operation, there is already a fcntl
operation to set random flags.
- Sorry for taking forever to respin this patch :(
---
fs/fcntl.c | 7 +++
include/uapi/asm-generic/fcntl.h | 8
2 files changed, 15 insertions(+)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 19ac5baad50f..6c32edc4099a 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -335,10 +335,17 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned
long arg,
break;
case F_GETFD:
err = get_close_on_exec(fd) ? FD_CLOEXEC : 0;
+ /* Report 32bit file system mode */
+ if (filp->f_mode & FMODE_32BITHASH)
+ err |= FD_32BIT_MODE;
break;
case F_SETFD:
err = 0;
set_close_on_exec(fd, arg & FD_CLOEXEC);
+ if (arg & FD_32BIT_MODE)
+ filp->f_mode |= FMODE_32BITHASH;
+ else
+ filp->f_mode &= ~FMODE_32BITHASH;
break;
case F_GETFL:
err = filp->f_flags;
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 9dc0bf0c5a6e..edd3573cb7ef 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -160,6 +160,14 @@ struct f_owner_ex {
/* for F_[GET|SET]FL */
#define FD_CLOEXEC 1 /* actually anything with low bit set goes */
+/*
+ * This instructs the kernel to provide 32bit semantics (such as hashes) from
+ * the file system layer, when running a userland that depend on 32bit
+ * semantics on a kernel that supports 64bit userland, but does not use the
+ * compat ioctl() for e.g. open(), so that the kernel would otherwise assume
+ * that the userland process is capable of dealing with 64bit semantics.
+ */
+#define FD_32BIT_MODE 2
/* for posix fcntl() and lockf() */
#ifndef F_RDLCK
--
2.26.2
Re: [PATCH v3 RESEND] fcntl: Add 32bit filesystem mode
On Tue, Oct 13, 2020 at 11:22 AM Dave Martin wrote: > > case F_SETFD: > > err = 0; > > set_close_on_exec(fd, arg & FD_CLOEXEC); > > + if (arg & FD_32BIT_MODE) > > + filp->f_mode |= FMODE_32BITHASH; > > + else > > + filp->f_mode &= ~FMODE_32BITHASH; > > This seems inconsistent? F_SETFD is for setting flags on a file > descriptor. Won't setting a flag on filp here instead cause the > behaviour to change for all file descriptors across the system that are > open on this struct file? Compare set_close_on_exec(). > > I don't see any discussion on whether this should be an F_SETFL or an > F_SETFD, though I see F_SETFD was Ted's suggestion originally. I cannot honestly say I know the semantic difference. I would ask the QEMU people how a user program would expect the flag to behave. Yours, Linus Walleij
[PATCH v3 RESEND] fcntl: Add 32bit filesystem mode
It was brought to my attention that this bug from 2018 was
still unresolved: 32 bit emulators like QEMU were given
64 bit hashes when running 32 bit emulation on 64 bit systems.
This adds a flag to the fcntl() F_GETFD and F_SETFD operations
to set the underlying filesystem into 32bit mode even if the
file handle was opened using 64bit mode without the compat
syscalls.
Programs that need the 32 bit file system behavior need to
issue a fcntl() system call such as in this example:
#define FD_32BIT_MODE 2
int main(int argc, char** argv) {
DIR* dir;
int err;
int fd;
dir = opendir("/boot");
fd = dirfd(dir);
err = fcntl(fd, F_SETFD, FD_32BIT_MODE);
if (err) {
printf("fcntl() failed! err=%d\n", err);
return 1;
}
printf("dir=%p\n", dir);
printf("readdir(dir)=%p\n", readdir(dir));
printf("errno=%d: %s\n", errno, strerror(errno));
return 0;
}
This can be pretty hard to test since C libraries and linux
userspace security extensions aggressively filter the parameters
that are passed down and allowed to commit into actual system
calls.
Cc: Florian Weimer
Cc: Peter Maydell
Cc: Andy Lutomirski
Suggested-by: Theodore Ts'o
Link: https://bugs.launchpad.net/qemu/+bug/1805913
Link: https://lore.kernel.org/lkml/87bm56vqg4@mid.deneb.enyo.de/
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=205957
Signed-off-by: Linus Walleij
---
ChangeLog v3->v3 RESEND 1:
- Resending during the v5.10 merge window to get attention.
ChangeLog v2->v3:
- Realized that I also have to clear the flag correspondingly
if someone ask for !FD_32BIT_MODE after setting it the
first time.
ChangeLog v1->v2:
- Use a new flag FD_32BIT_MODE to F_GETFD and F_SETFD
instead of a new fcntl operation, there is already a fcntl
operation to set random flags.
- Sorry for taking forever to respin this patch :(
---
fs/fcntl.c | 7 +++
include/uapi/asm-generic/fcntl.h | 8
2 files changed, 15 insertions(+)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 19ac5baad50f..6c32edc4099a 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -335,10 +335,17 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned
long arg,
break;
case F_GETFD:
err = get_close_on_exec(fd) ? FD_CLOEXEC : 0;
+ /* Report 32bit file system mode */
+ if (filp->f_mode & FMODE_32BITHASH)
+ err |= FD_32BIT_MODE;
break;
case F_SETFD:
err = 0;
set_close_on_exec(fd, arg & FD_CLOEXEC);
+ if (arg & FD_32BIT_MODE)
+ filp->f_mode |= FMODE_32BITHASH;
+ else
+ filp->f_mode &= ~FMODE_32BITHASH;
break;
case F_GETFL:
err = filp->f_flags;
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 9dc0bf0c5a6e..edd3573cb7ef 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -160,6 +160,14 @@ struct f_owner_ex {
/* for F_[GET|SET]FL */
#define FD_CLOEXEC 1 /* actually anything with low bit set goes */
+/*
+ * This instructs the kernel to provide 32bit semantics (such as hashes) from
+ * the file system layer, when running a userland that depend on 32bit
+ * semantics on a kernel that supports 64bit userland, but does not use the
+ * compat ioctl() for e.g. open(), so that the kernel would otherwise assume
+ * that the userland process is capable of dealing with 64bit semantics.
+ */
+#define FD_32BIT_MODE 2
/* for posix fcntl() and lockf() */
#ifndef F_RDLCK
--
2.26.2
[PATCH v3] fcntl: Add 32bit filesystem mode
It was brought to my attention that this bug from 2018 was
still unresolved: 32 bit emulators like QEMU were given
64 bit hashes when running 32 bit emulation on 64 bit systems.
This adds a flag to the fcntl() F_GETFD and F_SETFD operations
to set the underlying filesystem into 32bit mode even if the
file handle was opened using 64bit mode without the compat
syscalls.
Programs that need the 32 bit file system behavior need to
issue a fcntl() system call such as in this example:
#define FD_32BIT_MODE 2
int main(int argc, char** argv) {
DIR* dir;
int err;
int fd;
dir = opendir("/boot");
fd = dirfd(dir);
err = fcntl(fd, F_SETFD, FD_32BIT_MODE);
if (err) {
printf("fcntl() failed! err=%d\n", err);
return 1;
}
printf("dir=%p\n", dir);
printf("readdir(dir)=%p\n", readdir(dir));
printf("errno=%d: %s\n", errno, strerror(errno));
return 0;
}
This can be pretty hard to test since C libraries and linux
userspace security extensions aggressively filter the parameters
that are passed down and allowed to commit into actual system
calls.
Cc: Florian Weimer
Cc: Peter Maydell
Cc: Andy Lutomirski
Suggested-by: Theodore Ts'o
Link: https://bugs.launchpad.net/qemu/+bug/1805913
Link: https://lore.kernel.org/lkml/87bm56vqg4@mid.deneb.enyo.de/
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=205957
Signed-off-by: Linus Walleij
---
ChangeLog v2->v3:
- Realized that I also have to clear the flag correspondingly
if someone ask for !FD_32BIT_MODE after setting it the
first time.
ChangeLog v1->v2:
- Use a new flag FD_32BIT_MODE to F_GETFD and F_SETFD
instead of a new fcntl operation, there is already a fcntl
operation to set random flags.
- Sorry for taking forever to respin this patch :(
---
fs/fcntl.c | 7 +++
include/uapi/asm-generic/fcntl.h | 8
2 files changed, 15 insertions(+)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 2e4c0fa2074b..a937be835924 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -335,10 +335,17 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned
long arg,
break;
case F_GETFD:
err = get_close_on_exec(fd) ? FD_CLOEXEC : 0;
+ /* Report 32bit file system mode */
+ if (filp->f_mode & FMODE_32BITHASH)
+ err |= FD_32BIT_MODE;
break;
case F_SETFD:
err = 0;
set_close_on_exec(fd, arg & FD_CLOEXEC);
+ if (arg & FD_32BIT_MODE)
+ filp->f_mode |= FMODE_32BITHASH;
+ else
+ filp->f_mode &= ~FMODE_32BITHASH;
break;
case F_GETFL:
err = filp->f_flags;
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 9dc0bf0c5a6e..edd3573cb7ef 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -160,6 +160,14 @@ struct f_owner_ex {
/* for F_[GET|SET]FL */
#define FD_CLOEXEC 1 /* actually anything with low bit set goes */
+/*
+ * This instructs the kernel to provide 32bit semantics (such as hashes) from
+ * the file system layer, when running a userland that depend on 32bit
+ * semantics on a kernel that supports 64bit userland, but does not use the
+ * compat ioctl() for e.g. open(), so that the kernel would otherwise assume
+ * that the userland process is capable of dealing with 64bit semantics.
+ */
+#define FD_32BIT_MODE 2
/* for posix fcntl() and lockf() */
#ifndef F_RDLCK
--
2.26.2
Re: [PATCH v2] fcntl: Add 32bit filesystem mode
On Sun, Jul 19, 2020 at 2:34 PM Linus Walleij wrote: > On Mon, Jul 6, 2020 at 10:54 AM Linus Walleij > wrote: > > > Ted, can you merge this patch? > > > > It seems QEMU is happy and AFICT it uses the approach you want :) > > Gentle ping! Special merge-window ping. Shall I resend the patch? Yours, Linus Walleij
Re: [PATCH v2] fcntl: Add 32bit filesystem mode
On Mon, Jul 6, 2020 at 10:54 AM Linus Walleij wrote: > Ted, can you merge this patch? > > It seems QEMU is happy and AFICT it uses the approach you want :) Gentle ping! Yours, Linus Walleij
Re: [PATCH v2] fcntl: Add 32bit filesystem mode
On Tue, Jun 23, 2020 at 12:08 PM Peter Maydell wrote: > On Fri, 29 May 2020 at 08:22, Linus Walleij wrote: > > > > It was brought to my attention that this bug from 2018 was > > still unresolved: 32 bit emulators like QEMU were given > > 64 bit hashes when running 32 bit emulation on 64 bit systems. > > > > This adds a flag to the fcntl() F_GETFD and F_SETFD operations > > to set the underlying filesystem into 32bit mode even if the > > file handle was opened using 64bit mode without the compat > > syscalls. > > I somewhat belatedly got round to updating my QEMU patch > that uses this new fcntl() flag to fix the bug. Sorry for > the delay getting round to this. You can find the QEMU patch here: > https://patchew.org/QEMU/20200623100101.6041-1-peter.mayd...@linaro.org/ > (it's an RFC because obviously we won't put it into QEMU until > the kernel side has gone upstream and the API is final.) > > What's the next step for moving this forward? Ted, can you merge this patch? It seems QEMU is happy and AFICT it uses the approach you want :) Yours, Linus Walleij
[PATCH v2] fcntl: Add 32bit filesystem mode
It was brought to my attention that this bug from 2018 was
still unresolved: 32 bit emulators like QEMU were given
64 bit hashes when running 32 bit emulation on 64 bit systems.
This adds a flag to the fcntl() F_GETFD and F_SETFD operations
to set the underlying filesystem into 32bit mode even if the
file handle was opened using 64bit mode without the compat
syscalls.
Programs that need the 32 bit file system behavior need to
issue a fcntl() system call such as in this example:
#define FD_32BIT_MODE 2
int main(int argc, char** argv) {
DIR* dir;
int err;
int fd;
dir = opendir("/boot");
fd = dirfd(dir);
err = fcntl(fd, F_SETFD, FD_32BIT_MODE);
if (err) {
printf("fcntl() failed! err=%d\n", err);
return 1;
}
printf("dir=%p\n", dir);
printf("readdir(dir)=%p\n", readdir(dir));
printf("errno=%d: %s\n", errno, strerror(errno));
return 0;
}
This can be pretty hard to test since C libraries and linux
userspace security extensions aggressively filter the parameters
that are passed down and allowed to commit into actual system
calls.
Cc: Florian Weimer
Cc: Peter Maydell
Cc: Andy Lutomirski
Suggested-by: Theodore Ts'o
Link: https://bugs.launchpad.net/qemu/+bug/1805913
Link: https://lore.kernel.org/lkml/87bm56vqg4@mid.deneb.enyo.de/
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=205957
Signed-off-by: Linus Walleij
---
ChangeLog v1->v2:
- Use a new flag FD_32BIT_MODE to F_GETFD and F_SETFD
instead of a new fcntl operation, there is already a fcntl
operation to set random flags.
- Sorry for taking forever to respin this patch :(
---
fs/fcntl.c | 5 +
include/uapi/asm-generic/fcntl.h | 8
2 files changed, 13 insertions(+)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 2e4c0fa2074b..10a6b712d3a7 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -335,10 +335,15 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned
long arg,
break;
case F_GETFD:
err = get_close_on_exec(fd) ? FD_CLOEXEC : 0;
+ /* Report 32bit file system mode */
+ if (filp->f_mode & FMODE_32BITHASH)
+ err |= FD_32BIT_MODE;
break;
case F_SETFD:
err = 0;
set_close_on_exec(fd, arg & FD_CLOEXEC);
+ if (arg & FD_32BIT_MODE)
+ filp->f_mode |= FMODE_32BITHASH;
break;
case F_GETFL:
err = filp->f_flags;
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 9dc0bf0c5a6e..edd3573cb7ef 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -160,6 +160,14 @@ struct f_owner_ex {
/* for F_[GET|SET]FL */
#define FD_CLOEXEC 1 /* actually anything with low bit set goes */
+/*
+ * This instructs the kernel to provide 32bit semantics (such as hashes) from
+ * the file system layer, when running a userland that depend on 32bit
+ * semantics on a kernel that supports 64bit userland, but does not use the
+ * compat ioctl() for e.g. open(), so that the kernel would otherwise assume
+ * that the userland process is capable of dealing with 64bit semantics.
+ */
+#define FD_32BIT_MODE 2
/* for posix fcntl() and lockf() */
#ifndef F_RDLCK
--
2.25.4
Re: [PATCH v7 3/6] gpiolib: Add support for GPIO lookup by line name
On Mon, May 11, 2020 at 4:53 PM Geert Uytterhoeven
wrote:
> Currently a GPIO lookup table can only refer to a specific GPIO by a
> tuple, consisting of a GPIO controller label and a GPIO offset inside
> the controller.
>
> However, a GPIO may also carry a line name, defined by DT or ACPI.
> If present, the line name is the most use-centric way to refer to a
> GPIO. Hence add support for looking up GPIOs by line name.
> Note that there is no guarantee that GPIO line names are globally
> unique, so this will use the first match found.
>
> Implement this by reusing the existing gpiod_lookup infrastructure.
> Rename gpiod_lookup.chip_label to gpiod_lookup.key, to make it clear
> that this field can have two meanings, and update the kerneldoc and
> GPIO_LOOKUP*() macros.
>
> Signed-off-by: Geert Uytterhoeven
> Reviewed-by: Ulrich Hecht
> Reviewed-by: Eugeniu Rosca
> Tested-by: Eugeniu Rosca
> ---
> v7:
> - Document non-uniqueness of line names,
> - Rebase on top of commit a0b66a73785ccc8f ("gpio: Rename variable in
This is likely the most controversial patch of this series but since
noone seems to be especially upset, I think I just accept this
heuristic.
It is pretty clearly cut I think, and fits very well with the
aggregator use case, which is an important one.
Yours,
Linus Walleij
Re: [PATCH v7 0/6] gpio: Add GPIO Aggregator
Hi Geert, I have queued this v7 patch set in an immutable branch for testing and also merged to my "devel" branch for testing. If all goes well it also hits linux-next soon. Yours, Linus Walleij
[PATCH] fcntl: Add 32bit filesystem mode
It was brought to my attention that this bug from 2018 was
still unresolved: 32 bit emulators like QEMU were given
64 bit hashes when running 32 bit emulation on 64 bit systems.
This adds a fcntl() operation to set the underlying filesystem
into 32bit mode even if the file hanle was opened using 64bit
mode without the compat syscalls.
Programs that need the 32 bit file system behavior need to
issue a fcntl() system call such as in this example:
#define F_SET_FILE_32BIT_FS (1024 + 15)
int main(int argc, char** argv) {
DIR* dir;
int err;
int fd;
dir = opendir("/boot");
fd = dirfd(dir);
err = fcntl(fd, F_SET_FILE_32BIT_FS);
if (err) {
printf("fcntl() failed! err=%d\n", err);
return 1;
}
printf("dir=%p\n", dir);
printf("readdir(dir)=%p\n", readdir(dir));
printf("errno=%d: %s\n", errno, strerror(errno));
return 0;
}
This can be pretty hard to test since C libraries and linux
userspace security extensions aggressively filter the parameters
that are passed down and allowed to commit into actual system
calls.
Cc: Florian Weimer
Cc: Peter Maydell
Cc: Andy Lutomirski
Suggested-by: Theodore Ts'o
Link: https://bugs.launchpad.net/qemu/+bug/1805913
Link: https://lore.kernel.org/lkml/87bm56vqg4@mid.deneb.enyo.de/
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=205957
Signed-off-by: Linus Walleij
---
fs/fcntl.c | 4
include/uapi/linux/fcntl.h | 9 +
tools/include/uapi/linux/fcntl.h | 9 +
tools/perf/trace/beauty/fcntl.c | 3 ++-
4 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 2e4c0fa2074b..d194b1265bd4 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -426,6 +426,10 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned
long arg,
case F_SET_FILE_RW_HINT:
err = fcntl_rw_hint(filp, cmd, arg);
break;
+ case F_SET_FILE_32BIT_FS:
+ filp->f_mode |= FMODE_32BITHASH;
+ err = 0;
+ break;
default:
break;
}
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index ca88b7bce553..b9ad934147e8 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -73,6 +73,15 @@
*/
#define RWF_WRITE_LIFE_NOT_SET RWH_WRITE_LIFE_NOT_SET
+/*
+ * This instructs the kernel to provide 32bit semantics (such as hashes) from
+ * the file system layer, when running a userland that depend on 32bit
+ * semantics on a kernel that supports 64bit userland, but does not use the
+ * compat ioctl() for e.g. open(), so that the kernel would otherwise assume
+ * that the userland process is capable of dealing with 64bit semantics.
+ */
+#define F_SET_FILE_32BIT_FS(F_LINUX_SPECIFIC_BASE + 15)
+
/*
* Types of directory notifications that may be requested.
*/
diff --git a/tools/include/uapi/linux/fcntl.h b/tools/include/uapi/linux/fcntl.h
index ca88b7bce553..b9ad934147e8 100644
--- a/tools/include/uapi/linux/fcntl.h
+++ b/tools/include/uapi/linux/fcntl.h
@@ -73,6 +73,15 @@
*/
#define RWF_WRITE_LIFE_NOT_SET RWH_WRITE_LIFE_NOT_SET
+/*
+ * This instructs the kernel to provide 32bit semantics (such as hashes) from
+ * the file system layer, when running a userland that depend on 32bit
+ * semantics on a kernel that supports 64bit userland, but does not use the
+ * compat ioctl() for e.g. open(), so that the kernel would otherwise assume
+ * that the userland process is capable of dealing with 64bit semantics.
+ */
+#define F_SET_FILE_32BIT_FS(F_LINUX_SPECIFIC_BASE + 15)
+
/*
* Types of directory notifications that may be requested.
*/
diff --git a/tools/perf/trace/beauty/fcntl.c b/tools/perf/trace/beauty/fcntl.c
index 56ef83b3d130..da80264678cb 100644
--- a/tools/perf/trace/beauty/fcntl.c
+++ b/tools/perf/trace/beauty/fcntl.c
@@ -94,7 +94,8 @@ size_t syscall_arg__scnprintf_fcntl_arg(char *bf, size_t
size, struct syscall_ar
cmd == F_OFD_SETLK || cmd == F_OFD_SETLKW || cmd == F_OFD_GETLK ||
cmd == F_GETOWN_EX || cmd == F_SETOWN_EX ||
cmd == F_GET_RW_HINT || cmd == F_SET_RW_HINT ||
- cmd == F_GET_FILE_RW_HINT || cmd == F_SET_FILE_RW_HINT)
+ cmd == F_GET_FILE_RW_HINT || cmd == F_SET_FILE_RW_HINT ||
+ cmd == F_SET_FILE_32BIT_FS)
return syscall_arg__scnprintf_hex(bf, size, arg);
return syscall_arg__scnprintf_long(bf, size, arg);
--
2.25.1
Re: [PATCH v6 5/8] gpiolib: Introduce gpiod_set_config()
On Tue, Mar 24, 2020 at 2:57 PM Geert Uytterhoeven wrote: > The GPIO Aggregator will need a method to forward a .set_config() call > to its parent gpiochip. This requires obtaining the gpio_chip and > offset for a given gpio_desc. While gpiod_to_chip() is public, > gpio_chip_hwgpio() is not, so there is currently no method to obtain the > needed GPIO offset parameter. > > Hence introduce a public gpiod_set_config() helper, which invokes the > .set_config() callback through a gpio_desc pointer, like is done for > most other gpio_chip callbacks. > > Rewrite the existing gpiod_set_debounce() helper as a wrapper around > gpiod_set_config(), to avoid duplication. > > Signed-off-by: Geert Uytterhoeven > --- > v6: > - New. Patch applied in preparation for the next kernel cycle so we get Geert's patch stack down. Yours, Linus Walleij
Re: [PATCH v6 5/8] gpiolib: Introduce gpiod_set_config()
On Fri, Mar 27, 2020 at 9:45 AM Geert Uytterhoeven wrote: > On Thu, Mar 26, 2020 at 10:26 PM Linus Walleij > wrote: > > On Tue, Mar 24, 2020 at 2:57 PM Geert Uytterhoeven > > wrote: > > > The GPIO Aggregator will need a method to forward a .set_config() call > > > to its parent gpiochip. This requires obtaining the gpio_chip and > > > offset for a given gpio_desc. While gpiod_to_chip() is public, > > > gpio_chip_hwgpio() is not, so there is currently no method to obtain the > > > needed GPIO offset parameter. > > > > > > Hence introduce a public gpiod_set_config() helper, which invokes the > > > .set_config() callback through a gpio_desc pointer, like is done for > > > most other gpio_chip callbacks. > > > > > > Rewrite the existing gpiod_set_debounce() helper as a wrapper around > > > gpiod_set_config(), to avoid duplication. > > > > > > Signed-off-by: Geert Uytterhoeven > > > --- > > > v6: > > > - New. > > > > This is nice, I tried to actually just apply this (you also sent some > > two cleanups that I tried to apply) byt Yue's cleanup patch > > commit d18fddff061d2796525e6d4a958cb3d30aed8efd > > "gpiolib: Remove duplicated function gpio_do_set_config()" > > makes none of them apply :/ > > /me confused. > > That commit was reverted later, so it shouldn't matter. > > I have just verified, and both my full series and just this single > patch, do apply fine to all of current gpio/for-next, linus/master, and > next-20200327. They even apply fine to gpio/for-next before or after > the two cleanups I sent, too. > > What am I missing? Ah I see, it is because my development branch is based on v5.6-rc1. So I have to merge in a later -rc where this revert is applied so that this applies. Yours, Linus Walleij
Re: [PATCH v6 5/8] gpiolib: Introduce gpiod_set_config()
On Tue, Mar 24, 2020 at 2:57 PM Geert Uytterhoeven wrote: > The GPIO Aggregator will need a method to forward a .set_config() call > to its parent gpiochip. This requires obtaining the gpio_chip and > offset for a given gpio_desc. While gpiod_to_chip() is public, > gpio_chip_hwgpio() is not, so there is currently no method to obtain the > needed GPIO offset parameter. > > Hence introduce a public gpiod_set_config() helper, which invokes the > .set_config() callback through a gpio_desc pointer, like is done for > most other gpio_chip callbacks. > > Rewrite the existing gpiod_set_debounce() helper as a wrapper around > gpiod_set_config(), to avoid duplication. > > Signed-off-by: Geert Uytterhoeven > --- > v6: > - New. This is nice, I tried to actually just apply this (you also sent some two cleanups that I tried to apply) byt Yue's cleanup patch commit d18fddff061d2796525e6d4a958cb3d30aed8efd "gpiolib: Remove duplicated function gpio_do_set_config()" makes none of them apply :/ Yours, Linus Walleij
Re: [PATCH v6 4/8] gpiolib: Add support for GPIO lookup by line name
On Tue, Mar 24, 2020 at 2:57 PM Geert Uytterhoeven wrote: > Currently a GPIO lookup table can only refer to a specific GPIO by a > tuple, consisting of a GPIO controller label and a GPIO offset inside > the controller. > > However, a GPIO may also carry a line name, defined by DT or ACPI. > If present, the line name is the most use-centric way to refer to a > GPIO. Hence add support for looking up GPIOs by line name. > > Implement this by reusing the existing gpiod_lookup infrastructure. > Rename gpiod_lookup.chip_label to gpiod_lookup.key, to make it clear > that this field can have two meanings, and update the kerneldoc and > GPIO_LOOKUP*() macros. > > Signed-off-by: Geert Uytterhoeven > Reviewed-by: Ulrich Hecht > Reviewed-by: Eugeniu Rosca > Tested-by: Eugeniu Rosca I kind of like this approach, however there are things here that need to be considered: the line name is in no way globally unique, and I think there are already quite a few GPIO chips that have the same line names assigned for every instance of that chip. gpiochip_set_desc_names() only warns if there is a line with the same name on the same gpio_chip. I suppose we need to document that the line name look-up will be on a first-come-first-served basis: whatever line we find first with this name is what you will get a reference to, no matter what chip it is on, and it is possible albeit not recommended that some other chip has a line with the same name. Yours, Linus Walleij
Re: [PATCH v6 1/8] ARM: integrator: impd1: Use GPIO_LOOKUP() helper macro
On Tue, Mar 24, 2020 at 2:57 PM Geert Uytterhoeven wrote: > impd1_probe() fills in the GPIO lookup table by manually populating an > array of gpiod_lookup structures. Use the existing GPIO_LOOKUP() helper > macro instead, to relax a dependency on the gpiod_lookup structure's > member names. > > Signed-off-by: Geert Uytterhoeven > Cc: linux-arm-ker...@lists.infradead.org > --- > While this patch is a dependency for "[PATCH v6 4/8] gpiolib: Add > support for GPIO lookup by line name", it can be applied independently. > But an Acked-by would be nice, too. I simply applied this patch for v5.7 in the GPIO tree since I am the maintainer of this platform, and I might want to change stuff around Integrator next cycle so it's good to have this covered. Yours, Linus Walleij
Re: [PATCH] ext4: Give 32bit personalities 32bit hashes
On Tue, Mar 24, 2020 at 7:48 PM Theodore Y. Ts'o wrote: > On Tue, Mar 24, 2020 at 09:29:58AM +, Peter Maydell wrote: > > > > On the contrary, that would be a much better interface for QEMU. > > We always know when we're doing an open-syscall on behalf > > of the guest, and it would be trivial to make the fcntl() call then. > > That would ensure that we don't accidentally get the > > '32-bit semantics' on file descriptors QEMU opens for its own > > purposes, and wouldn't leave us open to the risk in future that > > setting the PER_LINUX32 flag for all of QEMU causes > > unexpected extra behaviour in future kernels that would be correct > > for the guest binary but wrong/broken for QEMU's own internals. > > If using a flag set by fcntl is better for qemu, then by all means > let's go with that instead of using a personality flag/number. > > Linus, do you have what you need to do a respin of the patch? Absolutely, I'm a bit occupied this week but I will try to get to it early next week! Thanks a lot for the directions here, it's highly valuable. Yours, Linus Walleij
Re: [PATCH] ext4: Give 32bit personalities 32bit hashes
On Thu, Mar 19, 2020 at 4:25 PM Peter Maydell wrote: > On Thu, 19 Mar 2020 at 15:13, Linus Walleij wrote: > > On Tue, Mar 17, 2020 at 12:58 PM Peter Maydell > > wrote: > > > What in particular does this personality setting affect? > > > My copy of the personality(2) manpage just says: > > > > > >PER_LINUX32 (since Linux 2.2) > > > [To be documented.] > > > > > > which isn't very informative. > > > > It's not a POSIX thing (not part of the Single Unix Specification) > > so as with most Linux things it has some fuzzy semantics > > defined by the community... > > > > I usually just go to the source. > > If we're going to decide that this is the way to say > "give me 32-bit semantics" we need to actually document > that and define in at least broad terms what we mean > by it, so that when new things are added that might or > might not check against the setting there is a reference > defining whether they should or not, and so that > userspace knows what it's opting into by setting the flag. > The kernel loves undocumented APIs but userspace > consumers of them are not so enamoured :-) OK I guess we can at least take this opportunity to add some kerneldoc to the include file. > As a concrete example, should "give me 32-bit semantics > via PER_LINUX32" mean "mmap should always return addresses > within 4GB" ? That would seem like it would make sense -- Incidentally that thing in particular has its own personality flag (personalities are additive, it's a bit schizophrenic) so PER_LINUX_32BIT is defined as: PER_LINUX_32BIT = 0x | ADDR_LIMIT_32BIT, and that is specifically for limiting the address space to 32bit. There is also PER_LINUX32_3GB for a 3GB lowmem limit. Since the personality is kind of additive, if we want a flag *specifically* for indicating that we want 32bit hashes from the file system, there are bits left so we can provide that. Is this what we want to do? I just think we shouldn't decide on that lightly as we will be using up personality bug bits, but sometimes you have to use them. PER_LINUX32 as it stands means 32bit personality but very specifically does not include memory range limitations since that has its own flags. Yours, Linus Walleij
Re: [PATCH] ext4: Give 32bit personalities 32bit hashes
On Tue, Mar 17, 2020 at 11:29 PM Andreas Dilger wrote: > That said, I'd think it would be preferable for ease of use and > compatibility that applications didn't have to be modified > (e.g. have QEMU or glibc internally set PER_LINUX32 for this > process before the 32-bit syscall is called, given that it knows > whether it is emulating a 32-bit runtime or not). I think the plan is that QEMU set this, either directly when you run qemu-user or through the binfmt handler. https://www.kernel.org/doc/html/latest/admin-guide/binfmt-misc.html IIUC the binfmt handler is invoked when you try to execute ELF so-and-so-for-arch-so-and-so when you are not that arch yourself. So that can set up this personality. Not that I know how the binfmt handler works, I am just assuming that thing is some program that can issue syscalls. It JustWorks for me after installing the QEMU packages... The problem still stands that userspace need to somehow inform kernelspace that 32bit is going on, and this was the best I could think of. Yours, Linus Walleij
Re: [PATCH] ext4: Give 32bit personalities 32bit hashes
On Tue, Mar 17, 2020 at 12:58 PM Peter Maydell wrote: > On Tue, 17 Mar 2020 at 11:31, Linus Walleij wrote: > > > > It was brought to my attention that this bug from 2018 was > > still unresolved: 32 bit emulators like QEMU were given > > 64 bit hashes when running 32 bit emulation on 64 bit systems. > > > > The personality(2) system call supports to let processes > > indicate that they are 32 bit Linux to the kernel. This > > was suggested by Teo in the original thread, so I just wired > > it up and it solves the problem. > > Thanks for having a look at this. I'm not sure this is what > QEMU needs, though. When QEMU runs, it is not a 32-bit > process, it's a 64-bit process. Some of the syscalls > it makes are on behalf of the guest and would need 32-bit > semantics (including this one of wanting 32-bit hash sizes > in directory reads). But some syscalls it makes for itself > (either directly, or via libraries it's linked against > including glibc and glib) -- those would still want the > usual 64-bit semantics, I would have thought. The "personality" thing is a largely unused facility that a process can use to say it has this generic behaviour. In this case we say we have the PER_LINUX32 personality so it will make the process evoke 32bit behaviours inside the kernel when dealing with this process. Which I (loosely) appreciate that we want to achieve. > > Programs that need the 32 bit hash only need to issue the > > personality(PER_LINUX32) call and things start working. > > What in particular does this personality setting affect? > My copy of the personality(2) manpage just says: > >PER_LINUX32 (since Linux 2.2) > [To be documented.] > > which isn't very informative. It's not a POSIX thing (not part of the Single Unix Specification) so as with most Linux things it has some fuzzy semantics defined by the community... I usually just go to the source. If you grep the kernel what turns up is a bunch of architecture-specific behaviors on arm64, ia64, mips, parisc, powerpc, s390, sparc. They are very minor. On x86_64 the semantic effect is none so this would work for any kind of 32bit userspace on x86_64. It would be the first time this flag has any effect at all on x86_64, but arguably a useful one. I would not be surprised if running say i586 on x86_64 has the same problem, just that noone has reported it yet. But what do I know. If they have the same problem they can use the same solution. Hm QEMU supports emulating i586 or even i386 ... maybe you could test? Or tell me how to test? We might be solving a bigger issue here. Yours, Linus Walleij
Re: [PATCH] ext4: Give 32bit personalities 32bit hashes
On Tue, Mar 17, 2020 at 12:53 PM Florian Weimer wrote: > Just be sure: Is it possible to move the PER_LINUX32 setting into QEMU? > (I see why not.) I set it in the program explicitly, but what actually happens when I run it is that the binfmt handler invokes qemu-user so certainly that program can set the flag, any process can. Yours, Linus Walleij
[PATCH] ext4: Give 32bit personalities 32bit hashes
It was brought to my attention that this bug from 2018 was
still unresolved: 32 bit emulators like QEMU were given
64 bit hashes when running 32 bit emulation on 64 bit systems.
The personality(2) system call supports to let processes
indicate that they are 32 bit Linux to the kernel. This
was suggested by Teo in the original thread, so I just wired
it up and it solves the problem.
Programs that need the 32 bit hash only need to issue the
personality(PER_LINUX32) call and things start working.
I made a test program like this:
#include
#include
#include
#include
#include
#include
int main(int argc, char** argv) {
DIR* dir;
personality(PER_LINUX32);
dir = opendir("/boot");
printf("dir=%p\n", dir);
printf("readdir(dir)=%p\n", readdir(dir));
printf("errno=%d: %s\n", errno, strerror(errno));
return 0;
}
This was compiled with an ARM32 toolchain from Bootlin using
glibc 2.28 and thus suffering from the bug.
Before the patch:
$ ./readdir-bug
dir=0x86000
readdir(dir)=(nil)
errno=75: Value too large for defined data type
After the patch:
$ ./readdir-bug
dir=0x86000
readdir(dir)=0x86020
errno=0: Success
Problem solved.
Cc: Florian Weimer
Cc: Peter Maydell
Cc: Andy Lutomirski
Cc: sta...@vger.kernel.org
Suggested-by: Theodore Ts'o
Link: https://bugs.launchpad.net/qemu/+bug/1805913
Link: https://lore.kernel.org/lkml/87bm56vqg4@mid.deneb.enyo.de/
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=205957
Signed-off-by: Linus Walleij
---
fs/ext4/dir.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
index 9aa1f75409b0..3faf9edf3e92 100644
--- a/fs/ext4/dir.c
+++ b/fs/ext4/dir.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "ext4.h"
#include "xattr.h"
@@ -618,6 +619,14 @@ static int ext4_dx_readdir(struct file *file, struct
dir_context *ctx)
static int ext4_dir_open(struct inode * inode, struct file * filp)
{
+ /*
+* If we are currently running e.g. a 32 bit emulator on
+* a 64 bit machine, the emulator will indicate that it needs
+* a 32 bit personality and thus 32 bit hashes from the file
+* system.
+*/
+ if (personality(current->personality) == PER_LINUX32)
+ filp->f_mode |= FMODE_32BITHASH;
if (IS_ENCRYPTED(inode))
return fscrypt_get_encryption_info(inode) ? -EACCES : 0;
return 0;
--
2.24.1
Re: [PATCH v5 3/5] gpio: Add GPIO Aggregator
Hi Geert,
thanks for your patience and again sorry for procrastination on my part :(
Overall I start to like this driver a lot. It has come a long way.
Some comments below are nitpicky, bear with me if they seem stupid.
On Tue, Feb 18, 2020 at 4:18 PM Geert Uytterhoeven
wrote:
> +#define DRV_NAME "gpio-aggregator"
> +#define pr_fmt(fmt)DRV_NAME ": " fmt
I would just use dev_[info|err] for all messages to get rid of this.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "gpiolib.h"
When this file is includes I prefer if there is a comment next to
this include saying why we have to touch internals and which
ones.
> +struct gpio_aggregator {
> + struct gpiod_lookup_table *lookups;
> + struct platform_device *pdev;
What about just storing struct device *dev?
Then callbacks can just
dev_err(aggregator->dev, "myerror\n");
> +static char *get_arg(char **args)
> +{
> + char *start = *args, *end;
> +
> + start = skip_spaces(start);
> + if (!*start)
> + return NULL;
> +
> + if (*start == '"') {
> + /* Quoted arg */
> + end = strchr(++start, '"');
> + if (!end)
> + return ERR_PTR(-EINVAL);
> + } else {
> + /* Unquoted arg */
> + for (end = start; *end && !isspace(*end); end++) ;
> + }
> +
> + if (*end)
> + *end++ = '\0';
> +
> + *args = end;
> + return start;
> +}
Isn't this function reimplementing strsep()?
while ((s = strsep(, " \""))) {
or something.
I'm not the best with strings, just asking so I know you tried it
already.
> +static int aggr_parse(struct gpio_aggregator *aggr)
> +{
> + unsigned int first_index, last_index, i, n = 0;
> + char *name, *offsets, *first, *last, *next;
> + char *args = aggr->args;
> + int error;
> +
> + for (name = get_arg(), offsets = get_arg(); name;
> +offsets = get_arg()) {
> + if (IS_ERR(name)) {
> + pr_err("Cannot get GPIO specifier: %pe\n", name);
If gpio_aggregrator contained struct device *dev this would be
dev_err(aggr->dev, "...\n");
> +static void gpio_aggregator_free(struct gpio_aggregator *aggr)
> +{
> + platform_device_unregister(aggr->pdev);
Aha maybe store both the pdev and the dev in the struct then?
Or print using >pdev.dev.
> + /*
> +* If any of the GPIO lines are sleeping, then the entire forwarder
> +* will be sleeping.
> +* If any of the chips support .set_config(), then the forwarder will
> +* support setting configs.
> +*/
> + for (i = 0; i < ngpios; i++) {
> + dev_dbg(dev, "gpio %u => gpio-%d (%s)\n", i,
> + desc_to_gpio(descs[i]), descs[i]->label ? : "?");
If this desc->label business is why you need to include
"gpiolib.h" then I'd prefer if you just add a
const char *gpiod_get_producer_name(struct gpio_desc *desc);
to gpiolib (add in so that gpiolib can
try to give you something reasonable to print for the label here.
I ran into that problem before (wanting to print something like this)
and usually just printed the offset.
But if it is a serious debug issue, let's fix a helper for this.
gpiod_get_producer_name() could return the thing in
desc->label if that is set or else something along
"chipname-offset" or "unknown", I'm not very picky
with that.
> error = aggr_add_gpio(aggr, name, U16_MAX, );
Is the reason why you use e.g. "gpiochip0" as name here that this
is a simple ABI for userspace?
Such like obtained from /sys/bus/gpio/devices/?
I would actually prefer to just add a sysfs attribute
such as "name" and set it to the value of gpiochip->label.
These labels are compulsory and supposed to be unique.
Then whatever creates an aggregator can just use
cat /sys/bus/gpio/devices/gpiochipN/name to send in
through the sysfs interface to this kernel driver.
This will protect you in the following way:
When a system is booted and populated the N in
gpiochipN is not stable and this aggregator will be used
by scripts that assume it is. We already had this dilemma
with things like network interfaces like eth0/1.
This can be because of things like probe order which
can be random, or because someone compiled a
kernel with a new driver for a gpiochip that wasn't
detected before. This recently happened to Raspberry Pi,
that added gpio driver for "firmware GPIOs" (IIRC).
The label on the chip is going to be more stable
I think, so it is better to use that.
This should also rid the need to include "gpiolib.h"
which makes me nervous.
Yours,
Linus Walleij
Re: [PATCH v5 1/5] gpiolib: Add support for gpiochipN-based table lookup
On Tue, Feb 18, 2020 at 4:18 PM Geert Uytterhoeven wrote: > Currently GPIO controllers can only be referred to by label in GPIO > lookup tables. > > Add support for looking them up by "gpiochipN" name, with "N" the > corresponding GPIO device's ID number. > > Signed-off-by: Geert Uytterhoeven > Reviewed-by: Ulrich Hecht > Reviewed-by: Eugeniu Rosca > Tested-by: Eugeniu Rosca Just like with patch 2/5 I have the same problem here that the commit message doesn't state the technical reason why we need to change this and support the device name in these tables and not just labels. (Possibly again I will realize it...) Yours, Linus Walleij
Re: [PATCH v5 2/5] gpiolib: Add support for GPIO line table lookup
Hi Geert, I'm sorry for the slow review, it's a large patch set and takes some time to sit down and review, and see whether my earlier comments have been addressed. On Tue, Feb 18, 2020 at 4:18 PM Geert Uytterhoeven wrote: > Currently GPIOs can only be referred to by GPIO controller and offset in > GPIO lookup tables. > > Add support for looking them up by line name. > Rename gpiod_lookup.chip_label to gpiod_lookup.key, to make it clear > that this field can have two meanings, and update the kerneldoc and > GPIO_LOOKUP*() macros. > > Signed-off-by: Geert Uytterhoeven > Reviewed-by: Ulrich Hecht > Reviewed-by: Eugeniu Rosca > Tested-by: Eugeniu Rosca I will try to understand why this change is necessary to implement the gpio aggregator (probablt I will comment that on the other patches like "aha now I see it" or so, but it would help a lot if the commit message would state the technical reason to why we need to do this change, like what it is that you want to do and why you cannot do it without this change. Yours, Linus Walleij
Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
On Mon, Jan 6, 2020 at 9:23 AM Geert Uytterhoeven wrote: > > The rest I think we cleared out else I will see it when I review again. > > The remaining discussion point is "GPIO Repeater in Device Tree", i.e. > the GPIO inverter usecase, which might be solved better by adding a > GPIO_INVERTED flag. > > Shall I rip that out, incorporate review comments, and report? Don't know, if it blocks progress I guess the diplomatic solution is to do that, divide and conquer. But review is a social process so I don't really know the right strategy. Yours, Linus Walleij
Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
Sorry for slowness... christmas.
On Thu, Dec 12, 2019 at 4:24 PM Geert Uytterhoeven wrote:
> On Thu, Dec 12, 2019 at 3:34 PM Linus Walleij
> wrote:
> > > + This can serve the following purposes:
> > > + 1. Assign a collection of GPIOs to a user, or export them to a
> > > + virtual machine,
> >
> > This is ambiguous. What is a "user"? A process calling from
> > userspace? A device tree node?
>
> A user is an entity with a UID, typically listed in /etc/passwd.
> This is similar to letting some, not all, people on the machine access
> the CD-ROM drive.
Ah I get it. Maybe we can say "assign permissions for a collection
of GPIOs to a user".
> > I would write "assign a collection of GPIO lines from any lines on
> > existing physical GPIO chips to form a new virtual GPIO chip"
> >
> > That should be to the point, right?
>
> Yes, that's WHAT it does. The WHY is the granular access control.
So I guess we can write both?
> > > + 3. Provide a generic driver for a GPIO-operated device, to be
> > > + controlled from userspace using the GPIO chardev
> > > interface.
> >
> > I don't understand this, it needs to be elaborated. What is meant
> > by a "GPIO-operated device" in this context? Example?
>
> E.g. a motor. Or a door opener.
>
> door-opener {
> compatible = "mydoor,opener";
>
> gpios = < 19 GPIO_ACTIVE_HIGH>;
> };
>
> You don't need a full-featured kernel driver for that, so just bind the
> gpio-aggregator to the door-opener, and control it through libgpiod.
Yep it's a perfect industrial control example, I get it.
Maybe we should blurb something about industrial control?
The rest I think we cleared out else I will see it when I review again.
Yours,
Linus Walleij
Re: [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation
On Thu, Dec 12, 2019 at 3:48 PM Geert Uytterhoeven wrote: > On Thu, Dec 12, 2019 at 3:42 PM Linus Walleij > wrote: > > On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven > > wrote: > > > +The GPIO Aggregator allows access control for individual GPIOs, by > > > aggregating > > > +them into a new gpio_chip, which can be assigned to a group or user using > > > +standard UNIX file ownership and permissions. Furthermore, this > > > simplifies and > > > +hardens exporting GPIOs to a virtual machine, as the VM can just grab > > > the full > > > +GPIO controller, and no longer needs to care about which GPIOs to grab > > > and > > > +which not, reducing the attack surface. > > > + > > > +Aggregated GPIO controllers are instantiated and destroyed by writing to > > > +write-only attribute files in sysfs. > > > > I suppose virtual machines will have a lengthy config file where > > they specify which GPIO lines to pick and use for their GPIO > > aggregator, and that will all be fine, the VM starts and the aggregator > > is there and we can start executing. > > > > I would perhaps point out a weakness as with all sysfs and with the current > > gpio sysfs: if a process creates an aggregator device, and then that > > process crashes, what happens when you try to restart the process and > > run e.g. your VM again? > > > > Time for a hard reboot? Or should we add some design guidelines for > > these machines so that they can cleanly tear down aggregators > > previously created by the crashed VM? > > No, the VM does not create the aggregator. > > The idea is for the user to create one or more aggregators, set up > permissions on /dev/gpiochipX, and launch the VM, passing the aggregated > /dev/gpiochipX as parameters. > If the VM crashes, just launch it again. > > Destroying the aggregators is a manual and independent process, after > the VM has exited. I'm thinking about someone making some industrial application for some control of a machinery say a robotic arm. And do make sure this VM is only controlling these GPIOs related to this robotic arm, they create a GPIO aggregator. And we care about cases like that since we provide this security argument. Surely that machine will be rebooted. Surely they don't have a printed paper with all the commands lying at the console, and asking whoever powers it back on to manually type it all in again. That feels a bit 1981. So they will have a script for this I suppose. Possibly in some initscript so it is set up on boot. And this script echos stuff all over the place to set up the aggregator. Is this the use case you're thinking of? I just like to have the whole picture here. Yours, Linus Walleij
Re: [PATCH v3 6/7] docs: gpio: Add GPIO Aggregator/Repeater documentation
On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven wrote: > +The GPIO Aggregator allows access control for individual GPIOs, by > aggregating > +them into a new gpio_chip, which can be assigned to a group or user using > +standard UNIX file ownership and permissions. Furthermore, this simplifies > and > +hardens exporting GPIOs to a virtual machine, as the VM can just grab the > full > +GPIO controller, and no longer needs to care about which GPIOs to grab and > +which not, reducing the attack surface. > + > +Aggregated GPIO controllers are instantiated and destroyed by writing to > +write-only attribute files in sysfs. I suppose virtual machines will have a lengthy config file where they specify which GPIO lines to pick and use for their GPIO aggregator, and that will all be fine, the VM starts and the aggregator is there and we can start executing. I would perhaps point out a weakness as with all sysfs and with the current gpio sysfs: if a process creates an aggregator device, and then that process crashes, what happens when you try to restart the process and run e.g. your VM again? Time for a hard reboot? Or should we add some design guidelines for these machines so that they can cleanly tear down aggregators previously created by the crashed VM? Yours, Linus Walleij
Re: [PATCH v3 2/7] gpiolib: Add support for gpiochipN-based table lookup
On Thu, Dec 12, 2019 at 2:33 PM Geert Uytterhoeven wrote: > On Thu, Dec 12, 2019 at 2:20 PM Linus Walleij > wrote: > > On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven > > wrote: > > > Currently GPIO controllers can only be referred to by label in GPIO > > > lookup tables. > > > > > > Add support for looking them up by "gpiochipN" name, with "N" either the > > > corresponding GPIO device's ID number, or the GPIO controller's first > > > GPIO number. > > > > > > Signed-off-by: Geert Uytterhoeven > > > > What the commit message is missing is a rationale, why is this needed? > > Right. To be added: so they can be looked up in the GPIO lookup table > using either the chip's label, or the "gpiochipN" name. After reading the aggregator/forwarder driver I am not convinced that this is needed at all and I think this patch can be dropped, but check my review and see what you think! Thanks, Linus Walleij
Re: [PATCH v3 5/7] gpio: Add GPIO Aggregator/Repeater driver
ere something wrong with my reasoning here?
At the pointe later when the lines are counted from the
allocated lookups using gpiod_count() that will just figure
out this number anyways, so it is not like we don't know
it at the end of the day.
So it seems the patch to gpiolib is just to use machine
descriptor tables as a substitute for a simple counter
variable in this local struct to me.
> +static void __exit gpio_aggregator_remove_all(void)
> +{
> + mutex_lock(_aggregator_lock);
> + idr_for_each(_aggregator_idr, gpio_aggregator_idr_remove, NULL);
> + idr_destroy(_aggregator_idr);
> + mutex_unlock(_aggregator_lock);
> +}
> +
> +
> + /*
> +* Common GPIO Forwarder
> +*/
> +
Nitpick: lots and weird spacing here.
> +struct gpiochip_fwd {
> + struct gpio_chip chip;
> + struct gpio_desc **descs;
> + union {
> + struct mutex mlock; /* protects tmp[] if can_sleep */
> + spinlock_t slock; /* protects tmp[] if !can_sleep */
> + };
That was a very elegant use of union!
> +static int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask,
> +unsigned long *bits)
> +static void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long
> *mask,
> + unsigned long *bits)
I guess these can both be optimized to use get/set_multiple on
the target chip if the offsets are consecutive?
However that is going to be tricky so I'm not saying you should
implement that. So for now, let's say just add a TODO: comment
about it.
> +static int gpio_fwd_init_valid_mask(struct gpio_chip *chip,
> + unsigned long *valid_mask,
> + unsigned int ngpios)
> +{
> + struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> + unsigned int i;
> +
> + for (i = 0; i < ngpios; i++) {
> + if (!gpiochip_line_is_valid(fwd->descs[i]->gdev->chip,
> + gpio_chip_hwgpio(fwd->descs[i])))
> + clear_bit(i, valid_mask);
> + }
This is what uses "gpiolib.h" is it not?
devm_gpiod_get_index() will not succeed if the line
is not valid so I think this can be just dropped, since
what you do before this is exactly devm_gpiod_get_index()
on each line, then you call gpiochip_fwd_create()
with the result.
So I think you can just drop this entire function.
This will not happen.
If it does happen, add a comment above this loop
explaining which circumstances would make lines on
the forwarder invalid.
> + for (i = 0; i < ngpios; i++) {
> + dev_dbg(dev, "gpio %u => gpio-%d (%s)\n", i,
> + desc_to_gpio(descs[i]), descs[i]->label ? : "?");
> +
> + if (gpiod_cansleep(descs[i]))
> + chip->can_sleep = true;
> + if (descs[i]->gdev->chip->set_config)
> + chip->set_config = gpio_fwd_set_config;
> + if (descs[i]->gdev->chip->init_valid_mask)
> + chip->init_valid_mask = gpio_fwd_init_valid_mask;
> + }
I do not think you should need to inspect the init_valid_mask()
as explained above.
Add a comment above the loop that if any of the GPIO lines
are sleeping then the entire forwarder will be sleeping
and if any of the chips support .set_config() we will support
setting configs.
However the way that the .gpio_fwd_set_config() is coded
it looks like you can just unconditionally assign it and
only check the cansleep condition in this loop.
> +}
> +
> +
> + /*
> +* Common GPIO Aggregator/Repeater platform device
> +*/
> +
Nitpick: weird and excess spacing again.
> + for (i = 0; i < n; i++) {
> + descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
> + if (IS_ERR(descs[i]))
> + return PTR_ERR(descs[i]);
> + }
If this succeeds none of the obtained gpio_desc:s can be
invalid.
Yours,
Linus Walleij
Re: [PATCH v3 3/7] gpiolib: Add support for GPIO line table lookup
On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
wrote:
> Currently GPIOs can only be referred to by GPIO controller and offset in
> GPIO lookup tables.
>
> Add support for looking them up by line name.
>
> Signed-off-by: Geert Uytterhoeven
OK I see what you want to do...
> + if (p->chip_hwnum == (u16)-1) {
If you want to use this then use:
if (p->chip_hwnum == U16_MAX)
from
> + desc = gpio_name_to_desc(p->chip_label);
> + if (desc) {
> + *flags = p->flags;
> + return desc;
> + }
> +
> + dev_warn(dev, "cannot find GPIO line %s, deferring\n",
> +p->chip_label);
> + return ERR_PTR(-EPROBE_DEFER);
> + }
> +
> chip = find_chip_by_name(p->chip_label);
>
> if (!chip) {
> diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
> index 1ebe5be05d5f81fa..84c1c097e55eefaf 100644
> --- a/include/linux/gpio/machine.h
> +++ b/include/linux/gpio/machine.h
> @@ -31,7 +31,7 @@ enum gpio_lookup_flags {
> */
> struct gpiod_lookup {
> const char *chip_label;
> - u16 chip_hwnum;
> + u16 chip_hwnum; /* if -1, chip_label is named line */
/* if U16_MAX then chip_label is the named line we are looking for */
But the member name "chip_label" is completely abused with this
setup, it should then be renamed as part of the patch set to something
like chip_label_or_line_name so it is clear what it is or
just name it "const char *key".
But I'm not entirely convinced about reusing the existing
struct gpio_lookup for this.
What about constructing a new lookup struct specifically for this?
I understand it is more work, but will that not be more
maintainable and readable?
Yours,
Linus Walleij
Re: [PATCH v3 2/7] gpiolib: Add support for gpiochipN-based table lookup
Hi Geert!
On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven
wrote:
> Currently GPIO controllers can only be referred to by label in GPIO
> lookup tables.
>
> Add support for looking them up by "gpiochipN" name, with "N" either the
> corresponding GPIO device's ID number, or the GPIO controller's first
> GPIO number.
>
> Signed-off-by: Geert Uytterhoeven
What the commit message is missing is a rationale, why is this needed?
> If this is rejected, the GPIO Aggregator documentation must be updated.
>
> The second variant is currently used by the legacy sysfs interface only,
> so perhaps the chip->base check should be dropped?
Anything improving the sysfs is actively discouraged by me.
If it is just about staying compatible it is another thing.
> +static int gpiochip_match_id(struct gpio_chip *chip, void *data)
> +{
> + int id = (uintptr_t)data;
> +
> + return id == chip->base || id == chip->gpiodev->id;
> +}
> static struct gpio_chip *find_chip_by_name(const char *name)
> {
> - return gpiochip_find((void *)name, gpiochip_match_name);
> + struct gpio_chip *chip;
> + int id;
> +
> + chip = gpiochip_find((void *)name, gpiochip_match_name);
> + if (chip)
> + return chip;
> +
> + if (!str_has_prefix(name, GPIOCHIP_NAME))
> + return NULL;
> +
> + if (kstrtoint(name + strlen(GPIOCHIP_NAME), 10, ))
> + return NULL;
> +
> + return gpiochip_find((void *)(uintptr_t)id, gpiochip_match_id);
Isn't it easier to just augment the existing match function to
check like this:
static int gpiochip_match_name(struct gpio_chip *chip, void *data)
{
const char *name = data;
if (!strcmp(chip->label, name))
return 0;
return !strcmp(dev_name(>gpiodev->dev), name);
}
We should I guess also add some kerneldoc to say we first
match on the label and second on dev_name().
Yours,
Linus Walleij
Re: [PATCH v3 1/7] gpiolib: Add GPIOCHIP_NAME definition
On Wed, Nov 27, 2019 at 9:43 AM Geert Uytterhoeven wrote: > The string literal "gpiochip" is used in several places. > Add a definition for it, and use it everywhere, to make sure everything > stays in sync. > > Signed-off-by: Geert Uytterhoeven > --- > v3: > - New. This is a good patch on its own merits so I have applied this with the ACKs. (Haven't looked at the rest yet...) Yours, Linus Walleij
Re: [Qemu-devel] [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver
On Fri, Jul 5, 2019 at 5:05 PM Geert Uytterhoeven wrote: > GPIO controllers are exported to userspace using /dev/gpiochip* > character devices. Access control to these devices is provided by > standard UNIX file system permissions, on an all-or-nothing basis: > either a GPIO controller is accessible for a user, or it is not. > Currently no mechanism exists to control access to individual GPIOs. > > Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32), > and expose them as a new gpiochip. This is useful for implementing > access control, and assigning a set of GPIOs to a specific user. > Furthermore, it would simplify and harden exporting GPIOs to a virtual > machine, as the VM can just grab the full virtual GPIO controller, and > no longer needs to care about which GPIOs to grab and which not, > reducing the attack surface. > > Virtual GPIO controllers are instantiated by writing to the "new_device" > attribute file in sysfs: > > $ echo " [ ...]" >"[, [ ...]] ...]" > > /sys/bus/platform/drivers/gpio-virt-agg/new_device > > Likewise, virtual GPIO controllers can be destroyed after use: > > $ echo gpio-virt-agg. \ > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device > > Signed-off-by: Geert Uytterhoeven > --- > Aggregating GPIOs and exposing them as a new gpiochip was suggested in > response to my proof-of-concept for GPIO virtualization with QEMU[1][2]. > > Sample session on r8a7791/koelsch: > > - Disable the leds node in arch/arm/boot/dts/r8a7791-koelsch.dts > > - Create virtual aggregators: > > $ echo "e6052000.gpio 19 20" \ > > /sys/bus/platform/drivers/gpio-virt-agg/new_device > > gpio-virt-agg gpio-virt-agg.0: GPIO 0 => e6052000.gpio/19 > gpio-virt-agg gpio-virt-agg.0: GPIO 1 => e6052000.gpio/20 > gpiochip_find_base: found new base at 778 > gpio gpiochip8: (gpio-virt-agg.0): added GPIO chardev (254:8) > gpiochip_setup_dev: registered GPIOs 778 to 779 on device: gpiochip8 > (gpio-virt-agg.0) > > $ echo "e6052000.gpio 21, e605.gpio 20 21 22" \ > > /sys/bus/platform/drivers/gpio-virt-agg/new_device > > gpio-virt-agg gpio-virt-agg.1: GPIO 0 => e6052000.gpio/21 > gpio-virt-agg gpio-virt-agg.1: GPIO 1 => e605.gpio/20 > gpio-virt-agg gpio-virt-agg.1: GPIO 2 => e605.gpio/21 > gpio-virt-agg gpio-virt-agg.1: GPIO 3 => e605.gpio/22 > gpiochip_find_base: found new base at 774 > gpio gpiochip9: (gpio-virt-agg.1): added GPIO chardev (254:9) > gpiochip_setup_dev: registered GPIOs 774 to 777 on device: gpiochip9 > (gpio-virt-agg.1) > > - Adjust permissions on /dev/gpiochip[89] (optional) > > - Control LEDs: > > $ gpioset gpiochip8 0=0 1=1 # LED6 OFF, LED7 ON > $ gpioset gpiochip8 0=1 1=0 # LED6 ON, LED7 OFF > $ gpioset gpiochip9 0=0 # LED8 OFF > $ gpioset gpiochip9 0=1 # LED8 ON > > - Destroy virtual aggregators: > > $ echo gpio-virt-agg.0 \ > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device > $ echo gpio-virt-agg.1 \ > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device > > Thanks for your comments! > > References: > - [1] "[PATCH QEMU POC] Add a GPIO backend" > > (https://lore.kernel.org/linux-renesas-soc/20181003152521.23144-1-geert+rene...@glider.be/) > - [2] "Getting To Blinky: Virt Edition / Making device pass-through > work on embedded ARM" > (https://fosdem.org/2019/schedule/event/vai_getting_to_blinky/) I'm looping in my friends at Google for this discussion. They need a virtualized gpio_chip for their Android emulator, and their current approach for other devices has been around using virtio in most cases and an emulated AC97 for the audio case as far as I remember. It would be great to have their input on this so we can create a virtualization/aggregate that works for all. Please include ade...@google.com on future postings of this! Yours, Linus Walleij
Re: [Qemu-devel] [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver
On Mon, Aug 5, 2019 at 12:21 PM Marc Zyngier wrote: > On 01/08/2019 09:41, Linus Walleij wrote: > > I would even go so far as to call it "gpio-virtualization" or > > "gpio-virtualized" rather than "gpio-virtual" so it is clear what the > > intended usecase is. We have a bit of confusion in the kernel > > because people misuse the word "virtual" left and right, like for > > "virtual IRQ number" (Linux IRQ numbers) which are just some > > random number space, but not really "virtual", it's a semantic > > disease similar to the confusion of using the word "manual" in > > computer code. > > I'd drop the notion of "virtual" altogether. Nothing is virtual in this > thing at all (the GPIOs are very real, from what I gather). Instead (and > assuming I got it right, which is a long shot), what you have is a > "synthetic" GPIO controller, made from the GPIOs belonging to other > controllers. I'd call it "GPIO aggregator". +1 on this. Next thing that will predictably follow is a userspace ABI to create those aggregators and have them go away if the process creating it dies. Something to think of... > > If QEMU can deal in a simple and straight-forward way with this > > I see it quickly becoming a very useful tool for industrial automation > > where you want to run each control system in isolation and just > > respawn the virtual machine if something goes wrong. > > What the VMM (QEMU, kvmtool) would need to do is to present this as a > "standard" GPIO IP, and use the backend aggregator to emulate it. > Certainly doable. The nice part is that all the work is in userspace, > and thus completely off my plate! ;-) Yeah there is not really any "standard" GPIO, but if the user is running e.g. a Versatile Express model, that has a PL061 GPIO, and then a user would create an "aggregator GPIO" with say 8 lines and QEMU would pick that up as /dev/gpiochipN and bind it inside of QEMU to the lines of the virttualized PL061 GPIO controller, so the machine thinks it is using a PL061 but in reality it is just 8 GPIO lines on the host computer. > You also may want to look into not emulating a standard IP, but use > something virtio-based instead. The ACRN project seems to have something > like this in progress, but I know nothing about it. That sounds quite interesting as well. Then the virtualized machine can just pick this "virtio GPIO" and some command line switches or config file on the host can set up and route a GPIO aggregator. Yours, Linus Walleij
Re: [Qemu-devel] [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver
Hi Geert! Thanks for this very interesting patch! On Fri, Jul 5, 2019 at 6:05 PM Geert Uytterhoeven wrote: > GPIO controllers are exported to userspace using /dev/gpiochip* > character devices. Access control to these devices is provided by > standard UNIX file system permissions, on an all-or-nothing basis: > either a GPIO controller is accessible for a user, or it is not. > Currently no mechanism exists to control access to individual GPIOs. Yes, I did that decision deliberately, as the chip is one device and the base system control is usually on a per-device granularity. At one point some people were asking for individual GPIO line permissions in the character device and my argument was something like why can't I have individual control over the access rights on a block device or the pixels on a graphics device then. Jokes aside, filesystems do provide access control over individual blocks on a block device in a way. So it is further up the stack. The same goes for this: something above the GPIO chip provide more granular access control, and as such it fits the need very well. > Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32), > and expose them as a new gpiochip. This is useful for implementing > access control, and assigning a set of GPIOs to a specific user. > Furthermore, it would simplify and harden exporting GPIOs to a virtual > machine, as the VM can just grab the full virtual GPIO controller, and > no longer needs to care about which GPIOs to grab and which not, > reducing the attack surface. Excellent approach. I would even go so far as to call it "gpio-virtualization" or "gpio-virtualized" rather than "gpio-virtual" so it is clear what the intended usecase is. We have a bit of confusion in the kernel because people misuse the word "virtual" left and right, like for "virtual IRQ number" (Linux IRQ numbers) which are just some random number space, but not really "virtual", it's a semantic disease similar to the confusion of using the word "manual" in computer code. Here it is however used correctly! (Maybe for the first time.) > Virtual GPIO controllers are instantiated by writing to the "new_device" > attribute file in sysfs: > > $ echo " [ ...]" >"[, [ ...]] ...]" > > /sys/bus/platform/drivers/gpio-virt-agg/new_device > > Likewise, virtual GPIO controllers can be destroyed after use: > > $ echo gpio-virt-agg. \ > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device I suppose this is the right way to use sysfs. I would check with some virtualization people (paged Marc Zyngier and Christoffer Dall) so they can say whether this is the way any virtual machine wants to populate its local GPIO chip for use with a certain machine. If QEMU can deal in a simple and straight-forward way with this I see it quickly becoming a very useful tool for industrial automation where you want to run each control system in isolation and just respawn the virtual machine if something goes wrong. Since this might be very popular we need some scrutiny but the concept as a whole is very appetizing! Yours, Linus Walleij
Re: [Qemu-devel] [PATCH 0/5] Versatile Express SiI9022 emulation
On Tue, Feb 27, 2018 at 6:26 PM, Peter Maydell <peter.mayd...@linaro.org> wrote: > On 27 February 2018 at 10:48, Linus Walleij <linus.wall...@linaro.org> wrote: >> This series adds proper display bridge/connector emulation >> for the Versatile Express, implementing a simple Silicon >> Image 9022 emulation spawning a DDC I2C child. >> >> After the series the Versatile Express is successfully >> presented the "QEMU monitor" through DDC I2C. >> >> The series includes two refactorings from Corey and a >> minor bug fix for the i2c-ddc so that everything is smoothly >> integrated. >> >> Corey Minyard (2): >> i2c: Fix some brace style issues >> i2c: Move the bus class to i2c.h >> >> Linus Walleij (3): >> hw/i2c-ddc: Do not fail writes >> hw/sii9022: Add support for Silicon Image SII9022 >> arm/vexpress: Add proper display connector emulation > > Hi; I've applied this to target-arm.next with the tweak to > the commit message of patch 5 and the reset function in patch 4. > Let me know if you think those are wrong or you'd prefer to > respin the series yourself. I am certain you did it better than I could, thanks a lot. I forgot about the reset business, sorry :( I will test it once it hits the trunk! Yours, Linus Walleij
[Qemu-devel] [PATCH 5/5] arm/vexpress: Add proper display connector emulation
This adds the SiI9022 and EDID I2C devices to the ARM Versatile
Express machine, and selects the two I2C devices necessary in the
arm-softmmy.mak configuration so everything will build smoothly.
I am implementing proper handling of the graphics in the Linux
kernel and adding proper emulation of SiI9022 and EDID makes the
driver probe as nicely as before, retrieveing the resolutions
supported by the "QEMU monitor" and overall just working nice.
The assignment of the SiI9022 at address 0x39 and the EDID
DDC I2C at address 0x50 is not strictly correct: the DDC I2C
is there all the time but in the actual component it only
appears once activated inside the SiI9022, so ideally it should
be added and removed to the bus by the SiI9022. However for this
purpose it works fine to just have it around.
Cc: Peter Maydell <peter.mayd...@linaro.org>
Signed-off-by: Linus Walleij <linus.wall...@linaro.org>
---
ChangeLog v1->v2:
- Only add the SII9022 now that it will by itself realize
the DDCI2C as part of the bridge.
---
default-configs/arm-softmmu.mak | 2 ++
hw/arm/vexpress.c | 6 +-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index ca34cf446242..54f855d07206 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -21,6 +21,8 @@ CONFIG_STELLARIS_INPUT=y
CONFIG_STELLARIS_ENET=y
CONFIG_SSD0303=y
CONFIG_SSD0323=y
+CONFIG_DDC=y
+CONFIG_SII9022=y
CONFIG_ADS7846=y
CONFIG_MAX111X=y
CONFIG_SSI=y
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index dc5928ae1ab5..9fad79177a19 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -29,6 +29,7 @@
#include "hw/arm/arm.h"
#include "hw/arm/primecell.h"
#include "hw/devices.h"
+#include "hw/i2c/i2c.h"
#include "net/net.h"
#include "sysemu/sysemu.h"
#include "hw/boards.h"
@@ -537,6 +538,7 @@ static void vexpress_common_init(MachineState *machine)
uint32_t sys_id;
DriveInfo *dinfo;
pflash_t *pflash0;
+I2CBus *i2c;
ram_addr_t vram_size, sram_size;
MemoryRegion *sysmem = get_system_memory();
MemoryRegion *vram = g_new(MemoryRegion, 1);
@@ -628,7 +630,9 @@ static void vexpress_common_init(MachineState *machine)
sysbus_create_simple("sp804", map[VE_TIMER01], pic[2]);
sysbus_create_simple("sp804", map[VE_TIMER23], pic[3]);
-/* VE_SERIALDVI: not modelled */
+dev = sysbus_create_simple("versatile_i2c", map[VE_SERIALDVI], NULL);
+i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
+i2c_create_slave(i2c, "sii9022", 0x39);
sysbus_create_simple("pl031", map[VE_RTC], pic[4]); /* RTC */
--
2.14.3
[Qemu-devel] [PATCH 3/5] hw/i2c-ddc: Do not fail writes
The tx function of the DDC I2C slave emulation was returning 1
on all writes resulting in NACK in the I2C bus. Changing it to
0 makes the DDC I2C work fine with bit-banged I2C such as the
versatile I2C.
I guess it was not affecting whatever I2C controller this was
used with until now, but with the Versatile I2C it surely
does not work.
Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
Signed-off-by: Linus Walleij <linus.wall...@linaro.org>
---
hw/i2c/i2c-ddc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/i2c/i2c-ddc.c b/hw/i2c/i2c-ddc.c
index 199dac9e41c1..bec0c91e2dd0 100644
--- a/hw/i2c/i2c-ddc.c
+++ b/hw/i2c/i2c-ddc.c
@@ -259,12 +259,12 @@ static int i2c_ddc_tx(I2CSlave *i2c, uint8_t data)
s->reg = data;
s->firstbyte = false;
DPRINTF("[EDID] Written new pointer: %u\n", data);
-return 1;
+return 0;
}
/* Ignore all writes */
s->reg++;
-return 1;
+return 0;
}
static void i2c_ddc_init(Object *obj)
--
2.14.3
[Qemu-devel] [PATCH 0/5] Versatile Express SiI9022 emulation
This series adds proper display bridge/connector emulation for the Versatile Express, implementing a simple Silicon Image 9022 emulation spawning a DDC I2C child. After the series the Versatile Express is successfully presented the "QEMU monitor" through DDC I2C. The series includes two refactorings from Corey and a minor bug fix for the i2c-ddc so that everything is smoothly integrated. Corey Minyard (2): i2c: Fix some brace style issues i2c: Move the bus class to i2c.h Linus Walleij (3): hw/i2c-ddc: Do not fail writes hw/sii9022: Add support for Silicon Image SII9022 arm/vexpress: Add proper display connector emulation default-configs/arm-softmmu.mak | 2 + hw/arm/vexpress.c | 6 +- hw/display/Makefile.objs| 1 + hw/display/sii9022.c| 188 hw/display/trace-events | 5 ++ hw/i2c/core.c | 18 hw/i2c/i2c-ddc.c| 4 +- include/hw/i2c/i2c.h| 23 - 8 files changed, 222 insertions(+), 25 deletions(-) create mode 100644 hw/display/sii9022.c -- 2.14.3
[Qemu-devel] [PATCH 4/5] hw/sii9022: Add support for Silicon Image SII9022
This adds support for emulating the Silicon Image SII9022 DVI/HDMI
bridge. It's not very clever right now, it just acknowledges
the switch into DDC I2C mode and back. Combining this with the
existing DDC I2C emulation gives the right behavior on the Versatile
Express emulation passing through the QEMU EDID to the emulated
platform.
Cc: Peter Maydell <peter.mayd...@linaro.org>
Signed-off-by: Linus Walleij <linus.wall...@linaro.org>
---
ChangeLog v1->v2:
- Update license to GPLv2 or later + SPDX identifier
- Instantiate the DDC I2C as part of the class realization so
we do not need to add the DDC "on the side" in machines
using SII9022.
- Switch to using trace events instead of debug prints.
---
hw/display/Makefile.objs | 1 +
hw/display/sii9022.c | 188 +++
hw/display/trace-events | 5 ++
3 files changed, 194 insertions(+)
create mode 100644 hw/display/sii9022.c
diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
index d3a4cb396eb9..3c7c75b94da5 100644
--- a/hw/display/Makefile.objs
+++ b/hw/display/Makefile.objs
@@ -3,6 +3,7 @@ common-obj-$(CONFIG_VGA_CIRRUS) += cirrus_vga.o
common-obj-$(CONFIG_G364FB) += g364fb.o
common-obj-$(CONFIG_JAZZ_LED) += jazz_led.o
common-obj-$(CONFIG_PL110) += pl110.o
+common-obj-$(CONFIG_SII9022) += sii9022.o
common-obj-$(CONFIG_SSD0303) += ssd0303.o
common-obj-$(CONFIG_SSD0323) += ssd0323.o
common-obj-$(CONFIG_XEN) += xenfb.o
diff --git a/hw/display/sii9022.c b/hw/display/sii9022.c
new file mode 100644
index ..b019ac0ca880
--- /dev/null
+++ b/hw/display/sii9022.c
@@ -0,0 +1,188 @@
+/*
+ * Silicon Image SiI9022
+ *
+ * This is a pretty hollow emulation: all we do is acknowledge that we
+ * exist (chip ID) and confirm that we get switched over into DDC mode
+ * so the emulated host can proceed to read out EDID data. All subsequent
+ * set-up of connectors etc will be acknowledged and ignored.
+ *
+ * Copyright (C) 2018 Linus Walleij
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "hw/i2c/i2c.h"
+#include "hw/i2c/i2c-ddc.h"
+#include "trace.h"
+
+#define SII9022_SYS_CTRL_DATA 0x1a
+#define SII9022_SYS_CTRL_PWR_DWN 0x10
+#define SII9022_SYS_CTRL_AV_MUTE 0x08
+#define SII9022_SYS_CTRL_DDC_BUS_REQ 0x04
+#define SII9022_SYS_CTRL_DDC_BUS_GRTD 0x02
+#define SII9022_SYS_CTRL_OUTPUT_MODE 0x01
+#define SII9022_SYS_CTRL_OUTPUT_HDMI 1
+#define SII9022_SYS_CTRL_OUTPUT_DVI 0
+#define SII9022_REG_CHIPID 0x1b
+#define SII9022_INT_ENABLE 0x3c
+#define SII9022_INT_STATUS 0x3d
+#define SII9022_INT_STATUS_HOTPLUG 0x01;
+#define SII9022_INT_STATUS_PLUGGED 0x04;
+
+#define TYPE_SII9022 "sii9022"
+#define SII9022(obj) OBJECT_CHECK(sii9022_state, (obj), TYPE_SII9022)
+
+typedef struct sii9022_state {
+I2CSlave parent_obj;
+uint8_t ptr;
+bool addr_byte;
+bool ddc_req;
+bool ddc_skip_finish;
+bool ddc;
+} sii9022_state;
+
+static const VMStateDescription vmstate_sii9022 = {
+.name = "sii9022",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_I2C_SLAVE(parent_obj, sii9022_state),
+VMSTATE_UINT8(ptr, sii9022_state),
+VMSTATE_BOOL(addr_byte, sii9022_state),
+VMSTATE_BOOL(ddc_req, sii9022_state),
+VMSTATE_BOOL(ddc_skip_finish, sii9022_state),
+VMSTATE_BOOL(ddc, sii9022_state),
+VMSTATE_END_OF_LIST()
+}
+};
+
+static int sii9022_event(I2CSlave *i2c, enum i2c_event event)
+{
+sii9022_state *s = SII9022(i2c);
+
+switch (event) {
+case I2C_START_SEND:
+s->addr_byte = true;
+break;
+case I2C_START_RECV:
+break;
+case I2C_FINISH:
+break;
+case I2C_NACK:
+break;
+}
+
+return 0;
+}
+
+static int sii9022_rx(I2CSlave *i2c)
+{
+sii9022_state *s = SII9022(i2c);
+uint8_t res = 0x00;
+
+switch (s->ptr) {
+case SII9022_SYS_CTRL_DATA:
+if (s->ddc_req) {
+/* Acknowledge DDC bus request */
+res = SII9022_SYS_CTRL_DDC_BUS_GRTD | SII9022_SYS_CTRL_DDC_BUS_REQ;
+}
+break;
+case SII9022_REG_CHIPID:
+res = 0xb0;
+break;
+case SII9022_INT_STATUS:
+/* Something is cold-plugged in, no interrupts */
+res = SII9022_INT_STATUS_PLUGGED;
+break;
+default:
+break;
+}
+
+trace_sii9022_read_reg(s->ptr, res);
+s->ptr++;
+
+return res;
+}
+
+static int sii9022_tx(I2CSlave *i2c, uint8_t data)
+{
+sii9022_state *s = SII9022(i2c);
+
+if (s->addr_byte) {
+s->ptr = data;
+s->addr_byte = false;
+return 0;
+}
+
+switch (s->ptr) {
+case SII902
[Qemu-devel] [PATCH 1/5] i2c: Fix some brace style issues
From: Corey Minyard <cminy...@mvista.com>
Signed-off-by: Corey Minyard <cminy...@mvista.com>
Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
Signed-off-by: Linus Walleij <linus.wall...@linaro.org>
---
hw/i2c/core.c| 3 +--
include/hw/i2c/i2c.h | 6 ++
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 59068f157eb5..9a54b61c1dbd 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -19,8 +19,7 @@ struct I2CNode {
#define I2C_BROADCAST 0x00
-struct I2CBus
-{
+struct I2CBus {
BusState qbus;
QLIST_HEAD(, I2CNode) current_devs;
uint8_t saved_address;
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 24e95d015589..8fd449f645f0 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -25,8 +25,7 @@ typedef struct I2CSlave I2CSlave;
#define I2C_SLAVE_GET_CLASS(obj) \
OBJECT_GET_CLASS(I2CSlaveClass, (obj), TYPE_I2C_SLAVE)
-typedef struct I2CSlaveClass
-{
+typedef struct I2CSlaveClass {
DeviceClass parent_class;
/* Callbacks provided by the device. */
@@ -50,8 +49,7 @@ typedef struct I2CSlaveClass
int (*event)(I2CSlave *s, enum i2c_event event);
} I2CSlaveClass;
-struct I2CSlave
-{
+struct I2CSlave {
DeviceState qdev;
/* Remaining fields for internal use by the I2C code. */
--
2.14.3
[Qemu-devel] [PATCH 2/5] i2c: Move the bus class to i2c.h
From: Corey Minyard <cminy...@mvista.com>
Some devices need access to it.
Signed-off-by: Corey Minyard <cminy...@mvista.com>
Reviewed-by: Peter Maydell <peter.mayd...@linaro.org>
Signed-off-by: Linus Walleij <linus.wall...@linaro.org>
---
hw/i2c/core.c| 17 -
include/hw/i2c/i2c.h | 17 +
2 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 9a54b61c1dbd..cfccefca3d6c 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -10,30 +10,13 @@
#include "qemu/osdep.h"
#include "hw/i2c/i2c.h"
-typedef struct I2CNode I2CNode;
-
-struct I2CNode {
-I2CSlave *elt;
-QLIST_ENTRY(I2CNode) next;
-};
-
#define I2C_BROADCAST 0x00
-struct I2CBus {
-BusState qbus;
-QLIST_HEAD(, I2CNode) current_devs;
-uint8_t saved_address;
-bool broadcast;
-};
-
static Property i2c_props[] = {
DEFINE_PROP_UINT8("address", struct I2CSlave, address, 0),
DEFINE_PROP_END_OF_LIST(),
};
-#define TYPE_I2C_BUS "i2c-bus"
-#define I2C_BUS(obj) OBJECT_CHECK(I2CBus, (obj), TYPE_I2C_BUS)
-
static const TypeInfo i2c_bus_info = {
.name = TYPE_I2C_BUS,
.parent = TYPE_BUS,
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 8fd449f645f0..d727379b487a 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -56,6 +56,23 @@ struct I2CSlave {
uint8_t address;
};
+#define TYPE_I2C_BUS "i2c-bus"
+#define I2C_BUS(obj) OBJECT_CHECK(I2CBus, (obj), TYPE_I2C_BUS)
+
+typedef struct I2CNode I2CNode;
+
+struct I2CNode {
+I2CSlave *elt;
+QLIST_ENTRY(I2CNode) next;
+};
+
+struct I2CBus {
+BusState qbus;
+QLIST_HEAD(, I2CNode) current_devs;
+uint8_t saved_address;
+bool broadcast;
+};
+
I2CBus *i2c_init_bus(DeviceState *parent, const char *name);
void i2c_set_slave_address(I2CSlave *dev, uint8_t address);
int i2c_bus_busy(I2CBus *bus);
--
2.14.3
Re: [Qemu-devel] [Qemu-arm] [PATCH 2/3] hw/sii9022: Add support for Silicon Image SII9022
On Tue, Feb 27, 2018 at 11:09 AM, Peter Maydell
<peter.mayd...@linaro.org> wrote:
> On 27 February 2018 at 07:41, Linus Walleij <linus.wall...@linaro.org> wrote:
>> On Sat, Feb 17, 2018 at 7:32 PM, Philippe Mathieu-Daudé <f4...@amsat.org>
>> wrote:
>>
>>> [Me]
>>>> +#define DEBUG_SII9022 0
>>>> +
>>>> +#define DPRINTF(fmt, ...) \
>>>> +do { \
>>>> +if (DEBUG_SII9022) { \
>>>> +printf("sii9022: " fmt, ## __VA_ARGS__); \
>>>> +} \
>>>> +} while (0)
>>>
>>> Can you replace DPRINTF() by trace events?
>>
>> Absolutely but which ones?
>>
>> I do not feel senior enough to also invent new trace events
>> for displays or I2C devices...
>
> Just put a trace event where you've put DPRINTF debug statements.
Yeah, hm the question might be silly or something but I don't
know how to do that.
I guess I should include "trace.h".
#include "trace.h" says it is autogenerated from tracetool.
Is there some documentation I should be reading or a good
example commit I can study to get the hang of it?
Yours,
Linus Walleij
Re: [Qemu-devel] [Qemu-arm] [PATCH 2/3] hw/sii9022: Add support for Silicon Image SII9022
On Sat, Feb 17, 2018 at 7:32 PM, Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
> [Me]
>> +#define DEBUG_SII9022 0
>> +
>> +#define DPRINTF(fmt, ...) \
>> +do { \
>> +if (DEBUG_SII9022) { \
>> +printf("sii9022: " fmt, ## __VA_ARGS__); \
>> +} \
>> +} while (0)
>
> Can you replace DPRINTF() by trace events?
Absolutely but which ones?
I do not feel senior enough to also invent new trace events
for displays or I2C devices...
Yours,
Linus Walleij
Re: [Qemu-devel] [PATCH 2/2] i2c: Move the bus class to i2c.h
On Mon, Feb 19, 2018 at 4:20 PM, <miny...@acm.org> wrote: > From: Corey Minyard <cminy...@mvista.com> > > Some devices need access to it. > > Signed-off-by: Corey Minyard <cminy...@mvista.com> Reviewed-by: Linus Walleij <linus.wall...@linaro.org> Yours, Linus Walleij
Re: [Qemu-devel] [PATCH 2/2] i2c: Move the bus class to i2c.h
On Thu, Feb 22, 2018 at 4:44 PM, Peter Maydell <peter.mayd...@linaro.org> wrote: > On 22 February 2018 at 15:39, Linus Walleij <linus.wall...@linaro.org> wrote: >> On Tue, Feb 20, 2018 at 2:06 PM, Corey Minyard <miny...@acm.org> wrote: >>> Linus, Philippe, do you want me to submit this, or do you want >>> to take it? You can pull it from: >>> >>> https://github.com/cminyard/qemu.git tags/i2c-bus-move >> >> I don't have any commit rights, if you can push this to the >> QEMU master I will happily rebase my stuff and resubmit :) > > I suggest you take Corey's 2 patches, add them to the front > of your patchset and add your signed-off-by line to them. > That way you don't have to wait for them to go into git master. OK no problem! Yours, Linus Walleij
Re: [Qemu-devel] [PATCH 2/2] i2c: Move the bus class to i2c.h
On Tue, Feb 20, 2018 at 2:06 PM, Corey Minyard <miny...@acm.org> wrote: > On 02/19/2018 09:25 AM, Peter Maydell wrote: >> >> On 19 February 2018 at 15:20, <miny...@acm.org> wrote: >>> >>> From: Corey Minyard <cminy...@mvista.com> >>> >>> Some devices need access to it. >>> >>> Signed-off-by: Corey Minyard <cminy...@mvista.com> >>> --- >>> hw/i2c/core.c| 17 - >>> include/hw/i2c/i2c.h | 17 + >>> 2 files changed, 17 insertions(+), 17 deletions(-) >> >> Reviewed-by: Peter Maydell <peter.mayd...@linaro.org> > > > Linus, Philippe, do you want me to submit this, or do you want > to take it? You can pull it from: > > https://github.com/cminyard/qemu.git tags/i2c-bus-move I don't have any commit rights, if you can push this to the QEMU master I will happily rebase my stuff and resubmit :) Yours, Linus Walleij
Re: [Qemu-devel] [PATCH 1/2] i2c: Fix some brace style issues
On Mon, Feb 19, 2018 at 4:20 PM, <miny...@acm.org> wrote: > From: Corey Minyard <cminy...@mvista.com> > > Signed-off-by: Corey Minyard <cminy...@mvista.com> Reviewed-by: Linus Walleij <linus.wall...@linaro.org> Yours, Linus Walleij
Re: [Qemu-devel] [PATCH 3/3] arm/vexpress: Add proper display connector emulation
On Sat, Feb 17, 2018 at 7:28 PM, Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
> On 02/17/2018 11:00 AM, Linus Walleij wrote:
>> The assignment of the SiI9022 at address 0x39 and the EDID
>> DDC I2C at address 0x50 is not strictly correct: the DDC I2C
>> is there all the time but in the actual component it only
>> appears once activated inside the SiI9022, so ideally it should
>> be added and removed to the bus by the SiI9022. However for this
>> purpose it works fine to just have it around.
>
> This seems easier to just do it now rather than postpone :)
>
> In your patch #2:
>
> static void sii9022_realize(DeviceState *dev, Error **errp)
> {
> I2CBus *bus;
>
> bus = I2C_BUS(qdev_get_parent_bus(dev));
> i2c_create_slave(bus, TYPE_I2CDDC, 0x50);
> }
I tried an approach like this but the I2C_BUS() macro was not
publicly visible in any header file and trying to pry it out just
brought along a lot of refactoring in the i2c core.c file that
scared me away...
Has someone else fixed it already?
Yours,
Linus Walleij
[Qemu-devel] [PATCH 3/3] arm/vexpress: Add proper display connector emulation
This adds the SiI9022 and EDID I2C devices to the ARM Versatile
Express machine, and selects the two I2C devices necessary in the
arm-softmmy.mak configuration so everything will build smoothly.
I am implementing proper handling of the graphics in the Linux
kernel and adding proper emulation of SiI9022 and EDID makes the
driver probe as nicely as before, retrieveing the resolutions
supported by the "QEMU monitor" and overall just working nice.
The assignment of the SiI9022 at address 0x39 and the EDID
DDC I2C at address 0x50 is not strictly correct: the DDC I2C
is there all the time but in the actual component it only
appears once activated inside the SiI9022, so ideally it should
be added and removed to the bus by the SiI9022. However for this
purpose it works fine to just have it around.
Signed-off-by: Linus Walleij <linus.wall...@linaro.org>
---
default-configs/arm-softmmu.mak | 2 ++
hw/arm/vexpress.c | 7 ++-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index ca34cf446242..54f855d07206 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -21,6 +21,8 @@ CONFIG_STELLARIS_INPUT=y
CONFIG_STELLARIS_ENET=y
CONFIG_SSD0303=y
CONFIG_SSD0323=y
+CONFIG_DDC=y
+CONFIG_SII9022=y
CONFIG_ADS7846=y
CONFIG_MAX111X=y
CONFIG_SSI=y
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index dc5928ae1ab5..d6c912c97684 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -29,6 +29,7 @@
#include "hw/arm/arm.h"
#include "hw/arm/primecell.h"
#include "hw/devices.h"
+#include "hw/i2c/i2c.h"
#include "net/net.h"
#include "sysemu/sysemu.h"
#include "hw/boards.h"
@@ -537,6 +538,7 @@ static void vexpress_common_init(MachineState *machine)
uint32_t sys_id;
DriveInfo *dinfo;
pflash_t *pflash0;
+I2CBus *i2c;
ram_addr_t vram_size, sram_size;
MemoryRegion *sysmem = get_system_memory();
MemoryRegion *vram = g_new(MemoryRegion, 1);
@@ -628,7 +630,10 @@ static void vexpress_common_init(MachineState *machine)
sysbus_create_simple("sp804", map[VE_TIMER01], pic[2]);
sysbus_create_simple("sp804", map[VE_TIMER23], pic[3]);
-/* VE_SERIALDVI: not modelled */
+dev = sysbus_create_simple("versatile_i2c", map[VE_SERIALDVI], NULL);
+i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
+i2c_create_slave(i2c, "sii9022", 0x39);
+i2c_create_slave(i2c, "i2c-ddc", 0x50);
sysbus_create_simple("pl031", map[VE_RTC], pic[4]); /* RTC */
--
2.14.3
[Qemu-devel] [PATCH 2/3] hw/sii9022: Add support for Silicon Image SII9022
This adds support for emulating the Silicon Image SII9022 DVI/HDMI
bridge. It's not very clever right now, it just acknowledges
the switch into DDC I2C mode and back. Combining this with the
existing DDC I2C emulation gives the right behavior on the Versatile
Express emulation passing through the QEMU EDID to the emulated
platform.
Signed-off-by: Linus Walleij <linus.wall...@linaro.org>
---
hw/display/Makefile.objs | 1 +
hw/display/sii9022.c | 185 +++
2 files changed, 186 insertions(+)
create mode 100644 hw/display/sii9022.c
diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
index d3a4cb396eb9..3c7c75b94da5 100644
--- a/hw/display/Makefile.objs
+++ b/hw/display/Makefile.objs
@@ -3,6 +3,7 @@ common-obj-$(CONFIG_VGA_CIRRUS) += cirrus_vga.o
common-obj-$(CONFIG_G364FB) += g364fb.o
common-obj-$(CONFIG_JAZZ_LED) += jazz_led.o
common-obj-$(CONFIG_PL110) += pl110.o
+common-obj-$(CONFIG_SII9022) += sii9022.o
common-obj-$(CONFIG_SSD0303) += ssd0303.o
common-obj-$(CONFIG_SSD0323) += ssd0323.o
common-obj-$(CONFIG_XEN) += xenfb.o
diff --git a/hw/display/sii9022.c b/hw/display/sii9022.c
new file mode 100644
index ..d6f3cdc04293
--- /dev/null
+++ b/hw/display/sii9022.c
@@ -0,0 +1,185 @@
+/*
+ * Silicon Image SiI9022
+ *
+ * This is a pretty hollow emulation: all we do is acknowledge that we
+ * exist (chip ID) and confirm that we get switched over into DDC mode
+ * so the emulated host can proceed to read out EDID data. All subsequent
+ * set-up of connectors etc will be acknowledged and ignored.
+ *
+ * Copyright (c) 2018 Linus Walleij
+ *
+ * This code is licensed under the GNU GPL v2.
+ *
+ * Contributions after 2012-01-13 are licensed under the terms of the
+ * GNU GPL, version 2 or (at your option) any later version.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "hw/i2c/i2c.h"
+
+#define DEBUG_SII9022 0
+
+#define DPRINTF(fmt, ...) \
+do { \
+if (DEBUG_SII9022) { \
+printf("sii9022: " fmt, ## __VA_ARGS__); \
+} \
+} while (0)
+
+#define SII9022_SYS_CTRL_DATA 0x1a
+#define SII9022_SYS_CTRL_PWR_DWN 0x10
+#define SII9022_SYS_CTRL_AV_MUTE 0x08
+#define SII9022_SYS_CTRL_DDC_BUS_REQ 0x04
+#define SII9022_SYS_CTRL_DDC_BUS_GRTD 0x02
+#define SII9022_SYS_CTRL_OUTPUT_MODE 0x01
+#define SII9022_SYS_CTRL_OUTPUT_HDMI 1
+#define SII9022_SYS_CTRL_OUTPUT_DVI 0
+#define SII9022_REG_CHIPID 0x1b
+#define SII9022_INT_ENABLE 0x3c
+#define SII9022_INT_STATUS 0x3d
+#define SII9022_INT_STATUS_HOTPLUG 0x01;
+#define SII9022_INT_STATUS_PLUGGED 0x04;
+
+#define TYPE_SII9022 "sii9022"
+#define SII9022(obj) OBJECT_CHECK(sii9022_state, (obj), TYPE_SII9022)
+
+typedef struct sii9022_state {
+I2CSlave parent_obj;
+uint8_t ptr;
+bool addr_byte;
+bool ddc_req;
+bool ddc_skip_finish;
+bool ddc;
+} sii9022_state;
+
+static const VMStateDescription vmstate_sii9022 = {
+.name = "sii9022",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_I2C_SLAVE(parent_obj, sii9022_state),
+VMSTATE_UINT8(ptr, sii9022_state),
+VMSTATE_BOOL(addr_byte, sii9022_state),
+VMSTATE_BOOL(ddc_req, sii9022_state),
+VMSTATE_BOOL(ddc_skip_finish, sii9022_state),
+VMSTATE_BOOL(ddc, sii9022_state),
+VMSTATE_END_OF_LIST()
+}
+};
+
+static int sii9022_event(I2CSlave *i2c, enum i2c_event event)
+{
+sii9022_state *s = SII9022(i2c);
+
+switch (event) {
+case I2C_START_SEND:
+s->addr_byte = true;
+break;
+case I2C_START_RECV:
+break;
+case I2C_FINISH:
+break;
+case I2C_NACK:
+break;
+}
+
+return 0;
+}
+
+static int sii9022_rx(I2CSlave *i2c)
+{
+sii9022_state *s = SII9022(i2c);
+uint8_t res = 0x00;
+
+switch (s->ptr) {
+case SII9022_SYS_CTRL_DATA:
+if (s->ddc_req) {
+/* Acknowledge DDC bus request */
+res = SII9022_SYS_CTRL_DDC_BUS_GRTD | SII9022_SYS_CTRL_DDC_BUS_REQ;
+}
+break;
+case SII9022_REG_CHIPID:
+res = 0xb0;
+break;
+case SII9022_INT_STATUS:
+/* Something is cold-plugged in, no interrupts */
+res = SII9022_INT_STATUS_PLUGGED;
+break;
+default:
+break;
+}
+DPRINTF("%02x read from %02x\n", res, s->ptr);
+s->ptr++;
+
+return res;
+}
+
+static int sii9022_tx(I2CSlave *i2c, uint8_t data)
+{
+sii9022_state *s = SII9022(i2c);
+
+if (s->addr_byte) {
+s->ptr = data;
+s->addr_byte = false;
+return 0;
+}
+
+switch (s->ptr) {
+case SII9022_SYS_CTRL_DATA:
+if (data & SII9022_SYS_CTRL_DDC_BUS_REQ) {
+s->ddc_req = true;
+if (data & SII9022_SYS_CTRL_DDC_BUS_GRTD) {
+s->ddc = true;
+/* S
[Qemu-devel] [PATCH 1/3] hw/i2c-ddc: Do not fail writes
The tx function of the DDC I2C slave emulation was returning 1
on all writes resulting in NACK in the I2C bus. Changing it to
0 makes the DDC I2C work fine with bit-banged I2C such as the
versatile I2C.
I guess it was not affecting whatever I2C controller this was
used with until now, but with the Versatile I2C it surely
does not work.
Signed-off-by: Linus Walleij <linus.wall...@linaro.org>
---
hw/i2c/i2c-ddc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/i2c/i2c-ddc.c b/hw/i2c/i2c-ddc.c
index 199dac9e41c1..bec0c91e2dd0 100644
--- a/hw/i2c/i2c-ddc.c
+++ b/hw/i2c/i2c-ddc.c
@@ -259,12 +259,12 @@ static int i2c_ddc_tx(I2CSlave *i2c, uint8_t data)
s->reg = data;
s->firstbyte = false;
DPRINTF("[EDID] Written new pointer: %u\n", data);
-return 1;
+return 0;
}
/* Ignore all writes */
s->reg++;
-return 1;
+return 0;
}
static void i2c_ddc_init(Object *obj)
--
2.14.3
[Qemu-devel] [PATCH] pl110: Implement vertical compare/next base interrupts
This implements rudimentary support for interrupt generation on the
PL110. I am working on a new DRI/KMS driver for Linux and since that
uses the blanking interrupt, we need something to fire here. Without
any interrupt support Linux waits for a while and then gives ugly
messages about the vblank not working in the console (it does not
hang perpetually or anything though, DRI is pretty forgiving).
I solved it for now by setting up a timer to fire at 60Hz and pull
the interrupts for "vertical compare" and "next memory base"
at this interval. This works fine and fires roughly the same number
of IRQs on QEMU as on the hardware and leaves the console clean
and nice.
People who want to create more accurate emulation can probably work
on top of this if need be. It is certainly closer to the hardware
behaviour than what we have today anyway.
Cc: Peter Maydell <peter.mayd...@linaro.org>
Signed-off-by: Linus Walleij <linus.wall...@linaro.org>
---
hw/display/pl110.c | 27 ++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/hw/display/pl110.c b/hw/display/pl110.c
index 8c7dcc6f0a69..777bb3f44503 100644
--- a/hw/display/pl110.c
+++ b/hw/display/pl110.c
@@ -12,6 +12,7 @@
#include "ui/console.h"
#include "framebuffer.h"
#include "ui/pixel_ops.h"
+#include "qemu/timer.h"
#include "qemu/log.h"
#define PL110_CR_EN 0x001
@@ -19,6 +20,8 @@
#define PL110_CR_BEBO 0x200
#define PL110_CR_BEPO 0x400
#define PL110_CR_PWR 0x800
+#define PL110_IE_NB 0x004
+#define PL110_IE_VC 0x008
enum pl110_bppmode
{
@@ -50,6 +53,7 @@ typedef struct PL110State {
MemoryRegion iomem;
MemoryRegionSection fbsection;
QemuConsole *con;
+QEMUTimer *vblank_timer;
int version;
uint32_t timing[4];
@@ -320,7 +324,23 @@ static void pl110_resize(PL110State *s, int width, int
height)
/* Update interrupts. */
static void pl110_update(PL110State *s)
{
- /* TODO: Implement interrupts. */
+/* Raise IRQ if enabled and any status bit is 1 */
+if (s->int_status & s->int_mask) {
+qemu_irq_raise(s->irq);
+} else {
+qemu_irq_lower(s->irq);
+}
+}
+
+static void pl110_vblank_interrupt(void *opaque)
+{
+PL110State *s = opaque;
+
+/* Fire the vertical compare and next base IRQs and re-arm */
+s->int_status |= (PL110_IE_NB | PL110_IE_VC);
+timer_mod(s->vblank_timer,
+ qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + NANOSECONDS_PER_SECOND /
60);
+pl110_update(s);
}
static uint64_t pl110_read(void *opaque, hwaddr offset,
@@ -429,6 +449,10 @@ static void pl110_write(void *opaque, hwaddr offset,
s->bpp = (val >> 1) & 7;
if (pl110_enabled(s)) {
qemu_console_resize(s->con, s->cols, s->rows);
+timer_mod(s->vblank_timer,
+ qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
NANOSECONDS_PER_SECOND / 60);
+} else {
+timer_del(s->vblank_timer);
}
break;
case 10: /* LCDICR */
@@ -474,6 +498,7 @@ static void pl110_realize(DeviceState *dev, Error **errp)
memory_region_init_io(>iomem, OBJECT(s), _ops, s, "pl110",
0x1000);
sysbus_init_mmio(sbd, >iomem);
sysbus_init_irq(sbd, >irq);
+s->vblank_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, pl110_vblank_interrupt,
s);
qdev_init_gpio_in(dev, pl110_mux_ctrl_set, 1);
s->con = graphic_console_init(dev, 0, _gfx_ops, s);
}
--
2.14.3
Re: [Qemu-devel] [PATCH for-1.5 0/3] hw/pci-host/versatile: Fix issues with newer kernels
On Thu, May 16, 2013 at 6:58 PM, Arnd Bergmann a...@arndb.de wrote: FWIW, I plan to really get this done in the kernel for 3.11 properly and rework the entire versatile and realview code base to work without any platform specific code in arch/arm. Sweet! The plan is to use the new infrastructure for PCI and put that code into drivers/pci/host, and have it scan the hardware using DT only. We can have a backwards compatibility setup for versatile-pb without DT, but in the long run, I would prefer to kill off that boot option. Do we have this on a topic branch in ARM SoC now? I need a baseline to send a pull request for my cleanup of the Integrator PCI, and I'd probably like to move the PCIv3 controller as well if I can use the same baseline as you for this. Yours, Linus Walleij
Re: [Qemu-devel] [PATCH for-1.5 0/3] hw/pci-host/versatile: Fix issues with newer kernels
On Tue, May 14, 2013 at 5:33 PM, Peter Maydell peter.mayd...@linaro.org wrote: The reworking of the versatile PCI controller model so that it actually behaved like hardware included an attempt to autodetect whether the guest Linux kernel was assuming the old broken behaviour. Unfortunately it turns out that there are several different variant broken kernels which behave slightly differently (though none of them will work on real hardware). The first two patches in this series improve the autodetection so that we will work out of the box on more kernels. The third patch adds a property for forcing the behaviour, so that if there are further cases we didn't know about, at least users have a command line workaround they can enable. This looks like a viable approach to me. So FWIW: Acked-by: Linus Walleij linus.wall...@linaro.org Yours, Linus Walleij
