Re: [PATCH] altp2m: Allow the hostp2m to be shared

[George Dunlap <george.dunlap@citrix.com>]

On 27/04/16 16:37, Tamas K Lengyel wrote:
> On Wed, Apr 27, 2016 at 9:31 AM, George Dunlap <george.dunlap@citrix.com>
> wrote:
> 
>> On 27/04/16 16:18, Tamas K Lengyel wrote:
>>> On Wed, Apr 27, 2016 at 9:01 AM, George Dunlap <george.dunlap@citrix.com
>>>
>>> wrote:
>>>
>>>> On 21/04/16 18:10, Tamas K Lengyel wrote:
>>>>> Don't propagate altp2m changes from ept_set_entry for memshare as
>>>> memshare
>>>>> already has the lock. We call altp2m propagate changes once memshare
>>>>> successfully finishes. Also, allow the hostp2m entries to be of type
>>>>> p2m_ram_shared.
>>>>>
>>>>> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
>>>>
>>>> Sorry for the delay in reviewing -- trying to get my head back around
>>>> the altp2m code.  On the whole looks reasonable, but one question...
>>>>
>>>>> ---
>>>>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
>>>>> Cc: Keir Fraser <keir@xen.org>
>>>>> Cc: Jan Beulich <jbeulich@suse.com>
>>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> Cc: Jun Nakajima <jun.nakajima@intel.com>
>>>>> Cc: Kevin Tian <kevin.tian@intel.com>
>>>>> ---
>>>>>  xen/arch/x86/mm/mem_sharing.c | 11 +++++++++++
>>>>>  xen/arch/x86/mm/p2m-ept.c     |  2 +-
>>>>>  xen/arch/x86/mm/p2m.c         |  7 +++----
>>>>>  3 files changed, 15 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/mm/mem_sharing.c
>>>> b/xen/arch/x86/mm/mem_sharing.c
>>>>> index a522423..d5b4b2d 100644
>>>>> --- a/xen/arch/x86/mm/mem_sharing.c
>>>>> +++ b/xen/arch/x86/mm/mem_sharing.c
>>>>> @@ -35,6 +35,7 @@
>>>>>  #include <asm/p2m.h>
>>>>>  #include <asm/atomic.h>
>>>>>  #include <asm/event.h>
>>>>> +#include <asm/altp2m.h>
>>>>>  #include <xsm/xsm.h>
>>>>>
>>>>>  #include "mm-locks.h"
>>>>> @@ -1026,6 +1027,16 @@ int mem_sharing_share_pages(struct domain *sd,
>>>> unsigned long sgfn, shr_handle_t
>>>>>      /* We managed to free a domain page. */
>>>>>      atomic_dec(&nr_shared_mfns);
>>>>>      atomic_inc(&nr_saved_mfns);
>>>>> +
>>>>> +    if( altp2m_active(cd) )
>>>>> +    {
>>>>> +        p2m_access_t a;
>>>>> +        struct p2m_domain *p2m = p2m_get_hostp2m(cd);
>>>>> +        p2m->get_entry(p2m, cgfn, NULL, &a, 0, NULL, NULL);
>>>>> +        p2m_altp2m_propagate_change(cd, _gfn(cgfn), smfn,
>> PAGE_ORDER_4K,
>>>>> +                                    p2m_ram_shared, a);
>>>>> +    }
>>>>> +
>>>>>      ret = 0;
>>>>>
>>>>>  err_out:
>>>>> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
>>>>> index 3cb6868..1ac3018 100644
>>>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>>>> @@ -846,7 +846,7 @@ out:
>>>>>      if ( is_epte_present(&old_entry) )
>>>>>          ept_free_entry(p2m, &old_entry, target);
>>>>>
>>>>> -    if ( rc == 0 && p2m_is_hostp2m(p2m) )
>>>>> +    if ( rc == 0 && p2m_is_hostp2m(p2m) && p2mt != p2m_ram_shared )
>>>>>          p2m_altp2m_propagate_change(d, _gfn(gfn), mfn, order, p2mt,
>>>> p2ma);
>>>>>
>>>>>      return rc;
>>>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>>>>> index b3fce1b..d2aebf7 100644
>>>>> --- a/xen/arch/x86/mm/p2m.c
>>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>>> @@ -1739,11 +1739,10 @@ int p2m_set_altp2m_mem_access(struct domain *d,
>>>> struct p2m_domain *hp2m,
>>>>>      /* Check host p2m if no valid entry in alternate */
>>>>>      if ( !mfn_valid(mfn) )
>>>>>      {
>>>>> -        mfn = hp2m->get_entry(hp2m, gfn_l, &t, &old_a,
>>>>> -                              P2M_ALLOC | P2M_UNSHARE, &page_order,
>>>> NULL);
>>>>> +        mfn = hp2m->get_entry(hp2m, gfn_l, &t, &old_a, 0, &page_order,
>>>> NULL);
>>>>
>>>> Why are you getting rid of P2M_ALLOC here?  What happens if the hp2m
>>>> entry is populate-on-demand?
>>>>
>>>
>>> There is a check further down here that only allows p2m_ram_rw and
>>> p2m_ram_shared.
>>
>> So what P2M_ALLOC means is, "If this is entry is PoD, then please
>> populate it so I get a ram page."  So the only way you can get a
>> p2m_populate_on_demand type returned is if you remove this flag.  Leave
>> it and (assuming there's enough ram to go around), you'll always get
>> p2m_ram_rw.  :-)
>>
>>> On the non-altp2m path mem_access doesn't request P2M_ALLOC
>>> either (but doesn't check the type), so I would say mem_access is not
>>> compatible with PoD.
>>
>> Off the top of my head I can't see a reason why they couldn't co-exist
>> in principle, if you added P2M_ALLOC in a few key places.
>>
> 
> Sure, I just rather do that in a separate patch and for now have the
> mem_access paths behaving the same way before doing that adjustment.

Ok, in that case please add a few sentences in the changelog addressing
the change.

Thanks,
 -George


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel