* [PATCH] target/i386: Fixed size of constant for Windows
@ 2021-08-12 11:10 Lara Lazier
2021-08-12 11:37 ` [PATCH-for-6.1?] " Philippe Mathieu-Daudé
2021-08-12 17:48 ` [PATCH] " Paolo Bonzini
0 siblings, 2 replies; 5+ messages in thread
From: Lara Lazier @ 2021-08-12 11:10 UTC (permalink / raw)
To: qemu-devel; +Cc: Volker Rümelin, Lara Lazier
~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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH-for-6.1?] target/i386: Fixed size of constant for Windows
2021-08-12 11:10 [PATCH] target/i386: Fixed size of constant for Windows Lara Lazier
@ 2021-08-12 11:37 ` Philippe Mathieu-Daudé
2021-08-12 12:54 ` Peter Maydell
2021-08-12 17:48 ` [PATCH] " Paolo Bonzini
1 sibling, 1 reply; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-12 11:37 UTC (permalink / raw)
To: Lara Lazier, qemu-devel
Cc: Paolo Bonzini, Thomas Huth, Volker Rümelin,
Richard Henderson, Eric Blake
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?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH-for-6.1?] target/i386: Fixed size of constant for Windows
2021-08-12 11:37 ` [PATCH-for-6.1?] " Philippe Mathieu-Daudé
@ 2021-08-12 12:54 ` Peter Maydell
2021-08-12 12:58 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 5+ messages in thread
From: Peter Maydell @ 2021-08-12 12:54 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Thomas Huth, Lara Lazier, Volker Rümelin, Richard Henderson,
QEMU Developers, Paolo Bonzini, Eric Blake
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH-for-6.1?] target/i386: Fixed size of constant for Windows
2021-08-12 12:54 ` Peter Maydell
@ 2021-08-12 12:58 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-12 12:58 UTC (permalink / raw)
To: Peter Maydell
Cc: Thomas Huth, Lara Lazier, Volker Rümelin, Richard Henderson,
QEMU Developers, Paolo Bonzini, Eric Blake
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] target/i386: Fixed size of constant for Windows
2021-08-12 11:10 [PATCH] target/i386: Fixed size of constant for Windows Lara Lazier
2021-08-12 11:37 ` [PATCH-for-6.1?] " Philippe Mathieu-Daudé
@ 2021-08-12 17:48 ` Paolo Bonzini
1 sibling, 0 replies; 5+ messages in thread
From: Paolo Bonzini @ 2021-08-12 17:48 UTC (permalink / raw)
To: Lara Lazier, qemu-devel; +Cc: Volker Rümelin
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-08-12 17:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-12 11:10 [PATCH] target/i386: Fixed size of constant for Windows Lara Lazier
2021-08-12 11:37 ` [PATCH-for-6.1?] " Philippe Mathieu-Daudé
2021-08-12 12:54 ` Peter Maydell
2021-08-12 12:58 ` Philippe Mathieu-Daudé
2021-08-12 17:48 ` [PATCH] " Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).