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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

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.

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.

>> +    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.


-- 
  Vitaly