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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 18.12.2019 12:45, Jan Beulich wrote:
> On 17.12.2019 16:12, Alexandru Stefan ISAILA wrote:
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -314,9 +314,9 @@ static int set_mem_access(struct domain *d, struct 
>> p2m_domain *p2m,
>>       return rc;
>>   }
>>   
>> -static bool xenmem_access_to_p2m_access(struct p2m_domain *p2m,
>> -                                        xenmem_access_t xaccess,
>> -                                        p2m_access_t *paccess)
>> +bool xenmem_access_to_p2m_access(struct p2m_domain *p2m,
>> +                                 xenmem_access_t xaccess,
>> +                                 p2m_access_t *paccess)
> 
> Would you mind taking the opportunity and add const to the first
> parameter?

Sure, given that there will be a new version, it will add it.

> 
>> @@ -2601,7 +2610,15 @@ 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];
>> +
>> +        if ( !xenmem_access_to_p2m_access(p2m, hvmmem_default_access, &a) )
>> +        {
>> +            altp2m_list_unlock(d);
>> +            return -EINVAL;
>> +        }
> 
> Can this be pulled out of the locked region, ahead of the loop?
> The p2m getting passed in here (which is why it's in the loop)
> shouldn't have been in use yet, i.e. its ->default_access should
> have a known value. Hence this case could be taken care of
> independently, e.g. by adjusting xenmem_access_to_p2m_access()
> to cope with a NULL p2m coming in (producing whatever the default
> of the default is).
> 

Yes this sounds good. In xenmem_access_to_p2m_access() there can be a 
check like:

if ( !p2m )
     *paccess = p2m_access_rwx;
else
     *paccess = p2m->default_access;

But before I change this maybe Tamas or George have something to add?
And can this stay in the same patch or should it have a prereq one?

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