Re: [Xen-devel] [PATCH RFC v2 07/12] x86: allow per-domain mappings without NX bit or with specific mfn

[Juergen Gross <jgross@xxxxxxxx>]

On 29/01/18 18:06, Jan Beulich wrote:
>>>> On 22.01.18 at 13:32, <jgross@xxxxxxxx> wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -1568,7 +1568,7 @@ void init_xen_l4_slots(l4_pgentry_t *l4t, mfn_t l4mfn,
>>  
>>      /* Slot 260: Per-domain mappings (if applicable). */
>>      l4t[l4_table_offset(PERDOMAIN_VIRT_START)] =
>> -        d ? l4e_from_page(d->arch.perdomain_l3_pg, __PAGE_HYPERVISOR_RW)
>> +        d ? l4e_from_page(d->arch.perdomain_l3_pg, __PAGE_HYPERVISOR)
>>            : l4e_empty();
>>  
>>      /* Slot 261-: text/data/bss, RW M2P, vmap, frametable, directmap. */
>> @@ -5269,7 +5269,7 @@ int create_perdomain_mapping(struct domain *d, 
>> unsigned long va,
>>          }
>>          l2tab = __map_domain_page(pg);
>>          clear_page(l2tab);
>> -        l3tab[l3_table_offset(va)] = l3e_from_page(pg, 
>> __PAGE_HYPERVISOR_RW);
>> +        l3tab[l3_table_offset(va)] = l3e_from_page(pg, __PAGE_HYPERVISOR);
>>      }
>>      else
>>          l2tab = map_l2t_from_l3e(l3tab[l3_table_offset(va)]);
>> @@ -5311,7 +5311,7 @@ int create_perdomain_mapping(struct domain *d, 
>> unsigned long va,
>>                  l1tab = __map_domain_page(pg);
>>              }
>>              clear_page(l1tab);
>> -            *pl2e = l2e_from_page(pg, __PAGE_HYPERVISOR_RW);
>> +            *pl2e = l2e_from_page(pg, __PAGE_HYPERVISOR);
> 
> These changes (in the absence of the description saying otherwise)
> leave open whether any of the per-domain mappings now suddenly
> become executable.

Are you fine with me adding something like the following to the commit
message:

As create_perdomain_mapping() creates L1 mappings with flags being
__PAGE_HYPERVISOR_RW this won't change any of the current per domain
mappings to become executable.

> 
>> @@ -5401,6 +5401,81 @@ void destroy_perdomain_mapping(struct domain *d, 
>> unsigned long va,
>>      unmap_domain_page(l3tab);
>>  }
>>  
>> +void flipflags_perdomain_mapping(struct domain *d, unsigned long va,
>> +                                 unsigned int flags)
> 
> Flipping flags means the caller has to know (perhaps track) what state
> the flags are in at present. I think it would be better to pass in two
> masks - one for flags to be set, and the other for flags to be cleared.

Okay.

> 
>> +void addmfn_to_perdomain_mapping(struct domain *d, unsigned long va, mfn_t 
>> mfn)
>> +{
>> +    const l3_pgentry_t *l3tab, *pl3e;
>> +
>> +    ASSERT(va >= PERDOMAIN_VIRT_START &&
>> +           va < PERDOMAIN_VIRT_SLOT(PERDOMAIN_SLOTS));
>> +
>> +    if ( !d->arch.perdomain_l3_pg )
>> +        return;
>> +
>> +    l3tab = __map_domain_page(d->arch.perdomain_l3_pg);
>> +    pl3e = l3tab + l3_table_offset(va);
>> +
>> +    if ( l3e_get_flags(*pl3e) & _PAGE_PRESENT )
>> +    {
>> +        const l2_pgentry_t *l2tab = map_l2t_from_l3e(*pl3e);
>> +        const l2_pgentry_t *pl2e = l2tab + l2_table_offset(va);
>> +
>> +        if ( l2e_get_flags(*pl2e) & _PAGE_PRESENT )
>> +        {
>> +            l1_pgentry_t *l1tab = map_l1t_from_l2e(*pl2e);
>> +            unsigned int off = l1_table_offset(va);
>> +
>> +            if ( (l1e_get_flags(l1tab[off]) & (_PAGE_PRESENT | 
>> _PAGE_AVAIL0)) ==
>> +                 (_PAGE_PRESENT | _PAGE_AVAIL0) )
>> +                free_domheap_page(l1e_get_page(l1tab[off]));
>> +
>> +            l1tab[off] = l1e_from_mfn(mfn, __PAGE_HYPERVISOR_RW);
>> +
>> +            unmap_domain_page(l1tab);
>> +        }
>> +
>> +        unmap_domain_page(l2tab);
>> +    }
>> +
>> +    unmap_domain_page(l3tab);
>> +}
> 
> Here even more than in the flipflags function - what if an
> intermediate page table entry was not present? The caller will
> have no ideal that what was requested wasn't carried out.

I'll add returning -ENOENT for both functions in that case.


Juergen

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