* Re: [PATCH v2 17/20] mm: free_area_init: allow defining max_zone_pfn in descending order
From: Mike Rapoport @ 2020-05-05 13:45 UTC (permalink / raw)
To: Guenter Roeck
Cc: Rich Felker, linux-ia64, linux-doc, Catalin Marinas,
Heiko Carstens, x86, Michal Hocko, James E.J. Bottomley,
Max Filippov, Guo Ren, Ley Foon Tan, sparclinux, linux-riscv,
Greg Ungerer, linux-arch, linux-s390, linux-c6x-dev, Baoquan He,
Jonathan Corbet, linux-hexagon, Helge Deller, linux-sh,
Russell King, linux-csky, Geert Uytterhoeven, Hoan Tran,
Mark Salter, Matt Turner, linux-snps-arc, uclinux-h8-devel,
linux-xtensa, Nick Hu, linux-alpha, linux-um, linux-mips,
Richard Weinberger, linux-m68k, Thomas Bogendoerfer, Qian Cai,
Greentime Hu, Paul Walmsley, Stafford Horne, Guan Xuetao,
linux-arm-kernel, Michal Simek, Tony Luck, Yoshinori Sato,
linux-parisc, linux-mm, Vineet Gupta, Brian Cain, linux-kernel,
openrisc, Andrew Morton, linuxppc-dev, David S. Miller,
Mike Rapoport
In-Reply-To: <ca099c3e-c0bc-cd2f-cdb0-852dfc2c10db@roeck-us.net>
On Tue, May 05, 2020 at 06:18:11AM -0700, Guenter Roeck wrote:
> On 5/4/20 8:39 AM, Mike Rapoport wrote:
> > On Sun, May 03, 2020 at 11:43:00AM -0700, Guenter Roeck wrote:
> >> On Sun, May 03, 2020 at 10:41:38AM -0700, Guenter Roeck wrote:
> >>> Hi,
> >>>
> >>> On Wed, Apr 29, 2020 at 03:11:23PM +0300, Mike Rapoport wrote:
> >>>> From: Mike Rapoport <rppt@linux.ibm.com>
> >>>>
> >>>> Some architectures (e.g. ARC) have the ZONE_HIGHMEM zone below the
> >>>> ZONE_NORMAL. Allowing free_area_init() parse max_zone_pfn array even it is
> >>>> sorted in descending order allows using free_area_init() on such
> >>>> architectures.
> >>>>
> >>>> Add top -> down traversal of max_zone_pfn array in free_area_init() and use
> >>>> the latter in ARC node/zone initialization.
> >>>>
> >>>> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> >>>
> >>> This patch causes my microblazeel qemu boot test in linux-next to fail.
> >>> Reverting it fixes the problem.
> >>>
> >> The same problem is seen with s390 emulations.
> >
> > Yeah, this patch breaks some others as well :(
> >
> > My assumption that max_zone_pfn defines architectural limit for maximal
> > PFN that can belong to a zone was over-optimistic. Several arches
> > actually do that, but others do
> >
> > max_zone_pfn[ZONE_DMA] = MAX_DMA_PFN;
> > max_zone_pfn[ZONE_NORMAL] = max_pfn;
> >
> > where MAX_DMA_PFN is build-time constrain and max_pfn is run time limit
> > for the current system.
> >
> > So, when max_pfn is lower than MAX_DMA_PFN, the free_init_area() will
> > consider max_zone_pfn as descending and will wrongly calculate zone
> > extents.
> >
> > That said, instead of trying to create a generic way to special case
> > ARC, I suggest to simply use the below patch instead.
> >
>
> As a reminder, I reported the problem against s390 and microblazeel
> (interestingly enough, microblaze (big endian) works), not against arc.
With this fix microblazeel and s390 worked for me and also Christian had
reported that s390 is fixed.
microblaze (big endian) works because its defconfig does not enable HIGHMEM
while little endian does.
ARC is mentioned because it is the only arch that may have ZONE_HIGHMEM
and ZONE_NORMAL and this patch was required to consolidate
free_area_init* variants.
> Guenter
>
> > diff --git a/arch/arc/mm/init.c b/arch/arc/mm/init.c
> > index 41eb9be1653c..386959bac3d2 100644
> > --- a/arch/arc/mm/init.c
> > +++ b/arch/arc/mm/init.c
> > @@ -77,6 +77,11 @@ void __init early_init_dt_add_memory_arch(u64 base, u64 size)
> > base, TO_MB(size), !in_use ? "Not used":"");
> > }
> >
> > +bool arch_has_descending_max_zone_pfns(void)
> > +{
> > + return true;
> > +}
> > +
> > /*
> > * First memory setup routine called from setup_arch()
> > * 1. setup swapper's mm @init_mm
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index b990e9734474..114f0e027144 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -7307,6 +7307,15 @@ static void check_for_memory(pg_data_t *pgdat, int nid)
> > }
> > }
> >
> > +/*
> > + * Some architecturs, e.g. ARC may have ZONE_HIGHMEM below ZONE_NORMAL. For
> > + * such cases we allow max_zone_pfn sorted in the descending order
> > + */
> > +bool __weak arch_has_descending_max_zone_pfns(void)
> > +{
> > + return false;
> > +}
> > +
> > /**
> > * free_area_init - Initialise all pg_data_t and zone data
> > * @max_zone_pfn: an array of max PFNs for each zone
> > @@ -7324,7 +7333,7 @@ void __init free_area_init(unsigned long *max_zone_pfn)
> > {
> > unsigned long start_pfn, end_pfn;
> > int i, nid, zone;
> > - bool descending = false;
> > + bool descending;
> >
> > /* Record where the zone boundaries are */
> > memset(arch_zone_lowest_possible_pfn, 0,
> > @@ -7333,14 +7342,7 @@ void __init free_area_init(unsigned long *max_zone_pfn)
> > sizeof(arch_zone_highest_possible_pfn));
> >
> > start_pfn = find_min_pfn_with_active_regions();
> > -
> > - /*
> > - * Some architecturs, e.g. ARC may have ZONE_HIGHMEM below
> > - * ZONE_NORMAL. For such cases we allow max_zone_pfn sorted in the
> > - * descending order
> > - */
> > - if (MAX_NR_ZONES > 1 && max_zone_pfn[0] > max_zone_pfn[1])
> > - descending = true;
> > + descending = arch_has_descending_max_zone_pfns();
> >
> > for (i = 0; i < MAX_NR_ZONES; i++) {
> > if (descending)
> >
> >> Guenter
> >>
> >>> qemu command line:
> >>>
> >>> qemu-system-microblazeel -M petalogix-ml605 -m 256 \
> >>> -kernel arch/microblaze/boot/linux.bin -no-reboot \
> >>> -initrd rootfs.cpio \
> >>> -append 'panic=-1 slub_debug=FZPUA rdinit=/sbin/init console=ttyS0,115200' \
> >>> -monitor none -serial stdio -nographic
> >>>
> >>> initrd:
> >>> https://github.com/groeck/linux-build-test/blob/master/rootfs/microblazeel/rootfs.cpio.gz
> >>> configuration:
> >>> https://github.com/groeck/linux-build-test/blob/master/rootfs/microblazeel/qemu_microblazeel_ml605_defconfig
> >>>
> >>> Bisect log is below.
> >>>
> >>> Guenter
> >>>
> >>> ---
> >>> # bad: [fb9d670f57e3f6478602328bbbf71138be06ca4f] Add linux-next specific files for 20200501
> >>> # good: [6a8b55ed4056ea5559ebe4f6a4b247f627870d4c] Linux 5.7-rc3
> >>> git bisect start 'HEAD' 'v5.7-rc3'
> >>> # good: [068b80b68a670f0b17288c8a3d1ee751f35162ab] Merge remote-tracking branch 'drm/drm-next'
> >>> git bisect good 068b80b68a670f0b17288c8a3d1ee751f35162ab
> >>> # good: [46c70fc6a3ac35cd72ddad248dcbe4eee716d2a5] Merge remote-tracking branch 'drivers-x86/for-next'
> >>> git bisect good 46c70fc6a3ac35cd72ddad248dcbe4eee716d2a5
> >>> # good: [f39c4ad479a2f005f972a2b941b40efa6b9c9349] Merge remote-tracking branch 'rpmsg/for-next'
> >>> git bisect good f39c4ad479a2f005f972a2b941b40efa6b9c9349
> >>> # bad: [165d3ee0162fe28efc2c8180176633e33515df15] ipc-convert-ipcs_idr-to-xarray-update
> >>> git bisect bad 165d3ee0162fe28efc2c8180176633e33515df15
> >>> # good: [001f1d211ed2ed0f005838dc4390993930bbbd69] mm: remove early_pfn_in_nid() and CONFIG_NODES_SPAN_OTHER_NODES
> >>> git bisect good 001f1d211ed2ed0f005838dc4390993930bbbd69
> >>> # bad: [aaad7401bd32f10c1d591dd886b3a9b9595c6d77] mm/vmsan: fix some typos in comment
> >>> git bisect bad aaad7401bd32f10c1d591dd886b3a9b9595c6d77
> >>> # bad: [09f9d0ab1fbed85623b283995aa7a7d78daa1611] khugepaged: allow to collapse PTE-mapped compound pages
> >>> git bisect bad 09f9d0ab1fbed85623b283995aa7a7d78daa1611
> >>> # bad: [c942fc8a3e5088407bc32d94f554bab205175f8a] mm/vmstat.c: do not show lowmem reserve protection information of empty zone
> >>> git bisect bad c942fc8a3e5088407bc32d94f554bab205175f8a
> >>> # bad: [b29358d269ace3826d8521bea842fc2984cfc11b] mm/page_alloc.c: rename free_pages_check() to check_free_page()
> >>> git bisect bad b29358d269ace3826d8521bea842fc2984cfc11b
> >>> # bad: [be0fb591a1f1df20a00c8f023f9ca4891f177b0d] mm: simplify find_min_pfn_with_active_regions()
> >>> git bisect bad be0fb591a1f1df20a00c8f023f9ca4891f177b0d
> >>> # bad: [c17422a008d36dcf3e9f51469758c5762716cb0a] mm: rename free_area_init_node() to free_area_init_memoryless_node()
> >>> git bisect bad c17422a008d36dcf3e9f51469758c5762716cb0a
> >>> # bad: [51a2f644fd020d5f090044825c388444d11029d5] mm: free_area_init: allow defining max_zone_pfn in descending order
> >>> git bisect bad 51a2f644fd020d5f090044825c388444d11029d5
> >>> # first bad commit: [51a2f644fd020d5f090044825c388444d11029d5] mm: free_area_init: allow defining max_zone_pfn in descending order
> >
>
--
Sincerely yours,
Mike.
^ permalink raw reply
* Re: [PATCH v4 1/2] powerpc/uaccess: Implement unsafe_put_user() using 'asm goto'
From: Michael Ellerman @ 2020-05-05 14:27 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras, npiggin,
segher
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <23e680624680a9a5405f4b88740d2596d4b17c26.1587143308.git.christophe.leroy@c-s.fr>
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> unsafe_put_user() is designed to take benefit of 'asm goto'.
>
> Instead of using the standard __put_user() approach and branch
> based on the returned error, use 'asm goto' and make the
> exception code branch directly to the error label. There is
> no code anymore in the fixup section.
>
> This change significantly simplifies functions using
> unsafe_put_user()
>
...
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/include/asm/uaccess.h | 61 +++++++++++++++++++++++++-----
> 1 file changed, 52 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 9cc9c106ae2a..9365b59495a2 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -196,6 +193,52 @@ do { \
> })
>
>
> +#define __put_user_asm_goto(x, addr, label, op) \
> + asm volatile goto( \
> + "1: " op "%U1%X1 %0,%1 # put_user\n" \
> + EX_TABLE(1b, %l2) \
> + : \
> + : "r" (x), "m<>" (*addr) \
The "m<>" here is breaking GCC 4.6.3, which we allegedly still support.
Plain "m" works, how much does the "<>" affect code gen in practice?
A quick diff here shows no difference from removing "<>".
cheers
^ permalink raw reply
* Re: [PATCH v3 00/29] Convert files to ReST - part 2
From: Jonathan Corbet @ 2020-05-05 15:28 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linux Doc Mailing List, linux-usb, linux-kernel, codalist,
linux-xfs, linux-cachefs, linux-fsdevel, linuxppc-dev
In-Reply-To: <20200428130128.22c4b973@lwn.net>
On Tue, 28 Apr 2020 13:01:28 -0600
Jonathan Corbet <corbet@lwn.net> wrote:
> So I'm happy to merge this set, but there is one thing that worries me a
> bit...
>
> > fs/cachefiles/Kconfig | 4 +-
> > fs/coda/Kconfig | 2 +-
> > fs/configfs/inode.c | 2 +-
> > fs/configfs/item.c | 2 +-
> > fs/fscache/Kconfig | 8 +-
> > fs/fscache/cache.c | 8 +-
> > fs/fscache/cookie.c | 2 +-
> > fs/fscache/object.c | 4 +-
> > fs/fscache/operation.c | 2 +-
> > fs/locks.c | 2 +-
> > include/linux/configfs.h | 2 +-
> > include/linux/fs_context.h | 2 +-
> > include/linux/fscache-cache.h | 4 +-
> > include/linux/fscache.h | 42 +-
> > include/linux/lsm_hooks.h | 2 +-
>
> I'd feel a bit better if I could get an ack or two from filesystem folks
> before I venture that far out of my own yard...what say you all?
It's been another week and nobody has complained, so I'm taking that as
assent; the series has been applied.
Thanks,
jon
^ permalink raw reply
* Re: [PATCH v4 1/2] powerpc/uaccess: Implement unsafe_put_user() using 'asm goto'
From: Segher Boessenkool @ 2020-05-05 15:32 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linux-kernel, npiggin, Paul Mackerras, linuxppc-dev
In-Reply-To: <87sggecv81.fsf@mpe.ellerman.id.au>
Hi!
On Wed, May 06, 2020 at 12:27:58AM +1000, Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
> > unsafe_put_user() is designed to take benefit of 'asm goto'.
> >
> > Instead of using the standard __put_user() approach and branch
> > based on the returned error, use 'asm goto' and make the
> > exception code branch directly to the error label. There is
> > no code anymore in the fixup section.
> >
> > This change significantly simplifies functions using
> > unsafe_put_user()
> >
> ...
> >
> > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> > ---
> > arch/powerpc/include/asm/uaccess.h | 61 +++++++++++++++++++++++++-----
> > 1 file changed, 52 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> > index 9cc9c106ae2a..9365b59495a2 100644
> > --- a/arch/powerpc/include/asm/uaccess.h
> > +++ b/arch/powerpc/include/asm/uaccess.h
> > @@ -196,6 +193,52 @@ do { \
> > })
> >
> >
> > +#define __put_user_asm_goto(x, addr, label, op) \
> > + asm volatile goto( \
> > + "1: " op "%U1%X1 %0,%1 # put_user\n" \
> > + EX_TABLE(1b, %l2) \
> > + : \
> > + : "r" (x), "m<>" (*addr) \
>
> The "m<>" here is breaking GCC 4.6.3, which we allegedly still support.
[ You shouldn't use 4.6.3, there has been 4.6.4 since a while. And 4.6
is nine years old now. Most projects do not support < 4.8 anymore, on
any architecture. ]
> Plain "m" works, how much does the "<>" affect code gen in practice?
>
> A quick diff here shows no difference from removing "<>".
It will make it impossible to use update-form instructions here. That
probably does not matter much at all, in this case.
If you remove the "<>" constraints, also remove the "%Un" output modifier?
Segher
^ permalink raw reply
* Re: [PATCH v4 1/2] powerpc/uaccess: Implement unsafe_put_user() using 'asm goto'
From: Segher Boessenkool @ 2020-05-05 15:59 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linux-kernel, npiggin, Paul Mackerras, linuxppc-dev
In-Reply-To: <1c6379b2-7e0a-91fe-34f0-51f5adca7929@csgroup.eu>
On Tue, May 05, 2020 at 05:40:21PM +0200, Christophe Leroy wrote:
> >>+#define __put_user_asm_goto(x, addr, label, op) \
> >>+ asm volatile goto( \
> >>+ "1: " op "%U1%X1 %0,%1 # put_user\n" \
> >>+ EX_TABLE(1b, %l2) \
> >>+ : \
> >>+ : "r" (x), "m<>" (*addr) \
> >
> >The "m<>" here is breaking GCC 4.6.3, which we allegedly still support.
> >
> >Plain "m" works, how much does the "<>" affect code gen in practice?
> >
> >A quick diff here shows no difference from removing "<>".
>
> It was recommended by Segher, there has been some discussion about it on
> v1 of this patch, see
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/4fdc2aba6f5e51887d1cd0fee94be0989eada2cd.1586942312.git.christophe.leroy@c-s.fr/
>
> As far as I understood that's mandatory on recent gcc to get the
> pre-update form of the instruction. With older versions "m" was doing
> the same, but not anymore.
Yes. How much that matters depends on the asm. On older CPUs (6xx/7xx,
say) the update form was just as fast as the non-update form. On newer
or bigger CPUs it is usually executed just the same as an add followed
by the memory access, so it just saves a bit of code size.
> Should we ifdef the "m<>" or "m" based on GCC
> version ?
That will be a lot of churn. Just make 4.8 minimum?
Segher
^ permalink raw reply
* Re: [PATCH] powerpc/5200: update contact email
From: Wolfram Sang @ 2020-05-05 16:04 UTC (permalink / raw)
To: Michael Ellerman
Cc: devicetree, linux-kernel, Rob Herring, Paul Mackerras, kernel,
linuxppc-dev
In-Reply-To: <877dxsdl5e.fsf@mpe.ellerman.id.au>
[-- Attachment #1: Type: text/plain, Size: 449 bytes --]
> > My 'pengutronix' address is defunct for years. Merge the entries and use
> > the proper contact address.
>
> Is there any point adding the new address? It's just likely to bit-rot
> one day too.
At least, this one is a group address, not an individual one, so less
likey.
> I figure the git history is a better source for more up-to-date emails.
But yes, can still be argued. I won't persist if you don't like it.
Thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: remove set_fs calls from the coredump code v6
From: Linus Torvalds @ 2020-05-05 16:52 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Arnd Bergmann, Jeremy Kerr, the arch/x86 maintainers,
Oleg Nesterov, Linux Kernel Mailing List, Eric W . Biederman,
linux-fsdevel, Andrew Morton, linuxppc-dev, Alexander Viro
In-Reply-To: <20200505101256.3121270-1-hch@lst.de>
On Tue, May 5, 2020 at 3:13 AM Christoph Hellwig <hch@lst.de> wrote:
>
> this series gets rid of playing with the address limit in the exec and
> coredump code. Most of this was fairly trivial, the biggest changes are
> those to the spufs coredump code.
Ack, nice, and looks good.
The only part I dislike is how we have that 'struct compat_siginfo' on
the stack, which is a huge waste (most of it is the nasty padding to
128 bytes).
But that's not new, I only reacted to it because the code moved a bit.
We cleaned up the regular siginfo to not have the padding in the
kernel (and by "we" I mean "Eric Biederman did it after some prodding
as part of his siginfo cleanups" - see commit 4ce5f9c9e754 "signal:
Use a smaller struct siginfo in the kernel"), and I wonder if we
could do something similar with that compat thing.
128 bytes of wasted kernel stack isn't the end of the world, but it's
sad when the *actual* data is only 32 bytes or so.
Linus
^ permalink raw reply
* Re: [PATCH v2 0/5] Statsfs: a new ram-based file sytem for Linux kernel statistics
From: Paolo Bonzini @ 2020-05-05 17:02 UTC (permalink / raw)
To: Jim Mattson, Emanuele Giuseppe Esposito
Cc: linux-s390, kvm list, David Hildenbrand, Cornelia Huck,
Emanuele Giuseppe Esposito, LKML, kvm-ppc, Jonathan Adams,
Christian Borntraeger, Alexander Viro, David Rientjes,
Linux FS Devel, Vitaly Kuznetsov, linux-mips, linuxppc-dev
In-Reply-To: <CALMp9eQYcLr_REzDC1kWTHX4SJWt7x+Zd1KwNvS1YGd5TVM1xA@mail.gmail.com>
On 05/05/20 18:53, Jim Mattson wrote:
>>> Since this is becoming a generic API (good!!), maybe we can discuss
>>> possible ways to optimize gathering of stats in mass?
>> Sure, the idea of a binary format was considered from the beginning in
>> [1], and it can be done either together with the current filesystem, or
>> as a replacement via different mount options.
>
> ASCII stats are not scalable. A binary format is definitely the way to go.
I am totally in favor of having a binary format, but it should be
introduced as a separate series on top of this one---and preferably by
someone who has already put some thought into the problem (which
Emanuele and I have not, beyond ensuring that the statsfs concept and
API is flexible enough).
ASCII stats are necessary for quick userspace consumption and for
backwards compatibility with KVM debugfs (which is not an ABI, but it's
damn useful and should not be dropped without providing something as
handy), so this is what this series starts from.
Paolo
^ permalink raw reply
* Re: [PATCH v2 0/5] Statsfs: a new ram-based file sytem for Linux kernel statistics
From: David Rientjes @ 2020-05-05 17:07 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Emanuele Giuseppe Esposito, linux-s390, kvm list,
David Hildenbrand, Cornelia Huck, Emanuele Giuseppe Esposito,
LKML, kvm-ppc, Jonathan Adams, Christian Borntraeger,
Alexander Viro, Linux FS Devel, Vitaly Kuznetsov, linux-mips,
linuxppc-dev, Jim Mattson
In-Reply-To: <1d12f846-bf89-7b0a-5c71-e61d83b1a36f@redhat.com>
On Tue, 5 May 2020, Paolo Bonzini wrote:
> >>> Since this is becoming a generic API (good!!), maybe we can discuss
> >>> possible ways to optimize gathering of stats in mass?
> >> Sure, the idea of a binary format was considered from the beginning in
> >> [1], and it can be done either together with the current filesystem, or
> >> as a replacement via different mount options.
> >
> > ASCII stats are not scalable. A binary format is definitely the way to go.
>
> I am totally in favor of having a binary format, but it should be
> introduced as a separate series on top of this one---and preferably by
> someone who has already put some thought into the problem (which
> Emanuele and I have not, beyond ensuring that the statsfs concept and
> API is flexible enough).
>
The concern is that once this series is merged then /sys/kernel/stats
could be considered an ABI and there would be a reasonable expectation
that it will remain stable, in so far as the stats that userspace is
interested in are stable and not obsoleted.
So is this a suggestion that the binary format becomes complementary to
statsfs and provide a means for getting all stats from a single subsystem,
or that this series gets converted to such a format before it is merged?
> ASCII stats are necessary for quick userspace consumption and for
> backwards compatibility with KVM debugfs (which is not an ABI, but it's
> damn useful and should not be dropped without providing something as
> handy), so this is what this series starts from.
>
^ permalink raw reply
* Re: [PATCH v2 0/5] Statsfs: a new ram-based file sytem for Linux kernel statistics
From: Paolo Bonzini @ 2020-05-05 17:21 UTC (permalink / raw)
To: David Rientjes
Cc: Emanuele Giuseppe Esposito, linux-s390, kvm list,
David Hildenbrand, Cornelia Huck, Emanuele Giuseppe Esposito,
LKML, kvm-ppc, Jonathan Adams, Christian Borntraeger,
Alexander Viro, Linux FS Devel, Vitaly Kuznetsov, linux-mips,
linuxppc-dev, Jim Mattson
In-Reply-To: <alpine.DEB.2.22.394.2005051003380.216575@chino.kir.corp.google.com>
On 05/05/20 19:07, David Rientjes wrote:
>> I am totally in favor of having a binary format, but it should be
>> introduced as a separate series on top of this one---and preferably by
>> someone who has already put some thought into the problem (which
>> Emanuele and I have not, beyond ensuring that the statsfs concept and
>> API is flexible enough).
>>
> The concern is that once this series is merged then /sys/kernel/stats
> could be considered an ABI and there would be a reasonable expectation
> that it will remain stable, in so far as the stats that userspace is
> interested in are stable and not obsoleted.
>
> So is this a suggestion that the binary format becomes complementary to
> statsfs and provide a means for getting all stats from a single subsystem,
> or that this series gets converted to such a format before it is merged?
The binary format should be complementary. The ASCII format should
indeed be considered stable even though individual statistics would come
and go. It may make sense to allow disabling ASCII files via mount
and/or Kconfig options; but either way, the binary format can and should
be added on top.
I have not put any thought into what the binary format would look like
and what its features would be. For example these are but the first
questions that come to mind:
* would it be possible to read/clear an arbitrary statistic with
pread/pwrite, or do you have to read all of them?
* if userspace wants to read the schema just once and then read the
statistics many times, how is it informed of schema changes?
* and of course the details of how the schema (names of stat and
subsources) is encoded and what details it should include about the
values (e.g. type or just signedness).
Another possibility is to query stats via BPF. This could be a third
way to access the stats, or it could be alternative to a binary format.
Paolo
^ permalink raw reply
* Re: [PATCH v2 0/5] Statsfs: a new ram-based file sytem for Linux kernel statistics
From: Christian Borntraeger @ 2020-05-05 17:30 UTC (permalink / raw)
To: Paolo Bonzini, David Rientjes
Cc: Emanuele Giuseppe Esposito, linux-s390, kvm list,
David Hildenbrand, Cornelia Huck, Emanuele Giuseppe Esposito,
LKML, kvm-ppc, Jonathan Adams, Alexander Viro, Stefan Raspl,
Linux FS Devel, Vitaly Kuznetsov, linux-mips, linuxppc-dev,
Jim Mattson
In-Reply-To: <6cfdf81f-caef-2489-0906-25915d9d58ff@redhat.com>
Adding Stefan Raspl, who has done a lot of kvm_stat work in the past.
On 05.05.20 19:21, Paolo Bonzini wrote:
> On 05/05/20 19:07, David Rientjes wrote:
>>> I am totally in favor of having a binary format, but it should be
>>> introduced as a separate series on top of this one---and preferably by
>>> someone who has already put some thought into the problem (which
>>> Emanuele and I have not, beyond ensuring that the statsfs concept and
>>> API is flexible enough).
>>>
>> The concern is that once this series is merged then /sys/kernel/stats
>> could be considered an ABI and there would be a reasonable expectation
>> that it will remain stable, in so far as the stats that userspace is
>> interested in are stable and not obsoleted.
>>
>> So is this a suggestion that the binary format becomes complementary to
>> statsfs and provide a means for getting all stats from a single subsystem,
>> or that this series gets converted to such a format before it is merged?
>
> The binary format should be complementary. The ASCII format should
> indeed be considered stable even though individual statistics would come
> and go. It may make sense to allow disabling ASCII files via mount
> and/or Kconfig options; but either way, the binary format can and should
> be added on top.
>
> I have not put any thought into what the binary format would look like
> and what its features would be. For example these are but the first
> questions that come to mind:
>
> * would it be possible to read/clear an arbitrary statistic with
> pread/pwrite, or do you have to read all of them?
>
> * if userspace wants to read the schema just once and then read the
> statistics many times, how is it informed of schema changes?
>
> * and of course the details of how the schema (names of stat and
> subsources) is encoded and what details it should include about the
> values (e.g. type or just signedness).
>
> Another possibility is to query stats via BPF. This could be a third
> way to access the stats, or it could be alternative to a binary format.
>
> Paolo
>
^ permalink raw reply
* Re: [PATCH v2 17/20] mm: free_area_init: allow defining max_zone_pfn in descending order
From: Vineet Gupta @ 2020-05-05 17:27 UTC (permalink / raw)
To: Guenter Roeck, Mike Rapoport
Cc: Rich Felker, linux-ia64@vger.kernel.org,
linux-doc@vger.kernel.org, Catalin Marinas, Heiko Carstens,
x86@kernel.org, Michal Hocko, James E.J. Bottomley, Max Filippov,
Guo Ren, Ley Foon Tan, sparclinux@vger.kernel.org,
linux-riscv@lists.infradead.org, Greg Ungerer,
linux-arch@vger.kernel.org, linux-s390@vger.kernel.org,
linux-c6x-dev@linux-c6x.org, Baoquan He, Jonathan Corbet,
linux-hexagon@vger.kernel.org, Helge Deller,
linux-sh@vger.kernel.org, Russell King,
linux-csky@vger.kernel.org, Mike Rapoport, Geert Uytterhoeven,
Hoan Tran, Mark Salter, Matt Turner,
linux-snps-arc@lists.infradead.org,
uclinux-h8-devel@lists.sourceforge.jp,
linux-xtensa@linux-xtensa.org, Nick Hu,
linux-alpha@vger.kernel.org, linux-um@lists.infradead.org,
linux-mips@vger.kernel.org, Richard Weinberger,
linux-m68k@lists.linux-m68k.org, Thomas Bogendoerfer, Qian Cai,
Greentime Hu, Paul Walmsley, Stafford Horne, Guan Xuetao,
linux-arm-kernel@lists.infradead.org, Michal Simek, Tony Luck,
Yoshinori Sato, linux-parisc@vger.kernel.org, linux-mm@kvack.org,
Brian Cain, linux-kernel@vger.kernel.org,
openrisc@lists.librecores.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org, David S. Miller
In-Reply-To: <ca099c3e-c0bc-cd2f-cdb0-852dfc2c10db@roeck-us.net>
On 5/5/20 6:18 AM, Guenter Roeck wrote:
> On 5/4/20 8:39 AM, Mike Rapoport wrote:
>> On Sun, May 03, 2020 at 11:43:00AM -0700, Guenter Roeck wrote:
>>> On Sun, May 03, 2020 at 10:41:38AM -0700, Guenter Roeck wrote:
>>>> Hi,
>>>>
>>>> On Wed, Apr 29, 2020 at 03:11:23PM +0300, Mike Rapoport wrote:
>>>>> From: Mike Rapoport <rppt@linux.ibm.com>
>>>>>
>>>>> Some architectures (e.g. ARC) have the ZONE_HIGHMEM zone below the
>>>>> ZONE_NORMAL. Allowing free_area_init() parse max_zone_pfn array even it is
>>>>> sorted in descending order allows using free_area_init() on such
>>>>> architectures.
>>>>>
>>>>> Add top -> down traversal of max_zone_pfn array in free_area_init() and use
>>>>> the latter in ARC node/zone initialization.
>>>>>
>>>>> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
>>>> This patch causes my microblazeel qemu boot test in linux-next to fail.
>>>> Reverting it fixes the problem.
>>>>
>>> The same problem is seen with s390 emulations.
>> Yeah, this patch breaks some others as well :(
>>
>> My assumption that max_zone_pfn defines architectural limit for maximal
>> PFN that can belong to a zone was over-optimistic. Several arches
>> actually do that, but others do
>>
>> max_zone_pfn[ZONE_DMA] = MAX_DMA_PFN;
>> max_zone_pfn[ZONE_NORMAL] = max_pfn;
>>
>> where MAX_DMA_PFN is build-time constrain and max_pfn is run time limit
>> for the current system.
>>
>> So, when max_pfn is lower than MAX_DMA_PFN, the free_init_area() will
>> consider max_zone_pfn as descending and will wrongly calculate zone
>> extents.
>>
>> That said, instead of trying to create a generic way to special case
>> ARC, I suggest to simply use the below patch instead.
>>
> As a reminder, I reported the problem against s390 and microblazeel
> (interestingly enough, microblaze (big endian) works), not against arc.
Understood and my comment was to point to any other problems in future.
Thx,
-Vineet
^ permalink raw reply
* Re: [PATCH 3/3] mm/hugetlb: Introduce HAVE_ARCH_CLEAR_HUGEPAGE_FLAGS
From: Andrew Morton @ 2020-05-05 20:12 UTC (permalink / raw)
To: Anshuman Khandual
Cc: Rich Felker, linux-ia64, linux-sh, Catalin Marinas,
Heiko Carstens, linux-kernel, James E.J. Bottomley, linux-mm,
Paul Mackerras, H. Peter Anvin, sparclinux, linux-riscv,
Will Deacon, linux-arch, linux-s390, Yoshinori Sato, Helge Deller,
x86, Russell King, Christian Borntraeger, Ingo Molnar, Fenghua Yu,
Vasily Gorbik, Thomas Bogendoerfer, Borislav Petkov,
Paul Walmsley, Thomas Gleixner, linux-arm-kernel, Tony Luck,
linux-parisc, linux-mips, Palmer Dabbelt, linuxppc-dev,
David S. Miller, Mike Kravetz
In-Reply-To: <21460cbc-8e9a-b956-5797-57b2e1df9fb1@arm.com>
On Tue, 5 May 2020 08:21:34 +0530 Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> >>> static inline void arch_clear_hugepage_flags(struct page *page)
> >>> {
> >>> <some implementation>
> >>> }
> >>> #define arch_clear_hugepage_flags arch_clear_hugepage_flags
> >>>
> >>> It's a small difference - mainly to avoid adding two variables to the
> >>> overall namespace where one would do.
> >>
> >> Understood, will change and resend.
> >
> > That's OK - I've queued up that fix.
> >
>
> Hello Andrew,
>
> I might not have searched all the relevant trees or might have just searched
> earlier than required. But I dont see these patches (or your proposed fixes)
> either in mmotm (2020-04-29-23-04) or in next-20200504. Wondering if you are
> waiting on a V2 for this series accommodating the changes you had proposed.
hm. I think I must have got confused and thought you were referring to
a different patch. Yes please, let's have v2.
^ permalink raw reply
* Re: remove set_fs calls from the coredump code v6
From: Eric W. Biederman @ 2020-05-05 20:28 UTC (permalink / raw)
To: Linus Torvalds
Cc: Arnd Bergmann, the arch/x86 maintainers, Oleg Nesterov,
Linux Kernel Mailing List, Jeremy Kerr, linux-fsdevel,
Andrew Morton, linuxppc-dev, Christoph Hellwig, Alexander Viro
In-Reply-To: <CAHk-=wgrHhaM1XCB=E3Zp2Br8E5c_kmVUTd5y06xh5sev5nRMA@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Tue, May 5, 2020 at 3:13 AM Christoph Hellwig <hch@lst.de> wrote:
>>
>> this series gets rid of playing with the address limit in the exec and
>> coredump code. Most of this was fairly trivial, the biggest changes are
>> those to the spufs coredump code.
>
> Ack, nice, and looks good.
>
> The only part I dislike is how we have that 'struct compat_siginfo' on
> the stack, which is a huge waste (most of it is the nasty padding to
> 128 bytes).
>
> But that's not new, I only reacted to it because the code moved a bit.
> We cleaned up the regular siginfo to not have the padding in the
> kernel (and by "we" I mean "Eric Biederman did it after some prodding
> as part of his siginfo cleanups" - see commit 4ce5f9c9e754 "signal:
> Use a smaller struct siginfo in the kernel"), and I wonder if we
> could do something similar with that compat thing.
>
> 128 bytes of wasted kernel stack isn't the end of the world, but it's
> sad when the *actual* data is only 32 bytes or so.
We probably can. After introducing a kernel_compat_siginfo that is
the size that userspace actually would need.
It isn't something I want to mess with until this code gets merged, as I
think the set_fs cleanups are more important.
Christoph made some good points about how ugly the #ifdefs are in
the generic copy_siginfo_to_user32 implementation.
I am thinking the right fix is to introduce.
- TS_X32 as a companion to TS_COMPAT in the x86_64.
- Modify in_x32_syscall() to test TS_X32
- Implement x32_copy_siginfo_to_user32 that forces TS_X32 to be
set. AKA:
x32_copy_siginfo_to_user32()
{
unsigned long state = current_thread_info()->state;
current_thread_info()->state |= TS_X32;
copy_siginfo_to_user32();
current_thread_info()->state = state;
}
That would make the #ifdefs go away, but I don't yet know what the x86
maintainers would say about that scheme. I think it is a good path as
it would isolate the runtime cost of that weird SIGCHLD siginfo format
to just x32. Then ia32 in compat mode would not need to pay.
Once I get that then it will be easier to introduce a yet another helper
of copy_siginfo_to_user32 that generates just the kernel_compat_siginfo
part, and the two visible derivatives can call memset and clear_user
to clear the unset parts.
I am assuming you don't don't mind having a full siginfo in
elf_note_info that ultimately gets copied into the core dump?
Eric
^ permalink raw reply
* Re: remove set_fs calls from the coredump code v6
From: Al Viro @ 2020-05-05 20:34 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Arnd Bergmann, linuxppc-dev, x86, Oleg Nesterov, linux-kernel,
Eric W . Biederman, linux-fsdevel, Andrew Morton, Linus Torvalds,
Jeremy Kerr
In-Reply-To: <20200505101256.3121270-1-hch@lst.de>
On Tue, May 05, 2020 at 12:12:49PM +0200, Christoph Hellwig wrote:
> Hi all,
>
> this series gets rid of playing with the address limit in the exec and
> coredump code. Most of this was fairly trivial, the biggest changes are
> those to the spufs coredump code.
>
> Changes since v5:
> - fix uaccess under spinlock in spufs (Jeremy)
> - remove use of access_ok in spufs
>
> Changes since v4:
> - change some goto names as suggested by Linus
>
> Changes since v3:
> - fix x86 compilation with x32 in the new version of the signal code
> - split the exec patches into a new series
>
> Changes since v2:
> - don't cleanup the compat siginfo calling conventions, use the patch
> variant from Eric with slight coding style fixes instead.
>
> Changes since v1:
> - properly spell NUL
> - properly handle the compat siginfo case in ELF coredumps
Looks good. Want me to put it into vfs.git? #work.set_fs-exec, perhaps?
^ permalink raw reply
* Re: remove set_fs calls from the coredump code v6
From: Christoph Hellwig @ 2020-05-05 20:42 UTC (permalink / raw)
To: Al Viro
Cc: Arnd Bergmann, linuxppc-dev, x86, Oleg Nesterov, linux-kernel,
Jeremy Kerr, linux-fsdevel, Andrew Morton, Linus Torvalds,
Christoph Hellwig, Eric W . Biederman
In-Reply-To: <20200505203446.GZ23230@ZenIV.linux.org.uk>
On Tue, May 05, 2020 at 09:34:46PM +0100, Al Viro wrote:
> Looks good. Want me to put it into vfs.git? #work.set_fs-exec, perhaps?
Sounds good.
^ permalink raw reply
* Re: remove set_fs calls from the coredump code v6
From: Al Viro @ 2020-05-05 20:47 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Arnd Bergmann, linuxppc-dev, x86, Oleg Nesterov, linux-kernel,
Eric W . Biederman, linux-fsdevel, Andrew Morton, Linus Torvalds,
Jeremy Kerr
In-Reply-To: <20200505204258.GA24277@lst.de>
On Tue, May 05, 2020 at 10:42:58PM +0200, Christoph Hellwig wrote:
> On Tue, May 05, 2020 at 09:34:46PM +0100, Al Viro wrote:
> > Looks good. Want me to put it into vfs.git? #work.set_fs-exec, perhaps?
>
> Sounds good.
Applied, pushed and added into #for-next
^ permalink raw reply
* Re: New powerpc vdso calling convention
From: Segher Boessenkool @ 2020-05-05 21:56 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Rich Felker, libc-alpha, Adhemerval Zanella, musl, binutils,
Andy Lutomirski, libc-dev, Thomas Gleixner, Vincenzo Frascino,
linuxppc-dev
In-Reply-To: <1588126678.zjwj4d1d90.astroid@bobo.none>
Hi!
On Wed, Apr 29, 2020 at 12:39:22PM +1000, Nicholas Piggin wrote:
> Excerpts from Adhemerval Zanella's message of April 27, 2020 11:09 pm:
> >> Right, I'm just talking about those comments -- it seems like the kernel
> >> vdso should contain an .opd section with function descriptors in it for
> >> elfv1 calls, rather than the hack it has now of creating one in the
> >> caller's .data section.
> >>
> >> But all that function descriptor code is gated by
> >>
> >> #if (defined(__PPC64__) || defined(__powerpc64__)) && _CALL_ELF != 2
> >>
> >> So it seems PPC32 does not use function descriptors but a direct pointer
> >> to the entry point like PPC64 with ELFv2.
> >
> > Yes, this hack is only for ELFv1. The missing ODP has not been an issue
> > or glibc because it has been using the inline assembly to emulate the
> > functions call since initial vDSO support (INTERNAL_VSYSCALL_CALL_TYPE).
> > It just has become an issue when I added a ifunc optimization to
> > gettimeofday so it can bypass the libc.so and make plt branch to vDSO
> > directly.
>
> I can't understand if it's actually a problem for you or not.
>
> Regardless if you can hack around it, it seems to me that if we're going
> to add sane calling conventions to the vdso, then we should also just
> have a .opd section for it as well, whether or not a particular libc
> requires it.
An OPD ("official procedure descriptor") is required for every function,
to have proper C semantics, so that pointers to functions (which are
pointers to descriptors, in fact) are unique. You can "manually" make
descriptors just fine, and use those to call functions -- but you cannot
(in general) use a pointer to such a "fake" descriptor as the "id" of
the function.
The way the ABIs define the OPDs makes them guaranteed unique.
Segher
^ permalink raw reply
* Re: [RFC PATCH 2/2] powerpc/64s: system call support for scv/rfscv instructions
From: Segher Boessenkool @ 2020-05-05 22:11 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev
In-Reply-To: <20200430040202.1735506-3-npiggin@gmail.com>
Hi!
On Thu, Apr 30, 2020 at 02:02:02PM +1000, Nicholas Piggin wrote:
> Add support for the scv instruction on POWER9 and later CPUs.
Looks good to me in general :-)
> For now this implements the zeroth scv vector 'scv 0', as identical
> to 'sc' system calls, with the exception that lr is not preserved, and
> it is 64-bit only. There may yet be changes made to this ABI, so it's
> for testing only.
What does it do with SF=0? I don't see how it is obviously not a
security hole currently (but I didn't look too closely).
Segher
^ permalink raw reply
* Re: [PATCH v4 7/8] powerpc/vdso32: implement clock_getres entirely
From: Aurelien Jarno @ 2020-05-05 22:52 UTC (permalink / raw)
To: Christophe Leroy; +Cc: arnd, linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <37f94e47c91070b7606fb3ec3fe6fd2302a475a0.1575273217.git.christophe.leroy@c-s.fr>
Hi,
On 2019-12-02 07:57, Christophe Leroy wrote:
> clock_getres returns hrtimer_res for all clocks but coarse ones
> for which it returns KTIME_LOW_RES.
>
> return EINVAL for unknown clocks.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/kernel/asm-offsets.c | 3 +++
> arch/powerpc/kernel/vdso32/gettimeofday.S | 19 +++++++++++--------
> 2 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> index 0013197d89a6..90e53d432f2e 100644
> --- a/arch/powerpc/kernel/asm-offsets.c
> +++ b/arch/powerpc/kernel/asm-offsets.c
> @@ -413,7 +413,10 @@ int main(void)
> DEFINE(CLOCK_MONOTONIC, CLOCK_MONOTONIC);
> DEFINE(CLOCK_REALTIME_COARSE, CLOCK_REALTIME_COARSE);
> DEFINE(CLOCK_MONOTONIC_COARSE, CLOCK_MONOTONIC_COARSE);
> + DEFINE(CLOCK_MAX, CLOCK_TAI);
> DEFINE(NSEC_PER_SEC, NSEC_PER_SEC);
> + DEFINE(EINVAL, EINVAL);
> + DEFINE(KTIME_LOW_RES, KTIME_LOW_RES);
>
> #ifdef CONFIG_BUG
> DEFINE(BUG_ENTRY_SIZE, sizeof(struct bug_entry));
> diff --git a/arch/powerpc/kernel/vdso32/gettimeofday.S b/arch/powerpc/kernel/vdso32/gettimeofday.S
> index 9aafacea9c4a..20ae38f3a5a3 100644
> --- a/arch/powerpc/kernel/vdso32/gettimeofday.S
> +++ b/arch/powerpc/kernel/vdso32/gettimeofday.S
> @@ -196,17 +196,20 @@ V_FUNCTION_END(__kernel_clock_gettime)
> V_FUNCTION_BEGIN(__kernel_clock_getres)
> .cfi_startproc
> /* Check for supported clock IDs */
> - cmpwi cr0,r3,CLOCK_REALTIME
> - cmpwi cr1,r3,CLOCK_MONOTONIC
> - cror cr0*4+eq,cr0*4+eq,cr1*4+eq
> - bne cr0,99f
> + cmplwi cr0, r3, CLOCK_MAX
> + cmpwi cr1, r3, CLOCK_REALTIME_COARSE
> + cmpwi cr7, r3, CLOCK_MONOTONIC_COARSE
> + bgt cr0, 99f
> + LOAD_REG_IMMEDIATE(r5, KTIME_LOW_RES)
> + beq cr1, 1f
> + beq cr7, 1f
>
> mflr r12
> .cfi_register lr,r12
> get_datapage r3, r0
> lwz r5, CLOCK_HRTIMER_RES(r3)
> mtlr r12
> - li r3,0
> +1: li r3,0
> cmpli cr0,r4,0
> crclr cr0*4+so
> beqlr
> @@ -215,11 +218,11 @@ V_FUNCTION_BEGIN(__kernel_clock_getres)
> blr
>
> /*
> - * syscall fallback
> + * invalid clock
> */
> 99:
> - li r0,__NR_clock_getres
> - sc
> + li r3, EINVAL
> + crset so
> blr
> .cfi_endproc
> V_FUNCTION_END(__kernel_clock_getres)
Removing the syscall fallback looks wrong, and broke access to
per-processes clocks. With this change a few glibc tests now fail.
This can be reproduced by the simple code below:
| #include <errno.h>
| #include <stdio.h>
| #include <string.h>
| #include <sys/types.h>
| #include <time.h>
| #include <unistd.h>
|
| int main()
| {
| struct timespec res;
| clockid_t ci;
| int e;
|
| e = clock_getcpuclockid(getpid(), &ci);
| if (e) {
| printf("clock_getcpuclockid returned %d\n", e);
| return e;
| }
| e = clock_getres (ci, &res);
| printf("clock_getres returned %d\n", e);
| if (e) {
| printf(" errno: %d, %s\n", errno, strerror(errno));
| }
|
| return e;
| }
Without this patch or with -m64, it returns:
| clock_getres returned 0
With this patch with -m32 it returns:
| clock_getres returned -1
| errno: 22, Invalid argument
Regards,
Aurelien
--
Aurelien Jarno GPG: 4096R/1DDD8C9B
aurelien@aurel32.net http://www.aurel32.net
^ permalink raw reply
* Re: [PATCH] powerpc/xive: Enforce load-after-store ordering when StoreEOI is active
From: Alistair Popple @ 2020-05-05 23:34 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: linuxppc-dev, Paul Mackerras, Greg Kurz
In-Reply-To: <20200220081506.31209-1-clg@kaod.org>
I am still slowly wrapping my head around XIVE and it's interaction with KVM
but from what I can see this looks good and is needed so we can enable
StoreEOI support in future so:
Reviewed-by: Alistair Popple <alistair@popple.id.au>
On Thursday, 20 February 2020 7:15:06 PM AEST Cédric Le Goater wrote:
> When an interrupt has been handled, the OS notifies the interrupt
> controller with a EOI sequence. On a POWER9 system using the XIVE
> interrupt controller, this can be done with a load or a store
> operation on the ESB interrupt management page of the interrupt. The
> StoreEOI operation has less latency and improves interrupt handling
> performance but it was deactivated during the POWER9 DD2.0 timeframe
> because of ordering issues. We use the LoadEOI today but we plan to
> reactivate StoreEOI in future architectures.
>
> There is usually no need to enforce ordering between ESB load and
> store operations as they should lead to the same result. E.g. a store
> trigger and a load EOI can be executed in any order. Assuming the
> interrupt state is PQ=10, a store trigger followed by a load EOI will
> return a Q bit. In the reverse order, it will create a new interrupt
> trigger from HW. In both cases, the handler processing interrupts is
> notified.
>
> In some cases, the XIVE_ESB_SET_PQ_10 load operation is used to
> disable temporarily the interrupt source (mask/unmask). When the
> source is reenabled, the OS can detect if interrupts were received
> while the source was disabled and reinject them. This process needs
> special care when StoreEOI is activated. The ESB load and store
> operations should be correctly ordered because a XIVE_ESB_STORE_EOI
> operation could leave the source enabled if it has not completed
> before the loads.
>
> For those cases, we enforce Load-after-Store ordering with a special
> load operation offset. To avoid performance impact, this ordering is
> only enforced when really needed, that is when interrupt sources are
> temporarily disabled with the XIVE_ESB_SET_PQ_10 load. It should not
> be needed for other loads.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> arch/powerpc/include/asm/xive-regs.h | 8 ++++++++
> arch/powerpc/kvm/book3s_xive_native.c | 6 ++++++
> arch/powerpc/kvm/book3s_xive_template.c | 3 +++
> arch/powerpc/sysdev/xive/common.c | 3 +++
> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 5 +++++
> 5 files changed, 25 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/xive-regs.h
> b/arch/powerpc/include/asm/xive-regs.h index f2dfcd50a2d3..b1996fbae59a
> 100644
> --- a/arch/powerpc/include/asm/xive-regs.h
> +++ b/arch/powerpc/include/asm/xive-regs.h
> @@ -37,6 +37,14 @@
> #define XIVE_ESB_SET_PQ_10 0xe00 /* Load */
> #define XIVE_ESB_SET_PQ_11 0xf00 /* Load */
>
> +/*
> + * Load-after-store ordering
> + *
> + * Adding this offset to the load address will enforce
> + * load-after-store ordering. This is required to use StoreEOI.
> + */
> +#define XIVE_ESB_LD_ST_MO 0x40 /* Load-after-store ordering */
> +
> #define XIVE_ESB_VAL_P 0x2
> #define XIVE_ESB_VAL_Q 0x1
>
> diff --git a/arch/powerpc/kvm/book3s_xive_native.c
> b/arch/powerpc/kvm/book3s_xive_native.c index d83adb1e1490..c80b6a447efd
> 100644
> --- a/arch/powerpc/kvm/book3s_xive_native.c
> +++ b/arch/powerpc/kvm/book3s_xive_native.c
> @@ -31,6 +31,12 @@ static u8 xive_vm_esb_load(struct xive_irq_data *xd, u32
> offset) {
> u64 val;
>
> + /*
> + * The KVM XIVE native device does not use the XIVE_ESB_SET_PQ_10
> + * load operation, so there is no need to enforce load-after-store
> + * ordering.
> + */
> +
> if (xd->flags & XIVE_IRQ_FLAG_SHIFT_BUG)
> offset |= offset << 4;
>
> diff --git a/arch/powerpc/kvm/book3s_xive_template.c
> b/arch/powerpc/kvm/book3s_xive_template.c index a8a900ace1e6..4ad3c0279458
> 100644
> --- a/arch/powerpc/kvm/book3s_xive_template.c
> +++ b/arch/powerpc/kvm/book3s_xive_template.c
> @@ -58,6 +58,9 @@ static u8 GLUE(X_PFX,esb_load)(struct xive_irq_data *xd,
> u32 offset) {
> u64 val;
>
> + if (offset == XIVE_ESB_SET_PQ_10 && xd->flags & XIVE_IRQ_FLAG_STORE_EOI)
> + offset |= XIVE_ESB_LD_ST_MO;
> +
> if (xd->flags & XIVE_IRQ_FLAG_SHIFT_BUG)
> offset |= offset << 4;
>
> diff --git a/arch/powerpc/sysdev/xive/common.c
> b/arch/powerpc/sysdev/xive/common.c index f5fadbd2533a..0dc421bb494f 100644
> --- a/arch/powerpc/sysdev/xive/common.c
> +++ b/arch/powerpc/sysdev/xive/common.c
> @@ -202,6 +202,9 @@ static notrace u8 xive_esb_read(struct xive_irq_data
> *xd, u32 offset) {
> u64 val;
>
> + if (offset == XIVE_ESB_SET_PQ_10 && xd->flags & XIVE_IRQ_FLAG_STORE_EOI)
> + offset |= XIVE_ESB_LD_ST_MO;
> +
> /* Handle HW errata */
> if (xd->flags & XIVE_IRQ_FLAG_SHIFT_BUG)
> offset |= offset << 4;
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> b/arch/powerpc/kvm/book3s_hv_rmhandlers.S index e11017897eb0..abe132ff2346
> 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -2911,6 +2911,11 @@ kvm_cede_exit:
> beq 4f
> li r0, 0
> stb r0, VCPU_CEDED(r9)
> + /*
> + * The escalation interrupts are special as we don't EOI them.
> + * There is no need to use the load-after-store ordering offset
> + * to set PQ to 10 as we won't use StoreEOI.
> + */
> li r6, XIVE_ESB_SET_PQ_10
> b 5f
> 4: li r0, 1
^ permalink raw reply
* Re: [PATCH v4 1/2] powerpc/uaccess: Implement unsafe_put_user() using 'asm goto'
From: Christophe Leroy @ 2020-05-05 15:40 UTC (permalink / raw)
To: Michael Ellerman, Christophe Leroy, Benjamin Herrenschmidt,
Paul Mackerras, npiggin, segher
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <87sggecv81.fsf@mpe.ellerman.id.au>
Hi,
Le 05/05/2020 à 16:27, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>> unsafe_put_user() is designed to take benefit of 'asm goto'.
>>
>> Instead of using the standard __put_user() approach and branch
>> based on the returned error, use 'asm goto' and make the
>> exception code branch directly to the error label. There is
>> no code anymore in the fixup section.
>>
>> This change significantly simplifies functions using
>> unsafe_put_user()
>>
> ...
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>> arch/powerpc/include/asm/uaccess.h | 61 +++++++++++++++++++++++++-----
>> 1 file changed, 52 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
>> index 9cc9c106ae2a..9365b59495a2 100644
>> --- a/arch/powerpc/include/asm/uaccess.h
>> +++ b/arch/powerpc/include/asm/uaccess.h
>> @@ -196,6 +193,52 @@ do { \
>> })
>>
>>
>> +#define __put_user_asm_goto(x, addr, label, op) \
>> + asm volatile goto( \
>> + "1: " op "%U1%X1 %0,%1 # put_user\n" \
>> + EX_TABLE(1b, %l2) \
>> + : \
>> + : "r" (x), "m<>" (*addr) \
>
> The "m<>" here is breaking GCC 4.6.3, which we allegedly still support.
>
> Plain "m" works, how much does the "<>" affect code gen in practice?
>
> A quick diff here shows no difference from removing "<>".
It was recommended by Segher, there has been some discussion about it on
v1 of this patch, see
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/4fdc2aba6f5e51887d1cd0fee94be0989eada2cd.1586942312.git.christophe.leroy@c-s.fr/
As far as I understood that's mandatory on recent gcc to get the
pre-update form of the instruction. With older versions "m" was doing
the same, but not anymore. Should we ifdef the "m<>" or "m" based on GCC
version ?
Christophe
^ permalink raw reply
* Re: [PATCH v2 0/5] Statsfs: a new ram-based file sytem for Linux kernel statistics
From: Jim Mattson @ 2020-05-05 16:53 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: linux-s390, kvm list, David Hildenbrand, Cornelia Huck,
Emanuele Giuseppe Esposito, LKML, kvm-ppc, Jonathan Adams,
Christian Borntraeger, Alexander Viro, David Rientjes,
Linux FS Devel, Paolo Bonzini, Vitaly Kuznetsov, linux-mips,
linuxppc-dev
In-Reply-To: <f2654143-b8e5-5a1f-8bd0-0cb0df2cd638@redhat.com>
On Tue, May 5, 2020 at 2:18 AM Emanuele Giuseppe Esposito
<eesposit@redhat.com> wrote:
>
>
>
> On 5/4/20 11:37 PM, David Rientjes wrote:
> > Since this is becoming a generic API (good!!), maybe we can discuss
> > possible ways to optimize gathering of stats in mass?
>
> Sure, the idea of a binary format was considered from the beginning in
> [1], and it can be done either together with the current filesystem, or
> as a replacement via different mount options.
ASCII stats are not scalable. A binary format is definitely the way to go.
^ permalink raw reply
* Re: [PATCH v4 1/2] powerpc/uaccess: Implement unsafe_put_user() using 'asm goto'
From: Michael Ellerman @ 2020-05-06 0:58 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linux-kernel, npiggin, Paul Mackerras, linuxppc-dev
In-Reply-To: <20200505153245.GN31009@gate.crashing.org>
Segher Boessenkool <segher@kernel.crashing.org> writes:
> Hi!
>
> On Wed, May 06, 2020 at 12:27:58AM +1000, Michael Ellerman wrote:
>> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>> > unsafe_put_user() is designed to take benefit of 'asm goto'.
>> >
>> > Instead of using the standard __put_user() approach and branch
>> > based on the returned error, use 'asm goto' and make the
>> > exception code branch directly to the error label. There is
>> > no code anymore in the fixup section.
>> >
>> > This change significantly simplifies functions using
>> > unsafe_put_user()
>> >
>> ...
>> >
>> > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> > ---
>> > arch/powerpc/include/asm/uaccess.h | 61 +++++++++++++++++++++++++-----
>> > 1 file changed, 52 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
>> > index 9cc9c106ae2a..9365b59495a2 100644
>> > --- a/arch/powerpc/include/asm/uaccess.h
>> > +++ b/arch/powerpc/include/asm/uaccess.h
>> > @@ -196,6 +193,52 @@ do { \
>> > })
>> >
>> >
>> > +#define __put_user_asm_goto(x, addr, label, op) \
>> > + asm volatile goto( \
>> > + "1: " op "%U1%X1 %0,%1 # put_user\n" \
>> > + EX_TABLE(1b, %l2) \
>> > + : \
>> > + : "r" (x), "m<>" (*addr) \
>>
>> The "m<>" here is breaking GCC 4.6.3, which we allegedly still support.
>
> [ You shouldn't use 4.6.3, there has been 4.6.4 since a while. And 4.6
> is nine years old now. Most projects do not support < 4.8 anymore, on
> any architecture. ]
Moving up to 4.6.4 wouldn't actually help with this though would it?
Also I have 4.6.3 compilers already built, I don't really have time to
rebuild them for 4.6.4.
The kernel has a top-level minimum version, which I'm not in charge of, see:
https://www.kernel.org/doc/html/latest/process/changes.html?highlight=gcc
There were discussions about making 4.8 the minimum, but I'm not sure
where they got to.
>> Plain "m" works, how much does the "<>" affect code gen in practice?
>>
>> A quick diff here shows no difference from removing "<>".
>
> It will make it impossible to use update-form instructions here. That
> probably does not matter much at all, in this case.
>
> If you remove the "<>" constraints, also remove the "%Un" output modifier?
So like this?
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 62cc8d7640ec..ca847aed8e45 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -207,10 +207,10 @@ do { \
#define __put_user_asm_goto(x, addr, label, op) \
asm volatile goto( \
- "1: " op "%U1%X1 %0,%1 # put_user\n" \
+ "1: " op "%X1 %0,%1 # put_user\n" \
EX_TABLE(1b, %l2) \
: \
- : "r" (x), "m<>" (*addr) \
+ : "r" (x), "m" (*addr) \
: \
: label)
cheers
^ permalink raw reply related
* Re: [RFC PATCH 2/2] powerpc/64s: system call support for scv/rfscv instructions
From: Nicholas Piggin @ 2020-05-06 1:11 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev
In-Reply-To: <20200505221119.GQ31009@gate.crashing.org>
Excerpts from Segher Boessenkool's message of May 6, 2020 8:11 am:
> Hi!
>
> On Thu, Apr 30, 2020 at 02:02:02PM +1000, Nicholas Piggin wrote:
>> Add support for the scv instruction on POWER9 and later CPUs.
>
> Looks good to me in general :-)
Thanks for taking a look.
>> For now this implements the zeroth scv vector 'scv 0', as identical
>> to 'sc' system calls, with the exception that lr is not preserved, and
>> it is 64-bit only. There may yet be changes made to this ABI, so it's
>> for testing only.
>
> What does it do with SF=0? I don't see how it is obviously not a
> security hole currently (but I didn't look too closely).
Oh that's an outdated comment, I since decided better to keep all the code
common and handle 32-bit compat the same way as existing sc syscall.
Thanks,
Nick
^ 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