Re: [Xen-devel] [PATCH RFC 4/4] xen: arch-specific hooks for domain_soft_reset()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.06.15 at 15:35, <vkuznets@xxxxxxxxxx> wrote:
> +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;

No need for leading underscores here.

> +    if ( is_hvm_domain(d) )
> +    {

Isn't the shared_info manipulation also HVM specific? Or at least
paging_mode_translate()-specific? xenmem_add_to_physmap_one()
bails on !paging_mode_translate()... But then I'm not really
understanding the issue you try to address there anyway, and
hence I may be overlooking something: Why does allocating a new
page help with a subsequent XENMAPSPACE_shared_info add-to-
physmap request?

> +        int i;

unsigned int

> +
> +        spin_lock(&d->event_lock);
> +        for ( i = 1; i < d->nr_pirqs ; i ++ )
> +            if ( domain_pirq_to_emuirq(d, i) != IRQ_UNBOUND )
> +            {
> +                ret = unmap_domain_pirq_emuirq(d, i);
> +                if (ret)

Coding style.

> +                    break;
> +            }
> +        spin_unlock(&d->event_lock);
> +    }
> +
> +    if ( ret || !d->shared_info || !page )

Again - how would page end up being NULL here?

> +        return ret;
> +
> +    /*
> +     * shared_info page needs to be replaced with a new page, otherwise we
> +     * will get a hole if the domain does XENMAPSPACE_shared_info
> +     */
> +
> +    owner = page_get_owner_and_reference(page);
> +    if ( owner != d )
> +    {
> +        put_page(page);
> +        return 0;

Isn't this an error?

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

Ignoring the return value? I think you should do any consistency
check you can in code like this.

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1467,6 +1467,8 @@ int domain_soft_reset(struct domain *d)
>      for_each_vcpu ( d, v )
>              unmap_vcpu_info(v);
>  
> +    ret = arch_domain_soft_reset(d);
> +
>   vcpu_unpause:
>      for_each_vcpu ( d, v )
>          if ( v != current )

Similar question as on an earlier patch - is it really correct to unpause
the vCPU-s again after a possible failure from arch_domain_soft_reset()?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel