Re: [kvm-unit-tests PATCH v4 00/10] arm/arm64: Various fixes

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Andrew,

On 2/3/20 6:59 PM, Andrew Jones wrote:
> On Fri, Jan 31, 2020 at 04:37:18PM +0000, Alexandru Elisei wrote:
>> These are the patches that were left unmerged from the previous version of
>> the series, plus a few new patches. Patch #1 "Makefile: Use
>> no-stack-protector compiler options" is straightforward and came about
>> because of a compile error I experienced on RockPro64.
>>
>> Patches #3 and #5 are the result of Andre's comments on the previous
>> version. When adding ISBs after register writes I noticed in the ARM ARM
>> that a read of the timer counter value can be reordered, and patch #4
>> tries to avoid that.
>>
>> Patch #7 is also the result of a review comment. For the GIC tests, we wait
>> up to 5 seconds for the interrupt to be asserted. However, the GIC tests
>> can use more than one CPU, which is not the case with the timer test. And
>> waiting for the GIC to assert the interrupt can happen up to 6 times (8
>> times after patch #9), so I figured that a timeout of 10 seconds for the
>> test is acceptable.
>>
>> Patch #8 tries to improve the way we test how the timer generates the
>> interrupt. If the GIC asserts the timer interrupt, but the device itself is
>> not generating it, that's a pretty big problem.
>>
>> Ran the same tests as before:
>>
>> - with kvmtool, on an arm64 host kernel: 64 and 32 bit tests, with GICv3
>>   (on an Ampere eMAG) and GICv2 (on a AMD Seattle box).
>>
>> - with qemu, on an arm64 host kernel:
>>     a. with accel=kvm, 64 and 32 bit tests, with GICv3 (Ampere eMAG) and
>>        GICv2 (Seattle).
>>     b. with accel=tcg, 64 and 32 bit tests, on the Ampere eMAG machine.
>>
>> Changes:
>> * Patches #1, #3, #4, #5, #7, #8 are new.
>> * For patch #2, as per Drew's suggestion, I changed the entry point to halt
>>   because the test is supposed to test if CPU_ON is successful.
>> * Removed the ISB from patch #6 because that was fixed by #3.
>> * Moved the architecture dependent function init_dcache_line_size to
>>   cpu_init in lib/arm/setup.c as per Drew's suggestion.
>>
>> Alexandru Elisei (10):
>>   Makefile: Use no-stack-protector compiler options
>>   arm/arm64: psci: Don't run C code without stack or vectors
>>   arm64: timer: Add ISB after register writes
>>   arm64: timer: Add ISB before reading the counter value
>>   arm64: timer: Make irq_received volatile
>>   arm64: timer: EOIR the interrupt after masking the timer
>>   arm64: timer: Wait for the GIC to sample timer interrupt state
>>   arm64: timer: Check the timer interrupt state
>>   arm64: timer: Test behavior when timer disabled or masked
>>   arm/arm64: Perform dcache clean + invalidate after turning MMU off
>>
>>  Makefile                  |  4 +-
>>  lib/arm/asm/processor.h   | 13 +++++++
>>  lib/arm64/asm/processor.h | 12 ++++++
>>  lib/arm/setup.c           |  8 ++++
>>  arm/cstart.S              | 22 +++++++++++
>>  arm/cstart64.S            | 23 ++++++++++++
>>  arm/psci.c                | 15 ++++++--
>>  arm/timer.c               | 79 ++++++++++++++++++++++++++++++++-------
>>  arm/unittests.cfg         |  2 +-
>>  9 files changed, 158 insertions(+), 20 deletions(-)
>>
>> -- 
>> 2.20.1
>>
> The series looks good to me. The first patch probably could have been
> posted separately, but I'll try to test the whole series tomorrow. If

Noted, next time I will try to do a better job separating the patches. I found the
bug while testing the arm64 fixes, and I was getting ready to send the patches, so
I just figured I'll send it as part of the series.

> all looks well, I'll prepare a pull request for Paolo.

Thank you very much for taking the time to review the patches! Much appreciated.

Thanks,
Alex
>
> Thanks,
> drew 
>