LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-26 22:00 UTC (permalink / raw)
  To: Arnd Bergmann, Jason Gunthorpe
  Cc: David Laight, Sinan Kaya, Oliver,
	open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
	linux-rdma@vger.kernel.org, Alexander Duyck, Will Deacon,
	Paul E. McKenney
In-Reply-To: <CAK8P3a2UU1xAM0NLo7Q4-Xgo1SzY3De1uqpFudr+2ZW7nHEPmA@mail.gmail.com>

On Mon, 2018-03-26 at 23:30 +0200, Arnd Bergmann wrote:
>  Most of the drivers have a unwound loop with writeq() or something to
> > do it.
> 
> But isn't the writeq() barrier much more expensive than anything you'd
> do in function calls?

It is for us, and will break any write combining.

> > > > The same document says that _relaxed() does not give that guarentee.
> > > > 
> > > > The lwn articule on this went into some depth on the interaction with
> > > > spinlocks.
> > > > 
> > > > As far as I can see, containment in a spinlock seems to be the only
> > > > different between writel and writel_relaxed..
> > > 
> > > I was always puzzled by this: The intention of _relaxed() on ARM
> > > (where it originates) was to skip the barrier that serializes DMA
> > > with MMIO, not to skip the serialization between MMIO and locks.
> > 
> > But that was never a requirement of writel(),
> > Documentation/memory-barriers.txt gives an explicit example demanding
> > the wmb() before writel() for ordering system memory against writel.

This is a bug in the documentation.

> Indeed, but it's in an example for when to use dma_wmb(), not wmb().
> Adding Alexander Duyck to Cc, he added that section as part of
> 1077fa36f23e ("arch: Add lightweight memory barriers dma_rmb() and
> dma_wmb()"). Also adding the other people that were involved with that.

Linus himself made it very clear years ago. readl and writel have to
order vs memory accesses.

> > I actually have no idea why ARM had that barrier, I always assumed it
> > was to give program ordering to the accesses and that _relaxed allowed
> > re-ordering (the usual meaning of relaxed)..
> > 
> > But the barrier document makes it pretty clear that the only
> > difference between the two is spinlock containment, and WillD wrote
> > this text, so I belive it is accurate for ARM.
> > 
> > Very confusing.
> 
> It does mention serialization with both DMA and locks in the
> section about  readX_relaxed()/writeX_relaxed(). The part
> about DMA is very clear here, and I must have just forgotten
> the exact semantics with regards to spinlocks. I'm still not
> sure what prevents a writel() from leaking out the end of a
> spinlock section that doesn't happen with writel_relaxed(), since
> the barrier in writel() comes before the access, and the
> spin_unlock() shouldn't affect the external buses.

So...

Historically, what happened is that we (we means whoever participated
in the discussion on the list with Linus calling the shots really)
decided that there was no sane way for drivers to understand a world
where readl/writel didn't fully order things vs. memory accesses (ie,
DMA).

So it should always be correct to do:

	- Write to some in-memory buffer
	- writel() to kick the DMA read of that buffer

without any extra barrier.

The spinlock situation however got murky. Mostly that came up because
on architecture (I forgot who, might have been ia64) has a hard time
providing that consistency without making writel insanely expensive.

Thus they created mmiowb whose main purpose was precisely to order
writel with a following spin_unlock.

I decided not to go down that path on power because getting all drivers
"fixed" to do the right thing was going to be a losing battle, and
instead added per-cpu tracking of writel in order to "escalate" to a
heavier barrier in spin_unlock itself when necessary.

Now, all this happened more than a decade ago and it's possible that
the understanding or expectations "shifted" over time...

Cheers,
Ben.
 

^ permalink raw reply

* Re: RFC on writel and writel_relaxed
From: Sinan Kaya @ 2018-03-26 21:46 UTC (permalink / raw)
  To: Arnd Bergmann, Jason Gunthorpe
  Cc: David Laight, Oliver,
	open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
	linux-rdma@vger.kernel.org, Alexander Duyck, Will Deacon,
	Benjamin Herrenschmidt, Paul E. McKenney
In-Reply-To: <CAK8P3a2UU1xAM0NLo7Q4-Xgo1SzY3De1uqpFudr+2ZW7nHEPmA@mail.gmail.com>

On 3/26/2018 5:30 PM, Arnd Bergmann wrote:
>> But that was never a requirement of writel(),
>> Documentation/memory-barriers.txt gives an explicit example demanding
>> the wmb() before writel() for ordering system memory against writel.
> Indeed, but it's in an example for when to use dma_wmb(), not wmb().
> Adding Alexander Duyck to Cc, he added that section as part of
> 1077fa36f23e ("arch: Add lightweight memory barriers dma_rmb() and
> dma_wmb()"). Also adding the other people that were involved with that.
> 

ARM developers can get away with not including wmb() in their code and use
writel() to observe memory writes due to implicit barriers.

However, same code will not work on Intel.

writel() has a compiler barrier in it for x86.
wmb() has a sync operation in it for x86. 

Unless wmb() is called, PCIe device won't observe memory updates from the CPU.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

^ permalink raw reply

* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-26 21:26 UTC (permalink / raw)
  To: Jason Gunthorpe, David Laight
  Cc: Sinan Kaya, open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
	Oliver, linux-rdma@vger.kernel.org
In-Reply-To: <20180326165425.GA15554@ziepe.ca>

On Mon, 2018-03-26 at 10:54 -0600, Jason Gunthorpe wrote:
> On Mon, Mar 26, 2018 at 11:08:45AM +0000, David Laight wrote:
> > > > This is a super performance critical operation for most drivers and
> > > > directly impacts network performance.
> > 
> > Perhaps there ought to be writel_nobarrier() (etc) that never contain
> > any barriers at all.
> > This might mean that they are always just the memory operation,
> > but it would make it more obvious what the driver was doing.
> 
> I think that is what writel_relaxed is supposed to be.
> 
> The only restriction it has is that the writes to a single device
> using UC memory must be kept in program order..

Which requires barriers on some architectures :-)

Also we don't have a clear definition of what happens on WC memory.

Cheers,
Ben.

^ permalink raw reply

* Re: [PATCH v9 21/24] perf tools: Add support for the SPF perf event
From: David Rientjes @ 2018-03-26 21:44 UTC (permalink / raw)
  To: Laurent Dufour
  Cc: paulmck, peterz, akpm, kirill, ak, mhocko, dave, jack,
	Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner, Ingo Molnar,
	hpa, Will Deacon, Sergey Senozhatsky, Andrea Arcangeli,
	Alexei Starovoitov, kemi.wang, sergey.senozhatsky.work,
	Daniel Jordan, linux-kernel, linux-mm, haren, khandual, npiggin,
	bsingharora, Tim Chen, linuxppc-dev, x86
In-Reply-To: <1520963994-28477-22-git-send-email-ldufour@linux.vnet.ibm.com>

On Tue, 13 Mar 2018, Laurent Dufour wrote:

> Add support for the new speculative faults event.
> 
> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>

Acked-by: David Rientjes <rientjes@google.com>

Aside: should there be a new spec_flt field for struct task_struct that 
complements maj_flt and min_flt and be exported through /proc/pid/stat?

^ permalink raw reply

* Re: [PATCH v9 20/24] perf: Add a speculative page fault sw event
From: David Rientjes @ 2018-03-26 21:43 UTC (permalink / raw)
  To: Laurent Dufour
  Cc: paulmck, peterz, akpm, kirill, ak, mhocko, dave, jack,
	Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner, Ingo Molnar,
	hpa, Will Deacon, Sergey Senozhatsky, Andrea Arcangeli,
	Alexei Starovoitov, kemi.wang, sergey.senozhatsky.work,
	Daniel Jordan, linux-kernel, linux-mm, haren, khandual, npiggin,
	bsingharora, Tim Chen, linuxppc-dev, x86
In-Reply-To: <1520963994-28477-21-git-send-email-ldufour@linux.vnet.ibm.com>

On Tue, 13 Mar 2018, Laurent Dufour wrote:

> Add a new software event to count succeeded speculative page faults.
> 
> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>

Acked-by: David Rientjes <rientjes@google.com>

^ permalink raw reply

* Re: [PATCH v9 23/24] x86/mm: Add speculative pagefault handling
From: David Rientjes @ 2018-03-26 21:41 UTC (permalink / raw)
  To: Laurent Dufour
  Cc: paulmck, peterz, akpm, kirill, ak, mhocko, dave, jack,
	Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner, Ingo Molnar,
	hpa, Will Deacon, Sergey Senozhatsky, Andrea Arcangeli,
	Alexei Starovoitov, kemi.wang, sergey.senozhatsky.work,
	Daniel Jordan, linux-kernel, linux-mm, haren, khandual, npiggin,
	bsingharora, Tim Chen, linuxppc-dev, x86
In-Reply-To: <1520963994-28477-24-git-send-email-ldufour@linux.vnet.ibm.com>

On Tue, 13 Mar 2018, Laurent Dufour wrote:

> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index e6af2b464c3d..a73cf227edd6 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -1239,6 +1239,9 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
>  		unsigned long address)
>  {
>  	struct vm_area_struct *vma;
> +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT
> +	struct vm_area_struct *spf_vma = NULL;
> +#endif
>  	struct task_struct *tsk;
>  	struct mm_struct *mm;
>  	int fault, major = 0;
> @@ -1332,6 +1335,27 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
>  	if (error_code & X86_PF_INSTR)
>  		flags |= FAULT_FLAG_INSTRUCTION;
>  
> +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT
> +	if ((error_code & X86_PF_USER) && (atomic_read(&mm->mm_users) > 1)) {
> +		fault = handle_speculative_fault(mm, address, flags,
> +						 &spf_vma);
> +
> +		if (!(fault & VM_FAULT_RETRY)) {
> +			if (!(fault & VM_FAULT_ERROR)) {
> +				perf_sw_event(PERF_COUNT_SW_SPF, 1,
> +					      regs, address);
> +				goto done;
> +			}
> +			/*
> +			 * In case of error we need the pkey value, but
> +			 * can't get it from the spf_vma as it is only returned
> +			 * when VM_FAULT_RETRY is returned. So we have to
> +			 * retry the page fault with the mmap_sem grabbed.
> +			 */
> +		}
> +	}
> +#endif /* CONFIG_SPECULATIVE_PAGE_FAULT */

All the comments from the powerpc version will apply here as well, the 
only interesting point is whether VM_FAULT_FALLBACK can be returned from 
handle_speculative_fault() to indicate its not possible.

> +
>  	/*
>  	 * When running in the kernel we expect faults to occur only to
>  	 * addresses in user space.  All other faults represent errors in
> @@ -1365,7 +1389,16 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
>  		might_sleep();
>  	}
>  
> -	vma = find_vma(mm, address);
> +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT
> +	if (spf_vma) {
> +		if (can_reuse_spf_vma(spf_vma, address))
> +			vma = spf_vma;
> +		else
> +			vma = find_vma(mm, address);
> +		spf_vma = NULL;
> +	} else
> +#endif
> +		vma = find_vma(mm, address);
>  	if (unlikely(!vma)) {
>  		bad_area(regs, error_code, address);
>  		return;
> @@ -1451,6 +1484,9 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code,
>  		return;
>  	}
>  
> +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT
> +done:
> +#endif
>  	/*
>  	 * Major/minor page fault accounting. If any of the events
>  	 * returned VM_FAULT_MAJOR, we account it as a major fault.

^ permalink raw reply

* Re: [PATCH v9 24/24] powerpc/mm: Add speculative page fault
From: David Rientjes @ 2018-03-26 21:39 UTC (permalink / raw)
  To: Laurent Dufour
  Cc: paulmck, peterz, akpm, kirill, ak, mhocko, dave, jack,
	Matthew Wilcox, benh, mpe, paulus, Thomas Gleixner, Ingo Molnar,
	hpa, Will Deacon, Sergey Senozhatsky, Andrea Arcangeli,
	Alexei Starovoitov, kemi.wang, sergey.senozhatsky.work,
	Daniel Jordan, linux-kernel, linux-mm, haren, khandual, npiggin,
	bsingharora, Tim Chen, linuxppc-dev, x86
In-Reply-To: <1520963994-28477-25-git-send-email-ldufour@linux.vnet.ibm.com>

On Tue, 13 Mar 2018, Laurent Dufour wrote:

> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 866446cf2d9a..104f3cc86b51 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -392,6 +392,9 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
>  			   unsigned long error_code)
>  {
>  	struct vm_area_struct * vma;
> +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT
> +	struct vm_area_struct *spf_vma = NULL;
> +#endif
>  	struct mm_struct *mm = current->mm;
>  	unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
>   	int is_exec = TRAP(regs) == 0x400;
> @@ -459,6 +462,20 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
>  	if (is_exec)
>  		flags |= FAULT_FLAG_INSTRUCTION;
>  
> +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT
> +	if (is_user && (atomic_read(&mm->mm_users) > 1)) {
> +		/* let's try a speculative page fault without grabbing the
> +		 * mmap_sem.
> +		 */
> +		fault = handle_speculative_fault(mm, address, flags, &spf_vma);
> +		if (!(fault & VM_FAULT_RETRY)) {
> +			perf_sw_event(PERF_COUNT_SW_SPF, 1,
> +				      regs, address);
> +			goto done;
> +		}
> +	}
> +#endif /* CONFIG_SPECULATIVE_PAGE_FAULT */
> +

Can't you elimiate all #ifdef's in this patch if 
handle_speculative_fault() can be passed is_user and return some error 
code that fallback is needed?  Maybe reuse VM_FAULT_FALLBACK?

>  	/* When running in the kernel we expect faults to occur only to
>  	 * addresses in user space.  All other faults represent errors in the
>  	 * kernel and should generate an OOPS.  Unfortunately, in the case of an
> @@ -489,7 +506,16 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
>  		might_sleep();
>  	}
>  
> -	vma = find_vma(mm, address);
> +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT
> +	if (spf_vma) {
> +		if (can_reuse_spf_vma(spf_vma, address))
> +			vma = spf_vma;
> +		else
> +			vma =  find_vma(mm, address);
> +		spf_vma = NULL;
> +	} else
> +#endif
> +		vma = find_vma(mm, address);
>  	if (unlikely(!vma))
>  		return bad_area(regs, address);
>  	if (likely(vma->vm_start <= address))

I think the code quality here could be improved such that you can pass mm, 
&spf_vma, and address and some helper function would return spf_vma if 
can_reuse_spf_vma() is true (and do *spf_vma to NULL) or otherwise return 
find_vma(mm, address).

Also, spf_vma is being set to NULL because of VM_FAULT_RETRY, but does it 
make sense to retry handle_speculative_fault() in this case since we've 
dropped mm->mmap_sem and there may have been a writer queued behind it?

> @@ -568,6 +594,9 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
>  
>  	up_read(&current->mm->mmap_sem);
>  
> +#ifdef CONFIG_SPECULATIVE_PAGE_FAULT
> +done:
> +#endif
>  	if (unlikely(fault & VM_FAULT_ERROR))
>  		return mm_fault_error(regs, address, fault);
>  

And things like this are trivially handled by doing

done: __maybe_unused

^ permalink raw reply

* Re: RFC on writel and writel_relaxed
From: Arnd Bergmann @ 2018-03-26 21:30 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: David Laight, Sinan Kaya, Oliver,
	open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
	linux-rdma@vger.kernel.org, Alexander Duyck, Will Deacon,
	Benjamin Herrenschmidt, Paul E. McKenney
In-Reply-To: <20180326210951.GD15554@ziepe.ca>

On Mon, Mar 26, 2018 at 11:09 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> On Mon, Mar 26, 2018 at 10:43:43PM +0200, Arnd Bergmann wrote:
>> On Mon, Mar 26, 2018 at 10:25 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>> > On Mon, Mar 26, 2018 at 09:44:15PM +0200, Arnd Bergmann wrote:
>> >> On Mon, Mar 26, 2018 at 6:54 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>> >> > On Mon, Mar 26, 2018 at 11:08:45AM +0000, David Laight wrote:
>> >> >> > > This is a super performance critical operation for most drivers and
>> >> >> > > directly impacts network performance.
>> >> >>
>> >> >> Perhaps there ought to be writel_nobarrier() (etc) that never contain
>> >> >> any barriers at all.
>> >> >> This might mean that they are always just the memory operation,
>> >> >> but it would make it more obvious what the driver was doing.
>> >> >
>> >> > I think that is what writel_relaxed is supposed to be.
>> >> >
>> >> > The only restriction it has is that the writes to a single device
>> >> > using UC memory must be kept in program order..
>> >>
>> >> Not sure about whether we have ever defined what happens to
>> >> writel_relaxed() on WC memory though: On ARM, we disallow
>> >> the compiler to combine writes, but the CPU still might.
>> >
>> > If the driver uses WC memory then I think it should not expect
>> > anything in terms of how writes map to TLPs other than nothing
>> > combines across mmiowb() and mmiowb() is fully globally ordered when
>> > enclosed in a spinlock.
>> >
>> > The entire point of using WC memory is usually to get combining :) If
>> > the driver doesn't want that then it should map UC..
>>
>> Usually, WC memory is used with memcpy_toio() though, which
>> by definition doesn't have any barriers between accesses, and
>> is required to get the correct byte ordering on writes to memory buffers.
>
> memcpy_toio is too expensive to actually use for anything performance
> though. It is too pessimistic. What the drivers usually want is a
> unwound block of 4 or 8 8-byte copies. No function calls, no
> branching. Everything is already known to be aligned.
>
> Most of the drivers have a unwound loop with writeq() or something to
> do it.

But isn't the writeq() barrier much more expensive than anything you'd
do in function calls?

>> > The same document says that _relaxed() does not give that guarentee.
>> >
>> > The lwn articule on this went into some depth on the interaction with
>> > spinlocks.
>> >
>> > As far as I can see, containment in a spinlock seems to be the only
>> > different between writel and writel_relaxed..
>>
>> I was always puzzled by this: The intention of _relaxed() on ARM
>> (where it originates) was to skip the barrier that serializes DMA
>> with MMIO, not to skip the serialization between MMIO and locks.
>
> But that was never a requirement of writel(),
> Documentation/memory-barriers.txt gives an explicit example demanding
> the wmb() before writel() for ordering system memory against writel.

Indeed, but it's in an example for when to use dma_wmb(), not wmb().
Adding Alexander Duyck to Cc, he added that section as part of
1077fa36f23e ("arch: Add lightweight memory barriers dma_rmb() and
dma_wmb()"). Also adding the other people that were involved with that.

> I actually have no idea why ARM had that barrier, I always assumed it
> was to give program ordering to the accesses and that _relaxed allowed
> re-ordering (the usual meaning of relaxed)..
>
> But the barrier document makes it pretty clear that the only
> difference between the two is spinlock containment, and WillD wrote
> this text, so I belive it is accurate for ARM.
>
> Very confusing.

It does mention serialization with both DMA and locks in the
section about  readX_relaxed()/writeX_relaxed(). The part
about DMA is very clear here, and I must have just forgotten
the exact semantics with regards to spinlocks. I'm still not
sure what prevents a writel() from leaking out the end of a
spinlock section that doesn't happen with writel_relaxed(), since
the barrier in writel() comes before the access, and the
spin_unlock() shouldn't affect the external buses.

      Arnd

^ permalink raw reply

* Re: RFC on writel and writel_relaxed
From: Jason Gunthorpe @ 2018-03-26 21:09 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: David Laight, Sinan Kaya, Oliver,
	open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
	linux-rdma@vger.kernel.org
In-Reply-To: <CAK8P3a3fc43ZcW626hmsd3DVcLw7hGkdUMxp7s4Rn3mdkziwMQ@mail.gmail.com>

On Mon, Mar 26, 2018 at 10:43:43PM +0200, Arnd Bergmann wrote:
> On Mon, Mar 26, 2018 at 10:25 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > On Mon, Mar 26, 2018 at 09:44:15PM +0200, Arnd Bergmann wrote:
> >> On Mon, Mar 26, 2018 at 6:54 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >> > On Mon, Mar 26, 2018 at 11:08:45AM +0000, David Laight wrote:
> >> >> > > This is a super performance critical operation for most drivers and
> >> >> > > directly impacts network performance.
> >> >>
> >> >> Perhaps there ought to be writel_nobarrier() (etc) that never contain
> >> >> any barriers at all.
> >> >> This might mean that they are always just the memory operation,
> >> >> but it would make it more obvious what the driver was doing.
> >> >
> >> > I think that is what writel_relaxed is supposed to be.
> >> >
> >> > The only restriction it has is that the writes to a single device
> >> > using UC memory must be kept in program order..
> >>
> >> Not sure about whether we have ever defined what happens to
> >> writel_relaxed() on WC memory though: On ARM, we disallow
> >> the compiler to combine writes, but the CPU still might.
> >
> > If the driver uses WC memory then I think it should not expect
> > anything in terms of how writes map to TLPs other than nothing
> > combines across mmiowb() and mmiowb() is fully globally ordered when
> > enclosed in a spinlock.
> >
> > The entire point of using WC memory is usually to get combining :) If
> > the driver doesn't want that then it should map UC..
> 
> Usually, WC memory is used with memcpy_toio() though, which
> by definition doesn't have any barriers between accesses, and
> is required to get the correct byte ordering on writes to memory buffers.

memcpy_toio is too expensive to actually use for anything performance
though. It is too pessimistic. What the drivers usually want is a
unwound block of 4 or 8 8-byte copies. No function calls, no
branching. Everything is already known to be aligned.

Most of the drivers have a unwound loop with writeq() or something to
do it.

> > The same document says that _relaxed() does not give that guarentee.
> >
> > The lwn articule on this went into some depth on the interaction with
> > spinlocks.
> >
> > As far as I can see, containment in a spinlock seems to be the only
> > different between writel and writel_relaxed..
> 
> I was always puzzled by this: The intention of _relaxed() on ARM
> (where it originates) was to skip the barrier that serializes DMA
> with MMIO, not to skip the serialization between MMIO and locks.

But that was never a requirement of writel(),
Documentation/memory-barriers.txt gives an explicit example demanding
the wmb() before writel() for ordering system memory against writel.

I actually have no idea why ARM had that barrier, I always assumed it
was to give program ordering to the accesses and that _relaxed allowed
re-ordering (the usual meaning of relaxed)..

But the barrier document makes it pretty clear that the only
difference between the two is spinlock containment, and WillD wrote
this text, so I belive it is accurate for ARM.

Very confusing.

> I never fully understood the part about the locks, but from what
> I remember, ARM is still serialized without the barrier here, but
> dropping the barrier on powerpc writel_relaxed() would not
> serialize against locks or DMA.

WC is usually the problem here.. I've been told it is necessary on ARM
as well..

Jason

^ permalink raw reply

* Re: RFC on writel and writel_relaxed
From: Arnd Bergmann @ 2018-03-26 20:43 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: David Laight, Sinan Kaya, Oliver,
	open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
	linux-rdma@vger.kernel.org
In-Reply-To: <20180326202545.GB15554@ziepe.ca>

On Mon, Mar 26, 2018 at 10:25 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> On Mon, Mar 26, 2018 at 09:44:15PM +0200, Arnd Bergmann wrote:
>> On Mon, Mar 26, 2018 at 6:54 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>> > On Mon, Mar 26, 2018 at 11:08:45AM +0000, David Laight wrote:
>> >> > > This is a super performance critical operation for most drivers and
>> >> > > directly impacts network performance.
>> >>
>> >> Perhaps there ought to be writel_nobarrier() (etc) that never contain
>> >> any barriers at all.
>> >> This might mean that they are always just the memory operation,
>> >> but it would make it more obvious what the driver was doing.
>> >
>> > I think that is what writel_relaxed is supposed to be.
>> >
>> > The only restriction it has is that the writes to a single device
>> > using UC memory must be kept in program order..
>>
>> Not sure about whether we have ever defined what happens to
>> writel_relaxed() on WC memory though: On ARM, we disallow
>> the compiler to combine writes, but the CPU still might.
>
> If the driver uses WC memory then I think it should not expect
> anything in terms of how writes map to TLPs other than nothing
> combines across mmiowb() and mmiowb() is fully globally ordered when
> enclosed in a spinlock.
>
> The entire point of using WC memory is usually to get combining :) If
> the driver doesn't want that then it should map UC..

Usually, WC memory is used with memcpy_toio() though, which
by definition doesn't have any barriers between accesses, and
is required to get the correct byte ordering on writes to memory buffers.

>> It's also not entirely clear to me what we want writel() inside a
>> spinlock to mean: should the spinlock guarantee that two writel()
>> calls on different CPUs that are protected by spinlocks are
>> serialized by those locks, or not?
>
> Yes for writel, I think that is already defined by the barriers
> document

Sorry, I meant writel_relaxed(), not writel()

> The same document says that _relaxed() does not give that guarentee.
>
> The lwn articule on this went into some depth on the interaction with
> spinlocks.
>
> As far as I can see, containment in a spinlock seems to be the only
> different between writel and writel_relaxed..

I was always puzzled by this: The intention of _relaxed() on ARM
(where it originates) was to skip the barrier that serializes DMA
with MMIO, not to skip the serialization between MMIO and locks.

I never fully understood the part about the locks, but from what
I remember, ARM is still serialized without the barrier here, but
dropping the barrier on powerpc writel_relaxed() would not
serialize against locks or DMA.

         Arnd

^ permalink raw reply

* [PATCH script] hwmon: Use octal not symbolic permissions
From: Joe Perches @ 2018-03-26 20:28 UTC (permalink / raw)
  To: Jean Delvare, Guenter Roeck
  Cc: linux-hwmon, linux-kernel, linuxppc-dev, linux-arm-kernel,
	patches

drivers/hwmon is the most frequent user of symbolic permissions
like S_IRUGO in the kernel tree.

$ git grep -w -P "S_[A-Z]{5,5}" | \
  cut -f1 -d: | cut -f1-2 -d"/" | sed -r 's/[A-Za-z0-9_-]+\.[ch]$//' | \
  sort | uniq -c | sort -rn | head
   3862 drivers/hwmon
    814 drivers/scsi
    763 drivers/net
    242 drivers/infiniband
    184 drivers/staging
    181 drivers/usb
    158 fs/proc
    150 fs/xfs
    148 fs/
    142 drivers/misc

But using octal and not symbolic permissions is preferred by many
as it can be more readable.

https://lkml.org/lkml/2016/8/2/1945

Rather than converting these piecemeal, perhaps just do them all
at once via a trivial script like the below:

$ git grep -w -P --name-only "S_[A-Z]{5,5}" drivers/hwmon | \
  xargs ./scripts/checkpatch.pl -f --types=symbolic_perms --fix-inplace
$ git grep -w -P --name-only "S_[A-Z]{5,5}" drivers/hwmon | \
  xargs ./scripts/checkpatch.pl -f --types=symbolic_perms --fix-inplace

It's run twice because checkpatch only does 1 conversion per line
and there are some multiple instance lines.

This currently results in a 669 KB patch which is too large
to post but can be easily generated when appropriate.

^ permalink raw reply

* Re: RFC on writel and writel_relaxed
From: Jason Gunthorpe @ 2018-03-26 20:25 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: David Laight, Sinan Kaya, Oliver,
	open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
	linux-rdma@vger.kernel.org
In-Reply-To: <CAK8P3a1zeMyj+Z-y4ER4moY6Zip9EWNOinf+VnboGOrgiwbBZA@mail.gmail.com>

On Mon, Mar 26, 2018 at 09:44:15PM +0200, Arnd Bergmann wrote:
> On Mon, Mar 26, 2018 at 6:54 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > On Mon, Mar 26, 2018 at 11:08:45AM +0000, David Laight wrote:
> >> > > This is a super performance critical operation for most drivers and
> >> > > directly impacts network performance.
> >>
> >> Perhaps there ought to be writel_nobarrier() (etc) that never contain
> >> any barriers at all.
> >> This might mean that they are always just the memory operation,
> >> but it would make it more obvious what the driver was doing.
> >
> > I think that is what writel_relaxed is supposed to be.
> >
> > The only restriction it has is that the writes to a single device
> > using UC memory must be kept in program order..
> 
> Not sure about whether we have ever defined what happens to
> writel_relaxed() on WC memory though: On ARM, we disallow
> the compiler to combine writes, but the CPU still might.

If the driver uses WC memory then I think it should not expect
anything in terms of how writes map to TLPs other than nothing
combines across mmiowb() and mmiowb() is fully globally ordered when
enclosed in a spinlock.

The entire point of using WC memory is usually to get combining :) If
the driver doesn't want that then it should map UC..

> It's also not entirely clear to me what we want writel() inside a
> spinlock to mean: should the spinlock guarantee that two writel()
> calls on different CPUs that are protected by spinlocks are
> serialized by those locks, or not?

Yes for writel, I think that is already defined by the barriers
document

The same document says that _relaxed() does not give that guarentee.

The lwn articule on this went into some depth on the interaction with
spinlocks.

As far as I can see, containment in a spinlock seems to be the only
different between writel and writel_relaxed..

Jason

^ permalink raw reply

* Re: [RFC PATCH v2 0/2] Randomization of address chosen by mmap.
From: Ilya Smith @ 2018-03-26 19:45 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Matthew Wilcox, rth, ink, mattst88, vgupta, linux, tony.luck,
	fenghua.yu, ralf, jejb, Helge Deller, benh, paulus, mpe,
	schwidefsky, heiko.carstens, ysato, dalias, davem, tglx, mingo,
	hpa, x86, nyc, viro, arnd, gregkh, deepa.kernel, Hugh Dickins,
	kstewart, pombredanne, Andrew Morton, steve.capper, punit.agrawal,
	aneesh.kumar, npiggin, Kees Cook, bhsharma, riel, nitin.m.gupta,
	Kirill A. Shutemov, Dan Williams, Jan Kara, ross.zwisler,
	Jerome Glisse, Andrea Arcangeli, Oleg Nesterov, linux-alpha, LKML,
	linux-snps-arc, linux-ia64, linux-metag, linux-mips, linux-parisc,
	linuxppc-dev, linux-s390, linux-sh, sparclinux, Linux-MM
In-Reply-To: <20180326084650.GC5652@dhcp22.suse.cz>


> On 26 Mar 2018, at 11:46, Michal Hocko <mhocko@kernel.org> wrote:
>=20
> On Fri 23-03-18 20:55:49, Ilya Smith wrote:
>>=20
>>> On 23 Mar 2018, at 15:48, Matthew Wilcox <willy@infradead.org> =
wrote:
>>>=20
>>> On Thu, Mar 22, 2018 at 07:36:36PM +0300, Ilya Smith wrote:
>>>> Current implementation doesn't randomize address returned by mmap.
>>>> All the entropy ends with choosing mmap_base_addr at the process
>>>> creation. After that mmap build very predictable layout of address
>>>> space. It allows to bypass ASLR in many cases. This patch make
>>>> randomization of address on any mmap call.
>>>=20
>>> Why should this be done in the kernel rather than libc?  libc is =
perfectly
>>> capable of specifying random numbers in the first argument of mmap.
>> Well, there is following reasons:
>> 1. It should be done in any libc implementation, what is not possible =
IMO;
>=20
> Is this really so helpful?

Yes, ASLR is one of very important mitigation techniques which are =
really used=20
to protect applications. If there is no ASLR, it is very easy to exploit=20=

vulnerable application and compromise the system. We can=E2=80=99t just =
fix all the=20
vulnerabilities right now, thats why we have mitigations - techniques =
which are=20
makes exploitation more hard or impossible in some cases.

Thats why it is helpful.

>=20
>> 2. User mode is not that layer which should be responsible for =
choosing
>> random address or handling entropy;
>=20
> Why?

Because of the following reasons:
1. To get random address you should have entropy. These entropy =
shouldn=E2=80=99t be=20
exposed to attacker anyhow, the best case is to get it from kernel. So =
this is
a syscall.
2. You should have memory map of your process to prevent remapping or =
big
fragmentation. Kernel already has this map. You will got another one in =
libc.
And any non-libc user of mmap (via syscall, etc) will make hole in your =
map.
This one also decrease performance cause you any way call syscall_mmap=20=

which will try to find some address for you in worst case, but after you =
already
did some computing on it.
3. The more memory you use in userland for these proposal, the easier =
for
attacker to leak it or use in exploitation techniques.
4. It is so easy to fix Kernel function and so hard to support memory
management from userspace.

>=20
>> 3. Memory fragmentation is unpredictable in this case
>>=20
>> Off course user mode could use random =E2=80=98hint=E2=80=99 address, =
but kernel may
>> discard this address if it is occupied for example and allocate just =
before
>> closest vma. So this solution doesn=E2=80=99t give that much security =
like=20
>> randomization address inside kernel.
>=20
> The userspace can use the new MAP_FIXED_NOREPLACE to probe for the
> address range atomically and chose a different range on failure.
>=20

This algorithm should track current memory. If he doesn=E2=80=99t he may =
cause
infinite loop while trying to choose memory. And each iteration increase =
time
needed on allocation new memory, what is not preferred by any libc =
library
developer.

Thats why I did this patch.

Thanks,
Ilya

^ permalink raw reply

* Re: RFC on writel and writel_relaxed
From: Arnd Bergmann @ 2018-03-26 19:44 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: David Laight, Sinan Kaya, Oliver,
	open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
	linux-rdma@vger.kernel.org
In-Reply-To: <20180326165425.GA15554@ziepe.ca>

On Mon, Mar 26, 2018 at 6:54 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> On Mon, Mar 26, 2018 at 11:08:45AM +0000, David Laight wrote:
>> > > This is a super performance critical operation for most drivers and
>> > > directly impacts network performance.
>>
>> Perhaps there ought to be writel_nobarrier() (etc) that never contain
>> any barriers at all.
>> This might mean that they are always just the memory operation,
>> but it would make it more obvious what the driver was doing.
>
> I think that is what writel_relaxed is supposed to be.
>
> The only restriction it has is that the writes to a single device
> using UC memory must be kept in program order..

Not sure about whether we have ever defined what happens to
writel_relaxed() on WC memory though: On ARM, we disallow
the compiler to combine writes, but the CPU still might.

It's also not entirely clear to me what we want writel() inside a
spinlock to mean: should the spinlock guarantee that two writel()
calls on different CPUs that are protected by spinlocks are
serialized by those locks, or not?

       Arnd

^ permalink raw reply

* Re: [PATCH v12 04/22] selftests/vm: typecast the pkey register
From: Thiago Jung Bauermann @ 2018-03-26 19:38 UTC (permalink / raw)
  To: Dave Hansen
  Cc: Ram Pai, shuahkh, linux-kselftest, mpe, linuxppc-dev, linux-mm,
	x86, linux-arch, linux-doc, linux-kernel, mingo, akpm, benh,
	paulus, khandual, aneesh.kumar, bsingharora, hbabu, mhocko,
	ebiederm, arnd
In-Reply-To: <00081300-e891-3381-3acd-e3312e54fb58@intel.com>


Dave Hansen <dave.hansen@intel.com> writes:

> On 02/21/2018 05:55 PM, Ram Pai wrote:
>> -static inline unsigned int _rdpkey_reg(int line)
>> +static inline pkey_reg_t _rdpkey_reg(int line)
>>  {
>> -	unsigned int pkey_reg = __rdpkey_reg();
>> +	pkey_reg_t pkey_reg = __rdpkey_reg();
>>
>> -	dprintf4("rdpkey_reg(line=%d) pkey_reg: %x shadow: %x\n",
>> +	dprintf4("rdpkey_reg(line=%d) pkey_reg: %016lx shadow: %016lx\n",
>>  			line, pkey_reg, shadow_pkey_reg);
>>  	assert(pkey_reg == shadow_pkey_reg);
>
> Hmm.  So we're using %lx for an int?  Doesn't the compiler complain
> about this?

It doesn't because dprintf4() doesn't have the annotation that tells the
compiler that it takes printf-like arguments. Once I add it:

--- a/tools/testing/selftests/vm/pkey-helpers.h
+++ b/tools/testing/selftests/vm/pkey-helpers.h
@@ -54,6 +54,10 @@
 #define DPRINT_IN_SIGNAL_BUF_SIZE 4096
 extern int dprint_in_signal;
 extern char dprint_in_signal_buffer[DPRINT_IN_SIGNAL_BUF_SIZE];
+
+#ifdef __GNUC__
+__attribute__((format(printf, 1, 2)))
+#endif
 static inline void sigsafe_printf(const char *format, ...)
 {
 	va_list ap;

Then it does complain about it. I'm working on a fix where each arch
will define a format string to use for its pkey_reg_t and use it like
this:

--- a/tools/testing/selftests/vm/pkey-helpers.h
+++ b/tools/testing/selftests/vm/pkey-helpers.h
@@ -19,6 +19,7 @@
 #define u32 uint32_t
 #define u64 uint64_t
 #define pkey_reg_t u32
+#define PKEY_REG_FMT "%016x"

 #ifdef __i386__
 #ifndef SYS_mprotect_key
@@ -112,7 +113,8 @@ static inline pkey_reg_t _read_pkey_reg(int line)
 {
 	pkey_reg_t pkey_reg = __read_pkey_reg();

-	dprintf4("read_pkey_reg(line=%d) pkey_reg: %016lx shadow: %016lx\n",
+	dprintf4("read_pkey_reg(line=%d) pkey_reg: "PKEY_REG_FMT
+			" shadow: "PKEY_REG_FMT"\n",
 			line, pkey_reg, shadow_pkey_reg);
 	assert(pkey_reg == shadow_pkey_reg);

--
Thiago Jung Bauermann
IBM Linux Technology Center

^ permalink raw reply

* Re: [PATCH 2/2] smp: introduce kick_active_cpus_sync()
From: Steven Rostedt @ 2018-03-26 18:57 UTC (permalink / raw)
  To: Andrea Parri
  Cc: Yury Norov, Paul E. McKenney, Chris Metcalf, Christopher Lameter,
	Russell King - ARM Linux, Mark Rutland, Mathieu Desnoyers,
	Catalin Marinas, Will Deacon, Pekka Enberg, David Rientjes,
	Joonsoo Kim, Andrew Morton, Benjamin Herrenschmidt,
	Paul Mackerras, Michael Ellerman, linux-arm-kernel, linuxppc-dev,
	kvm-ppc, linux-mm, linux-kernel
In-Reply-To: <20180326085313.GA4016@andrea>

On Mon, 26 Mar 2018 10:53:13 +0200
Andrea Parri <andrea.parri@amarulasolutions.com> wrote:

> > --- a/kernel/smp.c
> > +++ b/kernel/smp.c
> > @@ -724,6 +724,30 @@ void kick_all_cpus_sync(void)
> >  }
> >  EXPORT_SYMBOL_GPL(kick_all_cpus_sync);
> >  
> > +/**
> > + * kick_active_cpus_sync - Force CPUs that are not in extended
> > + * quiescent state (idle or nohz_full userspace) sync by sending
> > + * IPI. Extended quiescent state CPUs will sync at the exit of
> > + * that state.
> > + */
> > +void kick_active_cpus_sync(void)
> > +{
> > +	int cpu;
> > +	struct cpumask kernel_cpus;
> > +
> > +	smp_mb();  
> 
> (A general remark only:)
> 
> checkpatch.pl should have warned about the fact that this barrier is
> missing an accompanying comment (which accesses are being "ordered",
> what is the pairing barrier, etc.).

He could have simply copied the comment above the smp_mb() for
kick_all_cpus_sync():

	/* Make sure the change is visible before we kick the cpus */

The kick itself is pretty much a synchronization primitive.

That is, you make some changes and then you need all CPUs to see it,
and you call: kick_active_cpus_synch(), which is the barrier to make
sure you previous changes are seen on all CPUS before you proceed
further. Note, the matching barrier is implicit in the IPI itself.

-- Steve


> 
> Moreover if, as your reply above suggested, your patch is relying on
> "implicit barriers" (something I would not recommend) then even more
> so you should comment on these requirements.
> 
> This could: (a) force you to reason about the memory ordering stuff,
> (b) easy the task of reviewing and adopting your patch, (c) easy the
> task of preserving those requirements (as implementations changes).
> 
>   Andrea
> 
> 
> > +
> > +	cpumask_clear(&kernel_cpus);
> > +	preempt_disable();
> > +	for_each_online_cpu(cpu) {
> > +		if (!rcu_eqs_special_set(cpu))
> > +			cpumask_set_cpu(cpu, &kernel_cpus);
> > +	}
> > +	smp_call_function_many(&kernel_cpus, do_nothing, NULL, 1);
> > +	preempt_enable();
> > +}
> > +EXPORT_SYMBOL_GPL(kick_active_cpus_sync);
> > +
> >  /**
> >   * wake_up_all_idle_cpus - break all cpus out of idle
> >   * wake_up_all_idle_cpus try to break all cpus which is in idle state even
> > diff --git a/mm/slab.c b/mm/slab.c
> > index 324446621b3e..678d5dbd6f46 100644
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -3856,7 +3856,7 @@ static int __do_tune_cpucache(struct kmem_cache *cachep, int limit,
> >  	 * cpus, so skip the IPIs.
> >  	 */
> >  	if (prev)
> > -		kick_all_cpus_sync();
> > +		kick_active_cpus_sync();
> >  
> >  	check_irq_on();
> >  	cachep->batchcount = batchcount;
> > -- 
> > 2.14.1
> >   

^ permalink raw reply

* Re: [PATCH 17/21] powerpc: Add missing prototype for sys_debug_setcontext
From: Mathieu Malaterre @ 2018-03-26 18:56 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Benjamin Herrenschmidt, Paul Mackerras, Jiri Slaby, linuxppc-dev,
	LKML, Al Viro
In-Reply-To: <878tb2etva.fsf@concordia.ellerman.id.au>

Michael,

On Thu, Mar 8, 2018 at 11:36 AM, Michael Ellerman <mpe@ellerman.id.au> wrot=
e:
> Mathieu Malaterre <malat@debian.org> writes:
>
>> On Sun, Mar 4, 2018 at 11:54 AM, Michael Ellerman <mpe@ellerman.id.au> w=
rote:
>>> Mathieu Malaterre <malat@debian.org> writes:
>>>
>>>> In commit 81e7009ea46c ("powerpc: merge ppc signal.c and ppc64 signal3=
2.c")
>>>> the function sys_debug_setcontext was added without a prototype.
>>>>
>>>> Fix compilation warning (treated as error in W=3D1):
>>>>
>>>>   CC      arch/powerpc/kernel/signal_32.o
>>>> arch/powerpc/kernel/signal_32.c:1227:5: error: no previous prototype f=
or =E2=80=98sys_debug_setcontext=E2=80=99 [-Werror=3Dmissing-prototypes]
>>>>  int sys_debug_setcontext(struct ucontext __user *ctx,
>>>>      ^~~~~~~~~~~~~~~~~~~~
>>>> cc1: all warnings being treated as errors
>>>
>>> This one should actually be using the SYSCALL_DEFINE syntax, so that it
>>> can be used with CONFIG_FTRACE_SYSCALLS.
>>>
>>> See eg. our mmap:
>>>
>>>   SYSCALL_DEFINE6(mmap, unsigned long, addr, size_t, len,
>>>                 unsigned long, prot, unsigned long, flags,
>>>                 unsigned long, fd, off_t, offset)
>>>   {
>>>         return do_mmap2(addr, len, prot, flags, fd, offset, PAGE_SHIFT)=
;
>>>   }
>>>
>>>
>>> We probably still need this patch, but I'm not entirely sure because th=
e
>>> SYSCALL_DEFINE macro does all sorts of shenanigans.
>>
>> I see. Could you please drop this patch then. The patch does not look
>> that trivial anymore. I'll need to dig a bit more on how to do the
>> syscall stuff with a 7 params function.
>
> Ergh, yuck, seems we're the first suckers to need do that.
>
> I think I'll take this patch for now, it's still good for now at least,
> and then the SYSCALL_DEFINE stuff can be an addition.

Just to close the loop (for later reference). Here is what Al Viro
told me about this function

[begin quote]
int sys_debug_setcontext(struct ucontext __user *ctx,
                         int ndbg, struct sig_dbg_op __user *dbg,
                         int r6, int r7, int r8,
                         struct pt_regs *regs)

can't be converted to SYSCALL_DEFINE... not because it's a 7-argument
syscall (it isn't); the problem is that the last argument here does
*not* come from userland.  What it really is trying to be is
3-argument syscall:
SYSCALL_DEFINE3(debug_setcontext, struct ucontext __user *, ctx,
                                  int, ndbg,
                                  struct sig_dbg_op __user *, dbg)
with no dummy r6/r7/r8.  The thing is, ppc dispatcher combines the
"pass 6 arguments" and "pass pt_regs *" by passing both.

debug_setcontext(), swapcontext(), sigreturn() and rt_sigreturn() are,
AFAICS, the only ppc syscalls that make use of that argument.  All of
those are relying upon regs =3D=3D current_pt_regs() =3D=3D current->thread=
.regs,
[...]
But in any case, these are not 7-argument syscalls - debug_setcontext(2)
and swapcontext(2) are 3-argument and sigreturn()/rt_sigreturn() have
no arguments at all.
[end quote]

Sorry about the wrong analysis in the number of arguments count. I
believe commit 0d60619e1c0ca (powerpc/next) should be just fine for
now.

2cts

^ permalink raw reply

* [PATCH] powerpc: Clear branch trap (MSR.BE) before delivering SIGTRAP
From: Matt Evans @ 2018-03-26 16:55 UTC (permalink / raw)
  To: linuxppc-dev

When using SIG_DBG_BRANCH_TRACING, MSR.BE is left enabled in the
user context when single_step_exception() prepares the SIGTRAP
delivery.  The resulting branch-trap-within-the-SIGTRAP-handler
isn't healthy.

Commit 2538c2d08f46141550a1e68819efa8fe31c6e3dc broke this, by
replacing an MSR mask operation of ~(MSR_SE | MSR_BE) with a call
to clear_single_step() which only clears MSR_SE.

This patch adds a new helper, clear_br_trace(), which clears the
debug trap before invoking the signal handler.  This helper is a
NOP for BookE as SIG_DBG_BRANCH_TRACING isn't supported on BookE.

Signed-off-by: Matt Evans <matt@ozlabs.org>
---
 arch/powerpc/kernel/traps.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 1e48d157196a..5eaab234e747 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -460,7 +460,7 @@ static inline int check_io_access(struct pt_regs *regs)
 /* single-step stuff */
 #define single_stepping(regs)	(current->thread.debug.dbcr0 & DBCR0_IC)
 #define clear_single_step(regs)	(current->thread.debug.dbcr0 &= ~DBCR0_IC)
-
+#define clear_br_trace(regs)	do {} while(0)
 #else
 /* On non-4xx, the reason for the machine check or program
    exception is in the MSR. */
@@ -473,6 +473,7 @@ static inline int check_io_access(struct pt_regs *regs)
 
 #define single_stepping(regs)	((regs)->msr & MSR_SE)
 #define clear_single_step(regs)	((regs)->msr &= ~MSR_SE)
+#define clear_br_trace(regs)	((regs)->msr &= ~MSR_BE)
 #endif
 
 #if defined(CONFIG_E500)
@@ -988,6 +989,7 @@ void single_step_exception(struct pt_regs *regs)
 	enum ctx_state prev_state = exception_enter();
 
 	clear_single_step(regs);
+	clear_br_trace(regs);
 
 	if (kprobe_post_handler(regs))
 		return;
-- 
2.14.1

^ permalink raw reply related

* Re: RFC on writel and writel_relaxed
From: Jason Gunthorpe @ 2018-03-26 16:54 UTC (permalink / raw)
  To: David Laight
  Cc: 'Benjamin Herrenschmidt', Sinan Kaya,
	open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), Oliver,
	linux-rdma@vger.kernel.org
In-Reply-To: <58ce5b83f40f4775bec1be8db66adb0d@AcuMS.aculab.com>

On Mon, Mar 26, 2018 at 11:08:45AM +0000, David Laight wrote:
> > > This is a super performance critical operation for most drivers and
> > > directly impacts network performance.
> 
> Perhaps there ought to be writel_nobarrier() (etc) that never contain
> any barriers at all.
> This might mean that they are always just the memory operation,
> but it would make it more obvious what the driver was doing.

I think that is what writel_relaxed is supposed to be.

The only restriction it has is that the writes to a single device
using UC memory must be kept in program order..

Jason

^ permalink raw reply

* [PATCH v3 41/41] cxlflash: Handle spurious interrupts
From: Uma Krishnan @ 2018-03-26 16:35 UTC (permalink / raw)
  To: linux-scsi, James Bottomley, Martin K. Petersen, Matthew R. Ochs,
	Manoj N. Kumar
  Cc: linuxppc-dev, Andrew Donnellan, Frederic Barrat,
	Christophe Lombard
In-Reply-To: <1522081759-57431-1-git-send-email-ukrishn@linux.vnet.ibm.com>

The following Oops can occur when there is heavy I/O traffic and the host
is reset by a tool such as sg_reset.

[c000200fff3fbc90] c00800001690117c process_cmd_doneq+0x104/0x500
                                       [cxlflash] (unreliable)
[c000200fff3fbd80] c008000016901648 cxlflash_rrq_irq+0xd0/0x150 [cxlflash]
[c000200fff3fbde0] c000000000193130 __handle_irq_event_percpu+0xa0/0x310
[c000200fff3fbea0] c0000000001933d8 handle_irq_event_percpu+0x38/0x90
[c000200fff3fbee0] c000000000193494 handle_irq_event+0x64/0xb0
[c000200fff3fbf10] c000000000198ea0 handle_fasteoi_irq+0xc0/0x230
[c000200fff3fbf40] c00000000019182c generic_handle_irq+0x4c/0x70
[c000200fff3fbf60] c00000000001794c __do_irq+0x7c/0x1c0
[c000200fff3fbf90] c00000000002a390 call_do_irq+0x14/0x24
[c000200e5828fab0] c000000000017b2c do_IRQ+0x9c/0x130
[c000200e5828fb00] c000000000009b04 h_virt_irq_common+0x114/0x120

When a context is reset, the pending commands are flushed and the AFU
is notified. Before the AFU handles this request there could be command
completion interrupts queued to PHB which are yet to be delivered to the
context. In this scenario, a context could receive an interrupt for a
command that has been flushed, leading to a possible crash when the memory
for the flushed command is accessed.

To resolve this problem, a boolean will indicate if the hardware queue is
ready to process interrupts or not. This can be evaluated in the interrupt
handler before proessing an interrupt.

Signed-off-by: Uma Krishnan <ukrishn@linux.vnet.ibm.com>
---
 drivers/scsi/cxlflash/common.h |  1 +
 drivers/scsi/cxlflash/main.c   | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/scsi/cxlflash/common.h b/drivers/scsi/cxlflash/common.h
index b69fd32..3556b1d 100644
--- a/drivers/scsi/cxlflash/common.h
+++ b/drivers/scsi/cxlflash/common.h
@@ -224,6 +224,7 @@ struct hwq {
 	u64 *hrrq_end;
 	u64 *hrrq_curr;
 	bool toggle;
+	bool hrrq_online;
 
 	s64 room;
 
diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
index c920328..a24d7e6 100644
--- a/drivers/scsi/cxlflash/main.c
+++ b/drivers/scsi/cxlflash/main.c
@@ -801,6 +801,10 @@ static void term_mc(struct cxlflash_cfg *cfg, u32 index)
 		WARN_ON(cfg->ops->release_context(hwq->ctx_cookie));
 	hwq->ctx_cookie = NULL;
 
+	spin_lock_irqsave(&hwq->hrrq_slock, lock_flags);
+	hwq->hrrq_online = false;
+	spin_unlock_irqrestore(&hwq->hrrq_slock, lock_flags);
+
 	spin_lock_irqsave(&hwq->hsq_slock, lock_flags);
 	flush_pending_cmds(hwq);
 	spin_unlock_irqrestore(&hwq->hsq_slock, lock_flags);
@@ -1475,6 +1479,12 @@ static irqreturn_t cxlflash_rrq_irq(int irq, void *data)
 
 	spin_lock_irqsave(&hwq->hrrq_slock, hrrq_flags);
 
+	/* Silently drop spurious interrupts when queue is not online */
+	if (!hwq->hrrq_online) {
+		spin_unlock_irqrestore(&hwq->hrrq_slock, hrrq_flags);
+		return IRQ_HANDLED;
+	}
+
 	if (afu_is_irqpoll_enabled(afu)) {
 		irq_poll_sched(&hwq->irqpoll);
 		spin_unlock_irqrestore(&hwq->hrrq_slock, hrrq_flags);
@@ -1781,6 +1791,7 @@ static int init_global(struct cxlflash_cfg *cfg)
 
 		writeq_be((u64) hwq->hrrq_start, &hmap->rrq_start);
 		writeq_be((u64) hwq->hrrq_end, &hmap->rrq_end);
+		hwq->hrrq_online = true;
 
 		if (afu_is_sq_cmd_mode(afu)) {
 			writeq_be((u64)hwq->hsq_start, &hmap->sq_start);
-- 
2.1.0

^ permalink raw reply related

* [PATCH v3 40/41] cxlflash: Remove commmands from pending list on timeout
From: Uma Krishnan @ 2018-03-26 16:35 UTC (permalink / raw)
  To: linux-scsi, James Bottomley, Martin K. Petersen, Matthew R. Ochs,
	Manoj N. Kumar
  Cc: linuxppc-dev, Andrew Donnellan, Frederic Barrat,
	Christophe Lombard
In-Reply-To: <1522081759-57431-1-git-send-email-ukrishn@linux.vnet.ibm.com>

The following Oops can occur if an internal command sent to the AFU does
not complete within the timeout:

[c000000ff101b810] c008000016020d94 term_mc+0xfc/0x1b0 [cxlflash]
[c000000ff101b8a0] c008000016020fb0 term_afu+0x168/0x280 [cxlflash]
[c000000ff101b930] c0080000160232ec cxlflash_pci_error_detected+0x184/0x230
                                       [cxlflash]
[c000000ff101b9e0] c00800000d95d468 cxl_vphb_error_detected+0x90/0x150[cxl]
[c000000ff101ba20] c00800000d95f27c cxl_pci_error_detected+0xa4/0x240 [cxl]
[c000000ff101bac0] c00000000003eaf8 eeh_report_error+0xd8/0x1b0
[c000000ff101bb20] c00000000003d0b8 eeh_pe_dev_traverse+0x98/0x170
[c000000ff101bbb0] c00000000003f438 eeh_handle_normal_event+0x198/0x580
[c000000ff101bc60] c00000000003fba4 eeh_handle_event+0x2a4/0x338
[c000000ff101bd10] c0000000000400b8 eeh_event_handler+0x1f8/0x200
[c000000ff101bdc0] c00000000013da48 kthread+0x1a8/0x1b0
[c000000ff101be30] c00000000000b528 ret_from_kernel_thread+0x5c/0xb4

When an internal command times out, the command buffer is freed while it
is still in the pending commands list of the context. This corrupts the
list and when the context is cleaned up, a crash is encountered.

To resolve this issue, when an AFU command or TMF command times out, the
command should be deleted from the hardware queue pending command list
before freeing the buffer.

Signed-off-by: Uma Krishnan <ukrishn@linux.vnet.ibm.com>
---
 drivers/scsi/cxlflash/main.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
index dfe7648..c920328 100644
--- a/drivers/scsi/cxlflash/main.c
+++ b/drivers/scsi/cxlflash/main.c
@@ -473,6 +473,7 @@ static int send_tmf(struct cxlflash_cfg *cfg, struct scsi_device *sdev,
 	struct afu_cmd *cmd = NULL;
 	struct device *dev = &cfg->dev->dev;
 	struct hwq *hwq = get_hwq(afu, PRIMARY_HWQ);
+	bool needs_deletion = false;
 	char *buf = NULL;
 	ulong lock_flags;
 	int rc = 0;
@@ -527,6 +528,7 @@ static int send_tmf(struct cxlflash_cfg *cfg, struct scsi_device *sdev,
 	if (!to) {
 		dev_err(dev, "%s: TMF timed out\n", __func__);
 		rc = -ETIMEDOUT;
+		needs_deletion = true;
 	} else if (cmd->cmd_aborted) {
 		dev_err(dev, "%s: TMF aborted\n", __func__);
 		rc = -EAGAIN;
@@ -537,6 +539,12 @@ static int send_tmf(struct cxlflash_cfg *cfg, struct scsi_device *sdev,
 	}
 	cfg->tmf_active = false;
 	spin_unlock_irqrestore(&cfg->tmf_slock, lock_flags);
+
+	if (needs_deletion) {
+		spin_lock_irqsave(&hwq->hsq_slock, lock_flags);
+		list_del(&cmd->list);
+		spin_unlock_irqrestore(&hwq->hsq_slock, lock_flags);
+	}
 out:
 	kfree(buf);
 	return rc;
@@ -2284,6 +2292,7 @@ static int send_afu_cmd(struct afu *afu, struct sisl_ioarcb *rcb)
 	struct device *dev = &cfg->dev->dev;
 	struct afu_cmd *cmd = NULL;
 	struct hwq *hwq = get_hwq(afu, PRIMARY_HWQ);
+	ulong lock_flags;
 	char *buf = NULL;
 	int rc = 0;
 	int nretry = 0;
@@ -2329,6 +2338,11 @@ static int send_afu_cmd(struct afu *afu, struct sisl_ioarcb *rcb)
 	case -ETIMEDOUT:
 		rc = afu->context_reset(hwq);
 		if (rc) {
+			/* Delete the command from pending_cmds list */
+			spin_lock_irqsave(&hwq->hsq_slock, lock_flags);
+			list_del(&cmd->list);
+			spin_unlock_irqrestore(&hwq->hsq_slock, lock_flags);
+
 			cxlflash_schedule_async_reset(cfg);
 			break;
 		}
-- 
2.1.0

^ permalink raw reply related

* [PATCH v3 39/41] cxlflash: Synchronize reset and remove ops
From: Uma Krishnan @ 2018-03-26 16:35 UTC (permalink / raw)
  To: linux-scsi, James Bottomley, Martin K. Petersen, Matthew R. Ochs,
	Manoj N. Kumar
  Cc: linuxppc-dev, Andrew Donnellan, Frederic Barrat,
	Christophe Lombard
In-Reply-To: <1522081759-57431-1-git-send-email-ukrishn@linux.vnet.ibm.com>

The following Oops can be encountered if a device removal or system
shutdown is initiated while an EEH recovery is in process:

[c000000ff2f479c0] c008000015256f18 cxlflash_pci_slot_reset+0xa0/0x100
                                      [cxlflash]
[c000000ff2f47a30] c00800000dae22e0 cxl_pci_slot_reset+0x168/0x290 [cxl]
[c000000ff2f47ae0] c00000000003ef1c eeh_report_reset+0xec/0x170
[c000000ff2f47b20] c00000000003d0b8 eeh_pe_dev_traverse+0x98/0x170
[c000000ff2f47bb0] c00000000003f80c eeh_handle_normal_event+0x56c/0x580
[c000000ff2f47c60] c00000000003fba4 eeh_handle_event+0x2a4/0x338
[c000000ff2f47d10] c0000000000400b8 eeh_event_handler+0x1f8/0x200
[c000000ff2f47dc0] c00000000013da48 kthread+0x1a8/0x1b0
[c000000ff2f47e30] c00000000000b528 ret_from_kernel_thread+0x5c/0xb4

The remove handler frees AFU memory while the EEH recovery is in progress,
leading to a race condition. This can result in a crash if the recovery
thread tries to access this memory.

To resolve this issue, the cxlflash remove handler will evaluate the
device state and yield to any active reset or probing threads.

Signed-off-by: Uma Krishnan <ukrishn@linux.vnet.ibm.com>
---
 drivers/scsi/cxlflash/main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
index 42a95b7..dfe7648 100644
--- a/drivers/scsi/cxlflash/main.c
+++ b/drivers/scsi/cxlflash/main.c
@@ -946,9 +946,9 @@ static void cxlflash_remove(struct pci_dev *pdev)
 		return;
 	}
 
-	/* If a Task Management Function is active, wait for it to complete
-	 * before continuing with remove.
-	 */
+	/* Yield to running recovery threads before continuing with remove */
+	wait_event(cfg->reset_waitq, cfg->state != STATE_RESET &&
+				     cfg->state != STATE_PROBING);
 	spin_lock_irqsave(&cfg->tmf_slock, lock_flags);
 	if (cfg->tmf_active)
 		wait_event_interruptible_lock_irq(cfg->tmf_waitq,
-- 
2.1.0

^ permalink raw reply related

* [PATCH v3 38/41] cxlflash: Enable OCXL operations
From: Uma Krishnan @ 2018-03-26 16:35 UTC (permalink / raw)
  To: linux-scsi, James Bottomley, Martin K. Petersen, Matthew R. Ochs,
	Manoj N. Kumar
  Cc: linuxppc-dev, Andrew Donnellan, Frederic Barrat,
	Christophe Lombard
In-Reply-To: <1522081759-57431-1-git-send-email-ukrishn@linux.vnet.ibm.com>

This commit enables the OCXL operations for the OCXL devices.

Signed-off-by: Uma Krishnan <ukrishn@linux.vnet.ibm.com>
Acked-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
---
 drivers/scsi/cxlflash/main.c | 9 +++++++--
 drivers/scsi/cxlflash/main.h | 1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
index 8c55fcd..42a95b7 100644
--- a/drivers/scsi/cxlflash/main.c
+++ b/drivers/scsi/cxlflash/main.c
@@ -3168,7 +3168,8 @@ static struct dev_dependent_vals dev_corsa_vals = { CXLFLASH_MAX_SECTORS,
 static struct dev_dependent_vals dev_flash_gt_vals = { CXLFLASH_MAX_SECTORS,
 					CXLFLASH_NOTIFY_SHUTDOWN };
 static struct dev_dependent_vals dev_briard_vals = { CXLFLASH_MAX_SECTORS,
-					CXLFLASH_NOTIFY_SHUTDOWN };
+					(CXLFLASH_NOTIFY_SHUTDOWN |
+					CXLFLASH_OCXL_DEV) };
 
 /*
  * PCI device binding table
@@ -3679,9 +3680,13 @@ static int cxlflash_probe(struct pci_dev *pdev,
 
 	cfg->init_state = INIT_STATE_NONE;
 	cfg->dev = pdev;
-	cfg->ops = &cxlflash_cxl_ops;
 	cfg->cxl_fops = cxlflash_cxl_fops;
 
+	if (ddv->flags & CXLFLASH_OCXL_DEV)
+		cfg->ops = &cxlflash_ocxl_ops;
+	else
+		cfg->ops = &cxlflash_cxl_ops;
+
 	/*
 	 * Promoted LUNs move to the top of the LUN table. The rest stay on
 	 * the bottom half. The bottom half grows from the end (index = 255),
diff --git a/drivers/scsi/cxlflash/main.h b/drivers/scsi/cxlflash/main.h
index ba0108a..6f1be62 100644
--- a/drivers/scsi/cxlflash/main.h
+++ b/drivers/scsi/cxlflash/main.h
@@ -97,6 +97,7 @@ struct dev_dependent_vals {
 	u64 flags;
 #define CXLFLASH_NOTIFY_SHUTDOWN	0x0000000000000001ULL
 #define CXLFLASH_WWPN_VPD_REQUIRED	0x0000000000000002ULL
+#define CXLFLASH_OCXL_DEV		0x0000000000000004ULL
 };
 
 struct asyc_intr_info {
-- 
2.1.0

^ permalink raw reply related

* [PATCH v3 37/41] cxlflash: Support AFU reset
From: Uma Krishnan @ 2018-03-26 16:35 UTC (permalink / raw)
  To: linux-scsi, James Bottomley, Martin K. Petersen, Matthew R. Ochs,
	Manoj N. Kumar
  Cc: linuxppc-dev, Andrew Donnellan, Frederic Barrat,
	Christophe Lombard
In-Reply-To: <1522081759-57431-1-git-send-email-ukrishn@linux.vnet.ibm.com>

The cxlflash core driver resets the AFU when the master contexts are
created in the initialization or recovery paths. Today, the OCXL
provider service to perform this operation is pending implementation.
To avoid a crash due to a missing fop, log an error once and return
success to continue with execution.

Signed-off-by: Uma Krishnan <ukrishn@linux.vnet.ibm.com>
Acked-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
---
 drivers/scsi/cxlflash/ocxl_hw.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/scsi/cxlflash/ocxl_hw.c b/drivers/scsi/cxlflash/ocxl_hw.c
index 5b5565d..0a95b5f 100644
--- a/drivers/scsi/cxlflash/ocxl_hw.c
+++ b/drivers/scsi/cxlflash/ocxl_hw.c
@@ -468,6 +468,22 @@ static int ocxlflash_stop_context(void *ctx_cookie)
 }
 
 /**
+ * ocxlflash_afu_reset() - reset the AFU
+ * @ctx_cookie:	Adapter context.
+ */
+static int ocxlflash_afu_reset(void *ctx_cookie)
+{
+	struct ocxlflash_context *ctx = ctx_cookie;
+	struct device *dev = ctx->hw_afu->dev;
+
+	/* Pending implementation from OCXL transport services */
+	dev_err_once(dev, "%s: afu_reset() fop not supported\n", __func__);
+
+	/* Silently return success until it is implemented */
+	return 0;
+}
+
+/**
  * ocxlflash_set_master() - sets the context as master
  * @ctx_cookie:	Adapter context to set as master.
  */
@@ -1401,6 +1417,7 @@ const struct cxlflash_backend_ops cxlflash_ocxl_ops = {
 	.get_irq_objhndl	= ocxlflash_get_irq_objhndl,
 	.start_context		= ocxlflash_start_context,
 	.stop_context		= ocxlflash_stop_context,
+	.afu_reset		= ocxlflash_afu_reset,
 	.set_master		= ocxlflash_set_master,
 	.get_context		= ocxlflash_get_context,
 	.dev_context_init	= ocxlflash_dev_context_init,
-- 
2.1.0

^ permalink raw reply related

* [PATCH v3 36/41] cxlflash: Register for translation errors
From: Uma Krishnan @ 2018-03-26 16:35 UTC (permalink / raw)
  To: linux-scsi, James Bottomley, Martin K. Petersen, Matthew R. Ochs,
	Manoj N. Kumar
  Cc: linuxppc-dev, Andrew Donnellan, Frederic Barrat,
	Christophe Lombard
In-Reply-To: <1522081759-57431-1-git-send-email-ukrishn@linux.vnet.ibm.com>

While enabling a context on the link, a predefined callback can be
registered with the OCXL provider services to be notified on translation
errors. These errors can in turn be passed back to the user on a read
operation.

Signed-off-by: Uma Krishnan <ukrishn@linux.vnet.ibm.com>
Acked-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
---
 drivers/scsi/cxlflash/ocxl_hw.c | 31 +++++++++++++++++++++++++++++--
 drivers/scsi/cxlflash/ocxl_hw.h |  4 ++++
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/cxlflash/ocxl_hw.c b/drivers/scsi/cxlflash/ocxl_hw.c
index 89d3d89..5b5565d 100644
--- a/drivers/scsi/cxlflash/ocxl_hw.c
+++ b/drivers/scsi/cxlflash/ocxl_hw.c
@@ -335,6 +335,25 @@ static u64 ocxlflash_get_irq_objhndl(void *ctx_cookie, int irq)
 }
 
 /**
+ * ocxlflash_xsl_fault() - callback when translation error is triggered
+ * @data:	Private data provided at callback registration, the context.
+ * @addr:	Address that triggered the error.
+ * @dsisr:	Value of dsisr register.
+ */
+static void ocxlflash_xsl_fault(void *data, u64 addr, u64 dsisr)
+{
+	struct ocxlflash_context *ctx = data;
+
+	spin_lock(&ctx->slock);
+	ctx->fault_addr = addr;
+	ctx->fault_dsisr = dsisr;
+	ctx->pending_fault = true;
+	spin_unlock(&ctx->slock);
+
+	wake_up_all(&ctx->wq);
+}
+
+/**
  * start_context() - local routine to start a context
  * @ctx:	Adapter context to be started.
  *
@@ -378,7 +397,8 @@ static int start_context(struct ocxlflash_context *ctx)
 		mm = current->mm;
 	}
 
-	rc = ocxl_link_add_pe(link_token, ctx->pe, pid, 0, 0, mm, NULL, NULL);
+	rc = ocxl_link_add_pe(link_token, ctx->pe, pid, 0, 0, mm,
+			      ocxlflash_xsl_fault, ctx);
 	if (unlikely(rc)) {
 		dev_err(dev, "%s: ocxl_link_add_pe failed rc=%d\n",
 			__func__, rc);
@@ -512,6 +532,7 @@ static void *ocxlflash_dev_context_init(struct pci_dev *pdev, void *afu_cookie)
 	ctx->hw_afu = afu;
 	ctx->irq_bitmap = 0;
 	ctx->pending_irq = false;
+	ctx->pending_fault = false;
 out:
 	return ctx;
 err2:
@@ -965,7 +986,7 @@ static void *ocxlflash_create_afu(struct pci_dev *pdev)
  */
 static inline bool ctx_event_pending(struct ocxlflash_context *ctx)
 {
-	if (ctx->pending_irq)
+	if (ctx->pending_irq || ctx->pending_fault)
 		return true;
 
 	return false;
@@ -1070,6 +1091,12 @@ static ssize_t afu_read(struct file *file, char __user *buf, size_t count,
 		event.irq.irq = bit + 1;
 		if (bitmap_empty(&ctx->irq_bitmap, ctx->num_irqs))
 			ctx->pending_irq = false;
+	} else if (ctx->pending_fault) {
+		event.header.size += sizeof(struct cxl_event_data_storage);
+		event.header.type = CXL_EVENT_DATA_STORAGE;
+		event.fault.addr = ctx->fault_addr;
+		event.fault.dsisr = ctx->fault_dsisr;
+		ctx->pending_fault = false;
 	}
 
 	spin_unlock_irqrestore(&ctx->slock, lock_flags);
diff --git a/drivers/scsi/cxlflash/ocxl_hw.h b/drivers/scsi/cxlflash/ocxl_hw.h
index 1829e55..9270d35 100644
--- a/drivers/scsi/cxlflash/ocxl_hw.h
+++ b/drivers/scsi/cxlflash/ocxl_hw.h
@@ -70,4 +70,8 @@ struct ocxlflash_context {
 	int num_irqs;			/* Number of interrupts */
 	bool pending_irq;		/* Pending interrupt on the context */
 	ulong irq_bitmap;		/* Bits indicating pending irq num */
+
+	u64 fault_addr;			/* Address that triggered the fault */
+	u64 fault_dsisr;		/* Value of dsisr register at fault */
+	bool pending_fault;		/* Pending translation fault */
 };
-- 
2.1.0

^ 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