On 8/12/21 2:54 PM, Peter Maydell wrote:
> On Thu, 12 Aug 2021 at 12:38, Philippe Mathieu-Daudé <f4...@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_q...@t-online.de>
>>> Signed-off-by: Lara Lazier <laramglaz...@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.mayd...@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4...@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.
