Hi Marc,
On 2019/8/3 18:10, Marc Zyngier wrote:
On Sat, 3 Aug 2019 17:27:41 +0800
Zenghui Yu <yuzenghui@xxxxxxxxxx> wrote:
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?)
It should ever be only written once, as MPIDR is a constant from the
guest perspective. So it is not that it can do nothing. It is just that
there should never be any other value written to it.
Thanks for this explanation.
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?
How so?
bmap[0..15] will be set multiple times. But it also will not affect
anything now (it's safe).
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?
No, you're absolutely right.
+ }
}
> /**
@@ -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 "
Well spotted. I guess GFP_ATOMIC is in order.
> /* 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.
Pointer auth registers definitely have a reset function, set to
reset_unknown. So does ACTLR_EL1, which resets to the host's value.
I find them now :-)
2. Why does PMCR_EL0 register have no "reg" field, in sys_reg_descs[]?
This looks like a (very minor) bug. reset_pmcr writes directly to the
PMCR_EL0 shadow register without using r->reg as the register number.
But in the light of the reset tracking we want to add, this needs
fixing.
I will test this patch with kvm-unit-tests next week!
Well, wait until I repost something a bit less buggy...
Thanks,
zenghui
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
