linux-kernel - Re: [PATCH v3 1/7] swiotlb: make io_tlb_default

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: <20230627131100.661d5297@meshulam.tesarici.cz>
Date:   Tue, 27 Jun 2023 13:11:00 +0200
From:   Petr Tesařík <petr@...arici.cz>
To:     Robin Murphy <robin.murphy@....com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Petr Tesarik <petrtesarik@...weicloud.com>,
        Stefano Stabellini <sstabellini@...nel.org>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Juergen Gross <jgross@...e.com>,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@...m.com>,
        Christoph Hellwig <hch@....de>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Kees Cook <keescook@...omium.org>,
        Saravana Kannan <saravanak@...gle.com>,
        "moderated list:XEN HYPERVISOR ARM" <xen-devel@...ts.xenproject.org>,
        "moderated list:ARM PORT" <linux-arm-kernel@...ts.infradead.org>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:MIPS" <linux-mips@...r.kernel.org>,
        "open list:XEN SWIOTLB SUBSYSTEM" <iommu@...ts.linux.dev>,
        Roberto Sassu <roberto.sassu@...weicloud.com>,
        Kefeng Wang <wangkefeng.wang@...wei.com>
Subject: Re: [PATCH v3 1/7] swiotlb: make io_tlb_default_mem local to
 swiotlb.c

On Tue, 27 Jun 2023 11:55:00 +0100
Robin Murphy <robin.murphy@....com> wrote:

> On 27/06/2023 11:24 am, Greg Kroah-Hartman wrote:
> > On Tue, Jun 27, 2023 at 11:54:23AM +0200, Petr Tesarik wrote:  
> >> +/**
> >> + * is_swiotlb_active() - check if the software IO TLB is initialized
> >> + * @dev:	Device to check, or %NULL for the default IO TLB.
> >> + */
> >>   bool is_swiotlb_active(struct device *dev)
> >>   {
> >> -	struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> >> +	struct io_tlb_mem *mem = dev
> >> +		? dev->dma_io_tlb_mem
> >> +		: &io_tlb_default_mem;  
> > 
> > That's impossible to read and maintain over time, sorry.
> > 
> > Please use real "if () else" lines, so that it can be maintained over
> > time.  
> 
> Moreover, it makes for a horrible interface anyway. If there's a need 
> for a non-specific "is SWIOTLB present at all?" check unrelated to any 
> particular device (which arguably still smells of poking into 
> implementation details...), please encapsulate it in its own distinct 
> helper like, say, is_swiotlb_present(void).
> 
> However, the more I think about it, the more I doubt that logic like 
> octeon_pci_setup() can continue to work properly at all if SWIOTLB 
> allocation becomes dynamic... :/

Good, so I'm not alone. I don't know enough of the Octeon hardware to
understand how much magic is behind these PCI BARs and why one of them
should be (sometimes) programmed the way it is.

OTOH it doesn't seem to me that this platform forces DMA through
SWIOTLB. At least all calls to swiotlb_init() under arch/mips take this
form:

	swiotlb_init(true, SWIOTLB_VERBOSE);

This makes me believe that this PCI BAR setup is merely an optimization.

However, if nobody has a clear answer, a fallback solution is to stay
on the safe side and add a flag to struct io_tlb_mem whether SWIOTLB
can grow dynamically. The helper function would then set this flag and
make sure that on this Octeon platform, the SWIOTLB stays restricted to
the default pool.

Hopefully, Thomas Bogendoerfer can shed some light on that code.

Petr T