Re: [PATCH v2 2/4] powerpc/sstep: support emulation for vsx vector paired storage access instructions

[Ravi Bangoria <ravi.bangoria@linux.ibm.com>]

Hi Bala,

> @@ -709,6 +722,8 @@ void emulate_vsx_load(struct instruction_op *op, union vsx_reg *reg,
>   	reg->d[0] = reg->d[1] = 0;
>   
>   	switch (op->element_size) {
> +	case 32:
> +		/* [p]lxvp[x] or [p]stxvp[x] */

This function does not emulate stvxp ^^^^

>   	case 16:
>   		/* whole vector; lxv[x] or lxvl[l] */
>   		if (size == 0)
> @@ -717,7 +732,7 @@ void emulate_vsx_load(struct instruction_op *op, union vsx_reg *reg,
>   		if (IS_LE && (op->vsx_flags & VSX_LDLEFT))
>   			rev = !rev;
>   		if (rev)
> -			do_byte_reverse(reg, 16);
> +			do_byte_reverse(reg, size);
>   		break;
>   	case 8:
>   		/* scalar loads, lxvd2x, lxvdsx */
> @@ -793,6 +808,22 @@ void emulate_vsx_store(struct instruction_op *op, const union vsx_reg *reg,
>   	size = GETSIZE(op->type);
>   
>   	switch (op->element_size) {
> +	case 32:
> +		/* [p]lxvp[x] or [p]stxvp[x] */

This function does not emulate lxvp ^^^^

> +		if (size == 0)
> +			break;
> +		if (IS_LE && (op->vsx_flags & VSX_LDLEFT))
> +			rev = !rev;

Why this if condition ^^^^ ?

> +		if (rev) {
> +			/* reverse 32 bytes */
> +			buf.d[0] = byterev_8(reg->d[3]);
> +			buf.d[1] = byterev_8(reg->d[2]);
> +			buf.d[2] = byterev_8(reg->d[1]);
> +			buf.d[3] = byterev_8(reg->d[0]);
> +			reg = &buf;
> +		}
> +		memcpy(mem, reg, size);
> +		break;
>   	case 16:
>   		/* stxv, stxvx, stxvl, stxvll */
>   		if (size == 0)
> @@ -861,28 +892,33 @@ static nokprobe_inline int do_vsx_load(struct instruction_op *op,
>   				       bool cross_endian)
>   {
>   	int reg = op->reg;
> -	u8 mem[16];
> +	int i, nr_vsx_regs;
> +	u8 mem[32];
>   	union vsx_reg buf;
>   	int size = GETSIZE(op->type);
>   
>   	if (!address_ok(regs, ea, size) || copy_mem_in(mem, ea, size, regs))
>   		return -EFAULT;
>   
> +	nr_vsx_regs = size / sizeof(__vector128);
>   	emulate_vsx_load(op, &buf, mem, cross_endian);
>   	preempt_disable();
>   	if (reg < 32) {
>   		/* FP regs + extensions */
>   		if (regs->msr & MSR_FP) {
> -			load_vsrn(reg, &buf);
> +			for (i = 0; i < nr_vsx_regs; i++)
> +				load_vsrn(reg + i, &buf.v[i]);
>   		} else {
>   			current->thread.fp_state.fpr[reg][0] = buf.d[0];
>   			current->thread.fp_state.fpr[reg][1] = buf.d[1];

Should we change else part as well?

>   		}
>   	} else {
>   		if (regs->msr & MSR_VEC)
> -			load_vsrn(reg, &buf);
> +			for (i = 0; i < nr_vsx_regs; i++)
> +				load_vsrn(reg + i, &buf.v[i]);
> +

Unnecessary line.

>   		else
> -			current->thread.vr_state.vr[reg - 32] = buf.v;
> +			current->thread.vr_state.vr[reg - 32] = buf.v[0];

Same here. else part, should we add:

     if (vsx 32 byte)
         current->thread.vr_state.vr[reg - 32 + 1] = buf.v[1];

>   	}
>   	preempt_enable();
>   	return 0;
> @@ -893,27 +929,31 @@ static nokprobe_inline int do_vsx_store(struct instruction_op *op,
>   					bool cross_endian)
>   {
>   	int reg = op->reg;
> -	u8 mem[16];
> +	int i, nr_vsx_regs;
> +	u8 mem[32];
>   	union vsx_reg buf;
>   	int size = GETSIZE(op->type);
>   
>   	if (!address_ok(regs, ea, size))
>   		return -EFAULT;
>   
> +	nr_vsx_regs = size / sizeof(__vector128);
>   	preempt_disable();
>   	if (reg < 32) {
>   		/* FP regs + extensions */
>   		if (regs->msr & MSR_FP) {
> -			store_vsrn(reg, &buf);
> +			for (i = 0; i < nr_vsx_regs; i++)
> +				store_vsrn(reg + i, &buf.v[i]);
>   		} else {
>   			buf.d[0] = current->thread.fp_state.fpr[reg][0];
>   			buf.d[1] = current->thread.fp_state.fpr[reg][1];
>   		}
>   	} else {
>   		if (regs->msr & MSR_VEC)
> -			store_vsrn(reg, &buf);
> +			for (i = 0; i < nr_vsx_regs; i++)
> +				store_vsrn(reg + i, &buf.v[i]);
>   		else
> -			buf.v = current->thread.vr_state.vr[reg - 32];
> +			buf.v[0] = current->thread.vr_state.vr[reg - 32];
>   	}
>   	preempt_enable();
>   	emulate_vsx_store(op, &buf, mem, cross_endian);
> 

Ravi