Re: [Xen-devel] [PATCH 0 of 2] Add libxc API that sets mem_access type for an array of gfns

[Aravindh Puthiyaparambil <aravindh@xxxxxxxxxxxx>]

On Wed, May 2, 2012 at 8:28 PM, Christian Limpach <christian.limpach@xxxxxxxxx> wrote:
> On Fri, Apr 27, 2012 at 9:22 PM, Aravindh Puthiyaparambil
> <aravindh@xxxxxxxxxxxx> 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);

Sorry for not following and thanks for your patience. Hopefully this looks correct. I have just included the pertinent bit of the patch you were commenting on.

diff -r 9dda0efd8ce1 -r c180942992bf xen/arch/x86/mm/p2m-ept.c

--- a/xen/arch/x86/mm/p2m-ept.c Fri Apr 27 17:57:55 2012 +0200

+++ b/xen/arch/x86/mm/p2m-ept.c Fri May 04 14:56:12 2012 -0700

@@ -274,10 +274,13 @@ static int ept_next_level(struct p2m_dom

 /*

  * ept_set_entry() computes 'need_modify_vtd_table' for itself,

  * by observing whether any gfn->mfn translations are modified.

+ * If sync_deferred is not NULL, then the caller will take care of

+ * calling ept_sync_domain() in the cases where it can be deferred.

  */

 static int

 ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn, 

-              unsigned int order, p2m_type_t p2mt, p2m_access_t p2ma)

+              unsigned int order, p2m_type_t p2mt, p2m_access_t p2ma,

+              bool_t *sync_deferred)

 {

     ept_entry_t *table, *ept_entry = NULL;

     unsigned long gfn_remainder = gfn;

@@ -309,6 +312,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-- )

@@ -345,8 +351,12 @@ ept_set_entry(struct p2m_domain *p2m, un

         ept_entry_t new_entry = { .epte = 0 };

 

         /* No need to flush if the old entry wasn't valid */

-        if ( !is_epte_present(ept_entry) )

+        if ( !is_epte_present(ept_entry) || (!target && sync_deferred) )

+        {

             needs_sync = 0;

+            if ( sync_deferred )

+                *sync_deferred = 0;

+        }

 

         /* 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

@@ -477,7 +487,7 @@ out:

        last thing we do, after the ept_sync_domain() and removal

        from the iommu tables, so as to avoid a potential

        use-after-free. */

-    if ( is_epte_present(&old_entry) )

+    if ( target && is_epte_present(&old_entry) )

         ept_free_entry(p2m, &old_entry, target);

 

     return rv;

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