Re: [PATCH] MIPS: MSA: misaligned support

[James Hogan]

Hi Leonid,

On Wed, Mar 18, 2015 at 12:46:51PM -0700, Leonid Yegoshin wrote:
> On 03/18/2015 04:27 AM, James Hogan wrote:
> >
> >> +		break;
> >> +
> >> +	case 3: /* doubleword, no conversion */
> >> +		break;
> > don't you still need to copy the value though?
> 
> Good point! Never test this subroutine yet, HW team still should produce 
> BIG ENDIAN CPU+MSA variant.

P5600 big endian should be usable on Malta (I've ran it before).

Failing that, it should be pretty easy to force qemu to trigger an AdE
exception on all unaligned ld/st in order to test this.

> I will issue V2.
> 
> >
> >> +	}
> >> +}
> >> +#endif
> >> +#endif
> >> +
> >>   static void emulate_load_store_insn(struct pt_regs *regs,
> >>   	void __user *addr, unsigned int __user *pc)
> >>   {
> >> @@ -434,6 +497,10 @@ static void emulate_load_store_insn(struct pt_regs *regs,
> >>   #ifdef	CONFIG_EVA
> >>   	mm_segment_t seg;
> >>   #endif
> >> +#ifdef CONFIG_CPU_HAS_MSA
> >> +	union fpureg msadatabase[2], *msadata;
> >> +	unsigned int func, df, rs, wd;
> >> +#endif
> >>   	origpc = (unsigned long)pc;
> >>   	orig31 = regs->regs[31];
> >>   
> >> @@ -703,6 +770,82 @@ static void emulate_load_store_insn(struct pt_regs *regs,
> >>   			break;
> >>   		return;
> >>   
> >> +#ifdef CONFIG_CPU_HAS_MSA
> >> +	case msa_op:
> >> +		if (cpu_has_mdmx)
> >> +			goto sigill;
> >> +
> >> +		func = insn.msa_mi10_format.func;
> >> +		switch (func) {
> >> +		default:
> >> +			goto sigbus;
> >> +
> >> +		case msa_ld_op:
> >> +		case msa_st_op:
> >> +			;
> >> +		}
> >> +
> >> +		if (!thread_msa_context_live())
> >> +			goto sigbus;
> > Will this ever happen? (I can't see AdE handler enabling interrupts).
> >
> > If the MSA context genuinely isn't live (i.e. it can be considered
> > UNPREDICTABLE), then surely a load operation should still succeed?
> 
> thread_msa_context_live() == check of TIF_MSA_CTX_LIVE == existence of 
> MSA context for thread.
> It differs from MSA is owned by thread, it just says that thread has 
> already initialized MSA.
> 
> Unfortunate choice of function name, I believe.

Right (I mis-read when its cleared when i grepped). Still, that would
make it even harder to hit since lose_fpu wouldn't clear it, and you
already would've taken an MSA disabled exception first.

Anyway, my point was that there's nothing invalid about an unaligned
load being the first MSA instruction. You might use it to load the
initial vector state.

> 
> This is a guard against bad selection of exception priorities in HW... 
> sometime it happens.
> 
> >
> >> +
> >> +		df = insn.msa_mi10_format.df;
> >> +		rs = insn.msa_mi10_format.rs;
> >> +		wd = insn.msa_mi10_format.wd;
> >> +		addr = (unsigned long *)(regs->regs[rs] + (insn.msa_mi10_format.s10 * (1 << df)));
> > "* (1 << df)"?
> > why not just "<< df"?
> >
> >> +		/* align a working space in stack... */
> >> +		msadata = (union fpureg *)(((unsigned long)msadatabase + 15) & ~(unsigned long)0xf);
> > Maybe you could just use __aligned(16) on a single local union fpureg.
> 
> I am not sure that it works in stack. I don't trust toolchain here - 
> they even are not able to align a frame in stack to 16bytes.

I did wonder that, but found the following bug report which seems to
indicate that it was fixed generically in 4.6:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=16660

Unfortunately Linux supports MSA built with a toolchain that doesn't, so
that may not be good enough, I don't know :-(.

Cheers
James

Attachment: signature.asc
Description: Digital signature