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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

"Jan Beulich" <JBeulich@suse.com> writes:

>>>> On 03.06.15 at 15:35, <vkuznets@redhat.com> 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?

When an HVM domain starts it selects one of its pages and sacrifices it
in favor of the shared_info page which belongs to xen heap,
XENMAPSPACE_shared_info maps this page instead of the sacrificed one
which is being freed. After kexec the domain can select another page it
is willing to sacrifice and ask to map shared_info there but if we just
remap it from its previous location we get a hole. So here I'm trying to
unmap the shared_info page from domain's address space replacing it with
an empty page.

The other possible solution was suggested by Tim: instead of sucha a
swap reassign the currently-used shared_info page to the domain and
allocate a new shared_info page in xen heap. I haven't tried this
approach.

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

It probably means the shared_info page was never mapped to the domain's
space, no need to replace it.

>> --- 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()?

Not sure what's the best here: unpausing and hoping the domain can cope
(e.g. in kdump case we don't need much) or destroying the domain as we
can't recover from all possible failures.

-- 
  Vitaly