1. Patchew
  2. QEMU
  3. View series

[PATCH] target/i386: Fixed size of constant for Windows

Lara Lazier posted 1 patch 2 years, 8 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210812111056.26926-1-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(-)
[PATCH] target/i386: Fixed size of constant for Windows
Posted by Lara Lazier 2 years, 8 months ago 
~0UL has 64 bits on Linux and 32 bits on Windows.
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/512".

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))) {
             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));
-- 
2.25.1
Re: [PATCH] target/i386: Fixed size of constant for Windows
Posted by Paolo Bonzini 2 years, 8 months ago 
On 12/08/21 13:10, Lara Lazier wrote:
> ~0UL has 64 bits on Linux and 32 bits on Windows.
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/512".
> 
> 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))) {
>               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));
> 

Queued, thanks.

Paolo
Re: [PATCH-for-6.1?] target/i386: Fixed size of constant for Windows
Posted by Philippe Mathieu-Daudé 2 years, 8 months ago 
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) {

>              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?
Re: [PATCH-for-6.1?] target/i386: Fixed size of constant for Windows
Posted by Peter Maydell 2 years, 8 months ago 
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
Re: [PATCH-for-6.1?] target/i386: Fixed size of constant for Windows
Posted by Philippe Mathieu-Daudé 2 years, 8 months ago 
On 8/12/21 2:54 PM, Peter Maydell wrote:
> 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)

Ah, nicer.

> But for this bugfix I think just fixing the suffix is fine.

Sure.

> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.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.

OK, thanks.

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.