Re: [PATCH v3 1/3] x86/mm: Introduce altp2m_get_gfn_type_access

[Alexandru Stefan ISAILA <aisaila@bitdefender.com>]

On 11.04.2019 16:28, Tamas K Lengyel wrote:
> On Thu, Apr 11, 2019 at 6:50 AM George Dunlap <george.dunlap@citrix.com> wrote:
>>
>> On 4/11/19 1:17 PM, Alexandru Stefan ISAILA wrote:
>>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>>>>> index b9bbb8f485..d38d7c29ca 100644
>>>>> --- a/xen/arch/x86/mm/p2m.c
>>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>>> @@ -2626,6 +2626,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>>>        mfn_t mfn;
>>>>>        unsigned int page_order;
>>>>>        int rc = -EINVAL;
>>>>> +    bool copied_from_hostp2m;
>>>>>
>>>>>        if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
>>>>>            return rc;
>>>>> @@ -2636,7 +2637,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
>>>>>        p2m_lock(hp2m);
>>>>>        p2m_lock(ap2m);
>>>>>
>>>>> -    mfn = ap2m->get_entry(ap2m, old_gfn, &t, &a, 0, NULL, NULL);
>>>>> +    mfn = altp2m_get_gfn_type_access(ap2m, old_gfn, &t, &a, &page_order, &copied_from_hostp2m);
>>>>
>>>> Before, if new_gfn was INVALID_GFN, then the host p2m wasn't read at
>>>> all.  Now, the hostp2m will have __get_gfn_type_access() called with
>>>> P2M_ALLOC | P2M_UNSHARE.  Is that change intentional, and if so, why?
>>>
>>> This has been requested by Tamas in v2.
>>
>> That's nice, but 1) you still haven't answered the question, and 2) it's
>> not in the changelog.
> 
> What I requested was that if remapping is being done then both the old
> and new gfn's should be unshared in the hostp2m for keeping things
> consistent. The page type of old_gfn was already checked whether it's
> p2m_ram_rw and bail if it wasn't so functionality-wise this just
> simplifies things as a user don't have to request unsharing manually
> before remapping. Now, if the new_gfn is invalid it shouldn't query
> the hostp2m as that is effectively a request to remove the entry from
> the altp2m. But provided that scenario is used only when removing
> entries that were previously remapped/copied to the altp2m, those
> entries already went through P2M_ALLOC | P2M_UNSHARE before, so it
> won't have an affect. But it's also pointless.
> 

If this has been cleared there was one more question, where should the 
"if ( !mfn_valid(*mfn) || *t != p2m_ram_rw )" check end up in the 
attached form of the patch?

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