[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: Tue, 16 Apr 2019 16:07:47 +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: Tue, 16 Apr 2019 15:08:15 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Openpgp: preference=signencrypt

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.

Thoughts?

 -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®.