[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v5] x86/altp2m: Aggregate get entry and populate into common funcs


  • To: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
  • From: George Dunlap <george.dunlap@xxxxxxxxxx>
  • Date: Thu, 18 Apr 2019 18:02:51 +0100
  • Autocrypt: addr=george.dunlap@xxxxxxxxxx; prefer-encrypt=mutual; keydata= mQINBFPqG+MBEACwPYTQpHepyshcufo0dVmqxDo917iWPslB8lauFxVf4WZtGvQSsKStHJSj 92Qkxp4CH2DwudI8qpVbnWCXsZxodDWac9c3PordLwz5/XL41LevEoM3NWRm5TNgJ3ckPA+J K5OfSK04QtmwSHFP3G/SXDJpGs+oDJgASta2AOl9vPV+t3xG6xyfa2NMGn9wmEvvVMD44Z7R W3RhZPn/NEZ5gaJhIUMgTChGwwWDOX0YPY19vcy5fT4bTIxvoZsLOkLSGoZb/jHIzkAAznug Q7PPeZJ1kXpbW9EHHaUHiCD9C87dMyty0N3TmWfp0VvBCaw32yFtM9jUgB7UVneoZUMUKeHA fgIXhJ7I7JFmw3J0PjGLxCLHf2Q5JOD8jeEXpdxugqF7B/fWYYmyIgwKutiGZeoPhl9c/7RE Bf6f9Qv4AtQoJwtLw6+5pDXsTD5q/GwhPjt7ohF7aQZTMMHhZuS52/izKhDzIufl6uiqUBge 0lqG+/ViLKwCkxHDREuSUTtfjRc9/AoAt2V2HOfgKORSCjFC1eI0+8UMxlfdq2z1AAchinU0 eSkRpX2An3CPEjgGFmu2Je4a/R/Kd6nGU8AFaE8ta0oq5BSFDRYdcKchw4TSxetkG6iUtqOO ZFS7VAdF00eqFJNQpi6IUQryhnrOByw+zSobqlOPUO7XC5fjnwARAQABtCRHZW9yZ2UgVy4g RHVubGFwIDxkdW5sYXBnQHVtaWNoLmVkdT6JAlcEEwEKAEECGwMFCwkIBwMFFQoJCAsFFgID AQACHgECF4ACGQEWIQTXqBy2bTNXPzpOYFimNjwxBZC0bQUCXEowWQUJDCJ7dgAKCRCmNjwx BZC0beKvEACJ75YlJXd7TnNHgFyiCJkm/qPeoQ3sFGSDZuZh7SKcdt9+3V2bFEb0Mii1hQaz 3hRqZb8sYPHJrGP0ljK09k3wf8k3OuNxziLQBJyzvn7WNlE4wBEcy/Ejo9TVBdA4ph5D0YaZ nqdsPmxe/xlTFuSkgu4ep1v9dfVP1TQR0e+JIBa/Ss+cKC5intKm+8JxpOploAHuzaPu0L/X FapzsIXqgT9eIQeBEgO2hge6h9Jov3WeED/vh8kA7f8c6zQ/gs5E7VGALwsiLrhr0LZFcKcw kI3oCCrB/C/wyPZv789Ra8EXbeRSJmTjcnBwHRPjnjwQmetRDD1t+VyrkC6uujT5jmgOBzaj KCqZ8PcMAssOzdzQtKmjUQ2b3ICPs2X13xZ5M5/OVs1W3TG5gkvMh4YoHi4ilFnOk+v3/j7q 65FG6N0JLb94Ndi80HkIOQQ1XVGTyu6bUPaBg3rWK91Csp1682kD/dNVF3FKHrRLmSVtmEQR 5rK0+VGc/FmR6vd4haKGWIRuPxzg+pBR77avIZpU7C7+UXGuZ5CbHwIdY8LojJg2TuUdqaVj yxmEZLOA8rVHipCGrslRNthVbJrGN/pqtKjCClFZHIAYJQ9EGLHXLG9Pj76opfjHij3MpR3o pCGAh6KsCrfrsvjnpDwqSbngGyEVH030irSk4SwIqZ7FwLkBDQRUWmc6AQgAzpc8Ng5Opbrh iZrn69Xr3js28p+b4a+0BOvC48NfrNovZw4eFeKIzmI/t6EkJkSqBIxobWRpBkwGweENsqnd 0qigmsDw4N7J9Xx0h9ARDqiWxX4jr7u9xauI+CRJ1rBNO3VV30QdACwQ4LqhR/WA+IjdhyMH wj3EJGE61NdP/h0zfaLYAbvEg47/TPThFsm4m8Rd6bX7RkrrOgBbL/AOnYOMEivyfZZKX1vv iEemAvLfdk2lZt7Vm6X/fbKbV8tPUuZELzNedJvTTBS3/l1FVz9OUcLDeWhGEdlxqXH0sYWh E9+PXTAfz5JxKH+LMetwEM8DbuOoDIpmIGZKrZ+2fQARAQABiQNbBBgBCgAmAhsCFiEE16gc tm0zVz86TmBYpjY8MQWQtG0FAlxKMJ4FCQnQ/OQBKcBdIAQZAQoABgUCVFpnOgAKCRCyFcen x4Qb7cXrCAC0qQeEWmLa9oEAPa+5U6wvG1t/mi22gZN6uzQXH1faIOoDehr7PPESE6tuR/vI CTTnaSrd4UDPNeqOqVF07YexWD1LDcQG6PnRqC5DIX1RGE3BaSaMl2pFJP8y+chews11yP8G DBbxaIsTcHZI1iVIC9XLhoeegWi84vYc8F4ziADVfowbmbvcVw11gE8tmALCwTeBeZVteXjh 0OELHwrc1/4j4yvENjIXRO+QLIgk43kB57Upr4tP2MEcs0odgPM+Q+oETOJ00xzLgkTnLPim C1FIW2bOZdTj+Uq6ezRS2LKsNmW+PRRvNyA5ojEbA/faxmAjMZtLdSSSeFK8y4SoCRCmNjwx BZC0bevWEACRu+GyQgrdGmorUptniIeO1jQlpTiP5WpVnk9Oe8SiLoXUhXXNj6EtzyLGpYmf kEAbki+S6WAKnzZd3shL58AuMyDxtFNNjNeKJOcl6FL7JPBIIgIp3wR401Ep+/s5pl3Nw8Ii 157f0T7o8CPb54w6S1WsMkU78WzTxIs/1lLblSMcvyz1Jq64g4OqiWI85JfkzPLlloVf1rzy ebIBLrrmjhCE2tL1RONpE/KRVb+Q+PIs5+YcZ+Q1e0vXWA7NhTWFbWx3+N6WW6gaGpbFbopo FkYRpj+2TA5cX5zW148/xU5/ATEb5vdUkFLUFVy5YNUSyeBHuaf6fGmBrDc47rQjAOt1rmyD 56MUBHpLUbvA6NkPezb7T6bQpupyzGRkMUmSwHiLyQNJQhVe+9NiJJvtEE3jol0JVJoQ9WVn FAzPNCgHQyvbsIF3gYkCYKI0w8EhEoH5FHYLoKS6Jg880IY5rXzoAEfPvLXegy6mhYl+mNVN QUBD4h9XtOvcdzR559lZuC0Ksy7Xqw3BMolmKsRO3gWKhXSna3zKl4UuheyZtubVWoNWP/bn vbyiYnLwuiKDfNAinEWERC8nPKlv3PkZw5d3t46F1Dx0TMf16NmP+azsRpnMZyzpY8BL2eur feSGAOB9qjZNyzbo5nEKHldKWCKE7Ye0EPEjECS1gjKDwbkBDQRUWrq9AQgA7aJ0i1pQSmUR 6ZXZD2YEDxia2ByR0uZoTS7N0NYv1OjU8v6p017u0Fco5+Qoju/fZ97ScHhp5xGVAk5kxZBF DT4ovJd0nIeSr3bbWwfNzGx1waztfdzXt6n3MBKr7AhioB1m+vuk31redUdnhbtvN7O40MC+ fgSk5/+jRGxY3IOVPooQKzUO7M51GoOg4wl9ia3H2EzOoGhN2vpTbT8qCcL92ZZZwkBRldoA Wn7c1hEKSTuT3f1VpSmhjnX0J4uvKZ1V2R7rooKJYFBcySC0wa8aTmAtAvLgfcpe+legOtgq DKzLuN45xzEjyjCiI521t8zxNMPJY9FiCPNv0sCkDwARAQABiQI8BBgBCgAmAhsMFiEE16gc tm0zVz86TmBYpjY8MQWQtG0FAlxKNJYFCQnQrVkACgkQpjY8MQWQtG2Xxg//RrRP+PFYuNXt 9C5hec/JoY24TkGPPd2tMC9usWZVImIk7VlHlAeqHeE0lWU0LRGIvOBITbS9izw6fOVQBvCA Fni56S12fKLusWgWhgu03toT9ZGxZ9W22yfw5uThSHQ4y09wRWAIYvhJsKnPGGC2KDxFvtz5 4pYYNe8Icy4bwsxcgbaSFaRh+mYtts6wE9VzyJvyfTqbe8VrvE+3InG5rrlNn51AO6M4Wv20 iFEgYanJXfhicl0WCQrHyTLfdB5p1w+072CL8uryHQVfD0FcDe+J/wl3bmYze+aD1SlPzFoI MaSIXKejC6oh6DAT4rvU8kMAbX90T834Mvbc3jplaWorNJEwjAH/r+v877AI9Vsmptis+rni JwUissjRbcdlkKBisoUZRPmxQeUifxUpqgulZcYwbEC/a49+WvbaYUriaDLHzg9xisijHwD2 yWV8igBeg+cmwnk0mPz8tIVvwi4lICAgXob7HZiaqKnwaDXs4LiS4vdG5s/ElnE3rIc87yru 24n3ypeDZ6f5LkdqL1UNp5/0Aqbr3EiN7/ina4YVyscy9754l944kyHnnMRLVykg0v+kakj0 h0RJ5LbfLAMM8M52KIA3y14g0Fb7kHLcOUMVcgfQ3PrN6chtC+5l6ouDIlSLR3toxH8Aam7E rIFfe2Dk+lD9A9BVd2rfoHA=
  • Cc: "wei.liu2@xxxxxxxxxx" <wei.liu2@xxxxxxxxxx>, "rcojocaru@xxxxxxxxxxxxxxx" <rcojocaru@xxxxxxxxxxxxxxx>, "george.dunlap@xxxxxxxxxxxxx" <george.dunlap@xxxxxxxxxxxxx>, "andrew.cooper3@xxxxxxxxxx" <andrew.cooper3@xxxxxxxxxx>, "jbeulich@xxxxxxxx" <jbeulich@xxxxxxxx>, Alexandru Stefan ISAILA <aisaila@xxxxxxxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "roger.pau@xxxxxxxxxx" <roger.pau@xxxxxxxxxx>
  • Delivery-date: Thu, 18 Apr 2019 17:03:05 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Openpgp: preference=signencrypt

On 4/18/19 2:59 PM, Tamas K Lengyel wrote:
> On Thu, Apr 18, 2019 at 3:53 AM George Dunlap <george.dunlap@xxxxxxxxxx> 
> wrote:
>>
>> On 4/17/19 7:22 PM, Tamas K Lengyel wrote:
>>> On Wed, Apr 17, 2019 at 1:15 AM Alexandru Stefan ISAILA
>>> <aisaila@xxxxxxxxxxxxxxx> wrote:
>>>>
>>>>
>>>>
>>>> On 16.04.2019 18:07, George Dunlap wrote:
>>>>> On 4/16/19 3:19 PM, Tamas K Lengyel wrote:
>>>>>> On Tue, Apr 16, 2019 at 8:02 AM George Dunlap <george.dunlap@xxxxxxxxxx> 
>>>>>> wrote:
>>>>>>>
>>>>>>> On 4/16/19 2:44 PM, Tamas K Lengyel wrote:
>>>>>>>> On Tue, Apr 16, 2019 at 2:45 AM Alexandru Stefan ISAILA
>>>>>>>> <aisaila@xxxxxxxxxxxxxxx> 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 <aisaila@xxxxxxxxxxxxxxx>
>>>>>>>>> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
>>>>>>>>> Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxx>
>>>>>>>>>
>>>>>>>>> ---
>>>>>>>>> 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.
>>>>
>>>> This option with no defines seems to solve more of the naming problems
>>>> but it will still introduce the spaghetti code. I vote for this one and
>>>> if Tamas agrees I will have it this way in the next version.
>>>>
>>>
>>> Having altp2m_get_entry and altp2m_get_entry_prepopulate seem to be a
>>> better name for them, as long as altp2m_get_entry doesn't return an
>>> entry from the hostp2m if there isn't one in the altp2m, and
>>> altp2m_get_entry_prepopulate returns an entry only if prepopulation
>>> actually worked. In both of those cases the functions would only
>>> return entries from the altp2m, as their name actually suggests.
>>
>> You seem to have missed the whole point of this patch then.
> 
> Forgive me but then I don't see anywhere in the patch description that
> explain why these functions _have to_ perform a fall-back and return
> an entry from the hostp2m at all cost.

The primary effect of this patch is to move duplicated code into a
single common function.  The code being de-duplicated:
 1. Tries to read the altp2m entry; if it's there it uses it
 2. If it's not there, it tries to read the host p2m entry
 3. In most cases it then propagates the hostp2m entry to the altp2m entry.

Obviously the new "common" function has to do it because that's what the
original code does.  The original code does it because that's what
altp2m is -- a "patch" over the host p2m, such that you use the altp2m
if entries are present, but use the hostp2m otherwise.

>> Instead of saying, "I don't like these names" (but not offering
>> alternative), or saying, "If you use these names, the functions have to
>> do the exact opposite of what they do in this patch", it would be more
>> constructive if you proposed names which you would prefer for the
>> functionality actually in this patch.
>>
> 
> I'm not the maintainer of this code so feel free to ignore my
> comments. I just see way too many functions in Xen that are "do_x()"
> but in in fact turn out to be "do_x_and_y_and_z()" which does not help
> readability or even really understanding what is happening. I guess at
> least adding comments describing these additional and sometimes
> unexpected behaviors would be an improvement.

You are a maintainer for mem_access.c, which has a non-trivial change in
this patch.  It can go in with Razvan's ack, but not while you have open
objections.

I feel your pain with function naming; I've been digging through
x86/mm.c recently and the function names are unnecessarily confusing.  I
also agree that "altp2m_get_entry" isn't terribly informative (although
it's a bit more obvious if you know how altp2m is meant to work).  I'm
just trying to make sure that there's a clear way for Alexandru to move
this patch forward.  I don't mind trying to come up with a better name,
but the patch shouldn't be blocked if we can't.

I agree that the function should have a comment that describes its purpose.

What about "altp2m_resolve_entry()"?  "altp2m_get_effective_entry"?

 -George

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.