Re: [PATCH v5 09/17] xen/arm: p2m type definitions and changes

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

Hello Tamas,

On 12/09/14 01:31, Tamas K Lengyel wrote:
>
>
> On Thu, Sep 11, 2014 at 10:49 PM, Julien Grall <julien.grall@linaro.org
> <mailto:julien.grall@linaro.org>> wrote:
>
>     Hi Tamas,
>
>     On 10/09/14 06:28, Tamas K Lengyel wrote:
>       >   static lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int
>     mattr,
>
>         -                               p2m_type_t t)
>         +                               p2m_type_t t, p2m_access_t a)
>            {
>                paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
>                /* sh, xn and write bit will be defined in the following
>         switches
>         @@ -347,7 +350,7 @@ static int p2m_create_table(struct domain
>         *d, lpae_t *entry,
>                     for ( i=0 ; i < LPAE_ENTRIES; i++ )
>                     {
>                         pte = mfn_to_p2m_entry(base_pfn +
>         (i<<(level_shift-LPAE_SHIFT)),
>         -                                    MATTR_MEM, t);
>         +                                    MATTR_MEM, t,
>         p2m->default_access);
>
>
>     I though I've talked about it in an earlier version. I don't think
>     we should use the default_access to the P2M table.
>
>     Let assume a user decides to set default to another access type than
>     p2m_access_rwx, Xen will receive the same trap forever for the
>     domain because the access is not store in the radix tree (see your
>     patch #12). Therefore the guest will try to access back to the
>     guest, but the page attribute has not changed.
>
>     Also, you can't associate a PFN to this page because the table is
>     internal to Xen.
>
>     IHMO, I would use p2m_access_rwx for the table L1 and L2 tables. For
>     L3, you will have to set the permission in the radix tree.
>
>
> I'm not sure I follow exactly what you are saying. Setting
> default_access only effects pages created after the fact and I see that
> if a large page is created this still causes a problem.. Is that what
> you mean? A better solution would be to only allow creating 4k pages if
> default_access != rwx IMHO and then store the setting in the radix tree
> automatically.

That what I was trying to say.

You can do it easily in the function is_mapping_aligned.

But for the intermediate page table (see the mfn_to_p2m_entry(...,..., 
p2m_invalid), you should not use default_access but directly access_rwx.


>
> I was also contemplating if it would make sense if a setting was not
> found in the radix tree, to check the violation against the
> default_access.

The violation of the page may be different than the default access. I 
think it could be a mess for the guest. The radix tree should containe 
very pte where the access != rwx.


> That way we would not have to store those entries in the
> radix tree that are the same as the default_access (and rwx of course
> would not go in the radix tree either). Might cut down the size of the tree.

The default_access may change after the insertion of the PTE. How will 
you handle this case? IHMO, I don't think there is simple solution.

Regards,

-- 
Julien Grall