Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220611082514.37112-5-dongli.zhang@oracle.com>
 <20220820012031.1285979-1-yuzhao@google.com> <f8c743d8-fcbe-4ef7-5f86-d63086552ffd@arm.com>
 <YwNn92WP3rP4ylZu@infradead.org> <d5016c1e-55d9-4224-278a-50377d4c6454@arm.com>
 <82d5b78d-e027-316a-87de-f76f4383d736@oracle.com>
In-Reply-To: <82d5b78d-e027-316a-87de-f76f4383d736@oracle.com>
From:   Yu Zhao <yuzhao@google.com>
Date:   Mon, 22 Aug 2022 17:10:51 -0600
Message-ID: <CAOUHufYnFCqfZES1XF=nCbxTevGMVMqhNY-XOqR2xo_WWTwQbw@mail.gmail.com>
Subject: Re: [PATCH v1 4/4] swiotlb: panic if nslabs is too small
To:     Dongli Zhang <dongli.zhang@oracle.com>
Cc:     Robin Murphy <robin.murphy@arm.com>,
        Christoph Hellwig <hch@infradead.org>,
        Andi Kleen <ak@linux.intel.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        alexander.sverdlin@nokia.com, andi.kleen@intel.com,
        Borislav Petkov <bp@alien8.de>, bp@suse.de,
        cminyard@mvista.com, Jonathan Corbet <corbet@lwn.net>,
        damien.lemoal@opensource.wdc.com,
        Dave Hansen <dave.hansen@linux.intel.com>,
        iommu@lists.linux-foundation.org, joe.jin@oracle.com,
        joe@perches.com, Kees Cook <keescook@chromium.org>,
        "Shutemov, Kirill" <kirill.shutemov@intel.com>, kys@microsoft.com,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
        linux-hyperv@vger.kernel.org,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-mips@vger.kernel.org, ltykernel@gmail.com,
        michael.h.kelley@microsoft.com, Ingo Molnar <mingo@redhat.com>,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        parri.andrea@gmail.com, "Paul E . McKenney" <paulmck@kernel.org>,
        pmladek@suse.com, Randy Dunlap <rdunlap@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>, thomas.lendacky@amd.com,
        Tianyu.Lan@microsoft.com, tsbogend@alpha.franken.de,
        vkuznets@redhat.com, wei.liu@kernel.org,
        "the arch/x86 maintainers" <x86@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Aug 22, 2022 at 4:28 PM Dongli Zhang <dongli.zhang@oracle.com> wrote:
>
> Hi Yu, Robin and Christoph,
>
> The mips kernel panic because the SWIOTLB buffer is adjusted to a very small
> value (< 1MB, or < 512-slot), so that the swiotlb panic on purpose.
>
> software IO TLB: SWIOTLB bounce buffer size adjusted to 0MB
> software IO TLB: area num 1.
> Kernel panic - not syncing: swiotlb_init_remap: nslabs = 128 too small
>
>
> From mips code, the 'swiotlbsize' is set to PAGE_SIZE initially. It is always
> PAGE_SIZE unless it is used by CONFIG_PCI or CONFIG_USB_OHCI_HCD_PLATFORM.
>
> Finally, the swiotlb panic on purpose.
>
> 189 void __init plat_swiotlb_setup(void)
> 190 {
> ... ...
> 211         swiotlbsize = PAGE_SIZE;
> 212
> 213 #ifdef CONFIG_PCI
> 214         /*
> 215          * For OCTEON_DMA_BAR_TYPE_SMALL, size the iotlb at 1/4 memory
> 216          * size to a maximum of 64MB
> 217          */
> 218         if (OCTEON_IS_MODEL(OCTEON_CN31XX)
> 219             || OCTEON_IS_MODEL(OCTEON_CN38XX_PASS2)) {
> 220                 swiotlbsize = addr_size / 4;
> 221                 if (swiotlbsize > 64 * (1<<20))
> 222                         swiotlbsize = 64 * (1<<20);
> 223         } else if (max_addr > 0xf0000000ul) {
> 224                 /*
> 225                  * Otherwise only allocate a big iotlb if there is
> 226                  * memory past the BAR1 hole.
> 227                  */
> 228                 swiotlbsize = 64 * (1<<20);
> 229         }
> 230 #endif
> 231 #ifdef CONFIG_USB_OHCI_HCD_PLATFORM
> 232         /* OCTEON II ohci is only 32-bit. */
> 233         if (OCTEON_IS_OCTEON2() && max_addr >= 0x100000000ul)
> 234                 swiotlbsize = 64 * (1<<20);
> 235 #endif
> 236
> 237         swiotlb_adjust_size(swiotlbsize);
> 238         swiotlb_init(true, SWIOTLB_VERBOSE);
> 239 }
>
>
> Here are some thoughts. Would you mind suggesting which is the right way to go?
>
> 1. Will the PAGE_SIZE swiotlb be used by mips when it is only PAGE_SIZE? If it
> is not used, why not disable swiotlb completely in the code?
>
> 2. The swiotlb panic on purpose when it is less then 1MB. Should we remove that
> limitation?
>
> 3. ... or explicitly declare the limitation that: "swiotlb should be at least
> 1MB, otherwise please do not use it"?
>
>
> The reason I add the panic on purpose is for below case:
>
> The user's kernel is configured with very small swiotlb buffer. As a result, the
> device driver may work abnormally.

Which driver? This sounds like that driver is broken, and we should
fix that driver.

> As a result, the issue is reported to a
> specific driver's developers, who spend some time to confirm it is swiotlb
> issue.

Is this a fact or a hypothetical proposition?

> Suppose those developers are not familiar with IOMMU/swiotlb, it takes
> times until the root cause is identified.

Sorry but you are making quite a few assumptions in a series claimed
to be "swiotlb: some cleanup" -- I personally expect cleanup patches
not to have any runtime side effects.

> If we panic earlier, the issue will be reported to IOMMU/swiotlb developer.

Ok, I think we should at least revert this patch, if not the entire series.

> This
> is also to sync with the remap failure logic in swiotlb (used by xen).

We can have it back in after we have better understood how it
interacts with different archs/drivers, or better yet when the needs
arise, if they arise at all.

Thanks.