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

[Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>]

On 09.04.2019 18:26, Tamas K Lengyel wrote:
> On Tue, Apr 9, 2019 at 8:48 AM Alexandru Stefan ISAILA
> <aisaila@xxxxxxxxxxxxxxx> wrote:
>>
>>
>>
>> On 09.04.2019 17:37, Tamas K Lengyel wrote:
>>> On Tue, Apr 9, 2019 at 8:03 AM Alexandru Stefan ISAILA
>>> <aisaila@xxxxxxxxxxxxxxx> wrote:
>>>>
>>>>
>>>>
>>>> On 09.04.2019 16:48, Tamas K Lengyel wrote:
>>>>> On Tue, Apr 9, 2019 at 6:04 AM Alexandru Stefan ISAILA
>>>>> <aisaila@xxxxxxxxxxxxxxx> wrote:
>>>>>>
>>>>>> This patch moves common code from p2m_set_altp2m_mem_access() and
>>>>>> p2m_change_altp2m_gfn() into one function
>>>>>>
>>>>>> Signed-off-by: Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>
>>>>>>
>>>>>> ---
>>>>>> Changes since V2:
>>>>>>            - Change var name from found_in_hostp2m to copied_from_hostp2m
>>>>>>            - Move the type check from altp2m_get_gfn_type_access() to the
>>>>>>            callers.
>>>>>> ---
>>>>>>     xen/arch/x86/mm/mem_access.c | 32 ++++++++++++----------------
>>>>>>     xen/arch/x86/mm/p2m.c        | 41 
>>>>>> ++++++++++++++----------------------
>>>>>>     xen/include/asm-x86/p2m.h    | 19 +++++++++++++++++
>>>>>>     3 files changed, 49 insertions(+), 43 deletions(-)
>>>>>>
>>>>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>>>>>> index 56c06a4fc6..bf67ddb15a 100644
>>>>>> --- a/xen/arch/x86/mm/mem_access.c
>>>>>> +++ b/xen/arch/x86/mm/mem_access.c
>>>>>> @@ -265,31 +265,27 @@ int p2m_set_altp2m_mem_access(struct domain *d, 
>>>>>> struct p2m_domain *hp2m,
>>>>>>         unsigned int page_order;
>>>>>>         unsigned long gfn_l = gfn_x(gfn);
>>>>>>         int rc;
>>>>>> +    bool copied_from_hostp2m;
>>>>>>
>>>>>> -    mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
>>>>>> +    mfn = altp2m_get_gfn_type_access(ap2m, gfn, &t, &old_a, 
>>>>>> &page_order, &copied_from_hostp2m);
>>>>>>
>>>>>> -    /* Check host p2m if no valid entry in alternate */
>>>>>>         if ( !mfn_valid(mfn) )
>>>>>> +        return -ESRCH;
>>>>>> +
>>>>>> +    /* If this is a superpage, copy that first */
>>>>>> +    if ( page_order != PAGE_ORDER_4K && copied_from_hostp2m )

Regarding the drop of copied_from_hostp2m should this be dropped here as 
well and by that drop it all together?

>>>>>>         {
>>>>>> +        unsigned long mask = ~((1UL << page_order) - 1);
>>>>>> +        gfn_t gfn2 = _gfn(gfn_l & mask);
>>>>>> +        mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>>>>>
>>>>>> -        mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
>>>>>> -                                    P2M_ALLOC | P2M_UNSHARE, 
>>>>>> &page_order, 0);
>>>>>> +        /* Note: currently it is not safe to remap to a shared entry */
>>>>>> +        if ( t != p2m_ram_rw )
>>>>>> +            return -ESRCH;

And if so and regarding the comments from p2m_change_altp2m_gfn should I 
move the ( t != p2m_ram_rw ) check up with the ( !mfn_valid(mfn) ) check?

Alex

>>>>>>
>>>>>> -        rc = -ESRCH;
>>>>>> -        if ( !mfn_valid(mfn) || t != p2m_ram_rw )
>>>>>> +        rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, old_a, 1);
>>>>>> +        if ( rc )
>>>>>>                 return rc;
>>>>>> -
>>>>>> -        /* If this is a superpage, copy that first */
>>>>>> -        if ( page_order != PAGE_ORDER_4K )
>>>>>> -        {
>>>>>> -            unsigned long mask = ~((1UL << page_order) - 1);
>>>>>> -            gfn_t gfn2 = _gfn(gfn_l & mask);
>>>>>> -            mfn_t mfn2 = _mfn(mfn_x(mfn) & mask);
>>>>>> -
>>>>>> -            rc = ap2m->set_entry(ap2m, gfn2, mfn2, page_order, t, 
>>>>>> old_a, 1);
>>>>>> -            if ( rc )
>>>>>> -                return rc;
>>>>>> -        }
>>>>>>         }
>>>>>>
>>>>>>         /*
>>>>>> 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);
>>>>>>
>>>>>>         if ( gfn_eq(new_gfn, INVALID_GFN) )
>>>>>>         {
>>>>>> @@ -2646,37 +2647,27 @@ int p2m_change_altp2m_gfn(struct domain *d, 
>>>>>> unsigned int idx,
>>>>>>             goto out;
>>>>>>         }
>>>>>>
>>>>>> -    /* Check host p2m if no valid entry in alternate */
>>>>>> -    if ( !mfn_valid(mfn) )
>>>>>> -    {
>>>>>> -        mfn = __get_gfn_type_access(hp2m, gfn_x(old_gfn), &t, &a,
>>>>>> -                                    P2M_ALLOC, &page_order, 0);
>>>>>> +    if ( !mfn_valid(mfn) || (t != p2m_ram_rw && copied_from_hostp2m) )
>>>>>
>>>>> Is this check correct? Why do you want to get out only when type is
>>>>> non-rw *and* it's copied from the hostp2m? You could have non-rw
>>>>> entries like mmio in the altp2m that were lazily copied and I don't
>>>>> think we want to allow remapping to those either.
>>>>
>>>> I just copied the functionality. If this is needed I will add a || t !=
>>>> p2m_mmio_dm and p2m_mmio_direct.
>>>
>>> My problem is with the && copied_form_hostp2m part. Why is that a criteria?
>>
>> The (t != p2m_ram_rw) check was done only for the get from hostp2m.
>>
>> If you think that I should do the check for all mfns (hostp2 and altp2m)
>> then I can drop the copied_from_hostp2m bool and add mmio check.
> 
> I think you should just drop the && copied_from_hostp2m part of it.
> Remappings should only be allowed for p2m_ram_rw type, no matter which
> p2m its coming from.
> 

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