Re: [PATCH v8 06/11] xen: Introduce XEN_DOMCTL_soft_reset

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Jun 23, 2015 at 06:11:48PM +0200, Vitaly Kuznetsov wrote:
> New domctl resets state for a domain allowing it to 'start over': register
> vcpu_info, switch to FIFO ABI for event channels. Still active grants are
> being logged to help debugging misbehaving backends.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  xen/common/domain.c         | 29 ++++++++++++++++++++++++++---
>  xen/common/domctl.c         |  9 +++++++++
>  xen/include/public/domctl.h |  1 +
>  xen/include/xen/sched.h     |  2 ++
>  4 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 3bc52e6..ade80ff 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1010,6 +1010,26 @@ int domain_unpause_by_systemcontroller(struct domain *d)
>      return 0;
>  }
>  
> +int domain_soft_reset(struct domain *d)
> +{
> +    struct vcpu *v;
> +
> +    for_each_vcpu ( d, v )
> +        if ( !v->paused_for_shutdown )
> +            return -EINVAL;

I think you need to hold a lock to make sure that nobody else
during this loop messes with 'v->paused_for_shutdown' value.

That is the shutdown_lock.

> +
> +    evtchn_reset(d);
> +
> +    grant_table_warn_active_grants(d);
> +
> +    for_each_vcpu ( d, v )
> +        unmap_vcpu_info(v);
> +
> +    domain_resume(d);
> +
> +    return 0;
> +}
> +
>  int vcpu_reset(struct vcpu *v)
>  {
>      struct domain *d = v->domain;
> @@ -1121,12 +1141,12 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset)
>  
>  /*
>   * Unmap the vcpu info page if the guest decided to place it somewhere
> - * else.  This is only used from arch_domain_destroy, so there's no
> - * need to do anything clever.
> + * else. This is used from arch_domain_destroy and domain_soft_reset.
>   */
>  void unmap_vcpu_info(struct vcpu *v)
>  {
>      unsigned long mfn;
> +    struct domain *d = v->domain;
>  
>      if ( v->vcpu_info_mfn == INVALID_MFN )
>          return;
> @@ -1135,7 +1155,10 @@ void unmap_vcpu_info(struct vcpu *v)
>      unmap_domain_page_global((void *)
>                               ((unsigned long)v->vcpu_info & PAGE_MASK));
>  
> -    v->vcpu_info = &dummy_vcpu_info;
> +    v->vcpu_info = ((v->vcpu_id < XEN_LEGACY_MAX_VCPUS)
> +                    ? (vcpu_info_t *)&shared_info(d, vcpu_info[v->vcpu_id])
> +                    : &dummy_vcpu_info);
> +

Could you explain that bit please? Why not use dummy_vcpu_info?

Ah, you are doing what alloc_vcpu does.
>      v->vcpu_info_mfn = INVALID_MFN;

Perhaps we should jus thave a function called 'vcpu_info_reset' that
would do this - and called in both places?

>  
>      put_page_and_type(mfn_to_page(mfn));
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index ce517a7..8cb6a2a 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -703,6 +703,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>          break;
>      }
>  
> +    case XEN_DOMCTL_soft_reset:
> +        if ( d == current->domain )
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
> +        ret = domain_soft_reset(d);
> +        break;
> +
>      case XEN_DOMCTL_destroydomain:
>          ret = domain_kill(d);
>          if ( ret == -ERESTART )
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index bc45ea5..5b4ba6b 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1114,6 +1114,7 @@ struct xen_domctl {
>  #define XEN_DOMCTL_setvnumainfo                  74
>  #define XEN_DOMCTL_psr_cmt_op                    75
>  #define XEN_DOMCTL_monitor_op                    77
> +#define XEN_DOMCTL_soft_reset                    78
>  #define XEN_DOMCTL_gdbsx_guestmemio            1000
>  #define XEN_DOMCTL_gdbsx_pausevcpu             1001
>  #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index d810e1c..1120741 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -609,6 +609,8 @@ void domain_shutdown(struct domain *d, u8 reason);
>  void domain_resume(struct domain *d);
>  void domain_pause_for_debugger(void);
>  
> +int domain_soft_reset(struct domain *d);
> +
>  int vcpu_start_shutdown_deferral(struct vcpu *v);
>  void vcpu_end_shutdown_deferral(struct vcpu *v);
>  
> -- 
> 2.4.2
>