Hi Marc,
On 2019/8/2 23:56, Marc Zyngier wrote:
On 02/08/2019 11:56, Zenghui Yu wrote:
Hi folks,
Running kvm-unit-tests with Linux 5.3.0-rc2 on Kunpeng 920, we will get
the following fail info:
[...]
FAIL psci (4 tests, 1 unexpected failures)
[...]
and
[...]
INFO: unexpected cpu_on return value: caller=CPU9, ret=-2
FAIL: cpu-on
SUMMARY: 4 tests, 1 unexpected failures
I think this is an issue had been fixed once by commit 6c7a5dce22b3
("KVM: arm/arm64: fix races in kvm_psci_vcpu_on"), which makes use of
kvm->lock mutex to fix the race between two PSCI_CPU_ON calls - one
does reset on the MPIDR register whilst another reads it.
But commit 358b28f09f0 ("arm/arm64: KVM: Allow a VCPU to fully reset
itself") later moves the reset work into check_vcpu_requests(), by
making a KVM_REQ_VCPU_RESET request in PSCI code. Thus the reset work
has not been protected by kvm->lock mutex anymore, and the race shows up
again...
Do we need a fix for this issue? At least achieve a mutex execution
between the reset of MPIDR and kvm_mpidr_to_vcpu()?
The thing is that the way we reset registers is marginally insane.
Yes, it catches most reset bugs. It also introduces many more in
the rest of the paths.
The fun part is that there is hardly a need for resetting MPIDR.
It has already been set when we've created the vcpu. It is the
(That means we can let reset_mpidr() do nothing?)
poisoning of the sysreg array that creates a situation where
the MPIDR is temporarily invalid.
So instead of poisoning the array, how about we just keep
track of the registers for which we've called a reset function?
It should be enough to track the most obvious bugs... I've
The reset of DBG{BCR,BVR,WVR,WCR}n_EL1 registers will also be tracked.
It may affect our judgment?
cobbled the following patch together, which seems to fix the
issue on my TX2 with 64 vcpus.
Thoughts?
M.
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index f26e181d881c..17f46ee7dc83 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2254,13 +2254,17 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu,
}
static void reset_sys_reg_descs(struct kvm_vcpu *vcpu,
- const struct sys_reg_desc *table, size_t num)
+ const struct sys_reg_desc *table, size_t num,
+ unsigned long *bmap)
{
unsigned long i;
for (i = 0; i < num; i++)
- if (table[i].reset)
+ if (table[i].reset) {
table[i].reset(vcpu, &table[i]);
+ if (bmap)
+ set_bit(i, bmap);
I think this should be:
set_bit(table[i].reg, bmap);
Am I wrong?
+ }
}
/**
@@ -2772,21 +2776,23 @@ void kvm_sys_reg_table_init(void)
*/
void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
{
+ unsigned long *bmap;
size_t num;
const struct sys_reg_desc *table;
- /* Catch someone adding a register without putting in reset entry. */
- memset(&vcpu->arch.ctxt.sys_regs, 0x42, sizeof(vcpu->arch.ctxt.sys_regs));
+ bmap = bitmap_alloc(NR_SYS_REGS, GFP_KERNEL);
LOCKDEP kernel will be not happy with this bitmap_alloc:
" BUG: sleeping function called from invalid context at mm/slab.h:501
in_atomic(): 1, irqs_disabled(): 0, pid: 8710, name: qemu-system-aar "
/* Generic chip reset first (so target could override). */
- reset_sys_reg_descs(vcpu, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
+ reset_sys_reg_descs(vcpu, sys_reg_descs, ARRAY_SIZE(sys_reg_descs), bmap);
table = get_target_table(vcpu->arch.target, true, &num);
- reset_sys_reg_descs(vcpu, table, num);
+ reset_sys_reg_descs(vcpu, table, num, bmap);
for (num = 1; num < NR_SYS_REGS; num++) {
- if (WARN(__vcpu_sys_reg(vcpu, num) == 0x4242424242424242,
+ if (WARN(bmap && !test_bit(num, bmap),
"Didn't reset __vcpu_sys_reg(%zi)\n", num))
break;
}
+
+ kfree(bmap);
}
Some other minor questions about the sys reg resetting:
1. Pointer Authentication Registers haven't have reset entry yet,
do they need? The same for ACTLR_EL1.
2. Why does PMCR_EL0 register have no "reg" field, in sys_reg_descs[]?
I will test this patch with kvm-unit-tests next week!
Thanks,
zenghui
