Hi Alexandru, On 4/25/23 15:00, Alexandru Elisei wrote: > Hi, > > On Mon, Apr 24, 2023 at 10:11:11PM +0200, Eric Auger wrote: >> Hi Alexandru, >> >> On 4/21/23 12:25, Alexandru Elisei wrote: >>> Hi, >>> >>> On Wed, Mar 15, 2023 at 12:07:22PM +0100, Eric Auger wrote: >>>> The mem access loop currently features ISB barriers only. However >>>> the mem_access loop counts the number of accesses to memory. ISB >>>> do not garantee the PE cannot reorder memory access. Let's >>>> add a DSB ISH before the write to PMCR_EL0 that enables the PMU >>>> and after the last iteration, before disabling the PMU. >>>> >>>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx> >>>> Suggested-by: Alexandru Elisei <alexandru.elisei@xxxxxxx> >>>> >>>> --- >>>> >>>> This was discussed in https://lore.kernel.org/all/YzxmHpV2rpfaUdWi@monolith.localdoman/ >>>> --- >>>> arm/pmu.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/arm/pmu.c b/arm/pmu.c >>>> index b88366a8..dde399e2 100644 >>>> --- a/arm/pmu.c >>>> +++ b/arm/pmu.c >>>> @@ -301,6 +301,7 @@ static void mem_access_loop(void *addr, long loop, uint32_t pmcr) >>>> { >>>> uint64_t pmcr64 = pmcr; >>>> asm volatile( >>>> + " dsb ish\n" >>> I think it might still be possible to reorder memory accesses which are >>> part of the loop after the DSB above and before the PMU is enabled below. >>> But the DSB above is needed to make sure previous memory accesses, which >>> shouldn't be counted as part of the loop, are completed. >>> >>> I would put another DSB after the ISB which enables the PMU, that way all >>> memory accesses are neatly sandwitches between two DSBs. >>> >>> Having 3 DSBs might look like overdoing it, but I reason it to be correct. >>> What do you think? >> I need more time to investigate this. I will come back to you next week >> as I am OoO this week. Sorry for the inconvenience. > That's fine, I'm swamped too with other things, so don't expect a quick reply > :) My apologies for following up so late. I took your suggestion into account and added this new DSB. Thanks Eric > > Thanks, > Alex > >> Thank you for the review! >> >> Eric >>> Thanks, >>> Alex >>> >>>> " msr pmcr_el0, %[pmcr]\n" >>>> " isb\n" >>>> " mov x10, %[loop]\n" >>>> @@ -308,6 +309,7 @@ asm volatile( >>>> " ldr x9, [%[addr]]\n" >>>> " cmp x10, #0x0\n" >>>> " b.gt 1b\n" >>>> + " dsb ish\n" >>>> " msr pmcr_el0, xzr\n" >>>> " isb\n" >>>> : >>>> -- >>>> 2.38.1 >>>> >>>>
