* Re: [PATCH] xen: schedule: allow dom0 vCPUs to be re-pinned when dom0_vcpus_pin is set
2012-12-05 15:59 ` Matt Wilson
@ 2012-12-05 16:04 ` Matt Wilson
2012-12-05 16:06 ` Andrew Cooper
2012-12-05 16:25 ` Jan Beulich
2 siblings, 0 replies; 11+ messages in thread
From: Matt Wilson @ 2012-12-05 16:04 UTC (permalink / raw)
To: Andrew Cooper; +Cc: George Dunlap, Keir Fraser, xen-devel@lists.xen.org
On Wed, Dec 05, 2012 at 07:59:08AM -0800, Matt Wilson wrote:
> On Wed, Dec 05, 2012 at 10:44:04AM +0000, Andrew Cooper wrote:
> > On 05/12/12 06:02, Matt Wilson wrote:
> > > An administrator may want Xen to pin dom0 vCPUs to pCPUs 1:1 at boot,
> > > but still have the flexibility to change the configuration later.
> > > There's no logic that keys off of domain->is_pinned outside of
> > > sched_init_vcpu() and vcpu_set_affinity(). By adjusting the
> > > is_pinned_vcpu() macro to only check for a single CPU set in the
> > > cpu_affinity mask, dom0 vCPUs can safely be re-pinned after the system
> > > boots.
> >
> > Sadly this patch will break things. There are certain callers of
> > is_pinned_vcpu() which rely on the value to allow access to certain
> > power related MSRs, which is where the requirement for never permitting
> > an update of the affinity mask comes from.
>
> If this is true, the existing is_pinned_vcpu() test is broken:
>
> #define is_pinned_vcpu(v) ((v)->domain->is_pinned || \
> cpumask_weight((v)->cpu_affinity) == 1)
>
> It's && not ||. So if someone pins dom0 vCPUs to pCPUs 1:1 after boot,
> the MSR traps will suddenly start working.
>
> See commit: http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=cc0854dd
>
> > When I encountered this problem before, I considered implementing
> > dom0_vcpu_pin=dynamic (or name to suit) which sets up an identity pin at
> > create time, but leaves is_pinned as false.
>
> I could implement that, but I want to make sure we're fixing a real
> problem. It sounds like Keir thinks this can be relaxed.
Sorry, misread the commit. Jan made that change.
> Matt
>
> > > Signed-off-by: Matt Wilson <msw@amazon.com>
> > >
> > > diff -r 29247e44df47 -r 2614dd8be3a0 docs/misc/xen-command-line.markdown
> > > --- a/docs/misc/xen-command-line.markdown Fri Nov 30 21:51:17 2012 +0000
> > > +++ b/docs/misc/xen-command-line.markdown Wed Dec 05 05:48:23 2012 +0000
> > > @@ -453,7 +453,7 @@ Practices](http://wiki.xen.org/wiki/Xen_
> > >
> > > > Default: `false`
> > >
> > > -Pin dom0 vcpus to their respective pcpus
> > > +Initially pin dom0 vcpus to their respective pcpus
> > >
> > > ### e820-mtrr-clip
> > > > `= <boolean>`
> > > diff -r 29247e44df47 -r 2614dd8be3a0 xen/common/domain.c
> > > --- a/xen/common/domain.c Fri Nov 30 21:51:17 2012 +0000
> > > +++ b/xen/common/domain.c Wed Dec 05 05:48:23 2012 +0000
> > > @@ -45,10 +45,6 @@
> > > /* xen_processor_pmbits: xen control Cx, Px, ... */
> > > unsigned int xen_processor_pmbits = XEN_PROCESSOR_PM_PX;
> > >
> > > -/* opt_dom0_vcpus_pin: If true, dom0 VCPUs are pinned. */
> > > -bool_t opt_dom0_vcpus_pin;
> > > -boolean_param("dom0_vcpus_pin", opt_dom0_vcpus_pin);
> > > -
> > > /* Protect updates/reads (resp.) of domain_list and domain_hash. */
> > > DEFINE_SPINLOCK(domlist_update_lock);
> > > DEFINE_RCU_READ_LOCK(domlist_read_lock);
> > > @@ -235,7 +231,6 @@ struct domain *domain_create(
> > >
> > > if ( domid == 0 )
> > > {
> > > - d->is_pinned = opt_dom0_vcpus_pin;
> > > d->disable_migrate = 1;
> > > }
> > >
> > > diff -r 29247e44df47 -r 2614dd8be3a0 xen/common/schedule.c
> > > --- a/xen/common/schedule.c Fri Nov 30 21:51:17 2012 +0000
> > > +++ b/xen/common/schedule.c Wed Dec 05 05:48:23 2012 +0000
> > > @@ -52,6 +52,11 @@ boolean_param("sched_smt_power_savings",
> > > * */
> > > int sched_ratelimit_us = SCHED_DEFAULT_RATELIMIT_US;
> > > integer_param("sched_ratelimit_us", sched_ratelimit_us);
> > > +
> > > +/* opt_dom0_vcpus_pin: If true, dom0 VCPUs are pinned at boot. */
> > > +bool_t opt_dom0_vcpus_pin;
> > > +boolean_param("dom0_vcpus_pin", opt_dom0_vcpus_pin);
> > > +
> > > /* Various timer handlers. */
> > > static void s_timer_fn(void *unused);
> > > static void vcpu_periodic_timer_fn(void *data);
> > > @@ -194,7 +199,8 @@ int sched_init_vcpu(struct vcpu *v, unsi
> > > * domain-0 VCPUs, are pinned onto their respective physical CPUs.
> > > */
> > > v->processor = processor;
> > > - if ( is_idle_domain(d) || d->is_pinned )
> > > +
> > > + if ( is_idle_domain(d) || (d->domain_id == 0 && opt_dom0_vcpus_pin) )
> > > cpumask_copy(v->cpu_affinity, cpumask_of(processor));
> > > else
> > > cpumask_setall(v->cpu_affinity);
> > > @@ -595,8 +601,6 @@ int vcpu_set_affinity(struct vcpu *v, co
> > > cpumask_t online_affinity;
> > > cpumask_t *online;
> > >
> > > - if ( v->domain->is_pinned )
> > > - return -EINVAL;
> > > online = VCPU2ONLINE(v);
> > > cpumask_and(&online_affinity, affinity, online);
> > > if ( cpumask_empty(&online_affinity) )
> > > diff -r 29247e44df47 -r 2614dd8be3a0 xen/include/xen/sched.h
> > > --- a/xen/include/xen/sched.h Fri Nov 30 21:51:17 2012 +0000
> > > +++ b/xen/include/xen/sched.h Wed Dec 05 05:48:23 2012 +0000
> > > @@ -292,8 +292,6 @@ struct domain
> > > enum { DOMDYING_alive, DOMDYING_dying, DOMDYING_dead } is_dying;
> > > /* Domain is paused by controller software? */
> > > bool_t is_paused_by_controller;
> > > - /* Domain's VCPUs are pinned 1:1 to physical CPUs? */
> > > - bool_t is_pinned;
> > >
> > > /* Are any VCPUs polling event channels (SCHEDOP_poll)? */
> > > #if MAX_VIRT_CPUS <= BITS_PER_LONG
> > > @@ -713,8 +711,7 @@ void watchdog_domain_destroy(struct doma
> > >
> > > #define is_hvm_domain(d) ((d)->is_hvm)
> > > #define is_hvm_vcpu(v) (is_hvm_domain(v->domain))
> > > -#define is_pinned_vcpu(v) ((v)->domain->is_pinned || \
> > > - cpumask_weight((v)->cpu_affinity) == 1)
> > > +#define is_pinned_vcpu(v) (cpumask_weight((v)->cpu_affinity) == 1)
> > > #ifdef HAS_PASSTHROUGH
> > > #define need_iommu(d) ((d)->need_iommu)
> > > #else
> > >
> > > _______________________________________________
> > > Xen-devel mailing list
> > > Xen-devel@lists.xen.org
> > > http://lists.xen.org/xen-devel
> >
> > --
> > Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
> > T: +44 (0)1223 225 900, http://www.citrix.com
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] xen: schedule: allow dom0 vCPUs to be re-pinned when dom0_vcpus_pin is set
2012-12-05 15:59 ` Matt Wilson
2012-12-05 16:04 ` Matt Wilson
@ 2012-12-05 16:06 ` Andrew Cooper
2012-12-05 16:25 ` Jan Beulich
2 siblings, 0 replies; 11+ messages in thread
From: Andrew Cooper @ 2012-12-05 16:06 UTC (permalink / raw)
To: Matt Wilson; +Cc: George Dunlap, Keir (Xen.org), xen-devel@lists.xen.org
On 05/12/12 15:59, Matt Wilson wrote:
> On Wed, Dec 05, 2012 at 10:44:04AM +0000, Andrew Cooper wrote:
>> On 05/12/12 06:02, Matt Wilson wrote:
>>> An administrator may want Xen to pin dom0 vCPUs to pCPUs 1:1 at boot,
>>> but still have the flexibility to change the configuration later.
>>> There's no logic that keys off of domain->is_pinned outside of
>>> sched_init_vcpu() and vcpu_set_affinity(). By adjusting the
>>> is_pinned_vcpu() macro to only check for a single CPU set in the
>>> cpu_affinity mask, dom0 vCPUs can safely be re-pinned after the system
>>> boots.
>> Sadly this patch will break things. There are certain callers of
>> is_pinned_vcpu() which rely on the value to allow access to certain
>> power related MSRs, which is where the requirement for never permitting
>> an update of the affinity mask comes from.
> If this is true, the existing is_pinned_vcpu() test is broken:
>
> #define is_pinned_vcpu(v) ((v)->domain->is_pinned || \
> cpumask_weight((v)->cpu_affinity) == 1)
>
> It's && not ||. So if someone pins dom0 vCPUs to pCPUs 1:1 after boot,
> the MSR traps will suddenly start working.
>
> See commit: http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=cc0854dd
>
>> When I encountered this problem before, I considered implementing
>> dom0_vcpu_pin=dynamic (or name to suit) which sets up an identity pin at
>> create time, but leaves is_pinned as false.
> I could implement that, but I want to make sure we're fixing a real
> problem. It sounds like Keir thinks this can be relaxed.
>
> Matt
Hmm - the logic does indeed look broken. When I was working on this
problem, it was an older tree without this changeset.
With the current logic, the vcpu will transparently move to a different
set of MSRs whenever the affinity is changed, which does not sound safe
for an unaware vcpu. I did not pay much attention to the function of
the MSRs.
~Andrew
>
>>> Signed-off-by: Matt Wilson <msw@amazon.com>
>>>
>>> diff -r 29247e44df47 -r 2614dd8be3a0 docs/misc/xen-command-line.markdown
>>> --- a/docs/misc/xen-command-line.markdown Fri Nov 30 21:51:17 2012 +0000
>>> +++ b/docs/misc/xen-command-line.markdown Wed Dec 05 05:48:23 2012 +0000
>>> @@ -453,7 +453,7 @@ Practices](http://wiki.xen.org/wiki/Xen_
>>>
>>> > Default: `false`
>>>
>>> -Pin dom0 vcpus to their respective pcpus
>>> +Initially pin dom0 vcpus to their respective pcpus
>>>
>>> ### e820-mtrr-clip
>>> > `= <boolean>`
>>> diff -r 29247e44df47 -r 2614dd8be3a0 xen/common/domain.c
>>> --- a/xen/common/domain.c Fri Nov 30 21:51:17 2012 +0000
>>> +++ b/xen/common/domain.c Wed Dec 05 05:48:23 2012 +0000
>>> @@ -45,10 +45,6 @@
>>> /* xen_processor_pmbits: xen control Cx, Px, ... */
>>> unsigned int xen_processor_pmbits = XEN_PROCESSOR_PM_PX;
>>>
>>> -/* opt_dom0_vcpus_pin: If true, dom0 VCPUs are pinned. */
>>> -bool_t opt_dom0_vcpus_pin;
>>> -boolean_param("dom0_vcpus_pin", opt_dom0_vcpus_pin);
>>> -
>>> /* Protect updates/reads (resp.) of domain_list and domain_hash. */
>>> DEFINE_SPINLOCK(domlist_update_lock);
>>> DEFINE_RCU_READ_LOCK(domlist_read_lock);
>>> @@ -235,7 +231,6 @@ struct domain *domain_create(
>>>
>>> if ( domid == 0 )
>>> {
>>> - d->is_pinned = opt_dom0_vcpus_pin;
>>> d->disable_migrate = 1;
>>> }
>>>
>>> diff -r 29247e44df47 -r 2614dd8be3a0 xen/common/schedule.c
>>> --- a/xen/common/schedule.c Fri Nov 30 21:51:17 2012 +0000
>>> +++ b/xen/common/schedule.c Wed Dec 05 05:48:23 2012 +0000
>>> @@ -52,6 +52,11 @@ boolean_param("sched_smt_power_savings",
>>> * */
>>> int sched_ratelimit_us = SCHED_DEFAULT_RATELIMIT_US;
>>> integer_param("sched_ratelimit_us", sched_ratelimit_us);
>>> +
>>> +/* opt_dom0_vcpus_pin: If true, dom0 VCPUs are pinned at boot. */
>>> +bool_t opt_dom0_vcpus_pin;
>>> +boolean_param("dom0_vcpus_pin", opt_dom0_vcpus_pin);
>>> +
>>> /* Various timer handlers. */
>>> static void s_timer_fn(void *unused);
>>> static void vcpu_periodic_timer_fn(void *data);
>>> @@ -194,7 +199,8 @@ int sched_init_vcpu(struct vcpu *v, unsi
>>> * domain-0 VCPUs, are pinned onto their respective physical CPUs.
>>> */
>>> v->processor = processor;
>>> - if ( is_idle_domain(d) || d->is_pinned )
>>> +
>>> + if ( is_idle_domain(d) || (d->domain_id == 0 && opt_dom0_vcpus_pin) )
>>> cpumask_copy(v->cpu_affinity, cpumask_of(processor));
>>> else
>>> cpumask_setall(v->cpu_affinity);
>>> @@ -595,8 +601,6 @@ int vcpu_set_affinity(struct vcpu *v, co
>>> cpumask_t online_affinity;
>>> cpumask_t *online;
>>>
>>> - if ( v->domain->is_pinned )
>>> - return -EINVAL;
>>> online = VCPU2ONLINE(v);
>>> cpumask_and(&online_affinity, affinity, online);
>>> if ( cpumask_empty(&online_affinity) )
>>> diff -r 29247e44df47 -r 2614dd8be3a0 xen/include/xen/sched.h
>>> --- a/xen/include/xen/sched.h Fri Nov 30 21:51:17 2012 +0000
>>> +++ b/xen/include/xen/sched.h Wed Dec 05 05:48:23 2012 +0000
>>> @@ -292,8 +292,6 @@ struct domain
>>> enum { DOMDYING_alive, DOMDYING_dying, DOMDYING_dead } is_dying;
>>> /* Domain is paused by controller software? */
>>> bool_t is_paused_by_controller;
>>> - /* Domain's VCPUs are pinned 1:1 to physical CPUs? */
>>> - bool_t is_pinned;
>>>
>>> /* Are any VCPUs polling event channels (SCHEDOP_poll)? */
>>> #if MAX_VIRT_CPUS <= BITS_PER_LONG
>>> @@ -713,8 +711,7 @@ void watchdog_domain_destroy(struct doma
>>>
>>> #define is_hvm_domain(d) ((d)->is_hvm)
>>> #define is_hvm_vcpu(v) (is_hvm_domain(v->domain))
>>> -#define is_pinned_vcpu(v) ((v)->domain->is_pinned || \
>>> - cpumask_weight((v)->cpu_affinity) == 1)
>>> +#define is_pinned_vcpu(v) (cpumask_weight((v)->cpu_affinity) == 1)
>>> #ifdef HAS_PASSTHROUGH
>>> #define need_iommu(d) ((d)->need_iommu)
>>> #else
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xen.org/xen-devel
>> --
>> Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
>> T: +44 (0)1223 225 900, http://www.citrix.com
--
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] xen: schedule: allow dom0 vCPUs to be re-pinned when dom0_vcpus_pin is set
2012-12-05 15:59 ` Matt Wilson
2012-12-05 16:04 ` Matt Wilson
2012-12-05 16:06 ` Andrew Cooper
@ 2012-12-05 16:25 ` Jan Beulich
2012-12-05 17:06 ` Matt Wilson
2 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2012-12-05 16:25 UTC (permalink / raw)
To: Matt Wilson, Andrew Cooper
Cc: George Dunlap, Keir Fraser, xen-devel@lists.xen.org
>>> On 05.12.12 at 16:59, Matt Wilson <msw@amazon.com> wrote:
> On Wed, Dec 05, 2012 at 10:44:04AM +0000, Andrew Cooper wrote:
>> On 05/12/12 06:02, Matt Wilson wrote:
>> > An administrator may want Xen to pin dom0 vCPUs to pCPUs 1:1 at boot,
>> > but still have the flexibility to change the configuration later.
>> > There's no logic that keys off of domain->is_pinned outside of
>> > sched_init_vcpu() and vcpu_set_affinity(). By adjusting the
>> > is_pinned_vcpu() macro to only check for a single CPU set in the
>> > cpu_affinity mask, dom0 vCPUs can safely be re-pinned after the system
>> > boots.
>>
>> Sadly this patch will break things. There are certain callers of
>> is_pinned_vcpu() which rely on the value to allow access to certain
>> power related MSRs, which is where the requirement for never permitting
>> an update of the affinity mask comes from.
>
> If this is true, the existing is_pinned_vcpu() test is broken:
>
> #define is_pinned_vcpu(v) ((v)->domain->is_pinned || \
> cpumask_weight((v)->cpu_affinity) == 1)
>
> It's && not ||. So if someone pins dom0 vCPUs to pCPUs 1:1 after boot,
> the MSR traps will suddenly start working.
>
> See commit: http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=cc0854dd
I don't see what's wrong here. Certain things merely require the
pCPU that a vCPU runs on to be stable, which is what the test
above is for.
Jan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xen: schedule: allow dom0 vCPUs to be re-pinned when dom0_vcpus_pin is set
2012-12-05 16:25 ` Jan Beulich
@ 2012-12-05 17:06 ` Matt Wilson
2012-12-05 17:16 ` George Dunlap
2012-12-06 10:44 ` Jan Beulich
0 siblings, 2 replies; 11+ messages in thread
From: Matt Wilson @ 2012-12-05 17:06 UTC (permalink / raw)
To: Jan Beulich
Cc: George Dunlap, Andrew Cooper, Keir Fraser,
xen-devel@lists.xen.org
On Wed, Dec 05, 2012 at 04:25:36PM +0000, Jan Beulich wrote:
> >>> On 05.12.12 at 16:59, Matt Wilson <msw@amazon.com> wrote:
> > On Wed, Dec 05, 2012 at 10:44:04AM +0000, Andrew Cooper wrote:
> >> On 05/12/12 06:02, Matt Wilson wrote:
> >> > An administrator may want Xen to pin dom0 vCPUs to pCPUs 1:1 at boot,
> >> > but still have the flexibility to change the configuration later.
> >> > There's no logic that keys off of domain->is_pinned outside of
> >> > sched_init_vcpu() and vcpu_set_affinity(). By adjusting the
> >> > is_pinned_vcpu() macro to only check for a single CPU set in the
> >> > cpu_affinity mask, dom0 vCPUs can safely be re-pinned after the system
> >> > boots.
> >>
> >> Sadly this patch will break things. There are certain callers of
> >> is_pinned_vcpu() which rely on the value to allow access to certain
> >> power related MSRs, which is where the requirement for never permitting
> >> an update of the affinity mask comes from.
> >
> > If this is true, the existing is_pinned_vcpu() test is broken:
> >
> > #define is_pinned_vcpu(v) ((v)->domain->is_pinned || \
> > cpumask_weight((v)->cpu_affinity) == 1)
> >
> > It's && not ||. So if someone pins dom0 vCPUs to pCPUs 1:1 after boot,
> > the MSR traps will suddenly start working.
> >
> > See commit: http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=cc0854dd
>
> I don't see what's wrong here. Certain things merely require the
> pCPU that a vCPU runs on to be stable, which is what the test
> above is for.
Me either. That said, are you willing to Ack and commit my patch that
started this thread?
Thanks,
Matt
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xen: schedule: allow dom0 vCPUs to be re-pinned when dom0_vcpus_pin is set
2012-12-05 17:06 ` Matt Wilson
@ 2012-12-05 17:16 ` George Dunlap
2012-12-06 10:44 ` Jan Beulich
1 sibling, 0 replies; 11+ messages in thread
From: George Dunlap @ 2012-12-05 17:16 UTC (permalink / raw)
To: Matt Wilson
Cc: Andrew Cooper, Keir Fraser, Jan Beulich, xen-devel@lists.xen.org
[-- Attachment #1.1: Type: text/plain, Size: 301 bytes --]
On Wed, Dec 5, 2012 at 5:06 PM, Matt Wilson <msw@amazon.com> wrote:
> Me either. That said, are you willing to Ack and commit my patch that
> started this thread?
>
FWIW, I'm OK with the scheduler side of things; can't comment on the MSR
issue:
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
[-- Attachment #1.2: Type: text/html, Size: 689 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xen: schedule: allow dom0 vCPUs to be re-pinned when dom0_vcpus_pin is set
2012-12-05 17:06 ` Matt Wilson
2012-12-05 17:16 ` George Dunlap
@ 2012-12-06 10:44 ` Jan Beulich
2012-12-10 22:01 ` Matt Wilson
1 sibling, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2012-12-06 10:44 UTC (permalink / raw)
To: Matt Wilson
Cc: George Dunlap, Andrew Cooper, Keir Fraser,
xen-devel@lists.xen.org
>>> On 05.12.12 at 18:06, Matt Wilson <msw@amazon.com> wrote:
> On Wed, Dec 05, 2012 at 04:25:36PM +0000, Jan Beulich wrote:
>> >>> On 05.12.12 at 16:59, Matt Wilson <msw@amazon.com> wrote:
>> > On Wed, Dec 05, 2012 at 10:44:04AM +0000, Andrew Cooper wrote:
>> >> On 05/12/12 06:02, Matt Wilson wrote:
>> >> > An administrator may want Xen to pin dom0 vCPUs to pCPUs 1:1 at boot,
>> >> > but still have the flexibility to change the configuration later.
>> >> > There's no logic that keys off of domain->is_pinned outside of
>> >> > sched_init_vcpu() and vcpu_set_affinity(). By adjusting the
>> >> > is_pinned_vcpu() macro to only check for a single CPU set in the
>> >> > cpu_affinity mask, dom0 vCPUs can safely be re-pinned after the system
>> >> > boots.
>> >>
>> >> Sadly this patch will break things. There are certain callers of
>> >> is_pinned_vcpu() which rely on the value to allow access to certain
>> >> power related MSRs, which is where the requirement for never permitting
>> >> an update of the affinity mask comes from.
>> >
>> > If this is true, the existing is_pinned_vcpu() test is broken:
>> >
>> > #define is_pinned_vcpu(v) ((v)->domain->is_pinned || \
>> > cpumask_weight((v)->cpu_affinity) == 1)
>> >
>> > It's && not ||. So if someone pins dom0 vCPUs to pCPUs 1:1 after boot,
>> > the MSR traps will suddenly start working.
>> >
>> > See commit:
> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=cc0854dd
>>
>> I don't see what's wrong here. Certain things merely require the
>> pCPU that a vCPU runs on to be stable, which is what the test
>> above is for.
>
> Me either. That said, are you willing to Ack and commit my patch that
> started this thread?
In no case without Andrew's concerns addressed. Beyond that,
I'd be hesitant to ack it as I'm myself suspecting side effects that
we don't want and/or aren't aware of, and in no case could I
commit it without Keir's ack.
Jan
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xen: schedule: allow dom0 vCPUs to be re-pinned when dom0_vcpus_pin is set
2012-12-06 10:44 ` Jan Beulich
@ 2012-12-10 22:01 ` Matt Wilson
2012-12-11 10:43 ` Jan Beulich
0 siblings, 1 reply; 11+ messages in thread
From: Matt Wilson @ 2012-12-10 22:01 UTC (permalink / raw)
To: Jan Beulich
Cc: George Dunlap, Andrew Cooper, Keir Fraser,
xen-devel@lists.xen.org
On Thu, 6 Dec 2012 10:44:04 +0000, Jan Beulich <JBeulich@suse.com> wrote:
> On Wed, 5 Dec 2012 09:06:20 -0800, Matt Wilson <msw@amazon.com> wrote:
> > On Wed, 5 Dec 2012 16:25:36 +0000, Jan Beulich <JBeulich@suse.com> wrote:
> > > On Wed, 5 Dec 2012 07:59:08 -0800, Matt Wilson <msw@amazon.com> wrote:
> > > >
> > > > If this is true, the existing is_pinned_vcpu() test is broken:
> > > >
> > > > #define is_pinned_vcpu(v) ((v)->domain->is_pinned || \
> > > > cpumask_weight((v)->cpu_affinity) == 1)
> > > >
> > > > It's && not ||. So if someone pins dom0 vCPUs to pCPUs 1:1 after boot,
> > > > the MSR traps will suddenly start working.
> > > >
> > > > See commit:
> > > > http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=cc0854dd
> > >
> > > I don't see what's wrong here. Certain things merely require the
> > > pCPU that a vCPU runs on to be stable, which is what the test
> > > above is for.
> >
> > Me either. That said, are you willing to Ack and commit my patch that
> > started this thread?
>
> In no case without Andrew's concerns addressed. Beyond that,
> I'd be hesitant to ack it as I'm myself suspecting side effects that
> we don't want and/or aren't aware of, and in no case could I
> commit it without Keir's ack.
Jan,
So today if I boot Xen without dom0_vcpus_pin set, dom0's vCPUs will
be allowed to run on any pCPU. Xen will block attempts to write
certain MSRs (MSR_AMD64_NB_CFG, MSR_FAM10H_MMIO_CONF_BASE and
MSR_IA32_ENERGY_PERF_BIAS). The VCPUOP_get_physid subop of the vcpu_op
hypercall will not return the initial APIC ID or ACPI ID for dom0.
Also today, if I run "xl vcpu-pin 0 0", suddenly those MSR writes and
the VCPUOP_get_physid hypercall will start working for vCPU 0. For
what it's worth, only legacy XenoLinux-derived kernels appear to use
this hypercall during SMP boot. Upstream Linux does not.
I think that the real risk is in the XenoLinux SMP booting code on AMD
processors where sometimes initial APIC ID != ACPI ID. If the CPU
pinning changes, the ACPI ID to APIC ID mapping will be wrong. This
broke PowerNow! when it ran in dom0.
But PowerNow! is handled by the hypervisor now. So what's the real
danger here?
Andrew, your thoughts?
Thanks,
Matt
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] xen: schedule: allow dom0 vCPUs to be re-pinned when dom0_vcpus_pin is set
2012-12-10 22:01 ` Matt Wilson
@ 2012-12-11 10:43 ` Jan Beulich
0 siblings, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2012-12-11 10:43 UTC (permalink / raw)
To: Matt Wilson
Cc: George Dunlap, Andrew Cooper, Keir Fraser,
xen-devel@lists.xen.org
>>> On 10.12.12 at 23:01, Matt Wilson <msw@amazon.com> wrote:
> So today if I boot Xen without dom0_vcpus_pin set, dom0's vCPUs will
> be allowed to run on any pCPU. Xen will block attempts to write
> certain MSRs (MSR_AMD64_NB_CFG, MSR_FAM10H_MMIO_CONF_BASE and
> MSR_IA32_ENERGY_PERF_BIAS). The VCPUOP_get_physid subop of the vcpu_op
> hypercall will not return the initial APIC ID or ACPI ID for dom0.
>
> Also today, if I run "xl vcpu-pin 0 0", suddenly those MSR writes and
> the VCPUOP_get_physid hypercall will start working for vCPU 0. For
Exactly as intended.
> what it's worth, only legacy XenoLinux-derived kernels appear to use
> this hypercall during SMP boot. Upstream Linux does not.
Because that's being used for the "cpufreq=dom0" case, which I
don't think the pv-ops kernel really ever tried to support.
> I think that the real risk is in the XenoLinux SMP booting code on AMD
> processors where sometimes initial APIC ID != ACPI ID. If the CPU
> pinning changes, the ACPI ID to APIC ID mapping will be wrong. This
> broke PowerNow! when it ran in dom0.
>
> But PowerNow! is handled by the hypervisor now. So what's the real
> danger here?
I never liked the idea of "cpufreq=dom0", and our kernels
intentionally make it impossible to be used (i.e. much like
pv-ops). But for there potentially being people wanting this, we
can't simply rip it out.
The real danger is with exactly what you describe - a careless
caller using the result of any of the then possible operations in
a context where they're not valid anymore.
And there are use cases outside of Dom0-based power
management: If carefully done, this allows implementation of
a kernel driver (extension) similar to x86's msr.ko, allowing
access to the physical CPU's MSRs. And properly implementing
the dcdbas, coretemp, and via-cputemp drivers.
Jan
^ permalink raw reply [flat|nested] 11+ messages in thread