Re: [PATCH v1] Fix p2m_set_suppress_ve

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 4/3/19 5:58 PM, Jan Beulich wrote:
>>>> On 03.04.19 at 16:29, <aisaila@bitdefender.com> wrote:
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -3011,8 +3011,16 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
>>       mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
>>       if ( !mfn_valid(mfn) )
>>       {
>> -        rc = -ESRCH;
>> -        goto out;
>> +        unsigned int page_order;
>> +
>> +        mfn = __get_gfn_type_access(host_p2m, gfn_x(gfn), &t, &a,
>> +                                    P2M_ALLOC | P2M_UNSHARE, &page_order, 0);
> 
> I'm not entirely certain about P2M_ALLOC, but I'm pretty sure that
> at least P2M_UNSHARE is too heavy: Why would you want to force
> un-sharing of a page when all you want to alter is #VE behavior?

That logic was taken from p2m_set_altp2m_mem_access(), we thought the 
two cases are very similar.

269     mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
270
271     /* Check host p2m if no valid entry in alternate */
272     if ( !mfn_valid(mfn) )
273     {
274
275         mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
276                                     P2M_ALLOC | P2M_UNSHARE, 
&page_order, 0);
277
278         rc = -ESRCH;
279         if ( !mfn_valid(mfn) || t != p2m_ram_rw )
280             return rc;
281
282         /* If this is a superpage, copy that first */
283         if ( page_order != PAGE_ORDER_4K )
284         {
285             unsigned long mask = ~((1UL << page_order) - 1);
286             gfn_t gfn2 = _gfn(gfn_l & mask);
287             mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
288
289             rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, 
old_a, 1);
290             if ( rc )
291                 return rc;
292         }
293     }
294
295     /*
296      * Inherit the old suppress #VE bit value if it is already set, 
or set it
297      * to 1 otherwise
298      */
299     return ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a, -1);
300 }

I wonder if we should put the whole logic in the "if ( !mfn_valid(mfn) 
)" body in its own function and reuse that for both functions - although 
it doesn't look like the extra superpage logic matters for setting the 
suppress #VE bit alone (since even the code above only sets it with 
PAGE_ORDER_4K).

> Additionally, when you add such a lookup as error handling attempt,
> I think it is important to leave a code comment. But I wonder
> whether this shouldn't be done before the call to ->get_entry(), or
> whether in fact there's a bug here in how get_entry() behaves in
> this case.

Changes to the hostp2m (also known as altp2m view 0) propagate to all 
existing altp2ms, but they do so in a lazy manner, and also that won't 
happen for altp2ms created after a while. So altp2ms will not 
necessarily know about a page that the hostp2m knows about, which should 
not stop us from setting mem access restrictions or the value of the SVE 
bit.


Thanks,
Razvan

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