On 4/16/19 3:19 PM, Tamas K Lengyel wrote:
> On Tue, Apr 16, 2019 at 8:02 AM George Dunlap <george.dun...@citrix.com>
> wrote:
>>
>> On 4/16/19 2:44 PM, Tamas K Lengyel wrote:
>>> On Tue, Apr 16, 2019 at 2:45 AM Alexandru Stefan ISAILA
>>> <aisa...@bitdefender.com> wrote:
>>>>
>>>> The code for getting the entry and then populating was repeated in
>>>> p2m_change_altp2m_gfn() and in p2m_set_altp2m_mem_access().
>>>>
>>>> The code is now in one place with a bool param that lets the caller choose
>>>> if it populates after get_entry().
>>>>
>>>> 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
>>>> doesn'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 so the core function get_altp2m_entry() is calling
>>>> __get_gfn_type_access() with P2M_ALLOC | P2M_UNSHARE.
>>>>
>>>> altp2m_get_entry_direct() is also called in p2m_set_suppress_ve()
>>>> because on a new altp2m view the function will fail with invalid mfn if
>>>> p2m->set_entry() was not called before.
>>>>
>>>> Signed-off-by: Alexandru Isaila <aisa...@bitdefender.com>
>>>> Signed-off-by: George Dunlap <george.dun...@citrix.com>
>>>> Reviewed-by: George Dunlap <george.dun...@citrix.com>
>>>>
>>>> ---
>>>> Changes since V4:
>>>> - Add altp2m to patch name
>>>> - Change func name from get_altp2m_entry() to
>>>> altp2m_get_entry().
>>>> ---
>>>> xen/arch/x86/mm/mem_access.c | 30 ++-----------
>>>> xen/arch/x86/mm/p2m.c | 84 ++++++++++++++++++++----------------
>>>> xen/include/asm-x86/p2m.h | 17 ++++++++
>>>> 3 files changed, 66 insertions(+), 65 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>>>> index a144bb0ce4..ddfe0169c0 100644
>>>> --- a/xen/arch/x86/mm/mem_access.c
>>>> +++ b/xen/arch/x86/mm/mem_access.c
>>>> @@ -262,35 +262,11 @@ int p2m_set_altp2m_mem_access(struct domain *d,
>>>> struct p2m_domain *hp2m,
>>>> mfn_t mfn;
>>>> p2m_type_t t;
>>>> p2m_access_t old_a;
>>>> - unsigned int page_order;
>>>> - unsigned long gfn_l = gfn_x(gfn);
>>>> int rc;
>>>>
>>>> - mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL);
>>>> -
>>>> - /* Check host p2m if no valid entry in alternate */
>>>> - if ( !mfn_valid(mfn) )
>>>> - {
>>>> -
>>>> - mfn = __get_gfn_type_access(hp2m, gfn_l, &t, &old_a,
>>>> - P2M_ALLOC | P2M_UNSHARE, &page_order,
>>>> 0);
>>>> -
>>>> - rc = -ESRCH;
>>>> - if ( !mfn_valid(mfn) || t != p2m_ram_rw )
>>>> - 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;
>>>> - }
>>>> - }
>>>> + rc = altp2m_get_entry_prepopulate(ap2m, gfn, &mfn, &t, &old_a);
>>>> + if ( rc )
>>>> + return rc;
>>>>
>>>> /*
>>>> * Inherit the old suppress #VE bit value if it is already set, or
>>>> set it
>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>>>> index 9e81a30cc4..7bedfd593b 100644
>>>> --- a/xen/arch/x86/mm/p2m.c
>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>
>>> Wouldn't it make more sense to start adding new altp2m functions to
>>> mm/altp2m.c instead? Probably the altp2m functions from mm/p2m.c could
>>> also be relocated there at some point in the future.
>>>
>>>> @@ -478,6 +478,43 @@ void p2m_unlock_and_tlb_flush(struct p2m_domain *p2m)
>>>> mm_write_unlock(&p2m->lock);
>>>> }
>>>>
>>>> +int altp2m_get_entry(struct p2m_domain *ap2m,
>>>> + gfn_t gfn, mfn_t *mfn, p2m_type_t *t,
>>>> + p2m_access_t *a, bool prepopulate)
>>>> +{
>>>> + *mfn = ap2m->get_entry(ap2m, gfn, t, a, 0, NULL, NULL);
>>>> +
>>>> + /* Check host p2m if no valid entry in alternate */
>>>> + if ( !mfn_valid(*mfn) && !p2m_is_hostp2m(ap2m) )
>>>> + {
>>>> + struct p2m_domain *hp2m = p2m_get_hostp2m(ap2m->domain);
>>>> + unsigned int page_order;
>>>> + int rc;
>>>> +
>>>> + *mfn = __get_gfn_type_access(hp2m, gfn_x(gfn), t, a,
>>>> + P2M_ALLOC | P2M_UNSHARE,
>>>> &page_order, 0);
>>>
>>> So despite the name being altp2m_get_entry you now return an entry
>>> from the hostp2m, even if prepopulate is false. If the caller knows it
>>> doesn't want that entry to be copied into the altp2m, why not have it
>>> call __get_gfn_type_access itself for the hostp2m? IMHO this is just
>>> confusing and doesn't help readability of the altp2m code.
>>
>> You return the ap2m entry if it's present, or the hp2m entry if it's
>> not. It's not a lot of duplication, but it makes the logic cleaner I
>> think; why not deduplicate it?
>
> I have no problem with making the code more streamlined. The problem I
> have is that the function's name doesn't suggest it would get you
> anything but the entry from the specified altp2m. So you could be
> reading the code assuming you are dealing with an entry from that
> specified table when in fact you are not. That is not an expected
> behavior based on just the name of the function. This is going to make
> reading the altp2m code that much harder in the future.
Right -- I wasn't a huge fan of 'direct' either; it didn't really convey
to me 100% what the function did. My PoC had "seethrough", but that
wasn't that great either. "Peek"? Any other suggestions?
Other options:
* If we have a single function with a #define, this might get a bit
easier; we could have one be AP2MGET_dont_prepopulate or something.
( We could have the "core" function named _altp2m_get_entry, and have
altp2m_get_entry() call with prepopulate = false, and
altp2m_get_entry_prepopulate() call it with prepopulate = true.
Thoughts?
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
