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

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

On Tue, Jul 14, 2015 at 05:52:44PM +0200, Vitaly Kuznetsov wrote:
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> writes:
> 
> > On Tue, Jun 23, 2015 at 06:11:50PM +0200, Vitaly Kuznetsov 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;
> >> +    unsigned int i;
> >> +
> >> +    /* Soft reset is supported for HVM domains only */
> >
> > Missing full stop. But perhaps we could explain what would be needed
> > for an PV guest to make it work (not as something for you to do
> > of course but an victim^H^H^Hvolunteer!).
> >
> 
> Oh, I seriously doubt we'll ever be doing soft reset for classic PV. I
> don't see an easy way to preserve existent memory and without this soft
> reset is pointless. PVH (or PVHVM-without-dm) looks much more
> promising.
> 
> >> +    if ( !is_hvm_domain(d) )
> >> +        return -EINVAL;
> 
> I suggest we leave it here with the comment above and decide something
> later based on the chosen path for PVH.
> 
> >> +
> >> +    hvm_destroy_all_ioreq_servers(d);
> >> +
> >> +    spin_lock(&d->event_lock);
> >> +    for ( i = 1; i < d->nr_pirqs ; i ++ )
> >
> > Is pirq 0 special?
> >
> 
> No, for some reason I though it is not a valid number for pirq. Will fix
> in v9.
> 
> >> +    if ( owner != d )
> >> +        goto exit_put_page;
> >> +
> >> +    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);
> >
> > Would it be good to print out the PFN of the shared page to help narrow the cause?
> >
> 
> I think this case is impossibe under normal circumstances and it's not
> clear to me how to get the PFN (did you mean GFN?) in such case.

Yes.
> 
> shared_info is always allocated in arch_domain_create() from Xen heap
> and page_to_mfn() will never return INVALID_MFN here. In case we'll ever
> see this printed we'll have examine why this is not true anymore. This
> piece of code will have to be updated.

Ok, One way it could be if the guest decided to expunge this GFN fro
the guest (I think). Thought I am not sure why it would do such a thing :-)

> 
> >> +    if ( ret )
> >> +        printk(XENLOG_G_ERR "Failed to add a page to replace"
> >> +               " Dom%d's shared_info frame %lx\n", d->domain_id, gfn);
> >
> > Should we free the new_page in this case?
> >
> 
> The new page is already in domain's page_list so we won't lose it on
> domain destroy but there is also no point in keeping it there if we
> failed to add it to physmap. Will free it in v9.

Thanks!
> 
> 
> -- 
>   Vitaly