> -----Original Message----- > From: kvm-owner@xxxxxxxxxxxxxxx [mailto:kvm-owner@xxxxxxxxxxxxxxx] > On Behalf Of Marc Zyngier > Sent: Friday, April 24, 2020 3:45 PM > To: yuzenghui > Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; kvmarm@xxxxxxxxxxxxxxxxxxxxx; > kvm@xxxxxxxxxxxxxxx; Will Deacon; Andre Przywara; Dave Martin; > George Cherian; Zengtao (B); Catalin Marinas > Subject: Re: [PATCH 18/26] KVM: arm64: Don't use empty structures as > CPU reset state > > Hi Zenghui, > > On Fri, 24 Apr 2020 12:07:50 +0800 > Zenghui Yu <yuzenghui@xxxxxxxxxx> wrote: > > > Hi Marc, > > > > On 2020/4/22 20:00, Marc Zyngier wrote: > > > Keeping empty structure as the vcpu state initializer is slightly > > > wasteful: we only want to set pstate, and zero everything else. > > > Just do that. > > > > > > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx> > > > --- > > > arch/arm64/kvm/reset.c | 20 +++++++++----------- > > > 1 file changed, 9 insertions(+), 11 deletions(-) > > > > > > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c > > > index 241db35a7ef4f..895d7d9ad1866 100644 > > > --- a/arch/arm64/kvm/reset.c > > > +++ b/arch/arm64/kvm/reset.c > > > @@ -37,15 +37,11 @@ static u32 kvm_ipa_limit; > > > /* > > > * ARMv8 Reset Values > > > */ > > > -static const struct kvm_regs default_regs_reset = { > > > - .regs.pstate = (PSR_MODE_EL1h | PSR_A_BIT | PSR_I_BIT | > > > - PSR_F_BIT | PSR_D_BIT), > > > -}; > > > +#define VCPU_RESET_PSTATE_EL1 (PSR_MODE_EL1h | > PSR_A_BIT | PSR_I_BIT | \ > > > + PSR_F_BIT | PSR_D_BIT) > > > > -static const struct kvm_regs default_regs_reset32 = { > > > - .regs.pstate = (PSR_AA32_MODE_SVC | PSR_AA32_A_BIT | > > > - PSR_AA32_I_BIT | PSR_AA32_F_BIT), > > > -}; > > > +#define VCPU_RESET_PSTATE_SVC (PSR_AA32_MODE_SVC | > PSR_AA32_A_BIT | \ > > > + PSR_AA32_I_BIT | PSR_AA32_F_BIT) > > > > static bool cpu_has_32bit_el1(void) > > > { > > > @@ -261,6 +257,7 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu) > > > const struct kvm_regs *cpu_reset; > > > int ret = -EINVAL; > > > bool loaded; > > > + u32 pstate; > > > > /* Reset PMU outside of the non-preemptible section */ > > > kvm_pmu_vcpu_reset(vcpu); > > > @@ -291,16 +288,17 @@ int kvm_reset_vcpu(struct kvm_vcpu > *vcpu) > > > if (test_bit(KVM_ARM_VCPU_EL1_32BIT, > vcpu->arch.features)) { > > > if (!cpu_has_32bit_el1()) > > > goto out; > > > - cpu_reset = &default_regs_reset32; > > > + pstate = VCPU_RESET_PSTATE_SVC; > > > } else { > > > - cpu_reset = &default_regs_reset; > > > + pstate = VCPU_RESET_PSTATE_EL1; > > > } > > > > break; > > > } > > > > /* Reset core registers */ > > > - memcpy(vcpu_gp_regs(vcpu), cpu_reset, sizeof(*cpu_reset)); > > > + memset(vcpu_gp_regs(vcpu), 0, sizeof(*cpu_reset)); > > > > Be careful that we can *not* use 'sizeof(*cpu_reset)' here anymore. > As > > you're going to refactor the layout of the core registers whilst keeping > > the kvm_regs API unchanged. Resetting the whole kvm_regs will go > > corrupting some affected registers and make them temporarily invalid. > > The bad thing will show up after you start moving ELR_EL1 around, > > specifically in patch #20... > > Ah, awesome find! Yes, it is pretty obvious now that you point it out. > If I had removed this now useless cpu_reset variable, I'd have spotted > it! > > > And the first victim is ... MPIDR_EL1 (the first one in sys_regs array). > > Now you know how this was spotted ;-) I think this should be the > root > > cause of what Zengtao had previously reported [*]. > > It'd be good if Zengtao could confirm that changing this line to > > memset(vcpu_gp_regs(vcpu), 0, sizeof(*vcpu_gp_regs(vcpu))); > > fixes his problem. I tried this change, and it did fix my problem, thanks :). > > > If these registers are all expected to be reset to architecturally > > UNKNOWN values, I think we can just drop this memset(), though > haven't > > check with the ARM ARM carefully. > > D1.9.1 ("PE state on reset to AArch64 state"): > > "All general-purpose, and SIMD and floating-point registers are > UNKNOWN." > > There is a vaguely similar wording for AArch32 (G1.17.1), although it > is only described by omission: > > "Immediately after a reset, much of the PE state is UNKNOWN. However, > some of the PE state is defined." > > and the GPRs are not part of the list of defined states. > > Still, I'm worried to change KVM's behaviour after so long... I'll have > a try with a handful of non-Linux guests and see if anything breaks. > > Thanks again, > > M. > -- > Jazz is not dead. It just smells funny... _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
