Re: [PATCH v3 1/1] Introduce VCPUOP_reset_vcpu_info

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 19/08/14 11:04, Vitaly Kuznetsov wrote:
> When an SMP guest performs kexec/kdump it tries issuing VCPUOP_register_vcpu_info.
> This fails due to vcpu_info already being registered. Introduce new vcpu operation
> to reset vcpu_info to its default state.
>
> Based on the original patch by Konrad Rzeszutek Wilk.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> Changes from RFCv2:
> - modify unmap_vcpu_info() to match both needs [Jan Beulich]
> - support VCPUOP_reset_vcpu_info for current VCPU [Jan Beulich]
> - improve description in public/vcpu.h [Jan Beulich]
>
> Changes from RFCv1:
> - Don't use unsuitable unmap_vcpu_info(), rewrite [Jan Beulich]
> - Require FIFO ABI being in 2-level mode
> - Describe limitations in include/public/vcpu.h [Jan Beulich]
> ---
>  xen/arch/x86/hvm/hvm.c    |  1 +
>  xen/common/domain.c       | 73 +++++++++++++++++++++++++++++++++++++++++------
>  xen/include/public/vcpu.h | 19 ++++++++++++
>  3 files changed, 85 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 216c3f2..7917272 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3332,6 +3332,7 @@ static long hvm_vcpu_op(
>      case VCPUOP_set_singleshot_timer:
>      case VCPUOP_stop_singleshot_timer:
>      case VCPUOP_register_vcpu_info:
> +    case VCPUOP_reset_vcpu_info:
>      case VCPUOP_register_vcpu_time_memory_area:
>          rc = do_vcpu_op(cmd, vcpuid, arg);
>          break;
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 44e5cbe..9e10e1e 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -956,25 +956,52 @@ 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.
> + * Unmap the vcpu info page if the guest decided to place it somewhere else.
> + * There's no need to do anything clever when the domain is dying (when called
> + * from arch_domain_destroy) and we need to prepare for rebinding when it is not
> + * (in case VCPUOP_reset_vcpu_info was called).
>   */
>  void unmap_vcpu_info(struct vcpu *v)
>  {
> +    struct domain *d = v->domain;
>      unsigned long mfn;
> +    vcpu_info_t *old_info;
> +    unsigned int i;
> +
> +    mfn = v->vcpu_info_mfn;
> +    old_info = v->vcpu_info;
>  
> -    if ( v->vcpu_info_mfn == INVALID_MFN )
> +    if ( mfn == INVALID_MFN )
>          return;
>  
> -    mfn = v->vcpu_info_mfn;
> +    if ( (!d->is_dying) && (v->vcpu_id < XEN_LEGACY_MAX_VCPUS) )
> +    {
> +        memcpy(&shared_info(d, vcpu_info[v->vcpu_id]), v->vcpu_info,
> +               sizeof(vcpu_info_t));
> +        v->vcpu_info = (vcpu_info_t *)&shared_info(d, vcpu_info[v->vcpu_id]);
> +    }
> +    else
> +        v->vcpu_info = &dummy_vcpu_info;
>      unmap_domain_page_global((void *)
> -                             ((unsigned long)v->vcpu_info & PAGE_MASK));
> +                             ((unsigned long)old_info & PAGE_MASK));
>  
> -    v->vcpu_info = &dummy_vcpu_info;
> +    put_page_and_type(mfn_to_page(mfn));
>      v->vcpu_info_mfn = INVALID_MFN;
>  
> -    put_page_and_type(mfn_to_page(mfn));
> +    if ( (!d->is_dying) && (v->vcpu_id < XEN_LEGACY_MAX_VCPUS) &&
> +         (test_bit(_VPF_down, &v->pause_flags)) )
> +    {
> +        /* Make sure vcpu_info was set */
> +        smp_wmb();
> +
> +        /*
> +         * Mark everything as being pending for all offline VCPUs to make sure
> +         * nothing gets lost when this VCPU goes online again.
> +         */
> +        vcpu_info(v, evtchn_upcall_pending) = 1;
> +        for ( i = 0; i < BITS_PER_EVTCHN_WORD(d); i++ )
> +            set_bit(i, &vcpu_info(v, evtchn_pending_sel));
> +    }
>  }
>  
>  long do_vcpu_op(int cmd, int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
> @@ -1116,6 +1143,36 @@ long do_vcpu_op(int cmd, int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>      }
>  
> +    case VCPUOP_reset_vcpu_info:
> +    {
> +
> +        if ( (v != current) && !test_bit(_VPF_down, &v->pause_flags) )
> +        {
> +            printk(XENLOG_G_WARNING "%pv: VCPU is up or not current,"
> +                   "refusing to reset vcpu_info!\n", v);
> +            return -EBUSY;
> +        }
> +
> +        if ( v->evtchn_fifo )
> +        {
> +            printk(XENLOG_G_WARNING "%pv: FIFO evtchn ABI is being used,"
> +                   "refusing to reset vcpu_info!\n", v);
> +            return -EBUSY;
> +        }

These two checks should be in unmap_vcpu_info() similar to
map_vcpu_info(), and unmap_vcpu_info() should return an int.  I am not
sure whether the printks are really needed; a buggy/malicious guest is
very capable of causing logspam with them.

> +
> +        if ( v->vcpu_info_mfn == INVALID_MFN )
> +        {
> +            rc = 0;
> +            break;
> +        }

This check is not safe without the domain lock held.  Drop it entirely,
as the head of unmap_vcpu_info() already does it.

~Andrew

> +
> +        domain_lock(d);
> +        unmap_vcpu_info(v);
> +        domain_unlock(d);
> +        rc = 0;
> +        break;
> +    }
> +
>      case VCPUOP_register_runstate_memory_area:
>      {
>          struct vcpu_register_runstate_memory_area area;
> diff --git a/xen/include/public/vcpu.h b/xen/include/public/vcpu.h
> index e888daf..c4d166c 100644
> --- a/xen/include/public/vcpu.h
> +++ b/xen/include/public/vcpu.h
> @@ -227,6 +227,25 @@ struct vcpu_register_time_memory_area {
>  typedef struct vcpu_register_time_memory_area vcpu_register_time_memory_area_t;
>  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
>  
> +/*
> + * Reset all of the vcpu_info information from their previous location
> + * to the default one used at bootup. The following prerequisites should be met:
> + *  1. Domain should be using 2-level event channel ABI. In case another
> + *     non-default ABI is in use the domain is supposed to switch back to
> + *     2-level ABI with EVTCHNOP_reset.
> + *  2. The operation is unsupported for non-current online VCPUs. If performed
> + *     during shutdown/kexec/... a guest domain is supposed to behave in the
> + *     following sequence:
> + *     - Choose special 'shutdown' VCPU, bring all other VCPUs down;
> + *     - Issue VCPUOP_reset_vcpu_info for all offline VCPUs;
> + *     - Issue VCPUOP_reset_vcpu_info for the 'shutdown' VCPU.
> + *
> + * After the call vcpu_info is reset to its default state: first
> + * XEN_LEGACY_MAX_VCPUS VCPUs will get it switched to shared_info and all other
> + * VCPUs will get dummy_vcpu_info.
> + */
> +#define VCPUOP_reset_vcpu_info   14
> +
>  #endif /* __XEN_PUBLIC_VCPU_H__ */
>  
>  /*