Re: [PATCH v1] Fix p2m_set_suppress_ve

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 4/3/19 6:30 PM, Jan Beulich wrote:
>>>> On 03.04.19 at 17:17, <rcojocaru@bitdefender.com> wrote:
>> 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.
> 
> I see.

On the UNSHARE observation, we don't know why the author originally 
requested the flag. We decided to keep it on the assumption that it 
_probably_ handles some corner-case that somebody has come accross.

We'll prepare a mini-series factoring out the code we've been discussing 
in separate functions: one for getting things out of the hostp2m if the 
entry is not present in the altp2m, and one for the special 
page-order-dependent code (which is duplicated in 
p2m_set_altp2m_mem_access() and p2m_change_altp2m_gfn()).

Before going into that, are we now certain that ALLOC is sufficient? I 
believe it should be for _our_ use-cases, but we don't want to break 
anyone's code. Maybe Tamas knows more about this.


Thanks,
Razvan

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