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

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

[Eduardo Habkost]

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

[Vitaly Kuznetsov]

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

[Roman Kagan]

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

[Eduardo Habkost]

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

[Roman Kagan]

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

[Eduardo Habkost]

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

[Vadim Rozenfeld]

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

[Roman Kagan]

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

[Roman Kagan]

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

[Vadim Rozenfeld]

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

[Eduardo Habkost]

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