Re: [PATCH RFC 2/4] xen: add bitmap to indicate per-domain state changes

[Julien Grall <julien@xxxxxxx>]

Hi Juergen,

On 14/09/2021 17:35, 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@xxxxxxxx>
> ---
>   xen/common/domain.c        | 22 ++++++++++++++++++++++
>   xen/common/event_channel.c |  2 ++
>   xen/include/xen/sched.h    |  2 ++
>   3 files changed, 26 insertions(+)
>
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 755349b93f..14a427e2ef 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -87,6 +87,22 @@ bool __read_mostly vmtrace_available;
>   /* Unique domain identifier, protected by domctl lock. */
>   static uint64_t unique_id;
>   
> +static DECLARE_BITMAP(dom_state_changed, DOMID_MASK + 1);
> +
> +void domain_reset_states(void)
> +{
> +    struct domain *d;
> +
> +    bitmap_zero(dom_state_changed, DOMID_MASK + 1);
> +
> +    rcu_read_lock(&domlist_read_lock);
> +
> +    for_each_domain ( d )
> +        set_bit(d->domain_id, dom_state_changed);
> +
> +    rcu_read_unlock(&domlist_read_lock);
> +}
> +
>   static void __domain_finalise_shutdown(struct domain *d)
>   {
>       struct vcpu *v;
> @@ -101,6 +117,7 @@ static void __domain_finalise_shutdown(struct domain *d)
>               return;
>   
>       d->is_shut_down = 1;
> +    set_bit(d->domain_id, dom_state_changed);
Looking at the follow-up patch, I think you want a smp_wmb() before 
set_bit() otherwise this could be re-ordered on Arm (set_bit() doesn't 
guarantee any ordering). This is to avoid any issue if the new hypercall 
run concurrently.
>       if ( (d->shutdown_code == SHUTDOWN_suspend) && d->suspend_evtchn )
>           evtchn_send(d, d->suspend_evtchn);
>       else
> @@ -720,6 +737,8 @@ struct domain *domain_create(domid_t domid,
>           rcu_assign_pointer(domain_hash[DOMAIN_HASH(domid)], d);
>           spin_unlock(&domlist_update_lock);
>   
> +        set_bit(d->domain_id, dom_state_changed);
Here you should not need one because spin_unlock() contains one.

> +
>           memcpy(d->handle, config->handle, sizeof(d->handle));
>       }
>   
> @@ -954,6 +973,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);
> +        set_bit(d->domain_id, dom_state_changed);
You might want an smp_wmb() here as well.

>           put_domain(d);
>           send_global_virq(VIRQ_DOM_EXC);
>           /* fallthrough */
> @@ -1141,6 +1161,8 @@ static void complete_domain_destroy(struct rcu_head *head)
>   
>       xfree(d->vcpu);
>   
> +    set_bit(d->domain_id, dom_state_changed);
> +
I think this wants to be moved *after* _domain_destroy() with an 
smp_wmb() between the two. So you when the bit is set you know the 
domain is dead.
>       _domain_destroy(d);
>   
>       send_global_virq(VIRQ_DOM_EXC);
> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> index da88ad141a..e2a416052b 100644
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -1218,6 +1218,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();
>           break;
>       }
>   
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index b827c5af8d..2ae26bc50e 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -730,6 +730,8 @@ void domain_resume(struct domain *d);
>   
>   int domain_soft_reset(struct domain *d, bool resuming);
>   
> +void domain_reset_states(void);
> +
>   int vcpu_start_shutdown_deferral(struct vcpu *v);
>   void vcpu_end_shutdown_deferral(struct vcpu *v);
>   
Cheers,

--
Julien Grall