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

[Julien Grall <julien.grall@xxxxxxx>]

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

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

Regards,

--
Julien Grall

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