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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 29.11.2019 13:41, Jan Beulich wrote:
> On 21.11.2019 16:02, Alexandru Stefan ISAILA wrote:
>> Changes since V2:
>>      - Drop static from xenmem_access_to_p2m_access() and declare it
>> in mem_access.h
>>      - Use xenmem_access_to_p2m_access() in p2m_init_next_altp2m()
>>      - Pull out the p2m specifics from p2m_init_altp2m_ept().
> 
> I guess this last point would better have been a prereq patch,
> but anyway.

Should I have a prereq patch for this in the next version?

> 
>> @@ -2577,16 +2586,23 @@ 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)
> 
> Does this new parameter really need to be a fixed width type,
> rather than simply unsigned int (or even a suitable enum
> type if there [hopefully] is one)?

I think xenmem_access_t would be a good fit here.

> 
>>   {
>>       int rc = -EINVAL;
>>       unsigned int i;
>> +    p2m_access_t a;
>> +    struct p2m_domain *p2m;
>> +
>> +
> 
> Two successive blank lines again.

I will fix that.

> 
>> @@ -2595,7 +2611,12 @@ 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) )
>> +            return -EINVAL;
> 
> Returning with a lock still held?

Thanks for spotting this, it definitely needs a free.

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