On Wed, May 2, 2012 at 8:28 PM, Christian Limpach <christian.limpach@gmail.com> wrote:
> On Fri, Apr 27, 2012 at 9:22 PM, Aravindh Puthiyaparambil
> <aravindh@virtuata.com> wrote:
>>>> Let me re-iterate:
>>>> - if it's a leaf entry, then there's no need to call ept_free_entry
>>>> - if you don't need to call ept_free_entry, then you can defer
>>>> ept_sync_domain (subject to the iommu case)
>>>> - you could pass in a pointer to a bool to indicate to the caller that
>>>> ept_sync_domain was deferred and that the caller needs to do it, with
>>>> a NULL pointer indicating that the caller doesn't support defer
>>
>> How does this look?
>
> It's missing the "leaf entry" part of my description of how I think
> things should work. Without that, you're effectively ignoring the
> following comment at the end of ept_set_entry:
> /* Release the old intermediate tables, if any. This has to be the
> last thing we do, after the ept_sync_domain() and removal
> from the iommu tables, so as to avoid a potential
> use-after-free. */
>
> See inline comments in your patch below for how I'd change this, untested...
>
> christian
>
>> @@ -293,6 +296,7 @@ ept_set_entry(struct p2m_domain *p2m, un
>> int needs_sync = 1;
>> struct domain *d = p2m->domain;
>> ept_entry_t old_entry = { .epte = 0 };
>> + bool_t _sync_deferred = 0;
>
> don't need that
>
>> /*
>> * the caller must make sure:
>> @@ -309,6 +313,9 @@ ept_set_entry(struct p2m_domain *p2m, un
>> (target == 1 && hvm_hap_has_2mb(d)) ||
>> (target == 0));
>>
>> + if (sync_deferred)
>> + *sync_deferred = 1;
>> +
>> table = map_domain_page(ept_get_asr(d));
>>
>> for ( i = ept_get_wl(d); i > target; i-- )
>> @@ -346,7 +353,11 @@ ept_set_entry(struct p2m_domain *p2m, un
>>
>> /* No need to flush if the old entry wasn't valid */
>> if ( !is_epte_present(ept_entry) )
>> needs_sync = 0;
>
> This should be:
> if ( !is_epte_present(ept_entry) ||
> (!target && sync_deferred) ) {
> needs_sync = 0;
> if (sync_deferred)
> *sync_deferred = 0;
> }
>
> This expresses the notion that we're going to skip the call to
> ept_free_entry since it's a leaf entry (target == 0) and that the
> caller will make the ept_sync_domain call (sync_deferred != NULL).
>
>>
>> /* If we're replacing a non-leaf entry with a leaf entry
>> (1GiB or 2MiB),
>> * the intermediate tables will be freed below after the ept flush
>> @@ -385,6 +396,9 @@ ept_set_entry(struct p2m_domain *p2m, un
>>
>> ASSERT(is_epte_superpage(ept_entry));
>>
>> + if ( sync_deferred )
>> + _sync_deferred = 1;
>> +
>
> don't need that
>
>> split_ept_entry = atomic_read_ept_entry(ept_entry);
>>
>> if ( !ept_split_super_page(p2m, &split_ept_entry, i, target) )
>> @@ -438,7 +452,7 @@ ept_set_entry(struct p2m_domain *p2m, un
>> out:
>> unmap_domain_page(table);
>>
>> - if ( needs_sync )
>> + if ( needs_sync && !_sync_deferred )
>> ept_sync_domain(p2m->domain);
>
> don't need that change, test on needs_sync is sufficient
>
>>
>> if ( rv && iommu_enabled && need_iommu(p2m->domain) &&
>> need_modify_vtd_table )
>
> Then at the end of ept_set_pte add the test for non-leaf, which is
> more like an optimization/clarification since ept_free_entry has the
> same test already.
>
> if ( target && is_epte_present(&old_entry) )
> ept_free_entry(p2m, &old_entry, target);