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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 24.12.2019 10:48, George Dunlap wrote:
> On 12/23/19 2:04 PM, Alexandru Stefan ISAILA wrote:
>> At this moment the default_access param from xc_altp2m_create_view is
>> not used.
>>
>> This patch assigns default_access to p2m->default_access at the time of
>> initializing a new altp2m view.
> 
> That's certainly not what it looks like.  It looks like you're changing
> it from...
> 
>> @@ -2562,7 +2564,7 @@ static int p2m_activate_altp2m(struct domain *d,
> unsigned int idx)
>>           goto out;
>>       }
>>
>> -    p2m->default_access = hostp2m->default_access;
>> +    p2m->default_access = hvmmem_default_access;
>>       p2m->domain = hostp2m->domain;
>>       p2m->global_logdirty = hostp2m->global_logdirty;
>>       p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
> 
> ...hostp2m->default_access to...
> 
>> @@ -340,7 +340,10 @@ static bool xenmem_access_to_p2m_access(struct
> p2m_domain *p2m,
>>           *paccess = memaccess[xaccess];
>>           break;
>>       case XENMEM_access_default:
>> -        *paccess = p2m->default_access;
>> +        if ( !p2m )
>> +            *paccess = p2m_access_rwx;
>> +        else
>> +            *paccess = p2m->default_access;
>>           break;
>>       default:
>>           return false;
> 
> ...p2m_access_rwx (by passing NULL in to this function in
> p2m_init_next_altp2m).
> 
> Why don't you...
> 
>> -int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
>> +int p2m_init_next_altp2m(struct domain *d, uint16_t *idx,
>> +                         xenmem_access_t hvmmem_default_access)
>>   {
>>       int rc = -EINVAL;
>>       unsigned int i;
>> +    p2m_access_t a;
>> +    struct p2m_domain *p2m;
>> +
>> +    if ( hvmmem_default_access > XENMEM_access_default ||
>> +         !xenmem_access_to_p2m_access(NULL, hvmmem_default_access, &a) )
>> +        return rc;
>>
>>       altp2m_list_lock(d);
>>
> 
> ...pass in hostp2m here?

That sound better then the current version, thanks.

> 
> Also...
> 
>> @@ -2606,7 +2616,8 @@ 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);
>> +        p2m = d->arch.altp2m_p2m[i];
> 
> What's this about?
> 

This is an artifact form v3 when xenmem_access_to_p2m_access() needed 
p2m. I will clean it.

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