Re: [Xen-devel] [PATCH v2] x86/mm: Add mem access rights to NPT

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 29.08.2019 18:04, Jan Beulich wrote:
> On 22.08.2019 16:02, Alexandru Stefan ISAILA wrote:
>> This patch adds access control for NPT mode.
>>
>> The access rights are stored in the NPT p2m table 56:53.
> 
> Why starting from bit 53? I can't seem to find any use of bit 52.

There is a comment in page.h that warns that bit 12(52) is taken.
"/*
  * Bit 12 of a 24-bit flag mask. This corresponds to bit 52 of a pte.
  * This is needed to distinguish between user and kernel PTEs since 
_PAGE_USER
  * is asserted for both.
  */
#define _PAGE_GUEST_KERNEL (1U<<12)
"

> 
>> The bits are free after clearing the IOMMU flags [1].
>>
>> Modify p2m_type_to_flags() to accept and interpret an access value,
>> parallel to the ept code.
>>
>> Add a set_default_access() method to the p2m-pt and p2m-ept versions
>> of the p2m rather than setting it directly, to deal with different
>> default permitted access values.
> 
> I think this would better be a separate change.

Ok I will brake the patch in two parts. I was following George's v1 patch.

> 
>> --- a/xen/arch/x86/mm/p2m-pt.c
>> +++ b/xen/arch/x86/mm/p2m-pt.c
>> @@ -63,10 +63,13 @@
>>   #define needs_recalc(level, ent) _needs_recalc(level##e_get_flags(ent))
>>   #define valid_recalc(level, ent) (!(level##e_get_flags(ent) & 
>> _PAGE_ACCESSED))
>>   
>> +#define _PAGE_ACCESS_BITS  (0x1e00) /* mem_access rights 16:13 */
> 
> I guess this is too disconnected from the two page.h headers where
> the correlation between bit positions gets explained, so I guess
> you want to extend the comment and either refer there, or replicate
> some of it to make understandable why 16:13 matches 56:53.
> 

I will extend the comment so as the bit shifting will be clear.

> I'm also concerned how easy it'll be for someone to find this
> definition when wanting to use other of the available bits.

Yes you are right, I will add a comment in page.h that bits 56:53 are 
used for memory access rights on SVM.

> 
>> @@ -104,8 +112,32 @@ static unsigned long p2m_type_to_flags(const struct 
>> p2m_domain *p2m,
>>               flags |= _PAGE_PWT;
>>               ASSERT(!level);
>>           }
>> -        return flags | P2M_BASE_FLAGS | _PAGE_PCD;
>> +        flags |= P2M_BASE_FLAGS | _PAGE_PCD;
>> +        break;
>>       }
>> +
>> +    switch ( access )
>> +    {
>> +    case p2m_access_r:
>> +        flags |= _PAGE_NX_BIT;
>> +        flags &= ~_PAGE_RW;
>> +        break;
>> +    case p2m_access_rw:
>> +        flags |= _PAGE_NX_BIT;
>> +        break;
>> +    case p2m_access_rx:
>> +    case p2m_access_rx2rw:
>> +        flags &= ~(_PAGE_NX_BIT | _PAGE_RW);
>> +        break;
>> +    case p2m_access_x:
>> +        flags &= ~_PAGE_RW;
>> +        break;
> 
> I can't seem to be able to follow you here. In fact I don't see
> how you would be able to express execute-only with NPT. If this
> is really needed for some reason, then a justifying comment
> should be added.

Execute-only should be expressed as not PAGE_RW and PAGE_NX_BIT not set.

> 
>> @@ -152,6 +184,17 @@ p2m_free_entry(struct p2m_domain *p2m, l1_pgentry_t 
>> *p2m_entry, int page_order)
>>       p2m_free_ptp(p2m, l1e_get_page(*p2m_entry));
>>   }
>>   
>> +static void p2m_set_access(intpte_t *entry, p2m_access_t access)
>> +{
>> +    *entry |= put_pte_flags((get_pte_flags(*entry) & ~_PAGE_ACCESS_BITS) |
>> +                            (MASK_INSR(access, _PAGE_ACCESS_BITS)));
>> +}
>> +
>> +static p2m_access_t p2m_get_access(intpte_t entry)
>> +{
>> +    return (p2m_access_t)(MASK_EXTR(get_pte_flags(entry), 
>> _PAGE_ACCESS_BITS));
> 
> Is the cast really needed here?

Not really, I can remove it in the next version.

> 
>> @@ -226,6 +269,7 @@ p2m_next_level(struct p2m_domain *p2m, void **table,
>>           {
>>               new_entry = l1e_from_pfn(pfn | (i << ((level - 1) * 
>> PAGETABLE_ORDER)),
>>                                        flags);
>> +            p2m_set_access(&new_entry.l1, p2m->default_access);
>>               rc = p2m->write_p2m_entry(p2m, gfn, l1_entry + i, new_entry, 
>> level);
>>               if ( rc )
>>               {
>> @@ -237,6 +281,7 @@ p2m_next_level(struct p2m_domain *p2m, void **table,
>>           unmap_domain_page(l1_entry);
>>   
>>           new_entry = l1e_from_mfn(mfn, P2M_BASE_FLAGS | _PAGE_RW);
>> +        p2m_set_access(&new_entry.l1, p2m->default_access);
>>           rc = p2m->write_p2m_entry(p2m, gfn, p2m_entry, new_entry,
>>                                     level + 1);
>>           if ( rc )
> 
> Is it really intended to insert the access bits also into non-leaf
> entries (which may be what is being processed here)? (May also
> apply further down.)
> 
>> @@ -474,6 +520,32 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa)
>>       return rc;
>>   }
>>   
>> +static int p2m_pt_check_access(p2m_access_t p2ma)
>> +{
>> +    switch ( p2ma )
>> +    {
>> +    case p2m_access_n:
>> +    case p2m_access_w:
>> +    case p2m_access_wx:
>> +    case p2m_access_n2rwx:
>> +        return -EINVAL;
> 
> I'm not convinced EINVAL is appropriate here - the argument isn't
> invalid, it's just that there's no way to represent it.

Would EPERM be a better return here?

Regards,
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel