linux-kernel - Re: [PATCH] virtio: console: Prepare for making REMOTEPROC modular

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <uo3h33wijb7mhjwgugpkjhqg7wusz6tpkh5u5fxbsxp3kzpq75@ggsdroemmdmj>
Date: Fri, 14 Feb 2025 12:14:21 +0100
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Amit Shah <amit@...nel.org>
Cc: Arnd Bergmann <arnd@...db.de>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, virtualization@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] virtio: console: Prepare for making REMOTEPROC modular

Hello Amit,

On Fri, Feb 14, 2025 at 11:58:44AM +0100, Amit Shah wrote:
> On Thu, 2025-02-13 at 12:55 +0100, Uwe Kleine-König wrote:
> > virtio_console.c can make use of REMOTEPROC. Therefore it has several
> > tests evaluating
> > 
> > 	IS_ENABLED(CONFIG_REMOTEPROC)
> > 
> > . This currently only does the right thing because CONFIG_REMOTEPROC
> > cannot be modular. Otherwise the configuration
> > 
> > 	CONFIG_REMOTEPROC=m
> > 	CONFIG_VIRTIO_CONSOLE=y
> > 
> > would result in a build failure because then
> > IS_ENABLED(CONFIG_REMOTEPROC) evaluates to true but still the built-
> > in
> > virtio_console.o must not use symbols from the remoteproc module.
> > 
> > To prepare for making REMOTEPROC modular change the tests to use
> > IS_REACHABLE() instead of IS_ENABLED() which copes correctly for the
> > above case as it evaluates to false then.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@...libre.com>
> > ---
> > Hello,
> > 
> > I didn't check what else needs to be done to make CONFIG_REMOTEPROC
> > tristate but even if it stays a bool using IS_REACHABLE() is still
> > the
> > better choice.
> 
> It might lead to a false sense of "better" -- the value of IS_ENABLED
> is cached in a variable which is determined at compile-time.

Either I don't understand what you mean, or this is wrong.

	$ make allmodconfig drivers/char/virtio_console.i
	$ grep CONFIG_REMOTEPROC= .config
	CONFIG_REMOTEPROC=m
	$ cat drivers/char/virtio_console.i
	...
	static bool is_rproc_serial(const struct virtio_device *vdev)
	{
	 return 1 && vdev->id.device == 11;
	}
	...

so is_rproc_enabled is still constant and known at compile time.

> That
> caching, after this change, moves to driver init-time.  If the rproc
> module is loaded after virtio-console is initialized, there's no way
> it's going to be used.

If both are modular, modprobe should make sure that rproc is ready
before virtio-console. If virtio-console is builtin and rproc is
modular, IS_REACHABLE(CONFIG_REMOTEPROC) evaluates to false and so rproc
won't be used. (As is the case already today with CONFIG_REMOTEPROC=n).

> Only if the rproc module is loaded before
> virtio-console will the rproc functionality be used -- which means that
> nothing changed in reality..

With that patch indeed nothing changed yet, because CONFIG_REMOTEPROC
cannot be =m today. Until this changes IS_REACHABLE() and IS_ENABLED()
are equivalent.
 
> To properly detect and use rproc if available would need the rproc
> initialization out of virtcons_probe() and into something that happens
> either via sysfs for existing ports, or when adding a new port to a
> device.  However, the current spec doesn't allow for that, so some more
> changes will need to be made to ensure current backwards compat, and a
> new specification that allows for a late init of rproc.

I didn't understand that and hope it's irrelevant with the things I
wrote above.

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)