Re: [PATCH v1 4/4] swiotlb: panic if nslabs is too small

[Dongli Zhang]

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. As a result, the issue is reported to a
specific driver's developers, who spend some time to confirm it is swiotlb
issue. Suppose those developers are not familiar with IOMMU/swiotlb, it takes
times until the root cause is identified.

If we panic earlier, the issue will be reported to IOMMU/swiotlb developer. This
is also to sync with the remap failure logic in swiotlb (used by xen).


Thank you very much!

Dongli Zhang

On 8/22/22 5:32 AM, Robin Murphy wrote:
> On 2022-08-22 12:26, Christoph Hellwig wrote:
>> On Mon, Aug 22, 2022 at 10:49:09AM +0100, Robin Murphy wrote:
>>> Hmm, it's possible this might be quietly fixed by 20347fca71a3, but either
>>> way I'm not sure why we would need to panic *before* we've even tried to
>>> allocate anything, when we could simply return with no harm done? If we've
>>> ended up calculating (or being told) a buffer size which is too small to be
>>> usable, that should be no different to disabling SWIOTLB entirely.
>>
>> Hmm.  I think this might be a philosophical question, but I think
>> failing the boot with a clear error report for a configuration that is
>> supposed to work but can't is way better than just panicing later on.
> 
> Depends which context of "supposed to work" you mean there. The most logical
> reason to end up with a tiny SWIOTLB size is because you don't expect to need
> SWIOTLB, therefore if there's now a functional minimum size limit, failing
> gracefully such that the system keeps working as before is correct in that
> context. Even if we assume the expectation goes the other way, then it should be
> on SWIOTLB to adjust the initial allocation size to whatever minimum it now
> needs, which as I say it looks like 20347fca71a3 might do anyway. Creating new
> breakage by panicking instead of making a decision one way or the other was
> never the right answer.
> 
>>> Historically, passing "swiotlb=1" on the command line has been used to save
>>> memory when the user knows SWIOTLB isn't needed. That should definitely not
>>> be allowed to start panicking.
>>
>> I've never seen swiotlb=1 advertized as a way to disable swiotlb.
>> That's always been swiotlb=noforce, which cleanly disables it.
> 
> No, it's probably not been advertised as such, but it's what clearly fell out of
> the available options before "noforce" was added (which was considerably more
> recently than "always"), and the fact is that people *are* still using it even
> today (presumably copy-pasted through Android BSPs since before 4.10).
> 
> Thanks,
> Robin.