linux-kernel - Re: [PATCH v7u1 26/31] x86: Don't enable swiotlb if there is not enough ram for it

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]

Date:	Fri, 4 Jan 2013 15:56:41 -0700
From:	Shuah Khan <shuahkhan@...il.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Yinghai Lu <yinghai@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...e.hu>, "H. Peter Anvin" <hpa@...or.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Borislav Petkov <bp@...en8.de>, Jan Kiszka <jan.kiszka@....de>,
	Jason Wessel <jason.wessel@...driver.com>,
	linux-kernel@...r.kernel.org,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	Joerg Roedel <joro@...tes.org>
Subject: Re: [PATCH v7u1 26/31] x86: Don't enable swiotlb if there is not
 enough ram for it

On Fri, Jan 4, 2013 at 3:47 PM, Eric W. Biederman <ebiederm@...ssion.com> wrote:
> Shuah Khan <shuahkhan@...il.com> writes:
>
>> On Fri, Jan 4, 2013 at 3:10 PM, Yinghai Lu <yinghai@...nel.org> wrote:
>>> On Fri, Jan 4, 2013 at 1:02 PM, Shuah Khan <shuahkhan@...il.com> wrote:
>>>> Pani'cing the system doesn't sound like a good option to me in this
>>>> case. This change to disable swiotlb is made for kdump. However, with
>>>> this change several system fail to boot, unless crashkernel_low=72M is
>>>> specified.
>>>
>>> this patchset is new feature to put second kdump kernel above 4G.
>>>
>> I understand this is just one of the patches to implement the new
>> kdump feature. However, I think regression on existing behavior with a
>> panic is a bit of a big hammer. Thie change causes panic on systems
>> even when kdump is not enabled, if I understand it correctly.
>>
>> Granted kdump gets enabled by several distros, but it is not a
>> required feature. However, expecting system to boot with devices that
>> require swiotlb fully functioning is a basic feature. So I would argue
>> that not breaking the basic functionality is a higher priority over
>> enabling kdump in this case.
>
> Yinghai Lu it looks like your autodetection of the problem case in this
> patch is problematic and needs a rethink.  My quick skim says you are
> trying to detect failure too early in the code.  Furthermore having
> kexec on panic sized magic comments without explanation is wrong.
>
> Shuah Khan this is motivated by kdump.  However a correct implementation
> should be about dealing with the case when there is simply not enough
> memory available below 4G for bounce buffers.
>
> If a device needs an iommu, and swiotlb is the only iommu option, and
> there is not enough memory below 4G panic'ing is entirely reasonable.
>
> Do I read this discussion right that we are waisting 64M on systems
> that have the swiotlb code but don't use the swiotlb?
>

No. pci_swiotlb_late_init() does free reserved swiolb buffers on
systems that don't need swiolb. IOMMU drivers turn off swiotlb after
iommu is initialized correctly. It is possible on some systems when
BIOS is incorrect, iommu initialization could fail and swiotlb is left
enabled.

AMD IOMMU driver is using this lever to leave swiotlb enabled when it
detects devices that can't be supported by iommu. My concern is that
this change for kdump removes that handshake ability between iommu and
swiolb.

-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/