On 31.10.24 11:59, Jan Beulich wrote:
On 23.10.2024 15:10, 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.That's furthering the "there can be only one consumer" model that also is used for VIRQ_DOM_EXC itself. I consider the existing model flawed (nothing keeps a 2nd party with sufficient privilege from invoking XEN_DOMCTL_set_virq_handler a 2nd time, taking away the notification from whoever had first requested it), and hence I dislike this being extended. Conceivably multiple parties may indeed be interested in this kind of information. At which point resetting state when the vIRQ is bound is questionable (or the data would need to become per-domain rather than global, or even yet more fine-grained, albeit ->virq_to_evtchn[] is also per-domain, when considering global vIRQ-s).
The bitmap is directly tied to the VIRQ_DOM_EXC anyway, as it is that event which makes the consumer look into the bitmap via the new hypercall. If we decide to allow multiple consumers of VIRQ_DOM_EXC, we'll need to have one bitmap per consumer of the event. This is not very hard to modify. If you'd like that better, I can dynamically allocate the bitmap on binding VIRQ_DOM_EXC and freeing it again when unbinding is done.
--- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -138,6 +138,22 @@ bool __read_mostly vmtrace_available;bool __read_mostly vpmu_is_available; +static DECLARE_BITMAP(dom_state_changed, DOMID_MASK + 1);While it won't alter the size of the array, I think DOMID_FIRST_RESERVED would be more logical to use here and ...+void domain_reset_states(void) +{ + struct domain *d; + + bitmap_zero(dom_state_changed, DOMID_MASK + 1);... here.
Fine with me.
+ rcu_read_lock(&domlist_read_lock); + + for_each_domain ( d ) + set_bit(d->domain_id, dom_state_changed);d is used only here, so could be pointer-to-const?
Agreed.
--- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -1296,6 +1296,8 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) rc = evtchn_bind_virq(&bind_virq, 0); if ( !rc && __copy_to_guest(arg, &bind_virq, 1) ) rc = -EFAULT; /* Cleaning up here would be a mess! */ + if ( !rc && bind_virq.virq == VIRQ_DOM_EXC ) + domain_reset_states();evtchn_bind_virq() isn't static, so callers beyond the present ones could appear without noticing the need for this special casing. Is there a reason the check can't move into the function? Doing the check in spite of the copy-out failing is imo still reasonable behavior.
Moving the test into evtchn_bind_virq() should work. I'll change that. Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
