Re: [PATCH v9 08/11] xen: arch-specific hooks for domain_soft_reset()

["Jan Beulich" <JBeulich@suse.com>]

>>> On 16.07.15 at 18:27, <vkuznets@redhat.com> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -704,6 +704,90 @@ void arch_domain_unpause(struct domain *d)
>          viridian_time_ref_count_thaw(d);
>  }
>  
> +int arch_domain_soft_reset(struct domain *d)
> +{
> +    struct page_info *page = virt_to_page(d->shared_info), *new_page;
> +    int ret = 0;
> +    struct domain *owner;
> +    unsigned long mfn, mfn_new, gfn;
> +    p2m_type_t p2mt;
> +    unsigned int i;
> +
> +    /* Soft reset is supported for HVM domains only. */
> +    if ( !is_hvm_domain(d) )
> +        return -EINVAL;

This suggests that the whole function may better go somewhere in
hvm/.

And is this intentionally excluding PVH?

> +    hvm_destroy_all_ioreq_servers(d);

Or if the function was to stay here, you'd probably better add a
hvm_domain_soft_reset() instead of making this function non-
static.

> +    spin_lock(&d->event_lock);
> +    for ( i = 0; i < d->nr_pirqs ; i++ )
> +    {
> +        if ( domain_pirq_to_emuirq(d, i) != IRQ_UNBOUND )
> +        {
> +            ret = unmap_domain_pirq_emuirq(d, i);
> +            if ( ret )
> +                break;
> +        }
> +    }
> +    spin_unlock(&d->event_lock);
> +
> +    if ( ret )
> +        return ret;
> +
> +    /*
> +     * shared_info page needs to be replaced with a new page, otherwise we

"The shared info page ..."

> +     * will get a hole if the domain does XENMAPSPACE_shared_info.
> +     */
> +
> +    owner = page_get_owner_and_reference(page);
> +    /* If shared_info page wasn't used, we do not need to replace it. */
> +    if ( owner != d )

owner may be NULL, in which case you mustn't put_page(). Or if
you're sure owner can't be NULL here, then please ASSERT() so.
(I'm about to post a patch adjusting a remotely similar case in the
grant table code).

> +        goto exit_put_page;
> +
> +    mfn = page_to_mfn(page);
> +    if ( !mfn_valid(mfn) )

This check makes sense at best ahead of calling
page_get_owner_and_reference(), but I can't see how
virt_to_page() could produce an invalid page unless there was a
bug elsewhere, or d->shared_info was NULL.

> +    {
> +        printk(XENLOG_G_ERR "Dom%d's shared_info page points to invalid MFN\n",
> +               d->domain_id);
> +        ret = -EINVAL;
> +        goto exit_put_page;
> +    }
> +
> +    gfn = mfn_to_gmfn(d, mfn);
> +    if ( mfn_x(get_gfn_query(d, gfn, &p2mt)) == INVALID_MFN )

Perhaps even != mfn?

> +    {
> +        printk(XENLOG_G_ERR "Failed to get Dom%d's shared_info GFN (%lx)\n",
> +               d->domain_id, gfn);
> +        ret = -EINVAL;
> +        goto exit_put_page;
> +    }
> +
> +    new_page = alloc_domheap_page(d, 0);
> +    if ( !new_page )
> +    {
> +        printk(XENLOG_G_ERR "Failed to alloc a page to replace"
> +               " Dom%d's shared_info frame %lx\n", d->domain_id, gfn);
> +        ret = -ENOMEM;
> +        goto exit_put_gfn;
> +    }
> +    mfn_new = page_to_mfn(new_page);

mfn_new is used just once, and hence this variable can be easily
eliminated (either be re-using mfn after switching this and the
following lines, or by just not using an intermediate variable here
at all.

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1035,6 +1035,10 @@ int domain_soft_reset(struct domain *d)
>      for_each_vcpu ( d, v )
>          unmap_vcpu_info(v);
>  
> +    rc = arch_domain_soft_reset(d);
> +    if (rc)
> +        return rc;

I don't think this can be done this late, or else you'd need to have
a way to undo everything you've done earlier in the function. This
of course also applies to what's being done inside the function. Or
if undoing is - as it looks like - rather hard, perhaps the domain
would better be crashed than left in an inconsistent state?

Also - coding style.

Jan