| Message ID | 20241216-kvm-arm64-fix-set-id-asidbits-v1-1-8b105b888fc3@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: arm64: Fix set_id_regs selftest for ASIDBITS becoming unwritable | expand |
On Mon, 16 Dec 2024 19:28:24 +0000, Mark Brown <broonie@kernel.org> wrote: > > In commit 03c7527e97f7 ("KVM: arm64: Do not allow ID_AA64MMFR0_EL1.ASIDbits > to be overridden") we made that bitfield in the ID registers unwritable > however the change neglected to make the corresponding update to set_id_regs > resulting in it failing: > > # ok 56 ID_AA64MMFR0_EL1_BIGEND > # ==== Test Assertion Failure ==== > # aarch64/set_id_regs.c:434: masks[idx] & ftr_bits[j].mask == ftr_bits[j].mask > # pid=5566 tid=5566 errno=22 - Invalid argument > # 1 0x00000000004034a7: test_vm_ftr_id_regs at set_id_regs.c:434 > # 2 0x0000000000401b53: main at set_id_regs.c:684 > # 3 0x0000ffff8e6b7543: ?? ??:0 > # 4 0x0000ffff8e6b7617: ?? ??:0 > # 5 0x0000000000401e6f: _start at ??:? > # 0 != 0xf0 (masks[idx] & ftr_bits[j].mask != ftr_bits[j].mask) > not ok 8 selftests: kvm: set_id_regs # exit=254 > > Remove ID_AA64MMFR1_EL1.ASIDBITS from the set of bitfields we test for > writeability. > > Fixes: 03c7527e97f7 ("KVM: arm64: Do not allow ID_AA64MMFR0_EL1.ASIDbits to be overridden") A patch for a test doesn't fix anything in the kernel. > Signed-off-by: Mark Brown <broonie@kernel.org> > --- > tools/testing/selftests/kvm/aarch64/set_id_regs.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/tools/testing/selftests/kvm/aarch64/set_id_regs.c b/tools/testing/selftests/kvm/aarch64/set_id_regs.c > index a79b7f18452d2ec336ae623b8aa5c9cf329b6b4e..3a97c160b5fec990aaf8dfaf100a907b913f057c 100644 > --- a/tools/testing/selftests/kvm/aarch64/set_id_regs.c > +++ b/tools/testing/selftests/kvm/aarch64/set_id_regs.c > @@ -152,7 +152,6 @@ static const struct reg_ftr_bits ftr_id_aa64mmfr0_el1[] = { > REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64MMFR0_EL1, BIGENDEL0, 0), > REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64MMFR0_EL1, SNSMEM, 0), > REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64MMFR0_EL1, BIGEND, 0), > - REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64MMFR0_EL1, ASIDBITS, 0), > REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64MMFR0_EL1, PARANGE, 0), > REG_FTR_END, > }; > With the Fixes: line dropped, Acked-by: Marc Zyngier <maz@kernel.org> M.
On Mon, 16 Dec 2024 19:28:24 +0000, Mark Brown wrote: > In commit 03c7527e97f7 ("KVM: arm64: Do not allow ID_AA64MMFR0_EL1.ASIDbits > to be overridden") we made that bitfield in the ID registers unwritable > however the change neglected to make the corresponding update to set_id_regs > resulting in it failing: > > # ok 56 ID_AA64MMFR0_EL1_BIGEND > # ==== Test Assertion Failure ==== > # aarch64/set_id_regs.c:434: masks[idx] & ftr_bits[j].mask == ftr_bits[j].mask > # pid=5566 tid=5566 errno=22 - Invalid argument > # 1 0x00000000004034a7: test_vm_ftr_id_regs at set_id_regs.c:434 > # 2 0x0000000000401b53: main at set_id_regs.c:684 > # 3 0x0000ffff8e6b7543: ?? ??:0 > # 4 0x0000ffff8e6b7617: ?? ??:0 > # 5 0x0000000000401e6f: _start at ??:? > # 0 != 0xf0 (masks[idx] & ftr_bits[j].mask != ftr_bits[j].mask) > not ok 8 selftests: kvm: set_id_regs # exit=254 > > [...] Applied to fixes, thanks! [1/1] KVM: arm64: Fix set_id_regs selftest for ASIDBITS becoming unwritable https://git.kernel.org/kvmarm/kvmarm/c/212fbabe1dfe -- Best, Oliver
On Tue, Dec 17, 2024 at 08:30:37AM +0000, Marc Zyngier wrote: > Mark Brown <broonie@kernel.org> wrote: > > Fixes: 03c7527e97f7 ("KVM: arm64: Do not allow ID_AA64MMFR0_EL1.ASIDbits to be overridden") > A patch for a test doesn't fix anything in the kernel. The selftests are shipped as part of the kernel source and frequently used for testing the kernel, it's all one source base and we want to ensure that for example the test fix gets backported if the relevant kernel patch does.
On Tue, 17 Dec 2024 13:12:12 +0000, Mark Brown <broonie@kernel.org> wrote: > > [1 <text/plain; us-ascii (7bit)>] > On Tue, Dec 17, 2024 at 08:30:37AM +0000, Marc Zyngier wrote: > > Mark Brown <broonie@kernel.org> wrote: > > > > Fixes: 03c7527e97f7 ("KVM: arm64: Do not allow ID_AA64MMFR0_EL1.ASIDbits to be overridden") > > > A patch for a test doesn't fix anything in the kernel. > > The selftests are shipped as part of the kernel source and frequently > used for testing the kernel, it's all one source base and we want to > ensure that for example the test fix gets backported if the relevant > kernel patch does. That's not what Fixes: describes. If you want to invent a new tag that expresses a dependency, do that. Don't use these tags to misrepresent what the patches does. M.
On Tue, Dec 17, 2024 at 01:54:39PM +0000, Marc Zyngier wrote: > Mark Brown <broonie@kernel.org> wrote: > > The selftests are shipped as part of the kernel source and frequently > > used for testing the kernel, it's all one source base and we want to > > ensure that for example the test fix gets backported if the relevant > > kernel patch does. > That's not what Fixes: describes. If you want to invent a new tag that > expresses a dependency, do that. Don't use these tags to misrepresent > what the patches does. No, this isn't a new use - a Fixes: tag indicates that the referenced commit introduced the problem being fixed and that is exactly what's going on here. Like I say the selftests are not a completely separate project, they are a part of the same source release as the rest of the kernel and it is helpful to track information like this.
On Tue, 17 Dec 2024 15:10:28 +0000, Mark Brown <broonie@kernel.org> wrote: > > [1 <text/plain; us-ascii (7bit)>] > On Tue, Dec 17, 2024 at 01:54:39PM +0000, Marc Zyngier wrote: > > Mark Brown <broonie@kernel.org> wrote: > > > > The selftests are shipped as part of the kernel source and frequently > > > used for testing the kernel, it's all one source base and we want to > > > ensure that for example the test fix gets backported if the relevant > > > kernel patch does. > > > That's not what Fixes: describes. If you want to invent a new tag that > > expresses a dependency, do that. Don't use these tags to misrepresent > > what the patches does. > > No, this isn't a new use - a Fixes: tag indicates that the referenced > commit introduced the problem being fixed and that is exactly what's > going on here. Like I say the selftests are not a completely separate > project, they are a part of the same source release as the rest of the > kernel and it is helpful to track information like this. Well, we'll have to agree to disagree. M.
On Tue, Dec 17, 2024 at 03:10:28PM +0000, Mark Brown wrote: > On Tue, Dec 17, 2024 at 01:54:39PM +0000, Marc Zyngier wrote: > > Mark Brown <broonie@kernel.org> wrote: > > > > The selftests are shipped as part of the kernel source and frequently > > > used for testing the kernel, it's all one source base and we want to > > > ensure that for example the test fix gets backported if the relevant > > > kernel patch does. > > > That's not what Fixes: describes. If you want to invent a new tag that > > expresses a dependency, do that. Don't use these tags to misrepresent > > what the patches does. > > No, this isn't a new use - a Fixes: tag indicates that the referenced > commit introduced the problem being fixed and that is exactly what's > going on here. Like I say the selftests are not a completely separate > project, they are a part of the same source release as the rest of the > kernel and it is helpful to track information like this. A Fixes tag suggests a bug in the referenced commit, which isn't the case here. I agree that having some relation between the two is useful for determining the scope of a backport, but conveniently in this case the test failure was introduced in 6.13. I've taken the fix for 6.13, w/ the tag dropped.
On Tue, Dec 17, 2024 at 10:00:44AM -0800, Oliver Upton wrote: > On Tue, Dec 17, 2024 at 03:10:28PM +0000, Mark Brown wrote: > > No, this isn't a new use - a Fixes: tag indicates that the referenced > > commit introduced the problem being fixed and that is exactly what's > > going on here. Like I say the selftests are not a completely separate > > project, they are a part of the same source release as the rest of the > > kernel and it is helpful to track information like this. > A Fixes tag suggests a bug in the referenced commit, which isn't the > case here. > I agree that having some relation between the two is useful for > determining the scope of a backport, but conveniently in this case the > test failure was introduced in 6.13. While the patch introducing the test failure was introduced in -rc3 it was tagged as a fix for d5a32b60dc18 ("KVM: arm64: Allow userspace to change ID_AA64MMFR{0-2}_EL1") which was in v6.7 so I'm expecting to see it being backported to relevant stable kernels, which will in turn cause testsuite failures in those stable kernels if this change doesn't go back with it. Hopefully they'll find it from the commit message. I would say there's a case that leaving the tests broken is a bug, it's certainly some kind of problem. Obviously we're sometimes a bit relaxed on that within a series, though it's fortunately relatively rare.
diff --git a/tools/testing/selftests/kvm/aarch64/set_id_regs.c b/tools/testing/selftests/kvm/aarch64/set_id_regs.c index a79b7f18452d2ec336ae623b8aa5c9cf329b6b4e..3a97c160b5fec990aaf8dfaf100a907b913f057c 100644 --- a/tools/testing/selftests/kvm/aarch64/set_id_regs.c +++ b/tools/testing/selftests/kvm/aarch64/set_id_regs.c @@ -152,7 +152,6 @@ static const struct reg_ftr_bits ftr_id_aa64mmfr0_el1[] = { REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64MMFR0_EL1, BIGENDEL0, 0), REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64MMFR0_EL1, SNSMEM, 0), REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64MMFR0_EL1, BIGEND, 0), - REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64MMFR0_EL1, ASIDBITS, 0), REG_FTR_BITS(FTR_LOWER_SAFE, ID_AA64MMFR0_EL1, PARANGE, 0), REG_FTR_END, };
In commit 03c7527e97f7 ("KVM: arm64: Do not allow ID_AA64MMFR0_EL1.ASIDbits to be overridden") we made that bitfield in the ID registers unwritable however the change neglected to make the corresponding update to set_id_regs resulting in it failing: # ok 56 ID_AA64MMFR0_EL1_BIGEND # ==== Test Assertion Failure ==== # aarch64/set_id_regs.c:434: masks[idx] & ftr_bits[j].mask == ftr_bits[j].mask # pid=5566 tid=5566 errno=22 - Invalid argument # 1 0x00000000004034a7: test_vm_ftr_id_regs at set_id_regs.c:434 # 2 0x0000000000401b53: main at set_id_regs.c:684 # 3 0x0000ffff8e6b7543: ?? ??:0 # 4 0x0000ffff8e6b7617: ?? ??:0 # 5 0x0000000000401e6f: _start at ??:? # 0 != 0xf0 (masks[idx] & ftr_bits[j].mask != ftr_bits[j].mask) not ok 8 selftests: kvm: set_id_regs # exit=254 Remove ID_AA64MMFR1_EL1.ASIDBITS from the set of bitfields we test for writeability. Fixes: 03c7527e97f7 ("KVM: arm64: Do not allow ID_AA64MMFR0_EL1.ASIDbits to be overridden") Signed-off-by: Mark Brown <broonie@kernel.org> --- tools/testing/selftests/kvm/aarch64/set_id_regs.c | 1 - 1 file changed, 1 deletion(-) --- base-commit: 78d4f34e2115b517bcbfe7ec0d018bbbb6f9b0b8 change-id: 20241216-kvm-arm64-fix-set-id-asidbits-9bede25b7ad3 Best regards,