Messages in this thread | | | Date | Tue, 30 May 2023 17:36:18 +0100 | Subject | Re: [PATCH v2 07/11] arm64: mops: handle MOPS exceptions | From | Kristina Martsenko <> |
| |
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
| |