Re: [kvm-unit-tests PATCH 3/6] arm: pmu: Add extra DSB barriers in the mem_access loop

[Eric Auger <eric.auger@redhat.com>]

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@redhat.com>
>> Suggested-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>
>> ---
>>
>> 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.
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
>>
>>