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

[Christian Limpach <christian.limpach@xxxxxxxxx>]

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);

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