| Message ID | 20241217111247.2204-3-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remove libxenctrl usage from xenstored | expand |
On 17.12.2024 12:12, Juergen Gross wrote: > V4: > - add __read_mostly (Jan Beulich) > - use __set_biz() (Jan Beulich) > - fix error handling in evtchn_bind_virq() (Jan Beulich) I'm sorry, I should have spotted a 2nd issue already when reviewing v3 (or even an earlier version). > @@ -138,6 +139,60 @@ bool __read_mostly vmtrace_available; > > bool __read_mostly vpmu_is_available; > > +static DEFINE_SPINLOCK(dom_state_changed_lock); > +static unsigned long *__read_mostly 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); This needs to not happen when ... > @@ -485,11 +486,21 @@ 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 ) > + return rc; > + > + deinit_if_err = true; > + } > + > write_lock(&d->event_lock); > > if ( read_atomic(&v->virq_to_evtchn[virq]) ) > { > rc = -EEXIST; > + deinit_if_err = false; > gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc); > goto out; > } ... we take this error path. Which I think calls for moving the domain_init_states() invocation ... > @@ -527,6 +538,9 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port) > out: > write_unlock(&d->event_lock); > > + if ( rc && deinit_if_err ) > + domain_deinit_states(); > + > return rc; > } ... somewhere here. It really doesn't need doing early, as the caller may assume the bitmap was set up only when this hypercall returns successfully. Jan
On 17.12.24 12:30, Jan Beulich wrote: > On 17.12.2024 12:12, Juergen Gross wrote: >> V4: >> - add __read_mostly (Jan Beulich) >> - use __set_biz() (Jan Beulich) >> - fix error handling in evtchn_bind_virq() (Jan Beulich) > > I'm sorry, I should have spotted a 2nd issue already when reviewing v3 (or > even an earlier version). > >> @@ -138,6 +139,60 @@ bool __read_mostly vmtrace_available; >> >> bool __read_mostly vpmu_is_available; >> >> +static DEFINE_SPINLOCK(dom_state_changed_lock); >> +static unsigned long *__read_mostly 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); > > This needs to not happen when ... > >> @@ -485,11 +486,21 @@ 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 ) >> + return rc; >> + >> + deinit_if_err = true; >> + } >> + >> write_lock(&d->event_lock); >> >> if ( read_atomic(&v->virq_to_evtchn[virq]) ) >> { >> rc = -EEXIST; >> + deinit_if_err = false; >> gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc); >> goto out; >> } > > ... we take this error path. Which I think calls for moving the > domain_init_states() invocation ... > >> @@ -527,6 +538,9 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port) >> out: >> write_unlock(&d->event_lock); >> >> + if ( rc && deinit_if_err ) >> + domain_deinit_states(); >> + >> return rc; >> } > > ... somewhere here. It really doesn't need doing early, as the caller > may assume the bitmap was set up only when this hypercall returns > successfully. OTOH this will require undoing the binding of the virq in case of an error returned by domain_init_states(). It would probably be best to place the call of domain_init_states() after the -EEXIST case. Juergen
On 17.12.2024 12:59, Jürgen Groß wrote: > On 17.12.24 12:30, Jan Beulich wrote: >> On 17.12.2024 12:12, Juergen Gross wrote: >>> V4: >>> - add __read_mostly (Jan Beulich) >>> - use __set_biz() (Jan Beulich) >>> - fix error handling in evtchn_bind_virq() (Jan Beulich) >> >> I'm sorry, I should have spotted a 2nd issue already when reviewing v3 (or >> even an earlier version). >> >>> @@ -138,6 +139,60 @@ bool __read_mostly vmtrace_available; >>> >>> bool __read_mostly vpmu_is_available; >>> >>> +static DEFINE_SPINLOCK(dom_state_changed_lock); >>> +static unsigned long *__read_mostly 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); >> >> This needs to not happen when ... >> >>> @@ -485,11 +486,21 @@ 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 ) >>> + return rc; >>> + >>> + deinit_if_err = true; >>> + } >>> + >>> write_lock(&d->event_lock); >>> >>> if ( read_atomic(&v->virq_to_evtchn[virq]) ) >>> { >>> rc = -EEXIST; >>> + deinit_if_err = false; >>> gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc); >>> goto out; >>> } >> >> ... we take this error path. Which I think calls for moving the >> domain_init_states() invocation ... >> >>> @@ -527,6 +538,9 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port) >>> out: >>> write_unlock(&d->event_lock); >>> >>> + if ( rc && deinit_if_err ) >>> + domain_deinit_states(); >>> + >>> return rc; >>> } >> >> ... somewhere here. It really doesn't need doing early, as the caller >> may assume the bitmap was set up only when this hypercall returns >> successfully. > > OTOH this will require undoing the binding of the virq in case of an > error returned by domain_init_states(). > > It would probably be best to place the call of domain_init_states() > after the -EEXIST case. Hmm, right. Jan
diff --git a/xen/common/domain.c b/xen/common/domain.c index e33a0a5a21..87633b1f7b 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -34,6 +34,7 @@ #include <xen/xenoprof.h> #include <xen/irq.h> #include <xen/argo.h> +#include <xen/xvmalloc.h> #include <asm/p2m.h> #include <asm/processor.h> #include <public/sched.h> @@ -138,6 +139,60 @@ bool __read_mostly vmtrace_available; bool __read_mostly vpmu_is_available; +static DEFINE_SPINLOCK(dom_state_changed_lock); +static unsigned long *__read_mostly 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 = xvzalloc_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); + + XVFREE(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 +207,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 +895,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 +1161,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 +1351,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..041aacad02 100644 --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -469,6 +469,7 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port) struct domain *d = current->domain; int virq = bind->virq, vcpu = bind->vcpu; int rc = 0; + bool deinit_if_err = false; if ( (virq < 0) || (virq >= ARRAY_SIZE(v->virq_to_evtchn)) ) return -EINVAL; @@ -485,11 +486,21 @@ 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 ) + return rc; + + deinit_if_err = true; + } + write_lock(&d->event_lock); if ( read_atomic(&v->virq_to_evtchn[virq]) ) { rc = -EEXIST; + deinit_if_err = false; gdprintk(XENLOG_WARNING, "EVTCHNOP failure: error %d\n", rc); goto out; } @@ -527,6 +538,9 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port) out: write_unlock(&d->event_lock); + if ( rc && deinit_if_err ) + domain_deinit_states(); + return rc; } @@ -730,6 +744,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. 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. 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) V3: - use xvzalloc_array() (Jan Beulich) - don't rename existing label (Jan Beulich) V4: - add __read_mostly (Jan Beulich) - use __set_biz() (Jan Beulich) - fix error handling in evtchn_bind_virq() (Jan Beulich) --- xen/common/domain.c | 60 ++++++++++++++++++++++++++++++++++++++ xen/common/event_channel.c | 17 +++++++++++ xen/include/xen/sched.h | 3 ++ 3 files changed, 80 insertions(+)