1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts

Eduardo Habkost posted 1 patch 4 years, 10 months ago
Test s390x passed
Test checkpatch passed
Test asan passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190615200505.31348-1-ehabkost@redhat.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <rth@twiddle.net>, Eduardo Habkost <ehabkost@redhat.com>
target/i386/cpu.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
Posted by Eduardo Habkost 4 years, 10 months ago 
The current default value for hv-spinlocks is 0xFFFFFFFF (meaning
"never retry").  However, the value is stored as a signed
integer, making the getter of the hv-spinlocks QOM property
return -1 instead of 0xFFFFFFFF.

Fix this by changing the type of X86CPU::hyperv_spinlock_attempts
to uint32_t.  This has no visible effect to guest operating
systems, affecting just the behavior of the QOM getter.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target/i386/cpu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 0732e059ec..8158d0de73 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1372,7 +1372,7 @@ struct X86CPU {
 
     bool hyperv_vapic;
     bool hyperv_relaxed_timing;
-    int hyperv_spinlock_attempts;
+    uint32_t hyperv_spinlock_attempts;
     char *hyperv_vendor_id;
     bool hyperv_time;
     bool hyperv_crash;
-- 
2.18.0.rc1.1.g3f1ff2140
Re: [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
Posted by Vitaly Kuznetsov 4 years, 10 months ago 
Eduardo Habkost <ehabkost@redhat.com> writes:

> The current default value for hv-spinlocks is 0xFFFFFFFF (meaning
> "never retry").  However, the value is stored as a signed
> integer, making the getter of the hv-spinlocks QOM property
> return -1 instead of 0xFFFFFFFF.
>
> Fix this by changing the type of X86CPU::hyperv_spinlock_attempts
> to uint32_t.  This has no visible effect to guest operating
> systems, affecting just the behavior of the QOM getter.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target/i386/cpu.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 0732e059ec..8158d0de73 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1372,7 +1372,7 @@ struct X86CPU {
>  
>      bool hyperv_vapic;
>      bool hyperv_relaxed_timing;
> -    int hyperv_spinlock_attempts;
> +    uint32_t hyperv_spinlock_attempts;
>      char *hyperv_vendor_id;
>      bool hyperv_time;
>      bool hyperv_crash;

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly
Re: [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
Posted by Roman Kagan 4 years, 10 months ago 
On Sat, Jun 15, 2019 at 05:05:05PM -0300, Eduardo Habkost wrote:
> The current default value for hv-spinlocks is 0xFFFFFFFF (meaning
> "never retry").  However, the value is stored as a signed
> integer, making the getter of the hv-spinlocks QOM property
> return -1 instead of 0xFFFFFFFF.
> 
> Fix this by changing the type of X86CPU::hyperv_spinlock_attempts
> to uint32_t.  This has no visible effect to guest operating
> systems, affecting just the behavior of the QOM getter.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target/i386/cpu.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>

That said, it's tempting to just nuke qdev_prop_spinlocks and make
hv-spinlocks a regular DEFINE_PROP_UINT32...

> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 0732e059ec..8158d0de73 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1372,7 +1372,7 @@ struct X86CPU {
>  
>      bool hyperv_vapic;
>      bool hyperv_relaxed_timing;
> -    int hyperv_spinlock_attempts;
> +    uint32_t hyperv_spinlock_attempts;
>      char *hyperv_vendor_id;
>      bool hyperv_time;
>      bool hyperv_crash;
> -- 
> 2.18.0.rc1.1.g3f1ff2140
>
Re: [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
Posted by Eduardo Habkost 4 years, 10 months ago 
On Mon, Jun 17, 2019 at 01:48:59PM +0000, Roman Kagan wrote:
> On Sat, Jun 15, 2019 at 05:05:05PM -0300, Eduardo Habkost wrote:
> > The current default value for hv-spinlocks is 0xFFFFFFFF (meaning
> > "never retry").  However, the value is stored as a signed
> > integer, making the getter of the hv-spinlocks QOM property
> > return -1 instead of 0xFFFFFFFF.
> > 
> > Fix this by changing the type of X86CPU::hyperv_spinlock_attempts
> > to uint32_t.  This has no visible effect to guest operating
> > systems, affecting just the behavior of the QOM getter.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target/i386/cpu.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> 
> That said, it's tempting to just nuke qdev_prop_spinlocks and make
> hv-spinlocks a regular DEFINE_PROP_UINT32...

Agreed.  The only difference is that we would validate the
property at realize time instead of object_property_set().

-- 
Eduardo
Re: [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
Posted by Roman Kagan 4 years, 10 months ago 
On Mon, Jun 17, 2019 at 11:23:01AM -0300, Eduardo Habkost wrote:
> On Mon, Jun 17, 2019 at 01:48:59PM +0000, Roman Kagan wrote:
> > On Sat, Jun 15, 2019 at 05:05:05PM -0300, Eduardo Habkost wrote:
> > > The current default value for hv-spinlocks is 0xFFFFFFFF (meaning
> > > "never retry").  However, the value is stored as a signed
> > > integer, making the getter of the hv-spinlocks QOM property
> > > return -1 instead of 0xFFFFFFFF.
> > > 
> > > Fix this by changing the type of X86CPU::hyperv_spinlock_attempts
> > > to uint32_t.  This has no visible effect to guest operating
> > > systems, affecting just the behavior of the QOM getter.
> > > 
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > >  target/i386/cpu.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> > 
> > That said, it's tempting to just nuke qdev_prop_spinlocks and make
> > hv-spinlocks a regular DEFINE_PROP_UINT32...
> 
> Agreed.  The only difference is that we would validate the
> property at realize time instead of object_property_set().

Right.  But currently it's validated to be no less than 0xfff and no
bigger than 0xffffffff.  The latter check would become unnecessary, and
I'm unable to find any reason to do the former (neither spec references
nor the log messages of the commits that introduced it).

Roman.
Re: [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
Posted by Eduardo Habkost 4 years, 10 months ago 
On Mon, Jun 17, 2019 at 05:32:13PM +0000, Roman Kagan wrote:
> On Mon, Jun 17, 2019 at 11:23:01AM -0300, Eduardo Habkost wrote:
> > On Mon, Jun 17, 2019 at 01:48:59PM +0000, Roman Kagan wrote:
> > > On Sat, Jun 15, 2019 at 05:05:05PM -0300, Eduardo Habkost wrote:
> > > > The current default value for hv-spinlocks is 0xFFFFFFFF (meaning
> > > > "never retry").  However, the value is stored as a signed
> > > > integer, making the getter of the hv-spinlocks QOM property
> > > > return -1 instead of 0xFFFFFFFF.
> > > > 
> > > > Fix this by changing the type of X86CPU::hyperv_spinlock_attempts
> > > > to uint32_t.  This has no visible effect to guest operating
> > > > systems, affecting just the behavior of the QOM getter.
> > > > 
> > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > ---
> > > >  target/i386/cpu.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> > > 
> > > That said, it's tempting to just nuke qdev_prop_spinlocks and make
> > > hv-spinlocks a regular DEFINE_PROP_UINT32...
> > 
> > Agreed.  The only difference is that we would validate the
> > property at realize time instead of object_property_set().
> 
> Right.  But currently it's validated to be no less than 0xfff and no
> bigger than 0xffffffff.  The latter check would become unnecessary, and
> I'm unable to find any reason to do the former (neither spec references
> nor the log messages of the commits that introduced it).

The 0xFFF lower limit was originally introduced by commit
28f52cc04d34 ("hyper-v: introduce Hyper-V support infrastructure").

Vadim, do you know where the 0xFFF limit comes from?

-- 
Eduardo
Re: [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
Posted by Vadim Rozenfeld 4 years, 10 months ago 
On Mon, 2019-06-17 at 14:49 -0300, Eduardo Habkost wrote:
> On Mon, Jun 17, 2019 at 05:32:13PM +0000, Roman Kagan wrote:
> > On Mon, Jun 17, 2019 at 11:23:01AM -0300, Eduardo Habkost wrote:
> > > On Mon, Jun 17, 2019 at 01:48:59PM +0000, Roman Kagan wrote:
> > > > On Sat, Jun 15, 2019 at 05:05:05PM -0300, Eduardo Habkost
> > > > wrote:
> > > > > The current default value for hv-spinlocks is 0xFFFFFFFF
> > > > > (meaning
> > > > > "never retry").  However, the value is stored as a signed
> > > > > integer, making the getter of the hv-spinlocks QOM property
> > > > > return -1 instead of 0xFFFFFFFF.
> > > > > 
> > > > > Fix this by changing the type of
> > > > > X86CPU::hyperv_spinlock_attempts
> > > > > to uint32_t.  This has no visible effect to guest operating
> > > > > systems, affecting just the behavior of the QOM getter.
> > > > > 
> > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > > ---
> > > > >  target/i386/cpu.h | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> > > > 
> > > > That said, it's tempting to just nuke qdev_prop_spinlocks and
> > > > make
> > > > hv-spinlocks a regular DEFINE_PROP_UINT32...
> > > 
> > > Agreed.  The only difference is that we would validate the
> > > property at realize time instead of object_property_set().
> > 
> > Right.  But currently it's validated to be no less than 0xfff and
> > no
> > bigger than 0xffffffff.  The latter check would become unnecessary,
> > and
> > I'm unable to find any reason to do the former (neither spec
> > references
> > nor the log messages of the commits that introduced it).
> 
> The 0xFFF lower limit was originally introduced by commit
> 28f52cc04d34 ("hyper-v: introduce Hyper-V support infrastructure").
> 
> Vadim, do you know where the 0xFFF limit comes from?

I simply took this value from Windows Server 2008 R2 that 
I used as a reference while working on Hyper-V support for KVM.
I also remember some paper (probably published by AMD ???) mentioned
that 0x2fff seemed to have the best balance for PLE logic.


Best,
Vadim.
Re: [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
Posted by Roman Kagan 4 years, 10 months ago 
On Tue, Jun 18, 2019 at 11:24:57AM +1000, Vadim Rozenfeld wrote:
> On Mon, 2019-06-17 at 14:49 -0300, Eduardo Habkost wrote:
> > On Mon, Jun 17, 2019 at 05:32:13PM +0000, Roman Kagan wrote:
> > > On Mon, Jun 17, 2019 at 11:23:01AM -0300, Eduardo Habkost wrote:
> > > > On Mon, Jun 17, 2019 at 01:48:59PM +0000, Roman Kagan wrote:
> > > > > On Sat, Jun 15, 2019 at 05:05:05PM -0300, Eduardo Habkost
> > > > > wrote:
> > > > > > The current default value for hv-spinlocks is 0xFFFFFFFF
> > > > > > (meaning
> > > > > > "never retry").  However, the value is stored as a signed
> > > > > > integer, making the getter of the hv-spinlocks QOM property
> > > > > > return -1 instead of 0xFFFFFFFF.
> > > > > > 
> > > > > > Fix this by changing the type of
> > > > > > X86CPU::hyperv_spinlock_attempts
> > > > > > to uint32_t.  This has no visible effect to guest operating
> > > > > > systems, affecting just the behavior of the QOM getter.
> > > > > > 
> > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > > > ---
> > > > > >  target/i386/cpu.h | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> > > > > 
> > > > > That said, it's tempting to just nuke qdev_prop_spinlocks and
> > > > > make
> > > > > hv-spinlocks a regular DEFINE_PROP_UINT32...
> > > > 
> > > > Agreed.  The only difference is that we would validate the
> > > > property at realize time instead of object_property_set().
> > > 
> > > Right.  But currently it's validated to be no less than 0xfff and
> > > no
> > > bigger than 0xffffffff.  The latter check would become unnecessary,
> > > and
> > > I'm unable to find any reason to do the former (neither spec
> > > references
> > > nor the log messages of the commits that introduced it).
> > 
> > The 0xFFF lower limit was originally introduced by commit
> > 28f52cc04d34 ("hyper-v: introduce Hyper-V support infrastructure").
> > 
> > Vadim, do you know where the 0xFFF limit comes from?
> 
> I simply took this value from Windows Server 2008 R2 that 
> I used as a reference while working on Hyper-V support for KVM.
> I also remember some paper (probably published by AMD ???) mentioned
> that 0x2fff seemed to have the best balance for PLE logic.

The question is whether the user should be disallowed to set it below
0xfff?
I don't see this mandated by the spec, so I'd rather remove the lower
limit and convert the property to a regular DEFINE_PROP_UINT32.

Roman.
Re: [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
Posted by Roman Kagan 4 years, 10 months ago 
On Tue, Jun 18, 2019 at 10:35:05AM +0000, Roman Kagan wrote:
> On Tue, Jun 18, 2019 at 11:24:57AM +1000, Vadim Rozenfeld wrote:
> > On Mon, 2019-06-17 at 14:49 -0300, Eduardo Habkost wrote:
> > > On Mon, Jun 17, 2019 at 05:32:13PM +0000, Roman Kagan wrote:
> > > > On Mon, Jun 17, 2019 at 11:23:01AM -0300, Eduardo Habkost wrote:
> > > > > On Mon, Jun 17, 2019 at 01:48:59PM +0000, Roman Kagan wrote:
> > > > > > On Sat, Jun 15, 2019 at 05:05:05PM -0300, Eduardo Habkost
> > > > > > wrote:
> > > > > > > The current default value for hv-spinlocks is 0xFFFFFFFF
> > > > > > > (meaning
> > > > > > > "never retry").  However, the value is stored as a signed
> > > > > > > integer, making the getter of the hv-spinlocks QOM property
> > > > > > > return -1 instead of 0xFFFFFFFF.
> > > > > > > 
> > > > > > > Fix this by changing the type of
> > > > > > > X86CPU::hyperv_spinlock_attempts
> > > > > > > to uint32_t.  This has no visible effect to guest operating
> > > > > > > systems, affecting just the behavior of the QOM getter.
> > > > > > > 
> > > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > > > > ---
> > > > > > >  target/i386/cpu.h | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> > > > > > 
> > > > > > That said, it's tempting to just nuke qdev_prop_spinlocks and
> > > > > > make
> > > > > > hv-spinlocks a regular DEFINE_PROP_UINT32...
> > > > > 
> > > > > Agreed.  The only difference is that we would validate the
> > > > > property at realize time instead of object_property_set().
> > > > 
> > > > Right.  But currently it's validated to be no less than 0xfff and
> > > > no
> > > > bigger than 0xffffffff.  The latter check would become unnecessary,
> > > > and
> > > > I'm unable to find any reason to do the former (neither spec
> > > > references
> > > > nor the log messages of the commits that introduced it).
> > > 
> > > The 0xFFF lower limit was originally introduced by commit
> > > 28f52cc04d34 ("hyper-v: introduce Hyper-V support infrastructure").
> > > 
> > > Vadim, do you know where the 0xFFF limit comes from?
> > 
> > I simply took this value from Windows Server 2008 R2 that 
> > I used as a reference while working on Hyper-V support for KVM.
> > I also remember some paper (probably published by AMD ???) mentioned
> > that 0x2fff seemed to have the best balance for PLE logic.
> 
> The question is whether the user should be disallowed to set it below
> 0xfff?
> I don't see this mandated by the spec, so I'd rather remove the lower
> limit and convert the property to a regular DEFINE_PROP_UINT32.

I've just posted a patch to that end, feel free to (dis)approve.

Roman.
Re: [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
Posted by Vadim Rozenfeld 4 years, 10 months ago 
On Tue, 2019-06-18 at 10:35 +0000, Roman Kagan wrote:
> On Tue, Jun 18, 2019 at 11:24:57AM +1000, Vadim Rozenfeld wrote:
> > On Mon, 2019-06-17 at 14:49 -0300, Eduardo Habkost wrote:
> > > On Mon, Jun 17, 2019 at 05:32:13PM +0000, Roman Kagan wrote:
> > > > On Mon, Jun 17, 2019 at 11:23:01AM -0300, Eduardo Habkost
> > > > wrote:
> > > > > On Mon, Jun 17, 2019 at 01:48:59PM +0000, Roman Kagan wrote:
> > > > > > On Sat, Jun 15, 2019 at 05:05:05PM -0300, Eduardo Habkost
> > > > > > wrote:
> > > > > > > The current default value for hv-spinlocks is 0xFFFFFFFF
> > > > > > > (meaning
> > > > > > > "never retry").  However, the value is stored as a signed
> > > > > > > integer, making the getter of the hv-spinlocks QOM
> > > > > > > property
> > > > > > > return -1 instead of 0xFFFFFFFF.
> > > > > > > 
> > > > > > > Fix this by changing the type of
> > > > > > > X86CPU::hyperv_spinlock_attempts
> > > > > > > to uint32_t.  This has no visible effect to guest
> > > > > > > operating
> > > > > > > systems, affecting just the behavior of the QOM getter.
> > > > > > > 
> > > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > > > > > ---
> > > > > > >  target/i386/cpu.h | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> > > > > > 
> > > > > > That said, it's tempting to just nuke qdev_prop_spinlocks
> > > > > > and
> > > > > > make
> > > > > > hv-spinlocks a regular DEFINE_PROP_UINT32...
> > > > > 
> > > > > Agreed.  The only difference is that we would validate the
> > > > > property at realize time instead of object_property_set().
> > > > 
> > > > Right.  But currently it's validated to be no less than 0xfff
> > > > and
> > > > no
> > > > bigger than 0xffffffff.  The latter check would become
> > > > unnecessary,
> > > > and
> > > > I'm unable to find any reason to do the former (neither spec
> > > > references
> > > > nor the log messages of the commits that introduced it).
> > > 
> > > The 0xFFF lower limit was originally introduced by commit
> > > 28f52cc04d34 ("hyper-v: introduce Hyper-V support
> > > infrastructure").
> > > 
> > > Vadim, do you know where the 0xFFF limit comes from?
> > 
> > I simply took this value from Windows Server 2008 R2 that 
> > I used as a reference while working on Hyper-V support for KVM.
> > I also remember some paper (probably published by AMD ???)
> > mentioned
> > that 0x2fff seemed to have the best balance for PLE logic.
> 
> The question is whether the user should be disallowed to set it below
> 0xfff?
> I don't see this mandated by the spec, so I'd rather remove the lower
> limit and convert the property to a regular DEFINE_PROP_UINT32.
> 

Honestly, I don't have any strong opinions on this matter. Having some
lower boundary limit seemed quite logical to me. However, if a user
wants to experiment and see how the smaller number of spinlock acquire
attempts before calling HvNotifyLongSpinWait will affect the overall
system performance, then why not?

Vadim.

> Roman.
Re: [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
Posted by Eduardo Habkost 4 years, 10 months ago 
On Sat, Jun 15, 2019 at 05:05:05PM -0300, Eduardo Habkost wrote:
> The current default value for hv-spinlocks is 0xFFFFFFFF (meaning
> "never retry").  However, the value is stored as a signed
> integer, making the getter of the hv-spinlocks QOM property
> return -1 instead of 0xFFFFFFFF.
> 
> Fix this by changing the type of X86CPU::hyperv_spinlock_attempts
> to uint32_t.  This has no visible effect to guest operating
> systems, affecting just the behavior of the QOM getter.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Queued on x86-next.

-- 
Eduardo

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.