Re: [Xen-devel] [PATCH] xen: schedule: allow dom0 vCPUs to be re-pinned when dom0_vcpus_pin is set

[Matt Wilson <msw@xxxxxxxxxx>]

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

> > Signed-off-by: Matt Wilson <msw@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel