LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [RFC PATCH 1/2] libnvdimm: Add prctl control for disabling synchronous fault support.
From: Jan Kara @ 2020-06-01 10:09 UTC (permalink / raw)
  To: Aneesh Kumar K.V
  Cc: Jan Kara, linux-nvdimm, jack, Jeff Moyer, oohall, dan.j.williams,
	Michal Suchánek, linuxppc-dev
In-Reply-To: <7e8ee9e3-4d4d-e4b9-913b-1c2448adc62a@linux.ibm.com>

On Fri 29-05-20 16:25:35, Aneesh Kumar K.V wrote:
> On 5/29/20 3:22 PM, Jan Kara wrote:
> > On Fri 29-05-20 15:07:31, Aneesh Kumar K.V wrote:
> > > Thanks Michal. I also missed Jeff in this email thread.
> > 
> > And I think you'll also need some of the sched maintainers for the prctl
> > bits...
> > 
> > > On 5/29/20 3:03 PM, Michal Suchánek wrote:
> > > > Adding Jan
> > > > 
> > > > On Fri, May 29, 2020 at 11:11:39AM +0530, Aneesh Kumar K.V wrote:
> > > > > With POWER10, architecture is adding new pmem flush and sync instructions.
> > > > > The kernel should prevent the usage of MAP_SYNC if applications are not using
> > > > > the new instructions on newer hardware.
> > > > > 
> > > > > This patch adds a prctl option MAP_SYNC_ENABLE that can be used to enable
> > > > > the usage of MAP_SYNC. The kernel config option is added to allow the user
> > > > > to control whether MAP_SYNC should be enabled by default or not.
> > > > > 
> > > > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> > ...
> > > > > diff --git a/kernel/fork.c b/kernel/fork.c
> > > > > index 8c700f881d92..d5a9a363e81e 100644
> > > > > --- a/kernel/fork.c
> > > > > +++ b/kernel/fork.c
> > > > > @@ -963,6 +963,12 @@ __cacheline_aligned_in_smp DEFINE_SPINLOCK(mmlist_lock);
> > > > >    static unsigned long default_dump_filter = MMF_DUMP_FILTER_DEFAULT;
> > > > > +#ifdef CONFIG_ARCH_MAP_SYNC_DISABLE
> > > > > +unsigned long default_map_sync_mask = MMF_DISABLE_MAP_SYNC_MASK;
> > > > > +#else
> > > > > +unsigned long default_map_sync_mask = 0;
> > > > > +#endif
> > > > > +
> > 
> > I'm not sure CONFIG is really the right approach here. For a distro that would
> > basically mean to disable MAP_SYNC for all PPC kernels unless application
> > explicitly uses the right prctl. Shouldn't we rather initialize
> > default_map_sync_mask on boot based on whether the CPU we run on requires
> > new flush instructions or not? Otherwise the patch looks sensible.
> > 
> 
> yes that is correct. We ideally want to deny MAP_SYNC only w.r.t POWER10.
> But on a virtualized platform there is no easy way to detect that. We could
> ideally hook this into the nvdimm driver where we look at the new compat
> string ibm,persistent-memory-v2 and then disable MAP_SYNC
> if we find a device with the specific value.

Hum, couldn't we set some flag for nvdimm devices with
"ibm,persistent-memory-v2" property and then check it during mmap(2) time
and when the device has this propery and the mmap(2) caller doesn't have
the prctl set, we'd disallow MAP_SYNC? That should make things mostly
seamless, shouldn't it? Only apps that want to use MAP_SYNC on these
devices would need to use prctl(MMF_DISABLE_MAP_SYNC, 0) but then these
applications need to be aware of new instructions so this isn't that much
additional burden...

> With that I am wondering should we even have this patch? Can we expect
> userspace get updated to use new instruction?.
> 
> With ppc64 we never had a real persistent memory device available for end
> user to try. The available persistent memory stack was using vPMEM which was
> presented as a volatile memory region for which there is no need to use any
> of the flush instructions. We could safely assume that as we get
> applications certified/verified for working with pmem device on ppc64, they
> would all be using the new instructions?

This is a bit of a gamble... I don't have too much trust in certification /
verification because only the "big players" may do powerfail testing
throughout enough that they'd uncover these problems. So the question
really is: How many apps are out there using MAP_SYNC on ppc64? Hopefully
not many given the HW didn't ship yet as you wrote but I have no real clue.
Similarly there's a question: How many app writers will read manual for
older ppc64 architecture and write apps that won't work reliably on
POWER10? Again, I have no idea.

So the prctl would be IMHO a nice safety belt but I'm not 100% certain it
will be needed...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply

* Re: [RFC PATCH 1/2] libnvdimm: Add prctl control for disabling synchronous fault support.
From: Jan Kara @ 2020-06-01  9:50 UTC (permalink / raw)
  To: Dan Williams
  Cc: Jan Kara, linux-nvdimm, Aneesh Kumar K.V, jack, Jeff Moyer,
	Oliver O'Halloran, Michal Suchánek, linuxppc-dev
In-Reply-To: <CAPcyv4i7k7t8is_6FKAWbWsGHVO0kvj-OqqqJTzw=VS7xtZVvQ@mail.gmail.com>

On Sat 30-05-20 09:35:19, Dan Williams wrote:
> On Sat, May 30, 2020 at 12:18 AM Aneesh Kumar K.V
> <aneesh.kumar@linux.ibm.com> wrote:
> >
> > On 5/30/20 12:52 AM, Dan Williams wrote:
> > > On Fri, May 29, 2020 at 3:55 AM Aneesh Kumar K.V
> > > <aneesh.kumar@linux.ibm.com> wrote:
> > >>
> > >> On 5/29/20 3:22 PM, Jan Kara wrote:
> > >>> Hi!
> > >>>
> > >>> On Fri 29-05-20 15:07:31, Aneesh Kumar K.V wrote:
> > >>>> Thanks Michal. I also missed Jeff in this email thread.
> > >>>
> > >>> And I think you'll also need some of the sched maintainers for the prctl
> > >>> bits...
> > >>>
> > >>>> On 5/29/20 3:03 PM, Michal Suchánek wrote:
> > >>>>> Adding Jan
> > >>>>>
> > >>>>> On Fri, May 29, 2020 at 11:11:39AM +0530, Aneesh Kumar K.V wrote:
> > >>>>>> With POWER10, architecture is adding new pmem flush and sync instructions.
> > >>>>>> The kernel should prevent the usage of MAP_SYNC if applications are not using
> > >>>>>> the new instructions on newer hardware.
> > >>>>>>
> > >>>>>> This patch adds a prctl option MAP_SYNC_ENABLE that can be used to enable
> > >>>>>> the usage of MAP_SYNC. The kernel config option is added to allow the user
> > >>>>>> to control whether MAP_SYNC should be enabled by default or not.
> > >>>>>>
> > >>>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> > >>> ...
> > >>>>>> diff --git a/kernel/fork.c b/kernel/fork.c
> > >>>>>> index 8c700f881d92..d5a9a363e81e 100644
> > >>>>>> --- a/kernel/fork.c
> > >>>>>> +++ b/kernel/fork.c
> > >>>>>> @@ -963,6 +963,12 @@ __cacheline_aligned_in_smp DEFINE_SPINLOCK(mmlist_lock);
> > >>>>>>     static unsigned long default_dump_filter = MMF_DUMP_FILTER_DEFAULT;
> > >>>>>> +#ifdef CONFIG_ARCH_MAP_SYNC_DISABLE
> > >>>>>> +unsigned long default_map_sync_mask = MMF_DISABLE_MAP_SYNC_MASK;
> > >>>>>> +#else
> > >>>>>> +unsigned long default_map_sync_mask = 0;
> > >>>>>> +#endif
> > >>>>>> +
> > >>>
> > >>> I'm not sure CONFIG is really the right approach here. For a distro that would
> > >>> basically mean to disable MAP_SYNC for all PPC kernels unless application
> > >>> explicitly uses the right prctl. Shouldn't we rather initialize
> > >>> default_map_sync_mask on boot based on whether the CPU we run on requires
> > >>> new flush instructions or not? Otherwise the patch looks sensible.
> > >>>
> > >>
> > >> yes that is correct. We ideally want to deny MAP_SYNC only w.r.t
> > >> POWER10. But on a virtualized platform there is no easy way to detect
> > >> that. We could ideally hook this into the nvdimm driver where we look at
> > >> the new compat string ibm,persistent-memory-v2 and then disable MAP_SYNC
> > >> if we find a device with the specific value.
> > >>
> > >> BTW with the recent changes I posted for the nvdimm driver, older kernel
> > >> won't initialize persistent memory device on newer hardware. Newer
> > >> hardware will present the device to OS with a different device tree
> > >> compat string.
> > >>
> > >> My expectation  w.r.t this patch was, Distro would want to  mark
> > >> CONFIG_ARCH_MAP_SYNC_DISABLE=n based on the different application
> > >> certification.  Otherwise application will have to end up calling the
> > >> prctl(MMF_DISABLE_MAP_SYNC, 0) any way. If that is the case, should this
> > >> be dependent on P10?
> > >>
> > >> With that I am wondering should we even have this patch? Can we expect
> > >> userspace get updated to use new instruction?.
> > >>
> > >> With ppc64 we never had a real persistent memory device available for
> > >> end user to try. The available persistent memory stack was using vPMEM
> > >> which was presented as a volatile memory region for which there is no
> > >> need to use any of the flush instructions. We could safely assume that
> > >> as we get applications certified/verified for working with pmem device
> > >> on ppc64, they would all be using the new instructions?
> > >
> > > I think prctl is the wrong interface for this. I was thinking a sysfs
> > > interface along the same lines as /sys/block/pmemX/dax/write_cache.
> > > That attribute is toggling DAXDEV_WRITE_CACHE for the determination of
> > > whether the platform or the kernel needs to handle cache flushing
> > > relative to power loss. A similar attribute can be established for
> > > DAXDEV_SYNC, it would simply default to off based on a configuration
> > > time policy, but be dynamically changeable at runtime via sysfs.
> > >
> > > These flags are device properties that affect the kernel and
> > > userspace's handling of persistence.
> > >
> >
> > That will not handle the scenario with multiple applications using the
> > same fsdax mount point where one is updated to use the new instruction
> > and the other is not.
> 
> Right, it needs to be a global setting / flag day to switch from one
> regime to another. Per-process control is a recipe for disaster.

First I'd like to mention that hopefully the concern is mostly theoretical
since as Aneesh wrote above, real persistent memory never shipped for PPC
and so there are very few apps (if any) using the old way to ensure cache
flushing.

But I'd like to understand why do you think per-process control is a recipe
for disaster? Because from my POV the sysfs interface you propose is actually
difficult to use in practice. As a distributor, you have hard time picking
the default because you have a choice between picking safe option which is
going to confuse users because of failing MAP_SYNC and unsafe option where
everyone will be happy until someone looses data because of some ancient
application using wrong instructions to persist data. Poor experience for
users in either way. And when distro defaults to "safe option", then the
burden is on the sysadmin to toggle the switch but how is he supposed to
decide when that is safe? First he has to understand what the problem
actually is, then he has to audit all the applications using pmem whether
they use the new instruction - which is IMO a lot of effort if you have a
couple of applications and practically infeasible if you have more of them.
So IMO the burden should be *on the application* to declare that it is
aware of the new instructions to flush pmem on the platform and only to
such application the kernel should give the trust to use MAP_SYNC mappings.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply

* Re: [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling
From: Geert Uytterhoeven @ 2020-06-01  9:12 UTC (permalink / raw)
  To: Finn Thain
  Cc: linux-m68k, linuxppc-dev, Linux Kernel Mailing List,
	Joshua Thompson
In-Reply-To: <alpine.LNX.2.22.394.2006011006080.8@nippy.intranet>

Hi Finn,

On Mon, Jun 1, 2020 at 2:15 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> On Sun, 31 May 2020, Geert Uytterhoeven wrote:
> > On Sun, May 31, 2020 at 1:20 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> > >  arch/m68k/include/asm/adb_iop.h |  1 +
> > >  drivers/macintosh/adb-iop.c     | 32 ++++++++++++++++++++++++++------
> >
> > As this header file is used by a single source file only, perhaps it
> > should just be absorbed by the latter?
>
> Sure, it could be absorbed by both asm/mac_iop.h and
> drivers/macintosh/adb-iop.c but I don't see the point...

asm/mac_iop.h doesn't include asm/adb_iop.h (at least not in my tree,
but perhaps you have plans to change that?), so there's only a single
user.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply

* Re: [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling
From: Geert Uytterhoeven @ 2020-06-01  9:10 UTC (permalink / raw)
  To: Brad Boyer; +Cc: linux-m68k, linuxppc-dev, Joshua Thompson, Finn Thain
In-Reply-To: <20200531200140.GA27809@allandria.com>

Hi Brad,

On Sun, May 31, 2020 at 10:01 PM Brad Boyer <flar@allandria.com> wrote:
> On Sun, May 31, 2020 at 10:38:04AM +0200, Geert Uytterhoeven wrote:
> > >  arch/m68k/include/asm/adb_iop.h |  1 +
> > As this header file is used by a single source file only, perhaps it should
> > just be absorbed by the latter?
> > Then you no longer need my Acked-by for future changes ;-)
>
> While I don't really feel involved in this specific change (although I
> was one of the testers when the driver was first written), I am a little
> curious about the current coding standards. This header is pretty much
> just a declaration of the hardware interface, of which there are many
> in this directory. Is it better to just define all the offsets and bits
> for hardware registers inside the driver? We used to always put them in
> headers like this, but is that not the current standard? Would it be
> cleaner to put such headers in the directory with the driver effectively
> making them private? I seem to see quite a bit of that as well.

The idea behind header files is to share definitions among multipe
source files, right? It seems there is only one user of this header
file, so no sharing is involved, and thus these definitions are
de-facto private to the driver.  Hence, the hardware declarations
could be absorbed by the driver source file.

In this case having two separate files makes maintenance more
difficult, as the two files belong to different maintainer spaces
(drivers/macintosh/ and arch/m68k/).

In general, moving header files to the driver directory is an option,
if nothing outside the driver directory needs it.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply

* Re: [PATCH v5] powerpc/irq: inline call_do_irq() and call_do_softirq() on PPC32
From: Christophe Leroy @ 2020-06-01  7:26 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: Paul Mackerras, linuxppc-dev, linux-kernel
In-Reply-To: <72a6cd86137b2a7ab835213cf5c74df6ed2f6ea7.1575739197.git.christophe.leroy@c-s.fr>

Hi Michael,

Le 07/12/2019 à 18:20, Christophe Leroy a écrit :
> call_do_irq() and call_do_softirq() are simple enough to be
> worth inlining.
> 
> Inlining them avoids an mflr/mtlr pair plus a save/reload on stack.
> It also allows GCC to keep the saved ksp_limit in an nonvolatile reg.
> 
> This is inspired from S390 arch. Several other arches do more or
> less the same. The way sparc arch does seems odd thought.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> Reviewed-by: Segher Boessenkool <segher@kernel.crashing.org>

Anything more I need to do for this patch to get merged ?

Thanks
Christophe


> 
> ---
> v2: no change.
> v3: no change.
> v4:
> - comment reminding the purpose of the inline asm block.
> - added r2 as clobbered reg
> v5:
> - Limiting the change to PPC32 for now.
> - removed r2 from the clobbered regs list (on PPC32 r2 points to current all the time)
> - Removed patch 1 and merged ksp_limit handling in here.
> ---
>   arch/powerpc/include/asm/irq.h |  2 ++
>   arch/powerpc/kernel/irq.c      | 48 ++++++++++++++++++++++++++++++++++++++++++
>   arch/powerpc/kernel/misc_32.S  | 39 ----------------------------------
>   3 files changed, 50 insertions(+), 39 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/irq.h b/arch/powerpc/include/asm/irq.h
> index 814dfab7e392..e4a92f0b4ad4 100644
> --- a/arch/powerpc/include/asm/irq.h
> +++ b/arch/powerpc/include/asm/irq.h
> @@ -56,8 +56,10 @@ extern void *mcheckirq_ctx[NR_CPUS];
>   extern void *hardirq_ctx[NR_CPUS];
>   extern void *softirq_ctx[NR_CPUS];
>   
> +#ifdef CONFIG_PPC64
>   void call_do_softirq(void *sp);
>   void call_do_irq(struct pt_regs *regs, void *sp);
> +#endif
>   extern void do_IRQ(struct pt_regs *regs);
>   extern void __init init_IRQ(void);
>   extern void __do_irq(struct pt_regs *regs);
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index 5645bc9cbc09..240eca12c71d 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -611,6 +611,54 @@ static inline void check_stack_overflow(void)
>   #endif
>   }
>   
> +#ifdef CONFIG_PPC32
> +static inline void call_do_softirq(const void *sp)
> +{
> +	register unsigned long ret asm("r3");
> +	unsigned long limit = current->thread.ksp_limit;
> +
> +	/* Adjust the stack limit */
> +	current->thread.ksp_limit = (unsigned long)sp;
> +
> +	/* Temporarily switch r1 to sp, call __do_softirq() then restore r1. */
> +	asm volatile(
> +		"	"PPC_STLU"	1, %2(%1);\n"
> +		"	mr		1, %1;\n"
> +		"	bl		%3;\n"
> +		"	"PPC_LL"	1, 0(1);\n" :
> +		"=r"(ret) :
> +		"b"(sp), "i"(THREAD_SIZE - STACK_FRAME_OVERHEAD), "i"(__do_softirq) :
> +		"lr", "xer", "ctr", "memory", "cr0", "cr1", "cr5", "cr6", "cr7",
> +		"r0", "r4", "r5", "r6", "r7", "r8", "r9", "r10", "r11", "r12");
> +
> +	/* Restore stack limit */
> +	current->thread.ksp_limit = limit;
> +}
> +
> +static inline void call_do_irq(struct pt_regs *regs, void *sp)
> +{
> +	register unsigned long r3 asm("r3") = (unsigned long)regs;
> +	unsigned long limit = current->thread.ksp_limit;
> +
> +	/* Adjust the stack limit */
> +	current->thread.ksp_limit = (unsigned long)sp;
> +
> +	/* Temporarily switch r1 to sp, call __do_irq() then restore r1 */
> +	asm volatile(
> +		"	"PPC_STLU"	1, %2(%1);\n"
> +		"	mr		1, %1;\n"
> +		"	bl		%3;\n"
> +		"	"PPC_LL"	1, 0(1);\n" :
> +		"+r"(r3) :
> +		"b"(sp), "i"(THREAD_SIZE - STACK_FRAME_OVERHEAD), "i"(__do_irq) :
> +		"lr", "xer", "ctr", "memory", "cr0", "cr1", "cr5", "cr6", "cr7",
> +		"r0", "r4", "r5", "r6", "r7", "r8", "r9", "r10", "r11", "r12");
> +
> +	/* Restore stack limit */
> +	current->thread.ksp_limit = limit;
> +}
> +#endif
> +
>   void __do_irq(struct pt_regs *regs)
>   {
>   	unsigned int irq;
> diff --git a/arch/powerpc/kernel/misc_32.S b/arch/powerpc/kernel/misc_32.S
> index d80212be8698..341a3cd199cb 100644
> --- a/arch/powerpc/kernel/misc_32.S
> +++ b/arch/powerpc/kernel/misc_32.S
> @@ -28,45 +28,6 @@
>   	.text
>   
>   /*
> - * We store the saved ksp_limit in the unused part
> - * of the STACK_FRAME_OVERHEAD
> - */
> -_GLOBAL(call_do_softirq)
> -	mflr	r0
> -	stw	r0,4(r1)
> -	lwz	r10,THREAD+KSP_LIMIT(r2)
> -	stw	r3, THREAD+KSP_LIMIT(r2)
> -	stwu	r1,THREAD_SIZE-STACK_FRAME_OVERHEAD(r3)
> -	mr	r1,r3
> -	stw	r10,8(r1)
> -	bl	__do_softirq
> -	lwz	r10,8(r1)
> -	lwz	r1,0(r1)
> -	lwz	r0,4(r1)
> -	stw	r10,THREAD+KSP_LIMIT(r2)
> -	mtlr	r0
> -	blr
> -
> -/*
> - * void call_do_irq(struct pt_regs *regs, void *sp);
> - */
> -_GLOBAL(call_do_irq)
> -	mflr	r0
> -	stw	r0,4(r1)
> -	lwz	r10,THREAD+KSP_LIMIT(r2)
> -	stw	r4, THREAD+KSP_LIMIT(r2)
> -	stwu	r1,THREAD_SIZE-STACK_FRAME_OVERHEAD(r4)
> -	mr	r1,r4
> -	stw	r10,8(r1)
> -	bl	__do_irq
> -	lwz	r10,8(r1)
> -	lwz	r1,0(r1)
> -	lwz	r0,4(r1)
> -	stw	r10,THREAD+KSP_LIMIT(r2)
> -	mtlr	r0
> -	blr
> -
> -/*
>    * This returns the high 64 bits of the product of two 64-bit numbers.
>    */
>   _GLOBAL(mulhdu)
> 

^ permalink raw reply

* [powerpc:next-test] BUILD SUCCESS e376ca093587eafd840bb0f9df04090e2a54249c
From: kbuild test robot @ 2020-06-01  5:19 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: linuxppc-dev

tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git  next-test
branch HEAD: e376ca093587eafd840bb0f9df04090e2a54249c  powerpc/module_64: Use special stub for _mcount() with -mprofile-kernel

Warning in current branch:

kernel/events/hw_breakpoint.c:216:12: warning: no previous prototype for 'arch_reserve_bp_slot' [-Wmissing-prototypes]
kernel/events/hw_breakpoint.c:221:13: warning: no previous prototype for 'arch_release_bp_slot' [-Wmissing-prototypes]

Warning ids grouped by kconfigs:

recent_errors
|-- arm64-allmodconfig
|   |-- kernel-events-hw_breakpoint.c:warning:no-previous-prototype-for-arch_release_bp_slot
|   `-- kernel-events-hw_breakpoint.c:warning:no-previous-prototype-for-arch_reserve_bp_slot
|-- arm64-allyesconfig
|   |-- kernel-events-hw_breakpoint.c:warning:no-previous-prototype-for-arch_release_bp_slot
|   `-- kernel-events-hw_breakpoint.c:warning:no-previous-prototype-for-arch_reserve_bp_slot
|-- arm64-defconfig
|   |-- kernel-events-hw_breakpoint.c:warning:no-previous-prototype-for-arch_release_bp_slot
|   `-- kernel-events-hw_breakpoint.c:warning:no-previous-prototype-for-arch_reserve_bp_slot
|-- i386-allnoconfig
|   |-- kernel-events-hw_breakpoint.c:warning:no-previous-prototype-for-arch_release_bp_slot
|   `-- kernel-events-hw_breakpoint.c:warning:no-previous-prototype-for-arch_reserve_bp_slot
|-- i386-randconfig-m021-20200529
|   |-- kernel-events-hw_breakpoint.c:warning:no-previous-prototype-for-arch_release_bp_slot
|   `-- kernel-events-hw_breakpoint.c:warning:no-previous-prototype-for-arch_reserve_bp_slot
|-- i386-tinyconfig
|   |-- kernel-events-hw_breakpoint.c:warning:no-previous-prototype-for-arch_release_bp_slot
|   `-- kernel-events-hw_breakpoint.c:warning:no-previous-prototype-for-arch_reserve_bp_slot
|-- sh-shmin_defconfig
|   |-- kernel-events-hw_breakpoint.c:warning:no-previous-prototype-for-arch_release_bp_slot
|   `-- kernel-events-hw_breakpoint.c:warning:no-previous-prototype-for-arch_reserve_bp_slot
|-- x86_64-defconfig
|   |-- kernel-events-hw_breakpoint.c:warning:no-previous-prototype-for-arch_release_bp_slot
|   `-- kernel-events-hw_breakpoint.c:warning:no-previous-prototype-for-arch_reserve_bp_slot
`-- x86_64-randconfig-m001-20200531
    |-- kernel-events-hw_breakpoint.c:warning:no-previous-prototype-for-arch_release_bp_slot
    `-- kernel-events-hw_breakpoint.c:warning:no-previous-prototype-for-arch_reserve_bp_slot

elapsed time: 3868m

configs tested: 143
configs skipped: 11

The following configs have been built successfully.
More configs may be tested in the coming days.

arm                                 defconfig
arm                              allyesconfig
arm                              allmodconfig
arm                               allnoconfig
arm64                            allyesconfig
arm64                               defconfig
arm64                            allmodconfig
arm64                             allnoconfig
arm                          pxa3xx_defconfig
arm                         palmz72_defconfig
sh                            shmin_defconfig
ia64                            zx1_defconfig
riscv                          rv32_defconfig
xtensa                          iss_defconfig
powerpc                      pasemi_defconfig
sparc                       sparc64_defconfig
mips                  decstation_64_defconfig
mips                          ath79_defconfig
mips                              allnoconfig
mips                        qi_lb60_defconfig
sh                            migor_defconfig
sh                     magicpanelr2_defconfig
mips                   sb1250_swarm_defconfig
powerpc                  mpc885_ads_defconfig
arm                       aspeed_g5_defconfig
mips                        maltaup_defconfig
arc                             nps_defconfig
sh                          rsk7269_defconfig
mips                             allyesconfig
ia64                        generic_defconfig
m68k                            q40_defconfig
sh                           se7724_defconfig
mips                malta_qemu_32r6_defconfig
mips                            e55_defconfig
mips                       lemote2f_defconfig
arm                         ebsa110_defconfig
arm                            lart_defconfig
sh                         microdev_defconfig
arm                          badge4_defconfig
arm                        oxnas_v6_defconfig
x86_64                              defconfig
powerpc                    adder875_defconfig
mips                            ar7_defconfig
arm                        keystone_defconfig
arm                           viper_defconfig
arm                            pleb_defconfig
sh                             espt_defconfig
arm                           omap1_defconfig
arm                       spear13xx_defconfig
sparc64                          allyesconfig
microblaze                    nommu_defconfig
arm                     davinci_all_defconfig
c6x                                 defconfig
i386                              allnoconfig
arm                            qcom_defconfig
powerpc                    amigaone_defconfig
nios2                         10m50_defconfig
arm                         s3c6400_defconfig
openrisc                            defconfig
powerpc                             defconfig
arc                           tb10x_defconfig
arm                        mvebu_v7_defconfig
powerpc                mpc7448_hpc2_defconfig
xtensa                              defconfig
s390                       zfcpdump_defconfig
s390                             allyesconfig
arm                       cns3420vb_defconfig
arm                        spear3xx_defconfig
i386                             allyesconfig
i386                                defconfig
i386                              debian-10.3
ia64                             allmodconfig
ia64                                defconfig
ia64                              allnoconfig
ia64                             allyesconfig
m68k                             allmodconfig
m68k                              allnoconfig
m68k                           sun3_defconfig
m68k                                defconfig
m68k                             allyesconfig
nios2                               defconfig
nios2                            allyesconfig
c6x                              allyesconfig
c6x                               allnoconfig
openrisc                         allyesconfig
nds32                               defconfig
nds32                             allnoconfig
csky                             allyesconfig
csky                                defconfig
alpha                               defconfig
alpha                            allyesconfig
xtensa                           allyesconfig
h8300                            allyesconfig
h8300                            allmodconfig
arc                                 defconfig
arc                              allyesconfig
sh                               allmodconfig
sh                                allnoconfig
microblaze                        allnoconfig
mips                             allmodconfig
parisc                            allnoconfig
parisc                              defconfig
parisc                           allyesconfig
parisc                           allmodconfig
powerpc                          allyesconfig
powerpc                          rhel-kconfig
powerpc                          allmodconfig
powerpc                           allnoconfig
i386                 randconfig-a004-20200529
i386                 randconfig-a001-20200529
i386                 randconfig-a002-20200529
i386                 randconfig-a006-20200529
i386                 randconfig-a003-20200529
i386                 randconfig-a005-20200529
i386                 randconfig-a013-20200529
i386                 randconfig-a011-20200529
i386                 randconfig-a012-20200529
i386                 randconfig-a015-20200529
i386                 randconfig-a016-20200529
i386                 randconfig-a014-20200529
riscv                            allyesconfig
riscv                             allnoconfig
riscv                               defconfig
riscv                            allmodconfig
s390                              allnoconfig
s390                             allmodconfig
s390                                defconfig
sparc                            allyesconfig
sparc                               defconfig
sparc64                             defconfig
sparc64                           allnoconfig
sparc64                          allmodconfig
um                               allmodconfig
um                                allnoconfig
um                                  defconfig
um                               allyesconfig
x86_64                                   rhel
x86_64                               rhel-7.6
x86_64                    rhel-7.6-kselftests
x86_64                         rhel-7.2-clear
x86_64                                    lkp
x86_64                              fedora-25
x86_64                                  kexec

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

^ permalink raw reply

* [powerpc:fixes-test] BUILD SUCCESS 2f26ed1764b42a8c40d9c48441c73a70d805decf
From: kbuild test robot @ 2020-06-01  5:14 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: linuxppc-dev

tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git  fixes-test
branch HEAD: 2f26ed1764b42a8c40d9c48441c73a70d805decf  powerpc/64s: Disable sanitisers for C syscall/interrupt entry/exit code

elapsed time: 3867m

configs tested: 186
configs skipped: 14

The following configs have been built successfully.
More configs may be tested in the coming days.

arm                                 defconfig
arm                              allyesconfig
arm                              allmodconfig
arm                               allnoconfig
arm64                            allyesconfig
arm64                               defconfig
arm64                            allmodconfig
arm64                             allnoconfig
arm                          pxa3xx_defconfig
arm                         palmz72_defconfig
sh                            shmin_defconfig
ia64                            zx1_defconfig
riscv                          rv32_defconfig
um                           x86_64_defconfig
m68k                       m5249evb_defconfig
sh                   sh7770_generic_defconfig
arm                         vf610m4_defconfig
arm                        trizeps4_defconfig
powerpc                      pasemi_defconfig
sparc                       sparc64_defconfig
mips                  decstation_64_defconfig
mips                          ath79_defconfig
mips                              allnoconfig
mips                        qi_lb60_defconfig
sh                            migor_defconfig
sh                     magicpanelr2_defconfig
mips                   sb1250_swarm_defconfig
sh                        sh7763rdp_defconfig
s390                              allnoconfig
mips                         tb0226_defconfig
m68k                          multi_defconfig
h8300                            allyesconfig
sh                             sh03_defconfig
sh                           se7343_defconfig
arm                          badge4_defconfig
sh                           se7780_defconfig
powerpc                  mpc885_ads_defconfig
arm                       aspeed_g5_defconfig
mips                        maltaup_defconfig
arc                             nps_defconfig
sh                          rsk7269_defconfig
mips                             allyesconfig
ia64                        generic_defconfig
arm                       mainstone_defconfig
arm                            hisi_defconfig
powerpc                     mpc83xx_defconfig
m68k                             allyesconfig
xtensa                           allyesconfig
sh                          sdk7780_defconfig
mips                        jmr3927_defconfig
riscv                               defconfig
sh                           cayman_defconfig
arm                         ebsa110_defconfig
arm                            lart_defconfig
sh                         microdev_defconfig
arm                        oxnas_v6_defconfig
x86_64                              defconfig
powerpc                    adder875_defconfig
mips                            ar7_defconfig
arm                        keystone_defconfig
arm                           viper_defconfig
ia64                              allnoconfig
powerpc                     pseries_defconfig
arm                            dove_defconfig
h8300                            alldefconfig
arm                            pleb_defconfig
sh                             espt_defconfig
arm                           omap1_defconfig
arm                       spear13xx_defconfig
sparc64                          allyesconfig
microblaze                    nommu_defconfig
arc                          axs101_defconfig
powerpc                       maple_defconfig
nds32                             allnoconfig
sh                               allmodconfig
arm                     davinci_all_defconfig
c6x                                 defconfig
i386                              allnoconfig
powerpc                             defconfig
arc                           tb10x_defconfig
arm                        mvebu_v7_defconfig
powerpc                mpc7448_hpc2_defconfig
xtensa                              defconfig
c6x                         dsk6455_defconfig
riscv                    nommu_virt_defconfig
sh                        apsh4ad0a_defconfig
openrisc                            defconfig
mips                          rb532_defconfig
arm                         assabet_defconfig
arc                              alldefconfig
xtensa                       common_defconfig
s390                       zfcpdump_defconfig
s390                             allyesconfig
arm                       cns3420vb_defconfig
arm                        spear3xx_defconfig
i386                             allyesconfig
i386                                defconfig
i386                              debian-10.3
ia64                             allmodconfig
ia64                                defconfig
ia64                             allyesconfig
m68k                             allmodconfig
m68k                              allnoconfig
m68k                           sun3_defconfig
m68k                                defconfig
nios2                               defconfig
nios2                            allyesconfig
c6x                              allyesconfig
c6x                               allnoconfig
openrisc                         allyesconfig
nds32                               defconfig
csky                             allyesconfig
csky                                defconfig
alpha                               defconfig
alpha                            allyesconfig
h8300                            allmodconfig
arc                                 defconfig
arc                              allyesconfig
sh                                allnoconfig
microblaze                        allnoconfig
mips                             allmodconfig
parisc                            allnoconfig
parisc                              defconfig
parisc                           allyesconfig
parisc                           allmodconfig
powerpc                          allyesconfig
powerpc                          rhel-kconfig
powerpc                          allmodconfig
powerpc                           allnoconfig
i386                 randconfig-a004-20200529
i386                 randconfig-a001-20200529
i386                 randconfig-a002-20200529
i386                 randconfig-a006-20200529
i386                 randconfig-a003-20200529
i386                 randconfig-a005-20200529
i386                 randconfig-a004-20200531
i386                 randconfig-a003-20200531
i386                 randconfig-a006-20200531
i386                 randconfig-a002-20200531
i386                 randconfig-a005-20200531
i386                 randconfig-a001-20200531
x86_64               randconfig-a002-20200529
x86_64               randconfig-a006-20200529
x86_64               randconfig-a005-20200529
x86_64               randconfig-a001-20200529
x86_64               randconfig-a004-20200529
x86_64               randconfig-a003-20200529
x86_64               randconfig-a011-20200531
x86_64               randconfig-a016-20200531
x86_64               randconfig-a012-20200531
x86_64               randconfig-a014-20200531
x86_64               randconfig-a013-20200531
x86_64               randconfig-a015-20200531
i386                 randconfig-a013-20200529
i386                 randconfig-a011-20200529
i386                 randconfig-a012-20200529
i386                 randconfig-a015-20200529
i386                 randconfig-a016-20200529
i386                 randconfig-a014-20200529
i386                 randconfig-a013-20200531
i386                 randconfig-a012-20200531
i386                 randconfig-a015-20200531
i386                 randconfig-a011-20200531
i386                 randconfig-a016-20200531
i386                 randconfig-a014-20200531
riscv                            allyesconfig
riscv                             allnoconfig
riscv                            allmodconfig
s390                             allmodconfig
s390                                defconfig
sparc                            allyesconfig
sparc                               defconfig
sparc64                             defconfig
sparc64                           allnoconfig
sparc64                          allmodconfig
um                               allmodconfig
um                                allnoconfig
um                               allyesconfig
um                                  defconfig
x86_64                                   rhel
x86_64                               rhel-7.6
x86_64                    rhel-7.6-kselftests
x86_64                         rhel-7.2-clear
x86_64                                    lkp
x86_64                              fedora-25
x86_64                                  kexec

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

^ permalink raw reply

* [powerpc:merge] BUILD SUCCESS 00ec79b0b767994422c43792d73ff1327714a73f
From: kbuild test robot @ 2020-06-01  5:14 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: linuxppc-dev

tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git  merge
branch HEAD: 00ec79b0b767994422c43792d73ff1327714a73f  Automatic merge of 'master', 'next' and 'fixes' (2020-05-29 22:32)

elapsed time: 3865m

configs tested: 134
configs skipped: 6

The following configs have been built successfully.
More configs may be tested in the coming days.

arm                                 defconfig
arm                              allyesconfig
arm                              allmodconfig
arm                               allnoconfig
arm64                            allyesconfig
arm64                               defconfig
arm64                            allmodconfig
arm64                             allnoconfig
powerpc                      pasemi_defconfig
sparc                       sparc64_defconfig
mips                  decstation_64_defconfig
mips                          ath79_defconfig
powerpc                  mpc885_ads_defconfig
arm                       aspeed_g5_defconfig
mips                        maltaup_defconfig
arc                             nps_defconfig
sh                          rsk7269_defconfig
ia64                        generic_defconfig
xtensa                           allyesconfig
sh                          sdk7780_defconfig
mips                        jmr3927_defconfig
riscv                               defconfig
sh                           cayman_defconfig
arm                         ebsa110_defconfig
arm                            lart_defconfig
sh                         microdev_defconfig
x86_64                              defconfig
arm                          badge4_defconfig
arm                        oxnas_v6_defconfig
arm                            pleb_defconfig
sh                             espt_defconfig
arm                           omap1_defconfig
arm                       spear13xx_defconfig
sparc64                          allyesconfig
microblaze                    nommu_defconfig
arm                     davinci_all_defconfig
c6x                                 defconfig
i386                              allnoconfig
riscv                          rv32_defconfig
powerpc                             defconfig
arc                           tb10x_defconfig
arm                        mvebu_v7_defconfig
powerpc                mpc7448_hpc2_defconfig
xtensa                              defconfig
s390                       zfcpdump_defconfig
s390                             allyesconfig
arm                       cns3420vb_defconfig
arm                        spear3xx_defconfig
sh                          landisk_defconfig
mips                      fuloong2e_defconfig
s390                              allnoconfig
xtensa                           alldefconfig
arm                          lpd270_defconfig
i386                             allyesconfig
i386                                defconfig
i386                              debian-10.3
ia64                             allmodconfig
ia64                                defconfig
ia64                              allnoconfig
ia64                             allyesconfig
m68k                             allmodconfig
m68k                              allnoconfig
m68k                           sun3_defconfig
m68k                                defconfig
m68k                             allyesconfig
nios2                               defconfig
nios2                            allyesconfig
openrisc                            defconfig
c6x                              allyesconfig
c6x                               allnoconfig
openrisc                         allyesconfig
csky                                defconfig
alpha                               defconfig
alpha                            allyesconfig
nds32                               defconfig
nds32                             allnoconfig
csky                             allyesconfig
h8300                            allyesconfig
h8300                            allmodconfig
arc                                 defconfig
arc                              allyesconfig
sh                               allmodconfig
sh                                allnoconfig
microblaze                        allnoconfig
mips                             allyesconfig
mips                              allnoconfig
mips                             allmodconfig
parisc                            allnoconfig
parisc                              defconfig
parisc                           allyesconfig
parisc                           allmodconfig
powerpc                          allyesconfig
powerpc                          rhel-kconfig
powerpc                          allmodconfig
powerpc                           allnoconfig
i386                 randconfig-a004-20200529
i386                 randconfig-a001-20200529
i386                 randconfig-a002-20200529
i386                 randconfig-a006-20200529
i386                 randconfig-a003-20200529
i386                 randconfig-a005-20200529
i386                 randconfig-a013-20200529
i386                 randconfig-a011-20200529
i386                 randconfig-a012-20200529
i386                 randconfig-a015-20200529
i386                 randconfig-a016-20200529
i386                 randconfig-a014-20200529
x86_64               randconfig-a002-20200529
x86_64               randconfig-a006-20200529
x86_64               randconfig-a005-20200529
x86_64               randconfig-a001-20200529
x86_64               randconfig-a004-20200529
x86_64               randconfig-a003-20200529
riscv                            allyesconfig
riscv                             allnoconfig
riscv                            allmodconfig
s390                             allmodconfig
s390                                defconfig
sparc                            allyesconfig
sparc                               defconfig
sparc64                             defconfig
sparc64                           allnoconfig
sparc64                          allmodconfig
um                                allnoconfig
um                               allyesconfig
um                                  defconfig
um                               allmodconfig
x86_64                                   rhel
x86_64                               rhel-7.6
x86_64                    rhel-7.6-kselftests
x86_64                         rhel-7.2-clear
x86_64                                    lkp
x86_64                              fedora-25
x86_64                                  kexec

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

^ permalink raw reply

* Re: [PATCH v1 4/4] KVM: PPC: Book3S HV: migrate hot plugged memory
From: kbuild test robot @ 2020-06-01  4:45 UTC (permalink / raw)
  To: Ram Pai, kvm-ppc, linuxppc-dev
  Cc: ldufour, kbuild-all, bharata, aneesh.kumar, sukadev, bauerman
In-Reply-To: <1590892071-25549-5-git-send-email-linuxram@us.ibm.com>

[-- Attachment #1: Type: text/plain, Size: 2155 bytes --]

Hi Ram,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on powerpc/next]
[also build test ERROR on v5.7]
[cannot apply to next-20200529]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Ram-Pai/Migrate-non-migrated-pages-of-a-SVM/20200601-034649
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-defconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

arch/powerpc/kvm/book3s_64_mmu_radix.c:345:6: error: no previous prototype for 'kvmppc_radix_set_pte_at' [-Werror=missing-prototypes]
345 | void kvmppc_radix_set_pte_at(struct kvm *kvm, unsigned long addr,
|      ^~~~~~~~~~~~~~~~~~~~~~~
In file included from arch/powerpc/kvm/book3s_64_mmu_radix.c:23:
>> arch/powerpc/include/asm/kvm_book3s_uvmem.h:89:12: error: 'uv_migrate_mem_slot' declared 'static' but never defined [-Werror=unused-function]
89 | static int uv_migrate_mem_slot(struct kvm *kvm,
|            ^~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

vim +89 arch/powerpc/include/asm/kvm_book3s_uvmem.h

    83	
    84	static inline void
    85	kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *free,
    86				struct kvm *kvm, bool skip_page_out,
    87				bool purge_gfn) { }
    88	
  > 89	static int uv_migrate_mem_slot(struct kvm *kvm,

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26147 bytes --]

^ permalink raw reply

* Re: [PATCH v1 4/4] KVM: PPC: Book3S HV: migrate hot plugged memory
From: kbuild test robot @ 2020-06-01  4:35 UTC (permalink / raw)
  To: Ram Pai, kvm-ppc, linuxppc-dev
  Cc: ldufour, kbuild-all, bharata, aneesh.kumar, sukadev, bauerman
In-Reply-To: <1590892071-25549-5-git-send-email-linuxram@us.ibm.com>

[-- Attachment #1: Type: text/plain, Size: 2082 bytes --]

Hi Ram,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on powerpc/next]
[also build test WARNING on v5.7]
[cannot apply to next-20200529]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Ram-Pai/Migrate-non-migrated-pages-of-a-SVM/20200601-034649
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-rhel-kconfig (attached as .config)
compiler: powerpc64le-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

arch/powerpc/kvm/book3s_hv.c:3516:5: warning: no previous prototype for 'kvmhv_p9_guest_entry' [-Wmissing-prototypes]
3516 | int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
|     ^~~~~~~~~~~~~~~~~~~~
In file included from arch/powerpc/kvm/book3s_hv.c:75:
>> arch/powerpc/include/asm/kvm_book3s_uvmem.h:89:12: warning: 'uv_migrate_mem_slot' used but never defined
89 | static int uv_migrate_mem_slot(struct kvm *kvm,
|            ^~~~~~~~~~~~~~~~~~~

vim +/uv_migrate_mem_slot +89 arch/powerpc/include/asm/kvm_book3s_uvmem.h

    83	
    84	static inline void
    85	kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *free,
    86				struct kvm *kvm, bool skip_page_out,
    87				bool purge_gfn) { }
    88	
  > 89	static int uv_migrate_mem_slot(struct kvm *kvm,

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 15580 bytes --]

^ permalink raw reply

* Re: [PATCH v1 2/4] KVM: PPC: Book3S HV: track shared GFNs of secure VMs
From: kbuild test robot @ 2020-06-01  4:12 UTC (permalink / raw)
  To: Ram Pai, kvm-ppc, linuxppc-dev
  Cc: ldufour, kbuild-all, bharata, aneesh.kumar, sukadev, bauerman
In-Reply-To: <1590892071-25549-3-git-send-email-linuxram@us.ibm.com>

[-- Attachment #1: Type: text/plain, Size: 8401 bytes --]

Hi Ram,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on powerpc/next]
[also build test WARNING on v5.7 next-20200529]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Ram-Pai/Migrate-non-migrated-pages-of-a-SVM/20200601-034649
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-allyesconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

arch/powerpc/kvm/book3s_hv_uvmem.c:158:6: warning: no previous prototype for 'kvmppc_uvmem_available' [-Wmissing-prototypes]
158 | bool kvmppc_uvmem_available(void)
|      ^~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/kvm/book3s_hv_uvmem.c:167:5: warning: no previous prototype for 'kvmppc_uvmem_slot_init' [-Wmissing-prototypes]
167 | int kvmppc_uvmem_slot_init(struct kvm *kvm, const struct kvm_memory_slot *slot)
|     ^~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/kvm/book3s_hv_uvmem.c:192:6: warning: no previous prototype for 'kvmppc_uvmem_slot_free' [-Wmissing-prototypes]
192 | void kvmppc_uvmem_slot_free(struct kvm *kvm, const struct kvm_memory_slot *slot)
|      ^~~~~~~~~~~~~~~~~~~~~~
>> arch/powerpc/kvm/book3s_hv_uvmem.c:279:6: warning: no previous prototype for 'kvmppc_gfn_is_uvmem_shared' [-Wmissing-prototypes]
279 | bool kvmppc_gfn_is_uvmem_shared(unsigned long gfn, struct kvm *kvm)
|      ^~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/kvm/book3s_hv_uvmem.c:293:15: warning: no previous prototype for 'kvmppc_h_svm_init_start' [-Wmissing-prototypes]
293 | unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
|               ^~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/kvm/book3s_hv_uvmem.c:335:15: warning: no previous prototype for 'kvmppc_h_svm_init_done' [-Wmissing-prototypes]
335 | unsigned long kvmppc_h_svm_init_done(struct kvm *kvm)
|               ^~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/kvm/book3s_hv_uvmem.c:356:6: warning: no previous prototype for 'kvmppc_uvmem_drop_pages' [-Wmissing-prototypes]
356 | void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *free,
|      ^~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/kvm/book3s_hv_uvmem.c:397:15: warning: no previous prototype for 'kvmppc_h_svm_init_abort' [-Wmissing-prototypes]
397 | unsigned long kvmppc_h_svm_init_abort(struct kvm *kvm)
|               ^~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/kvm/book3s_hv_uvmem.c:599:15: warning: no previous prototype for 'kvmppc_h_svm_page_in' [-Wmissing-prototypes]
599 | unsigned long kvmppc_h_svm_page_in(struct kvm *kvm, unsigned long gpa,
|               ^~~~~~~~~~~~~~~~~~~~
arch/powerpc/kvm/book3s_hv_uvmem.c:784:1: warning: no previous prototype for 'kvmppc_h_svm_page_out' [-Wmissing-prototypes]
784 | kvmppc_h_svm_page_out(struct kvm *kvm, unsigned long gpa,
| ^~~~~~~~~~~~~~~~~~~~~
arch/powerpc/kvm/book3s_hv_uvmem.c:822:5: warning: no previous prototype for 'kvmppc_send_page_to_uv' [-Wmissing-prototypes]
822 | int kvmppc_send_page_to_uv(struct kvm *kvm, unsigned long gfn)
|     ^~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/kvm/book3s_hv_uvmem.c:867:5: warning: no previous prototype for 'kvmppc_uvmem_init' [-Wmissing-prototypes]
867 | int kvmppc_uvmem_init(void)
|     ^~~~~~~~~~~~~~~~~
arch/powerpc/kvm/book3s_hv_uvmem.c:922:6: warning: no previous prototype for 'kvmppc_uvmem_free' [-Wmissing-prototypes]
922 | void kvmppc_uvmem_free(void)
|      ^~~~~~~~~~~~~~~~~

vim +/kvmppc_gfn_is_uvmem_shared +279 arch/powerpc/kvm/book3s_hv_uvmem.c

   157	
 > 158	bool kvmppc_uvmem_available(void)
   159	{
   160		/*
   161		 * If kvmppc_uvmem_bitmap != NULL, then there is an ultravisor
   162		 * and our data structures have been initialized successfully.
   163		 */
   164		return !!kvmppc_uvmem_bitmap;
   165	}
   166	
   167	int kvmppc_uvmem_slot_init(struct kvm *kvm, const struct kvm_memory_slot *slot)
   168	{
   169		struct kvmppc_uvmem_slot *p;
   170	
   171		p = kzalloc(sizeof(*p), GFP_KERNEL);
   172		if (!p)
   173			return -ENOMEM;
   174		p->pfns = vzalloc(array_size(slot->npages, sizeof(*p->pfns)));
   175		if (!p->pfns) {
   176			kfree(p);
   177			return -ENOMEM;
   178		}
   179		p->nr_pfns = slot->npages;
   180		p->base_pfn = slot->base_gfn;
   181	
   182		mutex_lock(&kvm->arch.uvmem_lock);
   183		list_add(&p->list, &kvm->arch.uvmem_pfns);
   184		mutex_unlock(&kvm->arch.uvmem_lock);
   185	
   186		return 0;
   187	}
   188	
   189	/*
   190	 * All device PFNs are already released by the time we come here.
   191	 */
   192	void kvmppc_uvmem_slot_free(struct kvm *kvm, const struct kvm_memory_slot *slot)
   193	{
   194		struct kvmppc_uvmem_slot *p, *next;
   195	
   196		mutex_lock(&kvm->arch.uvmem_lock);
   197		list_for_each_entry_safe(p, next, &kvm->arch.uvmem_pfns, list) {
   198			if (p->base_pfn == slot->base_gfn) {
   199				vfree(p->pfns);
   200				list_del(&p->list);
   201				kfree(p);
   202				break;
   203			}
   204		}
   205		mutex_unlock(&kvm->arch.uvmem_lock);
   206	}
   207	
   208	static void kvmppc_uvmem_pfn_insert(unsigned long gfn, unsigned long uvmem_pfn,
   209					    struct kvm *kvm)
   210	{
   211		struct kvmppc_uvmem_slot *p;
   212	
   213		list_for_each_entry(p, &kvm->arch.uvmem_pfns, list) {
   214			if (gfn >= p->base_pfn && gfn < p->base_pfn + p->nr_pfns) {
   215				unsigned long index = gfn - p->base_pfn;
   216	
   217				p->pfns[index] = uvmem_pfn | KVMPPC_UVMEM_PFN;
   218				return;
   219			}
   220		}
   221	}
   222	
   223	static void kvmppc_uvmem_pfn_remove(unsigned long gfn, struct kvm *kvm)
   224	{
   225		struct kvmppc_uvmem_slot *p;
   226	
   227		list_for_each_entry(p, &kvm->arch.uvmem_pfns, list) {
   228			if (gfn >= p->base_pfn && gfn < p->base_pfn + p->nr_pfns) {
   229				/*
   230				 * Reset everything, but keep the KVMPPC_UVMEM_SHARED
   231				 * flag intact.  A gfn continues to be shared or
   232				 * unshared, with or without an associated device pfn.
   233				 */
   234				p->pfns[gfn - p->base_pfn] &= KVMPPC_UVMEM_SHARED;
   235				return;
   236			}
   237		}
   238	}
   239	
   240	static bool kvmppc_gfn_is_uvmem_pfn(unsigned long gfn, struct kvm *kvm,
   241					    unsigned long *uvmem_pfn)
   242	{
   243		struct kvmppc_uvmem_slot *p;
   244	
   245		list_for_each_entry(p, &kvm->arch.uvmem_pfns, list) {
   246			if (gfn >= p->base_pfn && gfn < p->base_pfn + p->nr_pfns) {
   247				unsigned long index = gfn - p->base_pfn;
   248	
   249				if (p->pfns[index] & KVMPPC_UVMEM_PFN) {
   250					if (uvmem_pfn)
   251						*uvmem_pfn = p->pfns[index] &
   252							     KVMPPC_UVMEM_PFN_MASK;
   253					return true;
   254				} else
   255					return false;
   256			}
   257		}
   258		return false;
   259	}
   260	
   261	static void kvmppc_gfn_uvmem_shared(unsigned long gfn, struct kvm *kvm,
   262			bool set)
   263	{
   264		struct kvmppc_uvmem_slot *p;
   265	
   266		list_for_each_entry(p, &kvm->arch.uvmem_pfns, list) {
   267			if (gfn >= p->base_pfn && gfn < p->base_pfn + p->nr_pfns) {
   268				unsigned long index = gfn - p->base_pfn;
   269	
   270				if (set)
   271					p->pfns[index] |= KVMPPC_UVMEM_SHARED;
   272				else
   273					p->pfns[index] &= ~KVMPPC_UVMEM_SHARED;
   274				return;
   275			}
   276		}
   277	}
   278	
 > 279	bool kvmppc_gfn_is_uvmem_shared(unsigned long gfn, struct kvm *kvm)
   280	{
   281		struct kvmppc_uvmem_slot *p;
   282	
   283		list_for_each_entry(p, &kvm->arch.uvmem_pfns, list) {
   284			if (gfn >= p->base_pfn && gfn < p->base_pfn + p->nr_pfns) {
   285				unsigned long index = gfn - p->base_pfn;
   286	
   287				return (p->pfns[index] & KVMPPC_UVMEM_SHARED);
   288			}
   289		}
   290		return false;
   291	}
   292	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66040 bytes --]

^ permalink raw reply

* Re: [RFC][PATCH 0/2] Add support for using reserved memory for ima buffer pass
From: Prakhar Srivastava @ 2020-06-01  4:05 UTC (permalink / raw)
  To: Thiago Jung Bauermann
  Cc: Mark Rutland, kstewart, gregkh, bhsharma, tao.li, zohar, paulus,
	vincenzo.frascino, will, Rob Herring, nramas, frowand.list,
	masahiroy, jmorris, takahiro.akashi, linux-arm-kernel,
	catalin.marinas, serge, devicetree, pasha.tatashin, hsinyi,
	tusharsu, tglx, allison, christophe.leroy, mbrugger, balajib,
	dmitry.kasatkin, linux-kernel, linux-security-module, james.morse,
	linux-integrity, linuxppc-dev
In-Reply-To: <87v9knpa36.fsf@morokweng.localdomain>


On 5/22/20 9:08 PM, Thiago Jung Bauermann wrote:
> 
> Hello Prakhar,
> 
> Prakhar Srivastava <prsriva@linux.microsoft.com> writes:
> 
>> On 5/12/20 4:05 PM, Rob Herring wrote:
>>> On Wed, May 06, 2020 at 10:50:04PM -0700, Prakhar Srivastava wrote:
>>>> Hi Mark,
>>>
>>> Please don't top post.
>>>
>>>> This patch set currently only address the Pure DT implementation.
>>>> EFI and ACPI implementations will be posted in subsequent patchsets.
>>>>
>>>> The logs are intended to be carried over the kexec and once read the
>>>> logs are no longer needed and in prior conversation with James(
>>>> https://lore.kernel.org/linux-arm-kernel/0053eb68-0905-4679-c97a-00c5cb6f1abb@arm.com/)
>>>> the apporach of using a chosen node doesn't
>>>> support the case.
>>>>
>>>> The DT entries make the reservation permanent and thus doesnt need kernel
>>>> segments to be used for this, however using a chosen-node with
>>>> reserved memory only changes the node information but memory still is
>>>> reserved via reserved-memory section.
>>>
>>> I think Mark's point was whether it needs to be permanent. We don't
>>> hardcode the initrd address for example.
>>>
>> Thankyou for clarifying my misunderstanding, i am modelling this keeping to the
>> TPM log implementation that uses a reserved memory. I will rev up the version
>> with chosen-node support.
>> That will make the memory reservation free after use.
> 
> Nice. Do you intend to use the same property that powerpc uses
> (linux,ima-kexec-buffer)?
> 
I was naming it ima-buffer, but naming is not a huge concern.
I will use linux,ima-kexec-buffer.
>>>> On 5/5/20 2:59 AM, Mark Rutland wrote:
>>>>> 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
>>>
>>> What's IMAIMA is a LSM attempting to detect if files have been accidentally or
>> maliciously altered, both remotely and locally, it can also be used
>> to appraise a file's measurement against a "good" value stored as an extended
>> attribute, and enforce local file integrity.
>>
>> IMA also validates and measures the signers of the kernel and initrd
>> during kexec. The measurements are extended to PCR 10(configurable) and the logs
>> stored in memory, however once kexec'd the logs are lost. Kexec is used as
>> secondary boot loader in may use cases and loosing the signer
>> creates a security hole.
>>
>> This patch is an implementation to carry over the logs and making it
>> possible to remotely validate the signers of the kernel and initrd. Such a
>> support exits only in powerpc.
>> This patch makes the carry over of logs architecture independent and puts the
>> complexity in a driver.
> 
> If I'm not mistaken, the code at arch/powerpc/kexec/ima.c isn't actually
> powerpc-specific. It could be moved to an arch-independent directory and
> used by any other architecture which supports device trees.
> 
> I think that's the simplest way forward. And to be honest I'm still
> trying to understand why you didn't take that approach. Did you try it
> and hit some obstacle or noticed a disadvantage for your use case?
> 
The approach i have in this patch set is to provide an abstraction layer 
that can be called from any architecture.
However taking a deeper look only the setup dtb is probably architecture
specific, only because the architecture specific kexec sets up the 
device tree. I can also move the code up in security/ima. However i do
have some concerns with layering. I am hoping you can provide me with 
some guidance in this aspect, i will send you the patch i am working on
to get some early feedback.

Thanks,
Prakhar Srivastava


> --
> Thiago Jung Bauermann
> IBM Linux Technology Center
> 

^ permalink raw reply

* Re: [PATCH v2] selftests: powerpc: Add test for execute-disabled pkeys
From: Sandipan Das @ 2020-06-01  2:12 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: fweimer, aneesh.kumar, linuxram, linux-mm, linux-kselftest,
	linuxppc-dev, bauerman
In-Reply-To: <1eb388dc-0fde-64f3-9c05-7f9f2a398543@linux.ibm.com>

Hi Michael,

On 01/06/20 7:29 am, Sandipan Das wrote:
> Hi Michael,
> 
> Thanks for your suggestions. I had a few questions regarding some
> of them.
> 
> On 29/05/20 7:18 am, Michael Ellerman wrote:
>>> [...]
>>> +
>>> +static void pkeyreg_set(unsigned long uamr)
>>> +{
>>> +	asm volatile("isync; mtspr	0xd, %0; isync;" : : "r"(uamr));
>>> +}
>>
>> You can use mtspr() there, but you'll need to add the isync's yourself.
>>
> 
> Would it make sense to add a new macro that adds the CSI instructions?
> Something like this.
> 
> diff --git a/tools/testing/selftests/powerpc/include/reg.h b/tools/testing/selftests/powerpc/include/reg.h
> index 022c5076b2c5..d60f66380cad 100644
> --- a/tools/testing/selftests/powerpc/include/reg.h
> +++ b/tools/testing/selftests/powerpc/include/reg.h
> @@ -15,6 +15,10 @@
>  #define mtspr(rn, v)   asm volatile("mtspr " _str(rn) ",%0" : \
>                                     : "r" ((unsigned long)(v)) \
>                                     : "memory")
> +#define mtspr_csi(rn, v)       ({ \
> +                       asm volatile("isync; mtspr " _str(rn) ",%0; isync;" : \
> +                                   : "r" ((unsigned long)(v)) \
> +                                   : "memory"); })
>  
>  #define mb()           asm volatile("sync" : : : "memory");
>  #define barrier()      asm volatile("" : : : "memory");
> 
> 
> I also noticed that two of the ptrace-related pkey tests were also not
> using CSIs. I will fix those too.
> 
>>> [...]
>>> +	/* The following two cases will avoid SEGV_PKUERR */
>>> +	ftype = -1;
>>> +	fpkey = -1;
>>> +
>>> +	/*
>>> +	 * Read an instruction word from the address when AMR bits
>>> +	 * are not set.
>>
>> You should explain for people who aren't familiar with the ISA that "AMR
>> bits not set" means "read/write access allowed".
>>
>>> +	 *
>>> +	 * This should not generate a fault as having PROT_EXEC
>>> +	 * implicitly allows reads. The pkey currently restricts
>>
>> Whether PROT_EXEC implies read is not well defined (see the man page).
>> If you want to test this case I think you'd be better off specifying
>> PROT_EXEC | PROT_READ explicitly.
>>
> 
> But I guess specifying PROT_EXEC | PROT_READ defeats the purpose? I can
> tweak the passing condition though based on whether READ_IMPLIES_EXEC is
> set in the personality.
> 

Sorry, I read this the other way round. This won't work.

>> [...]
>>> +	FAIL_IF(faults != 0 || fcode != SEGV_ACCERR);
>>> +
>>> +	/* The following three cases will generate SEGV_PKUERR */
>>> +	ftype = PKEY_DISABLE_ACCESS;
>>> +	fpkey = pkey;
>>> +
>>> +	/*
>>> +	 * Read an instruction word from the address when AMR bits
>>> +	 * are set.
>>> +	 *
>>> +	 * This should generate a pkey fault based on AMR bits only
>>> +	 * as having PROT_EXEC implicitly allows reads.
>>
>> Again would be better to specify PROT_READ IMHO.
>>
> 
> I can use a personality check here too.

Same here.

> 
>>> +	 */
>>> +	faults = 1;
>>> +	FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
>>> +	printf("read from %p, pkey is execute-disabled, access-disabled\n",
>>> +	       (void *) faddr);
>>> +	pkey_set_rights(pkey, PKEY_DISABLE_ACCESS);
>>> +	i = *faddr;
>>> +	FAIL_IF(faults != 0 || fcode != SEGV_PKUERR);
>>> +
>>> +	/*
>>> +	 * Write an instruction word to the address when AMR bits
>>> +	 * are set.
>>> +	 *
>>> +	 * This should generate two faults. First, a pkey fault based
>>> +	 * on AMR bits and then an access fault based on PROT_EXEC.
>>> +	 */
>>> +	faults = 2;
>>
>> Setting faults to the expected value and decrementing it in the fault
>> handler is kind of weird.
>>
>> I think it would be clearer if faults was just a zero-based counter of
>> how many faults we've taken, and then you test that it's == 2 below.
>>
>>> +	FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
>>> +	printf("write to %p, pkey is execute-disabled, access-disabled\n",
>>> +	       (void *) faddr);
>>> +	pkey_set_rights(pkey, PKEY_DISABLE_ACCESS);
>>> +	*faddr = 0x60000000;	/* nop */
>>> +	FAIL_IF(faults != 0 || fcode != SEGV_ACCERR);
>>
>> ie. FAIL_IF(faults != 2 || ... )
>>
> 
> Agreed, it is weird. IIRC, I did this to make sure that if the test
> kept getting repeated faults at the same address and exceeded the
> maximum number of expected faults i.e. it gets another fault when
> 'faults' is already zero, then the signal handler will attempt to
> let the program continue by giving all permissions to the page and
> also the pkey. Would it make sense to just rename 'faults' to
> something like 'remaining_faults'?
> 
> 
> - Sandipan
> 

^ permalink raw reply

* Re: [PATCH v2] selftests: powerpc: Add test for execute-disabled pkeys
From: Sandipan Das @ 2020-06-01  1:59 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: fweimer, aneesh.kumar, linuxram, linux-mm, linux-kselftest,
	linuxppc-dev, bauerman
In-Reply-To: <87tuzzik8q.fsf@mpe.ellerman.id.au>

Hi Michael,

Thanks for your suggestions. I had a few questions regarding some
of them.

On 29/05/20 7:18 am, Michael Ellerman wrote:
>> [...]
>> +
>> +static void pkeyreg_set(unsigned long uamr)
>> +{
>> +	asm volatile("isync; mtspr	0xd, %0; isync;" : : "r"(uamr));
>> +}
> 
> You can use mtspr() there, but you'll need to add the isync's yourself.
> 

Would it make sense to add a new macro that adds the CSI instructions?
Something like this.

diff --git a/tools/testing/selftests/powerpc/include/reg.h b/tools/testing/selftests/powerpc/include/reg.h
index 022c5076b2c5..d60f66380cad 100644
--- a/tools/testing/selftests/powerpc/include/reg.h
+++ b/tools/testing/selftests/powerpc/include/reg.h
@@ -15,6 +15,10 @@
 #define mtspr(rn, v)   asm volatile("mtspr " _str(rn) ",%0" : \
                                    : "r" ((unsigned long)(v)) \
                                    : "memory")
+#define mtspr_csi(rn, v)       ({ \
+                       asm volatile("isync; mtspr " _str(rn) ",%0; isync;" : \
+                                   : "r" ((unsigned long)(v)) \
+                                   : "memory"); })
 
 #define mb()           asm volatile("sync" : : : "memory");
 #define barrier()      asm volatile("" : : : "memory");


I also noticed that two of the ptrace-related pkey tests were also not
using CSIs. I will fix those too.

>> [...]
>> +	/* The following two cases will avoid SEGV_PKUERR */
>> +	ftype = -1;
>> +	fpkey = -1;
>> +
>> +	/*
>> +	 * Read an instruction word from the address when AMR bits
>> +	 * are not set.
> 
> You should explain for people who aren't familiar with the ISA that "AMR
> bits not set" means "read/write access allowed".
> 
>> +	 *
>> +	 * This should not generate a fault as having PROT_EXEC
>> +	 * implicitly allows reads. The pkey currently restricts
> 
> Whether PROT_EXEC implies read is not well defined (see the man page).
> If you want to test this case I think you'd be better off specifying
> PROT_EXEC | PROT_READ explicitly.
> 

But I guess specifying PROT_EXEC | PROT_READ defeats the purpose? I can
tweak the passing condition though based on whether READ_IMPLIES_EXEC is
set in the personality.

> [...]
>> +	FAIL_IF(faults != 0 || fcode != SEGV_ACCERR);
>> +
>> +	/* The following three cases will generate SEGV_PKUERR */
>> +	ftype = PKEY_DISABLE_ACCESS;
>> +	fpkey = pkey;
>> +
>> +	/*
>> +	 * Read an instruction word from the address when AMR bits
>> +	 * are set.
>> +	 *
>> +	 * This should generate a pkey fault based on AMR bits only
>> +	 * as having PROT_EXEC implicitly allows reads.
> 
> Again would be better to specify PROT_READ IMHO.
> 

I can use a personality check here too.

>> +	 */
>> +	faults = 1;
>> +	FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
>> +	printf("read from %p, pkey is execute-disabled, access-disabled\n",
>> +	       (void *) faddr);
>> +	pkey_set_rights(pkey, PKEY_DISABLE_ACCESS);
>> +	i = *faddr;
>> +	FAIL_IF(faults != 0 || fcode != SEGV_PKUERR);
>> +
>> +	/*
>> +	 * Write an instruction word to the address when AMR bits
>> +	 * are set.
>> +	 *
>> +	 * This should generate two faults. First, a pkey fault based
>> +	 * on AMR bits and then an access fault based on PROT_EXEC.
>> +	 */
>> +	faults = 2;
> 
> Setting faults to the expected value and decrementing it in the fault
> handler is kind of weird.
> 
> I think it would be clearer if faults was just a zero-based counter of
> how many faults we've taken, and then you test that it's == 2 below.
> 
>> +	FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
>> +	printf("write to %p, pkey is execute-disabled, access-disabled\n",
>> +	       (void *) faddr);
>> +	pkey_set_rights(pkey, PKEY_DISABLE_ACCESS);
>> +	*faddr = 0x60000000;	/* nop */
>> +	FAIL_IF(faults != 0 || fcode != SEGV_ACCERR);
> 
> ie. FAIL_IF(faults != 2 || ... )
> 

Agreed, it is weird. IIRC, I did this to make sure that if the test
kept getting repeated faults at the same address and exceeded the
maximum number of expected faults i.e. it gets another fault when
'faults' is already zero, then the signal handler will attempt to
let the program continue by giving all permissions to the page and
also the pkey. Would it make sense to just rename 'faults' to
something like 'remaining_faults'?


- Sandipan

^ permalink raw reply related

* Re: [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling
From: Finn Thain @ 2020-06-01  0:14 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: linux-m68k, linuxppc-dev, Linux Kernel Mailing List,
	Joshua Thompson
In-Reply-To: <CAMuHMdUjrFDob01EWC4e04tAkj5JTm_2Ei5WsPqN1eGDz=x3+Q@mail.gmail.com>

On Sun, 31 May 2020, Geert Uytterhoeven wrote:

> Hi Finn,
> 
> On Sun, May 31, 2020 at 1:20 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> > The adb_driver.autopoll method is needed during ADB bus scan and device
> > address assignment. Implement this method so that the IOP's list of
> > device addresses can be updated. When the list is empty, disable SRQ
> > autopolling.
> >
> > Cc: Joshua Thompson <funaho@jurai.org>
> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > Tested-by: Stan Johnson <userm57@yahoo.com>
> > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> 
> Thanks for your patch!
> 
> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
> 

Thanks for your tag.

> >  arch/m68k/include/asm/adb_iop.h |  1 +
> >  drivers/macintosh/adb-iop.c     | 32 ++++++++++++++++++++++++++------
> 
> As this header file is used by a single source file only, perhaps it 
> should just be absorbed by the latter?

Sure, it could be absorbed by both asm/mac_iop.h and 
drivers/macintosh/adb-iop.c but I don't see the point...

> Then you no longer need my Acked-by for future changes ;-)
> 

But you shouldn't need to ack this kind of change in the first place.

IMHO, the arch/m68k/mac maintainer should be the one to ack changes to 
both arch/m68k/include/asm/adb_iop.h and drivers/macintosh/adb-iop.c.

Not that I'm complaining (thanks again for your ack!)

^ permalink raw reply

* Re: [musl] Re: ppc64le and 32-bit LE userland compatibility
From: Daniel Kolesa @ 2020-05-31 22:29 UTC (permalink / raw)
  To: Segher Boessenkool, musl, Will Springer
  Cc: eery, libc-alpha, linuxppc-dev, Palmer Dabbelt via binutils,
	libc-dev
In-Reply-To: <20200531204205.GI31009@gate.crashing.org>

On Sun, May 31, 2020, at 22:42, Segher Boessenkool wrote:
> On Sun, May 31, 2020 at 12:57:12AM +0000, Will Springer wrote:
> > On Saturday, May 30, 2020 12:22:12 PM PDT Segher Boessenkool wrote:
> > > The original sysv PowerPC supplement
> > > http://refspecs.linux-foundation.org/elf/elfspec_ppc.pdf
> > > supports LE as well, and most powerpcle ports use that.  But, the
> > > big-endian Linux ABI differs in quite a few places, and it of course
> > > makes a lot better sense if powerpcle-linux follows that.
> > 
> > Right, I should have clarified I was talking about Linux ABIs 
> > specifically.
> 
> That was the link you deleted.
> 
> > > What patches did you need?  I regularly build >30 cross compilers (on
> > > both BE and LE hosts; I haven't used 32-bit hosts for a long time, but
> > > in the past those worked fine as well).  I also cross-built
> > > powerpcle-linux-gcc quite a few times (from powerpc64le, from powerpc64,
> > > from various x86).
> > 
> > There was just an assumption that LE == powerpc64le in libgo, spotted by 
> > q66 (daniel@ on the CC). I just pushed the patch to [1].
> 
> Please send GCC patches to gcc-patches@ ?

FWIW, that patch alone is not very useful, we'd need to otherwise patch libgo to recognize a new GOARCH (as right now it's likely to just use 'ppc' which is wrong).

That said, we'll get back to you with any patches we have. One I can already think of - we will need to update the dynamic linker name so that it uses ld-musl-powerpcle.so instead of powerpc (musl needs to be updated the same way by adding the subarch variable for the 'le' prefix).

> 
> > > Almost no project that used 32-bit PowerPC in LE mode has sent patches
> > > to the upstreams.
> > 
> > Right, but I have heard concerns from at least one person familiar with 
> > the ppc kernel about breaking existing users of this arch-endianness 
> > combo, if any. It seems likely that none of those use upstream, though ^^;
> 
> So we don't care, because we *cannot* care.

Well, that's the reason this thread was opened in the first place - to call out to any potential users, and synchronize with upstreams on a single way forward that all upstreams can agree on, since this effort requires changes in various parts of the stack. We don't want to hog changes locally or otherwise do any changes that would be in conflict with upstream projects, as that would mean needlessly diverging, which only means trouble later on.

> 
> > > A huge factor in having good GCC support for powerpcle-linux (or
> > > anything else) is someone needs to regularly test it, and share test
> > > results with us (via gcc-testresults@).  Hint hint hint :-)
> > > 
> > > That way we know it is in good shape, know when we are regressing it,
> > > know there is interest in it.
> > 
> > Once I have more of a bootstrapped userland than a barely-functional 
> > cross chroot, I'll get back to you on that :)
> 
> Cool!  Looking forward to it.
> 
> Thanks,

Either way, thanks for the hints so far.

> 
> 
> Segher
>

Daniel (q66)

^ permalink raw reply

* Re: [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling
From: Brad Boyer @ 2020-05-31 20:01 UTC (permalink / raw)
  To: Geert Uytterhoeven; +Cc: linux-m68k, linuxppc-dev, Joshua Thompson, Finn Thain
In-Reply-To: <CAMuHMdUjrFDob01EWC4e04tAkj5JTm_2Ei5WsPqN1eGDz=x3+Q@mail.gmail.com>

On Sun, May 31, 2020 at 10:38:04AM +0200, Geert Uytterhoeven wrote:
> >  arch/m68k/include/asm/adb_iop.h |  1 +
> 
> As this header file is used by a single source file only, perhaps it should
> just be absorbed by the latter?
> Then you no longer need my Acked-by for future changes ;-)

While I don't really feel involved in this specific change (although I
was one of the testers when the driver was first written), I am a little
curious about the current coding standards. This header is pretty much
just a declaration of the hardware interface, of which there are many
in this directory. Is it better to just define all the offsets and bits
for hardware registers inside the driver? We used to always put them in
headers like this, but is that not the current standard? Would it be
cleaner to put such headers in the directory with the driver effectively
making them private? I seem to see quite a bit of that as well.

Thank you for your advice.

	Brad Boyer
	flar@allandria.com


^ permalink raw reply

* Re: ppc64le and 32-bit LE userland compatibility
From: Segher Boessenkool @ 2020-05-31 20:42 UTC (permalink / raw)
  To: Will Springer
  Cc: libc-alpha, eery, daniel, musl, binutils, libc-dev, linuxppc-dev
In-Reply-To: <2956705.fEcJ0Lxnt5@sheen>

On Sun, May 31, 2020 at 12:57:12AM +0000, Will Springer wrote:
> On Saturday, May 30, 2020 12:22:12 PM PDT Segher Boessenkool wrote:
> > The original sysv PowerPC supplement
> > http://refspecs.linux-foundation.org/elf/elfspec_ppc.pdf
> > supports LE as well, and most powerpcle ports use that.  But, the
> > big-endian Linux ABI differs in quite a few places, and it of course
> > makes a lot better sense if powerpcle-linux follows that.
> 
> Right, I should have clarified I was talking about Linux ABIs 
> specifically.

That was the link you deleted.

> > What patches did you need?  I regularly build >30 cross compilers (on
> > both BE and LE hosts; I haven't used 32-bit hosts for a long time, but
> > in the past those worked fine as well).  I also cross-built
> > powerpcle-linux-gcc quite a few times (from powerpc64le, from powerpc64,
> > from various x86).
> 
> There was just an assumption that LE == powerpc64le in libgo, spotted by 
> q66 (daniel@ on the CC). I just pushed the patch to [1].

Please send GCC patches to gcc-patches@ ?

> > Almost no project that used 32-bit PowerPC in LE mode has sent patches
> > to the upstreams.
> 
> Right, but I have heard concerns from at least one person familiar with 
> the ppc kernel about breaking existing users of this arch-endianness 
> combo, if any. It seems likely that none of those use upstream, though ^^;

So we don't care, because we *cannot* care.

> > A huge factor in having good GCC support for powerpcle-linux (or
> > anything else) is someone needs to regularly test it, and share test
> > results with us (via gcc-testresults@).  Hint hint hint :-)
> > 
> > That way we know it is in good shape, know when we are regressing it,
> > know there is interest in it.
> 
> Once I have more of a bootstrapped userland than a barely-functional 
> cross chroot, I'll get back to you on that :)

Cool!  Looking forward to it.

Thanks,


Segher

^ permalink raw reply

* Re: [PATCH v4 5/8] powerpc/pmem/of_pmem: Update of_pmem to use the new barrier instruction.
From: kbuild test robot @ 2020-05-31 14:46 UTC (permalink / raw)
  To: Aneesh Kumar K.V, linuxppc-dev, mpe, linux-nvdimm
  Cc: Aneesh Kumar K.V, dan.j.williams, kbuild-all, oohall
In-Reply-To: <20200529052820.151651-6-aneesh.kumar@linux.ibm.com>

[-- Attachment #1: Type: text/plain, Size: 4053 bytes --]

Hi "Aneesh,

I love your patch! Yet something to improve:

[auto build test ERROR on powerpc/next]
[also build test ERROR on linux-nvdimm/libnvdimm-for-next scottwood/next v5.7-rc7]
[cannot apply to next-20200529]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Aneesh-Kumar-K-V/Support-new-pmem-flush-and-sync-instructions-for-POWER/20200531-062841
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-ppc6xx_defconfig (attached as .config)
compiler: powerpc-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

In file included from arch/powerpc/include/asm/asm-prototypes.h:12,
from arch/powerpc/kernel/early_32.c:11:
arch/powerpc/include/asm/cacheflush.h: In function 'arch_pmem_flush_barrier':
arch/powerpc/include/asm/cacheflush.h:126:6: error: implicit declaration of function 'cpu_has_feature'; did you mean 'mmu_has_feature'? [-Werror=implicit-function-declaration]
126 |  if (cpu_has_feature(CPU_FTR_ARCH_207S))
|      ^~~~~~~~~~~~~~~
|      mmu_has_feature
In file included from arch/powerpc/include/asm/cputhreads.h:7,
from arch/powerpc/include/asm/mmu_context.h:12,
from arch/powerpc/include/asm/asm-prototypes.h:17,
from arch/powerpc/kernel/early_32.c:11:
arch/powerpc/include/asm/cpu_has_feature.h: At top level:
>> arch/powerpc/include/asm/cpu_has_feature.h:49:20: error: conflicting types for 'cpu_has_feature'
49 | static inline bool cpu_has_feature(unsigned long feature)
|                    ^~~~~~~~~~~~~~~
In file included from arch/powerpc/include/asm/asm-prototypes.h:12,
from arch/powerpc/kernel/early_32.c:11:
arch/powerpc/include/asm/cacheflush.h:126:6: note: previous implicit declaration of 'cpu_has_feature' was here
126 |  if (cpu_has_feature(CPU_FTR_ARCH_207S))
|      ^~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

vim +/cpu_has_feature +49 arch/powerpc/include/asm/cpu_has_feature.h

c812c7d8f1470a Aneesh Kumar K.V 2016-07-23  38  
4db7327194dba0 Kevin Hao        2016-07-23  39  	if (CPU_FTRS_ALWAYS & feature)
4db7327194dba0 Kevin Hao        2016-07-23  40  		return true;
4db7327194dba0 Kevin Hao        2016-07-23  41  
4db7327194dba0 Kevin Hao        2016-07-23  42  	if (!(CPU_FTRS_POSSIBLE & feature))
4db7327194dba0 Kevin Hao        2016-07-23  43  		return false;
4db7327194dba0 Kevin Hao        2016-07-23  44  
4db7327194dba0 Kevin Hao        2016-07-23  45  	i = __builtin_ctzl(feature);
4db7327194dba0 Kevin Hao        2016-07-23  46  	return static_branch_likely(&cpu_feature_keys[i]);
4db7327194dba0 Kevin Hao        2016-07-23  47  }
4db7327194dba0 Kevin Hao        2016-07-23  48  #else
b92a226e528423 Kevin Hao        2016-07-23 @49  static inline bool cpu_has_feature(unsigned long feature)
b92a226e528423 Kevin Hao        2016-07-23  50  {
b92a226e528423 Kevin Hao        2016-07-23  51  	return early_cpu_has_feature(feature);
b92a226e528423 Kevin Hao        2016-07-23  52  }
4db7327194dba0 Kevin Hao        2016-07-23  53  #endif
b92a226e528423 Kevin Hao        2016-07-23  54  

:::::: The code at line 49 was first introduced by commit
:::::: b92a226e528423b8d249dd09bb450d53361fbfcb powerpc: Move cpu_has_feature() to a separate file

:::::: TO: Kevin Hao <haokexin@gmail.com>
:::::: CC: Michael Ellerman <mpe@ellerman.id.au>

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31557 bytes --]

^ permalink raw reply

* Re: [PATCH 8/8] macintosh/adb-iop: Implement SRQ autopolling
From: Geert Uytterhoeven @ 2020-05-31  8:38 UTC (permalink / raw)
  To: Finn Thain
  Cc: linux-m68k, linuxppc-dev, Linux Kernel Mailing List,
	Joshua Thompson
In-Reply-To: <0fb7fdcd99d7820bb27faf1f27f7f6f1923914ef.1590880623.git.fthain@telegraphics.com.au>

Hi Finn,

On Sun, May 31, 2020 at 1:20 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> The adb_driver.autopoll method is needed during ADB bus scan and device
> address assignment. Implement this method so that the IOP's list of
> device addresses can be updated. When the list is empty, disable SRQ
> autopolling.
>
> Cc: Joshua Thompson <funaho@jurai.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Tested-by: Stan Johnson <userm57@yahoo.com>
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>

Thanks for your patch!

Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

>  arch/m68k/include/asm/adb_iop.h |  1 +
>  drivers/macintosh/adb-iop.c     | 32 ++++++++++++++++++++++++++------

As this header file is used by a single source file only, perhaps it should
just be absorbed by the latter?
Then you no longer need my Acked-by for future changes ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply

* [PATCH 5/8] macintosh/adb-iop: Resolve static checker warnings
From: Finn Thain @ 2020-05-30 23:17 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: linux-m68k, linuxppc-dev, linux-kernel, Joshua Thompson
In-Reply-To: <cover.1590880623.git.fthain@telegraphics.com.au>

drivers/macintosh/adb-iop.c:215:28: warning: Using plain integer as NULL pointer
drivers/macintosh/adb-iop.c:170:5: warning: symbol 'adb_iop_probe' was not declared. Should it be static?
drivers/macintosh/adb-iop.c:177:5: warning: symbol 'adb_iop_init' was not declared. Should it be static?
drivers/macintosh/adb-iop.c:184:5: warning: symbol 'adb_iop_send_request' was not declared. Should it be static?
drivers/macintosh/adb-iop.c:230:5: warning: symbol 'adb_iop_autopoll' was not declared. Should it be static?
drivers/macintosh/adb-iop.c:236:6: warning: symbol 'adb_iop_poll' was not declared. Should it be static?
drivers/macintosh/adb-iop.c:241:5: warning: symbol 'adb_iop_reset_bus' was not declared. Should it be static?

Cc: Joshua Thompson <funaho@jurai.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/macintosh/adb-iop.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/macintosh/adb-iop.c b/drivers/macintosh/adb-iop.c
index 7ecc41bc7358..a2b28e09f2ce 100644
--- a/drivers/macintosh/adb-iop.c
+++ b/drivers/macintosh/adb-iop.c
@@ -166,21 +166,21 @@ static void adb_iop_start(void)
 			 adb_iop_complete);
 }
 
-int adb_iop_probe(void)
+static int adb_iop_probe(void)
 {
 	if (!iop_ism_present)
 		return -ENODEV;
 	return 0;
 }
 
-int adb_iop_init(void)
+static int adb_iop_init(void)
 {
 	pr_info("adb: IOP ISM driver v0.4 for Unified ADB\n");
 	iop_listen(ADB_IOP, ADB_CHAN, adb_iop_listen, "ADB");
 	return 0;
 }
 
-int adb_iop_send_request(struct adb_request *req, int sync)
+static int adb_iop_send_request(struct adb_request *req, int sync)
 {
 	int err;
 
@@ -211,7 +211,7 @@ static int adb_iop_write(struct adb_request *req)
 
 	local_irq_save(flags);
 
-	if (current_req != 0) {
+	if (current_req) {
 		last_req->next = req;
 		last_req = req;
 	} else {
@@ -227,18 +227,18 @@ static int adb_iop_write(struct adb_request *req)
 	return 0;
 }
 
-int adb_iop_autopoll(int devs)
+static int adb_iop_autopoll(int devs)
 {
 	/* TODO: how do we enable/disable autopoll? */
 	return 0;
 }
 
-void adb_iop_poll(void)
+static void adb_iop_poll(void)
 {
 	iop_ism_irq_poll(ADB_IOP);
 }
 
-int adb_iop_reset_bus(void)
+static int adb_iop_reset_bus(void)
 {
 	struct adb_request req;
 
-- 
2.26.2


^ permalink raw reply related

* [PATCH 7/8] macintosh/adb-iop: Implement sending -> idle state transition
From: Finn Thain @ 2020-05-30 23:17 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: linux-m68k, linuxppc-dev, linux-kernel, Joshua Thompson
In-Reply-To: <cover.1590880623.git.fthain@telegraphics.com.au>

On leaving the 'sending' state, proceed to the 'idle' state if no reply is
expected. Drop redundant test for adb_iop_state == sending && current_req.

Cc: Joshua Thompson <funaho@jurai.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/macintosh/adb-iop.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/macintosh/adb-iop.c b/drivers/macintosh/adb-iop.c
index f22792c95d4f..8594e4f9a830 100644
--- a/drivers/macintosh/adb-iop.c
+++ b/drivers/macintosh/adb-iop.c
@@ -77,15 +77,14 @@ static void adb_iop_done(void)
 
 static void adb_iop_complete(struct iop_msg *msg)
 {
-	struct adb_request *req;
 	unsigned long flags;
 
 	local_irq_save(flags);
 
-	req = current_req;
-	if ((adb_iop_state == sending) && req && req->reply_expected) {
+	if (current_req->reply_expected)
 		adb_iop_state = awaiting_reply;
-	}
+	else
+		adb_iop_done();
 
 	local_irq_restore(flags);
 }
-- 
2.26.2


^ permalink raw reply related

* [PATCH 4/8] macintosh/adb-iop: Access current_req and adb_iop_state when inside lock
From: Finn Thain @ 2020-05-30 23:17 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: linux-m68k, linuxppc-dev, linux-kernel, Joshua Thompson
In-Reply-To: <cover.1590880623.git.fthain@telegraphics.com.au>

Drop the redundant local_irq_save/restore() from adb_iop_start() because
the caller has to do it anyway. This is the pattern used in via-macii.

Cc: Joshua Thompson <funaho@jurai.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/macintosh/adb-iop.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/macintosh/adb-iop.c b/drivers/macintosh/adb-iop.c
index c3089dacf2e2..7ecc41bc7358 100644
--- a/drivers/macintosh/adb-iop.c
+++ b/drivers/macintosh/adb-iop.c
@@ -137,7 +137,6 @@ static void adb_iop_listen(struct iop_msg *msg)
 
 static void adb_iop_start(void)
 {
-	unsigned long flags;
 	struct adb_request *req;
 	struct adb_iopmsg amsg;
 
@@ -146,8 +145,6 @@ static void adb_iop_start(void)
 	if (!req)
 		return;
 
-	local_irq_save(flags);
-
 	/* The IOP takes MacII-style packets, so strip the initial
 	 * ADB_PACKET byte.
 	 */
@@ -161,7 +158,6 @@ static void adb_iop_start(void)
 
 	req->sent = 1;
 	adb_iop_state = sending;
-	local_irq_restore(flags);
 
 	/* Now send it. The IOP manager will call adb_iop_complete
 	 * when the message has been sent.
@@ -208,13 +204,13 @@ static int adb_iop_write(struct adb_request *req)
 		return -EINVAL;
 	}
 
-	local_irq_save(flags);
-
 	req->next = NULL;
 	req->sent = 0;
 	req->complete = 0;
 	req->reply_len = 0;
 
+	local_irq_save(flags);
+
 	if (current_req != 0) {
 		last_req->next = req;
 		last_req = req;
@@ -223,10 +219,11 @@ static int adb_iop_write(struct adb_request *req)
 		last_req = req;
 	}
 
-	local_irq_restore(flags);
-
 	if (adb_iop_state == idle)
 		adb_iop_start();
+
+	local_irq_restore(flags);
+
 	return 0;
 }
 
-- 
2.26.2


^ permalink raw reply related

* [PATCH 3/8] macintosh/adb-iop: Adopt bus reset algorithm from via-macii driver
From: Finn Thain @ 2020-05-30 23:17 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: linux-m68k, linuxppc-dev, linux-kernel, Joshua Thompson
In-Reply-To: <cover.1590880623.git.fthain@telegraphics.com.au>

This algorithm is slightly shorter and avoids the surprising
adb_iop_start() call in adb_iop_poll().

Cc: Joshua Thompson <funaho@jurai.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/macintosh/adb-iop.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/macintosh/adb-iop.c b/drivers/macintosh/adb-iop.c
index ca3b411b0742..c3089dacf2e2 100644
--- a/drivers/macintosh/adb-iop.c
+++ b/drivers/macintosh/adb-iop.c
@@ -238,24 +238,19 @@ int adb_iop_autopoll(int devs)
 
 void adb_iop_poll(void)
 {
-	if (adb_iop_state == idle)
-		adb_iop_start();
 	iop_ism_irq_poll(ADB_IOP);
 }
 
 int adb_iop_reset_bus(void)
 {
-	struct adb_request req = {
-		.reply_expected = 0,
-		.nbytes = 2,
-		.data = { ADB_PACKET, 0 },
-	};
-
-	adb_iop_write(&req);
-	while (!req.complete) {
-		adb_iop_poll();
-		schedule();
-	}
+	struct adb_request req;
+
+	/* Command = 0, Address = ignored */
+	adb_request(&req, NULL, ADBREQ_NOSEND, 1, ADB_BUSRESET);
+	adb_iop_send_request(&req, 1);
+
+	/* Don't want any more requests during the Global Reset low time. */
+	mdelay(3);
 
 	return 0;
 }
-- 
2.26.2


^ permalink raw reply related

* [PATCH 2/8] macintosh/adb-iop: Correct comment text
From: Finn Thain @ 2020-05-30 23:17 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: linux-m68k, linuxppc-dev, linux-kernel, Joshua Thompson
In-Reply-To: <cover.1590880623.git.fthain@telegraphics.com.au>

This patch improves comment style and corrects some misunderstandings
in the text.

Cc: Joshua Thompson <funaho@jurai.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 drivers/macintosh/adb-iop.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/macintosh/adb-iop.c b/drivers/macintosh/adb-iop.c
index ce28ff40fb9c..ca3b411b0742 100644
--- a/drivers/macintosh/adb-iop.c
+++ b/drivers/macintosh/adb-iop.c
@@ -101,11 +101,10 @@ static void adb_iop_listen(struct iop_msg *msg)
 
 	req = current_req;
 
-	/* Handle a timeout. Timeout packets seem to occur even after */
-	/* we've gotten a valid reply to a TALK, so I'm assuming that */
-	/* a "timeout" is actually more like an "end-of-data" signal. */
-	/* We need to send back a timeout packet to the IOP to shut   */
-	/* it up, plus complete the current request, if any.          */
+	/* Handle a timeout. Timeout packets seem to occur even after
+	 * we've gotten a valid reply to a TALK, presumably because of
+	 * autopolling.
+	 */
 
 	if (amsg->flags & ADB_IOP_TIMEOUT) {
 		msg->reply[0] = ADB_IOP_TIMEOUT | ADB_IOP_AUTOPOLL;
@@ -115,9 +114,6 @@ static void adb_iop_listen(struct iop_msg *msg)
 			adb_iop_end_req(req, idle);
 		}
 	} else {
-		/* TODO: is it possible for more than one chunk of data  */
-		/*       to arrive before the timeout? If so we need to */
-		/*       use reply_ptr here like the other drivers do.  */
 		if ((adb_iop_state == awaiting_reply) &&
 		    (amsg->flags & ADB_IOP_EXPLICIT)) {
 			req->reply_len = amsg->count + 1;
@@ -152,23 +148,24 @@ static void adb_iop_start(void)
 
 	local_irq_save(flags);
 
-	/* The IOP takes MacII-style packets, so */
-	/* strip the initial ADB_PACKET byte.    */
-
+	/* The IOP takes MacII-style packets, so strip the initial
+	 * ADB_PACKET byte.
+	 */
 	amsg.flags = ADB_IOP_EXPLICIT;
 	amsg.count = req->nbytes - 2;
 
-	/* amsg.data immediately follows amsg.cmd, effectively making */
-	/* amsg.cmd a pointer to the beginning of a full ADB packet.  */
+	/* amsg.data immediately follows amsg.cmd, effectively making
+	 * &amsg.cmd a pointer to the beginning of a full ADB packet.
+	 */
 	memcpy(&amsg.cmd, req->data + 1, req->nbytes - 1);
 
 	req->sent = 1;
 	adb_iop_state = sending;
 	local_irq_restore(flags);
 
-	/* Now send it. The IOP manager will call adb_iop_complete */
-	/* when the packet has been sent.                          */
-
+	/* Now send it. The IOP manager will call adb_iop_complete
+	 * when the message has been sent.
+	 */
 	iop_send_message(ADB_IOP, ADB_CHAN, req, sizeof(amsg), (__u8 *)&amsg,
 			 adb_iop_complete);
 }
-- 
2.26.2


^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox