Re: [Xen-devel] [PATCH V2 2/2] x86/mm: Make use of the default access param from xc_altp2m_create_view

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 12.11.2019 14:02, Jan Beulich wrote:
> On 06.11.2019 16:35, Alexandru Stefan ISAILA wrote:
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -1345,13 +1345,14 @@ void setup_ept_dump(void)
>>       register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 
>> 0);
>>   }
>>   
>> -void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
>> +void p2m_init_altp2m_ept(struct domain *d, unsigned int i,
>> +                         p2m_access_t default_access)
>>   {
>>       struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
>>       struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>>       struct ept_data *ept;
>>   
>> -    p2m->default_access = hostp2m->default_access;
>> +    p2m->default_access = default_access;
>>       p2m->domain = hostp2m->domain;
>>   
>>       p2m->global_logdirty = hostp2m->global_logdirty;
> 
> All of this is not EPT-specific. Before adding more infrastructure to
> cover for this (here: another function parameter), how about moving
> these parts into vendor-independent code?

Ok, I will move the non ept code in p2m_activate_altp2m().

> 
>> @@ -2572,17 +2574,36 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned 
>> int idx)
>>       altp2m_list_lock(d);
>>   
>>       if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>> -        rc = p2m_activate_altp2m(d, idx);
>> +        rc = p2m_activate_altp2m(d, idx, hostp2m->default_access);
>>   
>>       altp2m_list_unlock(d);
>>       return rc;
>>   }
>>   
>> -int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
>> +int p2m_init_next_altp2m(struct domain *d, uint16_t *idx,
>> +                         uint16_t hvmmem_default_access)
>>   {
>>       int rc = -EINVAL;
>>       unsigned int i;
>>   
>> +    static const p2m_access_t memaccess[] = {
>> +#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
>> +        ACCESS(n),
>> +        ACCESS(r),
>> +        ACCESS(w),
>> +        ACCESS(rw),
>> +        ACCESS(x),
>> +        ACCESS(rx),
>> +        ACCESS(wx),
>> +        ACCESS(rwx),
>> +        ACCESS(rx2rw),
>> +        ACCESS(n2rwx),
>> +#undef ACCESS
>> +    };
>> +
>> +    if ( hvmmem_default_access > XENMEM_access_default )
>> +        return rc;
>> +
>>       altp2m_list_lock(d);
>>   
>>       for ( i = 0; i < MAX_ALTP2M; i++ )
>> @@ -2590,7 +2611,7 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t 
>> *idx)
>>           if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
>>               continue;
>>   
>> -        rc = p2m_activate_altp2m(d, i);
>> +        rc = p2m_activate_altp2m(d, i, memaccess[hvmmem_default_access]);
> 
> Aren't you open-coding xenmem_access_to_p2m_access() here? In
> no event should there be two instances of the same static array.

I did this because xenmem_access_to_p2m_access() is defined static in 
x86/mm/mem_access.c. If it's ok to have it defined in mem_access.h then 
I can go with that and drop this part of the code.

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