linux-kernel - Re: [PATCH v8 3/5] drm: handle HAS

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 for Android: free password hash cracker in your pocket

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

Message-ID: <eptfarsehuuiulqz5523xu7h26jvb365rd3u5mx3mmubw74uld@53ebnpkpq6sc>
Date: Thu, 24 Oct 2024 11:30:40 +0200
From: Niklas Schnelle <schnelle@...ux.ibm.com>
To: Thomas Zimmermann <tzimmermann@...e.de>, Arnd Bergmann <arnd@...db.de>,
        Brian Cain <bcain@...cinc.com>, Marcel Holtmann <marcel@...tmann.org>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>,
        Patrik Jakobsson <patrik.r.jakobsson@...il.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>, Dave Airlie <airlied@...il.com>,
        Simona Vetter <simona@...ll.ch>, Dave Airlie <airlied@...hat.com>,
        Gerd Hoffmann <kraxel@...hat.com>,
        Lucas De Marchi <lucas.demarchi@...el.com>,
        Thomas Hellström <thomas.hellstrom@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        "Maciej W. Rozycki" <macro@...am.me.uk>,
        Heiko Carstens <hca@...ux.ibm.com>
Cc: linux-kernel@...r.kernel.org, linux-hexagon@...r.kernel.org,
        linux-bluetooth@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        virtualization@...ts.linux.dev, spice-devel@...ts.freedesktop.org,
        intel-xe@...ts.freedesktop.org, linux-serial@...r.kernel.org,
        Linux-Arch <linux-arch@...r.kernel.org>,
        Arnd Bergmann <arnd@...nel.org>
Subject: Re: [PATCH v8 3/5] drm: handle HAS_IOPORT dependencies

On Mon, Oct 21, 2024 at 01:18:20PM +0200, Niklas Schnelle wrote:
> On Mon, 2024-10-21 at 12:58 +0200, Thomas Zimmermann wrote:
> > Hi
> > 
> > Am 21.10.24 um 12:08 schrieb Arnd Bergmann:
> > > On Mon, Oct 21, 2024, at 07:52, Thomas Zimmermann wrote:
> > > > Am 08.10.24 um 14:39 schrieb Niklas Schnelle:
> > > d 100644
> > > > > --- a/drivers/gpu/drm/qxl/Kconfig
> > > > > +++ b/drivers/gpu/drm/qxl/Kconfig
> > > > > @@ -2,6 +2,7 @@
> > > > >    config DRM_QXL
> > > > >    	tristate "QXL virtual GPU"
> > > > >    	depends on DRM && PCI && MMU
> > > > > +	depends on HAS_IOPORT
> > > > Is there a difference between this style (multiple 'depends on') and the
> > > > one used for gma500 (&& && &&)?
> > > No, it's the same. Doing it in one line is mainly useful
> > > if you have some '||' as well.
> > > 
> > > > > @@ -105,7 +106,9 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val)
> > > > >    
> > > > >    		writeb(val, bochs->mmio + offset);
> > > > >    	} else {
> > > > > +#ifdef CONFIG_HAS_IOPORT
> > > > >    		outb(val, ioport);
> > > > > +#endif
> > > > Could you provide empty defines for the out() interfaces at the top of
> > > > the file?
> > > That no longer works since there are now __compiletime_error()
> > > versions of these funcitons. However we can do it more nicely like:
> > > 
> > > diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
> > > index 9b337f948434..034af6e32200 100644
> > > --- a/drivers/gpu/drm/tiny/bochs.c
> > > +++ b/drivers/gpu/drm/tiny/bochs.c
> > > @@ -112,14 +112,12 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val)
> > >   	if (WARN_ON(ioport < 0x3c0 || ioport > 0x3df))
> > >   		return;
> > >   
> > > -	if (bochs->mmio) {
> > > +	if (!IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio) {
> > >   		int offset = ioport - 0x3c0 + 0x400;
> > >   
> > >   		writeb(val, bochs->mmio + offset);
> > >   	} else {
> > > -#ifdef CONFIG_HAS_IOPORT
> > >   		outb(val, ioport);
> > > -#endif
> > >   	}
> > 
> > For all functions with such a pattern, could we use:
> > 
> > bool bochs_uses_mmio(bochs)
> > {
> >      return !IS_DEFINED(CONFIG_HAS_IOPORT) || bochs->mmio
> > }
> > 
> > void writeb_func()
> > {
> >      if (bochs_uses_mmio()) {
> >        writeb()
> > #if CONFIG_HAS_IOPORT
> >      } else {
> >        outb()
> > #endif
> >      }
> > }
> > 
> 
> I think if the helper were __always_inline we could still take
> advantage of the dead code elimination and combine this with Arnd's
> approach. Though I feel like it is a bit odd to try to do the MMIO
> approach despite bochs->mmio being false on !HAS_IOPORT systems.
> Is that what you wanted to correct by keeping the #ifdef
> CONFIG_HAS_IOPORT around the else? And yes the warning makes sense to
> me too.

Working on this now, I think we don't need a warning in the bochs_uses_mmio()
helper because we should never get here with !IS_ENABLED(CONFIG_HAS_IOPORT)
at runtime thanks to the check added in bochs_hw_init(). This also takes
care of my original worry that we might try writeb()/readb() with an invalid
bochs->mmio value. I'll sent a v9 with the helper added and #ifdefs's removed.

Thanks,
Niklas