[PATCH V5 09/14] powerpc/vas: Update CSB and notify process for fault CRBs

[Michael Neuling]

> > > +
> > > +	csb.cc = CSB_CC_TRANSLATION;
> > > +	csb.ce = CSB_CE_TERMINATION;
> > > +	csb.cs = 0;
> > > +	csb.count = 0;
> > > +
> > > +	/*
> > > +	 * Returns the fault address in CPU format since it is passed with
> > > +	 * signal. But if the user space expects BE format, need changes.
> > > +	 * i.e either kernel (here) or user should convert to CPU format.
> > > +	 * Not both!
> > > +	 */
> > > +	csb.address = be64_to_cpu(crb->stamp.nx.fault_storage_addr);
> > 
> > This looks wrong and I don't understand the comment. You need to convert
> > this
> > back to be64 to write it to csb.address. ie.
> > 
> >   csb.address = cpu_to_be64(be64_to_cpu(crb->stamp.nx.fault_storage_addr));
> > 
> > Which I think you can just avoid the endian conversion all together.
> 
> NX pastes fault CRB in big-endian, so passing this address in CPU format
> to user space, otherwise the library has to convert. 

OK, then please change the definition in struct coprocessor_status_block to just
__u64.

struct coprocessor_status_block {
	u8 flags;
	u8 cs;
	u8 cc;
	u8 ce;
	__be32 count;
	__be64 address;
} __packed __aligned(CSB_ALIGN);

Big but....

I thought "struct coprocessor_status_block" was also written by hardware. If
that's the case then it needs to be __be64 and you need the kernel to synthesize
exactly what the hardware is doing. Hence the struct definition is correct and
the kernel needs to convert to _be64 on writing. 

> What is the standard way for passing to user space? 

CPU endian.

> > > +	 * process will be polling on csb.flags after request is sent to
> > > +	 * NX. So generally CSB update should not fail except when an
> > > +	 * application does not follow the process properly. So an error
> > > +	 * message will be displayed and leave it to user space whether
> > > +	 * to ignore or handle this signal.
> > > +	 */
> > > +	rcu_read_lock();
> > > +	rc = kill_pid_info(SIGSEGV, &info, pid);
> > > +	rcu_read_unlock();
> > 
> > why the rcu_read_un/lock() here?
> 
> Used same as in kill_proc_info()/kill_something_info()

Please document.

Mikey