| Message ID | 20241206130221.17773-5-jgross@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | remove libxenctrl usage from xenstored | expand |
On 06.12.2024 14:02, Juergen Gross wrote: > Add a bitmap with one bit per possible domid indicating the respective > domain has changed its state (created, deleted, dying, crashed, > shutdown). > > Registering the VIRQ_DOM_EXC event will result in setting the bits for > all existing domains and resetting all other bits. > > Resetting a bit will be done in a future patch. > > This information is needed for Xenstore to keep track of all domains. > > Signed-off-by: Juergen Gross <jgross@suse.com> What I'm still missing is at least mention of the global-ness of all of this, and why that's deemed okay for now. > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -138,6 +138,60 @@ bool __read_mostly vmtrace_available; > > bool __read_mostly vpmu_is_available; > > +static DEFINE_SPINLOCK(dom_state_changed_lock); > +static unsigned long *dom_state_changed; > + > +int domain_init_states(void) > +{ > + const struct domain *d; > + int rc = -ENOMEM; > + > + spin_lock(&dom_state_changed_lock); > + > + if ( dom_state_changed ) > + bitmap_zero(dom_state_changed, DOMID_FIRST_RESERVED); > + else > + { > + dom_state_changed = xzalloc_array(unsigned long, > + BITS_TO_LONGS(DOMID_FIRST_RESERVED)); New code wants to use xvmalloc() et al. > --- a/xen/common/event_channel.c > +++ b/xen/common/event_channel.c > @@ -485,20 +485,27 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port) > if ( (v = domain_vcpu(d, vcpu)) == NULL ) > return -ENOENT; > > + if ( virq == VIRQ_DOM_EXC ) > + { > + rc = domain_init_states(); > + if ( rc ) > + goto out; > + } > + > write_lock(&d->event_lock); > > if ( read_atomic(&v->virq_to_evtchn[virq]) ) > { > rc = -EEXIST; > gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc); > - goto out; > + goto unlock; > } > > port = rc = evtchn_get_port(d, port); > if ( rc < 0 ) > { > gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc); > - goto out; > + goto unlock; > } > > rc = 0; > @@ -524,9 +531,13 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port) > */ > write_atomic(&v->virq_to_evtchn[virq], port); > > - out: > + unlock: > write_unlock(&d->event_lock); > > + out: > + if ( rc ) > + domain_deinit_states(); > + > return rc; > } Renaming the prior label (and hence needing to fiddle with existing goto-s) feels a little fragile. How about keeping "out" as is and introducing "deinit" or some such? Jan
On 09.12.24 17:52, Jan Beulich wrote: > On 06.12.2024 14:02, Juergen Gross wrote: >> Add a bitmap with one bit per possible domid indicating the respective >> domain has changed its state (created, deleted, dying, crashed, >> shutdown). >> >> Registering the VIRQ_DOM_EXC event will result in setting the bits for >> all existing domains and resetting all other bits. >> >> Resetting a bit will be done in a future patch. >> >> This information is needed for Xenstore to keep track of all domains. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> > > What I'm still missing is at least mention of the global-ness of all of > this, and why that's deemed okay for now. I'll add: As the usage of this bitmap is tightly coupled with the VIRQ_DOM_EXC event, it is meant to be used only by a single consumer in the system, just like the VIRQ_DOM_EXC event. > >> --- a/xen/common/domain.c >> +++ b/xen/common/domain.c >> @@ -138,6 +138,60 @@ bool __read_mostly vmtrace_available; >> >> bool __read_mostly vpmu_is_available; >> >> +static DEFINE_SPINLOCK(dom_state_changed_lock); >> +static unsigned long *dom_state_changed; >> + >> +int domain_init_states(void) >> +{ >> + const struct domain *d; >> + int rc = -ENOMEM; >> + >> + spin_lock(&dom_state_changed_lock); >> + >> + if ( dom_state_changed ) >> + bitmap_zero(dom_state_changed, DOMID_FIRST_RESERVED); >> + else >> + { >> + dom_state_changed = xzalloc_array(unsigned long, >> + BITS_TO_LONGS(DOMID_FIRST_RESERVED)); > > New code wants to use xvmalloc() et al. Okay. > >> --- a/xen/common/event_channel.c >> +++ b/xen/common/event_channel.c >> @@ -485,20 +485,27 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port) >> if ( (v = domain_vcpu(d, vcpu)) == NULL ) >> return -ENOENT; >> >> + if ( virq == VIRQ_DOM_EXC ) >> + { >> + rc = domain_init_states(); >> + if ( rc ) >> + goto out; >> + } >> + >> write_lock(&d->event_lock); >> >> if ( read_atomic(&v->virq_to_evtchn[virq]) ) >> { >> rc = -EEXIST; >> gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc); >> - goto out; >> + goto unlock; >> } >> >> port = rc = evtchn_get_port(d, port); >> if ( rc < 0 ) >> { >> gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc); >> - goto out; >> + goto unlock; >> } >> >> rc = 0; >> @@ -524,9 +531,13 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port) >> */ >> write_atomic(&v->virq_to_evtchn[virq], port); >> >> - out: >> + unlock: >> write_unlock(&d->event_lock); >> >> + out: >> + if ( rc ) >> + domain_deinit_states(); >> + >> return rc; >> } > > Renaming the prior label (and hence needing to fiddle with existing goto-s) > feels a little fragile. How about keeping "out" as is and introducing "deinit" > or some such? Fine with me. Juergen
diff --git a/xen/common/domain.c b/xen/common/domain.c index e33a0a5a21..4e3a593a9d 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -138,6 +138,60 @@ bool __read_mostly vmtrace_available; bool __read_mostly vpmu_is_available; +static DEFINE_SPINLOCK(dom_state_changed_lock); +static unsigned long *dom_state_changed; + +int domain_init_states(void) +{ + const struct domain *d; + int rc = -ENOMEM; + + spin_lock(&dom_state_changed_lock); + + if ( dom_state_changed ) + bitmap_zero(dom_state_changed, DOMID_FIRST_RESERVED); + else + { + dom_state_changed = xzalloc_array(unsigned long, + BITS_TO_LONGS(DOMID_FIRST_RESERVED)); + if ( !dom_state_changed ) + goto unlock; + } + + rcu_read_lock(&domlist_read_lock); + + for_each_domain ( d ) + set_bit(d->domain_id, dom_state_changed); + + rcu_read_unlock(&domlist_read_lock); + + rc = 0; + + unlock: + spin_unlock(&dom_state_changed_lock); + + return rc; +} + +void domain_deinit_states(void) +{ + spin_lock(&dom_state_changed_lock); + + XFREE(dom_state_changed); + + spin_unlock(&dom_state_changed_lock); +} + +static void domain_changed_state(const struct domain *d) +{ + spin_lock(&dom_state_changed_lock); + + if ( dom_state_changed ) + set_bit(d->domain_id, dom_state_changed); + + spin_unlock(&dom_state_changed_lock); +} + static void __domain_finalise_shutdown(struct domain *d) { struct vcpu *v; @@ -152,6 +206,7 @@ static void __domain_finalise_shutdown(struct domain *d) return; d->is_shut_down = 1; + domain_changed_state(d); if ( (d->shutdown_code == SHUTDOWN_suspend) && d->suspend_evtchn ) evtchn_send(d, d->suspend_evtchn); else @@ -839,6 +894,7 @@ struct domain *domain_create(domid_t domid, */ domlist_insert(d); + domain_changed_state(d); memcpy(d->handle, config->handle, sizeof(d->handle)); return d; @@ -1104,6 +1160,7 @@ int domain_kill(struct domain *d) /* Mem event cleanup has to go here because the rings * have to be put before we call put_domain. */ vm_event_cleanup(d); + domain_changed_state(d); put_domain(d); send_global_virq(VIRQ_DOM_EXC); /* fallthrough */ @@ -1293,6 +1350,8 @@ static void cf_check complete_domain_destroy(struct rcu_head *head) xfree(d->vcpu); + domain_changed_state(d); + _domain_destroy(d); send_global_virq(VIRQ_DOM_EXC); diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c index 8db2ca4ba2..cd7bad68c0 100644 --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -485,20 +485,27 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port) if ( (v = domain_vcpu(d, vcpu)) == NULL ) return -ENOENT; + if ( virq == VIRQ_DOM_EXC ) + { + rc = domain_init_states(); + if ( rc ) + goto out; + } + write_lock(&d->event_lock); if ( read_atomic(&v->virq_to_evtchn[virq]) ) { rc = -EEXIST; gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc); - goto out; + goto unlock; } port = rc = evtchn_get_port(d, port); if ( rc < 0 ) { gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc); - goto out; + goto unlock; } rc = 0; @@ -524,9 +531,13 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port) */ write_atomic(&v->virq_to_evtchn[virq], port); - out: + unlock: write_unlock(&d->event_lock); + out: + if ( rc ) + domain_deinit_states(); + return rc; } @@ -730,6 +741,9 @@ int evtchn_close(struct domain *d1, int port1, bool guest) struct vcpu *v; unsigned long flags; + if ( chn1->u.virq == VIRQ_DOM_EXC ) + domain_deinit_states(); + v = d1->vcpu[virq_is_global(chn1->u.virq) ? 0 : chn1->notify_vcpu_id]; write_lock_irqsave(&v->virq_lock, flags); diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 711668e028..16684bbaf9 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -800,6 +800,9 @@ void domain_resume(struct domain *d); int domain_soft_reset(struct domain *d, bool resuming); +int domain_init_states(void); +void domain_deinit_states(void); + int vcpu_start_shutdown_deferral(struct vcpu *v); void vcpu_end_shutdown_deferral(struct vcpu *v);
Add a bitmap with one bit per possible domid indicating the respective domain has changed its state (created, deleted, dying, crashed, shutdown). Registering the VIRQ_DOM_EXC event will result in setting the bits for all existing domains and resetting all other bits. Resetting a bit will be done in a future patch. This information is needed for Xenstore to keep track of all domains. Signed-off-by: Juergen Gross <jgross@suse.com> --- V2: - use DOMID_FIRST_RESERVED instead of DOMID_MASK + 1 (Jan Beulich) - use const (Jan Beulich) - move call of domain_reset_states() into evtchn_bind_virq() (Jan Beulich) - dynamically allocate dom_state_changed bitmap (Jan Beulich) --- xen/common/domain.c | 59 ++++++++++++++++++++++++++++++++++++++ xen/common/event_channel.c | 20 +++++++++++-- xen/include/xen/sched.h | 3 ++ 3 files changed, 79 insertions(+), 3 deletions(-)