* Re: [PATCH v1 4/9] powerpc/vdso: Remove unnecessary ifdefs in vdso_pagelist initialization
From: Christophe Leroy @ 2020-08-28 5:40 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <87d03c2plb.fsf@mpe.ellerman.id.au>
Le 27/08/2020 à 15:19, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> On 08/26/2020 02:58 PM, Michael Ellerman wrote:
>>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>>> diff --git a/arch/powerpc/kernel/vdso.c b/arch/powerpc/kernel/vdso.c
>>>> index daef14a284a3..bbb69832fd46 100644
>>>> --- a/arch/powerpc/kernel/vdso.c
>>>> +++ b/arch/powerpc/kernel/vdso.c
>>>> @@ -718,16 +710,14 @@ static int __init vdso_init(void)
>>> ...
>>>>
>>>> -
>>>> -#ifdef CONFIG_VDSO32
>>>> vdso32_kbase = &vdso32_start;
>>>>
>>>> /*
>>>> @@ -735,8 +725,6 @@ static int __init vdso_init(void)
>>>> */
>>>> vdso32_pages = (&vdso32_end - &vdso32_start) >> PAGE_SHIFT;
>>>> DBG("vdso32_kbase: %p, 0x%x pages\n", vdso32_kbase, vdso32_pages);
>>>> -#endif
>>>
>>> This didn't build for ppc64le:
>>>
>>> /opt/cross/gcc-8.20_binutils-2.32/powerpc64-unknown-linux-gnu/bin/powerpc64-unknown-linux-gnu-ld: arch/powerpc/kernel/vdso.o:(.toc+0x0): undefined reference to `vdso32_end'
>>> /opt/cross/gcc-8.20_binutils-2.32/powerpc64-unknown-linux-gnu/bin/powerpc64-unknown-linux-gnu-ld: arch/powerpc/kernel/vdso.o:(.toc+0x8): undefined reference to `vdso32_start'
>>> make[1]: *** [/scratch/michael/build/maint/Makefile:1166: vmlinux] Error 1
>>> make: *** [Makefile:185: __sub-make] Error 2
>>>
>>> So I just put that ifdef back.
>>>
>>
>> The problem is because is_32bit() can still return true even when
>> CONFIG_VDSO32 is not set.
>
> Hmm, you're right. My config had CONFIG_COMPAT enabled.
>
> But that seems like a bug, if someone enables COMPAT on ppc64le they are
> almost certainly going to want VDSO32 as well.
>
> So I think I'll do a lead up patch as below.
Ah yes, and with that then no need to consider the case where
is_32bit_task() is true and CONFIG_VDSO32 is not selected.
I'll update my leading series accordingly.
Christophe
>
> cheers
>
> diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
> index d4fd109f177e..cf2da1e401ef 100644
> --- a/arch/powerpc/platforms/Kconfig.cputype
> +++ b/arch/powerpc/platforms/Kconfig.cputype
> @@ -501,13 +501,12 @@ endmenu
>
> config VDSO32
> def_bool y
> - depends on PPC32 || CPU_BIG_ENDIAN
> + depends on PPC32 || COMPAT
> help
> This symbol controls whether we build the 32-bit VDSO. We obviously
> want to do that if we're building a 32-bit kernel. If we're building
> - a 64-bit kernel then we only want a 32-bit VDSO if we're building for
> - big endian. That is because the only little endian configuration we
> - support is ppc64le which is 64-bit only.
> + a 64-bit kernel then we only want a 32-bit VDSO if we're also enabling
> + COMPAT.
>
> choice
> prompt "Endianness selection"
>
^ permalink raw reply
* Re: [PATCH v1 01/10] powerpc/pseries/iommu: Replace hard-coded page shift
From: Alexey Kardashevskiy @ 2020-08-28 2:27 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Christophe Leroy, Joel Stanley,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <31e913d842693b6e107cb2b8e51fd45118b1bd2c.camel@gmail.com>
On 28/08/2020 01:32, Leonardo Bras wrote:
> Hello Alexey, thank you for this feedback!
>
> On Sat, 2020-08-22 at 19:33 +1000, Alexey Kardashevskiy wrote:
>>> +#define TCE_RPN_BITS 52 /* Bits 0-51 represent RPN on TCE */
>>
>> Ditch this one and use MAX_PHYSMEM_BITS instead? I am pretty sure this
>> is the actual limit.
>
> I understand this MAX_PHYSMEM_BITS(51) comes from the maximum physical memory addressable in the machine. IIUC, it means we can access physical address up to (1ul << MAX_PHYSMEM_BITS).
>
> This 52 comes from PAPR "Table 9. TCE Definition" which defines bits
> 0-51 as the RPN. By looking at code, I understand that it means we may input any address < (1ul << 52) to TCE.
>
> In practice, MAX_PHYSMEM_BITS should be enough as of today, because I suppose we can't ever pass a physical page address over
> (1ul << 51), and TCE accepts up to (1ul << 52).
> But if we ever increase MAX_PHYSMEM_BITS, it doesn't necessarily means that TCE_RPN_BITS will also be increased, so I think they are independent values.
>
> Does it make sense? Please let me know if I am missing something.
The underlying hardware is PHB3/4 about which the IODA2 Version 2.4
6Apr2012.pdf spec says:
"The number of most significant RPN bits implemented in the TCE is
dependent on the max size of System Memory to be supported by the platform".
IODA3 is the same on this matter.
This is MAX_PHYSMEM_BITS and PHB itself does not have any other limits
on top of that. So the only real limit comes from MAX_PHYSMEM_BITS and
where TCE_RPN_BITS comes from exactly - I have no idea.
>
>>
>>
>>> +#define TCE_RPN_MASK(ps) ((1ul << (TCE_RPN_BITS - (ps))) - 1)
>>> #define TCE_VALID 0x800 /* TCE valid */
>>> #define TCE_ALLIO 0x400 /* TCE valid for all lpars */
>>> #define TCE_PCI_WRITE 0x2 /* write from PCI allowed */
>>> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
>>> index e4198700ed1a..8fe23b7dff3a 100644
>>> --- a/arch/powerpc/platforms/pseries/iommu.c
>>> +++ b/arch/powerpc/platforms/pseries/iommu.c
>>> @@ -107,6 +107,9 @@ static int tce_build_pSeries(struct iommu_table *tbl, long index,
>>> u64 proto_tce;
>>> __be64 *tcep;
>>> u64 rpn;
>>> + const unsigned long tceshift = tbl->it_page_shift;
>>> + const unsigned long pagesize = IOMMU_PAGE_SIZE(tbl);
>>> + const u64 rpn_mask = TCE_RPN_MASK(tceshift);
>>
>> Using IOMMU_PAGE_SIZE macro for the page size and not using
>> IOMMU_PAGE_MASK for the mask - this incosistency makes my small brain
>> explode :) I understand the history but maaaaan... Oh well, ok.
>>
>
> Yeah, it feels kind of weird after two IOMMU related consts. :)
> But sure IOMMU_PAGE_MASK() would not be useful here :)
>
> And this kind of let me thinking:
>>> + rpn = __pa(uaddr) >> tceshift;
>>> + *tcep = cpu_to_be64(proto_tce | (rpn & rpn_mask) << tceshift);
> Why not:
> rpn_mask = TCE_RPN_MASK(tceshift) << tceshift;
A mask for a page number (but not the address!) hurts my brain, masks
are good against addresses but numbers should already have all bits
adjusted imho, may be it is just me :-/
>
> rpn = __pa(uaddr) & rpn_mask;
> *tcep = cpu_to_be64(proto_tce | rpn)
>
> I am usually afraid of changing stuff like this, but I think it's safe.
>
>> Good, otherwise. Thanks,
>
> Thank you for reviewing!
>
>
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v8 2/8] powerpc/vdso: Remove __kernel_datapage_offset and simplify __get_datapage()
From: Michael Ellerman @ 2020-08-28 2:14 UTC (permalink / raw)
To: Dmitry Safonov
Cc: nathanl, linux-arch, Arnd Bergmann, open list, Will Deacon,
Paul Mackerras, Andy Lutomirski, Thomas Gleixner,
Vincenzo Frascino, linuxppc-dev
In-Reply-To: <CAJwJo6ZANqYkSHbQ+3b+Fi_VT80MtrzEV5yreQAWx-L8j8x2zA@mail.gmail.com>
Dmitry Safonov <0x7f454c46@gmail.com> writes:
> Hello,
>
> On Wed, 26 Aug 2020 at 15:39, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> [..]
>> > arch_remap() gets replaced by vdso_remap()
>> >
>> > For arch_unmap(), I'm wondering how/what other architectures do, because
>> > powerpc seems to be the only one to erase the vdso context pointer when
>> > unmapping the vdso.
>>
>> Yeah. The original unmap/remap stuff was added for CRIU, which I thought
>> people tested on other architectures (more than powerpc even).
>>
>> Possibly no one really cares about vdso unmap though, vs just moving the
>> vdso.
>>
>> We added a test for vdso unmap recently because it happened to trigger a
>> KAUP failure, and someone actually hit it & reported it.
>
> You right, CRIU cares much more about moving vDSO.
> It's done for each restoree and as on most setups vDSO is premapped and
> used by the application - it's actively tested.
> Speaking about vDSO unmap - that's concerning only for heterogeneous C/R,
> i.e when an application is migrated from a system that uses vDSO to the one
> which doesn't - it's much rare scenario.
> (for arm it's !CONFIG_VDSO, for x86 it's `vdso=0` boot parameter)
Ah OK that explains it.
The case we hit of VDSO unmapping was some strange "library OS" thing
which had explicitly unmapped the VDSO, so also very rare.
> Looking at the code, it seems quite easy to provide/maintain .close() for
> vm_special_mapping. A bit harder to add a test from CRIU side
> (as glibc won't know on restore that it can't use vdso anymore),
> but totally not impossible.
>
>> Running that test on arm64 segfaults:
>>
>> # ./sigreturn_vdso
>> VDSO is at 0xffff8191f000-0xffff8191ffff (4096 bytes)
>> Signal delivered OK with VDSO mapped
>> VDSO moved to 0xffff8191a000-0xffff8191afff (4096 bytes)
>> Signal delivered OK with VDSO moved
>> Unmapped VDSO
>> Remapped the stack executable
>> [ 48.556191] potentially unexpected fatal signal 11.
>> [ 48.556752] CPU: 0 PID: 140 Comm: sigreturn_vdso Not tainted 5.9.0-rc2-00057-g2ac69819ba9e #190
>> [ 48.556990] Hardware name: linux,dummy-virt (DT)
>> [ 48.557336] pstate: 60001000 (nZCv daif -PAN -UAO BTYPE=--)
>> [ 48.557475] pc : 0000ffff8191a7bc
>> [ 48.557603] lr : 0000ffff8191a7bc
>> [ 48.557697] sp : 0000ffffc13c9e90
>> [ 48.557873] x29: 0000ffffc13cb0e0 x28: 0000000000000000
>> [ 48.558201] x27: 0000000000000000 x26: 0000000000000000
>> [ 48.558337] x25: 0000000000000000 x24: 0000000000000000
>> [ 48.558754] x23: 0000000000000000 x22: 0000000000000000
>> [ 48.558893] x21: 00000000004009b0 x20: 0000000000000000
>> [ 48.559046] x19: 0000000000400ff0 x18: 0000000000000000
>> [ 48.559180] x17: 0000ffff817da300 x16: 0000000000412010
>> [ 48.559312] x15: 0000000000000000 x14: 000000000000001c
>> [ 48.559443] x13: 656c626174756365 x12: 7865206b63617473
>> [ 48.559625] x11: 0000000000000003 x10: 0101010101010101
>> [ 48.559828] x9 : 0000ffff818afda8 x8 : 0000000000000081
>> [ 48.559973] x7 : 6174732065687420 x6 : 64657070616d6552
>> [ 48.560115] x5 : 000000000e0388bd x4 : 000000000040135d
>> [ 48.560270] x3 : 0000000000000000 x2 : 0000000000000001
>> [ 48.560412] x1 : 0000000000000003 x0 : 00000000004120b8
>> Segmentation fault
>> #
>>
>> So I think we need to keep the unmap hook. Maybe it should be handled by
>> the special_mapping stuff generically.
>
> I'll cook a patch for vm_special_mapping if you don't mind :-)
That would be great, thanks!
cheers
^ permalink raw reply
* [PATCH] powerpc/tools: Remove 90 line limit in checkpatch script
From: Russell Currey @ 2020-08-28 2:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Russell Currey
As of commit bdc48fa11e46, scripts/checkpatch.pl now has a default line
length warning of 100 characters. The powerpc wrapper script was using
a length of 90 instead of 80 in order to make checkpatch less
restrictive, but now it's making it more restrictive instead.
I think it makes sense to just use the default value now.
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
arch/powerpc/tools/checkpatch.sh | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/powerpc/tools/checkpatch.sh b/arch/powerpc/tools/checkpatch.sh
index 3ce5c093b19d..91c04802ec31 100755
--- a/arch/powerpc/tools/checkpatch.sh
+++ b/arch/powerpc/tools/checkpatch.sh
@@ -9,7 +9,6 @@ script_base=$(realpath $(dirname $0))
exec $script_base/../../../scripts/checkpatch.pl \
--subjective \
--no-summary \
- --max-line-length=90 \
--show-types \
--ignore ARCH_INCLUDE_LINUX \
--ignore BIT_MACRO \
--
2.28.0
^ permalink raw reply related
* Re: [PATCH v1 06/10] powerpc/pseries/iommu: Add ddw_list_add() helper
From: Alexey Kardashevskiy @ 2020-08-28 1:58 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Christophe Leroy, Joel Stanley,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <f80040bf941755469918fb75cf520590a4a5e3db.camel@gmail.com>
On 28/08/2020 08:11, Leonardo Bras wrote:
> On Mon, 2020-08-24 at 13:46 +1000, Alexey Kardashevskiy wrote:
>>> static int find_existing_ddw_windows(void)
>>> {
>>> int len;
>>> @@ -887,18 +905,11 @@ static int find_existing_ddw_windows(void)
>>> if (!direct64)
>>> continue;
>>>
>>> - window = kzalloc(sizeof(*window), GFP_KERNEL);
>>> - if (!window || len < sizeof(struct dynamic_dma_window_prop)) {
>>> + window = ddw_list_add(pdn, direct64);
>>> + if (!window || len < sizeof(*direct64)) {
>>
>> Since you are touching this code, it looks like the "len <
>> sizeof(*direct64)" part should go above to "if (!direct64)".
>
> Sure, makes sense.
> It will be fixed for v2.
>
>>
>>
>>
>>> kfree(window);
>>> remove_ddw(pdn, true);
>>> - continue;
>>> }
>>> -
>>> - window->device = pdn;
>>> - window->prop = direct64;
>>> - spin_lock(&direct_window_list_lock);
>>> - list_add(&window->list, &direct_window_list);
>>> - spin_unlock(&direct_window_list_lock);
>>> }
>>>
>>> return 0;
>>> @@ -1261,7 +1272,8 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>> dev_dbg(&dev->dev, "created tce table LIOBN 0x%x for %pOF\n",
>>> create.liobn, dn);
>>>
>>> - window = kzalloc(sizeof(*window), GFP_KERNEL);
>>> + /* Add new window to existing DDW list */
>>
>> The comment seems to duplicate what the ddw_list_add name already suggests.
>
> Ok, I will remove it then.
>
>>> + window = ddw_list_add(pdn, ddwprop);
>>> if (!window)
>>> goto out_clear_window;
>>>
>>> @@ -1280,16 +1292,14 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
>>> goto out_free_window;
>>> }
>>>
>>> - window->device = pdn;
>>> - window->prop = ddwprop;
>>> - spin_lock(&direct_window_list_lock);
>>> - list_add(&window->list, &direct_window_list);
>>> - spin_unlock(&direct_window_list_lock);
>>
>> I'd leave these 3 lines here and in find_existing_ddw_windows() (which
>> would make ddw_list_add -> ddw_prop_alloc). In general you want to have
>> less stuff to do on the failure path. kmalloc may fail and needs kfree
>> but you can safely delay list_add (which cannot fail) and avoid having
>> the lock help twice in the same function (one of them is hidden inside
>> ddw_list_add).
>> Not sure if this change is really needed after all. Thanks,
>
> I understand this leads to better performance in case anything fails.
> Also, I think list_add happening in the end is less error-prone (in
> case the list is checked between list_add and a fail).
Performance was not in my mind at all.
I noticed you remove from a list with a lock help and it was not there
before and there is a bunch on labels on the exit path and started
looking for list_add() and if you do not double remove from the list.
> But what if we put it at the end?
> What is the chance of a kzalloc of 4 pointers (struct direct_window)
> failing after walk_system_ram_range?
This is not about chances really, it is about readability. If let's say
kmalloc failed, you just to the error exit label and simply call kfree()
on that pointer, kfree will do nothing if it is NULL already, simple.
list_del() does not have this simplicity.
> Is it not worthy doing that for making enable_ddw() easier to
> understand?
This is my goal here :)
>
> Best regards,
> Leonardo
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v1 04/10] powerpc/kernel/iommu: Add new iommu_table_in_use() helper
From: Alexey Kardashevskiy @ 2020-08-28 1:51 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Christophe Leroy, Joel Stanley,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <5f26d433abcde7cd3b4da705742e17ca6c0f0f0b.camel@gmail.com>
On 28/08/2020 04:34, Leonardo Bras wrote:
> On Sat, 2020-08-22 at 20:34 +1000, Alexey Kardashevskiy wrote:
>>> +
>>> + /*ignore reserved bit0*/
>>
>> s/ignore reserved bit0/ ignore reserved bit0 / (add spaces)
>
> Fixed
>
>>> + if (tbl->it_offset == 0)
>>> + p1_start = 1;
>>> +
>>> + /* Check if reserved memory is valid*/
>>
>> A missing space here.
>
> Fixed
>
>>
>>> + if (tbl->it_reserved_start >= tbl->it_offset &&
>>> + tbl->it_reserved_start <= (tbl->it_offset + tbl->it_size) &&
>>> + tbl->it_reserved_end >= tbl->it_offset &&
>>> + tbl->it_reserved_end <= (tbl->it_offset + tbl->it_size)) {
>>
>> Uff. What if tbl->it_reserved_end is bigger than tbl->it_offset +
>> tbl->it_size?
>>
>> The reserved area is to preserve MMIO32 so it is for it_offset==0 only
>> and the boundaries are checked in the only callsite, and it is unlikely
>> to change soon or ever.
>>
>> Rather that bothering with fixing that, may be just add (did not test):
>>
>> if (WARN_ON((
>> (tbl->it_reserved_start || tbl->it_reserved_end) && (it_offset != 0))
>> (tbl->it_reserved_start > it_offset && tbl->it_reserved_end < it_offset
>> + it_size) && (it_offset == 0)) )
>> return true;
>>
>> Or simply always look for it_offset..it_reserved_start and
>> it_reserved_end..it_offset+it_size and if there is no reserved area,
>> initialize it_reserved_start=it_reserved_end=it_offset so the first
>> it_offset..it_reserved_start becomes a no-op.
>
> The problem here is that the values of it_reserved_{start,end} are not
> necessarily valid. I mean, on iommu_table_reserve_pages() the values
> are stored however they are given (bit reserving is done only if they
> are valid).
>
> Having a it_reserved_{start,end} value outside the valid ranges would
> cause find_next_bit() to run over memory outside the bitmap.
> Even if the those values are < tbl->it_offset, the resulting
> subtraction on unsigned would cause it to become a big value and run
> over memory outside the bitmap.
>
> But I think you are right. That is not the place to check if the
> reserved values are valid. It should just trust them here.
> I intent to change iommu_table_reserve_pages() to only store the
> parameters in it_reserved_{start,end} if they are in the range, and or
> it_offset in both of them if they are not.
>
> What do you think?
This should work, yes.
>
> Thanks for the feedback!
> Leonardo Bras
>
>
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v1 02/10] powerpc/kernel/iommu: Align size for IOMMU_PAGE_SIZE on iommu_*_coherent()
From: Alexey Kardashevskiy @ 2020-08-28 1:40 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Christophe Leroy, Joel Stanley,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <c67c66e466ad27d15aa2b970c48d2336d95b2971.camel@gmail.com>
On 28/08/2020 02:51, Leonardo Bras wrote:
> On Sat, 2020-08-22 at 20:07 +1000, Alexey Kardashevskiy wrote:
>>
>> On 18/08/2020 09:40, Leonardo Bras wrote:
>>> Both iommu_alloc_coherent() and iommu_free_coherent() assume that once
>>> size is aligned to PAGE_SIZE it will be aligned to IOMMU_PAGE_SIZE.
>>
>> The only case when it is not aligned is when IOMMU_PAGE_SIZE > PAGE_SIZE
>> which is unlikely but not impossible, we could configure the kernel for
>> 4K system pages and 64K IOMMU pages I suppose. Do we really want to do
>> this here, or simply put WARN_ON(tbl->it_page_shift > PAGE_SHIFT)?
>
> I think it would be better to keep the code as much generic as possible
> regarding page sizes.
Then you need to test it. Does 4K guest even boot (it should but I would
not bet much on it)?
>
>> Because if we want the former (==support), then we'll have to align the
>> size up to the bigger page size when allocating/zeroing system pages,
>> etc.
>
> This part I don't understand. Why do we need to align everything to the
> bigger pagesize?
>
> I mean, is not that enough that the range [ret, ret + size[ is both
> allocated by mm and mapped on a iommu range?
>
> Suppose a iommu_alloc_coherent() of 16kB on PAGESIZE = 4k and
> IOMMU_PAGE_SIZE() == 64k.
> Why 4 * cpu_pages mapped by a 64k IOMMU page is not enough?
> All the space the user asked for is allocated and mapped for DMA.
The user asked to map 16K, the rest - 48K - is used for something else
(may be even mapped to another device) but you are making all 64K
accessible by the device which only should be able to access 16K.
In practice, if this happens, H_PUT_TCE will simply fail.
>
>> Bigger pages are not the case here as I understand it.
>
> I did not get this part, what do you mean?
Possible IOMMU page sizes are 4K, 64K, 2M, 16M, 256M, 1GB, and the
supported set of sizes is different for P8/P9 and type of IO (PHB,
NVLink/CAPI).
>
>>> Update those functions to guarantee alignment with requested size
>>> using IOMMU_PAGE_ALIGN() before doing iommu_alloc() / iommu_free().
>>>
>>> Also, on iommu_range_alloc(), replace ALIGN(n, 1 << tbl->it_page_shift)
>>> with IOMMU_PAGE_ALIGN(n, tbl), which seems easier to read.
>>>
>>> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
>>> ---
>>> arch/powerpc/kernel/iommu.c | 17 +++++++++--------
>>> 1 file changed, 9 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>>> index 9704f3f76e63..d7086087830f 100644
>>> --- a/arch/powerpc/kernel/iommu.c
>>> +++ b/arch/powerpc/kernel/iommu.c
>>> @@ -237,10 +237,9 @@ static unsigned long iommu_range_alloc(struct device *dev,
>>> }
>>>
>>> if (dev)
>>> - boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
>>> - 1 << tbl->it_page_shift);
>>> + boundary_size = IOMMU_PAGE_ALIGN(dma_get_seg_boundary(dev) + 1, tbl);
>>
>> Run checkpatch.pl, should complain about a long line.
>
> It's 86 columns long, which is less than the new limit of 100 columns
> Linus announced a few weeks ago. checkpatch.pl was updated too:
> https://www.phoronix.com/scan.php?page=news_item&px=Linux-Kernel-Deprecates-80-Col
Yay finally :) Thanks,
>
>>
>>
>>> else
>>> - boundary_size = ALIGN(1UL << 32, 1 << tbl->it_page_shift);
>>> + boundary_size = IOMMU_PAGE_ALIGN(1UL << 32, tbl);
>>> /* 4GB boundary for iseries_hv_alloc and iseries_hv_map */
>>>
>>> n = iommu_area_alloc(tbl->it_map, limit, start, npages, tbl->it_offset,
>>> @@ -858,6 +857,7 @@ void *iommu_alloc_coherent(struct device *dev, struct iommu_table *tbl,
>>> unsigned int order;
>>> unsigned int nio_pages, io_order;
>>> struct page *page;
>>> + size_t size_io = size;
>>>
>>> size = PAGE_ALIGN(size);
>>> order = get_order(size);
>>> @@ -884,8 +884,9 @@ void *iommu_alloc_coherent(struct device *dev, struct iommu_table *tbl,
>>> memset(ret, 0, size);
>>>
>>> /* Set up tces to cover the allocated range */
>>> - nio_pages = size >> tbl->it_page_shift;
>>> - io_order = get_iommu_order(size, tbl);
>>> + size_io = IOMMU_PAGE_ALIGN(size_io, tbl);
>>> + nio_pages = size_io >> tbl->it_page_shift;
>>> + io_order = get_iommu_order(size_io, tbl);
>>> mapping = iommu_alloc(dev, tbl, ret, nio_pages, DMA_BIDIRECTIONAL,
>>> mask >> tbl->it_page_shift, io_order, 0);
>>> if (mapping == DMA_MAPPING_ERROR) {
>>> @@ -900,11 +901,11 @@ void iommu_free_coherent(struct iommu_table *tbl, size_t size,
>>> void *vaddr, dma_addr_t dma_handle)
>>> {
>>> if (tbl) {
>>> - unsigned int nio_pages;
>>> + size_t size_io = IOMMU_PAGE_ALIGN(size, tbl);
>>> + unsigned int nio_pages = size_io >> tbl->it_page_shift;
>>>
>>> - size = PAGE_ALIGN(size);
>>> - nio_pages = size >> tbl->it_page_shift;
>>> iommu_free(tbl, dma_handle, nio_pages);
>>> +
>>
>> Unrelated new line.
>
> Will be removed. Thanks!
>
>>
>>
>>> size = PAGE_ALIGN(size);
>>> free_pages((unsigned long)vaddr, get_order(size));
>>> }
>>>
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v1 06/10] powerpc/pseries/iommu: Add ddw_list_add() helper
From: Leonardo Bras @ 2020-08-27 22:11 UTC (permalink / raw)
To: Alexey Kardashevskiy, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Christophe Leroy, Joel Stanley,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <af4246bb-9357-098e-f167-8f30c6b893f2@ozlabs.ru>
On Mon, 2020-08-24 at 13:46 +1000, Alexey Kardashevskiy wrote:
> > static int find_existing_ddw_windows(void)
> > {
> > int len;
> > @@ -887,18 +905,11 @@ static int find_existing_ddw_windows(void)
> > if (!direct64)
> > continue;
> >
> > - window = kzalloc(sizeof(*window), GFP_KERNEL);
> > - if (!window || len < sizeof(struct dynamic_dma_window_prop)) {
> > + window = ddw_list_add(pdn, direct64);
> > + if (!window || len < sizeof(*direct64)) {
>
> Since you are touching this code, it looks like the "len <
> sizeof(*direct64)" part should go above to "if (!direct64)".
Sure, makes sense.
It will be fixed for v2.
>
>
>
> > kfree(window);
> > remove_ddw(pdn, true);
> > - continue;
> > }
> > -
> > - window->device = pdn;
> > - window->prop = direct64;
> > - spin_lock(&direct_window_list_lock);
> > - list_add(&window->list, &direct_window_list);
> > - spin_unlock(&direct_window_list_lock);
> > }
> >
> > return 0;
> > @@ -1261,7 +1272,8 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> > dev_dbg(&dev->dev, "created tce table LIOBN 0x%x for %pOF\n",
> > create.liobn, dn);
> >
> > - window = kzalloc(sizeof(*window), GFP_KERNEL);
> > + /* Add new window to existing DDW list */
>
> The comment seems to duplicate what the ddw_list_add name already suggests.
Ok, I will remove it then.
> > + window = ddw_list_add(pdn, ddwprop);
> > if (!window)
> > goto out_clear_window;
> >
> > @@ -1280,16 +1292,14 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> > goto out_free_window;
> > }
> >
> > - window->device = pdn;
> > - window->prop = ddwprop;
> > - spin_lock(&direct_window_list_lock);
> > - list_add(&window->list, &direct_window_list);
> > - spin_unlock(&direct_window_list_lock);
>
> I'd leave these 3 lines here and in find_existing_ddw_windows() (which
> would make ddw_list_add -> ddw_prop_alloc). In general you want to have
> less stuff to do on the failure path. kmalloc may fail and needs kfree
> but you can safely delay list_add (which cannot fail) and avoid having
> the lock help twice in the same function (one of them is hidden inside
> ddw_list_add).
> Not sure if this change is really needed after all. Thanks,
I understand this leads to better performance in case anything fails.
Also, I think list_add happening in the end is less error-prone (in
case the list is checked between list_add and a fail).
But what if we put it at the end?
What is the chance of a kzalloc of 4 pointers (struct direct_window)
failing after walk_system_ram_range?
Is it not worthy doing that for making enable_ddw() easier to
understand?
Best regards,
Leonardo
^ permalink raw reply
* Re: fsl_espi errors on v5.7.15
From: Chris Packham @ 2020-08-27 22:07 UTC (permalink / raw)
To: Nicholas Piggin, benh@kernel.crashing.org, broonie@kernel.org,
Heiner Kallweit, mpe@ellerman.id.au, paulus@samba.org
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
linux-spi@vger.kernel.org
In-Reply-To: <1598510348.1g7wt0s02s.astroid@bobo.none>
On 27/08/20 7:12 pm, Nicholas Piggin wrote:
> Excerpts from Heiner Kallweit's message of August 26, 2020 4:38 pm:
>> On 26.08.2020 08:07, Chris Packham wrote:
>>> On 26/08/20 1:48 pm, Chris Packham wrote:
>>>> On 26/08/20 10:22 am, Chris Packham wrote:
>>>>> On 25/08/20 7:22 pm, Heiner Kallweit wrote:
>>>>>
>>>>> <snip>
>>>>>> I've been staring at spi-fsl-espi.c for while now and I think I've
>>>>>>> identified a couple of deficiencies that may or may not be related
>>>>>>> to my
>>>>>>> issue.
>>>>>>>
>>>>>>> First I think the 'Transfer done but SPIE_DON isn't set' message
>>>>>>> can be
>>>>>>> generated spuriously. In fsl_espi_irq() we read the ESPI_SPIE
>>>>>>> register.
>>>>>>> We also write back to it to clear the current events. We re-read it in
>>>>>>> fsl_espi_cpu_irq() and complain when SPIE_DON is not set. But we can
>>>>>>> naturally end up in that situation if we're doing a large read.
>>>>>>> Consider
>>>>>>> the messages for reading a block of data from a spi-nor chip
>>>>>>>
>>>>>>> tx = READ_OP + ADDR
>>>>>>> rx = data
>>>>>>>
>>>>>>> We setup the transfer and pump out the tx_buf. The first interrupt
>>>>>>> goes
>>>>>>> off and ESPI_SPIE has SPIM_DON and SPIM_RXT set. We empty the rx fifo,
>>>>>>> clear ESPI_SPIE and wait for the next interrupt. The next interrupt
>>>>>>> fires and this time we have ESPI_SPIE with just SPIM_RXT set. This
>>>>>>> continues until we've received all the data and we finish with
>>>>>>> ESPI_SPIE
>>>>>>> having only SPIM_RXT set. When we re-read it we complain that SPIE_DON
>>>>>>> isn't set.
>>>>>>>
>>>>>>> The other deficiency is that we only get an interrupt when the
>>>>>>> amount of
>>>>>>> data in the rx fifo is above FSL_ESPI_RXTHR. If there are fewer than
>>>>>>> FSL_ESPI_RXTHR left to be received we will never pull them out of
>>>>>>> the fifo.
>>>>>>>
>>>>>> SPIM_DON will trigger an interrupt once the last characters have been
>>>>>> transferred, and read the remaining characters from the FIFO.
>>>>> The T2080RM that I have says the following about the DON bit
>>>>>
>>>>> "Last character was transmitted. The last character was transmitted
>>>>> and a new command can be written for the next frame."
>>>>>
>>>>> That does at least seem to fit with my assertion that it's all about
>>>>> the TX direction. But the fact that it doesn't happen all the time
>>>>> throws some doubt on it.
>>>>>
>>>>>> I think the reason I'm seeing some variability is because of how fast
>>>>>>> (or slow) the interrupts get processed and how fast the spi-nor
>>>>>>> chip can
>>>>>>> fill the CPUs rx fifo.
>>>>>>>
>>>>>> To rule out timing issues at high bus frequencies I initially asked
>>>>>> for re-testing at lower frequencies. If you e.g. limit the bus to 1 MHz
>>>>>> or even less, then timing shouldn't be an issue.
>>>>> Yes I've currently got spi-max-frequency = <1000000>; in my dts. I
>>>>> would also expect a slower frequency would fit my "DON is for TX"
>>>>> narrative.
>>>>>> Last relevant functional changes have been done almost 4 years ago.
>>>>>> And yours is the first such report I see. So question is what could
>>>>>> be so
>>>>>> special with your setup that it seems you're the only one being
>>>>>> affected.
>>>>>> The scenarios you describe are standard, therefore much more people
>>>>>> should be affected in case of a driver bug.
>>>>> Agreed. But even on my hardware (which may have a latent issue
>>>>> despite being in the field for going on 5 years) the issue only
>>>>> triggers under some fairly specific circumstances.
>>>>>> You said that kernel config impacts how frequently the issue happens.
>>>>>> Therefore question is what's the diff in kernel config, and how could
>>>>>> the differences be related to SPI.
>>>>> It did seem to be somewhat random. Things like CONFIG_PREEMPT have an
>>>>> impact but every time I found something that seemed to be having an
>>>>> impact I've been able to disprove it. I actually think its about how
>>>>> busy the system is which may or may not affect when we get round to
>>>>> processing the interrupts.
>>>>>
>>>>> I have managed to get the 'Transfer done but SPIE_DON isn't set!' to
>>>>> occur on the T2080RDB.
>>>>>
>>>>> I've had to add the following to expose the environment as a mtd
>>>>> partition
>>>>>
>>>>> diff --git a/arch/powerpc/boot/dts/fsl/t208xrdb.dtsi
>>>>> b/arch/powerpc/boot/dts/fsl/t208xrdb.dtsi
>>>>> index ff87e67c70da..fbf95fc1fd68 100644
>>>>> --- a/arch/powerpc/boot/dts/fsl/t208xrdb.dtsi
>>>>> +++ b/arch/powerpc/boot/dts/fsl/t208xrdb.dtsi
>>>>> @@ -116,6 +116,15 @@ flash@0 {
>>>>> compatible = "micron,n25q512ax3",
>>>>> "jedec,spi-nor";
>>>>> reg = <0>;
>>>>> spi-max-frequency = <10000000>; /*
>>>>> input clock */
>>>>> +
>>>>> + partition@u-boot {
>>>>> + reg = <0x00000000 0x00100000>;
>>>>> + label = "u-boot";
>>>>> + };
>>>>> + partition@u-boot-env {
>>>>> + reg = <0x00100000 0x00010000>;
>>>>> + label = "u-boot-env";
>>>>> + };
>>>>> };
>>>>> };
>>>>>
>>>>> And I'm using the following script to poke at the environment
>>>>> (warning if anyone does try this and the bug hits it can render your
>>>>> u-boot environment invalid).
>>>>>
>>>>> cat flash/fw_env_test.sh
>>>>> #!/bin/sh
>>>>>
>>>>> generate_fw_env_config()
>>>>> {
>>>>> cat /proc/mtd | sed 's/[:"]//g' | while read dev size erasesize
>>>>> name ; do
>>>>> echo "$dev $size $erasesize $name"
>>>>> [ "$name" = "u-boot-env" ] && echo "/dev/$dev 0x0000 0x2000
>>>>> $erasesize" >/flash/fw_env.config
>>>>> done
>>>>> }
>>>>>
>>>>> cycles=10
>>>>> [ $# -ge 1 ] && cycles=$1
>>>>>
>>>>> generate_fw_env_config
>>>>>
>>>>> fw_printenv -c /flash/fw_env.config
>>>>>
>>>>> dmesg -c >/dev/null
>>>>> x=0
>>>>> while [ $x -lt $cycles ]; do
>>>>> fw_printenv -c /flash/fw_env.config >/dev/null || break
>>>>> fw_setenv -c /flash/fw_env.config foo $RANDOM || break;
>>>>> dmesg -c | grep -q fsl_espi && break;
>>>>> let x=x+1
>>>>> done
>>>>>
>>>>> echo "Ran $x cycles"
>>>> I've also now seen the RX FIFO not empty error on the T2080RDB
>>>>
>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's aren't empty!
>>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>
>>>> With my current workaround of emptying the RX FIFO. It seems
>>>> survivable. Interestingly it only ever seems to be 1 extra byte in the
>>>> RX FIFO and it seems to be after either a READ_SR or a READ_FSR.
>>>>
>>>> fsl_espi ffe110000.spi: tx 70
>>>> fsl_espi ffe110000.spi: rx 03
>>>> fsl_espi ffe110000.spi: Extra RX 00
>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's aren't empty!
>>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>> fsl_espi ffe110000.spi: tx 05
>>>> fsl_espi ffe110000.spi: rx 00
>>>> fsl_espi ffe110000.spi: Extra RX 03
>>>> fsl_espi ffe110000.spi: Transfer done but SPIE_DON isn't set!
>>>> fsl_espi ffe110000.spi: Transfer done but rx/tx fifo's aren't empty!
>>>> fsl_espi ffe110000.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>> fsl_espi ffe110000.spi: tx 05
>>>> fsl_espi ffe110000.spi: rx 00
>>>> fsl_espi ffe110000.spi: Extra RX 03
>>>>
>>>> From all the Micron SPI-NOR datasheets I've got access to it is
>>>> possible to continually read the SR/FSR. But I've no idea why it
>>>> happens some times and not others.
>>> So I think I've got a reproduction and I think I've bisected the problem
>>> to commit 3282a3da25bd ("powerpc/64: Implement soft interrupt replay in
>>> C"). My day is just finishing now so I haven't applied too much scrutiny
>>> to this result. Given the various rabbit holes I've been down on this
>>> issue already I'd take this information with a good degree of skepticism.
>>>
>> OK, so an easy test should be to re-test with a 5.4 kernel.
>> It doesn't have yet the change you're referring to, and the fsl-espi driver
>> is basically the same as in 5.7 (just two small changes in 5.7).
> There's 6cc0c16d82f88 and maybe also other interrupt related patches
> around this time that could affect book E, so it's good if that exact
> patch is confirmed.
My confirmation is basically that I can induce the issue in a 5.4 kernel
by cherry-picking 3282a3da25bd. I'm also able to "fix" the issue in
5.9-rc2 by reverting that one commit.
I both cases it's not exactly a clean cherry-pick/revert so I also
confirmed the bisection result by building at 3282a3da25bd (which sees
the issue) and the commit just before (which does not).
> I've been staring at 3282a3da25bd for a while and nothing immediately
> stands out. It doesn't look like the low level handlers do anything
> special (well 0x900 does ack the decrementer, but so does the masked
> handler).
>
> Can you try this patch and also enable CONFIG_PPC_IRQ_SOFT_MASK_DEBUG?
>
> Thanks,
> Nick
>
> ---
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index bf21ebd36190..10d339042330 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -214,7 +214,7 @@ void replay_soft_interrupts(void)
> struct pt_regs regs;
>
> ppc_save_regs(®s);
> - regs.softe = IRQS_ALL_DISABLED;
> + regs.softe = IRQS_ENABLED;
>
> again:
> if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG))
> @@ -349,6 +349,7 @@ notrace void arch_local_irq_restore(unsigned long mask)
> if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG))
> WARN_ON_ONCE(!(mfmsr() & MSR_EE));
> __hard_irq_disable();
> + local_paca->irq_happened |= PACA_IRQ_HARD_DIS;
> } else {
> /*
> * We should already be hard disabled here. We had bugs
> @@ -368,6 +369,7 @@ notrace void arch_local_irq_restore(unsigned long mask)
> }
> }
>
> + preempt_disable();
> irq_soft_mask_set(IRQS_ALL_DISABLED);
> trace_hardirqs_off();
>
> @@ -377,6 +379,7 @@ notrace void arch_local_irq_restore(unsigned long mask)
> trace_hardirqs_on();
> irq_soft_mask_set(IRQS_ENABLED);
> __hard_irq_enable();
> + preempt_enable();
> }
> EXPORT_SYMBOL(arch_local_irq_restore);
>
I still saw the issue with this change applied. PPC_IRQ_SOFT_MASK_DEBUG
didn't report anything (either with or without the change above).
^ permalink raw reply
* Re: [PATCH v1 05/10] powerpc/pseries/iommu: Add iommu_pseries_alloc_table() helper
From: Leonardo Bras @ 2020-08-27 21:23 UTC (permalink / raw)
To: Alexey Kardashevskiy, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Christophe Leroy, Joel Stanley,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <e4ea6264-578d-f8ea-a0c4-3b279b5c0411@ozlabs.ru>
On Mon, 2020-08-24 at 10:38 +1000, Alexey Kardashevskiy wrote:
>
> On 18/08/2020 09:40, Leonardo Bras wrote:
> > Creates a helper to allow allocating a new iommu_table without the need
> > to reallocate the iommu_group.
> >
> > This will be helpful for replacing the iommu_table for the new DMA window,
> > after we remove the old one with iommu_tce_table_put().
> >
> > Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> > ---
> > arch/powerpc/platforms/pseries/iommu.c | 25 ++++++++++++++-----------
> > 1 file changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> > index 8fe23b7dff3a..39617ce0ec83 100644
> > --- a/arch/powerpc/platforms/pseries/iommu.c
> > +++ b/arch/powerpc/platforms/pseries/iommu.c
> > @@ -53,28 +53,31 @@ enum {
> > DDW_EXT_QUERY_OUT_SIZE = 2
> > };
> >
> > -static struct iommu_table_group *iommu_pseries_alloc_group(int node)
> > +static struct iommu_table *iommu_pseries_alloc_table(int node)
> > {
> > - struct iommu_table_group *table_group;
> > struct iommu_table *tbl;
> >
> > - table_group = kzalloc_node(sizeof(struct iommu_table_group), GFP_KERNEL,
> > - node);
> > - if (!table_group)
> > - return NULL;
> > -
> > tbl = kzalloc_node(sizeof(struct iommu_table), GFP_KERNEL, node);
> > if (!tbl)
> > - goto free_group;
> > + return NULL;
> >
> > INIT_LIST_HEAD_RCU(&tbl->it_group_list);
> > kref_init(&tbl->it_kref);
> > + return tbl;
> > +}
> >
> > - table_group->tables[0] = tbl;
> > +static struct iommu_table_group *iommu_pseries_alloc_group(int node)
> > +{
> > + struct iommu_table_group *table_group;
> > +
> > + table_group = kzalloc_node(sizeof(*table_group), GFP_KERNEL, node);
>
> I'd prefer you did not make unrelated changes (sizeof(struct
> iommu_table_group) -> sizeof(*table_group)) so the diff stays shorter
> and easier to follow. You changed sizeof(struct iommu_table_group) but
> not sizeof(struct iommu_table) and this confused me enough to spend more
> time than this straight forward change deserves.
Sorry, I will keep this in mind for future patches.
Thank you for the tip!
>
> Not important in this case though so
>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Thank you!
^ permalink raw reply
* Re: [PATCH v8 2/8] powerpc/vdso: Remove __kernel_datapage_offset and simplify __get_datapage()
From: Dmitry Safonov @ 2020-08-27 20:34 UTC (permalink / raw)
To: Michael Ellerman
Cc: nathanl, linux-arch, Arnd Bergmann, open list, Will Deacon,
Paul Mackerras, Andy Lutomirski, Thomas Gleixner,
Vincenzo Frascino, linuxppc-dev
In-Reply-To: <87imd5h5kb.fsf@mpe.ellerman.id.au>
Hello,
On Wed, 26 Aug 2020 at 15:39, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
[..]
> > arch_remap() gets replaced by vdso_remap()
> >
> > For arch_unmap(), I'm wondering how/what other architectures do, because
> > powerpc seems to be the only one to erase the vdso context pointer when
> > unmapping the vdso.
>
> Yeah. The original unmap/remap stuff was added for CRIU, which I thought
> people tested on other architectures (more than powerpc even).
>
> Possibly no one really cares about vdso unmap though, vs just moving the
> vdso.
>
> We added a test for vdso unmap recently because it happened to trigger a
> KAUP failure, and someone actually hit it & reported it.
You right, CRIU cares much more about moving vDSO.
It's done for each restoree and as on most setups vDSO is premapped and
used by the application - it's actively tested.
Speaking about vDSO unmap - that's concerning only for heterogeneous C/R,
i.e when an application is migrated from a system that uses vDSO to the one
which doesn't - it's much rare scenario.
(for arm it's !CONFIG_VDSO, for x86 it's `vdso=0` boot parameter)
Looking at the code, it seems quite easy to provide/maintain .close() for
vm_special_mapping. A bit harder to add a test from CRIU side
(as glibc won't know on restore that it can't use vdso anymore),
but totally not impossible.
> Running that test on arm64 segfaults:
>
> # ./sigreturn_vdso
> VDSO is at 0xffff8191f000-0xffff8191ffff (4096 bytes)
> Signal delivered OK with VDSO mapped
> VDSO moved to 0xffff8191a000-0xffff8191afff (4096 bytes)
> Signal delivered OK with VDSO moved
> Unmapped VDSO
> Remapped the stack executable
> [ 48.556191] potentially unexpected fatal signal 11.
> [ 48.556752] CPU: 0 PID: 140 Comm: sigreturn_vdso Not tainted 5.9.0-rc2-00057-g2ac69819ba9e #190
> [ 48.556990] Hardware name: linux,dummy-virt (DT)
> [ 48.557336] pstate: 60001000 (nZCv daif -PAN -UAO BTYPE=--)
> [ 48.557475] pc : 0000ffff8191a7bc
> [ 48.557603] lr : 0000ffff8191a7bc
> [ 48.557697] sp : 0000ffffc13c9e90
> [ 48.557873] x29: 0000ffffc13cb0e0 x28: 0000000000000000
> [ 48.558201] x27: 0000000000000000 x26: 0000000000000000
> [ 48.558337] x25: 0000000000000000 x24: 0000000000000000
> [ 48.558754] x23: 0000000000000000 x22: 0000000000000000
> [ 48.558893] x21: 00000000004009b0 x20: 0000000000000000
> [ 48.559046] x19: 0000000000400ff0 x18: 0000000000000000
> [ 48.559180] x17: 0000ffff817da300 x16: 0000000000412010
> [ 48.559312] x15: 0000000000000000 x14: 000000000000001c
> [ 48.559443] x13: 656c626174756365 x12: 7865206b63617473
> [ 48.559625] x11: 0000000000000003 x10: 0101010101010101
> [ 48.559828] x9 : 0000ffff818afda8 x8 : 0000000000000081
> [ 48.559973] x7 : 6174732065687420 x6 : 64657070616d6552
> [ 48.560115] x5 : 000000000e0388bd x4 : 000000000040135d
> [ 48.560270] x3 : 0000000000000000 x2 : 0000000000000001
> [ 48.560412] x1 : 0000000000000003 x0 : 00000000004120b8
> Segmentation fault
> #
>
> So I think we need to keep the unmap hook. Maybe it should be handled by
> the special_mapping stuff generically.
I'll cook a patch for vm_special_mapping if you don't mind :-)
Thanks,
Dmitry
^ permalink raw reply
* [powerpc:next-test] BUILD SUCCESS adf3447eddc59967b18faf51dff97c07a5a8220b
From: kernel test robot @ 2020-08-27 20:00 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next-test
branch HEAD: adf3447eddc59967b18faf51dff97c07a5a8220b powerpc: Update documentation of ISA versions for Power10
elapsed time: 735m
configs tested: 118
configs skipped: 13
The following configs have been built successfully.
More configs may be tested in the coming days.
arm defconfig
arm64 allyesconfig
arm64 defconfig
arm allyesconfig
arm allmodconfig
riscv nommu_k210_defconfig
mips decstation_defconfig
arm milbeaut_m10v_defconfig
powerpc mpc7448_hpc2_defconfig
sh ul2_defconfig
mips ip28_defconfig
arm clps711x_defconfig
m68k m5208evb_defconfig
arm vf610m4_defconfig
m68k mac_defconfig
powerpc ps3_defconfig
arm rpc_defconfig
sh shmin_defconfig
arm s3c2410_defconfig
m68k m5272c3_defconfig
sh migor_defconfig
arm multi_v7_defconfig
mips mpc30x_defconfig
powerpc ppc40x_defconfig
parisc allyesconfig
powerpc mpc5200_defconfig
arm pxa255-idp_defconfig
mips ip22_defconfig
arc tb10x_defconfig
sparc64 alldefconfig
sh sh7757lcr_defconfig
mips maltaaprp_defconfig
arm hackkit_defconfig
arm moxart_defconfig
parisc alldefconfig
arm mvebu_v7_defconfig
nios2 3c120_defconfig
arm h3600_defconfig
sparc sparc32_defconfig
arm mainstone_defconfig
m68k allmodconfig
c6x dsk6455_defconfig
m68k apollo_defconfig
sh sh03_defconfig
sh edosk7760_defconfig
sh sh2007_defconfig
s390 zfcpdump_defconfig
x86_64 allyesconfig
mips pic32mzda_defconfig
sh sh7763rdp_defconfig
arm netwinder_defconfig
arm at91_dt_defconfig
sh kfr2r09_defconfig
arm pcm027_defconfig
powerpc ppc6xx_defconfig
nios2 alldefconfig
powerpc amigaone_defconfig
powerpc skiroot_defconfig
powerpc tqm8xx_defconfig
arm aspeed_g4_defconfig
ia64 allmodconfig
ia64 defconfig
ia64 allyesconfig
m68k defconfig
m68k allyesconfig
nios2 defconfig
arc allyesconfig
nds32 allnoconfig
c6x allyesconfig
nds32 defconfig
nios2 allyesconfig
csky defconfig
alpha defconfig
alpha allyesconfig
xtensa allyesconfig
h8300 allyesconfig
arc defconfig
sh allmodconfig
parisc defconfig
s390 allyesconfig
s390 defconfig
i386 allyesconfig
sparc allyesconfig
sparc defconfig
i386 defconfig
mips allyesconfig
mips allmodconfig
powerpc defconfig
powerpc allyesconfig
powerpc allmodconfig
powerpc allnoconfig
x86_64 randconfig-a003-20200827
x86_64 randconfig-a002-20200827
x86_64 randconfig-a001-20200827
x86_64 randconfig-a005-20200827
x86_64 randconfig-a006-20200827
x86_64 randconfig-a004-20200827
i386 randconfig-a002-20200827
i386 randconfig-a004-20200827
i386 randconfig-a003-20200827
i386 randconfig-a005-20200827
i386 randconfig-a006-20200827
i386 randconfig-a001-20200827
i386 randconfig-a013-20200827
i386 randconfig-a012-20200827
i386 randconfig-a011-20200827
i386 randconfig-a016-20200827
i386 randconfig-a015-20200827
i386 randconfig-a014-20200827
riscv allyesconfig
riscv allnoconfig
riscv defconfig
riscv allmodconfig
x86_64 rhel
x86_64 rhel-7.6-kselftests
x86_64 defconfig
x86_64 rhel-8.3
x86_64 kexec
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply
* [powerpc:merge] BUILD SUCCESS 2281d5a219ba2dca30e0e74e49b811ed8bfc6a81
From: kernel test robot @ 2020-08-27 20:00 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git merge
branch HEAD: 2281d5a219ba2dca30e0e74e49b811ed8bfc6a81 Automatic merge of 'master', 'next' and 'fixes' (2020-08-27 17:47)
elapsed time: 722m
configs tested: 115
configs skipped: 14
The following configs have been built successfully.
More configs may be tested in the coming days.
arm defconfig
arm64 allyesconfig
arm64 defconfig
arm allyesconfig
arm allmodconfig
riscv nommu_k210_defconfig
mips decstation_defconfig
arm milbeaut_m10v_defconfig
powerpc mpc7448_hpc2_defconfig
sh ul2_defconfig
powerpc ps3_defconfig
arm rpc_defconfig
sh shmin_defconfig
arm s3c2410_defconfig
m68k m5272c3_defconfig
sh migor_defconfig
arm multi_v7_defconfig
sh sdk7780_defconfig
arm ezx_defconfig
mips e55_defconfig
arm pxa910_defconfig
sh apsh4ad0a_defconfig
mips ath25_defconfig
mips mpc30x_defconfig
powerpc ppc40x_defconfig
parisc allyesconfig
powerpc mpc5200_defconfig
arm pxa255-idp_defconfig
mips ip22_defconfig
arc tb10x_defconfig
sparc64 alldefconfig
sh sh7757lcr_defconfig
mips maltaaprp_defconfig
arm hackkit_defconfig
arm moxart_defconfig
parisc alldefconfig
arm mvebu_v7_defconfig
nios2 3c120_defconfig
arm h3600_defconfig
sparc sparc32_defconfig
arm mainstone_defconfig
m68k allmodconfig
c6x dsk6455_defconfig
s390 zfcpdump_defconfig
x86_64 allyesconfig
mips pic32mzda_defconfig
sh sh7763rdp_defconfig
arm netwinder_defconfig
arm at91_dt_defconfig
sh kfr2r09_defconfig
arm pcm027_defconfig
powerpc ppc6xx_defconfig
nios2 alldefconfig
powerpc amigaone_defconfig
powerpc skiroot_defconfig
powerpc tqm8xx_defconfig
arm aspeed_g4_defconfig
ia64 allmodconfig
ia64 defconfig
ia64 allyesconfig
m68k defconfig
m68k allyesconfig
nios2 defconfig
arc allyesconfig
nds32 allnoconfig
c6x allyesconfig
nds32 defconfig
nios2 allyesconfig
csky defconfig
alpha defconfig
alpha allyesconfig
xtensa allyesconfig
h8300 allyesconfig
arc defconfig
sh allmodconfig
parisc defconfig
s390 allyesconfig
s390 defconfig
i386 allyesconfig
sparc allyesconfig
sparc defconfig
i386 defconfig
mips allyesconfig
mips allmodconfig
powerpc defconfig
powerpc allyesconfig
powerpc allmodconfig
powerpc allnoconfig
x86_64 randconfig-a003-20200827
x86_64 randconfig-a002-20200827
x86_64 randconfig-a001-20200827
x86_64 randconfig-a005-20200827
x86_64 randconfig-a006-20200827
x86_64 randconfig-a004-20200827
i386 randconfig-a002-20200827
i386 randconfig-a004-20200827
i386 randconfig-a003-20200827
i386 randconfig-a005-20200827
i386 randconfig-a006-20200827
i386 randconfig-a001-20200827
i386 randconfig-a013-20200827
i386 randconfig-a012-20200827
i386 randconfig-a011-20200827
i386 randconfig-a016-20200827
i386 randconfig-a015-20200827
i386 randconfig-a014-20200827
riscv allyesconfig
riscv allnoconfig
riscv defconfig
riscv allmodconfig
x86_64 rhel
x86_64 rhel-7.6-kselftests
x86_64 defconfig
x86_64 rhel-8.3
x86_64 kexec
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply
* [powerpc:fixes-test] BUILD SUCCESS 16d83a540ca4e7f1ebb2b3756869b77451d31414
From: kernel test robot @ 2020-08-27 20:00 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git fixes-test
branch HEAD: 16d83a540ca4e7f1ebb2b3756869b77451d31414 Revert "powerpc/powernv/idle: Replace CPU feature check with PVR check"
elapsed time: 736m
configs tested: 142
configs skipped: 15
The following configs have been built successfully.
More configs may be tested in the coming days.
arm defconfig
arm64 allyesconfig
arm64 defconfig
arm allyesconfig
arm allmodconfig
riscv nommu_k210_defconfig
mips decstation_defconfig
arm milbeaut_m10v_defconfig
powerpc mpc7448_hpc2_defconfig
sh ul2_defconfig
arm u8500_defconfig
arm badge4_defconfig
m68k sun3_defconfig
arm ixp4xx_defconfig
m68k multi_defconfig
powerpc ps3_defconfig
arm rpc_defconfig
sh sdk7780_defconfig
arm ezx_defconfig
mips e55_defconfig
arm pxa910_defconfig
s390 debug_defconfig
sh se7712_defconfig
arm stm32_defconfig
sparc64 alldefconfig
arm pxa168_defconfig
sh apsh4ad0a_defconfig
mips ath25_defconfig
ia64 allmodconfig
sh sh7763rdp_defconfig
arm orion5x_defconfig
powerpc mpc885_ads_defconfig
sh rsk7203_defconfig
powerpc pq2fads_defconfig
mips xway_defconfig
m68k sun3x_defconfig
sh shmin_defconfig
mips malta_defconfig
arm spitz_defconfig
mips rbtx49xx_defconfig
sh se7721_defconfig
powerpc mpc5200_defconfig
arm pxa255-idp_defconfig
mips ip22_defconfig
sh apsh4a3a_defconfig
microblaze mmu_defconfig
microblaze defconfig
arc tb10x_defconfig
sh sh7757lcr_defconfig
mips maltaaprp_defconfig
arm hackkit_defconfig
arm moxart_defconfig
parisc alldefconfig
arm mvebu_v7_defconfig
arm mainstone_defconfig
m68k allmodconfig
c6x dsk6455_defconfig
powerpc powernv_defconfig
mips rb532_defconfig
ia64 bigsur_defconfig
arm multi_v5_defconfig
sh ecovec24_defconfig
arm axm55xx_defconfig
sh urquell_defconfig
arm assabet_defconfig
h8300 h8300h-sim_defconfig
h8300 defconfig
m68k apollo_defconfig
sh sh03_defconfig
sh edosk7760_defconfig
sh sh2007_defconfig
sparc allyesconfig
sh microdev_defconfig
arm magician_defconfig
c6x evmc6678_defconfig
sparc defconfig
arc vdk_hs38_smp_defconfig
s390 zfcpdump_defconfig
x86_64 allyesconfig
mips pic32mzda_defconfig
arm netwinder_defconfig
nios2 alldefconfig
powerpc amigaone_defconfig
powerpc skiroot_defconfig
powerpc tqm8xx_defconfig
arm aspeed_g4_defconfig
ia64 defconfig
ia64 allyesconfig
m68k defconfig
m68k allyesconfig
nios2 defconfig
arc allyesconfig
nds32 allnoconfig
c6x allyesconfig
nds32 defconfig
nios2 allyesconfig
csky defconfig
alpha defconfig
alpha allyesconfig
xtensa allyesconfig
h8300 allyesconfig
arc defconfig
sh allmodconfig
parisc defconfig
s390 allyesconfig
parisc allyesconfig
s390 defconfig
i386 allyesconfig
i386 defconfig
mips allyesconfig
mips allmodconfig
powerpc defconfig
powerpc allyesconfig
powerpc allmodconfig
powerpc allnoconfig
x86_64 randconfig-a005-20200827
x86_64 randconfig-a006-20200827
x86_64 randconfig-a004-20200827
x86_64 randconfig-a003-20200827
x86_64 randconfig-a002-20200827
x86_64 randconfig-a001-20200827
i386 randconfig-a002-20200827
i386 randconfig-a004-20200827
i386 randconfig-a003-20200827
i386 randconfig-a005-20200827
i386 randconfig-a006-20200827
i386 randconfig-a001-20200827
i386 randconfig-a016-20200827
i386 randconfig-a015-20200827
i386 randconfig-a013-20200827
i386 randconfig-a012-20200827
i386 randconfig-a011-20200827
i386 randconfig-a014-20200827
riscv allyesconfig
riscv allnoconfig
riscv defconfig
riscv allmodconfig
x86_64 rhel
x86_64 rhel-7.6-kselftests
x86_64 defconfig
x86_64 rhel-8.3
x86_64 kexec
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply
* Re: [PATCH v1 04/10] powerpc/kernel/iommu: Add new iommu_table_in_use() helper
From: Leonardo Bras @ 2020-08-27 18:34 UTC (permalink / raw)
To: Alexey Kardashevskiy, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Christophe Leroy, Joel Stanley,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <e7d0e85c-c4c4-ad1d-899a-72d4fbd92852@ozlabs.ru>
On Sat, 2020-08-22 at 20:34 +1000, Alexey Kardashevskiy wrote:
> > +
> > + /*ignore reserved bit0*/
>
> s/ignore reserved bit0/ ignore reserved bit0 / (add spaces)
Fixed
> > + if (tbl->it_offset == 0)
> > + p1_start = 1;
> > +
> > + /* Check if reserved memory is valid*/
>
> A missing space here.
Fixed
>
> > + if (tbl->it_reserved_start >= tbl->it_offset &&
> > + tbl->it_reserved_start <= (tbl->it_offset + tbl->it_size) &&
> > + tbl->it_reserved_end >= tbl->it_offset &&
> > + tbl->it_reserved_end <= (tbl->it_offset + tbl->it_size)) {
>
> Uff. What if tbl->it_reserved_end is bigger than tbl->it_offset +
> tbl->it_size?
>
> The reserved area is to preserve MMIO32 so it is for it_offset==0 only
> and the boundaries are checked in the only callsite, and it is unlikely
> to change soon or ever.
>
> Rather that bothering with fixing that, may be just add (did not test):
>
> if (WARN_ON((
> (tbl->it_reserved_start || tbl->it_reserved_end) && (it_offset != 0))
> (tbl->it_reserved_start > it_offset && tbl->it_reserved_end < it_offset
> + it_size) && (it_offset == 0)) )
> return true;
>
> Or simply always look for it_offset..it_reserved_start and
> it_reserved_end..it_offset+it_size and if there is no reserved area,
> initialize it_reserved_start=it_reserved_end=it_offset so the first
> it_offset..it_reserved_start becomes a no-op.
The problem here is that the values of it_reserved_{start,end} are not
necessarily valid. I mean, on iommu_table_reserve_pages() the values
are stored however they are given (bit reserving is done only if they
are valid).
Having a it_reserved_{start,end} value outside the valid ranges would
cause find_next_bit() to run over memory outside the bitmap.
Even if the those values are < tbl->it_offset, the resulting
subtraction on unsigned would cause it to become a big value and run
over memory outside the bitmap.
But I think you are right. That is not the place to check if the
reserved values are valid. It should just trust them here.
I intent to change iommu_table_reserve_pages() to only store the
parameters in it_reserved_{start,end} if they are in the range, and or
it_offset in both of them if they are not.
What do you think?
Thanks for the feedback!
Leonardo Bras
^ permalink raw reply
* [PATCH v2] powerpc/32s: Disable VMAP stack which CONFIG_ADB_PMU
From: Christophe Leroy @ 2020-08-27 18:30 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
giuseppe
Cc: linuxppc-dev, linux-kernel
low_sleep_handler() can't restore the context from virtual
stack because the stack can hardly be accessed with MMU OFF.
For now, disable VMAP stack when CONFIG_ADB_PMU is selected.
Reported-by: Giuseppe Sacco <giuseppe@sguazz.it>
Fixes: cd08f109e262 ("powerpc/32s: Enable CONFIG_VMAP_STACK")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v2: Argh, went too quick. CONFIG_ADB_PMU ==> ADB_PMU
---
arch/powerpc/platforms/Kconfig.cputype | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
index 87737ec86d39..1dc9d3c81872 100644
--- a/arch/powerpc/platforms/Kconfig.cputype
+++ b/arch/powerpc/platforms/Kconfig.cputype
@@ -36,7 +36,7 @@ config PPC_BOOK3S_6xx
select PPC_HAVE_PMU_SUPPORT
select PPC_HAVE_KUEP
select PPC_HAVE_KUAP
- select HAVE_ARCH_VMAP_STACK
+ select HAVE_ARCH_VMAP_STACK if !ADB_PMU
config PPC_BOOK3S_601
bool "PowerPC 601"
--
2.25.0
^ permalink raw reply related
* Re: kernel since 5.6 do not boot anymore on Apple PowerBook
From: Christophe Leroy @ 2020-08-27 18:28 UTC (permalink / raw)
To: Giuseppe Sacco, linuxppc-dev
In-Reply-To: <37c4253abc08ef3e93d67cae5b92252af96fcf65.camel@sguazz.it>
Le 27/08/2020 à 16:37, Giuseppe Sacco a écrit :
> Il giorno gio, 27/08/2020 alle 12.39 +0200, Christophe Leroy ha
> scritto:
>> Hi,
>>
>> Le 27/08/2020 à 10:28, Giuseppe Sacco a écrit :
> [...]
>>> Sorry, I made a mistake. The real problem is down, on the same
>>> function, when it calls low_sleep_handler(). This is where the problem
>>> probably is.
>>
>> Great, you spotted the problem.
>>
>> I see what it is, it is in low_sleep_handler() in
>> arch/powerpc/platforms/powermac/sleep.S
>>
>> All critical registers are saved on the stack. At restore, they are
>> restore BEFORE re-enabling MMU (because they are needed for that). But
>> when we have VMAP_STACK, the stack can hardly be accessed without the
>> MMU enabled. tophys() doesn't work for virtual stack addresses.
>>
>> Therefore, the low_sleep_handler() has to be reworked for using an area
>> in the linear mem instead of the stack.
>
> I am sorry, but I don't know how to fix it. Should I open a bug for
> tracking this problem?
Yes please, at https://github.com/linuxppc/issues/issues
In the meantime, I have sent a patch to disable CONFIG_VMAP_STACK when
CONFIG_ADB_PMU is selected until this is fixed.
Have you tried without CONFIG_ADB_PMU ? Or does it make no sense ?
Christophe
^ permalink raw reply
* [PATCH] powerpc/32s: Disable VMAP stack which CONFIG_ADB_PMU
From: Christophe Leroy @ 2020-08-27 18:26 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
giuseppe
Cc: linuxppc-dev, linux-kernel
low_sleep_handler() can't restore the context from virtual
stack because the stack can hardly be accessed with MMU OFF.
For now, disable VMAP stack when CONFIG_ADB_PMU is selected.
Reported-by: Giuseppe Sacco <giuseppe@sguazz.it>
Fixes: cd08f109e262 ("powerpc/32s: Enable CONFIG_VMAP_STACK")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/platforms/Kconfig.cputype | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
index 87737ec86d39..c12768242c17 100644
--- a/arch/powerpc/platforms/Kconfig.cputype
+++ b/arch/powerpc/platforms/Kconfig.cputype
@@ -36,7 +36,7 @@ config PPC_BOOK3S_6xx
select PPC_HAVE_PMU_SUPPORT
select PPC_HAVE_KUEP
select PPC_HAVE_KUAP
- select HAVE_ARCH_VMAP_STACK
+ select HAVE_ARCH_VMAP_STACK if !CONFIG_ADB_PMU
config PPC_BOOK3S_601
bool "PowerPC 601"
--
2.25.0
^ permalink raw reply related
* Re: [PATCH 08/10] x86: remove address space overrides using set_fs()
From: Linus Torvalds @ 2020-08-27 18:15 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, Kees Cook, the arch/x86 maintainers,
Linux Kernel Mailing List, Al Viro, linux-fsdevel, linuxppc-dev
In-Reply-To: <20200827150030.282762-9-hch@lst.de>
On Thu, Aug 27, 2020 at 8:00 AM Christoph Hellwig <hch@lst.de> wrote:
>
> SYM_FUNC_START(__get_user_2)
> add $1,%_ASM_AX
> jc bad_get_user
This no longer makes sense, and
> - mov PER_CPU_VAR(current_task), %_ASM_DX
> - cmp TASK_addr_limit(%_ASM_DX),%_ASM_AX
> + LOAD_TASK_SIZE_MAX
> + cmp %_ASM_DX,%_ASM_AX
This should be
LOAD_TASK_SIZE_MAX_MINUS_N(1)
cmp %_ASM_DX,%_ASM_AX
instead (and then because we no longer modify _ASM_AX, we'd also
remove the offset on the access).
> SYM_FUNC_START(__put_user_2)
> - ENTER
> - mov TASK_addr_limit(%_ASM_BX),%_ASM_BX
> + LOAD_TASK_SIZE_MAX
> sub $1,%_ASM_BX
It's even more obvious here. We load a constant and then immediately
do a "sub $1" on that value.
It's not a huge deal, you don't have to respin the series for this, I
just wanted to point it out so that people are aware of it and if I
forget somebody else will hopefully remember that "we should fix that
too".
Linus
^ permalink raw reply
* Re: [PATCH v3 3/6] Add LKDTM test to hijack a patch mapping (powerpc, x86_64)
From: kernel test robot @ 2020-08-27 18:10 UTC (permalink / raw)
To: Christopher M. Riedl, linuxppc-dev; +Cc: kbuild-all, kernel-hardening
In-Reply-To: <20200827052659.24922-4-cmr@codefail.de>
[-- Attachment #1: Type: text/plain, Size: 1365 bytes --]
Hi "Christopher,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on char-misc/char-misc-testing tip/x86/core v5.9-rc2 next-20200827]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Christopher-M-Riedl/Use-per-CPU-temporary-mappings-for-patching/20200827-161532
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: parisc-allyesconfig (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> hppa-linux-ld: drivers/misc/lkdtm/core.o:(.rodata+0x1b4): undefined reference to `lkdtm_HIJACK_PATCH'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 65969 bytes --]
^ permalink raw reply
* Re: [PATCH 05/10] lkdtm: disable set_fs-based tests for !CONFIG_SET_FS
From: Linus Torvalds @ 2020-08-27 18:06 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-arch, Kees Cook, the arch/x86 maintainers,
Linux Kernel Mailing List, Al Viro, linux-fsdevel, linuxppc-dev
In-Reply-To: <20200827150030.282762-6-hch@lst.de>
On Thu, Aug 27, 2020 at 8:00 AM Christoph Hellwig <hch@lst.de> wrote:
>
> Once we can't manipulate the address limit, we also can't test what
> happens when the manipulation is abused.
Just remove these tests entirely.
Once set_fs() doesn't exist on x86, the tests no longer make any sense
what-so-ever, because test coverage will be basically zero.
So don't make the code uglier just to maintain a fiction that
something is tested when it isn't really.
Linus
^ permalink raw reply
* Re: [PATCH v1 03/10] powerpc/kernel/iommu: Use largepool as a last resort when !largealloc
From: Leonardo Bras @ 2020-08-27 16:58 UTC (permalink / raw)
To: Alexey Kardashevskiy, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Christophe Leroy, Joel Stanley,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <75fb1e8b-2506-ffd8-64af-4abfefe03d2f@ozlabs.ru>
On Sat, 2020-08-22 at 20:09 +1000, Alexey Kardashevskiy wrote:
> > + goto again;
> > +
>
> A nit: unnecessary new line.
I was following the pattern used above. There is a newline after every
"goto again" in this 'if'.
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Thank you!
^ permalink raw reply
* Re: [PATCH v1 02/10] powerpc/kernel/iommu: Align size for IOMMU_PAGE_SIZE on iommu_*_coherent()
From: Leonardo Bras @ 2020-08-27 16:51 UTC (permalink / raw)
To: Alexey Kardashevskiy, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Christophe Leroy, Joel Stanley,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <7b9640e0-568f-1470-40f4-a3ccec8abcf2@ozlabs.ru>
On Sat, 2020-08-22 at 20:07 +1000, Alexey Kardashevskiy wrote:
>
> On 18/08/2020 09:40, Leonardo Bras wrote:
> > Both iommu_alloc_coherent() and iommu_free_coherent() assume that once
> > size is aligned to PAGE_SIZE it will be aligned to IOMMU_PAGE_SIZE.
>
> The only case when it is not aligned is when IOMMU_PAGE_SIZE > PAGE_SIZE
> which is unlikely but not impossible, we could configure the kernel for
> 4K system pages and 64K IOMMU pages I suppose. Do we really want to do
> this here, or simply put WARN_ON(tbl->it_page_shift > PAGE_SHIFT)?
I think it would be better to keep the code as much generic as possible
regarding page sizes.
> Because if we want the former (==support), then we'll have to align the
> size up to the bigger page size when allocating/zeroing system pages,
> etc.
This part I don't understand. Why do we need to align everything to the
bigger pagesize?
I mean, is not that enough that the range [ret, ret + size[ is both
allocated by mm and mapped on a iommu range?
Suppose a iommu_alloc_coherent() of 16kB on PAGESIZE = 4k and
IOMMU_PAGE_SIZE() == 64k.
Why 4 * cpu_pages mapped by a 64k IOMMU page is not enough?
All the space the user asked for is allocated and mapped for DMA.
> Bigger pages are not the case here as I understand it.
I did not get this part, what do you mean?
> > Update those functions to guarantee alignment with requested size
> > using IOMMU_PAGE_ALIGN() before doing iommu_alloc() / iommu_free().
> >
> > Also, on iommu_range_alloc(), replace ALIGN(n, 1 << tbl->it_page_shift)
> > with IOMMU_PAGE_ALIGN(n, tbl), which seems easier to read.
> >
> > Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> > ---
> > arch/powerpc/kernel/iommu.c | 17 +++++++++--------
> > 1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> > index 9704f3f76e63..d7086087830f 100644
> > --- a/arch/powerpc/kernel/iommu.c
> > +++ b/arch/powerpc/kernel/iommu.c
> > @@ -237,10 +237,9 @@ static unsigned long iommu_range_alloc(struct device *dev,
> > }
> >
> > if (dev)
> > - boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
> > - 1 << tbl->it_page_shift);
> > + boundary_size = IOMMU_PAGE_ALIGN(dma_get_seg_boundary(dev) + 1, tbl);
>
> Run checkpatch.pl, should complain about a long line.
It's 86 columns long, which is less than the new limit of 100 columns
Linus announced a few weeks ago. checkpatch.pl was updated too:
https://www.phoronix.com/scan.php?page=news_item&px=Linux-Kernel-Deprecates-80-Col
>
>
> > else
> > - boundary_size = ALIGN(1UL << 32, 1 << tbl->it_page_shift);
> > + boundary_size = IOMMU_PAGE_ALIGN(1UL << 32, tbl);
> > /* 4GB boundary for iseries_hv_alloc and iseries_hv_map */
> >
> > n = iommu_area_alloc(tbl->it_map, limit, start, npages, tbl->it_offset,
> > @@ -858,6 +857,7 @@ void *iommu_alloc_coherent(struct device *dev, struct iommu_table *tbl,
> > unsigned int order;
> > unsigned int nio_pages, io_order;
> > struct page *page;
> > + size_t size_io = size;
> >
> > size = PAGE_ALIGN(size);
> > order = get_order(size);
> > @@ -884,8 +884,9 @@ void *iommu_alloc_coherent(struct device *dev, struct iommu_table *tbl,
> > memset(ret, 0, size);
> >
> > /* Set up tces to cover the allocated range */
> > - nio_pages = size >> tbl->it_page_shift;
> > - io_order = get_iommu_order(size, tbl);
> > + size_io = IOMMU_PAGE_ALIGN(size_io, tbl);
> > + nio_pages = size_io >> tbl->it_page_shift;
> > + io_order = get_iommu_order(size_io, tbl);
> > mapping = iommu_alloc(dev, tbl, ret, nio_pages, DMA_BIDIRECTIONAL,
> > mask >> tbl->it_page_shift, io_order, 0);
> > if (mapping == DMA_MAPPING_ERROR) {
> > @@ -900,11 +901,11 @@ void iommu_free_coherent(struct iommu_table *tbl, size_t size,
> > void *vaddr, dma_addr_t dma_handle)
> > {
> > if (tbl) {
> > - unsigned int nio_pages;
> > + size_t size_io = IOMMU_PAGE_ALIGN(size, tbl);
> > + unsigned int nio_pages = size_io >> tbl->it_page_shift;
> >
> > - size = PAGE_ALIGN(size);
> > - nio_pages = size >> tbl->it_page_shift;
> > iommu_free(tbl, dma_handle, nio_pages);
> > +
>
> Unrelated new line.
Will be removed. Thanks!
>
>
> > size = PAGE_ALIGN(size);
> > free_pages((unsigned long)vaddr, get_order(size));
> > }
> >
^ permalink raw reply
* Re: [PATCH v2 25/25] powerpc/signal32: Transform save_user_regs() and save_tm_user_regs() in 'unsafe' version
From: Christophe Leroy @ 2020-08-27 15:59 UTC (permalink / raw)
To: kernel test robot, Michael Ellerman
Cc: linuxppc-dev@lists.ozlabs.org, kbuild-all
In-Reply-To: <202008271728.tFAPDKU8%lkp@intel.com>
Le 27/08/2020 à 11:07, kernel test robot a écrit :
> Hi Christophe,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on powerpc/next]
> [also build test ERROR on linus/master v5.9-rc2 next-20200827]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-Switch-signal-32-to-using-unsafe_put_user-and-friends/20200819-012411
> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> config: powerpc64-randconfig-r005-20200827 (attached as .config)
> compiler: powerpc-linux-gcc (GCC) 9.3.0
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc64
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
>
> All errors (new ones prefixed by >>):
>
> arch/powerpc/kernel/signal_32.c: In function 'save_user_regs_unsafe':
>>> arch/powerpc/kernel/signal_32.c:314:34: error: macro "unsafe_copy_to_user" requires 4 arguments, but only 3 given
> 314 | ELF_NEVRREG * sizeof(u32)), failed);
> | ^
> In file included from include/linux/uaccess.h:9,
> from include/linux/sched/task.h:11,
> from include/linux/sched/signal.h:9,
> from include/linux/rcuwait.h:6,
> from include/linux/percpu-rwsem.h:7,
> from include/linux/fs.h:33,
> from include/linux/huge_mm.h:8,
> from include/linux/mm.h:672,
> from arch/powerpc/kernel/signal_32.c:17:
> arch/powerpc/include/asm/uaccess.h:605: note: macro "unsafe_copy_to_user" defined here
> 605 | #define unsafe_copy_to_user(d, s, l, e) \
> |
>>> arch/powerpc/kernel/signal_32.c:313:3: error: 'unsafe_copy_to_user' undeclared (first use in this function); did you mean 'raw_copy_to_user'?
> 313 | unsafe_copy_to_user(&frame->mc_vregs, current->thread.evr,
> | ^~~~~~~~~~~~~~~~~~~
> | raw_copy_to_user
> arch/powerpc/kernel/signal_32.c:313:3: note: each undeclared identifier is reported only once for each function it appears in
>>> arch/powerpc/kernel/signal_32.c:314:37: error: 'failed' undeclared (first use in this function)
> 314 | ELF_NEVRREG * sizeof(u32)), failed);
> | ^~~~~~
> arch/powerpc/kernel/signal_32.c:314:35: warning: left-hand operand of comma expression has no effect [-Wunused-value]
> 314 | ELF_NEVRREG * sizeof(u32)), failed);
> | ^
>>> arch/powerpc/kernel/signal_32.c:314:43: error: expected ';' before ')' token
> 314 | ELF_NEVRREG * sizeof(u32)), failed);
> | ^
> | ;
>>> arch/powerpc/kernel/signal_32.c:314:43: error: expected statement before ')' token
>
Should be fixed by:
diff --git a/arch/powerpc/kernel/signal_32.c
b/arch/powerpc/kernel/signal_32.c
index f795fe0240a1..123682299d4f 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -311,7 +311,7 @@ static int save_user_regs_unsafe(struct pt_regs
*regs, struct mcontext __user *f
/* save spe registers */
if (current->thread.used_spe) {
unsafe_copy_to_user(&frame->mc_vregs, current->thread.evr,
- ELF_NEVRREG * sizeof(u32)), failed);
+ ELF_NEVRREG * sizeof(u32), failed);
/* set MSR_SPE in the saved MSR value to indicate that
frame->mc_vregs contains valid data */
msr |= MSR_SPE;
---
Christophe
^ permalink raw reply related
* RE: [PATCH 01/10] fs: don't allow kernel reads and writes without iter ops
From: David Laight @ 2020-08-27 15:58 UTC (permalink / raw)
To: 'Christoph Hellwig', Linus Torvalds, Al Viro,
Michael Ellerman, x86@kernel.org
Cc: linux-fsdevel@vger.kernel.org, linux-arch@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, Kees Cook,
linux-kernel@vger.kernel.org
In-Reply-To: <20200827150030.282762-2-hch@lst.de>
From: Christoph Hellwig
> Sent: 27 August 2020 16:00
>
> Don't allow calling ->read or ->write with set_fs as a preparation for
> killing off set_fs. All the instances that we use kernel_read/write on
> are using the iter ops already.
>
> If a file has both the regular ->read/->write methods and the iter
> variants those could have different semantics for messed up enough
> drivers. Also fails the kernel access to them in that case.
Is there a real justification for that?
For system calls supplying both methods makes sense to avoid
the extra code paths for a simple read/write.
Any one stupid enough to make them behave differently gets
what they deserve.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox