Re: [PATCH-for-6.1?] target/i386: Fixed size of constant for Windows

[Peter Maydell]

On Thu, 12 Aug 2021 at 12:38, Philippe Mathieu-Daudé <f4bug@amsat.org> 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 <vr_qemu@t-online.de>
> > Signed-off-by: Lara Lazier <laramglazier@gmail.com>
> > ---
> >  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 <peter.maydell@linaro.org>

> >              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