| Message ID | 20250107101711.5980-3-jgross@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | remove libxenctrl usage from xenstored | expand |
On 07.01.2025 11:17, Juergen Gross wrote: > --- a/xen/common/event_channel.c > +++ b/xen/common/event_channel.c > @@ -120,6 +120,13 @@ static uint8_t get_xen_consumer(xen_event_channel_notification_t fn) > /* Get the notification function for a given Xen-bound event channel. */ > #define xen_notification_fn(e) (xen_consumers[(e)->xen_consumer-1]) > > +static struct domain *global_virq_handlers[NR_VIRQS] __read_mostly; Nit: While you move this line around, it would be nice if the attribute could then also move to its canonical place (between type and identifier). > +static struct domain *get_global_virq_handler(unsigned int virq) > +{ > + return global_virq_handlers[virq] ?: hardware_domain; > +} > + > static bool virq_is_global(unsigned int virq) > { > switch ( virq ) > @@ -479,8 +486,13 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port) > */ > virq = array_index_nospec(virq, ARRAY_SIZE(v->virq_to_evtchn)); > > - if ( virq_is_global(virq) && (vcpu != 0) ) > - return -EINVAL; > + if ( virq_is_global(virq) ) > + { > + if ( get_global_virq_handler(virq) != d ) > + return -EBUSY; Hmm. While this eliminates the problem for the common, race free case, the handler changing right after the check would still mean the bind would succeed. Plus this way you're breaking a case that afaict has been working so far: The bind happening before the setting of the handler. With a lot of unrelated if-s and when-s this could e.g. be of interest when considering a re-startable Xenstore domain. The one to take over could start first, obtain state from the original one while that's still active, and be nominated the handler of the global vIRQ only in the last moment. Jan
On 07.01.25 16:34, Jan Beulich wrote: > On 07.01.2025 11:17, Juergen Gross wrote: >> --- a/xen/common/event_channel.c >> +++ b/xen/common/event_channel.c >> @@ -120,6 +120,13 @@ static uint8_t get_xen_consumer(xen_event_channel_notification_t fn) >> /* Get the notification function for a given Xen-bound event channel. */ >> #define xen_notification_fn(e) (xen_consumers[(e)->xen_consumer-1]) >> >> +static struct domain *global_virq_handlers[NR_VIRQS] __read_mostly; > > Nit: While you move this line around, it would be nice if the attribute > could then also move to its canonical place (between type and identifier). > >> +static struct domain *get_global_virq_handler(unsigned int virq) >> +{ >> + return global_virq_handlers[virq] ?: hardware_domain; >> +} >> + >> static bool virq_is_global(unsigned int virq) >> { >> switch ( virq ) >> @@ -479,8 +486,13 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port) >> */ >> virq = array_index_nospec(virq, ARRAY_SIZE(v->virq_to_evtchn)); >> >> - if ( virq_is_global(virq) && (vcpu != 0) ) >> - return -EINVAL; >> + if ( virq_is_global(virq) ) >> + { >> + if ( get_global_virq_handler(virq) != d ) >> + return -EBUSY; > > Hmm. While this eliminates the problem for the common, race free case, > the handler changing right after the check would still mean the bind > would succeed. Are you fine with me adding a paragraph to the commit message saying that a future patch will handle this case? This future patch is patch 4 of the series, which will need to be modified to check the handling domain inside the event_lock. > Plus this way you're breaking a case that afaict has been working so > far: The bind happening before the setting of the handler. With a lot > of unrelated if-s and when-s this could e.g. be of interest when > considering a re-startable Xenstore domain. The one to take over could > start first, obtain state from the original one while that's still > active, and be nominated the handler of the global vIRQ only in the > last moment. This is a racy situation, too. If the old domain receives the virq after sending the state, this would need to be handled by transferring the virq information to the new domain, which can result in a never ending story. This is the reason why the domain state bitmap is reset to contain all existing domains to be flagged as "changed", as otherwise a change might get lost. I'd rather be able to handle today's use cases in a sane way than to try handling any weird future use cases which we don't know yet. I think today's behavior is more or less insane and the new behavior is much easier to understand and more intuitive. Juergen
On 07.01.2025 17:07, Jürgen Groß wrote: > On 07.01.25 16:34, Jan Beulich wrote: >> On 07.01.2025 11:17, Juergen Gross wrote: >>> @@ -479,8 +486,13 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port) >>> */ >>> virq = array_index_nospec(virq, ARRAY_SIZE(v->virq_to_evtchn)); >>> >>> - if ( virq_is_global(virq) && (vcpu != 0) ) >>> - return -EINVAL; >>> + if ( virq_is_global(virq) ) >>> + { >>> + if ( get_global_virq_handler(virq) != d ) >>> + return -EBUSY; >> >> Hmm. While this eliminates the problem for the common, race free case, >> the handler changing right after the check would still mean the bind >> would succeed. > > Are you fine with me adding a paragraph to the commit message saying > that a future patch will handle this case? > > This future patch is patch 4 of the series, which will need to be > modified to check the handling domain inside the event_lock. I think this would be okay, so long as patches 2...4 are then also all committed together. >> Plus this way you're breaking a case that afaict has been working so >> far: The bind happening before the setting of the handler. With a lot >> of unrelated if-s and when-s this could e.g. be of interest when >> considering a re-startable Xenstore domain. The one to take over could >> start first, obtain state from the original one while that's still >> active, and be nominated the handler of the global vIRQ only in the >> last moment. > > This is a racy situation, too. If the old domain receives the virq after > sending the state, this would need to be handled by transferring the virq > information to the new domain, which can result in a never ending story. > > This is the reason why the domain state bitmap is reset to contain all > existing domains to be flagged as "changed", as otherwise a change might > get lost. > > I'd rather be able to handle today's use cases in a sane way than to try > handling any weird future use cases which we don't know yet. > > I think today's behavior is more or less insane and the new behavior is > much easier to understand and more intuitive. Hmm, I'd like to leave this then for input by other maintainers. Jan
On 07.01.25 17:38, Jan Beulich wrote: > On 07.01.2025 17:07, Jürgen Groß wrote: >> On 07.01.25 16:34, Jan Beulich wrote: >>> On 07.01.2025 11:17, Juergen Gross wrote: >>>> @@ -479,8 +486,13 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port) >>>> */ >>>> virq = array_index_nospec(virq, ARRAY_SIZE(v->virq_to_evtchn)); >>>> >>>> - if ( virq_is_global(virq) && (vcpu != 0) ) >>>> - return -EINVAL; >>>> + if ( virq_is_global(virq) ) >>>> + { >>>> + if ( get_global_virq_handler(virq) != d ) >>>> + return -EBUSY; >>> >>> Hmm. While this eliminates the problem for the common, race free case, >>> the handler changing right after the check would still mean the bind >>> would succeed. >> >> Are you fine with me adding a paragraph to the commit message saying >> that a future patch will handle this case? >> >> This future patch is patch 4 of the series, which will need to be >> modified to check the handling domain inside the event_lock. > > I think this would be okay, so long as patches 2...4 are then also all > committed together. > >>> Plus this way you're breaking a case that afaict has been working so >>> far: The bind happening before the setting of the handler. With a lot >>> of unrelated if-s and when-s this could e.g. be of interest when >>> considering a re-startable Xenstore domain. The one to take over could >>> start first, obtain state from the original one while that's still >>> active, and be nominated the handler of the global vIRQ only in the >>> last moment. >> >> This is a racy situation, too. If the old domain receives the virq after >> sending the state, this would need to be handled by transferring the virq >> information to the new domain, which can result in a never ending story. >> >> This is the reason why the domain state bitmap is reset to contain all >> existing domains to be flagged as "changed", as otherwise a change might >> get lost. >> >> I'd rather be able to handle today's use cases in a sane way than to try >> handling any weird future use cases which we don't know yet. >> >> I think today's behavior is more or less insane and the new behavior is >> much easier to understand and more intuitive. > > Hmm, I'd like to leave this then for input by other maintainers. Just one additional remark to your re-startable xenstore domain scenario above: It wouldn't be possible today to do the same with a xenstore daemon in e.g. dom0, as binding the virq another time from within the same domain would be rejected by the hypervisor. In the xenstore domain case you'd either need the old domain to ask dom0 to change the handler (so much about less communication needed), or you'd need to give the xenstore domain the right to do the handler change itself, requiring to use flask or to modify the dummy XSM rights of the xenstore domain. Juergen
On 08.01.2025 10:02, Jürgen Groß wrote: > On 07.01.25 17:38, Jan Beulich wrote: >> On 07.01.2025 17:07, Jürgen Groß wrote: >>> On 07.01.25 16:34, Jan Beulich wrote: >>>> On 07.01.2025 11:17, Juergen Gross wrote: >>>>> @@ -479,8 +486,13 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port) >>>>> */ >>>>> virq = array_index_nospec(virq, ARRAY_SIZE(v->virq_to_evtchn)); >>>>> >>>>> - if ( virq_is_global(virq) && (vcpu != 0) ) >>>>> - return -EINVAL; >>>>> + if ( virq_is_global(virq) ) >>>>> + { >>>>> + if ( get_global_virq_handler(virq) != d ) >>>>> + return -EBUSY; >>>> >>>> Hmm. While this eliminates the problem for the common, race free case, >>>> the handler changing right after the check would still mean the bind >>>> would succeed. >>> >>> Are you fine with me adding a paragraph to the commit message saying >>> that a future patch will handle this case? >>> >>> This future patch is patch 4 of the series, which will need to be >>> modified to check the handling domain inside the event_lock. >> >> I think this would be okay, so long as patches 2...4 are then also all >> committed together. >> >>>> Plus this way you're breaking a case that afaict has been working so >>>> far: The bind happening before the setting of the handler. With a lot >>>> of unrelated if-s and when-s this could e.g. be of interest when >>>> considering a re-startable Xenstore domain. The one to take over could >>>> start first, obtain state from the original one while that's still >>>> active, and be nominated the handler of the global vIRQ only in the >>>> last moment. >>> >>> This is a racy situation, too. If the old domain receives the virq after >>> sending the state, this would need to be handled by transferring the virq >>> information to the new domain, which can result in a never ending story. >>> >>> This is the reason why the domain state bitmap is reset to contain all >>> existing domains to be flagged as "changed", as otherwise a change might >>> get lost. >>> >>> I'd rather be able to handle today's use cases in a sane way than to try >>> handling any weird future use cases which we don't know yet. >>> >>> I think today's behavior is more or less insane and the new behavior is >>> much easier to understand and more intuitive. >> >> Hmm, I'd like to leave this then for input by other maintainers. > > Just one additional remark to your re-startable xenstore domain scenario > above: > > It wouldn't be possible today to do the same with a xenstore daemon in > e.g. dom0, as binding the virq another time from within the same domain > would be rejected by the hypervisor. In the xenstore domain case you'd > either need the old domain to ask dom0 to change the handler (so much > about less communication needed), Not quite. There needs to be an indication anyway of info transfer being complete. That'll be where Dom0 would then (also) put in place the new handler. The vIRQ first arriving in the new XS domain could then serve as an indication that it is now in charge of the system; I didn't check whether a courtesy one would be sent right away, or whether such sending might need adding. (Plus anyway - XS is only an example here.) Jan > or you'd need to give the xenstore domain > the right to do the handler change itself, requiring to use flask or to > modify the dummy XSM rights of the xenstore domain. > > > Juergen
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c index f2b64c48fb..62060dc66b 100644 --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -120,6 +120,13 @@ static uint8_t get_xen_consumer(xen_event_channel_notification_t fn) /* Get the notification function for a given Xen-bound event channel. */ #define xen_notification_fn(e) (xen_consumers[(e)->xen_consumer-1]) +static struct domain *global_virq_handlers[NR_VIRQS] __read_mostly; + +static struct domain *get_global_virq_handler(unsigned int virq) +{ + return global_virq_handlers[virq] ?: hardware_domain; +} + static bool virq_is_global(unsigned int virq) { switch ( virq ) @@ -479,8 +486,13 @@ int evtchn_bind_virq(evtchn_bind_virq_t *bind, evtchn_port_t port) */ virq = array_index_nospec(virq, ARRAY_SIZE(v->virq_to_evtchn)); - if ( virq_is_global(virq) && (vcpu != 0) ) - return -EINVAL; + if ( virq_is_global(virq) ) + { + if ( get_global_virq_handler(virq) != d ) + return -EBUSY; + if ( vcpu != 0 ) + return -EINVAL; + } if ( (v = domain_vcpu(d, vcpu)) == NULL ) return -ENOENT; @@ -965,15 +977,13 @@ void send_guest_pirq(struct domain *d, const struct pirq *pirq) } } -static struct domain *global_virq_handlers[NR_VIRQS] __read_mostly; - static DEFINE_SPINLOCK(global_virq_handlers_lock); void send_global_virq(uint32_t virq) { ASSERT(virq_is_global(virq)); - send_guest_global_virq(global_virq_handlers[virq] ?: hardware_domain, virq); + send_guest_global_virq(get_global_virq_handler(virq), virq); } int set_global_virq_handler(struct domain *d, uint32_t virq)
Today Xen will happily allow binding a global virq by a domain which isn't configured to receive it. This won't result in any bad actions, but the bind will appear to have succeeded with no event ever being received by that event channel. Instead of allowing the bind, error out if the domain isn't set to handle that virq. Signed-off-by: Juergen Gross <jgross@suse.com> --- V6: - new patch --- xen/common/event_channel.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)