All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
@ 2019-06-15 20:05 Eduardo Habkost
  2019-06-17  6:59 ` Vitaly Kuznetsov
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Eduardo Habkost @ 2019-06-15 20:05 UTC (permalink / raw)
  To: Roman Kagan, Vitaly Kuznetsov, qemu-devel
  Cc: Paolo Bonzini, Eduardo Habkost, Richard Henderson

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



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
  2019-06-15 20:05 [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts Eduardo Habkost
@ 2019-06-17  6:59 ` Vitaly Kuznetsov
  2019-06-17 13:48 ` Roman Kagan
  2019-06-18 22:54 ` Eduardo Habkost
  2 siblings, 0 replies; 11+ messages in thread
From: Vitaly Kuznetsov @ 2019-06-17  6:59 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: qemu-devel, Paolo Bonzini, Roman Kagan, Richard Henderson

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
  2019-06-15 20:05 [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts Eduardo Habkost
  2019-06-17  6:59 ` Vitaly Kuznetsov
@ 2019-06-17 13:48 ` Roman Kagan
  2019-06-17 14:23   ` Eduardo Habkost
  2019-06-18 22:54 ` Eduardo Habkost
  2 siblings, 1 reply; 11+ messages in thread
From: Roman Kagan @ 2019-06-17 13:48 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: Paolo Bonzini, Vitaly Kuznetsov, qemu-devel, Richard Henderson

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
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
  2019-06-17 13:48 ` Roman Kagan
@ 2019-06-17 14:23   ` Eduardo Habkost
  2019-06-17 17:32     ` Roman Kagan
  0 siblings, 1 reply; 11+ messages in thread
From: Eduardo Habkost @ 2019-06-17 14:23 UTC (permalink / raw)
  To: Roman Kagan, Vitaly Kuznetsov, qemu-devel, Paolo Bonzini,
	Richard Henderson

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
  2019-06-17 14:23   ` Eduardo Habkost
@ 2019-06-17 17:32     ` Roman Kagan
  2019-06-17 17:49       ` Eduardo Habkost
  0 siblings, 1 reply; 11+ messages in thread
From: Roman Kagan @ 2019-06-17 17:32 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: Paolo Bonzini, Vitaly Kuznetsov, qemu-devel, Richard Henderson

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.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
  2019-06-17 17:32     ` Roman Kagan
@ 2019-06-17 17:49       ` Eduardo Habkost
  2019-06-18  1:24         ` Vadim Rozenfeld
  0 siblings, 1 reply; 11+ messages in thread
From: Eduardo Habkost @ 2019-06-17 17:49 UTC (permalink / raw)
  To: Roman Kagan, Vitaly Kuznetsov, qemu-devel, Paolo Bonzini,
	Richard Henderson, Vadim Rozenfeld

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
  2019-06-17 17:49       ` Eduardo Habkost
@ 2019-06-18  1:24         ` Vadim Rozenfeld
  2019-06-18 10:35           ` Roman Kagan
  0 siblings, 1 reply; 11+ messages in thread
From: Vadim Rozenfeld @ 2019-06-18  1:24 UTC (permalink / raw)
  To: Eduardo Habkost, Roman Kagan, Vitaly Kuznetsov, qemu-devel,
	Paolo Bonzini, Richard Henderson

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.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
  2019-06-18  1:24         ` Vadim Rozenfeld
@ 2019-06-18 10:35           ` Roman Kagan
  2019-06-18 11:08             ` Roman Kagan
  2019-06-18 11:17             ` Vadim Rozenfeld
  0 siblings, 2 replies; 11+ messages in thread
From: Roman Kagan @ 2019-06-18 10:35 UTC (permalink / raw)
  To: Vadim Rozenfeld
  Cc: Paolo Bonzini, Vitaly Kuznetsov, Richard Henderson,
	Eduardo Habkost, qemu-devel

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.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
  2019-06-18 10:35           ` Roman Kagan
@ 2019-06-18 11:08             ` Roman Kagan
  2019-06-18 11:17             ` Vadim Rozenfeld
  1 sibling, 0 replies; 11+ messages in thread
From: Roman Kagan @ 2019-06-18 11:08 UTC (permalink / raw)
  To: Vadim Rozenfeld
  Cc: qemu-devel, Paolo Bonzini, Vitaly Kuznetsov, Eduardo Habkost,
	Richard Henderson

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.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
  2019-06-18 10:35           ` Roman Kagan
  2019-06-18 11:08             ` Roman Kagan
@ 2019-06-18 11:17             ` Vadim Rozenfeld
  1 sibling, 0 replies; 11+ messages in thread
From: Vadim Rozenfeld @ 2019-06-18 11:17 UTC (permalink / raw)
  To: Roman Kagan
  Cc: Paolo Bonzini, Vitaly Kuznetsov, Richard Henderson,
	Eduardo Habkost, qemu-devel

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.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts
  2019-06-15 20:05 [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts Eduardo Habkost
  2019-06-17  6:59 ` Vitaly Kuznetsov
  2019-06-17 13:48 ` Roman Kagan
@ 2019-06-18 22:54 ` Eduardo Habkost
  2 siblings, 0 replies; 11+ messages in thread
From: Eduardo Habkost @ 2019-06-18 22:54 UTC (permalink / raw)
  To: Roman Kagan, Vitaly Kuznetsov, qemu-devel
  Cc: Paolo Bonzini, Richard Henderson

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2019-06-18 22:56 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-15 20:05 [Qemu-devel] [PATCH] i386: Fix signedness of hyperv_spinlock_attempts Eduardo Habkost
2019-06-17  6:59 ` Vitaly Kuznetsov
2019-06-17 13:48 ` Roman Kagan
2019-06-17 14:23   ` Eduardo Habkost
2019-06-17 17:32     ` Roman Kagan
2019-06-17 17:49       ` Eduardo Habkost
2019-06-18  1:24         ` Vadim Rozenfeld
2019-06-18 10:35           ` Roman Kagan
2019-06-18 11:08             ` Roman Kagan
2019-06-18 11:17             ` Vadim Rozenfeld
2019-06-18 22:54 ` Eduardo Habkost

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.