Re: [PATCH v4 2/2] xen: move TLB-flush filtering out into populate_physmap during vm creation

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4444 bytes --]

On Mon, 2016-09-12 at 16:16 +0800, Dongli Zhang wrote:
> This patch implemented parts of TODO left in commit id
> a902c12ee45fc9389eb8fe54eeddaf267a555c58. 
>
We usually put both the (not necessarily full) hash and the subject
line of the commit in here.

> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index a8804e4..7be1bee 100644
> @@ -303,6 +303,8 @@ struct domain *domain_create(domid_t domid,
> unsigned int domcr_flags,
>      if ( !zalloc_cpumask_var(&d->domain_dirty_cpumask) )
>          goto fail;
>  
> +    d->is_ever_unpaused = false;
> +
>
I'd go for something like "first_unpaused" or "creation_finished", but
if maintainers are happy with this one already, I'm fine too.

> @@ -1004,6 +1006,15 @@ int domain_unpause_by_systemcontroller(struct
> domain *d)
>  {
>      int old, new, prev = d->controller_pause_count;
>  
> +    /*
> +     * Set is_ever_unpaused to true when this domain gets unpaused
> for the
> +     * first time. We record this information here to help
> populate_physmap
> +     * verify whether the domain has ever been unpaused.
> MEMF_no_tlbflush
> +     * is allowed to be set by populate_physmap only during vm
> creation.
> +     */

"We record this information here for populate_physmap to figure out
 that the domain has already been unpaused, after finishing being
 created. That's because we're allowed to set MEMF_no_tlbflush only
 during VM creation."

Or, de-focusing the unpausing even more:

"We record this information here for populate_physmap to figure out
 tha
t the domain has finished being created. In fact, we're only
 allowed to
set the MEMF_no_tlbflush flag during VM creation."

I.e., the important thing is not really the unpausing (that's where we
found it handy to put the check), it's the fact that something should
only happen at creation time and why (see below).

> +    if ( unlikely(!d->is_ever_unpaused) )
> +        d->is_ever_unpaused = true;
> +
>      do
>      {
>          old = prev;

> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index cc0f69e..f3a733b 100644
> @@ -150,6 +152,14 @@ static void populate_physmap(struct memop_args
> *a)
>                              max_order(curr_d)) )
>          return;
>  
> +    /*
> +     * MEMF_no_tlbflush can be set only during vm creation phase
> when
> +     * is_ever_unpaused is still false before this domain gets
> unpaused for
> +     * the first time.
> +     */
>
What about, 'citing' from the changelog:

"With MEMF_no_tlbflush set, alloc_heap_pages() will ignore TLB-
 flushes. After VM creation, this is a security issue (it can make
 pages accessible to guest B, when guest A may still have a cached
 mapping to them). So we only do this only during domain creation,
 when the domain itself has not yet been unpaused for the first
 time."

> +    if ( unlikely(!d->is_ever_unpaused) )
> +        a->memflags |= MEMF_no_tlbflush;
> +
>      for ( i = a->nr_done; i < a->nr_extents; i++ )
>      {
>          if ( i != a->nr_done && hypercall_preempt_check() )

> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 2f9c15f..7fe8841 100644
> @@ -474,6 +474,9 @@ struct domain
>          unsigned int guest_request_enabled       : 1;
>          unsigned int guest_request_sync          : 1;
>      } monitor;
> +
> +    /* set to true the first time this domain gets unpaused. */
>
I think it's relevant to say _when_ that is. What about:

/*
 * Set to true at the very end of domain creation, when the domain is 
 * unpaused for the first time by the systemcontroller.
 */

(not 100% happy about the "by the systemcontroller" part... but that's
the idea.)

> +    bool_t is_ever_unpaused;
>
As said by Jan already --here and elsewhere-- new code should use
'bool'.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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