On Thu, 12 Aug 2021 at 12:38, Philippe Mathieu-Daudé <[email protected]> wrote: > > On 8/12/21 1:10 PM, Lara Lazier wrote: > > ~0UL has 64 bits on Linux and 32 bits on Windows. > > > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/512";. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/512 > Fixes: 3407259b20c ("target/i386: Added consistency checks for CR3") > > > Reported-by: Volker Rümelin <[email protected]> > > Signed-off-by: Lara Lazier <[email protected]> > > --- > > target/i386/tcg/sysemu/misc_helper.c | 2 +- > > target/i386/tcg/sysemu/svm_helper.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/target/i386/tcg/sysemu/misc_helper.c > > b/target/i386/tcg/sysemu/misc_helper.c > > index 0a7a58ca9a..91b0fc916b 100644 > > --- a/target/i386/tcg/sysemu/misc_helper.c > > +++ b/target/i386/tcg/sysemu/misc_helper.c > > @@ -97,7 +97,7 @@ void helper_write_crN(CPUX86State *env, int reg, > > target_ulong t0) > > break; > > case 3: > > if ((env->efer & MSR_EFER_LMA) && > > - (t0 & ((~0UL) << env_archcpu(env)->phys_bits))) { > > + (t0 & ((~0ULL) << env_archcpu(env)->phys_bits))) { > > Maybe safer to use extract64()? I'm not sure how many bits you want > to check, so used TARGET_LONG_BITS: > > extract64(t0, env_archcpu(env)->phys_bits, > TARGET_LONG_BITS - env_archcpu(env)->phys_bits) {
The repetition of the 'phys_bits' expression in both the shift and the length field is pretty awkward. Maybe t0 & ~MAKE_64BIT_MASK(0, env_archcpu(env)->phys_bits) But for this bugfix I think just fixing the suffix is fine. Reviewed-by: Peter Maydell <[email protected]> > > cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC()); > > } > > if (!(env->efer & MSR_EFER_LMA)) { > > diff --git a/target/i386/tcg/sysemu/svm_helper.c > > b/target/i386/tcg/sysemu/svm_helper.c > > index 3891f4e4a8..42cd5053e5 100644 > > --- a/target/i386/tcg/sysemu/svm_helper.c > > +++ b/target/i386/tcg/sysemu/svm_helper.c > > @@ -290,7 +290,7 @@ void helper_vmrun(CPUX86State *env, int aflag, int > > next_eip_addend) > > } > > new_cr3 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, > > save.cr3)); > > if ((env->efer & MSR_EFER_LMA) && > > - (new_cr3 & ((~0UL) << cpu->phys_bits))) { > > + (new_cr3 & ((~0ULL) << cpu->phys_bits))) { > > cpu_vmexit(env, SVM_EXIT_ERR, 0, GETPC()); > > } > > new_cr4 = x86_ldq_phys(cs, env->vm_vmcb + offsetof(struct vmcb, > > save.cr4)); > > > > Any idea how to avoid that class of bug? Some CFLAG or plain checkpatch? In general the 'UL' is often dubious and worth checking -- either 'U' suffices or you wanted 'ULL'. But in a few places where you're actually working with 'unsigned long' it's what you need, so I'm not sure a checkpatch rule would be possible. -- PMM
