Re: [Xen-devel] [PATCH 14/18] arm/altp2m: Add HVMOP_altp2m_set_mem_access.

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

On 07/06/2016 06:12 PM, Tamas K Lengyel wrote:
> On Wed, Jul 6, 2016 at 8:32 AM, Julien Grall <julien.grall@xxxxxxx> wrote:
>>
>> On 05/07/16 22:55, Sergej Proskurin wrote:
>>> Hello Julien,
>>>
>>> On 07/05/2016 02:49 PM, Julien Grall wrote:
>>>> Hello Sergej,
>>>>
>>>> On 04/07/16 12:45, Sergej Proskurin wrote:
>>>>> +static inline
>>>>> +int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain
>>>>> *hp2m,
>>>>> +                              struct p2m_domain *ap2m, p2m_access_t a,
>>>>> +                              gfn_t gfn)
>>>>> +{
>>>>
>>>> [...]
>>>>
>>>>> +    /* Set mem access attributes - currently supporting only one (4K)
>>>>> page. */
>>>>> +    mask = level_masks[3];
>>>>> +    return apply_p2m_changes(d, ap2m, INSERT,
>>>>> +                             gpa & mask,
>>>>> +                             (gpa + level_sizes[level]) & mask,
>>>>> +                             maddr & mask, mattr, 0, p2mt, a);
>>>>
>>>> The operation INSERT will remove the previous mapping and decrease page
>>>> reference for foreign mapping (see p2m_put_l3_page). So if you set the
>>>> memory access for this kind of page, the reference count will be wrong
>>>> afterward.
>>>>
>>> I see your point. As far as I understand, the purpose of the function
>>> p2m_put_l3_page to simply decrement the ref count of the particular pte
>>> and free the page if its' ref count reaches zero. Since p2m_put_l3_page
>>> is called only twice in p2m.c, we could insert a check
>>> (p2m_is_hostp2m/altp2m) making sure that the ref count is decremented
>>> only if the p2m in question is the hostp2m. This will make sure that the
>>> ref count is maintained correctly.
>>
>> Why don't you use the operation MEMACCESS?
> I believe it would require duplicating some parts of the INSERT
> routine as MEMACCESS assumes the pte is already present in the p2m,
> which may not be the case for ap2m.
>
Yes, exactly. MEMACCESS expects the particular entry to be present in
the associated p2m view. We could use it but only in addition to INSERT.
Otherwise we will not be able to insert the entry into the targeted
altp2m view.Because of this, IMHO it would make sense to introduce an
additional (unlikely) check making sure that the ref count is not
decremented if altp2m is active.

Cheers,
~Sergej


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