Re: [PATCH v2 21/25] arm/altp2m: Add HVMOP_altp2m_change_gfn.

[Sergej Proskurin <proskurin@sec.in.tum.de>]

Hi Julien,


On 08/06/2016 04:34 PM, Julien Grall wrote:
>
>
> On 06/08/2016 14:45, Sergej Proskurin wrote:
>> Hi Julien,
>
> Hello Sergej,
>
>> On 08/04/2016 04:04 PM, Julien Grall wrote:
>>> On 01/08/16 18:10, Sergej Proskurin wrote:
>>>> +        return rc;
>>>> +
>>>> +    hp2m = p2m_get_hostp2m(d);
>>>> +    ap2m = d->arch.altp2m_p2m[idx];
>>>> +
>>>> +    altp2m_lock(d);
>>>> +
>>>> +    /*
>>>> +     * Flip mem_access_enabled to true when a permission is set, as
>>>> to prevent
>>>> +     * allocating or inserting super-pages.
>>>> +     */
>>>> +    ap2m->mem_access_enabled = true;
>>>
>>> Can you give more details about why you need this?
>>>
>>
>> Similar to altp2m_set_mem_access, if we remap a page that is part of a
>> super page in the hostp2m, we first map the superpage in form of 512
>> pages into the ap2m and then change only one page. So, we set
>> mem_access_enabled to true to shatter the superpage on the ap2m side.
>
> mem_access_enabled should only be set when mem access is enabled and
> nothing.
>
> I don't understand why you want to avoid superpage in the altp2m. If
> you copy a host mapping is a superpage, then a altp2m mapping should
> be a superpage.
>
> The code is able to cope with inserting a mapping in the middle of a
> superpage without mem_access_enabled.
>

Alright, I will try it out in the next patch. Thank you.

>>
>>>> +
>>>> +    mfn = p2m_lookup_attr(ap2m, old_gfn, &p2mt, &level, NULL, NULL);
>>>> +
>>>> +    /* Check whether the page needs to be reset. */
>>>> +    if ( gfn_eq(new_gfn, INVALID_GFN) )
>>>> +    {
>>>> +        /* If mfn is mapped by old_gpa, remove old_gpa from the
>>>> altp2m table. */
>>>> +        if ( !mfn_eq(mfn, INVALID_MFN) )
>>>> +        {
>>>> +            rc = remove_altp2m_entry(d, ap2m, old_gpa,
>>>> pfn_to_paddr(mfn_x(mfn)), level);
>>>
>>> remove_altp2m_entry should take a gfn and mfn in parameter and not an
>>> address. The latter is a call for misusage of the API.
>>>
>>
>> Ok. This will also remove the need for level_sizes/level_masks in the
>> associated function.
>>
>>>> +            if ( rc )
>>>> +            {
>>>> +                rc = -EINVAL;
>>>> +                goto out;
>>>> +            }
>>>> +        }
>>>> +
>>>> +        rc = 0;
>>>> +        goto out;
>>>> +    }
>>>> +
>>>> +    /* Check host p2m if no valid entry in altp2m present. */
>>>> +    if ( mfn_eq(mfn, INVALID_MFN) )
>>>> +    {
>>>> +        mfn = p2m_lookup_attr(hp2m, old_gfn, &p2mt, &level, NULL,
>>>> &xma);
>>>> +        if ( mfn_eq(mfn, INVALID_MFN) || (p2mt != p2m_ram_rw) )
>>>
>>> Please add a comment to explain why the second check.
>>>
>>
>> Ok, I will. It has the same reason as in patch #19: It is not sufficient
>> so simply check for invalid MFN's as the type might be invalid. Also,
>> the x86 implementation did not allow to remap a gfn to a shared page.
>
> Patch #19 has a different check which does not explain this one. (p2mt
> != p2m_ram_rw) only guest read-write RAM can effectively be remapped
> which is different than shared page cannot be remapped.
>
> BTW, ARM does not support shared page.
>
> This also lead to my question, why not allowing p2m_ram_ro?
>

I don't see a reason why not. Thank you. I will remove the check.

>>
>>>> +        {
>>>> +            rc = -EINVAL;
>>>> +            goto out;
>>>> +        }
>>>> +
>>>> +        /* If this is a superpage, copy that first. */
>>>> +        if ( level != 3 )
>>>> +        {
>>>> +            rc = modify_altp2m_entry(d, ap2m, old_gpa,
>>>> pfn_to_paddr(mfn_x(mfn)),
>>>> +                                     level, p2mt, memaccess[xma]);
>>>> +            if ( rc )
>>>> +            {
>>>> +                rc = -EINVAL;
>>>> +                goto out;
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +
>>>> +    mfn = p2m_lookup_attr(ap2m, new_gfn, &p2mt, &level, NULL, &xma);
>>>> +
>>>> +    /* If new_gfn is not part of altp2m, get the mapping information
>>>> from hp2m */
>>>> +    if ( mfn_eq(mfn, INVALID_MFN) )
>>>> +        mfn = p2m_lookup_attr(hp2m, new_gfn, &p2mt, &level, NULL,
>>>> &xma);
>>>> +
>>>> +    if ( mfn_eq(mfn, INVALID_MFN) || (p2mt != p2m_ram_rw) )
>>>
>>> Please add a comment to explain why the second check.
>>>
>>
>> Same reason as above.
>
> Then add a comment in the code.

I will also remove this check.

Best regards,
~Sergej


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