Re: [V4 PATCH 6/7] pvh dom0: Add and remove foreign pages

[Julien Grall <julien.grall@linaro.org>]

On 12/05/2013 01:40 AM, Mukesh Rathor wrote:
> On Wed, 04 Dec 2013 17:00:57 +0000
> Julien Grall <julien.grall@linaro.org> wrote:
>
>> On 12/04/2013 01:05 AM, Mukesh Rathor wrote:
>>> On Wed, 04 Dec 2013 00:00:54 +0000
>>> Julien Grall <julien.grall@linaro.org> wrote:
>>>
> ......
>> refcnt'd
>> +         * the page. See also xenmem_add_foreign_to_pmap().
>> +         */
>> +        page = get_page_from_gfn(d, xrfp.gpfn, &p2mt, P2M_ALLOC);
>> +
>> +        if ( page || p2m_is_foreign(p2mt) )
>>           {
>> -            guest_physmap_remove_page(d, xrfp.gpfn,
>> page_to_mfn(page), 0);
>> -            put_page(page);
>> +            if ( page )
>> +                mfn = page_to_mfn(page);
>> +            else
>> +            {
>> +                mfn = gmfn_to_mfn(d, xrfp.gpfn);
>> +                page = mfn_to_page(mfn);
>> +
>> +                ASSERT(d != page_get_owner(page));
>> +            }
>> +
>> +            guest_physmap_remove_page(d, xrfp.gpfn, mfn, 0);
>> +            if (page)
>> +                put_page(page);
>
>
> Why did you drop the following release of refcnt from here?

When it's the foreign page, I have assigned mfn_to_page() to the 
variable "page". It's avoid duplication code.

>
> +            if ( p2m_is_foreign(p2mt) )
> +            {
> +                put_page(mfn_to_page(mfn));
> +                put_gfn(d, xrfp.gpfn);
> +            }
>
>
>>           }
>>           else
>>               rc = -ENOENT;
>>
>> This change is assuming that:
>>    - p2m_is_foreign is defined on ARM (I have a patch for that)
>
> I added #define p2m_is_foreign to 0 in my patch for now.
>
>>    - gmfn_to_mfn is defined on x86 (I don't have a patch, but the code
>> should be trivial?)
>
> I'd rather just ifdef CONFIG_X86 like guest_remove_page() does in the
> same file for gmfn_to_mfn.
>
> Also, lets not mix any partial arm changes with these x86 changes here.
>
> So, in this patch, I've added p2m_is_foreign set to 0, and changed the
> remove path to remain exactly same for arm. So, arm will continue to work
> the same way, and no build break. This way, you can add arm change
> incrementally and we can track things nicely.

I'm fine with this solution as long as this patch series (or at least 
this patch) is applied for Xen 4.4. On ARM side it's a bug fix that must 
be resolved for the next release.

-- 
Julien Grall