The ID register for S1PIE is ID_AA64MMFR3_EL1.S1PIE which is bits 11:8 but get-reg-list uses a shift of 4, checking SCTLRX instead. Use a shift of 8 instead.
Fixes: 5f0419a0083b ("KVM: selftests: get-reg-list: add Permission Indirection registers") Signed-off-by: Mark Brown broonie@kernel.org --- tools/testing/selftests/kvm/aarch64/get-reg-list.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c index 709d7d721760..4abebde78187 100644 --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c @@ -32,13 +32,13 @@ static struct feature_id_reg feat_id_regs[] = { { ARM64_SYS_REG(3, 0, 10, 2, 2), /* PIRE0_EL1 */ ARM64_SYS_REG(3, 0, 0, 7, 3), /* ID_AA64MMFR3_EL1 */ - 4, + 8, 1 }, { ARM64_SYS_REG(3, 0, 10, 2, 3), /* PIR_EL1 */ ARM64_SYS_REG(3, 0, 0, 7, 3), /* ID_AA64MMFR3_EL1 */ - 4, + 8, 1 } };
--- base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b change-id: 20240731-kvm-arm64-fix-s1pie-test-a40b6be58d7b
Best regards,
On Wed, Jul 31, 2024 at 05:21:13PM +0100, Mark Brown wrote:
The ID register for S1PIE is ID_AA64MMFR3_EL1.S1PIE which is bits 11:8 but get-reg-list uses a shift of 4, checking SCTLRX instead. Use a shift of 8 instead.
Fixes: 5f0419a0083b ("KVM: selftests: get-reg-list: add Permission Indirection registers") Signed-off-by: Mark Brown broonie@kernel.org
Argh!
Thanks for spotting that.
Reviewed-by: Joey Gouly joey.gouly@arm.com
tools/testing/selftests/kvm/aarch64/get-reg-list.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c index 709d7d721760..4abebde78187 100644 --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c @@ -32,13 +32,13 @@ static struct feature_id_reg feat_id_regs[] = { { ARM64_SYS_REG(3, 0, 10, 2, 2), /* PIRE0_EL1 */ ARM64_SYS_REG(3, 0, 0, 7, 3), /* ID_AA64MMFR3_EL1 */
4,
8,
4,
8,};
base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b change-id: 20240731-kvm-arm64-fix-s1pie-test-a40b6be58d7b
Best regards,
Mark Brown broonie@kernel.org
On Wed, 31 Jul 2024 17:21:13 +0100, Mark Brown broonie@kernel.org wrote:
The ID register for S1PIE is ID_AA64MMFR3_EL1.S1PIE which is bits 11:8 but get-reg-list uses a shift of 4, checking SCTLRX instead. Use a shift of 8 instead.
Fixes: 5f0419a0083b ("KVM: selftests: get-reg-list: add Permission Indirection registers") Signed-off-by: Mark Brown broonie@kernel.org
tools/testing/selftests/kvm/aarch64/get-reg-list.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kvm/aarch64/get-reg-list.c b/tools/testing/selftests/kvm/aarch64/get-reg-list.c index 709d7d721760..4abebde78187 100644 --- a/tools/testing/selftests/kvm/aarch64/get-reg-list.c +++ b/tools/testing/selftests/kvm/aarch64/get-reg-list.c @@ -32,13 +32,13 @@ static struct feature_id_reg feat_id_regs[] = { { ARM64_SYS_REG(3, 0, 10, 2, 2), /* PIRE0_EL1 */ ARM64_SYS_REG(3, 0, 0, 7, 3), /* ID_AA64MMFR3_EL1 */
4,
8,
4,
8,};
Thanks for spotting this. However, we are fixing it in a very backward way.
Can we please switch all this stuff to symbolic naming instead of magic numbers? Given how much effort is going into the "automated generation" thing, it is mind-boggling that the tests still rely on handcrafted numbers. We just end-up with two different sets of bugs.
At the moment, the level of confidence I have in this stuff is sub-zero.
M.
On Thu, Aug 01, 2024 at 05:45:49PM +0100, Marc Zyngier wrote:
Can we please switch all this stuff to symbolic naming instead of magic numbers? Given how much effort is going into the "automated generation" thing, it is mind-boggling that the tests still rely on handcrafted numbers. We just end-up with two different sets of bugs.
At the moment, the level of confidence I have in this stuff is sub-zero.
Yeah, I was wondering why this wasn't using the generated values especially given that the generated headers are available to tools - I wasn't sure if this was a deliberate decision to cross check the data entry or something. I'd certainly be happy to convert, though that does seem a bit invasive for a fix.
On Thu, 01 Aug 2024 20:14:38 +0100, Mark Brown broonie@kernel.org wrote:
[1 <text/plain; us-ascii (7bit)>] On Thu, Aug 01, 2024 at 05:45:49PM +0100, Marc Zyngier wrote:
Can we please switch all this stuff to symbolic naming instead of magic numbers? Given how much effort is going into the "automated generation" thing, it is mind-boggling that the tests still rely on handcrafted numbers. We just end-up with two different sets of bugs.
At the moment, the level of confidence I have in this stuff is sub-zero.
Yeah, I was wondering why this wasn't using the generated values especially given that the generated headers are available to tools - I wasn't sure if this was a deliberate decision to cross check the data entry or something.
We've lost that battle a long time ago, given the numbers of bugs the sysreg file has had. The real reason is that the ABI reports the encoding, and that it is rather easy to just dump stuff back into the test using the script described in the very first commit for the test.
Also, the test predates the generated stuff by some margin.
I'd certainly be happy to convert, though that does seem a bit invasive for a fix.
Not for a point fix, for sure. And if you do, make sure it is entirely scripted.
Thanks,
M.
On Fri, Aug 02, 2024 at 10:00:28AM +0100, Marc Zyngier wrote:
Also, the test predates the generated stuff by some margin.
Yeah, there were still defines in the main kernel source that were being retyped rather than shared previously which made me wonder.
Mark Brown broonie@kernel.org wrote:
I'd certainly be happy to convert, though that does seem a bit invasive for a fix.
Not for a point fix, for sure. And if you do, make sure it is entirely scripted.
When you say "entirely scripted" here I take it you're referring to the list of registers as well, and I guess also to the information about what is enumerated by which ID register values? I'd already been thinking about looking at the latter bit, and possibly also tracking wiring things up to traps (though that's only relevant inside the kernel). I agree that seems sensible, but I do think we can usefully do things in stages - even just replacing the magic numbers with use of the defines would be less error prone. It would be great if we just automatically covered every sysreg we know about in this test without any manual steps.
On Fri, 02 Aug 2024 13:43:03 +0100, Mark Brown broonie@kernel.org wrote:
[1 <text/plain; us-ascii (7bit)>] On Fri, Aug 02, 2024 at 10:00:28AM +0100, Marc Zyngier wrote:
Also, the test predates the generated stuff by some margin.
Yeah, there were still defines in the main kernel source that were being retyped rather than shared previously which made me wonder.
Definitions in the kernel are likely to exist for a long time though, as the tool is still pretty primitive and doesn't handle anything that changes layout (such as any register affected by E2H).
Mark Brown broonie@kernel.org wrote:
I'd certainly be happy to convert, though that does seem a bit invasive for a fix.
Not for a point fix, for sure. And if you do, make sure it is entirely scripted.
When you say "entirely scripted" here I take it you're referring to the list of registers as well, and I guess also to the information about what is enumerated by which ID register values?
The register list is indeed the #1 offender, and that should just be a script that goes over all the occurrences of ARM64_SYS_REG() and replace the encoding with something that uses the symbolic name.
For the rest (shifts and stuff), we can probably do that by hand (there are only a few occurrences).
M.
On Fri, Aug 02, 2024 at 02:36:14PM +0100, Marc Zyngier wrote:
Mark Brown broonie@kernel.org wrote:
Not for a point fix, for sure. And if you do, make sure it is entirely scripted.
When you say "entirely scripted" here I take it you're referring to the list of registers as well, and I guess also to the information about what is enumerated by which ID register values?
The register list is indeed the #1 offender, and that should just be a script that goes over all the occurrences of ARM64_SYS_REG() and replace the encoding with something that uses the symbolic name.
Oh, I see - the scripting of an in place update. It sounded like you wanted the tables generating at build time (which I do think should be the eventual goal).
On Wed, 31 Jul 2024 17:21:13 +0100, Mark Brown wrote:
The ID register for S1PIE is ID_AA64MMFR3_EL1.S1PIE which is bits 11:8 but get-reg-list uses a shift of 4, checking SCTLRX instead. Use a shift of 8 instead.
Applied to kvmarm/fixes, thanks!
[1/1] KVM: selftests: arm64: Correct feature test for S1PIE in get-reg-list https://git.kernel.org/kvmarm/kvmarm/c/ad518452fd26
-- Best, Oliver
linux-kselftest-mirror@lists.linaro.org
-
Joey Gouly -
Marc Zyngier -
Mark Brown -
Oliver Upton