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(-)

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,
 };

---
base-commit: 78d4f34e2115b517bcbfe7ec0d018bbbb6f9b0b8
change-id: 20241216-kvm-arm64-fix-set-id-asidbits-9bede25b7ad3

Best regards,
-- 
Mark Brown <broonie@kernel.org>

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 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.

-- 
Without deviation from the norm, progress is not possible.

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.

-- 
Without deviation from the norm, progress is not possible.

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, 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.

-- 
Thanks,
Oliver

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.

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.

-- 
Without deviation from the norm, progress is not possible.