* 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: 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: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: 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] 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: [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 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 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: 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 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 v2 17/20] mm: free_area_init: allow defining max_zone_pfn in descending order
From: Guenter Roeck @ 2020-05-05 13:18 UTC (permalink / raw)
To: Mike Rapoport
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, Mike Rapoport, 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
In-Reply-To: <20200504153901.GM14260@kernel.org>
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.
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
>
^ permalink raw reply
* Re: [PATCH 2/2] powerpc/perf: Add support for outputting extended regs in perf intr_regs
From: Ravi Bangoria @ 2020-05-05 10:31 UTC (permalink / raw)
To: Anju T Sudhakar
Cc: Ravi Bangoria, maddy, linux-kernel, acme, jolsa, linuxppc-dev
In-Reply-To: <20200429060415.25930-3-anju@linux.vnet.ibm.com>
Hi Anju,
Minor neats...
> /*
> diff --git a/arch/powerpc/include/uapi/asm/perf_regs.h b/arch/powerpc/include/uapi/asm/perf_regs.h
> index f599064dd8dc..604b831378fe 100644
> --- a/arch/powerpc/include/uapi/asm/perf_regs.h
> +++ b/arch/powerpc/include/uapi/asm/perf_regs.h
> @@ -48,6 +48,17 @@ enum perf_event_powerpc_regs {
> PERF_REG_POWERPC_DSISR,
> PERF_REG_POWERPC_SIER,
> PERF_REG_POWERPC_MMCRA,
> - PERF_REG_POWERPC_MAX,
> + /* Extended registers */
> + PERF_REG_POWERPC_MMCR0,
> + PERF_REG_POWERPC_MMCR1,
> + PERF_REG_POWERPC_MMCR2,
> + PERF_REG_EXTENDED_MAX,
> + /* Max regs without the extended regs */
> + PERF_REG_POWERPC_MAX = PERF_REG_POWERPC_MMCRA + 1,
> };
> +
> +#define PERF_REG_PMU_MASK ((1ULL << PERF_REG_POWERPC_MAX) - 1)
Would it make sense to reuse PERF_REG_MASK? Userspace code already uses
that name for the same expression.
> +#define PERF_REG_EXTENDED_MASK (((1ULL << (PERF_REG_EXTENDED_MAX)) \
> + - 1) - PERF_REG_PMU_MASK)
You don't need parenthesis in (PERF_REG_EXTENDED_MAX). Also, better to
keep that `- 1)` in first line.
> +
> #endif /* _UAPI_ASM_POWERPC_PERF_REGS_H */
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index 3dcfecf858f3..f56b77800a7b 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -2276,6 +2276,7 @@ int register_power_pmu(struct power_pmu *pmu)
>
> power_pmu.attr_groups = ppmu->attr_groups;
>
> + power_pmu.capabilities |= (ppmu->capabilities & PERF_PMU_CAP_EXTENDED_REGS);
> #ifdef MSR_HV
> /*
> * Use FCHV to ignore kernel events if MSR.HV is set.
> diff --git a/arch/powerpc/perf/perf_regs.c b/arch/powerpc/perf/perf_regs.c
> index a213a0aa5d25..57aa02568caf 100644
> --- a/arch/powerpc/perf/perf_regs.c
> +++ b/arch/powerpc/perf/perf_regs.c
> @@ -15,7 +15,8 @@
>
> #define PT_REGS_OFFSET(id, r) [id] = offsetof(struct pt_regs, r)
>
> -#define REG_RESERVED (~((1ULL << PERF_REG_POWERPC_MAX) - 1))
> +#define REG_RESERVED (~(PERF_REG_EXTENDED_MASK) & \
> + (~((1ULL << PERF_REG_POWERPC_MAX) - 1)))
Can we reuse PERF_REG_PMU_MASK here and simplify it to:
#define REG_RESERVED (~(PERF_REG_EXTENDED_MASK | PERF_REG_PMU_MASK))
>
> static unsigned int pt_regs_offset[PERF_REG_POWERPC_MAX] = {
> PT_REGS_OFFSET(PERF_REG_POWERPC_R0, gpr[0]),
> @@ -69,10 +70,22 @@ static unsigned int pt_regs_offset[PERF_REG_POWERPC_MAX] = {
> PT_REGS_OFFSET(PERF_REG_POWERPC_MMCRA, dsisr),
> };
>
> +/* Function to return the extended register values */
> +static u64 get_ext_regs_value(int idx)
> +{
> + switch (idx) {
> + case PERF_REG_POWERPC_MMCR0:
> + return mfspr(SPRN_MMCR0);
> + case PERF_REG_POWERPC_MMCR1:
> + return mfspr(SPRN_MMCR1);
> + case PERF_REG_POWERPC_MMCR2:
> + return mfspr(SPRN_MMCR2);
Unnecessary tabs.
[...]
> diff --git a/tools/arch/powerpc/include/uapi/asm/perf_regs.h b/tools/arch/powerpc/include/uapi/asm/perf_regs.h
> index f599064dd8dc..d66953294c73 100644
> --- a/tools/arch/powerpc/include/uapi/asm/perf_regs.h
> +++ b/tools/arch/powerpc/include/uapi/asm/perf_regs.h
> @@ -48,6 +48,17 @@ enum perf_event_powerpc_regs {
> PERF_REG_POWERPC_DSISR,
> PERF_REG_POWERPC_SIER,
> PERF_REG_POWERPC_MMCRA,
> - PERF_REG_POWERPC_MAX,
> + /* Extended arch registers */
> + PERF_REG_POWERPC_MMCR0,
> + PERF_REG_POWERPC_MMCR1,
> + PERF_REG_POWERPC_MMCR2,
> + PERF_REG_EXTENDED_MAX,
> + /* Max regs without extended arch regs */
> + PERF_REG_POWERPC_MAX = PERF_REG_POWERPC_MMCRA + 1,
> +
Unnecesasy line.
> };
> +#define PERF_REG_PMU_MASK ((1ULL << PERF_REG_POWERPC_MAX) - 1)
> +#define PERF_REG_EXTENDED_MASK (((1ULL << (PERF_REG_EXTENDED_MAX))\
> + - 1) - PERF_REG_PMU_MASK)
> +
> #endif /* _UAPI_ASM_POWERPC_PERF_REGS_H */
> diff --git a/tools/perf/arch/powerpc/include/perf_regs.h b/tools/perf/arch/powerpc/include/perf_regs.h
> index e18a3556f5e3..f7bbdb816f88 100644
> --- a/tools/perf/arch/powerpc/include/perf_regs.h
> +++ b/tools/perf/arch/powerpc/include/perf_regs.h
> @@ -64,7 +64,11 @@ static const char *reg_names[] = {
> [PERF_REG_POWERPC_DAR] = "dar",
> [PERF_REG_POWERPC_DSISR] = "dsisr",
> [PERF_REG_POWERPC_SIER] = "sier",
> - [PERF_REG_POWERPC_MMCRA] = "mmcra"
> + [PERF_REG_POWERPC_MMCRA] = "mmcra",
> + [PERF_REG_POWERPC_MMCR0] = "mmcr0",
> + [PERF_REG_POWERPC_MMCR1] = "mmcr1",
> + [PERF_REG_POWERPC_MMCR2] = "mmcr2",
> +
Unnecesasy line.
Apart from those, for the series:
Reviewed-and-Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
^ permalink raw reply
* [PATCH 7/7] binfmt_elf_fdpic: remove the set_fs(KERNEL_DS) in elf_fdpic_core_dump
From: Christoph Hellwig @ 2020-05-05 10:12 UTC (permalink / raw)
To: Andrew Morton, Alexander Viro
Cc: Arnd Bergmann, linuxppc-dev, x86, Oleg Nesterov, linux-kernel,
Eric W . Biederman, linux-fsdevel, Linus Torvalds, Jeremy Kerr
In-Reply-To: <20200505101256.3121270-1-hch@lst.de>
There is no logic in elf_fdpic_core_dump itself or in the various arch
helpers called from it which use uaccess routines on kernel pointers
except for the file writes thate are nicely encapsulated by using
__kernel_write in dump_emit.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/binfmt_elf_fdpic.c | 21 +++++++--------------
1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index 240f666635437..d9501a86cec97 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -1549,7 +1549,6 @@ static int elf_fdpic_core_dump(struct coredump_params *cprm)
{
#define NUM_NOTES 6
int has_dumped = 0;
- mm_segment_t fs;
int segs;
int i;
struct vm_area_struct *vma;
@@ -1589,31 +1588,31 @@ static int elf_fdpic_core_dump(struct coredump_params *cprm)
/* alloc memory for large data structures: too large to be on stack */
elf = kmalloc(sizeof(*elf), GFP_KERNEL);
if (!elf)
- goto cleanup;
+ goto end_coredump;
prstatus = kzalloc(sizeof(*prstatus), GFP_KERNEL);
if (!prstatus)
- goto cleanup;
+ goto end_coredump;
psinfo = kmalloc(sizeof(*psinfo), GFP_KERNEL);
if (!psinfo)
- goto cleanup;
+ goto end_coredump;
notes = kmalloc_array(NUM_NOTES, sizeof(struct memelfnote),
GFP_KERNEL);
if (!notes)
- goto cleanup;
+ goto end_coredump;
fpu = kmalloc(sizeof(*fpu), GFP_KERNEL);
if (!fpu)
- goto cleanup;
+ goto end_coredump;
#ifdef ELF_CORE_COPY_XFPREGS
xfpu = kmalloc(sizeof(*xfpu), GFP_KERNEL);
if (!xfpu)
- goto cleanup;
+ goto end_coredump;
#endif
for (ct = current->mm->core_state->dumper.next;
ct; ct = ct->next) {
tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
if (!tmp)
- goto cleanup;
+ goto end_coredump;
tmp->thread = ct->task;
list_add(&tmp->list, &thread_list);
@@ -1678,9 +1677,6 @@ static int elf_fdpic_core_dump(struct coredump_params *cprm)
"LINUX", ELF_CORE_XFPREG_TYPE, sizeof(*xfpu), xfpu);
#endif
- fs = get_fs();
- set_fs(KERNEL_DS);
-
offset += sizeof(*elf); /* Elf header */
offset += segs * sizeof(struct elf_phdr); /* Program headers */
@@ -1788,9 +1784,6 @@ static int elf_fdpic_core_dump(struct coredump_params *cprm)
}
end_coredump:
- set_fs(fs);
-
-cleanup:
while (!list_empty(&thread_list)) {
struct list_head *tmp = thread_list.next;
list_del(tmp);
--
2.26.2
^ permalink raw reply related
* [PATCH 6/7] binfmt_elf: remove the set_fs(KERNEL_DS) in elf_core_dump
From: Christoph Hellwig @ 2020-05-05 10:12 UTC (permalink / raw)
To: Andrew Morton, Alexander Viro
Cc: Arnd Bergmann, linuxppc-dev, x86, Oleg Nesterov, linux-kernel,
Eric W . Biederman, linux-fsdevel, Linus Torvalds, Jeremy Kerr
In-Reply-To: <20200505101256.3121270-1-hch@lst.de>
There is no logic in elf_core_dump itself or in the various arch helpers
called from it which use uaccess routines on kernel pointers except for
the file writes thate are nicely encapsulated by using __kernel_write in
dump_emit.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/binfmt_elf.c | 16 +---------------
1 file changed, 1 insertion(+), 15 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index a1f57e20c3cf2..ba6f87ba029a7 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1355,7 +1355,6 @@ static unsigned long vma_dump_size(struct vm_area_struct *vma,
vma->vm_pgoff == 0 && (vma->vm_flags & VM_READ)) {
u32 __user *header = (u32 __user *) vma->vm_start;
u32 word;
- mm_segment_t fs = get_fs();
/*
* Doing it this way gets the constant folded by GCC.
*/
@@ -1368,14 +1367,8 @@ static unsigned long vma_dump_size(struct vm_area_struct *vma,
magic.elfmag[EI_MAG1] = ELFMAG1;
magic.elfmag[EI_MAG2] = ELFMAG2;
magic.elfmag[EI_MAG3] = ELFMAG3;
- /*
- * Switch to the user "segment" for get_user(),
- * then put back what elf_core_dump() had in place.
- */
- set_fs(USER_DS);
if (unlikely(get_user(word, header)))
word = 0;
- set_fs(fs);
if (word == magic.cmp)
return PAGE_SIZE;
}
@@ -2183,7 +2176,6 @@ static void fill_extnum_info(struct elfhdr *elf, struct elf_shdr *shdr4extnum,
static int elf_core_dump(struct coredump_params *cprm)
{
int has_dumped = 0;
- mm_segment_t fs;
int segs, i;
size_t vma_data_size = 0;
struct vm_area_struct *vma, *gate_vma;
@@ -2232,13 +2224,10 @@ static int elf_core_dump(struct coredump_params *cprm)
* notes. This also sets up the file header.
*/
if (!fill_note_info(&elf, e_phnum, &info, cprm->siginfo, cprm->regs))
- goto cleanup;
+ goto end_coredump;
has_dumped = 1;
- fs = get_fs();
- set_fs(KERNEL_DS);
-
offset += sizeof(elf); /* Elf header */
offset += segs * sizeof(struct elf_phdr); /* Program headers */
@@ -2366,9 +2355,6 @@ static int elf_core_dump(struct coredump_params *cprm)
}
end_coredump:
- set_fs(fs);
-
-cleanup:
free_note_info(&info);
kfree(shdr4extnum);
kvfree(vma_filesz);
--
2.26.2
^ permalink raw reply related
* [PATCH 5/7] binfmt_elf: remove the set_fs in fill_siginfo_note
From: Christoph Hellwig @ 2020-05-05 10:12 UTC (permalink / raw)
To: Andrew Morton, Alexander Viro
Cc: Arnd Bergmann, linuxppc-dev, x86, Oleg Nesterov, linux-kernel,
Eric W . Biederman, linux-fsdevel, Linus Torvalds, Jeremy Kerr
In-Reply-To: <20200505101256.3121270-1-hch@lst.de>
From: "Eric W. Biederman" <ebiederm@xmission.com>
The code in binfmt_elf.c is differnt from the rest of the code that
processes siginfo, as it sends siginfo from a kernel buffer to a file
rather than from kernel memory to userspace buffers. To remove it's
use of set_fs the code needs some different siginfo helpers.
Add the helper copy_siginfo_to_external to copy from the kernel's
internal siginfo layout to a buffer in the siginfo layout that
userspace expects.
Modify fill_siginfo_note to use copy_siginfo_to_external instead of
set_fs and copy_siginfo_to_user.
Update compat_binfmt_elf.c to use the previously added
copy_siginfo_to_external32 to handle the compat case.
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/binfmt_elf.c | 5 +----
fs/compat_binfmt_elf.c | 2 +-
include/linux/signal.h | 8 ++++++++
3 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 13f25e241ac46..a1f57e20c3cf2 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1556,10 +1556,7 @@ static void fill_auxv_note(struct memelfnote *note, struct mm_struct *mm)
static void fill_siginfo_note(struct memelfnote *note, user_siginfo_t *csigdata,
const kernel_siginfo_t *siginfo)
{
- mm_segment_t old_fs = get_fs();
- set_fs(KERNEL_DS);
- copy_siginfo_to_user((user_siginfo_t __user *) csigdata, siginfo);
- set_fs(old_fs);
+ copy_siginfo_to_external(csigdata, siginfo);
fill_note(note, "CORE", NT_SIGINFO, sizeof(*csigdata), csigdata);
}
diff --git a/fs/compat_binfmt_elf.c b/fs/compat_binfmt_elf.c
index aaad4ca1217ef..fa0e24e1b7267 100644
--- a/fs/compat_binfmt_elf.c
+++ b/fs/compat_binfmt_elf.c
@@ -39,7 +39,7 @@
*/
#define user_long_t compat_long_t
#define user_siginfo_t compat_siginfo_t
-#define copy_siginfo_to_user copy_siginfo_to_user32
+#define copy_siginfo_to_external copy_siginfo_to_external32
/*
* The machine-dependent core note format types are defined in elfcore-compat.h,
diff --git a/include/linux/signal.h b/include/linux/signal.h
index 05bacd2ab1350..6bb1a3f0258c2 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -24,6 +24,14 @@ static inline void clear_siginfo(kernel_siginfo_t *info)
#define SI_EXPANSION_SIZE (sizeof(struct siginfo) - sizeof(struct kernel_siginfo))
+static inline void copy_siginfo_to_external(siginfo_t *to,
+ const kernel_siginfo_t *from)
+{
+ memcpy(to, from, sizeof(*from));
+ memset(((char *)to) + sizeof(struct kernel_siginfo), 0,
+ SI_EXPANSION_SIZE);
+}
+
int copy_siginfo_to_user(siginfo_t __user *to, const kernel_siginfo_t *from);
int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from);
--
2.26.2
^ permalink raw reply related
* [PATCH 4/7] signal: refactor copy_siginfo_to_user32
From: Christoph Hellwig @ 2020-05-05 10:12 UTC (permalink / raw)
To: Andrew Morton, Alexander Viro
Cc: Arnd Bergmann, linuxppc-dev, x86, Oleg Nesterov, linux-kernel,
Eric W . Biederman, linux-fsdevel, Linus Torvalds, Jeremy Kerr
In-Reply-To: <20200505101256.3121270-1-hch@lst.de>
Factor out a copy_siginfo_to_external32 helper from
copy_siginfo_to_user32 that fills out the compat_siginfo, but does so
on a kernel space data structure. With that we can let architectures
override copy_siginfo_to_user32 with their own implementations using
copy_siginfo_to_external32. That allows moving the x32 SIGCHLD purely
to x86 architecture code.
As a nice side effect copy_siginfo_to_external32 also comes in handy
for avoiding a set_fs() call in the coredump code later on.
Contains improvements from Eric W. Biederman <ebiederm@xmission.com>
and Arnd Bergmann <arnd@arndb.de>.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
arch/x86/ia32/ia32_signal.c | 2 +-
arch/x86/include/asm/compat.h | 8 ++-
arch/x86/kernel/signal.c | 28 ++++++++-
include/linux/compat.h | 11 +++-
kernel/signal.c | 106 +++++++++++++++++-----------------
5 files changed, 96 insertions(+), 59 deletions(-)
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index f9d8804144d09..81cf22398cd16 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -350,7 +350,7 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig,
unsafe_put_user(*(__u64 *)set, (__u64 *)&frame->uc.uc_sigmask, Efault);
user_access_end();
- if (__copy_siginfo_to_user32(&frame->info, &ksig->info, false))
+ if (__copy_siginfo_to_user32(&frame->info, &ksig->info))
return -EFAULT;
/* Set up registers for signal handler */
diff --git a/arch/x86/include/asm/compat.h b/arch/x86/include/asm/compat.h
index 52e9f3480f690..d4edf281fff49 100644
--- a/arch/x86/include/asm/compat.h
+++ b/arch/x86/include/asm/compat.h
@@ -214,7 +214,11 @@ static inline bool in_compat_syscall(void)
#endif
struct compat_siginfo;
-int __copy_siginfo_to_user32(struct compat_siginfo __user *to,
- const kernel_siginfo_t *from, bool x32_ABI);
+
+#ifdef CONFIG_X86_X32_ABI
+int copy_siginfo_to_user32(struct compat_siginfo __user *to,
+ const kernel_siginfo_t *from);
+#define copy_siginfo_to_user32 copy_siginfo_to_user32
+#endif /* CONFIG_X86_X32_ABI */
#endif /* _ASM_X86_COMPAT_H */
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 83b74fb38c8fc..f3df262e370b3 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -37,6 +37,7 @@
#include <asm/vm86.h>
#ifdef CONFIG_X86_64
+#include <linux/compat.h>
#include <asm/proto.h>
#include <asm/ia32_unistd.h>
#endif /* CONFIG_X86_64 */
@@ -511,6 +512,31 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
}
#endif /* CONFIG_X86_32 */
+#ifdef CONFIG_X86_X32_ABI
+static int x32_copy_siginfo_to_user(struct compat_siginfo __user *to,
+ const struct kernel_siginfo *from)
+{
+ struct compat_siginfo new;
+
+ copy_siginfo_to_external32(&new, from);
+ if (from->si_signo == SIGCHLD) {
+ new._sifields._sigchld_x32._utime = from->si_utime;
+ new._sifields._sigchld_x32._stime = from->si_stime;
+ }
+ if (copy_to_user(to, &new, sizeof(struct compat_siginfo)))
+ return -EFAULT;
+ return 0;
+}
+
+int copy_siginfo_to_user32(struct compat_siginfo __user *to,
+ const struct kernel_siginfo *from)
+{
+ if (in_x32_syscall())
+ return x32_copy_siginfo_to_user(to, from);
+ return __copy_siginfo_to_user32(to, from);
+}
+#endif /* CONFIG_X86_X32_ABI */
+
static int x32_setup_rt_frame(struct ksignal *ksig,
compat_sigset_t *set,
struct pt_regs *regs)
@@ -543,7 +569,7 @@ static int x32_setup_rt_frame(struct ksignal *ksig,
user_access_end();
if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
- if (__copy_siginfo_to_user32(&frame->info, &ksig->info, true))
+ if (x32_copy_siginfo_to_user(&frame->info, &ksig->info))
return -EFAULT;
}
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 0480ba4db5929..e90100c0de72e 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -402,8 +402,15 @@ long compat_get_bitmap(unsigned long *mask, const compat_ulong_t __user *umask,
unsigned long bitmap_size);
long compat_put_bitmap(compat_ulong_t __user *umask, unsigned long *mask,
unsigned long bitmap_size);
-int copy_siginfo_from_user32(kernel_siginfo_t *to, const struct compat_siginfo __user *from);
-int copy_siginfo_to_user32(struct compat_siginfo __user *to, const kernel_siginfo_t *from);
+void copy_siginfo_to_external32(struct compat_siginfo *to,
+ const struct kernel_siginfo *from);
+int copy_siginfo_from_user32(kernel_siginfo_t *to,
+ const struct compat_siginfo __user *from);
+int __copy_siginfo_to_user32(struct compat_siginfo __user *to,
+ const kernel_siginfo_t *from);
+#ifndef copy_siginfo_to_user32
+#define copy_siginfo_to_user32 __copy_siginfo_to_user32
+#endif
int get_compat_sigevent(struct sigevent *event,
const struct compat_sigevent __user *u_event);
diff --git a/kernel/signal.c b/kernel/signal.c
index 284fc1600063b..5ca48cc5da760 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3235,94 +3235,94 @@ int copy_siginfo_from_user(kernel_siginfo_t *to, const siginfo_t __user *from)
}
#ifdef CONFIG_COMPAT
-int copy_siginfo_to_user32(struct compat_siginfo __user *to,
- const struct kernel_siginfo *from)
-#if defined(CONFIG_X86_X32_ABI) || defined(CONFIG_IA32_EMULATION)
-{
- return __copy_siginfo_to_user32(to, from, in_x32_syscall());
-}
-int __copy_siginfo_to_user32(struct compat_siginfo __user *to,
- const struct kernel_siginfo *from, bool x32_ABI)
-#endif
+/**
+ * copy_siginfo_to_external32 - copy a kernel siginfo into a compat user siginfo
+ * @to: compat siginfo destination
+ * @from: kernel siginfo source
+ *
+ * Note: This function does not work properly for the SIGCHLD on x32, but
+ * fortunately it doesn't have to. The only valid callers for this function are
+ * copy_siginfo_to_user32, which is overriden for x32 and the coredump code.
+ * The latter does not care because SIGCHLD will never cause a coredump.
+ */
+void copy_siginfo_to_external32(struct compat_siginfo *to,
+ const struct kernel_siginfo *from)
{
- struct compat_siginfo new;
- memset(&new, 0, sizeof(new));
+ memset(to, 0, sizeof(*to));
- new.si_signo = from->si_signo;
- new.si_errno = from->si_errno;
- new.si_code = from->si_code;
+ to->si_signo = from->si_signo;
+ to->si_errno = from->si_errno;
+ to->si_code = from->si_code;
switch(siginfo_layout(from->si_signo, from->si_code)) {
case SIL_KILL:
- new.si_pid = from->si_pid;
- new.si_uid = from->si_uid;
+ to->si_pid = from->si_pid;
+ to->si_uid = from->si_uid;
break;
case SIL_TIMER:
- new.si_tid = from->si_tid;
- new.si_overrun = from->si_overrun;
- new.si_int = from->si_int;
+ to->si_tid = from->si_tid;
+ to->si_overrun = from->si_overrun;
+ to->si_int = from->si_int;
break;
case SIL_POLL:
- new.si_band = from->si_band;
- new.si_fd = from->si_fd;
+ to->si_band = from->si_band;
+ to->si_fd = from->si_fd;
break;
case SIL_FAULT:
- new.si_addr = ptr_to_compat(from->si_addr);
+ to->si_addr = ptr_to_compat(from->si_addr);
#ifdef __ARCH_SI_TRAPNO
- new.si_trapno = from->si_trapno;
+ to->si_trapno = from->si_trapno;
#endif
break;
case SIL_FAULT_MCEERR:
- new.si_addr = ptr_to_compat(from->si_addr);
+ to->si_addr = ptr_to_compat(from->si_addr);
#ifdef __ARCH_SI_TRAPNO
- new.si_trapno = from->si_trapno;
+ to->si_trapno = from->si_trapno;
#endif
- new.si_addr_lsb = from->si_addr_lsb;
+ to->si_addr_lsb = from->si_addr_lsb;
break;
case SIL_FAULT_BNDERR:
- new.si_addr = ptr_to_compat(from->si_addr);
+ to->si_addr = ptr_to_compat(from->si_addr);
#ifdef __ARCH_SI_TRAPNO
- new.si_trapno = from->si_trapno;
+ to->si_trapno = from->si_trapno;
#endif
- new.si_lower = ptr_to_compat(from->si_lower);
- new.si_upper = ptr_to_compat(from->si_upper);
+ to->si_lower = ptr_to_compat(from->si_lower);
+ to->si_upper = ptr_to_compat(from->si_upper);
break;
case SIL_FAULT_PKUERR:
- new.si_addr = ptr_to_compat(from->si_addr);
+ to->si_addr = ptr_to_compat(from->si_addr);
#ifdef __ARCH_SI_TRAPNO
- new.si_trapno = from->si_trapno;
+ to->si_trapno = from->si_trapno;
#endif
- new.si_pkey = from->si_pkey;
+ to->si_pkey = from->si_pkey;
break;
case SIL_CHLD:
- new.si_pid = from->si_pid;
- new.si_uid = from->si_uid;
- new.si_status = from->si_status;
-#ifdef CONFIG_X86_X32_ABI
- if (x32_ABI) {
- new._sifields._sigchld_x32._utime = from->si_utime;
- new._sifields._sigchld_x32._stime = from->si_stime;
- } else
-#endif
- {
- new.si_utime = from->si_utime;
- new.si_stime = from->si_stime;
- }
+ to->si_pid = from->si_pid;
+ to->si_uid = from->si_uid;
+ to->si_status = from->si_status;
+ to->si_utime = from->si_utime;
+ to->si_stime = from->si_stime;
break;
case SIL_RT:
- new.si_pid = from->si_pid;
- new.si_uid = from->si_uid;
- new.si_int = from->si_int;
+ to->si_pid = from->si_pid;
+ to->si_uid = from->si_uid;
+ to->si_int = from->si_int;
break;
case SIL_SYS:
- new.si_call_addr = ptr_to_compat(from->si_call_addr);
- new.si_syscall = from->si_syscall;
- new.si_arch = from->si_arch;
+ to->si_call_addr = ptr_to_compat(from->si_call_addr);
+ to->si_syscall = from->si_syscall;
+ to->si_arch = from->si_arch;
break;
}
+}
+int __copy_siginfo_to_user32(struct compat_siginfo __user *to,
+ const struct kernel_siginfo *from)
+{
+ struct compat_siginfo new;
+
+ copy_siginfo_to_external32(&new, from);
if (copy_to_user(to, &new, sizeof(struct compat_siginfo)))
return -EFAULT;
-
return 0;
}
--
2.26.2
^ permalink raw reply related
* [PATCH 3/7] powerpc/spufs: simplify spufs core dumping
From: Christoph Hellwig @ 2020-05-05 10:12 UTC (permalink / raw)
To: Andrew Morton, Alexander Viro
Cc: Arnd Bergmann, linuxppc-dev, x86, Oleg Nesterov, linux-kernel,
Eric W . Biederman, linux-fsdevel, Linus Torvalds, Jeremy Kerr
In-Reply-To: <20200505101256.3121270-1-hch@lst.de>
Replace the coredump ->read method with a ->dump method that must call
dump_emit itself. That way we avoid a buffer allocation an messing with
set_fs() to call into code that is intended to deal with user buffers.
For the ->get case we can now use a small on-stack buffer and avoid
memory allocations as well.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Jeremy Kerr <jk@ozlabs.org>
---
arch/powerpc/platforms/cell/spufs/coredump.c | 87 +++-----
arch/powerpc/platforms/cell/spufs/file.c | 203 ++++++++-----------
arch/powerpc/platforms/cell/spufs/spufs.h | 3 +-
3 files changed, 117 insertions(+), 176 deletions(-)
diff --git a/arch/powerpc/platforms/cell/spufs/coredump.c b/arch/powerpc/platforms/cell/spufs/coredump.c
index 8b3296b62f651..3b75e8f60609c 100644
--- a/arch/powerpc/platforms/cell/spufs/coredump.c
+++ b/arch/powerpc/platforms/cell/spufs/coredump.c
@@ -21,22 +21,6 @@
#include "spufs.h"
-static ssize_t do_coredump_read(int num, struct spu_context *ctx, void *buffer,
- size_t size, loff_t *off)
-{
- u64 data;
- int ret;
-
- if (spufs_coredump_read[num].read)
- return spufs_coredump_read[num].read(ctx, buffer, size, off);
-
- data = spufs_coredump_read[num].get(ctx);
- ret = snprintf(buffer, size, "0x%.16llx", data);
- if (ret >= size)
- return size;
- return ++ret; /* count trailing NULL */
-}
-
static int spufs_ctx_note_size(struct spu_context *ctx, int dfd)
{
int i, sz, total = 0;
@@ -118,58 +102,43 @@ int spufs_coredump_extra_notes_size(void)
static int spufs_arch_write_note(struct spu_context *ctx, int i,
struct coredump_params *cprm, int dfd)
{
- loff_t pos = 0;
- int sz, rc, total = 0;
- const int bufsz = PAGE_SIZE;
- char *name;
- char fullname[80], *buf;
+ size_t sz = spufs_coredump_read[i].size;
+ char fullname[80];
struct elf_note en;
- size_t skip;
-
- buf = (void *)get_zeroed_page(GFP_KERNEL);
- if (!buf)
- return -ENOMEM;
+ size_t ret;
- name = spufs_coredump_read[i].name;
- sz = spufs_coredump_read[i].size;
-
- sprintf(fullname, "SPU/%d/%s", dfd, name);
+ sprintf(fullname, "SPU/%d/%s", dfd, spufs_coredump_read[i].name);
en.n_namesz = strlen(fullname) + 1;
en.n_descsz = sz;
en.n_type = NT_SPU;
if (!dump_emit(cprm, &en, sizeof(en)))
- goto Eio;
-
+ return -EIO;
if (!dump_emit(cprm, fullname, en.n_namesz))
- goto Eio;
-
+ return -EIO;
if (!dump_align(cprm, 4))
- goto Eio;
-
- do {
- rc = do_coredump_read(i, ctx, buf, bufsz, &pos);
- if (rc > 0) {
- if (!dump_emit(cprm, buf, rc))
- goto Eio;
- total += rc;
- }
- } while (rc == bufsz && total < sz);
-
- if (rc < 0)
- goto out;
-
- skip = roundup(cprm->pos - total + sz, 4) - cprm->pos;
- if (!dump_skip(cprm, skip))
- goto Eio;
-
- rc = 0;
-out:
- free_page((unsigned long)buf);
- return rc;
-Eio:
- free_page((unsigned long)buf);
- return -EIO;
+ return -EIO;
+
+ if (spufs_coredump_read[i].dump) {
+ ret = spufs_coredump_read[i].dump(ctx, cprm);
+ if (ret < 0)
+ return ret;
+ } else {
+ char buf[32];
+
+ ret = snprintf(buf, sizeof(buf), "0x%.16llx",
+ spufs_coredump_read[i].get(ctx));
+ if (ret >= sizeof(buf))
+ return sizeof(buf);
+
+ /* count trailing the NULL: */
+ if (!dump_emit(cprm, buf, ret + 1))
+ return -EIO;
+ }
+
+ if (!dump_skip(cprm, roundup(cprm->pos - ret + sz, 4) - cprm->pos))
+ return -EIO;
+ return 0;
}
int spufs_coredump_extra_notes_write(struct coredump_params *cprm)
diff --git a/arch/powerpc/platforms/cell/spufs/file.c b/arch/powerpc/platforms/cell/spufs/file.c
index bd30b5e0c4c37..e44427c245850 100644
--- a/arch/powerpc/platforms/cell/spufs/file.c
+++ b/arch/powerpc/platforms/cell/spufs/file.c
@@ -9,6 +9,7 @@
#undef DEBUG
+#include <linux/coredump.h>
#include <linux/fs.h>
#include <linux/ioctl.h>
#include <linux/export.h>
@@ -129,6 +130,14 @@ static ssize_t spufs_attr_write(struct file *file, const char __user *buf,
return ret;
}
+static ssize_t spufs_dump_emit(struct coredump_params *cprm, void *buf,
+ size_t size)
+{
+ if (!dump_emit(cprm, buf, size))
+ return -EIO;
+ return size;
+}
+
#define DEFINE_SPUFS_SIMPLE_ATTRIBUTE(__fops, __get, __set, __fmt) \
static int __fops ## _open(struct inode *inode, struct file *file) \
{ \
@@ -172,12 +181,9 @@ spufs_mem_release(struct inode *inode, struct file *file)
}
static ssize_t
-__spufs_mem_read(struct spu_context *ctx, char __user *buffer,
- size_t size, loff_t *pos)
+spufs_mem_dump(struct spu_context *ctx, struct coredump_params *cprm)
{
- char *local_store = ctx->ops->get_ls(ctx);
- return simple_read_from_buffer(buffer, size, pos, local_store,
- LS_SIZE);
+ return spufs_dump_emit(cprm, ctx->ops->get_ls(ctx), LS_SIZE);
}
static ssize_t
@@ -190,7 +196,8 @@ spufs_mem_read(struct file *file, char __user *buffer,
ret = spu_acquire(ctx);
if (ret)
return ret;
- ret = __spufs_mem_read(ctx, buffer, size, pos);
+ ret = simple_read_from_buffer(buffer, size, pos, ctx->ops->get_ls(ctx),
+ LS_SIZE);
spu_release(ctx);
return ret;
@@ -459,12 +466,10 @@ spufs_regs_open(struct inode *inode, struct file *file)
}
static ssize_t
-__spufs_regs_read(struct spu_context *ctx, char __user *buffer,
- size_t size, loff_t *pos)
+spufs_regs_dump(struct spu_context *ctx, struct coredump_params *cprm)
{
- struct spu_lscsa *lscsa = ctx->csa.lscsa;
- return simple_read_from_buffer(buffer, size, pos,
- lscsa->gprs, sizeof lscsa->gprs);
+ return spufs_dump_emit(cprm, ctx->csa.lscsa->gprs,
+ sizeof(ctx->csa.lscsa->gprs));
}
static ssize_t
@@ -482,7 +487,8 @@ spufs_regs_read(struct file *file, char __user *buffer,
ret = spu_acquire_saved(ctx);
if (ret)
return ret;
- ret = __spufs_regs_read(ctx, buffer, size, pos);
+ ret = simple_read_from_buffer(buffer, size, pos, ctx->csa.lscsa->gprs,
+ sizeof(ctx->csa.lscsa->gprs));
spu_release_saved(ctx);
return ret;
}
@@ -517,12 +523,10 @@ static const struct file_operations spufs_regs_fops = {
};
static ssize_t
-__spufs_fpcr_read(struct spu_context *ctx, char __user * buffer,
- size_t size, loff_t * pos)
+spufs_fpcr_dump(struct spu_context *ctx, struct coredump_params *cprm)
{
- struct spu_lscsa *lscsa = ctx->csa.lscsa;
- return simple_read_from_buffer(buffer, size, pos,
- &lscsa->fpcr, sizeof(lscsa->fpcr));
+ return spufs_dump_emit(cprm, &ctx->csa.lscsa->fpcr,
+ sizeof(ctx->csa.lscsa->fpcr));
}
static ssize_t
@@ -535,7 +539,8 @@ spufs_fpcr_read(struct file *file, char __user * buffer,
ret = spu_acquire_saved(ctx);
if (ret)
return ret;
- ret = __spufs_fpcr_read(ctx, buffer, size, pos);
+ ret = simple_read_from_buffer(buffer, size, pos, &ctx->csa.lscsa->fpcr,
+ sizeof(ctx->csa.lscsa->fpcr));
spu_release_saved(ctx);
return ret;
}
@@ -953,28 +958,26 @@ spufs_signal1_release(struct inode *inode, struct file *file)
return 0;
}
-static ssize_t __spufs_signal1_read(struct spu_context *ctx, char __user *buf,
- size_t len, loff_t *pos)
+static ssize_t spufs_signal1_dump(struct spu_context *ctx,
+ struct coredump_params *cprm)
{
- int ret = 0;
- u32 data;
+ if (!ctx->csa.spu_chnlcnt_RW[3])
+ return 0;
+ return spufs_dump_emit(cprm, &ctx->csa.spu_chnldata_RW[3],
+ sizeof(ctx->csa.spu_chnldata_RW[3]));
+}
- if (len < 4)
+static ssize_t __spufs_signal1_read(struct spu_context *ctx, char __user *buf,
+ size_t len)
+{
+ if (len < sizeof(ctx->csa.spu_chnldata_RW[3]))
return -EINVAL;
-
- if (ctx->csa.spu_chnlcnt_RW[3]) {
- data = ctx->csa.spu_chnldata_RW[3];
- ret = 4;
- }
-
- if (!ret)
- goto out;
-
- if (copy_to_user(buf, &data, 4))
+ if (!ctx->csa.spu_chnlcnt_RW[3])
+ return 0;
+ if (copy_to_user(buf, &ctx->csa.spu_chnldata_RW[3],
+ sizeof(ctx->csa.spu_chnldata_RW[3])))
return -EFAULT;
-
-out:
- return ret;
+ return sizeof(ctx->csa.spu_chnldata_RW[3]);
}
static ssize_t spufs_signal1_read(struct file *file, char __user *buf,
@@ -986,7 +989,7 @@ static ssize_t spufs_signal1_read(struct file *file, char __user *buf,
ret = spu_acquire_saved(ctx);
if (ret)
return ret;
- ret = __spufs_signal1_read(ctx, buf, len, pos);
+ ret = __spufs_signal1_read(ctx, buf, len);
spu_release_saved(ctx);
return ret;
@@ -1090,28 +1093,26 @@ spufs_signal2_release(struct inode *inode, struct file *file)
return 0;
}
-static ssize_t __spufs_signal2_read(struct spu_context *ctx, char __user *buf,
- size_t len, loff_t *pos)
+static ssize_t spufs_signal2_dump(struct spu_context *ctx,
+ struct coredump_params *cprm)
{
- int ret = 0;
- u32 data;
+ if (!ctx->csa.spu_chnlcnt_RW[4])
+ return 0;
+ return spufs_dump_emit(cprm, &ctx->csa.spu_chnldata_RW[4],
+ sizeof(ctx->csa.spu_chnldata_RW[4]));
+}
- if (len < 4)
+static ssize_t __spufs_signal2_read(struct spu_context *ctx, char __user *buf,
+ size_t len)
+{
+ if (len < sizeof(ctx->csa.spu_chnldata_RW[4]))
return -EINVAL;
-
- if (ctx->csa.spu_chnlcnt_RW[4]) {
- data = ctx->csa.spu_chnldata_RW[4];
- ret = 4;
- }
-
- if (!ret)
- goto out;
-
- if (copy_to_user(buf, &data, 4))
+ if (!ctx->csa.spu_chnlcnt_RW[4])
+ return 0;
+ if (copy_to_user(buf, &ctx->csa.spu_chnldata_RW[4],
+ sizeof(ctx->csa.spu_chnldata_RW[4])))
return -EFAULT;
-
-out:
- return ret;
+ return sizeof(ctx->csa.spu_chnldata_RW[4]);
}
static ssize_t spufs_signal2_read(struct file *file, char __user *buf,
@@ -1123,7 +1124,7 @@ static ssize_t spufs_signal2_read(struct file *file, char __user *buf,
ret = spu_acquire_saved(ctx);
if (ret)
return ret;
- ret = __spufs_signal2_read(ctx, buf, len, pos);
+ ret = __spufs_signal2_read(ctx, buf, len);
spu_release_saved(ctx);
return ret;
@@ -1947,18 +1948,13 @@ static const struct file_operations spufs_caps_fops = {
.release = single_release,
};
-static ssize_t __spufs_mbox_info_read(struct spu_context *ctx,
- char __user *buf, size_t len, loff_t *pos)
+static ssize_t spufs_mbox_info_dump(struct spu_context *ctx,
+ struct coredump_params *cprm)
{
- u32 data;
-
- /* EOF if there's no entry in the mbox */
if (!(ctx->csa.prob.mb_stat_R & 0x0000ff))
return 0;
-
- data = ctx->csa.prob.pu_mb_R;
-
- return simple_read_from_buffer(buf, len, pos, &data, sizeof data);
+ return spufs_dump_emit(cprm, &ctx->csa.prob.pu_mb_R,
+ sizeof(ctx->csa.prob.pu_mb_R));
}
static ssize_t spufs_mbox_info_read(struct file *file, char __user *buf,
@@ -1990,18 +1986,13 @@ static const struct file_operations spufs_mbox_info_fops = {
.llseek = generic_file_llseek,
};
-static ssize_t __spufs_ibox_info_read(struct spu_context *ctx,
- char __user *buf, size_t len, loff_t *pos)
+static ssize_t spufs_ibox_info_dump(struct spu_context *ctx,
+ struct coredump_params *cprm)
{
- u32 data;
-
- /* EOF if there's no entry in the ibox */
if (!(ctx->csa.prob.mb_stat_R & 0xff0000))
return 0;
-
- data = ctx->csa.priv2.puint_mb_R;
-
- return simple_read_from_buffer(buf, len, pos, &data, sizeof data);
+ return spufs_dump_emit(cprm, &ctx->csa.priv2.puint_mb_R,
+ sizeof(ctx->csa.priv2.puint_mb_R));
}
static ssize_t spufs_ibox_info_read(struct file *file, char __user *buf,
@@ -2038,21 +2029,11 @@ static size_t spufs_wbox_info_cnt(struct spu_context *ctx)
return (4 - ((ctx->csa.prob.mb_stat_R & 0x00ff00) >> 8)) * sizeof(u32);
}
-static ssize_t __spufs_wbox_info_read(struct spu_context *ctx,
- char __user *buf, size_t len, loff_t *pos)
+static ssize_t spufs_wbox_info_dump(struct spu_context *ctx,
+ struct coredump_params *cprm)
{
- int i, cnt;
- u32 data[4];
- u32 wbox_stat;
-
- wbox_stat = ctx->csa.prob.mb_stat_R;
- cnt = spufs_wbox_info_cnt(ctx);
- for (i = 0; i < cnt; i++) {
- data[i] = ctx->csa.spu_mailbox_data[i];
- }
-
- return simple_read_from_buffer(buf, len, pos, &data,
- cnt * sizeof(u32));
+ return spufs_dump_emit(cprm, &ctx->csa.spu_mailbox_data,
+ spufs_wbox_info_cnt(ctx));
}
static ssize_t spufs_wbox_info_read(struct file *file, char __user *buf,
@@ -2102,15 +2083,13 @@ static void spufs_get_dma_info(struct spu_context *ctx,
}
}
-static ssize_t __spufs_dma_info_read(struct spu_context *ctx,
- char __user *buf, size_t len, loff_t *pos)
+static ssize_t spufs_dma_info_dump(struct spu_context *ctx,
+ struct coredump_params *cprm)
{
struct spu_dma_info info;
spufs_get_dma_info(ctx, &info);
-
- return simple_read_from_buffer(buf, len, pos, &info,
- sizeof info);
+ return spufs_dump_emit(cprm, &info, sizeof(info));
}
static ssize_t spufs_dma_info_read(struct file *file, char __user *buf,
@@ -2158,22 +2137,13 @@ static void spufs_get_proxydma_info(struct spu_context *ctx,
}
}
-static ssize_t __spufs_proxydma_info_read(struct spu_context *ctx,
- char __user *buf, size_t len, loff_t *pos)
+static ssize_t spufs_proxydma_info_dump(struct spu_context *ctx,
+ struct coredump_params *cprm)
{
struct spu_proxydma_info info;
- int ret = sizeof info;
-
- if (len < ret)
- return -EINVAL;
-
- if (!access_ok(buf, len))
- return -EFAULT;
spufs_get_proxydma_info(ctx, &info);
-
- return simple_read_from_buffer(buf, len, pos, &info,
- sizeof info);
+ return spufs_dump_emit(cprm, &info, sizeof(info));
}
static ssize_t spufs_proxydma_info_read(struct file *file, char __user *buf,
@@ -2183,6 +2153,9 @@ static ssize_t spufs_proxydma_info_read(struct file *file, char __user *buf,
struct spu_proxydma_info info;
int ret;
+ if (len < sizeof(info))
+ return -EINVAL;
+
ret = spu_acquire_saved(ctx);
if (ret)
return ret;
@@ -2636,23 +2609,23 @@ const struct spufs_tree_descr spufs_dir_debug_contents[] = {
};
const struct spufs_coredump_reader spufs_coredump_read[] = {
- { "regs", __spufs_regs_read, NULL, sizeof(struct spu_reg128[128])},
- { "fpcr", __spufs_fpcr_read, NULL, sizeof(struct spu_reg128) },
+ { "regs", spufs_regs_dump, NULL, sizeof(struct spu_reg128[128])},
+ { "fpcr", spufs_fpcr_dump, NULL, sizeof(struct spu_reg128) },
{ "lslr", NULL, spufs_lslr_get, 19 },
{ "decr", NULL, spufs_decr_get, 19 },
{ "decr_status", NULL, spufs_decr_status_get, 19 },
- { "mem", __spufs_mem_read, NULL, LS_SIZE, },
- { "signal1", __spufs_signal1_read, NULL, sizeof(u32) },
+ { "mem", spufs_mem_dump, NULL, LS_SIZE, },
+ { "signal1", spufs_signal1_dump, NULL, sizeof(u32) },
{ "signal1_type", NULL, spufs_signal1_type_get, 19 },
- { "signal2", __spufs_signal2_read, NULL, sizeof(u32) },
+ { "signal2", spufs_signal2_dump, NULL, sizeof(u32) },
{ "signal2_type", NULL, spufs_signal2_type_get, 19 },
{ "event_mask", NULL, spufs_event_mask_get, 19 },
{ "event_status", NULL, spufs_event_status_get, 19 },
- { "mbox_info", __spufs_mbox_info_read, NULL, sizeof(u32) },
- { "ibox_info", __spufs_ibox_info_read, NULL, sizeof(u32) },
- { "wbox_info", __spufs_wbox_info_read, NULL, 4 * sizeof(u32)},
- { "dma_info", __spufs_dma_info_read, NULL, sizeof(struct spu_dma_info)},
- { "proxydma_info", __spufs_proxydma_info_read,
+ { "mbox_info", spufs_mbox_info_dump, NULL, sizeof(u32) },
+ { "ibox_info", spufs_ibox_info_dump, NULL, sizeof(u32) },
+ { "wbox_info", spufs_wbox_info_dump, NULL, 4 * sizeof(u32)},
+ { "dma_info", spufs_dma_info_dump, NULL, sizeof(struct spu_dma_info)},
+ { "proxydma_info", spufs_proxydma_info_dump,
NULL, sizeof(struct spu_proxydma_info)},
{ "object-id", NULL, spufs_object_id_get, 19 },
{ "npc", NULL, spufs_npc_get, 19 },
diff --git a/arch/powerpc/platforms/cell/spufs/spufs.h b/arch/powerpc/platforms/cell/spufs/spufs.h
index 413c89afe1126..1ba4d884febfa 100644
--- a/arch/powerpc/platforms/cell/spufs/spufs.h
+++ b/arch/powerpc/platforms/cell/spufs/spufs.h
@@ -337,8 +337,7 @@ void spufs_dma_callback(struct spu *spu, int type);
extern struct spu_coredump_calls spufs_coredump_calls;
struct spufs_coredump_reader {
char *name;
- ssize_t (*read)(struct spu_context *ctx,
- char __user *buffer, size_t size, loff_t *pos);
+ ssize_t (*dump)(struct spu_context *ctx, struct coredump_params *cprm);
u64 (*get)(struct spu_context *ctx);
size_t size;
};
--
2.26.2
^ permalink raw reply related
* [PATCH 2/7] powerpc/spufs: stop using access_ok
From: Christoph Hellwig @ 2020-05-05 10:12 UTC (permalink / raw)
To: Andrew Morton, Alexander Viro
Cc: Arnd Bergmann, linuxppc-dev, x86, Oleg Nesterov, linux-kernel,
Eric W . Biederman, linux-fsdevel, Linus Torvalds, Jeremy Kerr
In-Reply-To: <20200505101256.3121270-1-hch@lst.de>
Just use the proper non __-prefixed get/put_user variants where that is
not done yet.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
arch/powerpc/platforms/cell/spufs/file.c | 42 +++++-------------------
1 file changed, 8 insertions(+), 34 deletions(-)
diff --git a/arch/powerpc/platforms/cell/spufs/file.c b/arch/powerpc/platforms/cell/spufs/file.c
index f4a4dfb191e7d..bd30b5e0c4c37 100644
--- a/arch/powerpc/platforms/cell/spufs/file.c
+++ b/arch/powerpc/platforms/cell/spufs/file.c
@@ -590,17 +590,12 @@ static ssize_t spufs_mbox_read(struct file *file, char __user *buf,
size_t len, loff_t *pos)
{
struct spu_context *ctx = file->private_data;
- u32 mbox_data, __user *udata;
+ u32 mbox_data, __user *udata = (void __user *)buf;
ssize_t count;
if (len < 4)
return -EINVAL;
- if (!access_ok(buf, len))
- return -EFAULT;
-
- udata = (void __user *)buf;
-
count = spu_acquire(ctx);
if (count)
return count;
@@ -616,7 +611,7 @@ static ssize_t spufs_mbox_read(struct file *file, char __user *buf,
* but still need to return the data we have
* read successfully so far.
*/
- ret = __put_user(mbox_data, udata);
+ ret = put_user(mbox_data, udata);
if (ret) {
if (!count)
count = -EFAULT;
@@ -698,17 +693,12 @@ static ssize_t spufs_ibox_read(struct file *file, char __user *buf,
size_t len, loff_t *pos)
{
struct spu_context *ctx = file->private_data;
- u32 ibox_data, __user *udata;
+ u32 ibox_data, __user *udata = (void __user *)buf;
ssize_t count;
if (len < 4)
return -EINVAL;
- if (!access_ok(buf, len))
- return -EFAULT;
-
- udata = (void __user *)buf;
-
count = spu_acquire(ctx);
if (count)
goto out;
@@ -727,7 +717,7 @@ static ssize_t spufs_ibox_read(struct file *file, char __user *buf,
}
/* if we can't write at all, return -EFAULT */
- count = __put_user(ibox_data, udata);
+ count = put_user(ibox_data, udata);
if (count)
goto out_unlock;
@@ -741,7 +731,7 @@ static ssize_t spufs_ibox_read(struct file *file, char __user *buf,
* but still need to return the data we have
* read successfully so far.
*/
- ret = __put_user(ibox_data, udata);
+ ret = put_user(ibox_data, udata);
if (ret)
break;
}
@@ -836,17 +826,13 @@ static ssize_t spufs_wbox_write(struct file *file, const char __user *buf,
size_t len, loff_t *pos)
{
struct spu_context *ctx = file->private_data;
- u32 wbox_data, __user *udata;
+ u32 wbox_data, __user *udata = (void __user *)buf;
ssize_t count;
if (len < 4)
return -EINVAL;
- udata = (void __user *)buf;
- if (!access_ok(buf, len))
- return -EFAULT;
-
- if (__get_user(wbox_data, udata))
+ if (get_user(wbox_data, udata))
return -EFAULT;
count = spu_acquire(ctx);
@@ -873,7 +859,7 @@ static ssize_t spufs_wbox_write(struct file *file, const char __user *buf,
/* write as much as possible */
for (count = 4, udata++; (count + 4) <= len; count += 4, udata++) {
int ret;
- ret = __get_user(wbox_data, udata);
+ ret = get_user(wbox_data, udata);
if (ret)
break;
@@ -1982,9 +1968,6 @@ static ssize_t spufs_mbox_info_read(struct file *file, char __user *buf,
u32 stat, data;
int ret;
- if (!access_ok(buf, len))
- return -EFAULT;
-
ret = spu_acquire_saved(ctx);
if (ret)
return ret;
@@ -2028,9 +2011,6 @@ static ssize_t spufs_ibox_info_read(struct file *file, char __user *buf,
u32 stat, data;
int ret;
- if (!access_ok(buf, len))
- return -EFAULT;
-
ret = spu_acquire_saved(ctx);
if (ret)
return ret;
@@ -2082,9 +2062,6 @@ static ssize_t spufs_wbox_info_read(struct file *file, char __user *buf,
u32 data[ARRAY_SIZE(ctx->csa.spu_mailbox_data)];
int ret, count;
- if (!access_ok(buf, len))
- return -EFAULT;
-
ret = spu_acquire_saved(ctx);
if (ret)
return ret;
@@ -2143,9 +2120,6 @@ static ssize_t spufs_dma_info_read(struct file *file, char __user *buf,
struct spu_dma_info info;
int ret;
- if (!access_ok(buf, len))
- return -EFAULT;
-
ret = spu_acquire_saved(ctx);
if (ret)
return ret;
--
2.26.2
^ permalink raw reply related
* [PATCH 1/7] powerpc/spufs: fix copy_to_user while atomic
From: Christoph Hellwig @ 2020-05-05 10:12 UTC (permalink / raw)
To: Andrew Morton, Alexander Viro
Cc: Arnd Bergmann, linuxppc-dev, x86, Oleg Nesterov, linux-kernel,
Eric W . Biederman, linux-fsdevel, Linus Torvalds, Jeremy Kerr
In-Reply-To: <20200505101256.3121270-1-hch@lst.de>
From: Jeremy Kerr <jk@ozlabs.org>
Currently, we may perform a copy_to_user (through
simple_read_from_buffer()) while holding a context's register_lock,
while accessing the context save area.
This change uses a temporary buffer for the context save area data,
which we then pass to simple_read_from_buffer.
Includes changes from Christoph Hellwig <hch@lst.de>.
Fixes: bf1ab978be23 ("[POWERPC] coredump: Add SPU elf notes to coredump.")
Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
[hch: renamed to function to avoid ___-prefixes]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
arch/powerpc/platforms/cell/spufs/file.c | 113 +++++++++++++++--------
1 file changed, 75 insertions(+), 38 deletions(-)
diff --git a/arch/powerpc/platforms/cell/spufs/file.c b/arch/powerpc/platforms/cell/spufs/file.c
index c0f950a3f4e1f..f4a4dfb191e7d 100644
--- a/arch/powerpc/platforms/cell/spufs/file.c
+++ b/arch/powerpc/platforms/cell/spufs/file.c
@@ -1978,8 +1978,9 @@ static ssize_t __spufs_mbox_info_read(struct spu_context *ctx,
static ssize_t spufs_mbox_info_read(struct file *file, char __user *buf,
size_t len, loff_t *pos)
{
- int ret;
struct spu_context *ctx = file->private_data;
+ u32 stat, data;
+ int ret;
if (!access_ok(buf, len))
return -EFAULT;
@@ -1988,11 +1989,16 @@ static ssize_t spufs_mbox_info_read(struct file *file, char __user *buf,
if (ret)
return ret;
spin_lock(&ctx->csa.register_lock);
- ret = __spufs_mbox_info_read(ctx, buf, len, pos);
+ stat = ctx->csa.prob.mb_stat_R;
+ data = ctx->csa.prob.pu_mb_R;
spin_unlock(&ctx->csa.register_lock);
spu_release_saved(ctx);
- return ret;
+ /* EOF if there's no entry in the mbox */
+ if (!(stat & 0x0000ff))
+ return 0;
+
+ return simple_read_from_buffer(buf, len, pos, &data, sizeof(data));
}
static const struct file_operations spufs_mbox_info_fops = {
@@ -2019,6 +2025,7 @@ static ssize_t spufs_ibox_info_read(struct file *file, char __user *buf,
size_t len, loff_t *pos)
{
struct spu_context *ctx = file->private_data;
+ u32 stat, data;
int ret;
if (!access_ok(buf, len))
@@ -2028,11 +2035,16 @@ static ssize_t spufs_ibox_info_read(struct file *file, char __user *buf,
if (ret)
return ret;
spin_lock(&ctx->csa.register_lock);
- ret = __spufs_ibox_info_read(ctx, buf, len, pos);
+ stat = ctx->csa.prob.mb_stat_R;
+ data = ctx->csa.priv2.puint_mb_R;
spin_unlock(&ctx->csa.register_lock);
spu_release_saved(ctx);
- return ret;
+ /* EOF if there's no entry in the ibox */
+ if (!(stat & 0xff0000))
+ return 0;
+
+ return simple_read_from_buffer(buf, len, pos, &data, sizeof(data));
}
static const struct file_operations spufs_ibox_info_fops = {
@@ -2041,6 +2053,11 @@ static const struct file_operations spufs_ibox_info_fops = {
.llseek = generic_file_llseek,
};
+static size_t spufs_wbox_info_cnt(struct spu_context *ctx)
+{
+ return (4 - ((ctx->csa.prob.mb_stat_R & 0x00ff00) >> 8)) * sizeof(u32);
+}
+
static ssize_t __spufs_wbox_info_read(struct spu_context *ctx,
char __user *buf, size_t len, loff_t *pos)
{
@@ -2049,7 +2066,7 @@ static ssize_t __spufs_wbox_info_read(struct spu_context *ctx,
u32 wbox_stat;
wbox_stat = ctx->csa.prob.mb_stat_R;
- cnt = 4 - ((wbox_stat & 0x00ff00) >> 8);
+ cnt = spufs_wbox_info_cnt(ctx);
for (i = 0; i < cnt; i++) {
data[i] = ctx->csa.spu_mailbox_data[i];
}
@@ -2062,7 +2079,8 @@ static ssize_t spufs_wbox_info_read(struct file *file, char __user *buf,
size_t len, loff_t *pos)
{
struct spu_context *ctx = file->private_data;
- int ret;
+ u32 data[ARRAY_SIZE(ctx->csa.spu_mailbox_data)];
+ int ret, count;
if (!access_ok(buf, len))
return -EFAULT;
@@ -2071,11 +2089,13 @@ static ssize_t spufs_wbox_info_read(struct file *file, char __user *buf,
if (ret)
return ret;
spin_lock(&ctx->csa.register_lock);
- ret = __spufs_wbox_info_read(ctx, buf, len, pos);
+ count = spufs_wbox_info_cnt(ctx);
+ memcpy(&data, &ctx->csa.spu_mailbox_data, sizeof(data));
spin_unlock(&ctx->csa.register_lock);
spu_release_saved(ctx);
- return ret;
+ return simple_read_from_buffer(buf, len, pos, &data,
+ count * sizeof(u32));
}
static const struct file_operations spufs_wbox_info_fops = {
@@ -2084,27 +2104,33 @@ static const struct file_operations spufs_wbox_info_fops = {
.llseek = generic_file_llseek,
};
-static ssize_t __spufs_dma_info_read(struct spu_context *ctx,
- char __user *buf, size_t len, loff_t *pos)
+static void spufs_get_dma_info(struct spu_context *ctx,
+ struct spu_dma_info *info)
{
- struct spu_dma_info info;
- struct mfc_cq_sr *qp, *spuqp;
int i;
- info.dma_info_type = ctx->csa.priv2.spu_tag_status_query_RW;
- info.dma_info_mask = ctx->csa.lscsa->tag_mask.slot[0];
- info.dma_info_status = ctx->csa.spu_chnldata_RW[24];
- info.dma_info_stall_and_notify = ctx->csa.spu_chnldata_RW[25];
- info.dma_info_atomic_command_status = ctx->csa.spu_chnldata_RW[27];
+ info->dma_info_type = ctx->csa.priv2.spu_tag_status_query_RW;
+ info->dma_info_mask = ctx->csa.lscsa->tag_mask.slot[0];
+ info->dma_info_status = ctx->csa.spu_chnldata_RW[24];
+ info->dma_info_stall_and_notify = ctx->csa.spu_chnldata_RW[25];
+ info->dma_info_atomic_command_status = ctx->csa.spu_chnldata_RW[27];
for (i = 0; i < 16; i++) {
- qp = &info.dma_info_command_data[i];
- spuqp = &ctx->csa.priv2.spuq[i];
+ struct mfc_cq_sr *qp = &info->dma_info_command_data[i];
+ struct mfc_cq_sr *spuqp = &ctx->csa.priv2.spuq[i];
qp->mfc_cq_data0_RW = spuqp->mfc_cq_data0_RW;
qp->mfc_cq_data1_RW = spuqp->mfc_cq_data1_RW;
qp->mfc_cq_data2_RW = spuqp->mfc_cq_data2_RW;
qp->mfc_cq_data3_RW = spuqp->mfc_cq_data3_RW;
}
+}
+
+static ssize_t __spufs_dma_info_read(struct spu_context *ctx,
+ char __user *buf, size_t len, loff_t *pos)
+{
+ struct spu_dma_info info;
+
+ spufs_get_dma_info(ctx, &info);
return simple_read_from_buffer(buf, len, pos, &info,
sizeof info);
@@ -2114,6 +2140,7 @@ static ssize_t spufs_dma_info_read(struct file *file, char __user *buf,
size_t len, loff_t *pos)
{
struct spu_context *ctx = file->private_data;
+ struct spu_dma_info info;
int ret;
if (!access_ok(buf, len))
@@ -2123,11 +2150,12 @@ static ssize_t spufs_dma_info_read(struct file *file, char __user *buf,
if (ret)
return ret;
spin_lock(&ctx->csa.register_lock);
- ret = __spufs_dma_info_read(ctx, buf, len, pos);
+ spufs_get_dma_info(ctx, &info);
spin_unlock(&ctx->csa.register_lock);
spu_release_saved(ctx);
- return ret;
+ return simple_read_from_buffer(buf, len, pos, &info,
+ sizeof(info));
}
static const struct file_operations spufs_dma_info_fops = {
@@ -2136,13 +2164,31 @@ static const struct file_operations spufs_dma_info_fops = {
.llseek = no_llseek,
};
+static void spufs_get_proxydma_info(struct spu_context *ctx,
+ struct spu_proxydma_info *info)
+{
+ int i;
+
+ info->proxydma_info_type = ctx->csa.prob.dma_querytype_RW;
+ info->proxydma_info_mask = ctx->csa.prob.dma_querymask_RW;
+ info->proxydma_info_status = ctx->csa.prob.dma_tagstatus_R;
+
+ for (i = 0; i < 8; i++) {
+ struct mfc_cq_sr *qp = &info->proxydma_info_command_data[i];
+ struct mfc_cq_sr *puqp = &ctx->csa.priv2.puq[i];
+
+ qp->mfc_cq_data0_RW = puqp->mfc_cq_data0_RW;
+ qp->mfc_cq_data1_RW = puqp->mfc_cq_data1_RW;
+ qp->mfc_cq_data2_RW = puqp->mfc_cq_data2_RW;
+ qp->mfc_cq_data3_RW = puqp->mfc_cq_data3_RW;
+ }
+}
+
static ssize_t __spufs_proxydma_info_read(struct spu_context *ctx,
char __user *buf, size_t len, loff_t *pos)
{
struct spu_proxydma_info info;
- struct mfc_cq_sr *qp, *puqp;
int ret = sizeof info;
- int i;
if (len < ret)
return -EINVAL;
@@ -2150,18 +2196,7 @@ static ssize_t __spufs_proxydma_info_read(struct spu_context *ctx,
if (!access_ok(buf, len))
return -EFAULT;
- info.proxydma_info_type = ctx->csa.prob.dma_querytype_RW;
- info.proxydma_info_mask = ctx->csa.prob.dma_querymask_RW;
- info.proxydma_info_status = ctx->csa.prob.dma_tagstatus_R;
- for (i = 0; i < 8; i++) {
- qp = &info.proxydma_info_command_data[i];
- puqp = &ctx->csa.priv2.puq[i];
-
- qp->mfc_cq_data0_RW = puqp->mfc_cq_data0_RW;
- qp->mfc_cq_data1_RW = puqp->mfc_cq_data1_RW;
- qp->mfc_cq_data2_RW = puqp->mfc_cq_data2_RW;
- qp->mfc_cq_data3_RW = puqp->mfc_cq_data3_RW;
- }
+ spufs_get_proxydma_info(ctx, &info);
return simple_read_from_buffer(buf, len, pos, &info,
sizeof info);
@@ -2171,17 +2206,19 @@ static ssize_t spufs_proxydma_info_read(struct file *file, char __user *buf,
size_t len, loff_t *pos)
{
struct spu_context *ctx = file->private_data;
+ struct spu_proxydma_info info;
int ret;
ret = spu_acquire_saved(ctx);
if (ret)
return ret;
spin_lock(&ctx->csa.register_lock);
- ret = __spufs_proxydma_info_read(ctx, buf, len, pos);
+ spufs_get_proxydma_info(ctx, &info);
spin_unlock(&ctx->csa.register_lock);
spu_release_saved(ctx);
- return ret;
+ return simple_read_from_buffer(buf, len, pos, &info,
+ sizeof(info));
}
static const struct file_operations spufs_proxydma_info_fops = {
--
2.26.2
^ permalink raw reply related
* remove set_fs calls from the coredump code v6
From: Christoph Hellwig @ 2020-05-05 10:12 UTC (permalink / raw)
To: Andrew Morton, Alexander Viro
Cc: Arnd Bergmann, linuxppc-dev, x86, Oleg Nesterov, linux-kernel,
Eric W . Biederman, linux-fsdevel, Linus Torvalds, Jeremy Kerr
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
^ permalink raw reply
* Re: [RFC][PATCH 0/2] Add support for using reserved memory for ima buffer pass
From: Mark Rutland @ 2020-05-05 9:59 UTC (permalink / raw)
To: Prakhar Srivastava
Cc: kstewart, gregkh, bhsharma, tao.li, zohar, paulus,
vincenzo.frascino, will, nramas, frowand.list, masahiroy, jmorris,
takahiro.akashi, linux-arm-kernel, catalin.marinas, serge,
devicetree, pasha.tatashin, robh+dt, hsinyi, tusharsu, tglx,
allison, mbrugger, balajib, dmitry.kasatkin, linux-kernel,
linux-security-module, james.morse, linux-integrity, linuxppc-dev
In-Reply-To: <20200504203829.6330-1-prsriva@linux.microsoft.com>
Hi Prakhar,
On Mon, May 04, 2020 at 01:38:27PM -0700, Prakhar Srivastava wrote:
> IMA during kexec(kexec file load) verifies the kernel signature and measures
> the signature of the kernel. The signature in the logs can be used to verfiy the
> authenticity of the kernel. The logs don not get carried over kexec and thus
> remote attesation cannot verify the signature of the running kernel.
>
> Introduce an ABI to carry forward the ima logs over kexec.
> Memory reserved via device tree reservation can be used to store and read
> via the of_* functions.
This flow needs to work for:
1) Pure DT
2) DT + EFI memory map
3) ACPI + EFI memory map
... and if this is just for transiently passing the log, I don't think
that a reserved memory region is the right thing to use, since they're
supposed to be more permanent.
This sounds analogous to passing the initrd, and should probably use
properties under the chosen node (which can be used for all three boot
flows above).
For reference, how big is the IMA log likely to be? Does it need
physically contiguous space?
Thanks,
Mark.
>
> Reserved memory stores the size(sizeof(size_t)) of the buffer in the starting
> address, followed by the IMA log contents.
>
> Tested on:
> arm64 with Uboot
>
> Prakhar Srivastava (2):
> Add a layer of abstraction to use the memory reserved by device tree
> for ima buffer pass.
> Add support for ima buffer pass using reserved memory for arm64 kexec.
> Update the arch sepcific code path in kexec file load to store the
> ima buffer in the reserved memory. The same reserved memory is read
> on kexec or cold boot.
>
> arch/arm64/Kconfig | 1 +
> arch/arm64/include/asm/ima.h | 22 ++++
> arch/arm64/include/asm/kexec.h | 5 +
> arch/arm64/kernel/Makefile | 1 +
> arch/arm64/kernel/ima_kexec.c | 64 ++++++++++
> arch/arm64/kernel/machine_kexec_file.c | 1 +
> arch/powerpc/include/asm/ima.h | 3 +-
> arch/powerpc/kexec/ima.c | 14 ++-
> drivers/of/Kconfig | 6 +
> drivers/of/Makefile | 1 +
> drivers/of/of_ima.c | 165 +++++++++++++++++++++++++
> include/linux/of.h | 34 +++++
> security/integrity/ima/ima_kexec.c | 15 ++-
> 13 files changed, 325 insertions(+), 7 deletions(-)
> create mode 100644 arch/arm64/include/asm/ima.h
> create mode 100644 arch/arm64/kernel/ima_kexec.c
> create mode 100644 drivers/of/of_ima.c
>
> --
> 2.25.1
>
^ permalink raw reply
* Re: [PATCH v2 0/5] Statsfs: a new ram-based file sytem for Linux kernel statistics
From: Emanuele Giuseppe Esposito @ 2020-05-05 9:18 UTC (permalink / raw)
To: David Rientjes
Cc: linux-s390, kvm, David Hildenbrand, Cornelia Huck,
Emanuele Giuseppe Esposito, linux-kernel, kvm-ppc, Jonathan Adams,
Christian Borntraeger, Alexander Viro, linux-fsdevel,
Paolo Bonzini, Vitaly Kuznetsov, linux-mips, linuxppc-dev,
Jim Mattson
In-Reply-To: <alpine.DEB.2.22.394.2005041429210.224786@chino.kir.corp.google.com>
On 5/4/20 11:37 PM, David Rientjes wrote:
> On Mon, 4 May 2020, Emanuele Giuseppe Esposito wrote:
>
>>
>> In this patch series I introduce statsfs, a synthetic ram-based virtual
>> filesystem that takes care of gathering and displaying statistics for the
>> Linux kernel subsystems.
>>
>
> This is exciting, we have been looking in the same area recently. Adding
> Jonathan Adams <jwadams@google.com>.
>
> In your diffstat, one thing I notice that is omitted: an update to
> Documentation/* :) Any chance of getting some proposed Documentation/
> updates with structure of the fs, the per subsystem breakdown, and best
> practices for managing the stats from the kernel level?
Yes, I will write some documentation. Thank you for the suggestion.
>>
>> Values represent quantites that are gathered by the statsfs user. Examples
>> of values include the number of vm exits of a given kind, the amount of
>> memory used by some data structure, the length of the longest hash table
>> chain, or anything like that. Values are defined with the
>> statsfs_source_add_values function. Each value is defined by a struct
>> statsfs_value; the same statsfs_value can be added to many different
>> sources. A value can be considered "simple" if it fetches data from a
>> user-provided location, or "aggregate" if it groups all values in the
>> subordinates sources that include the same statsfs_value.
>>
>
> This seems like it could have a lot of overhead if we wanted to
> periodically track the totality of subsystem stats as a form of telemetry
> gathering from userspace. To collect telemetry for 1,000 different stats,
> do we need to issue lseek()+read() syscalls for each of them individually
> (or, worse, open()+read()+close())?
>
> Any thoughts on how that can be optimized? A couple of ideas:
>
> - an interface that allows gathering of all stats for a particular
> interface through a single file that would likely be encoded in binary
> and the responsibility of userspace to disseminate, or
>
> - an interface that extends beyond this proposal and allows the reader to
> specify which stats they are interested in collecting and then the
> kernel will only provide these stats in a well formed structure and
> also be binary encoded.
Are you thinking of another file, containing all the stats for the
directory in binary format?
> We've found that the one-file-per-stat method is pretty much a show
> stopper from the performance view and we always must execute at least two
> syscalls to obtain a single stat.
>
> 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.
Thank you,
Emanuele
>> [1] https://lore.kernel.org/kvm/5d6cdcb1-d8ad-7ae6-7351-3544e2fa366d@redhat.com/?fbclid=IwAR18LHJ0PBcXcDaLzILFhHsl3qpT3z2vlG60RnqgbpGYhDv7L43n0ZXJY8M
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>
>> v1->v2 remove unnecessary list_foreach_safe loops, fix wrong indentation,
>> change statsfs in stats_fs
>>
>> Emanuele Giuseppe Esposito (5):
>> refcount, kref: add dec-and-test wrappers for rw_semaphores
>> stats_fs API: create, add and remove stats_fs sources and values
>> kunit: tests for stats_fs API
>> stats_fs fs: virtual fs to show stats to the end-user
>> kvm_main: replace debugfs with stats_fs
>>
>> MAINTAINERS | 7 +
>> arch/arm64/kvm/Kconfig | 1 +
>> arch/arm64/kvm/guest.c | 2 +-
>> arch/mips/kvm/Kconfig | 1 +
>> arch/mips/kvm/mips.c | 2 +-
>> arch/powerpc/kvm/Kconfig | 1 +
>> arch/powerpc/kvm/book3s.c | 6 +-
>> arch/powerpc/kvm/booke.c | 8 +-
>> arch/s390/kvm/Kconfig | 1 +
>> arch/s390/kvm/kvm-s390.c | 16 +-
>> arch/x86/include/asm/kvm_host.h | 2 +-
>> arch/x86/kvm/Kconfig | 1 +
>> arch/x86/kvm/Makefile | 2 +-
>> arch/x86/kvm/debugfs.c | 64 --
>> arch/x86/kvm/stats_fs.c | 56 ++
>> arch/x86/kvm/x86.c | 6 +-
>> fs/Kconfig | 12 +
>> fs/Makefile | 1 +
>> fs/stats_fs/Makefile | 6 +
>> fs/stats_fs/inode.c | 337 ++++++++++
>> fs/stats_fs/internal.h | 35 +
>> fs/stats_fs/stats_fs-tests.c | 1088 +++++++++++++++++++++++++++++++
>> fs/stats_fs/stats_fs.c | 773 ++++++++++++++++++++++
>> include/linux/kref.h | 11 +
>> include/linux/kvm_host.h | 39 +-
>> include/linux/refcount.h | 2 +
>> include/linux/stats_fs.h | 304 +++++++++
>> include/uapi/linux/magic.h | 1 +
>> lib/refcount.c | 32 +
>> tools/lib/api/fs/fs.c | 21 +
>> virt/kvm/arm/arm.c | 2 +-
>> virt/kvm/kvm_main.c | 314 ++-------
>> 32 files changed, 2772 insertions(+), 382 deletions(-)
>> delete mode 100644 arch/x86/kvm/debugfs.c
>> create mode 100644 arch/x86/kvm/stats_fs.c
>> create mode 100644 fs/stats_fs/Makefile
>> create mode 100644 fs/stats_fs/inode.c
>> create mode 100644 fs/stats_fs/internal.h
>> create mode 100644 fs/stats_fs/stats_fs-tests.c
>> create mode 100644 fs/stats_fs/stats_fs.c
>> create mode 100644 include/linux/stats_fs.h
>>
>> --
>> 2.25.2
>>
^ permalink raw reply
* Re: [PATCH v4 2/7] KVM: arm64: clean up redundant 'kvm_run' parameters
From: Marc Zyngier @ 2020-05-05 8:39 UTC (permalink / raw)
To: Tianjia Zhang
Cc: wanpengli, kvm, david, heiko.carstens, peterx, linux-mips, hpa,
kvmarm, linux-s390, frankja, chenhuacai, joro, x86, borntraeger,
mingo, julien.thierry.kdev, thuth, gor, suzuki.poulose, kvm-ppc,
bp, tglx, linux-arm-kernel, jmattson, tsbogend, cohuck,
christoffer.dall, sean.j.christopherson, linux-kernel,
james.morse, pbonzini, vkuznets, linuxppc-dev
In-Reply-To: <20200427043514.16144-3-tianjia.zhang@linux.alibaba.com>
Hi Tianjia,
On 2020-04-27 05:35, Tianjia Zhang wrote:
> In the current kvm version, 'kvm_run' has been included in the
> 'kvm_vcpu'
> structure. For historical reasons, many kvm-related function parameters
> retain the 'kvm_run' and 'kvm_vcpu' parameters at the same time. This
> patch does a unified cleanup of these remaining redundant parameters.
>
> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
On the face of it, this looks OK, but I haven't tried to run the
resulting kernel. I'm not opposed to taking this patch *if* there
is an agreement across architectures to take the series (I value
consistency over the janitorial exercise).
Another thing is that this is going to conflict with the set of
patches that move the KVM/arm code back where it belongs
(arch/arm64/kvm),
so I'd probably cherry-pick that one directly.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* Re: [PATCH v7 25/28] powerpc: Test prefixed instructions in feature fixups
From: Jordan Niethe @ 2020-05-05 7:34 UTC (permalink / raw)
To: Alistair Popple
Cc: Nicholas Piggin, Balamuruhan S, naveen.n.rao, linuxppc-dev,
Daniel Axtens
In-Reply-To: <5803808.3aLiq0rt1P@townsend>
On Tue, May 5, 2020 at 5:15 PM Alistair Popple <alistair@popple.id.au> wrote:
>
> Hmm, I was hoping to add a tested by but I'm seeing the following failure in
> Mambo:
>
> [ 1.475459] feature-fixups: test failed at line 730
>
> Based on the name of the test it looks like you probably made a copy/paste
> error in ftr_fixup_prefix2_expected. I suspect you probably meant to use the alt
> fixup:
>
> globl(ftr_fixup_prefix2_expected)
> or 1,1,1
> .long 0x7000000
> .long 0x0000001
> or 2,2,2
Thanks, I changed from using 0x7000000 to 1 << 26 but missed here.
Changing that fixes this.
>
> Also for some reason these tests (and one of the code-patching tests) aren't
> passing on big endian.
Okay, will fix that.
>
> - Alistair
>
> On Friday, 1 May 2020 1:42:17 PM AEST Jordan Niethe wrote:
> > Expand the feature-fixups self-tests to includes tests for prefixed
> > instructions.
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > v6: New to series
> > ---
> > arch/powerpc/lib/feature-fixups-test.S | 68 +++++++++++++++++++++++
> > arch/powerpc/lib/feature-fixups.c | 74 ++++++++++++++++++++++++++
> > 2 files changed, 142 insertions(+)
> >
> > diff --git a/arch/powerpc/lib/feature-fixups-test.S
> > b/arch/powerpc/lib/feature-fixups-test.S index b12168c2447a..6e2da9123a9b
> > 100644
> > --- a/arch/powerpc/lib/feature-fixups-test.S
> > +++ b/arch/powerpc/lib/feature-fixups-test.S
> > @@ -791,3 +791,71 @@ globl(lwsync_fixup_test_expected_SYNC)
> > 1: or 1,1,1
> > sync
> >
> > +globl(ftr_fixup_prefix1)
> > + or 1,1,1
> > + .long 1 << 26
> > + .long 0x0000000
> > + or 2,2,2
> > +globl(end_ftr_fixup_prefix1)
> > +
> > +globl(ftr_fixup_prefix1_orig)
> > + or 1,1,1
> > + .long 1 << 26
> > + .long 0x0000000
> > + or 2,2,2
> > +
> > +globl(ftr_fixup_prefix1_expected)
> > + or 1,1,1
> > + nop
> > + nop
> > + or 2,2,2
> > +
> > +globl(ftr_fixup_prefix2)
> > + or 1,1,1
> > + .long 1 << 26
> > + .long 0x0000000
> > + or 2,2,2
> > +globl(end_ftr_fixup_prefix2)
> > +
> > +globl(ftr_fixup_prefix2_orig)
> > + or 1,1,1
> > + .long 1 << 26
> > + .long 0x0000000
> > + or 2,2,2
> > +
> > +globl(ftr_fixup_prefix2_alt)
> > + .long 0x7000000
> > + .long 0x0000001
> > +
> > +globl(ftr_fixup_prefix2_expected)
> > + or 1,1,1
> > + .long 1 << 26
> > + .long 0x0000001
> > + or 2,2,2
> > +
> > +globl(ftr_fixup_prefix3)
> > + or 1,1,1
> > + .long 1 << 26
> > + .long 0x0000000
> > + or 2,2,2
> > + or 3,3,3
> > +globl(end_ftr_fixup_prefix3)
> > +
> > +globl(ftr_fixup_prefix3_orig)
> > + or 1,1,1
> > + .long 1 << 26
> > + .long 0x0000000
> > + or 2,2,2
> > + or 3,3,3
> > +
> > +globl(ftr_fixup_prefix3_alt)
> > + .long 1 << 26
> > + .long 0x0000001
> > + nop
> > +
> > +globl(ftr_fixup_prefix3_expected)
> > + or 1,1,1
> > + .long 1 << 26
> > + .long 0x0000001
> > + nop
> > + or 3,3,3
> > diff --git a/arch/powerpc/lib/feature-fixups.c
> > b/arch/powerpc/lib/feature-fixups.c index 243011f85287..6fc499b1d63e 100644
> > --- a/arch/powerpc/lib/feature-fixups.c
> > +++ b/arch/powerpc/lib/feature-fixups.c
> > @@ -687,6 +687,75 @@ static void test_lwsync_macros(void)
> > }
> > }
> >
> > +#ifdef __powerpc64__
> > +static void __init test_prefix_patching(void)
> > +{
> > + extern unsigned int ftr_fixup_prefix1[];
> > + extern unsigned int end_ftr_fixup_prefix1[];
> > + extern unsigned int ftr_fixup_prefix1_orig[];
> > + extern unsigned int ftr_fixup_prefix1_expected[];
> > + int size = sizeof(unsigned int) * (end_ftr_fixup_prefix1 -
> > ftr_fixup_prefix1); +
> > + fixup.value = fixup.mask = 8;
> > + fixup.start_off = calc_offset(&fixup, ftr_fixup_prefix1 + 1);
> > + fixup.end_off = calc_offset(&fixup, ftr_fixup_prefix1 + 3);
> > + fixup.alt_start_off = fixup.alt_end_off = 0;
> > +
> > + /* Sanity check */
> > + check(memcmp(ftr_fixup_prefix1, ftr_fixup_prefix1_orig, size) == 0);
> > +
> > + patch_feature_section(0, &fixup);
> > + check(memcmp(ftr_fixup_prefix1, ftr_fixup_prefix1_expected, size) == 0);
> > + check(memcmp(ftr_fixup_prefix1, ftr_fixup_prefix1_orig, size) != 0);
> > +}
> > +
> > +static void __init test_prefix_alt_patching(void)
> > +{
> > + extern unsigned int ftr_fixup_prefix2[];
> > + extern unsigned int end_ftr_fixup_prefix2[];
> > + extern unsigned int ftr_fixup_prefix2_orig[];
> > + extern unsigned int ftr_fixup_prefix2_expected[];
> > + extern unsigned int ftr_fixup_prefix2_alt[];
> > + int size = sizeof(unsigned int) * (end_ftr_fixup_prefix2 -
> > ftr_fixup_prefix2); +
> > + fixup.value = fixup.mask = 8;
> > + fixup.start_off = calc_offset(&fixup, ftr_fixup_prefix2 + 1);
> > + fixup.end_off = calc_offset(&fixup, ftr_fixup_prefix2 + 3);
> > + fixup.alt_start_off = calc_offset(&fixup, ftr_fixup_prefix2_alt);
> > + fixup.alt_end_off = calc_offset(&fixup, ftr_fixup_prefix2_alt + 2);
> > + /* Sanity check */
> > + check(memcmp(ftr_fixup_prefix2, ftr_fixup_prefix2_orig, size) == 0);
> > +
> > + patch_feature_section(0, &fixup);
> > + check(memcmp(ftr_fixup_prefix2, ftr_fixup_prefix2_expected, size) == 0);
> > + patch_feature_section(0, &fixup);
> > + check(memcmp(ftr_fixup_prefix2, ftr_fixup_prefix2_orig, size) != 0);
> > +}
> > +
> > +static void __init test_prefix_word_alt_patching(void)
> > +{
> > + extern unsigned int ftr_fixup_prefix3[];
> > + extern unsigned int end_ftr_fixup_prefix3[];
> > + extern unsigned int ftr_fixup_prefix3_orig[];
> > + extern unsigned int ftr_fixup_prefix3_expected[];
> > + extern unsigned int ftr_fixup_prefix3_alt[];
> > + int size = sizeof(unsigned int) * (end_ftr_fixup_prefix3 -
> > ftr_fixup_prefix3); +
> > + fixup.value = fixup.mask = 8;
> > + fixup.start_off = calc_offset(&fixup, ftr_fixup_prefix3 + 1);
> > + fixup.end_off = calc_offset(&fixup, ftr_fixup_prefix3 + 4);
> > + fixup.alt_start_off = calc_offset(&fixup, ftr_fixup_prefix3_alt);
> > + fixup.alt_end_off = calc_offset(&fixup, ftr_fixup_prefix3_alt + 3);
> > + /* Sanity check */
> > + check(memcmp(ftr_fixup_prefix3, ftr_fixup_prefix3_orig, size) == 0);
> > +
> > + patch_feature_section(0, &fixup);
> > + check(memcmp(ftr_fixup_prefix3, ftr_fixup_prefix3_expected, size) == 0);
> > + patch_feature_section(0, &fixup);
> > + check(memcmp(ftr_fixup_prefix3, ftr_fixup_prefix3_orig, size) != 0);
> > +}
> > +#endif /* __powerpc64__ */
> > +
> > static int __init test_feature_fixups(void)
> > {
> > printk(KERN_DEBUG "Running feature fixup self-tests ...\n");
> > @@ -701,6 +770,11 @@ static int __init test_feature_fixups(void)
> > test_cpu_macros();
> > test_fw_macros();
> > test_lwsync_macros();
> > +#ifdef __powerpc64__
> > + test_prefix_patching();
> > + test_prefix_alt_patching();
> > + test_prefix_word_alt_patching();
> > +#endif
> >
> > return 0;
> > }
>
>
>
>
^ permalink raw reply
* Re: [PATCH v7 04/28] powerpc/xmon: Use bitwise calculations in_breakpoint_table()
From: Jordan Niethe @ 2020-05-05 7:31 UTC (permalink / raw)
To: Michael Ellerman
Cc: Alistair Popple, Nicholas Piggin, Balamuruhan S, naveen.n.rao,
linuxppc-dev, Daniel Axtens
In-Reply-To: <871rnyeu4t.fsf@mpe.ellerman.id.au>
On Tue, May 5, 2020 at 5:08 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Jordan Niethe <jniethe5@gmail.com> writes:
> > A modulo operation is used for calculating the current offset from a
> > breakpoint within the breakpoint table. As instruction lengths are
> > always a power of 2, this can be replaced with a bitwise 'and'. The
> > current check for word alignment can be replaced with checking that the
> > lower 2 bits are not set.
> >
> > Suggested-by: Christophe Leroy <christophe.leroy@c-s.fr>
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > v6: New to series
> > ---
> > arch/powerpc/xmon/xmon.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> > index bbfea22f4a96..e122f0c8a044 100644
> > --- a/arch/powerpc/xmon/xmon.c
> > +++ b/arch/powerpc/xmon/xmon.c
> > @@ -857,8 +857,8 @@ static struct bpt *in_breakpoint_table(unsigned long nip, unsigned long *offp)
> > off = nip - (unsigned long) bpt_table;
> > if (off >= sizeof(bpt_table))
> > return NULL;
> > - *offp = off % BPT_SIZE;
> > - if (*offp != 0 && *offp != 4)
> > + *offp = off & (BPT_SIZE - 1);
> > + if (off & 3)
> > return NULL;
>
> It would be even better if you didn't hard code the 3 wouldn't it?
>
The three is just checking word alignment, which I think was the
intention of the previous
- if (*offp != 0 && *offp != 4)
But using BPT_SIZE is is a different calculation.
BPT_SIZE == 2 * sizeof(unsigned int) == 8
Which would mean the trap of the breakpoint pair of instructions would
return NULL.
> eg:
>
> + *offp = off & (BPT_SIZE - 1);
> + if (off & (BPT_SIZE - 1))
> return NULL;
>
> cheers
^ 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