| Message ID | 1489744633-28760-7-git-send-email-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote: > --- a/xen/arch/x86/hvm/viridian.c > +++ b/xen/arch/x86/hvm/viridian.c > @@ -22,6 +22,12 @@ > #include <public/sched.h> > #include <public/hvm/hvm_op.h> > > +#define VIRIDIAN_SPINLOCK_RETRY_COUNT_DEFAULT 2047 > + > +static int __read_mostly viridian_spinlock_retry_count; Why don't you simply initialized the variable to 2047? None of ... > @@ -991,6 +1003,16 @@ static int viridian_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h) > HVM_REGISTER_SAVE_RESTORE(VIRIDIAN_VCPU, viridian_save_vcpu_ctxt, > viridian_load_vcpu_ctxt, 1, HVMSR_PER_VCPU); > > +static int __init viridian_init(void) > +{ > + if ( !viridian_spinlock_retry_count ) > + viridian_spinlock_retry_count = > + VIRIDIAN_SPINLOCK_RETRY_COUNT_DEFAULT; > + > + return 0; > +} > +__initcall(viridian_init); ... this would be needed then (and zero, while a useless value, isn't being named invalid by the spec afaics). Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 20 March 2017 12:26 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen- > devel@lists.xenproject.org > Subject: Re: [PATCH 6/7] x86/viridian: make the threshold for > HvNotifyLongSpinWait tunable > > >>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote: > > --- a/xen/arch/x86/hvm/viridian.c > > +++ b/xen/arch/x86/hvm/viridian.c > > @@ -22,6 +22,12 @@ > > #include <public/sched.h> > > #include <public/hvm/hvm_op.h> > > > > +#define VIRIDIAN_SPINLOCK_RETRY_COUNT_DEFAULT 2047 > > + > > +static int __read_mostly viridian_spinlock_retry_count; > > Why don't you simply initialized the variable to 2047? None of ... > I wasn't sure whether that was ok in Xen. I was following other code (mainly from grant table) as a template. I'll change to using an initializer and drop the __initcall. Paul > > @@ -991,6 +1003,16 @@ static int viridian_load_vcpu_ctxt(struct domain > *d, hvm_domain_context_t *h) > > HVM_REGISTER_SAVE_RESTORE(VIRIDIAN_VCPU, > viridian_save_vcpu_ctxt, > > viridian_load_vcpu_ctxt, 1, HVMSR_PER_VCPU); > > > > +static int __init viridian_init(void) > > +{ > > + if ( !viridian_spinlock_retry_count ) > > + viridian_spinlock_retry_count = > > + VIRIDIAN_SPINLOCK_RETRY_COUNT_DEFAULT; > > + > > + return 0; > > +} > > +__initcall(viridian_init); > > ... this would be needed then (and zero, while a useless value, > isn't being named invalid by the spec afaics). > > Jan
>>> On 20.03.17 at 13:51, <Paul.Durrant@citrix.com> wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: 20 March 2017 12:26 >> >>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote: >> > --- a/xen/arch/x86/hvm/viridian.c >> > +++ b/xen/arch/x86/hvm/viridian.c >> > @@ -22,6 +22,12 @@ >> > #include <public/sched.h> >> > #include <public/hvm/hvm_op.h> >> > >> > +#define VIRIDIAN_SPINLOCK_RETRY_COUNT_DEFAULT 2047 >> > + >> > +static int __read_mostly viridian_spinlock_retry_count; >> >> Why don't you simply initialized the variable to 2047? None of ... >> > > I wasn't sure whether that was ok in Xen. I was following other code (mainly > from grant table) as a template. That other approach in grant table code has, iirc, some other reason. Jan
On 17/03/17 09:57, Paul Durrant wrote: > The current threshold before the guest issues the hypercall is, and always > has been, hard-coded to 2047. It is not clear where this number came > from so, to at least allow for ease of experimentation, this patch makes > the threshold tunable via the Xen command line. > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> This is one example of a configuration option which I'd prefer to see done with the (planned) new CPUID interface. The value here doesn't have any impact on the hypervisor, so a configuration option is free to be chosen entirely by the toolstack, and would also become inherently per-domain configuration rather than global. Can I talk you into deferring this change until the CPUID interface is sorted, to avoid introducing yet another command line option? ~Andrew
> -----Original Message----- > From: Andrew Cooper > Sent: 20 March 2017 17:03 > To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org > Cc: Jan Beulich <jbeulich@suse.com> > Subject: Re: [PATCH 6/7] x86/viridian: make the threshold for > HvNotifyLongSpinWait tunable > > On 17/03/17 09:57, Paul Durrant wrote: > > The current threshold before the guest issues the hypercall is, and always > > has been, hard-coded to 2047. It is not clear where this number came > > from so, to at least allow for ease of experimentation, this patch makes > > the threshold tunable via the Xen command line. > > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > > This is one example of a configuration option which I'd prefer to see > done with the (planned) new CPUID interface. > > The value here doesn't have any impact on the hypervisor, so a > configuration option is free to be chosen entirely by the toolstack, and > would also become inherently per-domain configuration rather than global. > > Can I talk you into deferring this change until the CPUID interface is > sorted, to avoid introducing yet another command line option? > I don't really want to wait for something with no definite timescale. I don't see any harm in adding this and then stating it can be overridden by per-domain CPU policy later on. Presumably there's a bunch of other stuff where this would be the case. Paul
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown index 4daf5b5..7f7e0d9 100644 --- a/docs/misc/xen-command-line.markdown +++ b/docs/misc/xen-command-line.markdown @@ -1623,6 +1623,14 @@ The optional `keep` parameter causes Xen to continue using the vga console even after dom0 has been started. The default behaviour is to relinquish control to dom0. +### viridian\_spinlock\_retry\_count +> `= <integer>` + +> Default: `2047` + +Specify the maximum number of retries before an enlightened Windows +guest will notify Xen that it has failed to acquire a spinlock. + ### vpid (Intel) > `= <boolean>` diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c index deb57f9..e7cc4e4 100644 --- a/xen/arch/x86/hvm/viridian.c +++ b/xen/arch/x86/hvm/viridian.c @@ -22,6 +22,12 @@ #include <public/sched.h> #include <public/hvm/hvm_op.h> +#define VIRIDIAN_SPINLOCK_RETRY_COUNT_DEFAULT 2047 + +static int __read_mostly viridian_spinlock_retry_count; +integer_param("viridian_spinlock_retry_count", + viridian_spinlock_retry_count); + /* Viridian MSR numbers. */ #define HV_X64_MSR_GUEST_OS_ID 0x40000000 #define HV_X64_MSR_HYPERCALL 0x40000001 @@ -241,7 +247,13 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf, res->a |= CPUID4A_HCALL_REMOTE_TLB_FLUSH; if ( !cpu_has_vmx_apic_reg_virt ) res->a |= CPUID4A_MSR_BASED_APIC; - res->b = 2047; /* long spin count */ + + /* + * This value is the recommended number of attempts to try to + * acquire a spinlock before notifying the hypervisor via the + * HvNotifyLongSpinWait hypercall. + */ + res->b = viridian_spinlock_retry_count; break; case 6: @@ -991,6 +1003,16 @@ static int viridian_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h) HVM_REGISTER_SAVE_RESTORE(VIRIDIAN_VCPU, viridian_save_vcpu_ctxt, viridian_load_vcpu_ctxt, 1, HVMSR_PER_VCPU); +static int __init viridian_init(void) +{ + if ( !viridian_spinlock_retry_count ) + viridian_spinlock_retry_count = + VIRIDIAN_SPINLOCK_RETRY_COUNT_DEFAULT; + + return 0; +} +__initcall(viridian_init); + /* * Local variables: * mode: C
The current threshold before the guest issues the hypercall is, and always has been, hard-coded to 2047. It is not clear where this number came from so, to at least allow for ease of experimentation, this patch makes the threshold tunable via the Xen command line. Signed-off-by: Paul Durrant <paul.durrant@citrix.com> --- Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- docs/misc/xen-command-line.markdown | 8 ++++++++ xen/arch/x86/hvm/viridian.c | 24 +++++++++++++++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-)