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

[Philippe Mathieu-Daudé]

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.