Re: [PATCHv7] x86/ept: defer the invalidation until the p2m lock is released

[David Vrabel <david.vrabel@citrix.com>]

On 03/02/16 03:44, Tian, Kevin wrote:
>> From: David Vrabel [mailto:david.vrabel@citrix.com]
>> Sent: Tuesday, February 02, 2016 12:27 AM

Looks like I forgot about this patch.

>> It is safe to defer the invalidate until the p2m lock is released
>> except for two cases:
>>
>> 1. When freeing a page table page (since partial translations may be
>>    cached).
>> 2. When reclaiming a zero page as part of PoD.
>>
>> For these cases, add p2m_tlb_flush_sync() calls which will immediately
>> perform the invalidate before the page is freed or reclaimed.
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> [...]
>> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
>> index c094320..43c7f1b 100644
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -263,6 +263,7 @@ static void ept_free_entry(struct p2m_domain *p2m, ept_entry_t
>> *ept_entry, int l
>>          unmap_domain_page(epte);
>>      }
>>
>> +    p2m_tlb_flush_sync(p2m);
>>      p2m_free_ptp(p2m, mfn_to_page(ept_entry->mfn));
>>  }
>>
>> @@ -1095,15 +1096,10 @@ static void __ept_sync_domain(void *info)
>>       */
>>  }
>>
>> -void ept_sync_domain(struct p2m_domain *p2m)
>> +static void ept_sync_domain_prepare(struct p2m_domain *p2m)
>>  {
>>      struct domain *d = p2m->domain;
>>      struct ept_data *ept = &p2m->ept;
>> -    /* Only if using EPT and this domain has some VCPUs to dirty. */
>> -    if ( !paging_mode_hap(d) || !d->vcpu || !d->vcpu[0] )
>> -        return;
>> -
>> -    ASSERT(local_irq_is_enabled());
>>
>>      if ( nestedhvm_enabled(d) && !p2m_is_nestedp2m(p2m) )
>>          p2m_flush_nestedp2m(d);
> 
> should we postpone nestedp2m flush similarly, which also incurs 
> on_selected_cpus when holding p2m lock?

Possibly.  I have not looked at the nestedp2m stuff as it wasn't a use
case I cared about.

I think any changes in this area could be done separately.

>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -325,6 +325,25 @@ void p2m_flush_hardware_cached_dirty(struct domain *d)
>>      }
>>  }
>>
>> +/*
>> + * Force a synchronous P2M TLB flush if a deferred flush is pending.
>> + *
>> + * Must be called with the p2m lock held.
>> + */
>> +void p2m_tlb_flush_sync(struct p2m_domain *p2m)
>> +{
>> +    if ( p2m->need_flush )
>> +        p2m->flush_and_unlock(p2m, 0);
>> +}
>> +
>> +void p2m_tlb_flush_and_unlock(struct p2m_domain *p2m)
>> +{
>> +    if ( p2m->need_flush )
>> +        p2m->flush_and_unlock(p2m, 1);
>> +    else
>> +        mm_write_unlock(&p2m->lock);
>> +}
> 
> prefer to move general stuff into this function, then you could just
> keep a flush() callback, e.g.:
> 
> void p2m_tlb_flush_and_unlock(struct p2m_domain *p2m)
> {
>     if ( p2m->need_flush )
>     {
>         p2m->need_flush = 0;
>         mm_write_unlock(&p2m->lock);
>         p2m->flush(p2m);
>     }
>     else
>         mm_write_unlock(&p2m->lock);
> }
> 
> Same for p2m_tlb_flush_sync.

I'm sure there was a reason why I did it like this but I can't remember.
 Let me try your suggestion.

David


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