Re: [PATCH v2 07/11] arm64: mops: handle MOPS exceptions

[Kristina Martsenko <kristina.martsenko@arm.com>]

On 25/05/2023 20:50, Colton Lewis wrote:
>> +    if (esr & ESR_ELx_MOPS_ISS_MEM_INST) {
>> +        /* SET* instruction */
>> +        if (option_a ^ wrong_option) {
>> +            /* Format is from Option A; forward set */
>> +            pt_regs_write_reg(regs, dstreg, dst + size);
>> +            pt_regs_write_reg(regs, sizereg, -size);
>> +        }
>> +    } else {
>> +        /* CPY* instruction */
>> +        if (!(option_a ^ wrong_option)) {
>> +            /* Format is from Option B */
>> +            if (regs->pstate & PSR_N_BIT) {
>> +                /* Backward copy */
>> +                pt_regs_write_reg(regs, dstreg, dst - size);
>> +                pt_regs_write_reg(regs, srcreg, src - size);
>> +            }
>> +        } else {
>> +            /* Format is from Option A */
>> +            if (size & BIT(63)) {
>> +                /* Forward copy */
>> +                pt_regs_write_reg(regs, dstreg, dst + size);
>> +                pt_regs_write_reg(regs, srcreg, src + size);
>> +                pt_regs_write_reg(regs, sizereg, -size);
>> +            }
>> +        }
>> +    }
> 
> I can see an argument for styling things closely to the ARM manual as
> you have done here, but Linux style recommends against deep nesting. In
> this case it is unneeded. I believe this can be written as a single
> if-else chain and that makes it easier to distinguish the three options.
> 
> if ((esr & ESR_ELx_MOPS_ISS_MEM_INST) && (option_a ^ wrong_option)) {
>     /* Format is from Option A; forward set */
>     pt_regs_write_reg(regs, dstreg, dst + size);
>     pt_regs_write_reg(regs, sizereg, -size);
> } else if ((option_a ^ wrong_option) && (size & BIT(63)) {
>     /* Forward copy */
>     pt_regs_write_reg(regs, dstreg, dst + size);
>     pt_regs_write_reg(regs, srcreg, src + size);
>     pt_regs_write_reg(regs, sizereg, -size);
> } else if (regs-pstate & PSR_N_BIT) {
>     /* Backward copy */
>     pt_regs_write_reg(regs, dstreg, dst - size);
>     pt_regs_write_reg(regs, srcreg, src - size);
> }

Yeah, the nesting gets a bit deep here, but there are 6 cases in total, ie 6 
ways the hardware can set up the registers and pstate (in 3 of them the kernel
doesn't need to modify the registers), and I think the current structure makes
it clearer what the 6 are, so I'd prefer to keep it as it is for now.

Thanks,
Kristina