Hi Peter, really appreciate your feedback, thank you so much. I'll split the patch into smaller series as you suggested, and keep changes to bare minimum for this fix. I'll create another patch for the refactoring of the helper macros indentation.
Cheers, William On Tue, Jul 1, 2025 at 1:13 AM Peter Maydell <peter.mayd...@linaro.org> wrote: > On Wed, 25 Jun 2025 at 11:59, William Kosasih <kosasihwilli...@gmail.com> > wrote: > > > > Historically, M-profile helper functions in m_helper.c and mve_helper.c > > used the unaligned cpu_*_data_ra() routines to perform guest memory > > accesses. This meant we had no way to enforce alignment constraints > > when executing helper-based loads/stores. With the addition of the > > cpu_*_mmu() APIs, we can now combine the current MMU state > > (cpu_mmu_index(env, false)) with MO_ALIGN flags to build a MemOpIdx > > that enforces alignment at the helper level. > > > > This patch: > > - Replaces all calls to cpu_ldl_data_ra(), cpu_ldst_data_ra(), etc., > > in the M-profile helpers (m_helper.c) and the MVE helpers > > (mve_helper.c) with their cpu_*_mmu() equivalents. > > - Leaves SME and SVE helper code untouched, as those extensions > > support unaligned accesses by design. > > - Retains the manual alignment checks in the vlldm/vlstm helpers > > because those instructions enforce an 8-byte alignment requirement > > (instead of the 4-byte alignment for ordinary long loads/stores). > > References to cpu_*_data_* are still replaced with cpu_*_mmu(), so > > that the individual word accesses themselves also perform the standard > > alignment checks, in keeping with the ARM pseudocode. > > > > With this change, all M-profile and MVE helper-based loads and stores > > will now correctly honor their alignment requirements. > > > > Signed-off-by: William Kosasih <kosasihwilli...@gmail.com> > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1154 > > --- > > target/arm/tcg/m_helper.c | 33 +-- > > target/arm/tcg/mve_helper.c | 408 ++++++++++++++++++++---------------- > > 2 files changed, 254 insertions(+), 187 deletions(-) > > Hi; thanks for doing this work, this is something that it's definitely > nice to see fixed. > > My main comment here is that this patch is really too large > at 400+ lines to review easily. Could you split it up into > a multi-patch series where each patch does one coherent > thing, please? (For instance "honour alignment requirements in > vlldm and vlstm" could be one patch, and so on.) This will > make it easier to review, and also easier to track down any > problems in it by bisecting to the relevant commit if we get > reports of a regression. > > thanks > -- PMM >
