Re: [Xen-devel] [PATCH v5] altp2m: Allow shared entries to be copied to altp2m views during lazycopy

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Mon, Jul 25, 2016 at 9:58 AM, George Dunlap <george.dunlap@xxxxxxxxxx> wrote:
> On Wed, Jul 20, 2016 at 7:55 PM, Tamas K Lengyel
> <tamas.lengyel@xxxxxxxxxxxx> wrote:
>> Move sharing locks above altp2m to avoid locking order violation and crashing
>> the hypervisor during unsharing operations when altp2m is active.
>>
>> Applying mem_access settings or remapping gfns in altp2m views will
>> automatically unshare the page if it was shared previously. Also,
>> disallow nominating pages for which there are pre-existing altp2m
>> mem_access settings or remappings present. However, allow altp2m
>> to populate altp2m views with shared entries during lazycopy as
>> unsharing will automatically propagate the change to these entries in
>> altp2m views as well.
>>
>> Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>
>
> This looks a lot better.  One comment...
>
>> ---
>> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>
>> v5: Allow only lazycopy to copy shared entries to altp2m views
>>     Use get_gfn_type_access for unsharing as get_entry doesn't do that
>> ---
>>  xen/arch/x86/mm/mem_sharing.c | 25 ++++++++++++++++++++++++-
>>  xen/arch/x86/mm/mm-locks.h    | 30 +++++++++++++++---------------
>>  xen/arch/x86/mm/p2m.c         | 12 +++++++-----
>>  3 files changed, 46 insertions(+), 21 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
>> index a522423..3939cd0 100644
>> --- a/xen/arch/x86/mm/mem_sharing.c
>> +++ b/xen/arch/x86/mm/mem_sharing.c
>> @@ -33,6 +33,7 @@
>>  #include <asm/page.h>
>>  #include <asm/string.h>
>>  #include <asm/p2m.h>
>> +#include <asm/altp2m.h>
>>  #include <asm/atomic.h>
>>  #include <asm/event.h>
>>  #include <xsm/xsm.h>
>> @@ -828,20 +829,42 @@ int mem_sharing_nominate_page(struct domain *d,
>>                                int expected_refcnt,
>>                                shr_handle_t *phandle)
>>  {
>> +    struct p2m_domain *hp2m = p2m_get_hostp2m(d);
>>      p2m_type_t p2mt;
>> +    p2m_access_t p2ma;
>>      mfn_t mfn;
>>      struct page_info *page = NULL; /* gcc... */
>>      int ret;
>>
>>      *phandle = 0UL;
>>
>> -    mfn = get_gfn(d, gfn, &p2mt);
>> +    mfn = get_gfn_type_access(hp2m, gfn, &p2mt, &p2ma, 0, NULL);
>>
>>      /* Check if mfn is valid */
>>      ret = -EINVAL;
>>      if ( !mfn_valid(mfn) )
>>          goto out;
>>
>> +    /* Check if there are mem_access/remapped altp2m entries for this page 
>> */
>> +    if ( altp2m_active(d) )
>> +    {
>> +        unsigned int i;
>> +        struct p2m_domain *ap2m;
>> +        mfn_t amfn;
>> +        p2m_access_t ap2ma;
>> +
>> +        for ( i = 0; i < MAX_ALTP2M; i++ )
>> +        {
>> +            ap2m = d->arch.altp2m_p2m[i];
>> +            if ( !ap2m )
>> +                continue;
>> +
>> +            amfn = get_gfn_type_access(ap2m, gfn, NULL, &ap2ma, 0, NULL);
>> +            if ( mfn_valid(amfn) && (mfn_x(amfn) != mfn_x(mfn) || ap2ma != 
>> p2ma) )
>> +                goto out;
>> +        }
>
> You need to altp2m_list_[un]lock(d) around this loop, don't you?

Ah yes, that's right.

>
> Everything else looks great, thanks.

Awesome!

Thanks,
Tamas

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