Re: [Xen-devel] [PATCH v1] Fix p2m_set_suppress_ve

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.04.19 at 17:17, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 4/3/19 5:58 PM, Jan Beulich wrote:
>>>>> On 03.04.19 at 16:29, <aisaila@xxxxxxxxxxxxxxx> 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.

I see.

> 269     mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
> 270
> 271     /* Check host p2m if no valid entry in alternate */

This comment was not cloned.

> 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).

Indeed, this latter aspect doesn't make it look very attractive to
introduce a common helper. Otoh I wonder what other functions
might be affected.

>> 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.

But even if the propagation is lazy, a "get" should then return the
propagated value, shouldn't it? Or else callers (like is the case here)
can be mislead. I do recognize that there may be callers who
intentionally _don't_ want the propagated value, but I'd expect this
to be the exception, not the rule. I wonder therefore whether there
shouldn't be a wrapper around ->get_entry() to take care of this for
callers which want the propagated value. Calling the bare hook would
remain as is.

Jan



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