| Message ID | 20190615200505.31348-1-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | i386: Fix signedness of hyperv_spinlock_attempts | expand |
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>
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 >
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().
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.
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?
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.
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.
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.
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.
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.
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;
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(-)