[PATCH v5 7/7] KVM: arm64: selftests: Test ID_AA64PFR0.MPAM isn't completely ignored
Gavin Shan
gshan at redhat.com
Wed Oct 16 17:41:14 PDT 2024
On 10/15/24 11:39 PM, Joey Gouly wrote:
> From: James Morse <james.morse at arm.com>
>
> The ID_AA64PFR0.MPAM bit was previously accidentally exposed to guests,
> and is ignored by KVM. KVM will always present the guest with 0 here,
> and trap the MPAM system registers to inject an undef.
>
> But, this value is still needed to prevent migration when the value
> is incompatible with the target hardware. Add a kvm unit test to try
> and write multiple values to ID_AA64PFR0.MPAM. Only the hardware value
> previously exposed should be ignored, all other values should be
> rejected.
>
> Signed-off-by: James Morse <james.morse at arm.com>
> Signed-off-by: Joey Gouly <joey.gouly at arm.com>
> ---
> .../selftests/kvm/aarch64/set_id_regs.c | 100 +++++++++++++++++-
> 1 file changed, 99 insertions(+), 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 2a3fe7914b72..d985ead2cc45 100644
> --- a/tools/testing/selftests/kvm/aarch64/set_id_regs.c
> +++ b/tools/testing/selftests/kvm/aarch64/set_id_regs.c
> @@ -433,6 +433,103 @@ static void test_vm_ftr_id_regs(struct kvm_vcpu *vcpu, bool aarch64_only)
> }
> }
>
> +#define MPAM_IDREG_TEST 6
> +static void test_user_set_mpam_reg(struct kvm_vcpu *vcpu)
> +{
> + uint64_t masks[KVM_ARM_FEATURE_ID_RANGE_SIZE];
> + struct reg_mask_range range = {
> + .addr = (__u64)masks,
> + };
> + uint64_t val, ftr_mask;
> + int idx, err;
> +
> + /*
> + * If ID_AA64PFR0.MPAM is _not_ officially modifiable and is zero,
> + * check that if it can be set to 1, (i.e. it is supported by the
> + * hardware), that it can't be set to other values.
> + */
> +
> + /* Get writable masks for feature ID registers */
> + memset(range.reserved, 0, sizeof(range.reserved));
> + vm_ioctl(vcpu->vm, KVM_ARM_GET_REG_WRITABLE_MASKS, &range);
> +
> + /* Writeable? Nothing to test! */
> + idx = encoding_to_range_idx(SYS_ID_AA64PFR0_EL1);
> + ftr_mask = ID_AA64PFR0_EL1_MPAM_MASK;
> + if ((masks[idx] & ftr_mask) == ftr_mask) {
> + ksft_test_result_skip("ID_AA64PFR0_EL1.MPAM is officially writable, nothing to test\n");
> + return;
> + }
> +
The question is shall we proceed to test ID_AA64PFR1_EL1.MPAM_frac instead of
bailing when ID_AA64PFR0_EL1.MPAM accepts arbitrary values? To me, it doesn't
mean ID_AA64PFR1_EL1.MPAM_frac can accept arbitrary values when ID_AA64PFR0_EL1.MPAM
does.
@ftr_mask can be dropped since it's initialized to a fixed mask and used for
only for once.
if (mask[idx] & ID_AA64PFR0_EL1_MPAM_MASK] == ID_AA64PFR0_EL1_MPAM_MASK) {
:
}
> + /* Get the id register value */
> + vcpu_get_reg(vcpu, KVM_ARM64_SYS_REG(SYS_ID_AA64PFR0_EL1), &val);
> +
> + /* Try to set MPAM=0. This should always be possible. */
> + val &= ~GENMASK_ULL(44, 40);
> + val |= FIELD_PREP(GENMASK_ULL(44, 40), 0);
> + err = __vcpu_set_reg(vcpu, KVM_ARM64_SYS_REG(SYS_ID_AA64PFR0_EL1), val);
> + if (err)
> + ksft_test_result_fail("ID_AA64PFR0_EL1.MPAM=0 was not accepted\n");
> + else
> + ksft_test_result_pass("ID_AA64PFR0_EL1.MPAM=0 worked\n");
> +
> + /* Try to set MPAM=1 */
> + val &= ~GENMASK_ULL(44, 40);
> + val |= FIELD_PREP(GENMASK_ULL(44, 40), 1);
> + err = __vcpu_set_reg(vcpu, KVM_ARM64_SYS_REG(SYS_ID_AA64PFR0_EL1), val);
> + if (err)
> + ksft_test_result_skip("ID_AA64PFR0_EL1.MPAM is not writable, nothing to test\n");
> + else
> + ksft_test_result_pass("ID_AA64PFR0_EL1.MPAM=1 was writable\n");
> +
> + /* Try to set MPAM=2 */
> + val &= ~GENMASK_ULL(43, 40);
> + val |= FIELD_PREP(GENMASK_ULL(43, 40), 2);
> + err = __vcpu_set_reg(vcpu, KVM_ARM64_SYS_REG(SYS_ID_AA64PFR0_EL1), val);
> + if (err)
> + ksft_test_result_pass("ID_AA64PFR0_EL1.MPAM not arbitrarily modifiable\n");
> + else
> + ksft_test_result_fail("ID_AA64PFR0_EL1.MPAM value should not be ignored\n");
> +
GENMASK_ULL(44, 40) looks wrong to me. According to the spec, GENMASK_ULL(43, 40)
is the correct mask. Besides, I think it would be nice to use ID_AA64PFR0_EL1_MPAM_MASK
here, for example:
val &= ~ID_AA64PFR0_EL1_MPAM_MASK;
val |= FIELD_PREP(ID_AA64PFR0_EL1_MPAM_MASK, 2);
> + /* And again for ID_AA64PFR1_EL1.MPAM_frac */
> + idx = encoding_to_range_idx(SYS_ID_AA64PFR1_EL1);
> + ftr_mask = ID_AA64PFR1_EL1_MPAM_frac_MASK;
> + if ((masks[idx] & ftr_mask) == ftr_mask) {
> + ksft_test_result_skip("ID_AA64PFR1_EL1.MPAM_frac is officially writable, nothing to test\n");
> + return;
> + }
> +
> + /* Get the id register value */
> + vcpu_get_reg(vcpu, KVM_ARM64_SYS_REG(SYS_ID_AA64PFR1_EL1), &val);
> +
> + /* Try to set MPAM_frac=0. This should always be possible. */
> + val &= ~GENMASK_ULL(19, 16);
> + val |= FIELD_PREP(GENMASK_ULL(19, 16), 0);
> + err = __vcpu_set_reg(vcpu, KVM_ARM64_SYS_REG(SYS_ID_AA64PFR1_EL1), val);
> + if (err)
> + ksft_test_result_fail("ID_AA64PFR0_EL1.MPAM_frac=0 was not accepted\n");
> + else
> + ksft_test_result_pass("ID_AA64PFR0_EL1.MPAM_frac=0 worked\n");
> +
> + /* Try to set MPAM_frac=1 */
> + val &= ~GENMASK_ULL(19, 16);
> + val |= FIELD_PREP(GENMASK_ULL(19, 16), 1);
> + err = __vcpu_set_reg(vcpu, KVM_ARM64_SYS_REG(SYS_ID_AA64PFR1_EL1), val);
> + if (err)
> + ksft_test_result_skip("ID_AA64PFR1_EL1.MPAM_frac is not writable, nothing to test\n");
> + else
> + ksft_test_result_pass("ID_AA64PFR0_EL1.MPAM_frac=1 was writable\n");
> +
> + /* Try to set MPAM_frac=2 */
> + val &= ~GENMASK_ULL(19, 16);
> + val |= FIELD_PREP(GENMASK_ULL(19, 16), 2);
> + err = __vcpu_set_reg(vcpu, KVM_ARM64_SYS_REG(SYS_ID_AA64PFR1_EL1), val);
> + if (err)
> + ksft_test_result_pass("ID_AA64PFR1_EL1.MPAM_frac not arbitrarily modifiable\n");
> + else
> + ksft_test_result_fail("ID_AA64PFR1_EL1.MPAM_frac value should not be ignored\n");
> +}
> +
Similarly, ID_AA64PFR1_EL1_MPAM_frac_MASK can be used?
> static void test_guest_reg_read(struct kvm_vcpu *vcpu)
> {
> bool done = false;
> @@ -571,12 +668,13 @@ int main(void)
> ARRAY_SIZE(ftr_id_aa64isar2_el1) + ARRAY_SIZE(ftr_id_aa64pfr0_el1) +
> ARRAY_SIZE(ftr_id_aa64mmfr0_el1) + ARRAY_SIZE(ftr_id_aa64mmfr1_el1) +
> ARRAY_SIZE(ftr_id_aa64mmfr2_el1) + ARRAY_SIZE(ftr_id_aa64zfr0_el1) -
> - ARRAY_SIZE(test_regs) + 2;
> + ARRAY_SIZE(test_regs) + 2 + MPAM_IDREG_TEST;
>
> ksft_set_plan(test_cnt);
>
> test_vm_ftr_id_regs(vcpu, aarch64_only);
> test_vcpu_ftr_id_regs(vcpu);
> + test_user_set_mpam_reg(vcpu);
>
> test_guest_reg_read(vcpu);
>
Thanks,
Gavin
More information about the linux-arm-kernel
mailing list